Detect errors in audio output and report them upstream.

Detect errors in audio output and report them upstream.
Stop feeding audio packets to AUDIO DemuxerStreams once audio has been disabled.

BUG=111409
TEST=uninstall pulseaudio, make /dev/snd inaccessible, and observe <video> plays correctly (muted) instead of hanging.


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=119488

[Ami GONE FROM CHROMIUM, 2012-01-26 19:17:58]

scherkus: please review

crogers: please review and note that I gave you a TODO in this CL.  If you
believe this is the wrong approach for webaudio please let me know.

henrika: FYI since I gave you a TODO in this CL.  I based this on the fact that
you wrote most of the related file; if there's someone else I should have given
it to please let me know, or if you believe this approach is wrong for webrtc
please let me know.

[Chris Rogers, 2012-01-26 19:31:02]

Ami, this looks fine to me for the OnError() part.  I'm not yet quite sure how
we would handle the error, but now we have the ability to do so...

On 2012/01/26 19:17:58, Ami Fischman wrote:
> scherkus: please review
> 
> crogers: please review and note that I gave you a TODO in this CL.  If you
> believe this is the wrong approach for webaudio please let me know.
> 
> henrika: FYI since I gave you a TODO in this CL.  I based this on the fact
that
> you wrote most of the related file; if there's someone else I should have
given
> it to please let me know, or if you believe this approach is wrong for webrtc
> please let me know.

[Ami GONE FROM CHROMIUM, 2012-01-26 20:19:17]

jam: please OWNERS-review for content/renderer/.

[scherkus (not reviewing), 2012-01-26 21:46:12]

https://chromiumcodereview.appspot.com/9234066/diff/2001/content/renderer/med...
File content/renderer/media/audio_renderer_impl.cc (right):

https://chromiumcodereview.appspot.com/9234066/diff/2001/content/renderer/med...
content/renderer/media/audio_renderer_impl.cc:253: if (host())
nit: according to the superclass host() should always be valid

a crash would be preferable to a hang

https://chromiumcodereview.appspot.com/9234066/diff/2001/media/filters/ffmpeg...
File media/filters/ffmpeg_demuxer.cc (right):

https://chromiumcodereview.appspot.com/9234066/diff/2001/media/filters/ffmpeg...
media/filters/ffmpeg_demuxer.cc:705: audio_disabled_ = true;
any reason not to use FFmpegDemuxerStream::stopped_ as the signal for dropping
packets / detecting end of stream?

https://chromiumcodereview.appspot.com/9234066/diff/2001/media/filters/ffmpeg...
File media/filters/ffmpeg_demuxer.h (right):

https://chromiumcodereview.appspot.com/9234066/diff/2001/media/filters/ffmpeg...
media/filters/ffmpeg_demuxer.h:266: bool audio_disabled_;
QQ: this change wasn't mentioned in the CL description

was there something wonky w/ the previous code path? (it wouldn't surprise me if
there was)

[Ami GONE FROM CHROMIUM, 2012-01-26 22:11:01]

https://chromiumcodereview.appspot.com/9234066/diff/2001/content/renderer/med...
File content/renderer/media/audio_renderer_impl.cc (right):

https://chromiumcodereview.appspot.com/9234066/diff/2001/content/renderer/med...
content/renderer/media/audio_renderer_impl.cc:253: if (host())
On 2012/01/26 21:46:13, scherkus wrote:
> nit: according to the superclass host() should always be valid
> 
> a crash would be preferable to a hang

Done.
I wonder why we have these checks sprinkled, then?
http://code.google.com/p/chromium/source/search?q="if+(host())"

https://chromiumcodereview.appspot.com/9234066/diff/2001/media/filters/ffmpeg...
File media/filters/ffmpeg_demuxer.cc (right):

https://chromiumcodereview.appspot.com/9234066/diff/2001/media/filters/ffmpeg...
media/filters/ffmpeg_demuxer.cc:705: audio_disabled_ = true;
On 2012/01/26 21:46:13, scherkus wrote:
> any reason not to use FFmpegDemuxerStream::stopped_ as the signal for dropping
> packets / detecting end of stream?

Just that that would be a more intrusive change, since it would have the demuxer
query the streams for their status, which is counter to the way most information
& control flows today.

https://chromiumcodereview.appspot.com/9234066/diff/2001/media/filters/ffmpeg...
File media/filters/ffmpeg_demuxer.h (right):

https://chromiumcodereview.appspot.com/9234066/diff/2001/media/filters/ffmpeg...
media/filters/ffmpeg_demuxer.h:266: bool audio_disabled_;
On 2012/01/26 21:46:13, scherkus wrote:
> QQ: this change wasn't mentioned in the CL description

Done.

> was there something wonky w/ the previous code path? (it wouldn't surprise me
if
> there was)

Yes; the AUDIO demuxerstream would be Stop()'d but then continue to receive
EnqueuePacket()s, triggering a NOTREACHED().

[scherkus (not reviewing), 2012-01-26 22:28:09]

LGTM

want to check w/ imasaki whether we have a manual test case for this? I seem to
remember him mentioning something...

https://chromiumcodereview.appspot.com/9234066/diff/2001/content/renderer/med...
File content/renderer/media/audio_renderer_impl.cc (right):

https://chromiumcodereview.appspot.com/9234066/diff/2001/content/renderer/med...
content/renderer/media/audio_renderer_impl.cc:253: if (host())
On 2012/01/26 22:11:01, Ami Fischman wrote:
> On 2012/01/26 21:46:13, scherkus wrote:
> > nit: according to the superclass host() should always be valid
> > 
> > a crash would be preferable to a hang
> 
> Done.
> I wonder why we have these checks sprinkled, then?
> http://code.google.com/p/chromium/source/search?q=%22if+%28host%28%29%29%22

AFAIK demuxers can execute w/o having a host present (at least chunkdemuxer can)
but the other ones are superfluous

[Ami GONE FROM CHROMIUM, 2012-01-26 22:43:14]

>
> want to check w/ imasaki whether we have a manual test case for this? I
> seem to
> remember him mentioning something...


Done.


> AFAIK demuxers can execute w/o having a host present (at least

chunkdemuxer can) but the other ones are superfluous


Will send follow-up CL for them then.

-a

[henrika (OOO until Aug 14), 2012-01-27 15:01:30]

Looks great, thanks. I'm sure we can use it in WebRTC as well.

I have a more general comment though. Hope I don't mess things up here. The
issue states:

"What is the expected output? What do you see instead?
Expect: audio tag pretends to play but with output going nowhere.
Actual: audio tag "hangs" - stays in "playing" mode but thumb stays put at
00:00."

and I don't really understand how this CL generates the expected result.

I assume you mean that an audio object with a controls attribute should keep
playing (<=> slider move) under silence? My guess is that what will happen here
is that "nothing will happen", i.e., playing will not start at all and no
onerror event will happen on the JavaScript layer.

[Ami GONE FROM CHROMIUM, 2012-01-27 17:55:31]

On 2012/01/27 15:01:30, henrika wrote:
> and I don't really understand how this CL generates the expected result.

media::Pipeline::DisableAudioRenderer() effectively replaces the
audio-output-based clock with an artificial clock which pushes time forward.

> I assume you mean that an audio object with a controls attribute should keep
> playing (<=> slider move) under silence? 

Correct, although the more valuable user-facing feature here is to make <video>
not stall/hang when audio output is unavailable.  I only filed the bug for audio
b/c that's the tag I first ran into the problem with.

> My guess is that what will happen here is that "nothing will happen", i.e.,
playing will not start at all 

See my TEST= line in the CL description.  It does work in practice :)

> and no onerror event will happen on the JavaScript layer.

The OnError() callback I added has nothing to do with the error event in JS.

[jam, 2012-01-27 18:19:48]

lgtm for content/renderer/renderer_webaudiodevice_impl.*

why not move these files to the media subdir so you don't need owners
rubberstamp?

[commit-bot: I haz the power, 2012-01-27 18:23:53]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/fischman@chromium.org/9234066/6013

[Ami GONE FROM CHROMIUM, 2012-01-27 18:24:15]

> why not move these files to the media subdir so you don't need owners
> rubberstamp?

SGTM; will send follow-up CL.

[commit-bot: I haz the power, 2012-01-27 20:36:11]

Change committed as 119488