Drop WebRTC audio data if OS has skipped frames.

Drop WebRTC audio data if OS has skipped frames.

This ensures that we get what is actually played out with what we feed to the echo canceler as far-end data back in sync as quickly as possible after a skip, which in turn reduces the risk of echo. We drop data in WebRtcAudioRenderer::Render.

The fifo is created (if it doesn't exist) if the number of skipped frames is not 10 ms of data.

This CL also removes creating a new fifo unecessarily if only the sink's frames per buffer changes but not the source's (when they differ).

BUG=560371
Committed: https://crrev.com/b4bc1f2da7414fcbc7d18d860dd7c8260dd9c891
Cr-Commit-Position: refs/heads/master@{#370686}

[Henrik Grunell, 2016-01-15 15:54:31]

Description was changed from

==========
Drop WebRTC audio data if OS has skipped frames.

This ensures that we get what is actually played out with what we feed to the
echo canceler as far-end data back in sync as quickly as possible after a skip,
which in turn reduces the risk of echo. We drop data in
WebRtcAudioRenderer::Render.

The fifo is created (if it doesn't exist) if the number of skipped frames is not
10 ms of data.

This CL also removes creating a new fifo unecessarily if only the sink's frames
per buffer changes but not the source's (when they differ).

BUG=560371
==========

to

==========
Drop WebRTC audio data if OS has skipped frames.

This ensures that we get what is actually played out with what we feed to the
echo canceler as far-end data back in sync as quickly as possible after a skip,
which in turn reduces the risk of echo. We drop data in
WebRtcAudioRenderer::Render.

The fifo is created (if it doesn't exist) if the number of skipped frames is not
10 ms of data.

This CL also removes creating a new fifo unecessarily if only the sink's frames
per buffer changes but not the source's (when they differ).

BUG=560371
==========

[Henrik Grunell, 2016-01-15 15:54:31]

grunell@chromium.org changed reviewers:
+ dalecurtis@chromium.org, tommi@chromium.org

[Henrik Grunell, 2016-01-15 15:57:21]

grunell@chromium.org changed reviewers:
+ peah@chromium.org

[Henrik Grunell, 2016-01-15 15:57:22]

Tommi: content/renderer/media/*
Dale: media/*
Per: High level from AEC perspective.

This CL replaces https://codereview.chromium.org/1528473003/ with a new
approach, based on discussions with AEC experts peah@ and hlundin@.

[Henrik Grunell, 2016-01-15 16:03:22]

On 2016/01/15 15:57:22, Henrik Grunell wrote:
> Tommi: content/renderer/media/*
> Dale: media/*
> Per: High level from AEC perspective.
> 
> This CL replaces https://codereview.chromium.org/1528473003/ with a new
> approach, based on discussions with AEC experts peah@ and hlundin@.

Oh, and I shall update the unit test(s) too. Coming up on Monday.

[DaleCurtis, 2016-01-15 18:25:26]

media/ lgtm

[tommi (sloooow) - chröme, 2016-01-15 18:50:04]

https://codereview.chromium.org/1596523005/diff/20001/content/renderer/media/...
File content/renderer/media/webrtc_audio_renderer.cc (right):

https://codereview.chromium.org/1596523005/diff/20001/content/renderer/media/...
content/renderer/media/webrtc_audio_renderer.cc:455: // Pull the data we shall
deliver.
nit: s/shall/will

https://codereview.chromium.org/1596523005/diff/20001/content/renderer/media/...
content/renderer/media/webrtc_audio_renderer.cc:653:
media::ChannelLayoutToChannelCount(media::CHANNEL_LAYOUT_STEREO));
new assumption or did the previous code assume this?

[Henrik Grunell, 2016-01-18 13:00:54]

Per, please review too.

Tommi & Per: please verify that I got the fifo delay calculation correct.

https://codereview.chromium.org/1596523005/diff/20001/content/renderer/media/...
File content/renderer/media/webrtc_audio_renderer.cc (right):

https://codereview.chromium.org/1596523005/diff/20001/content/renderer/media/...
content/renderer/media/webrtc_audio_renderer.cc:455: // Pull the data we shall
deliver.
On 2016/01/15 18:50:03, tommi-chromium wrote:
> nit: s/shall/will

Done.

https://codereview.chromium.org/1596523005/diff/20001/content/renderer/media/...
content/renderer/media/webrtc_audio_renderer.cc:653:
media::ChannelLayoutToChannelCount(media::CHANNEL_LAYOUT_STEREO));
On 2016/01/15 18:50:03, tommi-chromium wrote:
> new assumption or did the previous code assume this?

Actually, the assumption is that source and sink operates on the same. But the
source params is always initialized with the sink params' channel layout. In
addition, the sink layout is never changed. So this assert is unnecessary.
Though the code in this function is a bit hard to read. Removed.

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

Hi,
I have quite a lot of newbie questions. Sorry for that, but as I'm new to the
code I need to get some more understanding of this in order to be able to verify
that it works correctly.

https://codereview.chromium.org/1596523005/diff/40001/content/renderer/media/...
File content/renderer/media/webrtc_audio_renderer.cc (right):

https://codereview.chromium.org/1596523005/diff/40001/content/renderer/media/...
content/renderer/media/webrtc_audio_renderer.cc:440: if (!audio_fifo_ &&
frames_skipped != frames_per_10ms) {
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.

https://codereview.chromium.org/1596523005/diff/40001/content/renderer/media/...
content/renderer/media/webrtc_audio_renderer.cc:441: audio_fifo_.reset(new
media::AudioPullFifo(
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?

https://codereview.chromium.org/1596523005/diff/40001/content/renderer/media/...
content/renderer/media/webrtc_audio_renderer.cc:450:
audio_fifo_->Consume(drop_bus.get(), drop_bus->frames());
How does this work? Does it consume audio_fifo_->SizeInFrames() or
drop_bus->frames() frames?

https://codereview.chromium.org/1596523005/diff/40001/content/renderer/media/...
content/renderer/media/webrtc_audio_renderer.cc:452: SourceCallback(0,
drop_bus.get());
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?

https://codereview.chromium.org/1596523005/diff/40001/content/renderer/media/...
content/renderer/media/webrtc_audio_renderer.cc:651:
scoped_ptr<media::AudioPullFifo> new_audio_fifo;
This seems to be unused after the change. Is that correct?

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

Answering questions and one fix.

https://codereview.chromium.org/1596523005/diff/40001/content/renderer/media/...
File content/renderer/media/webrtc_audio_renderer.cc (right):

https://codereview.chromium.org/1596523005/diff/40001/content/renderer/media/...
content/renderer/media/webrtc_audio_renderer.cc:440: if (!audio_fifo_ &&
frames_skipped != frames_per_10ms) {
On 2016/01/19 10:13:52, peah wrote:
> 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.

Yes, the fifo will handle that.

https://codereview.chromium.org/1596523005/diff/40001/content/renderer/media/...
content/renderer/media/webrtc_audio_renderer.cc:441: audio_fifo_.reset(new
media::AudioPullFifo(
On 2016/01/19 10:13:52, peah wrote:
> 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.

The |source_params| frames_per_buffer always corresponds to 10 ms (line 638).

> Will not this cause the FIFO to be recreated each time there is a frame skip
not
> equal to frames_per_10ms in length?

In the condition there is !audio_fifo_, so it will only be created is not
exists.

> And the next time PrepareSink is called it
> is again recreated if it does not happen to be the same length, right?

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.

> 
> 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?

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.

https://codereview.chromium.org/1596523005/diff/40001/content/renderer/media/...
content/renderer/media/webrtc_audio_renderer.cc:450:
audio_fifo_->Consume(drop_bus.get(), drop_bus->frames());
On 2016/01/19 10:13:52, peah wrote:
> How does this work? Does it consume audio_fifo_->SizeInFrames() or
> drop_bus->frames() frames?

It consumes drop_bus->frames() frames.

https://codereview.chromium.org/1596523005/diff/40001/content/renderer/media/...
content/renderer/media/webrtc_audio_renderer.cc:452: SourceCallback(0,
drop_bus.get());
On 2016/01/19 10:13:52, peah wrote:
> To me this is a bit strange. Please correct me if I get this wrong!

I sure will. ;)

> 
> 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)

It will drop |frames_skipped| frames. (Size of |drop_bus|.) Otherwise correct.

> else if (!audio_fifo_ && frames_skipped == frames_per_10ms)
>   Variant B:
>   -Way of dropping samples: Call to Sourcecallback
>   -Number of samples dropped: frames_skipped

Correct.

> else if (audio_fifo_)
>   Variant C:
>    -Way of dropping samples: audio_fifo_ callback.
>    -Number of samples dropped: audio_fifo_.length

It will drop |frames_skipped| frames. (Size of |drop_bus|.) Otherwise correct.

> 
> 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?

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.

https://codereview.chromium.org/1596523005/diff/40001/content/renderer/media/...
content/renderer/media/webrtc_audio_renderer.cc:651:
scoped_ptr<media::AudioPullFifo> new_audio_fifo;
On 2016/01/19 10:13:52, peah wrote:
> This seems to be unused after the change. Is that correct?

Very much so. Removed.

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

https://codereview.chromium.org/1596523005/diff/40001/content/renderer/media/...
File content/renderer/media/webrtc_audio_renderer.cc (right):

https://codereview.chromium.org/1596523005/diff/40001/content/renderer/media/...
content/renderer/media/webrtc_audio_renderer.cc:440: if (!audio_fifo_ &&
frames_skipped != frames_per_10ms) {
On 2016/01/19 13:03:41, Henrik Grunell wrote:
> On 2016/01/19 10:13:52, peah wrote:
> > 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.
> 
> Yes, the fifo will handle that.

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]

https://codereview.chromium.org/1596523005/diff/40001/content/renderer/media/...
File content/renderer/media/webrtc_audio_renderer.cc (right):

https://codereview.chromium.org/1596523005/diff/40001/content/renderer/media/...
content/renderer/media/webrtc_audio_renderer.cc:440: if (!audio_fifo_ &&
frames_skipped != frames_per_10ms) {
On 2016/01/20 15:16:47, peah wrote:
> On 2016/01/19 13:03:41, Henrik Grunell wrote:
> > On 2016/01/19 10:13:52, peah wrote:
> > > 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.
> > 
> > Yes, the fifo will handle that.
> 
> 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?

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.

> 
> 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)?

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.

> 
> 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?

Yes, it's always 10 ms.

> 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?

Yes, that's 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.

Yep, both are fulfilled.

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

https://codereview.chromium.org/1596523005/diff/40001/content/renderer/media/...
File content/renderer/media/webrtc_audio_renderer.cc (right):

https://codereview.chromium.org/1596523005/diff/40001/content/renderer/media/...
content/renderer/media/webrtc_audio_renderer.cc:440: if (!audio_fifo_ &&
frames_skipped != frames_per_10ms) {
On 2016/01/21 08:55:46, Henrik Grunell wrote:
> On 2016/01/20 15:16:47, peah wrote:
> > On 2016/01/19 13:03:41, Henrik Grunell wrote:
> > > On 2016/01/19 10:13:52, peah wrote:
> > > > 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.
> > > 
> > > Yes, the fifo will handle that.
> > 
> > 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?
> 
> 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.
> 
> > 
> > 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)?
> 
> 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.
> 
> > 
> > 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?
> 
> Yes, it's always 10 ms.
> 
> > 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?
> 
> Yes, that's 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.
> 
> Yep, both are fulfilled.

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?

https://codereview.chromium.org/1596523005/diff/40001/content/renderer/media/...
content/renderer/media/webrtc_audio_renderer.cc:441: audio_fifo_.reset(new
media::AudioPullFifo(
On 2016/01/19 13:03:41, Henrik Grunell wrote:
> On 2016/01/19 10:13:52, peah wrote:
> > 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.
> 
> The |source_params| frames_per_buffer always corresponds to 10 ms (line 638).
> 
> > Will not this cause the FIFO to be recreated each time there is a frame skip
> not
> > equal to frames_per_10ms in length?
> 
> In the condition there is !audio_fifo_, so it will only be created is not
> exists.
> 
> > And the next time PrepareSink is called it
> > is again recreated if it does not happen to be the same length, right?
> 
> 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.
> 
> > 
> > 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?
> 
> 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.

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.

[peah, 2016-01-21 11:47:18]

After offline discussions about how the FIFO works, I'm convinced this properly
compensates the AEC for the lost samples by building up an unavoidable buffering
delay (unavoidable in that it will always be present in the roundtrip delay
regardless of where it happens).

Great work!

lgtm

[tommi (sloooow) - chröme, 2016-01-21 11:51:12]

lgtm

https://codereview.chromium.org/1596523005/diff/60001/media/base/audio_pull_f...
File media/base/audio_pull_fifo.cc (right):

https://codereview.chromium.org/1596523005/diff/60001/media/base/audio_pull_f...
media/base/audio_pull_fifo.cc:49: int AudioPullFifo::SizeInFrames() {
can this method be const?

[Henrik Grunell, 2016-01-21 12:26:17]

I did some cleaning in the existing code since it was difficult to follow when
setting up parameters. (Patch 4:6.)

Tommi: ptal.

https://codereview.chromium.org/1596523005/diff/40001/content/renderer/media/...
File content/renderer/media/webrtc_audio_renderer.cc (right):

https://codereview.chromium.org/1596523005/diff/40001/content/renderer/media/...
content/renderer/media/webrtc_audio_renderer.cc:440: if (!audio_fifo_ &&
frames_skipped != frames_per_10ms) {
On 2016/01/21 09:16:25, peah wrote:
> On 2016/01/21 08:55:46, Henrik Grunell wrote:
> > On 2016/01/20 15:16:47, peah wrote:
> > > On 2016/01/19 13:03:41, Henrik Grunell wrote:
> > > > On 2016/01/19 10:13:52, peah wrote:
> > > > > 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.
> > > > 
> > > > Yes, the fifo will handle that.
> > > 
> > > 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?
> > 
> > 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.
> > 
> > > 
> > > 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)?
> > 
> > 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.
> > 
> > > 
> > > 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?
> > 
> > Yes, it's always 10 ms.
> > 
> > > 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?
> > 
> > Yes, that's 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.
> > 
> > Yep, both are fulfilled.
> 
> 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? 
> 

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

https://codereview.chromium.org/1596523005/diff/40001/content/renderer/media/...
content/renderer/media/webrtc_audio_renderer.cc:441: audio_fifo_.reset(new
media::AudioPullFifo(
On 2016/01/21 09:16:25, peah wrote:
> On 2016/01/19 13:03:41, Henrik Grunell wrote:
> > On 2016/01/19 10:13:52, peah wrote:
> > > 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.
> > 
> > The |source_params| frames_per_buffer always corresponds to 10 ms (line
638).
> > 
> > > Will not this cause the FIFO to be recreated each time there is a frame
skip
> > not
> > > equal to frames_per_10ms in length?
> > 
> > In the condition there is !audio_fifo_, so it will only be created is not
> > exists.
> > 
> > > And the next time PrepareSink is called it
> > > is again recreated if it does not happen to be the same length, right?
> > 
> > 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.
> > 
> > > 
> > > 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?
> > 
> > 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.
> 
> 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.
> 
> 

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.

https://codereview.chromium.org/1596523005/diff/60001/media/base/audio_pull_f...
File media/base/audio_pull_fifo.cc (right):

https://codereview.chromium.org/1596523005/diff/60001/media/base/audio_pull_f...
media/base/audio_pull_fifo.cc:49: int AudioPullFifo::SizeInFrames() {
On 2016/01/21 11:51:12, tommi-chromium wrote:
> can this method be const?

Yep. Done.

[Henrik Grunell, 2016-01-21 12:28:14]

The CQ bit was checked by grunell@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-01-21 12:28:52]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1596523005/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1596523005/100001

[tommi (sloooow) - chröme, 2016-01-21 12:40:15]

lgtm

[Henrik Grunell, 2016-01-21 13:00:21]

The CQ bit was unchecked by grunell@chromium.org

[Henrik Grunell, 2016-01-21 13:00:27]

The CQ bit was checked by grunell@chromium.org

[Henrik Grunell, 2016-01-21 13:00:28]

The patchset sent to the CQ was uploaded after l-g-t-m from
dalecurtis@chromium.org, peah@chromium.org
Link to the patchset: https://codereview.chromium.org/1596523005/#ps100001
(title: "Code review (tommi@)")

[commit-bot: I haz the power, 2016-01-21 13:00:42]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1596523005/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1596523005/100001

[commit-bot: I haz the power, 2016-01-21 13:46:16]

Description was changed from

==========
Drop WebRTC audio data if OS has skipped frames.

This ensures that we get what is actually played out with what we feed to the
echo canceler as far-end data back in sync as quickly as possible after a skip,
which in turn reduces the risk of echo. We drop data in
WebRtcAudioRenderer::Render.

The fifo is created (if it doesn't exist) if the number of skipped frames is not
10 ms of data.

This CL also removes creating a new fifo unecessarily if only the sink's frames
per buffer changes but not the source's (when they differ).

BUG=560371
==========

to

==========
Drop WebRTC audio data if OS has skipped frames.

This ensures that we get what is actually played out with what we feed to the
echo canceler as far-end data back in sync as quickly as possible after a skip,
which in turn reduces the risk of echo. We drop data in
WebRtcAudioRenderer::Render.

The fifo is created (if it doesn't exist) if the number of skipped frames is not
10 ms of data.

This CL also removes creating a new fifo unecessarily if only the sink's frames
per buffer changes but not the source's (when they differ).

BUG=560371
==========

[commit-bot: I haz the power, 2016-01-21 13:46:16]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-01-21 13:47:25]

Description was changed from

==========
Drop WebRTC audio data if OS has skipped frames.

This ensures that we get what is actually played out with what we feed to the
echo canceler as far-end data back in sync as quickly as possible after a skip,
which in turn reduces the risk of echo. We drop data in
WebRtcAudioRenderer::Render.

The fifo is created (if it doesn't exist) if the number of skipped frames is not
10 ms of data.

This CL also removes creating a new fifo unecessarily if only the sink's frames
per buffer changes but not the source's (when they differ).

BUG=560371
==========

to

==========
Drop WebRTC audio data if OS has skipped frames.

This ensures that we get what is actually played out with what we feed to the
echo canceler as far-end data back in sync as quickly as possible after a skip,
which in turn reduces the risk of echo. We drop data in
WebRtcAudioRenderer::Render.

The fifo is created (if it doesn't exist) if the number of skipped frames is not
10 ms of data.

This CL also removes creating a new fifo unecessarily if only the sink's frames
per buffer changes but not the source's (when they differ).

BUG=560371

Committed: https://crrev.com/b4bc1f2da7414fcbc7d18d860dd7c8260dd9c891
Cr-Commit-Position: refs/heads/master@{#370686}
==========

[commit-bot: I haz the power, 2016-01-21 13:47:26]

Patchset 6 (id:??) landed as
https://crrev.com/b4bc1f2da7414fcbc7d18d860dd7c8260dd9c891
Cr-Commit-Position: refs/heads/master@{#370686}

[o1ka, 2016-01-22 13:15:02]

olka@chromium.org changed reviewers:
+ olka@chromium.org

[o1ka, 2016-01-22 13:15:03]

Henrik, could you take a look at my question?
Thanks, Olga

https://codereview.chromium.org/1596523005/diff/100001/content/renderer/media...
File content/renderer/media/webrtc_audio_renderer.cc (right):

https://codereview.chromium.org/1596523005/diff/100001/content/renderer/media...
content/renderer/media/webrtc_audio_renderer.cc:659: (audio_fifo_ &&
So in certain cases an existing fifo is reset. Doesn't it mean that some frames
can be dropped, which will result in a glitch?
What is the scenario when source buffer size changes?

[Henrik Grunell, 2016-01-25 11:56:23]

https://codereview.chromium.org/1596523005/diff/100001/content/renderer/media...
File content/renderer/media/webrtc_audio_renderer.cc (right):

https://codereview.chromium.org/1596523005/diff/100001/content/renderer/media...
content/renderer/media/webrtc_audio_renderer.cc:659: (audio_fifo_ &&
On 2016/01/22 13:15:03, o1ka wrote:
> So in certain cases an existing fifo is reset. Doesn't it mean that some
frames
> can be dropped, which will result in a glitch?
> What is the scenario when source buffer size changes?

[We talked about this offline, summarizing here.]
Yes, some data will be dropped and we'll get a glitch. It was the case before as
well, but now we only recreate the fifo when its size has actually changed. The
only case we recreate the fifo is when the source buffer size changes, which
only happens when the sample rate changes. It won't happen often, and when it
does the user will likely expect/accept audio artifacts anyway.

[Henrik Grunell, 2016-04-05 08:31:08]

Description was changed from

==========
Drop WebRTC audio data if OS has skipped frames.

This ensures that we get what is actually played out with what we feed to the
echo canceler as far-end data back in sync as quickly as possible after a skip,
which in turn reduces the risk of echo. We drop data in
WebRtcAudioRenderer::Render.

The fifo is created (if it doesn't exist) if the number of skipped frames is not
10 ms of data.

This CL also removes creating a new fifo unecessarily if only the sink's frames
per buffer changes but not the source's (when they differ).

BUG=560371

Committed: https://crrev.com/b4bc1f2da7414fcbc7d18d860dd7c8260dd9c891
Cr-Commit-Position: refs/heads/master@{#370686}
==========

to

==========
Drop WebRTC audio data if OS has skipped frames.

This ensures that we get what is actually played out with what we feed to the
echo canceler as far-end data back in sync as quickly as possible after a skip,
which in turn reduces the risk of echo. We drop data in
WebRtcAudioRenderer::Render.

The fifo is created (if it doesn't exist) if the number of skipped frames is not
10 ms of data.

This CL also removes creating a new fifo unecessarily if only the sink's frames
per buffer changes but not the source's (when they differ).

BUG=560371

Committed: https://crrev.com/b4bc1f2da7414fcbc7d18d860dd7c8260dd9c891
Cr-Commit-Position: refs/heads/master@{#370686}
==========