1512958 - Media playback stalls because DecodedStream stops consuming data

Status: VERIFIED FIXED

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

Description

[Habib]

Attachment: testAudio.zip

User Agent: Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:65.0) Gecko/20100101 Firefox/65.0

Steps to reproduce:

- Take a look at the attached sample and run it, in Nightly 65.
- Click to play the sound.
- When you switch between tabs, the audio should pause or resume, as per the visibilitychange listener: when you go back to the tab after 2 or 3 seconds, the sound resumes as expected. However, if you stay outside from the tab just some seconds more (let's say 10 seconds), the sound will resume when you'll be back to the tab but will be stopped quite immediately after that.


Actual results:

The sound has stopped playing, instead of resuming.


Expected results:

The sound should have resumed. This issue seems to only apply to Audio with WebAudio/createMediaElementSource (there is another test case in the html file, which works fine for "basic" Audio).

Comment 1

[Andreas Pehrson [:pehrsons]]

Alastor, can you take a look?

I'm marking this a P2 for now. Please update appropriately as you find out more.

Comment 2

[Alastor Wu [:alwu]]

I can reproduce this issue, will investigate what's happened.

Comment 3

[Alastor Wu [:alwu]]

This issue is unrelated with autoplay policy, you can reproduce this issue even if turning off blocking-autoplay.

After some digging, this issue seems to me that the `DecodedStream` didn't consume data correctly. MDSM had decoded enough audio data in queue but nobody consumed them, so that the media playback stalled. In the meanwhile, the AudioContext and AudioDestnationNode were both running well.

--

Andreas, do you have any idea about that?

Thank you!

Comment 4

[Andreas Pehrson [:pehrsons]]

I reproduced it under rr, so I'll have a look.

It happened to me just after looping caused a seek back to the beginning, so perhaps it's related to seeking when captured.

Comment 5

[Andreas Pehrson [:pehrsons]]

It is related to seeking, and also triggered by a pause/play cycle.

Because on seek and pause/play we tear down the track we're feeding to the MSG and add a new one on the same stream.

After the refactor in bug 1423241 we are however not properly cleaning the old tracks up on pause, and so when data for them run out, we stall - even though the new track has data. Such is the logic of the MediaStreamGraph.

This will get clearer in the future when all audio is pulled (this is the only audio source that is pushed atm).

For the moment however, this is fine. What is missing is marking the old tracks ended. That will make everything jolly. I also noticed we are not cleaning up track listeners for the old tracks, so while they are not doing anything, they are getting unnecessarily notified. After a lot of seeking or pause/play cycles there might be many many many listeners around slowing us down. I'll write a patch to also clean those up.

Obviously this needs a test too.

Comment 6

[Andreas Pehrson [:pehrsons]]

Attachment: Bug 1512958 - Add mochitest. r?jya!, r?jib!

Comment 7

[Andreas Pehrson [:pehrsons]]

Attachment: Bug 1512958 - Properly clean up produced tracks in DecodedStream. r?jya

Comment 8

[Andreas Pehrson [:pehrsons]]

This mochitest reproduces locally, and the fix fixes it. Let's see what try thinks.

Test-only: https://treeherder.mozilla.org/#/jobs?repo=try&revision=782229c849ede9b00e9d2fc5280a184bc70449da
With fix: https://treeherder.mozilla.org/#/jobs?repo=try&revision=0b407851520b3959100f42529b449a6a6a5124b9

Comment 9

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

Tracking to make sure we follow up for 65.

Comment 10

[Andreas Pehrson [:pehrsons]]

The test was reworked a bit.

Test-only: https://treeherder.mozilla.org/#/jobs?repo=try&revision=4d12f7a89593588ade3ff1d5a3743f4b708804b7
With fix: https://treeherder.mozilla.org/#/jobs?repo=try&revision=446a19005dc615351231f595b3e8ad946e0ef246

Comment 13

[Andreas Pehrson [:pehrsons]]

Making this a P1 as it has busted seeking on SoundCloud (bug 1515099).

Comment 14

[Andreas Pehrson [:pehrsons]]

It looks like this is not triggering bug 1505250 so much on latest m-c. I'll make an attempt at landing and we'll see if it sticks. If it doesn't, the easiest would be to disable the failing test, unless bug 1505250 is ready by then.

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1d4816ef6e01
Add mochitest. r=jya,jib
https://hg.mozilla.org/integration/autoland/rev/74101900e7d4
Properly clean up produced tracks in DecodedStream. r=jya

Comment 16

[Andreas Pehrson [:pehrsons]]

Comment on attachment 9031491 [details]
Bug 1512958 - Properly clean up produced tracks in DecodedStream. r?jya

[Beta/Release Uplift Approval Request]

Feature/Bug causing the regression: Bug 1423241

User impact if declined: Playback of media elements can stall in some cases. A broken high-profile site is Soundcloud.

Is this code covered by automated tests?: Yes

Has the fix been verified in Nightly?: No

Needs manual test from QE?: Yes

If yes, steps to reproduce: See comment 0, and bug 1515099 comment 0.

List of other uplifts needed: None

Risk to taking this patch: Low

Why is the change risky/not risky? (and alternatives if risky): We're adding a cleanup step that was missing before, and clarifying the threading model somewhat. The formal is trivial, the latter is simplifying the code. This plus the coverage in automation makes this low risk.

String changes made/needed: None

Comment 17

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/1d4816ef6e01
https://hg.mozilla.org/mozilla-central/rev/74101900e7d4

Comment 18

[Cristina Coroiu [:ccoroiu]]

Backed out for frequently failing mda 
Backout: https://hg.mozilla.org/mozilla-central/rev/66865fdea5afa8c7613300502f6f1a33d704754a
Failure push: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=74101900e7d484cc9ddcba2cd867ca172b961ea0&searchStr=mda1&selectedJob=218453848
Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=218453848&repo=autoland&lineNumber=5035

Comment 19

[Alastor Wu [:alwu]]

Hi, Cristina,
The fix for the test fail has just landed (bug1505250), so I think now there won't see this fail anymore. Could you help me reland this patch?
Thank you.

Comment 20

[Cristina Coroiu [:ccoroiu]]

This was relanded: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=4d03957994c13b57e6d325f59c004be995342636

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/mozilla-central/rev/e7becb0458ac
Add mochitest. r=jya,jib

Comment 22

[Andrei Ciure[:aciure]]

https://hg.mozilla.org/mozilla-central/rev/e7becb0458ac
https://hg.mozilla.org/mozilla-central/rev/4d03957994c1

Comment 23

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

Comment on attachment 9031491 [details]
Bug 1512958 - Properly clean up produced tracks in DecodedStream. r?jya

Fix for regression in 65, has test coverage, let's uplift for next week's beta 8 build.

Comment 24

[Ryan VanderMeulen [:RyanVM]]

Needs a rebased patch for Beta uplift.

Comment 25

[Andreas Pehrson [:pehrsons]]

Attachment: [beta 65] Properly clean up produced tracks in DecodedStream.

Rebased.

Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=6470db6c864a90de99b8fe649a1f168db7f62875

Comment 26

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/3b1807d65b23
https://hg.mozilla.org/releases/mozilla-beta/rev/45373d310ef9

Comment 27

[Adrian Florinescu [:aflorinescu]]

Reproduced the bug with 65.0b6 2018-12-20, using test case https://bugzilla.mozilla.org/show_bug.cgi?id=1515099#c0 . 

Verified as fixed on Ubuntu 16.04x64, Windows 10 x64, Osx 10.14.2 on:
66.0a1 20190103220533
65.0b8 20190103150357