Drop WebRTC audio data if OS has skipped frames.

content/renderer/media/webrtc_audio_renderer.cc

Index: content/renderer/media/webrtc_audio_renderer.cc
diff --git a/content/renderer/media/webrtc_audio_renderer.cc b/content/renderer/media/webrtc_audio_renderer.cc
index 640d8dc4ad689ba6db96781017231662b532d0be..478ab92e28c60b212d3024ae6f0ac6baaef8b06b 100644
--- a/content/renderer/media/webrtc_audio_renderer.cc
+++ b/content/renderer/media/webrtc_audio_renderer.cc
@@ -187,7 +187,6 @@ WebRtcAudioRenderer::WebRtcAudioRenderer(
       play_ref_count_(0),
       start_ref_count_(0),
       audio_delay_milliseconds_(0),
-      fifo_delay_milliseconds_(0),
       sink_params_(media::AudioParameters::AUDIO_PCM_LOW_LATENCY,
                    media::CHANNEL_LAYOUT_STEREO,
                    0,
@@ -434,6 +433,26 @@ int WebRtcAudioRenderer::Render(media::AudioBus* audio_bus,
   DCHECK_LE(audio_delay_milliseconds, static_cast<uint32_t>(INT_MAX));
   audio_delay_milliseconds_ = static_cast<int>(audio_delay_milliseconds);
 
+  // If there are skipped frames, pull and throw away the same amount.
+  if (frames_skipped > 0) {
+    const uint32_t frames_per_10ms =
+        static_cast<uint32_t>(sink_params_.sample_rate() / 100);
+    if (!audio_fifo_ && frames_skipped != frames_per_10ms) {

[peah, 2016/01/19 10:13:52]

What happens if frames_skipped > frames_per_10_ms. Is that properly handled.
(Maybe obvious but since I'm new to this code I don't see that it is.

[Henrik Grunell, 2016/01/19 13:03:41]

Yes, the fifo will handle that.

[peah, 2016/01/20 15:16:47]

I think that one of the things I don't understand is why we only create the fifo
if frames_skipped != frames_per_10ms. Furthermore, I don't really follow where
the length fifo matters?

I thought that the fifo pulled data in blocks of 10 ms at a time from the
decoder/etc. But is it so that the fifo pulls 10 ms at a time (frames are
inserted into the fifo in blocks of 10 ms) but frames can be removed from the
fifo in any number (in which case the fifo may contain a number of frames that
are less than 10 ms in size)?

As I understand it the processing is done as follows (please correct me if I'm
wrong).
1) Assume that (!!audio_fifo_) ==true. Then the length of the fifo is 10 ms of
frames, right?
2) Assume that frames_skipped is (frames_per_10_ms+15).
3) Then the operation audio_fifo_->Consume(drop_bus.get(), drop_bus->frames());
removes (frames_per_10_ms+15) frames, right?
4) During step 3) the fifo pulls an additional frames_per_10_ms, and after step
3) it contains (frames_per_10_ms-15) frames. Is that correct?

Two examples of required behavior for this code are: 
-If 10 frames have been skipped, then exactly 10 frames need to be dropped.
-if (frames_per_10ms+5) frames have been skipped, then exactly
(frames_per_10ms+5) frames need to be dropped.

[Henrik Grunell, 2016/01/21 08:55:46]

Since we always pull 10 ms of data from the source, we can just pull that
directly if that's the amount to drop. No need for a fifo. Any other amount
requires buffering the remainder.
The fifo pulls its size of frames from the source, and we set the size to be 10
ms. Yes, any number of frames can be pulled from the fifo. And yes, the
remainder will always be smaller than 10 ms.
Yes, it's always 10 ms.
Yes, that's correct.
Yep, both are fulfilled.

[peah, 2016/01/21 09:16:25]

Great! Thanks for the explanation! I have one more fifo question? Can the fifo
ever contain 0 frames, or is it then immediately filled with 10 ms of frames? 

[Henrik Grunell, 2016/01/21 12:26:16]

Any time. :) It can contain 0. Data is only pulled from source if needed.

+      audio_fifo_.reset(new media::AudioPullFifo(

[peah, 2016/01/19 10:13:52]

One thing I don't really get is the recreation of the audio_fifo_. 
Here it is created to be frames_per_10_ms of length, but below (line 663) it is
recreated to be source_params.frames_per_buffer() of length.

Will not this cause the FIFO to be recreated each time there is a frame skip not
equal to frames_per_10ms in length? And the next time PrepareSink is called it
is again recreated if it does not happen to be the same length, right?

What happens with any audio that happens to be in the FIFO if it is recreated?
Or is it a zero-length FIFO in the sense that it does not contain any data?

[Henrik Grunell, 2016/01/19 13:03:41]

The |source_params| frames_per_buffer always corresponds to 10 ms (line 638).
In the condition there is !audio_fifo_, so it will only be created is not
exists.
As long as the source params frames per buffer is the same, the fifo won't be
touched if it exists. But if it changes, the fifo must be recreated.
That data will be lost, before and after this CL. Note that the only time the
fifo changes is actually when the output device sample rate changes. I don't
think it will matter much to reset, correct if wrong.

[peah, 2016/01/21 09:16:25]

Since it always holds that 
source_params.frames_per_buffer()==frames_per_10ms
(that was the case right?)
It is a bit misleading to have 
audio_fifo_.reset(new media::AudioPullFifo(sink_params_.channels(),
frames_per_10ms,...
in one place, and
audio_fifo_.reset(new media::AudioPullFifo(
 665           source_params.channels(), source_params.frames_per_buffer(),
in the other, as it looks that they may be different allthough they are always
the same.
Would it be possible to change that?
It seems possible to do by either
1) Here replicating the creation of source_params.channels done below.
2) Below dropping the use of source_params and instead do the creation the way
it is done here.

[Henrik Grunell, 2016/01/21 12:26:16]

Yes source_params.frames_per_buffer()==frames_per_10ms always holds.

I agree the readability isn't the best (PrepareSink is hard to follow to begin
with). I've cleaned up a bit, which should make things clearer and more
consistent.

+          sink_params_.channels(), frames_per_10ms,
+          base::Bind(&WebRtcAudioRenderer::SourceCallback,
+                     base::Unretained(this))));
+    }
+
+    scoped_ptr<media::AudioBus> drop_bus =
+        media::AudioBus::Create(audio_bus->channels(), frames_skipped);
+    if (audio_fifo_)
+      audio_fifo_->Consume(drop_bus.get(), drop_bus->frames());

[peah, 2016/01/19 10:13:52]

How does this work? Does it consume audio_fifo_->SizeInFrames() or
drop_bus->frames() frames?

[Henrik Grunell, 2016/01/19 13:03:41]

It consumes drop_bus->frames() frames.

+    else
+      SourceCallback(0, drop_bus.get());

[peah, 2016/01/19 10:13:52]

To me this is a bit strange. Please correct me if I get this wrong!

if (!audio_fifo_ && frames_skipped != frames_per_10ms)
  Variant A:
  -Way of dropping samples: audio_fifo_ callback.
  -Number of samples dropped: frames_per_10ms (audio_fifo_.length)
else if (!audio_fifo_ && frames_skipped == frames_per_10ms)
  Variant B:
  -Way of dropping samples: Call to Sourcecallback
  -Number of samples dropped: frames_skipped
else if (audio_fifo_)
  Variant C:
   -Way of dropping samples: audio_fifo_ callback.
   -Number of samples dropped: audio_fifo_.length

As far as I can see, in variant C the number of samples dropped can be any
number, right? And it is not guaranteed that it is equal to the number of
frames_skipped, right?

[Henrik Grunell, 2016/01/19 13:03:41]

I sure will. ;)
It will drop |frames_skipped| frames. (Size of |drop_bus|.) Otherwise correct.
Correct.
It will drop |frames_skipped| frames. (Size of |drop_bus|.) Otherwise correct.
To summarize:
* We'll always drop |frames_skipped| frames.
* If frames_skipped is not 10 ms, we ensure a fifo exists.
* If fifo exists pull from it, otherwise bypass it.

+  }
+
+  // Pull the data we will deliver.
   if (audio_fifo_)
     audio_fifo_->Consume(audio_bus, audio_bus->frames());
   else
@@ -457,7 +476,9 @@ void WebRtcAudioRenderer::SourceCallback(
            << audio_bus->frames() << ")";
 
   int output_delay_milliseconds = audio_delay_milliseconds_;
-  output_delay_milliseconds += fifo_delay_milliseconds_;
+  output_delay_milliseconds += fifo_frame_delay *
+                               base::Time::kMillisecondsPerSecond /
+                               sink_params_.sample_rate();
   DVLOG(2) << "output_delay_milliseconds: " << output_delay_milliseconds;
 
   // We need to keep render data for the |source_| regardless of |state_|,
@@ -627,34 +648,25 @@ void WebRtcAudioRenderer::PrepareSink() {
   // Create a FIFO if re-buffering is required to match the source input with
   // the sink request. The source acts as provider here and the sink as
   // consumer.
-  int new_fifo_delay_milliseconds = 0;
   scoped_ptr<media::AudioPullFifo> new_audio_fifo;

[peah, 2016/01/19 10:13:52]

This seems to be unused after the change. Is that correct?

[Henrik Grunell, 2016/01/19 13:03:41]

Very much so. Removed.

-  if (source_params.frames_per_buffer() !=
-      new_sink_params.frames_per_buffer()) {
+  const bool different_source_sink_frames =
+      source_params.frames_per_buffer() != new_sink_params.frames_per_buffer();
+  if (different_source_sink_frames) {
     DVLOG(1) << "Rebuffering from " << source_params.frames_per_buffer()
              << " to " << new_sink_params.frames_per_buffer();
-    new_audio_fifo.reset(new media::AudioPullFifo(
-        source_params.channels(), source_params.frames_per_buffer(),
-        base::Bind(&WebRtcAudioRenderer::SourceCallback,
-                   base::Unretained(this))));
-
-    if (new_sink_params.frames_per_buffer() >
-        source_params.frames_per_buffer()) {
-      int frame_duration_milliseconds =
-          base::Time::kMillisecondsPerSecond /
-          static_cast<double>(source_params.sample_rate());
-      new_fifo_delay_milliseconds = (new_sink_params.frames_per_buffer() -
-                                     source_params.frames_per_buffer()) *
-                                    frame_duration_milliseconds;
-    }
   }
 
   {
     base::AutoLock lock(lock_);
+    if ((!audio_fifo_ && different_source_sink_frames) ||
+        (audio_fifo_ &&
+         audio_fifo_->SizeInFrames() != source_params.frames_per_buffer())) {
+      audio_fifo_.reset(new media::AudioPullFifo(
+          source_params.channels(), source_params.frames_per_buffer(),
+          base::Bind(&WebRtcAudioRenderer::SourceCallback,
+                     base::Unretained(this))));
+    }
     sink_params_ = new_sink_params;
-    fifo_delay_milliseconds_ = new_fifo_delay_milliseconds;
-    if (new_audio_fifo.get())
-      audio_fifo_ = std::move(new_audio_fifo);
   }
 
   sink_->Initialize(new_sink_params, this);