1120241 - Videos continue to play after tab is closed

Status: RESOLVED FIXED

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

Description

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Steps to reproduce:

* Start compiling something
* Navigate to YouTube and watch a video
* Close tab

Actual results:

Sound continues to play

Expected results:

Sound stops.

Comment 1

[Bobby Holley (:bholley)]

Hm, this happened to me this morning, but I haven't been able to reproduce it since.

Anthony, are you able to hit this reliably?
Marcia, any ideas?

Comment 2

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

I've seen it a lot. It seems to happen more on a busy system, e.g. during compilation.

Comment 3

[Marcia Knous [:marcia]]

Is this mostly being seen on Mac, and if so using Nightly?

Comment 4

[Bobby Holley (:bholley)]

That is where I have reproduced it.

Comment 5

[Bobby Holley (:bholley)]

Attachment: log

So, I _think_ I just managed to reproduce it and get the attached log.

It seems that we do quite a lot of stuff between the logical state change to PAUSED and the call to StopPlayback. I believe that this is because nothing is actually scheduling the state machine when the logical state changes. The state machine adjusts between logical PLAY/PAUSE in the DECODER_STATE_DECODING case or RunStateMachine, so if we're displaying a particularly long frame (which presumably happens if the video content remains uniform?), we may not pick up the change for several seconds.

Does this make sense? Patch is easy enough if my theory is correct.

Comment 6

[Bobby Holley (:bholley)]

Though in opposition to my theory:
* The video I was watching was https://www.youtube.com/watch?v=Cm7mCmncbvA , which doesn't seem to have any noticeably long periods of unchanging video content.
* This wouldn't explain why we're suddenly seeing this with MSE.

Let's see what chris thinks of the patch.

Comment 7

[Bobby Holley (:bholley)]

Attachment: Schedule the state machine when setting logical decoder state. v1

Comment 8

[Bobby Holley (:bholley)]

Matt was going to have a look at this. I'm skeptical of my existing patch because it seems like we'd still be scheduling the state machine often enough via AdvanceFrame.

Comment 9

[Matt Woodrow (:mattwoodrow)]

I'm having trouble reproducing this, so not making much progress at the moment.

Comment 10

[Yunito]

I can reproduce the bug.

I think it is related with video buffer because It only happens to me when my connection is very slow. 

Previous requeriments:
1- Very slow connection or maybe connected to a Slow VPN

Steps to reproduce:

1- Ctrl + N (Open new windows)
2- Go to Youtube URL: www.youtube.com
3- Open some videos and choose the best quality you can afford.

Actual results:

Sound continues playing after tab is closed or video is stopped


Expected results:

Sound stops.

Comment 11

[Bobby Holley (:bholley)]

(In reply to Yunito from comment #10)
> I can reproduce the bug.

That's great! Does the sound keep playing indefinitely, or stop after a couple of seconds?

Would you mind capturing a log? To do that, set the environmental variables:

MEDIA_LOG_SAMPLES=1 NSPR_LOG_MODULES="MediaDecoder:5,MediaSource:5,MediaPromise:5,MP4Demuxer:5"

And then launch Firefox. If you need any help, please let me know.

Comment 12

[Bobby Holley (:bholley)]

Also, which OS are you running?

Comment 13

[Bobby Holley (:bholley)]

Just FYI Yunito - this is very time sensitive, and the sooner we get a log, the better chance we have to fix this bug before we ship this feature on Firefox 36. :-)

Comment 14

[Yunito]

(In reply to Bobby Holley (Busy with media, don't ask for DOM/JS/XPConnect things) from comment #12)
> Also, which OS are you running?

I'm running Antergos x64 (Arch Linux's Family)

(In reply to Bobby Holley (Busy with media, don't ask for DOM/JS/XPConnect things) from comment #11)
> (In reply to Yunito from comment #10)
> > I can reproduce the bug.
> 
> That's great! Does the sound keep playing indefinitely, or stop after a
> couple of seconds?
> 
> Would you mind capturing a log? To do that, set the environmental variables:
> 
> MEDIA_LOG_SAMPLES=1
> NSPR_LOG_MODULES="MediaDecoder:5,MediaSource:5,MediaPromise:5,MP4Demuxer:5"
> 
> And then launch Firefox. If you need any help, please let me know.

I don't know how to set that enviromental variables or capture a log. I would appreciate if you can help me with that

Comment 15

[Yunito]

I got it!

Finally I found the way to set environmental variables and here is the log:

http://pastebin.com/2fWC2ube

I hope it is useful.

P.S: I didn't know how much log do you want to. So, I copy almost of all

Comment 16

[Bobby Holley (:bholley)]

(In reply to Yunito from comment #15)
> I got it!
> 
> Finally I found the way to set environmental variables and here is the log:
> 
> http://pastebin.com/2fWC2ube
> 
> I hope it is useful.

Thanks Yunito! That is very useful - though it looks like the log begins after you've already paused the video. Were you logging before you paused the video? If so, could you post more of the log?

Comment 17

[Bobby Holley (:bholley)]

From what I can tell of the log, my initial theory was actually correct here - the state machine just never seems to get schedule.

Before, I figured that the theory was wrong, since AdvanceFrame would always schedule the state machine. However, I just noticed that the very top of AdvanceFrame checks the logical decoder state and then bails out if the state isn't playing. So if AdvanceFrame runs at the wrong time, we really might just never run the state machine. I'd need to see the rest of Yunito's log to be sure, but it seems like a decent theory.

So the original patch in the bug here is probably in fact what we want. It looks like I never pushed it to try before, so I'll do that now and then land it in the morning.

Comment 18

[Bobby Holley (:bholley)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=0be35b291fe2

Comment 19

[Bobby Holley (:bholley)]

(In reply to Bobby Holley (Busy with media, don't ask for DOM/JS/XPConnect things) from comment #18)
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=0be35b291fe2

This seems to be coming back green on linux. This is a super low risk patch, so I'm just going to push it now, since that'll let it be picked up by the morning merge, and potentially get this into Nightly a day sooner.

https://hg.mozilla.org/integration/mozilla-inbound/rev/21cb6497729f

Comment 20

[Yunito]

(In reply to Bobby Holley (Busy with media, don't ask for DOM/JS/XPConnect things) from comment #16)
> (In reply to Yunito from comment #15)
> > I got it!
> > 
> > Finally I found the way to set environmental variables and here is the log:
> > 
> > http://pastebin.com/2fWC2ube
> > 
> > I hope it is useful.
> 
> Thanks Yunito! That is very useful - though it looks like the log begins
> after you've already paused the video. Were you logging before you paused
> the video? If so, could you post more of the log?

The sound stops after a ten seconds more or less. But It continues playing when I pause the music or/and close the tab. Therefore the logs shows that I paused the music. I haven't more of that log because I copied all log from terminal, but, I will register a new log with these error so you'll be able to compare those logs.

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/21cb6497729f

Comment 22

[Bobby Holley (:bholley)]

Yunito, this should be in tomorrow's Nightly build (the build for jan 31). Once that build is spun, can you do some testing and let me know if you encounter the problem again? Thanks a ton for your help!

Comment 23

[Yunito]

(In reply to Bobby Holley (Busy with media, don't ask for DOM/JS/XPConnect things) from comment #22)
> Yunito, this should be in tomorrow's Nightly build (the build for jan 31).
> Once that build is spun, can you do some testing and let me know if you
> encounter the problem again? Thanks a ton for your help!

Hi, Bobby Holley!

At the moment, the issue seems fixed. I couldn't reproduce it anymore

Comment 24

[Bobby Holley (:bholley)]

Hooray! Thanks a lot for your help. :-)

Ralph, this is super-safe. We should uplift it regardless of what we decide on thursday - it will likely improve non-MSE experience as well.

Comment 25

[Ralph Giles (:rillian)]

Comment on attachment 8549283 [details] [diff] [review]
Schedule the state machine when setting logical decoder state. v1

Approval Request Comment
[Feature/regressing bug #]: MSE, HTML5 Video.
[User impact if declined]: Videos can keep playing audio after a tab is closed. YouTube playback can use more resources.
[Describe test coverage new/current, TreeHerder]: Landed on m-c, manual testing.
[Risks and why]: Risk is minimal. We just make sure we check for events before shutting down. This affects all video playback, but is an isolated and straightforward change.
[String/UUID change made/needed]: None.

Comment 26

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8549283 [details] [diff] [review]
Schedule the state machine when setting logical decoder state. v1

I agree. Simple fix for an annoying issue. This has already been on m-c for 5 days. Beta+ Aurora+

Comment 27

[Ralph Giles (:rillian)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/cdd2b77b371e

Comment 28

[Ralph Giles (:rillian)]

https://hg.mozilla.org/releases/mozilla-beta/rev/b0684ad8a47c