Cast: Only ACK decodable frames

media/cast/framer/framer.cc

 // Copyright 2013 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 "media/cast/framer/framer.h"
 
 #include "base/logging.h"
 
 namespace media {
 namespace cast {
 
 typedef FrameList::const_iterator ConstFrameIterator;
 
 Framer::Framer(base::TickClock* clock,
                RtpPayloadFeedback* incoming_payload_feedback,
                uint32 ssrc,
                bool decoder_faster_than_max_frame_rate,
                int max_unacked_frames)
     : decoder_faster_than_max_frame_rate_(decoder_faster_than_max_frame_rate),
       cast_msg_builder_(
           new CastMessageBuilder(clock,
                                  incoming_payload_feedback,
                                  &frame_id_map_,

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

I have limited context from just looking at this file, but I was wondering if it
makes sense to combine FrameIdMap with FrameList. It seems what we are doing
here is keeping the two objects "consistent" by modifying them similarly (e.g.
lookup FrameIdMap then use result to look up Framer, or erase from FrameIdMap
and then Framer), and it looks error prone this way.

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

That does seem like a good idea, but is out of scope of this CL.

                                  ssrc,
                                  decoder_faster_than_max_frame_rate,
                                  max_unacked_frames)) {
   DCHECK(incoming_payload_feedback) << "Invalid argument";
 }
 
 Framer::~Framer() {}
 
 bool Framer::InsertPacket(const uint8* payload_data,
                           size_t payload_size,
@@ skipping 17 matching lines @@
   if (it == frames_.end()) {
     // New frame.
     linked_ptr<FrameBuffer> frame_buffer(new FrameBuffer());
     frame_buffer->InsertPacket(payload_data, payload_size, rtp_header);
     frames_.insert(std::make_pair(rtp_header.frame_id, frame_buffer));
   } else {
     // Insert packet to existing frame buffer.
     it->second->InsertPacket(payload_data, payload_size, rtp_header);
   }
 
-  bool complete = (packet_type == kNewPacketCompletingFrame);
-  if (complete) {
-    // ACK as soon as possible.
-    VLOG(2) << "Complete frame " << static_cast<int>(rtp_header.frame_id);
-    cast_msg_builder_->CompleteFrameReceived(rtp_header.frame_id,
-                                             rtp_header.is_key_frame);
-  }
-  return complete;
+  return packet_type == kNewPacketCompletingFrame;
 }
 
 // This does not release the frame.
 bool Framer::GetEncodedAudioFrame(transport::EncodedAudioFrame* audio_frame,
                                   bool* next_frame) {
   uint32 frame_id;
   // Find frame id.
   if (frame_id_map_.NextContinuousFrame(&frame_id)) {
     // We have our next frame.
     *next_frame = true;
   } else {
     if (!frame_id_map_.NextAudioFrameAllowingMissingFrames(&frame_id)) {
       return false;
     }
     *next_frame = false;
   }
 
+  if (next_frame) {
+    VLOG(2) << "ACK frame " << frame_id;
+    cast_msg_builder_->CompleteFrameReceived(frame_id);
+  }
+
   ConstFrameIterator it = frames_.find(frame_id);
   DCHECK(it != frames_.end());
   if (it == frames_.end())
     return false;
 
   return it->second->GetEncodedAudioFrame(audio_frame);
 }
 
 // This does not release the frame.
 bool Framer::GetEncodedVideoFrame(transport::EncodedVideoFrame* video_frame,
-                                  bool* next_frame) {
+                                  bool* next_frame,
+                                  bool* have_multiple_decodable_frames) {
+  *have_multiple_decodable_frames = frame_id_map_.HaveMultipleDecodableFrames();
+
   uint32 frame_id;
   // Find frame id.
   if (frame_id_map_.NextContinuousFrame(&frame_id)) {
     // We have our next frame.
     *next_frame = true;
   } else {
     // Check if we can skip frames when our decoder is too slow.
     if (!decoder_faster_than_max_frame_rate_)
       return false;
 
     if (!frame_id_map_.NextVideoFrameAllowingSkippingFrames(&frame_id)) {
       return false;
     }
     *next_frame = false;
   }
 
+  if (next_frame) {

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

*next_frame

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

Done (so much for test coverage)

+    VLOG(2) << "ACK frame " << frame_id;
+    cast_msg_builder_->CompleteFrameReceived(frame_id);
+  }
+
   ConstFrameIterator it = frames_.find(frame_id);
   DCHECK(it != frames_.end());
   if (it == frames_.end())
     return false;
 
   return it->second->GetEncodedVideoFrame(video_frame);
 }
 
 void Framer::Reset() {
   frame_id_map_.Clear();
@@ skipping 22 matching lines @@
 }
 
 bool Framer::TimeToSendNextCastMessage(base::TimeTicks* time_to_send) {
   return cast_msg_builder_->TimeToSendNextCastMessage(time_to_send);
 }
 
 void Framer::SendCastMessage() { cast_msg_builder_->UpdateCastMessage(); }
 
 }  // namespace cast
 }  // namespace media