Cast: Make vp8 3-buffer mode work

Cast was originally intended to tell vp8 to only reference buffers which have been ACKed.
However, that mode was buggy and disabled. This CL fixes it again. The three-buffer mode
lets the receiver drop frames and continue, which should be quite useful if we want lower
latency.

BUG=351596

Review URL: https://codereview.chromium.org/339743002

Cr-Commit-Position: refs/heads/master@{#293243}

media/cast/net/rtp/framer.cc

@@ -83,11 +83,6 @@ bool Framer::GetEncodedFrame(EncodedFrame* frame,
     *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())
@@ -96,6 +91,11 @@ bool Framer::GetEncodedFrame(EncodedFrame* frame,
   return it->second->AssembleEncodedFrame(frame);
 }
 
+void Framer::AckFrame(uint32 frame_id) {
+  VLOG(2) << "ACK frame " << frame_id;
+  cast_msg_builder_->CompleteFrameReceived(frame_id);
+}
+
 void Framer::Reset() {
   frame_id_map_.Clear();
   frames_.clear();

media/cast/net/rtp/framer.h

@@ -49,6 +49,9 @@ class Framer {
                        bool* next_frame,
                        bool* have_multiple_complete_frames);
 
+  // TODO(hubbe): Move this elsewhere.
+  void AckFrame(uint32 frame_id);
+
   void ReleaseFrame(uint32 frame_id);
 
   // Reset framer state to original state and flush all pending buffers.

media/cast/receiver/frame_receiver.cc

@@ -216,9 +216,10 @@ void FrameReceiver::EmitAvailableEncodedFrames() {
     // skipping one or more frames.  Skip if the missing frame wouldn't complete
     // playing before the start of playback of the available frame.
     if (!is_consecutively_next_frame) {
-      // TODO(miu): Also account for expected decode time here?
+      // This assumes that decoding takes as long as playing, which might
+      // not be true.
       const base::TimeTicks earliest_possible_end_time_of_missing_frame =
-          now + expected_frame_duration_;
+          now + expected_frame_duration_ * 2;
       if (earliest_possible_end_time_of_missing_frame < playout_time) {
         VLOG(1) << "Wait for next consecutive frame instead of skipping.";
         if (!is_waiting_for_consecutive_frame_) {
@@ -234,6 +235,11 @@ void FrameReceiver::EmitAvailableEncodedFrames() {
       }
     }
 
+    // At this point, we have the complete next frame, or a decodable
+    // frame from somewhere later in the stream, AND we have given up
+    // on waiting for any frames in between, so now we can ACK the frame.
+    framer_.AckFrame(encoded_frame->frame_id);
+
     // Decrypt the payload data in the frame, if crypto is being used.
     if (decryptor_.is_activated()) {
       std::string decrypted_data;

media/cast/sender/video_encoder_impl_unittest.cc

@@ -36,15 +36,14 @@ class TestVideoEncoderCallback
 
   void DeliverEncodedVideoFrame(
       scoped_ptr<EncodedFrame> encoded_frame) {
-    if (expected_frame_id_ == expected_last_referenced_frame_id_) {
-      EXPECT_EQ(EncodedFrame::KEY, encoded_frame->dependency);
-    } else {
+    if (expected_frame_id_ != expected_last_referenced_frame_id_) {
       EXPECT_EQ(EncodedFrame::DEPENDENT,
                 encoded_frame->dependency);
     }
     EXPECT_EQ(expected_frame_id_, encoded_frame->frame_id);
     EXPECT_EQ(expected_last_referenced_frame_id_,
-              encoded_frame->referenced_frame_id);
+              encoded_frame->referenced_frame_id)
+        << "frame id: " << expected_frame_id_;
     EXPECT_EQ(expected_capture_time_, encoded_frame->reference_time);
   }
 

media/cast/sender/vp8_encoder.h

@@ -55,6 +55,16 @@ class Vp8Encoder : public SoftwareVideoEncoder {
     kNoBuffer = 3  // Note: must be last.
   };
 
+  enum Vp8BufferState {
+    kBufferStartState,
+    kBufferSent,
+    kBufferAcked
+  };
+  struct BufferState {
+    uint32 frame_id;
+    Vp8BufferState state;
+  };
+
   void InitEncode(int number_of_cores);
 
   // Calculate the max target in % for a keyframe.
@@ -63,11 +73,9 @@ class Vp8Encoder : public SoftwareVideoEncoder {
   // Calculate which next Vp8 buffers to update with the next frame.
   Vp8Buffers GetNextBufferToUpdate();
 
-  // Calculate which previous frame to reference.
-  uint32 GetLatestFrameIdToReference();
-
   // Get encoder flags for our referenced encoder buffers.
-  void GetCodecReferenceFlags(vpx_codec_flags_t* flags);
+  // Return which previous frame to reference.
+  uint32 GetCodecReferenceFlags(vpx_codec_flags_t* flags);
 
   // Get encoder flags for our encoder buffers to update with next frame.
   void GetCodecUpdateFlags(Vp8Buffers buffer_to_update,
@@ -75,7 +83,6 @@ class Vp8Encoder : public SoftwareVideoEncoder {
 
   const VideoSenderConfig cast_config_;
   const bool use_multiple_video_buffers_;
-  const int max_number_of_repeated_buffers_in_a_row_;
 
   // VP8 internal objects.
   scoped_ptr<vpx_codec_enc_cfg_t> config_;
@@ -86,10 +93,10 @@ class Vp8Encoder : public SoftwareVideoEncoder {
   bool first_frame_received_;
   base::TimeDelta first_frame_timestamp_;
   uint32 last_encoded_frame_id_;
-  uint32 used_buffers_frame_id_[kNumberOfVp8VideoBuffers];
-  bool acked_frame_buffers_[kNumberOfVp8VideoBuffers];
-  Vp8Buffers last_used_vp8_buffer_;
-  int number_of_repeated_buffers_;
+  uint32 last_acked_frame_id_;
+  uint32 frame_id_to_reference_;
+  uint32 undroppable_frames_;
+  BufferState buffer_state_[kNumberOfVp8VideoBuffers];
 
   // This is bound to the thread where Initialize() is called.
   base::ThreadChecker thread_checker_;

media/cast/test/end2end_unittest.cc

@@ -210,7 +210,7 @@ class LoopBackTransport : public PacketSender {
                           const base::Closure& cb) OVERRIDE {
     DCHECK(cast_environment_->CurrentlyOn(CastEnvironment::MAIN));
     if (!send_packets_)
-      return false;
+      return true;
 
     bytes_sent_ += packet->data.size();
     if (drop_packets_belonging_to_odd_frames_) {
@@ -406,7 +406,9 @@ class TestReceiverVideoCallback
     PopulateVideoFrame(expected_I420_frame.get(),
                        expected_video_frame.start_value);
 
-    EXPECT_GE(I420PSNR(expected_I420_frame, video_frame), kVideoAcceptedPSNR);
+    if (expected_video_frame.should_be_continuous) {
+      EXPECT_GE(I420PSNR(expected_I420_frame, video_frame), kVideoAcceptedPSNR);
+    }
 
     EXPECT_NEAR(
         (playout_time - expected_video_frame.playout_time).InMillisecondsF(),
@@ -988,79 +990,14 @@ TEST_F(End2EndTest, DISABLED_StartSenderBeforeReceiver) {
   EXPECT_EQ(10, test_receiver_video_callback_->number_times_called());
 }
 
-// This tests a network glitch lasting for 10 video frames.
-// Flaky. See crbug.com/351596.
-TEST_F(End2EndTest, DISABLED_GlitchWith3Buffers) {
-  Configure(CODEC_VIDEO_VP8, CODEC_AUDIO_OPUS,
-            kDefaultAudioSamplingRate, 3);
-  video_sender_config_.target_playout_delay =
-      base::TimeDelta::FromMilliseconds(67);
-  video_receiver_config_.rtp_max_delay_ms = 67;
-  Create();
-
-  int video_start = kVideoStart;
-  base::TimeTicks capture_time;
-  // Frames will rendered on completion until the render time stabilizes, i.e.
-  // we got enough data.
-  const int frames_before_glitch = 20;
-  for (int i = 0; i < frames_before_glitch; ++i) {
-    capture_time = testing_clock_sender_->NowTicks();
-    SendVideoFrame(video_start, capture_time);
-    test_receiver_video_callback_->AddExpectedResult(
-        video_start,
-        video_sender_config_.width,
-        video_sender_config_.height,
-        capture_time + base::TimeDelta::FromMilliseconds(kTargetPlayoutDelayMs),
-        true);
-    cast_receiver_->RequestDecodedVideoFrame(
-        base::Bind(&TestReceiverVideoCallback::CheckVideoFrame,
-                   test_receiver_video_callback_));
-    RunTasks(kFrameTimerMs);
-    video_start++;
-  }
-
-  // Introduce a glitch lasting for 10 frames.
-  sender_to_receiver_.SetSendPackets(false);
-  for (int i = 0; i < 10; ++i) {
-    capture_time = testing_clock_sender_->NowTicks();
-    // First 3 will be sent and lost.
-    SendVideoFrame(video_start, capture_time);
-    RunTasks(kFrameTimerMs);
-    video_start++;
-  }
-  sender_to_receiver_.SetSendPackets(true);
-  RunTasks(100);
-  capture_time = testing_clock_sender_->NowTicks();
-
-  // Frame 1 should be acked by now and we should have an opening to send 4.
-  SendVideoFrame(video_start, capture_time);
-  RunTasks(kFrameTimerMs);
-
-  // Frames 1-3 are old frames by now, and therefore should be decoded, but
-  // not rendered. The next frame we expect to render is frame #4.
-  test_receiver_video_callback_->AddExpectedResult(
-      video_start,
-      video_sender_config_.width,
-      video_sender_config_.height,
-      capture_time + base::TimeDelta::FromMilliseconds(kTargetPlayoutDelayMs),
-      true);
-
-  cast_receiver_->RequestDecodedVideoFrame(
-      base::Bind(&TestReceiverVideoCallback::CheckVideoFrame,
-                 test_receiver_video_callback_));
-
-  RunTasks(2 * kFrameTimerMs + 1);  // Empty the receiver pipeline.
-  EXPECT_EQ(frames_before_glitch + 1,
-            test_receiver_video_callback_->number_times_called());
-}
-
-// Disabled due to flakiness and crashiness.  http://crbug.com/360951
-TEST_F(End2EndTest, DISABLED_DropEveryOtherFrame3Buffers) {
-  Configure(CODEC_VIDEO_VP8, CODEC_AUDIO_OPUS,
-            kDefaultAudioSamplingRate, 3);
+TEST_F(End2EndTest, DropEveryOtherFrame3Buffers) {
+  Configure(CODEC_VIDEO_VP8, CODEC_AUDIO_OPUS, kDefaultAudioSamplingRate, 3);
+  int target_delay = 300;
   video_sender_config_.target_playout_delay =
-      base::TimeDelta::FromMilliseconds(67);
-  video_receiver_config_.rtp_max_delay_ms = 67;
+      base::TimeDelta::FromMilliseconds(target_delay);
+  audio_sender_config_.target_playout_delay =
+      base::TimeDelta::FromMilliseconds(target_delay);
+  video_receiver_config_.rtp_max_delay_ms = target_delay;
   Create();
   sender_to_receiver_.DropAllPacketsBelongingToOddFrames();
 
@@ -1078,7 +1015,7 @@ TEST_F(End2EndTest, DISABLED_DropEveryOtherFrame3Buffers) {
           video_sender_config_.width,
           video_sender_config_.height,
           capture_time +
-              base::TimeDelta::FromMilliseconds(kTargetPlayoutDelayMs),
+              base::TimeDelta::FromMilliseconds(target_delay),
           i == 0);
 
       // GetRawVideoFrame will not return the frame until we are close in
@@ -1091,7 +1028,7 @@ TEST_F(End2EndTest, DISABLED_DropEveryOtherFrame3Buffers) {
     video_start++;
   }
 
-  RunTasks(2 * kFrameTimerMs + 1);  // Empty the pipeline.
+  RunTasks(2 * kFrameTimerMs + target_delay);  // Empty the pipeline.
   EXPECT_EQ(i / 2, test_receiver_video_callback_->number_times_called());
 }