Implement video frame acknowledgements in the chromoting protocol.

remoting/protocol/client_video_dispatcher_unittest.cc

Index: remoting/protocol/client_video_dispatcher_unittest.cc
diff --git a/remoting/protocol/client_video_dispatcher_unittest.cc b/remoting/protocol/client_video_dispatcher_unittest.cc
new file mode 100644
index 0000000000000000000000000000000000000000..7a1a01f44fadc22196efa140383c81bc51ca9842
--- /dev/null
+++ b/remoting/protocol/client_video_dispatcher_unittest.cc
@@ -0,0 +1,221 @@
+// Copyright 2015 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "remoting/protocol/client_video_dispatcher.h"
+
+#include "base/bind.h"
+#include "base/message_loop/message_loop.h"
+#include "remoting/base/constants.h"
+#include "remoting/proto/video.pb.h"
+#include "remoting/protocol/fake_session.h"
+#include "remoting/protocol/fake_stream_socket.h"
+#include "remoting/protocol/message_serialization.h"
+#include "remoting/protocol/video_stub.h"
+#include "testing/gmock/include/gmock/gmock.h"
+
+using testing::_;
+
+namespace remoting {
+namespace protocol {
+
+class MockChannelEventHandler : public ChannelDispatcherBase::EventHandler {
+ public:
+  MockChannelEventHandler() {}
+  ~MockChannelEventHandler() override {}
+
+  MOCK_METHOD1(OnChannelInitialized,
+               void(ChannelDispatcherBase* channel_dispatcher));
+  MOCK_METHOD2(OnChannelError,
+               void(ChannelDispatcherBase* channel_dispatcher,
+                    ErrorCode error));
+};
+
+class ClientVideoDispatcherTest : public testing::Test, public VideoStub {
+ public:
+  ClientVideoDispatcherTest()
+      : dispatcher_(this),
+        parser_(base::Bind(&ClientVideoDispatcherTest::OnVideoAck,
+                           base::Unretained(this)),
+                &reader_) {}
+
+  void Initialize(int version) {

[Wez, 2015/01/21 01:35:39]

nit: InitializeWithProtocolVersion?

[Sergey Ulanov, 2015/01/29 01:33:28]

Done.

+    EXPECT_CALL(event_handler_, OnChannelInitialized(_));

[Wez, 2015/01/21 01:35:39]

Having this expectation in here immediately followed by code that calls it means
that the caller has to know to create any _other_ expectations before the
Initialize(). Consider moving the |event_handler_| expectation into SetUp()
instead?

Also, consider having a Times(0) expectation for OnChannelError()?

[Sergey Ulanov, 2015/01/29 01:33:28]

Done.

+    dispatcher_.Init(&session_,
+                     ChannelConfig(ChannelConfig::TRANSPORT_MUX_STREAM, version,
+                                   ChannelConfig::CODEC_UNDEFINED),
+                     &event_handler_);
+    message_loop_.RunUntilIdle();
+    host_socket_.PairWith(
+        session_.fake_channel_factory().GetFakeChannel(kVideoChannelName));
+    reader_.StartReading(&host_socket_);
+    writer_.Init(&host_socket_, BufferedSocketWriter::WriteFailedCallback());
+  }
+
+  // VideoStub interface.
+  void ProcessVideoPacket(scoped_ptr<VideoPacket> video_packet,
+                          const base::Closure& done) {

[Wez, 2015/01/21 01:35:39]

Why is ChannelEventHandler a mock but VideoStub a base-class?

[Sergey Ulanov, 2015/01/29 01:33:29]

Removed MockChannelEventHandler.

+    video_packet_ = video_packet.Pass();
+    packet_processed_closure_ = done;
+  }
+
+ protected:
+  void OnVideoAck(scoped_ptr<VideoAck> ack, const base::Closure& done) {
+    ack_message_ = ack.Pass();
+    done.Run();
+  }
+
+  base::MessageLoop message_loop_;
+
+  MockChannelEventHandler event_handler_;
+  ClientVideoDispatcher dispatcher_;
+  FakeSession session_;
+  FakeStreamSocket host_socket_;
+  MessageReader reader_;
+  ProtobufMessageParser<VideoAck> parser_;
+  BufferedSocketWriter writer_;

[Wez, 2015/01/21 01:35:39]

nit: Can you break these up a bit to separate out the object-under-test from the
mock host and client parts, respectively?

[Sergey Ulanov, 2015/01/29 01:33:29]

Done.

+
+  scoped_ptr<VideoPacket> video_packet_;
+  base::Closure packet_processed_closure_;
+
+  scoped_ptr<VideoAck> ack_message_;
+};
+
+// Verify that acks are not sent for older versions of the protocol.
+TEST_F(ClientVideoDispatcherTest, WithoutAcks) {

[Wez, 2015/01/21 01:35:39]

nit: Add a basic test to verify just that parsed VideoPackets reach the
VideoStub?

[Sergey Ulanov, 2015/01/29 01:33:28]

This test already verifies it. Updated the comment.

+  int kTestFrameId = 3;
+
+  Initialize(kDefaultStreamVersion);
+
+  VideoPacket packet;
+  packet.set_data(std::string());
+  packet.set_frame_id(kTestFrameId);
+
+  // Send a VideoPacket and verify that the client receives it.
+  writer_.Write(SerializeAndFrameMessage(packet), base::Closure());
+  message_loop_.RunUntilIdle();

[Wez, 2015/01/21 01:35:39]

IIUC the New Way is:

RunLoop().RunUntilIdle()

for situations like this.

[Sergey Ulanov, 2015/01/29 01:33:28]

Done.

+  ASSERT_TRUE(video_packet_);

[Wez, 2015/01/21 01:35:39]

EXPECT_TRUE

[Sergey Ulanov, 2015/01/29 01:33:28]

Done.

+
+  packet_processed_closure_.Run();
+  message_loop_.RunUntilIdle();
+
+  // Ack should never be sent for this version of the protocol.
+  ASSERT_FALSE(ack_message_);

[Wez, 2015/01/21 01:35:39]

EXPECT_FALSE

[Sergey Ulanov, 2015/01/29 01:33:29]

Done.

+}
+
+// Verifies that the dispatchers sends Ack message with correct rendering delay.

[Wez, 2015/01/21 01:35:39]

s/dispatchers/dispatcher

[Sergey Ulanov, 2015/01/29 01:33:28]

Done.

+TEST_F(ClientVideoDispatcherTest, WithAcks) {
+  int kTestFrameId = 3;
+
+  Initialize(kVideoWithAckStreamVersion);
+
+  VideoPacket packet;
+  packet.set_data(std::string());
+  packet.set_frame_id(kTestFrameId);
+
+  // Send a VideoPacket and verify that the client receives it.
+  base::TimeTicks write_started_time = base::TimeTicks::Now();
+  writer_.Write(SerializeAndFrameMessage(packet), base::Closure());
+  message_loop_.RunUntilIdle();
+  ASSERT_TRUE(video_packet_);

[Wez, 2015/01/21 01:35:39]

EXPECT_TRUE

[Sergey Ulanov, 2015/01/29 01:33:28]

Done.

+
+  // Ack should only be sent after the packet is processed.
+  ASSERT_FALSE(ack_message_);
+
+  // Simulate frame render delay.
+  base::TimeTicks render_started_time = base::TimeTicks::Now();
+  base::PlatformThread::Sleep(base::TimeDelta::FromMilliseconds(1));
+  base::TimeDelta render_delay_min =
+      base::TimeTicks::Now() - render_started_time;
+
+  packet_processed_closure_.Run();

[Wez, 2015/01/21 01:35:39]

Comment this, e.g. "Fake completion of video packet decoding, to trigger the
Ack."

[Sergey Ulanov, 2015/01/29 01:33:28]

Done.

+  base::TimeDelta render_delay_max =
+      base::TimeTicks::Now() - write_started_time;

[Wez, 2015/01/21 01:35:39]

nit: Could we use a base::TickTimer "seam" to allow this to be tested more
cleanly, perhaps?

[Sergey Ulanov, 2015/01/29 01:33:29]

There are two problems with base::TickClock
1. It's designed to be owned by the consumer, which means that it's hard to
share it between multiple objects (in this case it needs to be shared between
MessageReader and ClientVideoDispatcher) and different object may see different
time values if you create multiple MockTickClock instances. I wish there was a
global TickClock shared between all clock consumers :(
2. It makes it easier to miss bugs in this CL. E.g.
MessageReader::last_message_read_delay() is supposed to return 0 for messages
that are received in a single packet and to verify it the timer needs to be
incremented between two consecutive tasks executed by MessageLoop, which is hard
to achieve with MockTickClock.

+
+  message_loop_.RunUntilIdle();
+  ASSERT_TRUE(ack_message_);
+  EXPECT_EQ(kTestFrameId, ack_message_->frame_id());
+
+  // Verify that rendering latency is reported correctly.
+  base::TimeDelta render_delay =
+      base::TimeDelta::FromMicroseconds(ack_message_->rendering_delay_us());
+  EXPECT_TRUE(render_delay >= render_delay_min);
+  EXPECT_TRUE(render_delay <= render_delay_max);
+}
+
+// Verifies that the dispatcher reports correct read delay in the messages it
+// receives.
+TEST_F(ClientVideoDispatcherTest, ReadDelay) {
+  int kTestFrameId = 3;
+
+  Initialize(kVideoWithAckStreamVersion);
+
+  VideoPacket packet;
+  packet.set_data(std::string());
+  packet.set_frame_id(kTestFrameId);
+
+  // Generate buffers for two video frames.
+  scoped_refptr<net::IOBufferWithSize> packet_buffer_1 =
+      SerializeAndFrameMessage(packet);
+  packet.set_frame_id(kTestFrameId + 1);
+  scoped_refptr<net::IOBufferWithSize> packet_buffer_2 =
+      SerializeAndFrameMessage(packet);
+
+  base::TimeTicks send_started_time = base::TimeTicks::Now();
+
+  // First send all data for the first frame and the first byte for the second
+  // one in a single network packet.

[Wez, 2015/01/21 01:35:39]

nit: There's no guarantee that this will result in a single packet, just that
all of the data in this one buffer will be PSH'd to the receiver, IIUC?

[Wez, 2015/01/21 01:35:39]

nit: s/one/frame

[Sergey Ulanov, 2015/01/29 01:33:29]

Done.

[Sergey Ulanov, 2015/01/29 01:33:29]

I think we can make this assumption with the fake sockets.

+  scoped_refptr<net::IOBufferWithSize> buffer =
+      new net::IOBufferWithSize(packet_buffer_1->size() + 1);
+  memcpy(buffer->data(), packet_buffer_1->data(), packet_buffer_1->size());
+  memcpy(buffer->data() + packet_buffer_1->size(), packet_buffer_2->data(), 1);
+  writer_.Write(buffer, base::Closure());
+  message_loop_.RunUntilIdle();
+
+  // Verify that the packet was received.
+  ASSERT_TRUE(video_packet_);

[Wez, 2015/01/21 01:35:39]

nit: EXPECT_TRUE here

[Sergey Ulanov, 2015/01/29 01:33:28]

Done.

+  video_packet_.reset();
+  packet_processed_closure_.Run();
+  message_loop_.RunUntilIdle();
+
+  // Verify that the Ack message for the first video frame has time_to_receive
+  // field set to 0, because the whole frame was received in one message.

[Wez, 2015/01/21 01:35:39]

Doesn't that make this test susceptible to flake e.g. if we start reading the
bytes in only to have some other process scheduled, and don't get back to
processing until the timer has rolled round?

[Sergey Ulanov, 2015/01/29 01:33:28]

No. It's not possible with the current implementation in MessageReader: it calls
base::TimeTicks::Now() only once per frame.

+  ASSERT_TRUE(ack_message_);
+  EXPECT_EQ(kTestFrameId, ack_message_->frame_id());
+  EXPECT_EQ(0, ack_message_->time_to_receive_us());
+  ack_message_.reset();
+
+  // Simulate networking latency while receiving the second frame.
+  base::TimeTicks network_delay_started_time = base::TimeTicks::Now();
+  base::PlatformThread::Sleep(base::TimeDelta::FromMilliseconds(1));
+  base::TimeDelta time_to_receive_min =
+      base::TimeTicks::Now() - network_delay_started_time;
+
+  // Send the remainder of the second frame.
+  buffer = new net::IOBufferWithSize(packet_buffer_2->size() - 1);
+  memcpy(buffer->data(), packet_buffer_2->data() + 1,
+         packet_buffer_2->size() - 1);
+  writer_.Write(buffer, base::Closure());
+  message_loop_.RunUntilIdle();
+
+  // Verify that the second packet was received.
+  ASSERT_TRUE(video_packet_);

[Wez, 2015/01/21 01:35:39]

nit: EXPECT_TRUE

[Sergey Ulanov, 2015/01/29 01:33:28]

Done.

+  video_packet_.reset();

[Wez, 2015/01/21 01:35:39]

Do you need to reset it here? You're not touching it later in the test, and
earlier tests don't bother resetting it

[Sergey Ulanov, 2015/01/29 01:33:29]

Done.

+  packet_processed_closure_.Run();
+  message_loop_.RunUntilIdle();
+
+  // Verify that we've received Ack for the second frame.
+  ASSERT_TRUE(ack_message_);
+  EXPECT_EQ(kTestFrameId + 1, ack_message_->frame_id());
+
+  // Verify that the dispatcher set correct time_to_receive value.
+  base::TimeDelta time_to_receive =
+      base::TimeDelta::FromMicroseconds(ack_message_->time_to_receive_us());
+  base::TimeDelta time_to_receive_max =
+      base::TimeTicks::Now() - send_started_time;

[Wez, 2015/01/21 01:35:39]

nit: Shouldn't this calculation take place immediately before the
packet-processed closure is run?

[Sergey Ulanov, 2015/01/29 01:33:28]

Done.

+  EXPECT_TRUE(time_to_receive >= time_to_receive_min);
+  EXPECT_TRUE(time_to_receive <= time_to_receive_max);
+}
+
+}  // namespace protocol
+}  // namespace remoting