1246108 - Drained AudioStreams should not be restarted

Status: RESOLVED FIXED

(Core :: Audio/Video: Playback, defect, P1)

Description

[Alastor Wu [:alwu]]

This bug is fork from bug 1242774.

The CUBEB_STATE_DRAINED should only be notified when the audio track is ended, the cubeb backend shouldn't notify CUBEB_STATE_DRAINED more than once.

STR.
1. Run the video-replay-after-audio-end.html (from bug1242774) on Window

Expected.
2. The CUBEB_STATE_DRAINED should only be notified once

Actual.
2. The CUBEB_STATE_DRAINED is notified twice

Comment 1

[Liz Henry (:lizzard) (relman/hg->git project)]

Sounds like this affects 46 and higher from the comments / flags in bug 1245386. 
If a fix for this lands and appears to fix the crash there, please request uplift to beta. Thanks!

Comment 2

[Maire Reavy [:mreavy]]

Matthew is taking this, but he's still coming up to speed.  We don't have an estimate on how quickly this can be fixed.

Adding JW to this bug in case he has additional info that can help Matthew come up to speed on this issue more quickly.  (Ref: https://bugzilla.mozilla.org/show_bug.cgi?id=1245386#c4 )

Comment 3

[Paul Adenot (:padenot)]

This might be fixed by all the new things in cubeb_wasapi, but we might need something for uplift.

Comment 4

[Matthew Gregan [:kinetik]]

(In reply to Maire Reavy [:mreavy] (Plz ni?:mreavy) from comment #2)
> Matthew is taking this, but he's still coming up to speed.  We don't have an
> estimate on how quickly this can be fixed.

I should have a fix to test tomorrow or so, I understand what's going on but need to decide on the correct approach to fix it.

Comment 5

[Matthew Gregan [:kinetik]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=01f0de390fcb

Comment 6

[Matthew Gregan [:kinetik]]

Attachment: bug1246108_v0.patch

The root cause of this bug, bug 1242774, and bug 1245386 is that DecodedAudioDataSink tries to restart an AudioStream that has completed and drained.  That's not permitted by libcubeb, although the API doesn't defend itself against it in all implementations, which is why we were seeing strange behaviour such as multiple calls to Drained().

This is a regression from bug 948267.  That change removed the state machine from DADS which protected calls to AudioStream::Pause/Resume by returning early once mState was not PLAYING.

This simple fix intended for uplift restores part of that state machine by tracking the drained state of the DADS and returning early from DADS::SetPlaying when the DADS has drained.

Comment 8

[Matthew Gregan [:kinetik]]

Attachment: bug1246108_v0_tests.patch

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2a88e09b231d

Comment 9

[Matthew Gregan [:kinetik]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6505e8c631d6

Comment 10

[Matthew Gregan [:kinetik]]

Try pushed got a base base, retrying:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=21a2786fa245

Comment 11

[Liz Henry (:lizzard) (relman/hg->git project)]

This is the #5 topcrash for 46 beta 2.  
kinetik, if you do land a fix for this, please request uplift to beta, thanks!

Comment 12

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

Comment on attachment 8733123 [details] [diff] [review]
bug1246108_v0.patch

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

::: dom/media/mediasink/DecodedAudioDataSink.cpp
@@ +126,5 @@
>    // pause/resume AudioStream as necessary.
>    if (!aPlaying && !mAudioStream->IsPaused()) {
>      mAudioStream->Pause();
>    } else if (aPlaying && mAudioStream->IsPaused()) {
>      mAudioStream->Resume();

It appears to me that the race is still there and |mAudioStream->Resume()| could still happen after draining with the following call flow:

1. mPlaybackComplete is false on the state machine thread.
2. context switch to cubeb thread to run DecodedAudioDataSink::Drained().
3. context switch to the state machine thread to call |mAudioStream->Resume()|.

However, I believe this patch will reduce the chance of crash greatly and we need to uplift to beta as soon as possible.

Please file a follow-up bug if necessary.

Comment 13

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e9ada6525c25
https://hg.mozilla.org/integration/mozilla-inbound/rev/ab302d19ab51

Comment 14

[Matthew Gregan [:kinetik]]

Comment on attachment 8733123 [details] [diff] [review]
bug1246108_v0.patch

Approval Request Comment
[Feature/regressing bug #]: bug 948267
[User impact if declined]: frequent crash (beta topcrash) playing media
[Describe test coverage new/current, TreeHerder]: test added in bug 1242774, enabled for all platforms in this bug
[Risks and why]: low risk, simple fix; regressions from fix limited to resuming playback after pause
[String/UUID change made/needed]: n/a

Comment 15

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/e9ada6525c25
https://hg.mozilla.org/mozilla-central/rev/ab302d19ab51

Comment 16

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8733123 [details] [diff] [review]
bug1246108_v0.patch

Fix for topcrash, new tests added. This should land for beta 5 on Thursday.

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/ab8c8b724584
https://hg.mozilla.org/releases/mozilla-aurora/rev/53a4ca04c9f8

Comment 18

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-beta/rev/178db318f318
https://hg.mozilla.org/releases/mozilla-beta/rev/c9e2b215d29c

Comment 19

[Liz Henry (:lizzard) (relman/hg->git project)]

This is causing some intermittent crash test failures, https://treeherder.mozilla.org/logviewer.html#?job_id=938110&repo=mozilla-beta#L9348

I'm not sure if that means we need more attention on the patch or if it means the test is buggy.

Comment 20

[Matthew Gregan [:kinetik]]

I see both the fix and the test-enable patch were uplifted, but I only intended (and requested approval) to uplift the fix because I wasn't sure what the state of that test was with other changes between m-c (where I tested) and earlier versions.  So, one option is to pull the test-enable patch (c9e2b215d29c in beta) and go from there.

Comment 21

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Backed out the test change in https://hg.mozilla.org/releases/mozilla-beta/rev/b2bc67941041

Comment 22

[Liz Henry (:lizzard) (relman/hg->git project)]

Can you also back out the test from aurora, if it looks to be failing intermittently there?

Comment 23

[Liz Henry (:lizzard) (relman/hg->git project)]

Still seeing crashes in beta 5. kinetik can you follow up?  This is marked fixed, but is the #2 topcrash in beta 5.   Re-open, or file a new bug?

Comment 24

[[:philipp]]

it looks like the fix here landed on the beta branch after beta 5 was built...

Comment 25

[Matthew Gregan [:kinetik]]

(In reply to philipp from comment #24)
> it looks like the fix here landed on the beta branch after beta 5 was
> built...

Indeed... if I grab the source from http://ftp.mozilla.org/pub/firefox/candidates/46.0b5-candidates/build2/source/ the fix isn't present.

Comment 26

[Liz Henry (:lizzard) (relman/hg->git project)]

Thanks for catching that philipp and kinetik! Let's see how the crash signature looks after beta 6 then.

Comment 27

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Re-setting 46 to fixed since the patch did land, just not in time for beta 5.

Comment 29

[[:philipp]]

crashes with this signature are still occurring (though in less frequency than before the patch has landed).

Comment 30

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

(In reply to philipp from comment #29)
> crashes with this signature are still occurring (though in less frequency
> than before the patch has landed).

I guess this is the race described in comment 12.

Comment 31

[Matthew Gregan [:kinetik]]

(In reply to JW Wang [:jwwang] from comment #30)
> (In reply to philipp from comment #29)
> > crashes with this signature are still occurring (though in less frequency
> > than before the patch has landed).
> 
> I guess this is the race described in comment 12.

Seems likely, any suggestions on a better approach to fixing this?  Otherwise I can take a look, but probably not until next week at this stage.