Implement video frame acknowledgements in the chromoting protocol.

remoting/protocol/client_video_dispatcher.cc

Index: remoting/protocol/client_video_dispatcher.cc
diff --git a/remoting/protocol/client_video_dispatcher.cc b/remoting/protocol/client_video_dispatcher.cc
index 287bba329cab9ee428d4a87b2a5f850499f27910..1fef81c2c258f921bba88cc5c3b72aafa5dc679c 100644
--- a/remoting/protocol/client_video_dispatcher.cc
+++ b/remoting/protocol/client_video_dispatcher.cc
@@ -8,6 +8,8 @@
 #include "net/socket/stream_socket.h"
 #include "remoting/base/constants.h"
 #include "remoting/proto/video.pb.h"
+#include "remoting/protocol/errors.h"
+#include "remoting/protocol/message_serialization.h"
 #include "remoting/protocol/video_stub.h"
 
 namespace remoting {
@@ -15,13 +17,43 @@ namespace protocol {
 
 ClientVideoDispatcher::ClientVideoDispatcher(VideoStub* video_stub)
     : ChannelDispatcherBase(kVideoChannelName),
-      parser_(base::Bind(&VideoStub::ProcessVideoPacket,
-                         base::Unretained(video_stub)),
+      video_stub_(video_stub),
+      parser_(base::Bind(&ClientVideoDispatcher::OnVideoPacket,
+                         base::Unretained(this)),
               reader()) {
 }
 
 ClientVideoDispatcher::~ClientVideoDispatcher() {
 }
 
+void ClientVideoDispatcher::OnVideoPacket(scoped_ptr<VideoPacket> video_packet,
+                                          const base::Closure& done) {
+  base::TimeTicks now = base::TimeTicks::Now();
+
+  VideoAck ack_message;

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

Is creating and copying a protobuf super cheap? Consider just passing the two
parameters directly to OnPacketProcessed and creating the ACK message there?

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

Done.

+  ack_message.set_frame_id(video_packet->frame_id());
+
+  ack_message.set_time_to_receive_us(
+      reader()->last_message_read_delay().InMicroseconds());

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

nit: This line is logically part of initializing ack_message, so move it up to
make a since create-and-initialize-ack_message block?

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

Done.

+
+  video_stub_->ProcessVideoPacket(
+      video_packet.Pass(),
+      base::Bind(&ClientVideoDispatcher::OnPacketProcessed,
+                 base::Unretained(this), ack_message, now, done));
+}
+
+void ClientVideoDispatcher::OnPacketProcessed(VideoAck ack_message,
+                                              base::TimeTicks received_time,
+                                              const base::Closure& done) {
+  done.Run();
+
+  // Send Ack message for newer version of the protocol.
+  if (channel_config().version >= kVideoWithAckStreamVersion) {

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

The stream version never changes mid-session, so I'd suggest rearranging things
to check before the Bind(), except that this will become the "normal" path in
future.

So, given that the normal path is to send the ack, I'd suggest rewriting this
if() to be an early-exit.

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

Done.

+    base::TimeDelta rendering_delay = base::TimeTicks::Now() - received_time;
+    ack_message.set_rendering_delay_us(rendering_delay.InMicroseconds());
+    writer()->Write(SerializeAndFrameMessage(ack_message), base::Closure());
+  }
+}
+
 }  // namespace protocol
 }  // namespace remoting