Closed Bug 1120241 Opened 5 years ago Closed 5 years ago

Videos continue to play after tab is closed

Categories

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

x86_64
Linux
defect

Tracking

()

RESOLVED FIXED
mozilla38
Tracking Status
firefox36 --- fixed
firefox37 --- fixed
firefox38 --- fixed

People

(Reporter: ajones, Assigned: bholley)

References

(Blocks 1 open bug)

Details

Attachments

(2 files)

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.
Assignee: nobody → bobbyholley
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?
Flags: needinfo?(mozillamarcia.knous)
Flags: needinfo?(ajones)
I've seen it a lot. It seems to happen more on a busy system, e.g. during compilation.
Flags: needinfo?(ajones)
Is this mostly being seen on Mac, and if so using Nightly?
Flags: needinfo?(mozillamarcia.knous)
That is where I have reproduced it.
Attached file 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.
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.
Attachment #8549283 - Flags: review?(cpearce) → review+
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.
Assignee: bobbyholley → matt.woodrow
I'm having trouble reproducing this, so not making much progress at the moment.
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.
(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.
Flags: needinfo?(xavierupo1996)
Also, which OS are you running?
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. :-)
(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
Flags: needinfo?(xavierupo1996)
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
(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?
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.
Assignee: matt.woodrow → bobbyholley
(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
(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.
https://hg.mozilla.org/mozilla-central/rev/21cb6497729f
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
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!
Flags: needinfo?(xavierupo1996)
(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
Flags: needinfo?(xavierupo1996)
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.
Flags: needinfo?(giles)
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.
Attachment #8549283 - Flags: approval-mozilla-beta?
Attachment #8549283 - Flags: approval-mozilla-aurora?
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+
Attachment #8549283 - Flags: approval-mozilla-beta?
Attachment #8549283 - Flags: approval-mozilla-beta+
Attachment #8549283 - Flags: approval-mozilla-aurora?
Attachment #8549283 - Flags: approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.