Cast: Only ACK decodable frames

media/cast/framer/cast_message_builder.cc

Index: media/cast/framer/cast_message_builder.cc
diff --git a/media/cast/framer/cast_message_builder.cc b/media/cast/framer/cast_message_builder.cc
index 5c317711be126d31f5ebc791116aa57b1025957a..f3473f969028cf3c0a4631dc8b1b167ad3b76cd7 100644
--- a/media/cast/framer/cast_message_builder.cc
+++ b/media/cast/framer/cast_message_builder.cc
@@ -23,7 +23,6 @@ CastMessageBuilder::CastMessageBuilder(
       decoder_faster_than_max_frame_rate_(decoder_faster_than_max_frame_rate),
       max_unacked_frames_(max_unacked_frames),
       cast_msg_(media_ssrc),
-      waiting_for_key_frame_(true),
       slowing_down_ack_(false),
       acked_last_frame_(true),
       last_acked_frame_id_(kStartFrameId) {
@@ -32,67 +31,61 @@ CastMessageBuilder::CastMessageBuilder(
 
 CastMessageBuilder::~CastMessageBuilder() {}
 
-void CastMessageBuilder::CompleteFrameReceived(uint32 frame_id,
-                                               bool is_key_frame) {
+void CastMessageBuilder::CompleteFrameReceived(uint32 frame_id) {
+  DCHECK_GE(static_cast<int32>(frame_id - last_acked_frame_id_), 0);

[imcheng, 2014/05/14 20:11:24]

What guarantees this?

[hubbe, 2014/05/14 21:46:16]

Hopefully the cast_receiver does.
I put it in here because some of the logic below assumes it.

+  VLOG(2) << "CompleteFrameReceived: " << frame_id;
   if (last_update_time_.is_null()) {
     // Our first update.
     last_update_time_ = clock_->NowTicks();
   }
-  if (waiting_for_key_frame_) {
-    if (!is_key_frame) {
-      // Ignore that we have received this complete frame since we are
-      // waiting on a key frame.
-      return;
-    }
-    waiting_for_key_frame_ = false;
-    cast_msg_.missing_frames_and_packets_.clear();
-    cast_msg_.ack_frame_id_ = frame_id;
-    last_update_time_ = clock_->NowTicks();
-    // We might have other complete frames waiting after we receive the last
-    // packet in the key-frame.
-    UpdateAckMessage();
-  } else {
-    if (!UpdateAckMessage())
-      return;
-
-    BuildPacketList();
+
+  if (!UpdateAckMessage(frame_id)) {
+    return;
   }
+  BuildPacketList();
+
   // Send cast message.
   VLOG(2) << "Send cast message Ack:" << static_cast<int>(frame_id);
   cast_feedback_->CastFeedback(cast_msg_);
 }
 
-bool CastMessageBuilder::UpdateAckMessage() {
+bool CastMessageBuilder::UpdateAckMessage(uint32 frame_id) {
   if (!decoder_faster_than_max_frame_rate_) {
     int complete_frame_count = frame_id_map_->NumberOfCompleteFrames();
     if (complete_frame_count > max_unacked_frames_) {
       // We have too many frames pending in our framer; slow down ACK.
-      slowing_down_ack_ = true;
+      if (!slowing_down_ack_) {
+        slowing_down_ack_ = true;
+        ack_queue_.push_back(last_acked_frame_id_);
+      }
     } else if (complete_frame_count <= 1) {
       // We are down to one or less frames in our framer; ACK normally.
       slowing_down_ack_ = false;
+      ack_queue_.clear();
     }
   }
+
   if (slowing_down_ack_) {
     // We are slowing down acknowledgment by acknowledging every other frame.
-    if (acked_last_frame_) {
-      acked_last_frame_ = false;
-    } else {
-      acked_last_frame_ = true;
-      last_acked_frame_id_++;
-      // Note: frame skipping and slowdown ACK is not supported at the same
-      // time; and it's not needed since we can skip frames to catch up.
-    }
-  } else {
-    uint32 frame_id = frame_id_map_->LastContinuousFrame();
-
-    // Is it a new frame?
-    if (last_acked_frame_id_ == frame_id)
+    // Note: frame skipping and slowdown ACK is not supported at the same
+    // time; and it's not needed since we can skip frames to catch up.
+    if (!ack_queue_.empty() && ack_queue_.back() == frame_id) {
       return false;
+    }
+    ack_queue_.push_back(frame_id);
+    if (!acked_last_frame_) {
+      ack_queue_.pop_front();
+    }
+    frame_id = ack_queue_.front();

[imcheng, 2014/05/14 20:11:24]

Do you need to do another pop_front()? Comment above states ack every other
frame.

[imcheng, 2014/05/14 20:11:24]

Can ack_queue_ be empty at this point?

[hubbe, 2014/05/14 21:46:16]

What it means is that in slow-ack mode we ack delay acking like this:

We are asked to ack:
1     2     3     4     5     6     7
What we actually ack:
1           2           3           4

[hubbe, 2014/05/14 21:46:16]

No.
When we go into slow mode, we push one value on the queue.
After that we push one value every call and pull one value every other call. We
will have at least one value.

+  }
 
-    last_acked_frame_id_ = frame_id;
-    acked_last_frame_ = true;
+  acked_last_frame_ = false;
+  // Is it a new frame?
+  if (last_acked_frame_id_ == frame_id) {
+    return false;
   }
+  acked_last_frame_ = true;
+  last_acked_frame_id_ = frame_id;
   cast_msg_.ack_frame_id_ = last_acked_frame_id_;
   cast_msg_.missing_frames_and_packets_.clear();
   last_update_time_ = clock_->NowTicks();
@@ -120,7 +113,6 @@ void CastMessageBuilder::UpdateCastMessage() {
 }
 
 void CastMessageBuilder::Reset() {
-  waiting_for_key_frame_ = true;
   cast_msg_.ack_frame_id_ = kStartFrameId;
   cast_msg_.missing_frames_and_packets_.clear();
   time_last_nacked_map_.clear();
@@ -142,7 +134,8 @@ bool CastMessageBuilder::UpdateCastMessageInternal(RtcpCastMessage* message) {
   }
   last_update_time_ = now;
 
-  UpdateAckMessage();  // Needed to cover when a frame is skipped.
+  // Needed to cover when a frame is skipped.
+  UpdateAckMessage(last_acked_frame_id_);
   BuildPacketList();
   message->Copy(cast_msg_);
   return true;