Closed Bug 1051658 Opened 5 years ago Closed 5 years ago

Handle AudioStream Init failures in AudioSink

Categories

(Core :: Audio/Video, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla35

People

(Reporter: jwwang, Assigned: jwwang)

References

Details

Attachments

(3 files, 3 obsolete files)

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.
Blocks: 1050947
No longer blocks: 1050947
Blocks: 1050947
Blocks: 1055091
Blocks: 1055504
(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 :-)
(To avoid us filing multiple dupes)
Blocks: 929286
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?
(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 :-)
No longer blocks: 1050947
Blocks: 916399
Summary: Handle AudioStream Init failures → Handle AudioStream Init failures in AudioSink
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.
Assignee: nobody → jwwang
Status: NEW → ASSIGNED
Attachment #8496724 - Flags: review?(cpearce)
See comment 0.

Notify decode error when AudioSink init failed.
Attachment #8496725 - Flags: review?(kinetik)
Attachment #8496724 - Flags: review?(cpearce)
Attachment #8496725 - Flags: review?(kinetik)
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.
Attachment #8496724 - Attachment is obsolete: true
Attachment #8496725 - Attachment is obsolete: true
See comment 0.

Notify decode error when AudioSink init failed.
Wrong files uploaded...sorry.
Matthew Gregan should review this.
Attachment #8496727 - Flags: review?(kinetik)
Attachment #8496728 - Flags: review?(kinetik)
Try: https://tbpl.mozilla.org/?tree=Try&rev=62191f1dba28
Most green. Looks like it didn't break anything.
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."
Attachment #8496727 - Flags: review?(kinetik) → review+
Attachment #8496728 - Flags: review?(kinetik) → review+
Fix nits suggested by Matthew.
Attachment #8496727 - Attachment is obsolete: true
Attachment #8497197 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/aaae54a77270
https://hg.mozilla.org/mozilla-central/rev/12a04a66d5c1
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
(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
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(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.
They were mis-stars, it's a different failure mode it would seem.
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
Attachment #8498578 - Flags: review?(kinetik)
Attachment #8498578 - Flags: review?(kinetik) → review+
Keywords: checkin-needed
Part 3 should fix the oranges on OSX 10.6. Please try to check in the patches again. Thanks.
You need to log in before you can comment on or make changes to this bug.