1051658 - Handle AudioStream Init failures in AudioSink

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[JW Wang [:jwwang] [:jw_wang]]

Sometimes AudioStream fails to init properly due to failures in the backend and causes failures in media tests.

(see bug 1050947 comment 1)

I can think of 2 options here:

1. report error when AudioStream fails to init so the test script can listen to 'error' event and handle that. (It would still be a test failure thought...)

2. create a dummy AudioStream so that the playback can continue and tests can pass. However, it doesn't make much sense for the production code.

Comment 1

[Ed Morley [:emorley]]

(In reply to JW Wang [:jwwang] from comment #0)
> Sometimes AudioStream fails to init properly due to failures in the backend
> and causes failures in media tests.
> 
> (see bug 1050947 comment 1)
> 
> I can think of 2 options here:
> 
> 1. report error when AudioStream fails to init so the test script can listen
> to 'error' event and handle that. (It would still be a test failure
> thought...)

This would at least help with the identification of failures being due to this bug :-)

Comment 2

[Ed Morley [:emorley]]

(To avoid us filing multiple dupes)

Comment 3

[JW Wang [:jwwang] [:jw_wang]]

You can refer to bug 929286 comment 74 for an example where Windows failed to initialize an audio client and resulted in AudioStream failed to init.

I've mark block bugs when I saw such warning messages.

How about escalating the warning message to an assertion so that it is more test suite friendly to identify the error?

Comment 4

[Ed Morley [:emorley]]

(In reply to JW Wang [:jwwang] from comment #3)
> How about escalating the warning message to an assertion so that it is more
> test suite friendly to identify the error?

Sounds good to me :-)

Comment 5

[JW Wang [:jwwang] [:jw_wang]]

Attachment: 1038091_fix_cubeb_opensl_drain_positioin-v1.patch

See comment 0.

Add OnAudioSinkError() to MediaDecoderStateMachine to handle AudioSink init failure and send a decode error the to media element. This will allow the test cases to handle media errors more gracefully rather than failing with unknown timeout.

Comment 6

[JW Wang [:jwwang] [:jw_wang]]

Attachment: 1038091_fix_cubeb_opensl_drain_positioin-v2.patch

See comment 0.

Notify decode error when AudioSink init failed.

Comment 7

[JW Wang [:jwwang] [:jw_wang]]

Attachment: 1051658_handle_AudioStream_init_fail_part1-v1.patch

See comment 0.

Add OnAudioSinkError() to MediaDecoderStateMachine to handle AudioSink init failure and send a decode error the to media element. This will allow the test cases to handle media errors more gracefully rather than failing with unknown timeout.

Comment 8

[JW Wang [:jwwang] [:jw_wang]]

Attachment: 1051658_handle_AudioStream_init_fail_part2-v1.patch

See comment 0.

Notify decode error when AudioSink init failed.

Comment 9

[JW Wang [:jwwang] [:jw_wang]]

Wrong files uploaded...sorry.

Comment 10

[Chris Pearce [:cpearce (Not reading bugmail)]]

Matthew Gregan should review this.

Comment 11

[JW Wang [:jwwang] [:jw_wang]]

Try: https://tbpl.mozilla.org/?tree=Try&rev=62191f1dba28
Most green. Looks like it didn't break anything.

Comment 12

[Matthew Gregan [:kinetik]]

Comment on attachment 8496727 [details] [diff] [review]
1051658_handle_AudioStream_init_fail_part1-v1.patch

Review of attachment 8496727 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/media/MediaDecoderStateMachine.cpp
@@ +1793,5 @@
>      mAudioCompleted = false;
> +    mAudioSink = new AudioSink(this, mAudioStartTime,
> +                               mInfo.mAudio, mDecoder->GetAudioChannel());
> +    // OnAudioSinkError() will be called by AudioSink, should it be any error
> +    // during initialization.

So Init() will return an NS_ERROR *and* OnAudioSinkError will be called?

Maybe reword the comment to say something like:
"OnAudioSinkError() will be called before Init() returns if an error occurs during initialization."

@@ +3123,5 @@
>  
> +void MediaDecoderStateMachine::OnAudioSinkError()
> +{
> +  AssertCurrentThreadInMonitor();
> +  // Ignore the error when captured for AudioSink is no longer relevant.

Maybe:
"AudioSink is not used with captured streams, so ignore errors in this case."

Comment 13

[JW Wang [:jwwang] [:jw_wang]]

Attachment: 1051658_handle_AudioStream_init_fail_part1-v2.patch

Fix nits suggested by Matthew.

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/aaae54a77270
https://hg.mozilla.org/integration/mozilla-inbound/rev/12a04a66d5c1

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/aaae54a77270
https://hg.mozilla.org/mozilla-central/rev/12a04a66d5c1

Comment 16

[Ed Morley [:emorley]]

(In reply to JW Wang [:jwwang] from comment #11)
> Try: https://tbpl.mozilla.org/?tree=Try&rev=62191f1dba28
> Most green. Looks like it didn't break anything.

This wasn't green unfortunately - 3 out of 4 of "Rev4 MacOSX Snow Leopard 10.6 try opt test mochitest-1" had the same failure, and this failure is also now occurring on inbound.

Reverted:
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/2ec523ee7bd2
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/2ea087cf4b39

Comment 17

[JW Wang [:jwwang] [:jw_wang]]

(In reply to Ed Morley [:edmorley] from comment #16)
> (In reply to JW Wang [:jwwang] from comment #11)
> > Try: https://tbpl.mozilla.org/?tree=Try&rev=62191f1dba28
> > Most green. Looks like it didn't break anything.
> 
> This wasn't green unfortunately - 3 out of 4 of "Rev4 MacOSX Snow Leopard
> 10.6 try opt test mochitest-1" had the same failure, and this failure is
> also now occurring on inbound.

Do you mean the peaks of bug 832768 comment 346 ~ bug 832768 comment 361? They all failed in OSX 10.6 with small-shot.m4a not finished. I wonder if it has something to do with the MP4Reader.

Comment 18

[Ed Morley [:emorley]]

They were mis-stars, it's a different failure mode it would seem.

Comment 19

[JW Wang [:jwwang] [:jw_wang]]

Attachment: 1051658_handle_AudioStream_init_fail_part3-v1.patch

AudioSink::GetPosition() returns -1 when AudioStream init fails. And MediaDecoderStateMachine will switch to system clock when AudioSink::GetPosition() returns -1. Therefore test cases can still run well even with AudioStream init falures. However, this patch turns AudioStream init failure into a decode error and cause test case to fail.

After some investigation, the AudioStream init failures are caused by the limit of acitve cubeb streams on OSX 10.6. For now, the limit is 8 and it is easy to reach the limit within a single test case if more than 8 AudioStream are opened concurrently.

Fix:
1. Release decoders (which will close AudioStreams) more aggressively.
2. Call SpecialPowers.exactGC() to ensure all resources are cleaned up before proceeding to next test case.

Note, per comment 17, it just happens to fail in small-shot.m4a due to the fixed order of test cases. The opened cubeb streams are not closed in the previous test case and test_bug465498.html just reaches the limit.

Try: https://tbpl.mozilla.org/?tree=Try&rev=a85829463f91

Comment 20

[JW Wang [:jwwang] [:jw_wang]]

Part 3 should fix the oranges on OSX 10.6. Please try to check in the patches again. Thanks.

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5c0a0cf825b8
https://hg.mozilla.org/integration/mozilla-inbound/rev/423127691c48
https://hg.mozilla.org/integration/mozilla-inbound/rev/9e8624c37de0

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/5c0a0cf825b8
https://hg.mozilla.org/mozilla-central/rev/423127691c48
https://hg.mozilla.org/mozilla-central/rev/9e8624c37de0