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..99f797e9ad3035479fabaef07a2178479223d3f1 100644
--- a/remoting/protocol/client_video_dispatcher.cc
+++ b/remoting/protocol/client_video_dispatcher.cc
@@ -8,20 +8,65 @@
 #include "net/socket/stream_socket.h"
 #include "remoting/base/constants.h"
 #include "remoting/proto/video.pb.h"
+#include "remoting/protocol/message_serialization.h"
 #include "remoting/protocol/video_stub.h"
 
 namespace remoting {
 namespace protocol {
 
+struct ClientVideoDispatcher::PendingFrame {
+  PendingFrame(int frame_id)
+      : frame_id(frame_id),
+        done(false) {}
+  int frame_id;
+  bool done;
+};
+
 ClientVideoDispatcher::ClientVideoDispatcher(VideoStub* video_stub)
     : ChannelDispatcherBase(kVideoChannelName),
-      parser_(base::Bind(&VideoStub::ProcessVideoPacket,
-                         base::Unretained(video_stub)),
+      video_stub_(video_stub),
+      parser_(base::Bind(&ClientVideoDispatcher::ProcessVideoPacket,
+                         base::Unretained(this)),
               reader()) {
 }
 
 ClientVideoDispatcher::~ClientVideoDispatcher() {
 }
 
+void ClientVideoDispatcher::ProcessVideoPacket(
+    scoped_ptr<VideoPacket> video_packet,
+    const base::Closure& done) {
+  int frame_id = video_packet->frame_id();
+
+  if (!video_packet->has_frame_id()) {
+    video_stub_->ProcessVideoPacket(video_packet.Pass(), done);
+    return;
+  }
+
+  PendingFramesList::iterator pending_frame =
+      pending_frames_.insert(pending_frames_.end(), PendingFrame(frame_id));
+
+  video_stub_->ProcessVideoPacket(
+      video_packet.Pass(), base::Bind(&ClientVideoDispatcher::OnPacketDone,
+                                      base::Unretained(this), pending_frame));

[Wez, 2015/02/11 02:22:56]

Why do we only want to ACK after the frame has been processed? What does that
even mean in terms of client-side processing?

[Sergey Ulanov, 2015/02/17 19:37:07]

If the renderer is slow/blocked we want the host to stop sending frames.

[Wez, 2015/02/21 03:12:02]

Acknowledged.

+
+  done.Run();

[Wez, 2015/02/11 02:22:56]

ScopedTaskRunner for this?

[Sergey Ulanov, 2015/02/17 19:37:07]

Done.

+}
+
+void ClientVideoDispatcher::OnPacketDone(
+    PendingFramesList::iterator pending_frame) {

[Wez, 2015/02/11 02:22:56]

You're passing in an iterator into the list, but the loop below potentially pops
multiple items out of the list, so you've no guarantee that the iterator is
valid...

[Sergey Ulanov, 2015/02/17 19:37:07]

std::list<> iterators remain valid even when the list mutated, as long as the
element the iterator refers to is still in the list.

+  // Mark the frame as done.
+  pending_frame->done = true;
+
+  // Send VideoAck for all packets in the head of the queue that have finished
+  // rendering.
+  while (!pending_frames_.empty() && pending_frames_.front().done) {

[Wez, 2015/02/11 02:22:56]

Why do you need this loop? Surely you'll already have ACK'd and pop'd those
other frames if you reach here?

[Sergey Ulanov, 2015/02/17 19:37:07]

VideoAck messages should be sent in the order VideoFrame messages are received,
but VideoStub is allowed to process them in any order. So for Done callback
called out of order we just set |done| flag and send the ACK only after all
messages before it are marked as done

[Wez, 2015/02/21 03:12:02]

Ahhhh, OK, yes I see what you mean. SGTM

+    VideoAck ack_message;
+    ack_message.set_frame_id(pending_frames_.front().frame_id);
+    writer()->Write(SerializeAndFrameMessage(ack_message), base::Closure());
+    pending_frames_.pop_front();
+  }
+}
+
 }  // namespace protocol
 }  // namespace remoting