Use audio shifter instead of a fifo for local mediastream playback.

Use audio shifter instead of a fifo for local mediastream playback.

There are two reasons for this change:

1. The current code path doesn't work correctly if the media stream and the output devices clocks don't match up *exactly*. Underruns/lipsync issues can occur.

2. This allows for audio to be explicitly buffered for some period of time, which I plan to utilize in the cast_streaming receiver code.

Currently this code path is not often used, since it only happens when you get a media stream from getUserMedia and plug it into a media player without going through webrtc.

Committed: https://crrev.com/0650f24c76ebe1e30552f655c417668a1ba32518
Cr-Commit-Position: refs/heads/master@{#313193}

[hubbe, 2015-01-16 19:29:36]

hubbe@chromium.org changed reviewers:
+ hclam@google.com

[Alpha Left Google, 2015-01-20 21:46:14]

hclam@chromium.org changed reviewers:
+ hclam@chromium.org
- hclam@google.com

[Alpha Left Google, 2015-01-20 21:51:03]

hclam@chromium.org changed reviewers:
+ henrika@chromium.org, perkj@chromium.org

[Alpha Left Google, 2015-01-20 21:51:03]

I don't think I'm the best person to review this. Someone from WebRTC is a much
better fit.

Adding henrika@ and perkj@ to help.

I have one comment about the CL description though. It should mention what
project / area / class that it is changing. Also the rationale of making this
change.

[hubbe, 2015-01-20 21:56:34]

hubbe@chromium.org changed reviewers:
+ dalecurtis@chromium.org

[henrika (OOO until Aug 14), 2015-01-21 08:54:30]

Thanks hubbe. As you say, this code is not essential for the WebRTC stack today
but we have one demo that you can try out:
http://googlechrome.github.io/webrtc/samples/web/content/getusermedia/audio/.

It all looks good to me but I would like to understand some more about what the
audio shifter does and how it was tuned here.

I am OK as is and can say LGTM but would still appreciate some more details to
learn and understand better.

[henrika (OOO until Aug 14), 2015-01-21 08:54:38]

https://codereview.chromium.org/856843002/diff/1/content/renderer/media/webrt...
File content/renderer/media/webrtc_local_audio_renderer.cc (right):

https://codereview.chromium.org/856843002/diff/1/content/renderer/media/webrt...
content/renderer/media/webrtc_local_audio_renderer.cc:44: audio_shifter_->Pull(
What effect does the provided time value have here?

https://codereview.chromium.org/856843002/diff/1/content/renderer/media/webrt...
content/renderer/media/webrtc_local_audio_renderer.cc:285: media::AudioShifter*
const new_shifter = new media::AudioShifter(
It is not clear to me why you use 2 and 20 here. Care to comment about who they
were selected?

[hubbe, 2015-01-23 00:54:56]

https://codereview.chromium.org/856843002/diff/1/content/renderer/media/webrt...
File content/renderer/media/webrtc_local_audio_renderer.cc (right):

https://codereview.chromium.org/856843002/diff/1/content/renderer/media/webrt...
content/renderer/media/webrtc_local_audio_renderer.cc:44: audio_shifter_->Pull(
On 2015/01/21 08:54:38, henrika wrote:
> What effect does the provided time value have here?

The audio shifter tries to provide audio for the given timestamp.
There is a fair amount of comments in audio_shifter.h, but the gist of
it is that Push() can specify exactly when a piece of audio should be played,
and Pull() can specify when it intends to play the pulled audio and audio
shifter will take care of doing the right amount of buffering/stretching.

If Push() uses timestamps that are int he past, audio shifter tries to use the
smallest amount of buffering it can that will still result in smooth playout.

https://codereview.chromium.org/856843002/diff/1/content/renderer/media/webrt...
content/renderer/media/webrtc_local_audio_renderer.cc:285: media::AudioShifter*
const new_shifter = new media::AudioShifter(
On 2015/01/21 08:54:38, henrika wrote:
> It is not clear to me why you use 2 and 20 here. Care to comment about who
they
> were selected?

2 was selected because we need at least 1 second for cast.
Note that if push timestamps are in the past, a minimal buffer
will be used regardless of this argument

The 20 millseconds comes from the fact that windows clocks tend to have a 15
millsecond accuracy.

(Comment added)

[henrika (OOO until Aug 14), 2015-01-23 08:37:37]

Should have checked the AudioShifter header. Looks like great work, thanks!

[hubbe, 2015-01-23 19:18:02]

The CQ bit was checked by hubbe@chromium.org

[commit-bot: I haz the power, 2015-01-23 19:19:34]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/856843002/20001

[commit-bot: I haz the power, 2015-01-23 19:29:29]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-01-23 19:29:29]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[hubbe, 2015-01-23 19:34:23]

On 2015/01/23 08:37:37, henrika wrote:
> Should have checked the AudioShifter header. Looks like great work, thanks!

Still needs OWNER approval, dale?

[DaleCurtis, 2015-01-27 01:20:49]

rs lgtm

[hubbe, 2015-01-27 01:21:07]

The CQ bit was checked by hubbe@chromium.org

[commit-bot: I haz the power, 2015-01-27 01:22:46]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/856843002/20001

[commit-bot: I haz the power, 2015-01-27 01:23:34]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-01-27 01:23:35]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[hubbe, 2015-01-27 01:28:27]

The CQ bit was checked by hubbe@chromium.org

[commit-bot: I haz the power, 2015-01-27 01:29:03]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/856843002/20001

[commit-bot: I haz the power, 2015-01-27 01:30:38]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2015-01-27 01:32:21]

Patchset 2 (id:??) landed as
https://crrev.com/0650f24c76ebe1e30552f655c417668a1ba32518
Cr-Commit-Position: refs/heads/master@{#313193}