Closed Bug 1375922 Opened 4 years ago Closed 3 years ago

Youtube UHD 4K Video stucks after bringing tab in foreground

Categories

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

56 Branch
defect

Tracking

()

RESOLVED FIXED
mozilla56
Tracking Status
firefox56 --- fixed

People

(Reporter: github, Assigned: jwwang)

References

Details

(Keywords: nightly-community)

Attachments

(1 file)

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:56.0) Gecko/20100101 Firefox/56.0
Build ID: 20170623045418

Steps to reproduce:

1) Open Firefox with fresh profile
2) Open a UHD Video on youtube like https://www.youtube.com/watch?v=WsptdUFthWI and play it in 4K and in the cinema mode
3) Change to an other tab (e.g. about:newtab or about:home, it doesn't matter)
4) Wait for about 1minute to 1:30 minutes


Actual results:

When you now change back in the tab with the video to the video you see a frame that will be played in the future. Firefox now "waits" for this frame and then starts again to play the video smooth. The audio playback is not affacted at any time.


Expected results:

The video playback should run when I switch back to the tab.

I did a Gecko Profiler and you can check it here: http://bit.ly/2rKPFKV

I asked a little bit around and it seems like only FF56 and FF55 are affected under Windows 10.
Hey jwwang, I seem to recall you working on decoder / decoder shutdown. Is this a known bug?
Flags: needinfo?(jwwang)
I've been experiencing the same for some time now. However for me it is already happening with 1080p videos (and 720p videos to some lesser extent). 

I run an i7 6700k with an Radeon R9 390 (Crimson Relive 17.6.2). It is _not_ happening on my much less powerful laptop with an i5 6200U and Intel HD 520. 

Gecko Profiler: http://bit.ly/2tXtIJi

Additionally scrolling, especially mouse wheel scrolling, is very choppy while a 1080p upwards video is playing, both on the video page and on other pages after switching from the video. This usually stops sometime after switching from the tab with the video. Maybe there is a connection.
Maybe my Computer Details will also be useful:
Win10 Pro x64
16GB RAM
Nvidia GTX 980
Intel i5 6600k
512GB SSD
Kaku is the person for decoder shutdown.
Flags: needinfo?(jwwang) → needinfo?(kaku)
Component: Untriaged → Audio/Video: Playback
Product: Firefox → Core
This is not a performance issue, but a bug of our seeking algorithm. See bug 1366999.
Assignee: nobody → kaku
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Flags: needinfo?(kaku)
Depends on: 1366999
Is Youtube VP9 using a MP4 container? Isn't it WebM?
Just tested it again with the same profile:
For me it seems like it's using VP9 (video/webm; codecs="vp9")
Yes, YT does use VP9.
I should just making bug 1366999 as a see-also, instead of a dependency.
I'm going to understand why VP9 has such similar behavior as to MP4.
No longer depends on: 1366999
See Also: → 1366999
When the tab is put in the background, video decoding is stopped in order to save battery life and energy.

When you switch back to the tab, it resumes playback. To do so unfortunately it needs to seek to the current position. With a very high resolution video, seeking has an unfortunate delay and may cause a visual interruption
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → INVALID
See Also: 1366999
it has nothing to do with bug 1366999.

This is MSE, there's no MP4 or webm demuxer involved when seeking
Thank you, I thought it would be something like this. But in a desktop environment, performance is more important then save energy. I would be nice to have a perf to enable/disable the energy save behavior.
Flags: needinfo?(jyavenard)
(In reply to Patrick Albrecht from comment #11)
> Thank you, I thought it would be something like this. But in a desktop
> environment, performance is more important then save energy. I would be nice
> to have a perf to enable/disable the energy save behavior.

You can use media.suspend-bkgnd-video.enabled to disable/enable it. 

It is a good point that performance could be more important than energy on desktop. Maybe we should default disable it on desktop and default enable it on laptop and mobile. Or we should have a setting for user. I am going to discuss it with PM/UX to have further discussion.
Flags: needinfo?(jyavenard)
I think for a desktop environment this behavior is hardly tolerable, just because performance suffers so much. 

Thank you for providing the flag, I'll disable it and test performance again. 

If the flag solves this the question remains why it performs so much better on Intel GPUs compared to AMD/nVidia GPUs. Or if other factors are involved.
IIRC, we had discussion to ensure this feature is turned on only when resume is fast enough in order not to irritate user experience.
FWIW Chrome also shows this behavior from time to time. Could be an interplay with graphics drivers.
Just to explain, we shut down video decoding in background tabs in order to save CPU/GPU work. This frees up resources for foreground tabs. The way video works is that you get a key frame every 4 seconds and every subsequent frame is dependent on the prior frame. The compromise for better foreground performance is occasionally worse tab switching performance.

You should find that it works a whole lot better if you set YouTube to Auto instead of setting it to 4k.

If tab switching performance is more important to you than foreground tab perfomance then you can set media.suspend-bkgnd-video.enabled=false and it will continue to decode videos regardless of whether you are watching them as it did in the past.
I tried it with media.suspend-bkgnd-video.enabled=false and it seemed to help. Still not completely smooth, but much better. 

However the problem isn't that bad for me on Youtube anyway. It happens mostly on other sites that don't use WebM/VP9. 

"You should find that it works a whole lot better if you set YouTube to Auto instead of setting it to 4k."
Just because it's on Auto or because it's less likely to select 4k then?
I couldn't test it yet but I think for your issue @TMart you should maybe open a new ticket as it seems like you have an other bug.
There exists a ticket for my various issues with GPU performance, but it was incoclusive: https://bugzilla.mozilla.org/show_bug.cgi?id=1333880
(In reply to TMart from comment #13)
> I think for a desktop environment this behavior is hardly tolerable, just
> because performance suffers so much. 
> 
> Thank you for providing the flag, I'll disable it and test performance
> again. 
> 
> If the flag solves this the question remains why it performs so much better
> on Intel GPUs compared to AMD/nVidia GPUs. Or if other factors are involved.

We can use the Intel GPU for hardware VP9 decoding, but we've not enabled VP9 GPU video decoding for Nvidia hardware yet. When we do this, you should see the performance in this case improve significantly.
Bug 1376838 seems to be the relevant bug :)
You can either use Nightly or Beta for VP9 hardware decoding, for Beta, also set the pref media.wmf.vp9.force.enabled to true.
This is my new finding about this bug:

1. When we resume video deciding, we resume to the GetMediaTime() position which is slightly behind (40ms in the worst case) the current playback position. So it is quite possible skip-to-next-key-frame will be activated when decoding the next frame if resolution is high and decoding is slow.

2. Now we have [90s, 96s] in the video queue. Note we don't have frames between 90s and 96s for skip-to-next-key-frame is activated. Then the render loop sees current time is 91s so it removes [90s] from the queue and sends [96s] to the image container. Since [96s] is the only frame, the image container will render this frame immediately even though it is a 'future' frame. This explains the 'future frame' observed in comment 0.

[1] http://searchfox.org/mozilla-central/rev/5bb6dfcb3355a912c20383578bc435c1d4ecf575/dom/media/MediaDecoderStateMachine.cpp#3153

I will fix issue 2 in the bug. For issue 1, you need to enable VP9 hardware decoding to improve the decoding performance.
Status: RESOLVED → REOPENED
Resolution: INVALID → ---
(In reply to JW Wang [:jwwang] [:jw_wang] from comment #23)
> This is my new finding about this bug:
> 
> 1. When we resume video deciding, we resume to the GetMediaTime() position
> which is slightly behind (40ms in the worst case) the current playback
> position. 

http://searchfox.org/mozilla-central/rev/5bb6dfcb3355a912c20383578bc435c1d4ecf575/dom/media/MediaDecoderStateMachine.cpp#3152-3154

This could be alleviated by resuming to the current playback position + some time forward.

> So it is quite possible skip-to-next-key-frame will be activated
> when decoding the next frame if resolution is high and decoding is slow.

This leads to a frozen future frame shown on the screen and waiting for audio to catch up, and we can use the UI throbber to work around. 

While completing a video-only seek, the Gecko dispatches a VideoOnlySeekCompleted event and the video control listens to this event to cancel the throbber. 

Here, we're going to decouple the "video-only" seek into a subclass of SeekingState, and it will take the decoded video frame's time into consideration for completing seek. In this way, we should be able to extend the throbber if the skip-to-next-key-frame is activated.
Attachment #8883152 - Flags: review?(cpearce)
Blocks: 1378084
Blocks: 1378085
Priority: -- → P1
Comment on attachment 8883152 [details]
Bug 1375922 - don't send frames to the compositor if the 1st one is a future frame.

https://reviewboard.mozilla.org/r/154090/#review159382

::: dom/media/mediasink/VideoSink.cpp:320
(Diff revision 1)
> +    // No frames to render.
> +    return;
> +  }
> +
> +  TimeStamp nowTime;
> +  const auto clockTime = mAudioSink->GetPosition(&nowTime);

Please only use auto where it improves readability, for example for heavivly templatized iterators etc. 

Using it here makes it unclear whether clockTime is signed or not, it's not clear whether you're comparing a TimeStamp with a TimeUnit or an int64_t or whatever units the position is in.
Attachment #8883152 - Flags: review?(cpearce) → review+
Thanks for the patch. I think this makes things slightly less bad.

(In reply to JW Wang [:jwwang] [:jw_wang] from comment #23)
> This is my new finding about this bug:
> 
> 1. When we resume video deciding, we resume to the GetMediaTime() position
> which is slightly behind (40ms in the worst case) the current playback
> position. So it is quite possible skip-to-next-key-frame will be activated
> when decoding the next frame if resolution is high and decoding is slow.

Preffing off skip-to-next-keyframe with media.decoder.skip-to-next-key-frame.enabled=false doesn't seem to make things consistently better in my (unscientific) testing.

Blake, Anthony, and I spoke about this feature a lot at MozSF, and we're going to try to get the UX/PM people to come up with some sort of acceptance testing or acceptable restart delay metric for the background-decoder-shutdown.

We should probably only enable this feature for a subset of users, under some conditions. Like only on battery power, or for hardware decoders only, or for only some resolutions, etc.
(In reply to Chris Pearce (:cpearce) from comment #27)
> Preffing off skip-to-next-keyframe with
> media.decoder.skip-to-next-key-frame.enabled=false doesn't seem to make
> things consistently better in my (unscientific) testing.
I think this is because VideoSink drops late frames. When you disable skip-to-next-key-frame and decoding is slow, it is more likely to get 'late' frames.
Maybe we should not handle A/V sync in the first second in resuming. That may let users feel resuming is quicker.
Thanks for the review!
Assignee: kaku → jwwang
Pushed by jwwang@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b29d1213b4a3
don't send frames to the compositor if the 1st one is a future frame. r=cpearce
https://hg.mozilla.org/mozilla-central/rev/b29d1213b4a3
Status: REOPENED → RESOLVED
Closed: 4 years ago3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
(In reply to JW Wang [:jwwang] [:jw_wang] from comment #23)
> This is my new finding about this bug:
> 
> 1. When we resume video deciding, we resume to the GetMediaTime() position
> which is slightly behind (40ms in the worst case) the current playback
> position. So it is quite possible skip-to-next-key-frame will be activated
> when decoding the next frame if resolution is high and decoding is slow.
> 
> 2. 


The JS player (here YouTube player) only knows about currentTime and only ensure that it has buffered data at that time, and of course some ahead so that playback is uninterrupted. 


It is possible that attempting to seek further ahead, may not actually succeed as we do not have yet data there. 

Now if we're talking only about a few ms ahead of currentTime is probably doesn't matter. 

but seeking at currentTime is the only location guaranteed to succeed
Blocks: 1378691
Hey Patrick,

Has this fix improved things for you?
Flags: needinfo?(github)
Hi Mike,

Unfortunately I switched my computer these days and have no longer access to my old one. I retested it again on my MacBook 12' with 1440p (highest possible resolution) and it seems to be much better, thank you JW Wang for the patch!
Flags: needinfo?(github)
It has gotten a lot better for me with the latest patches (still not as smooth on my Radeon system as on my Intel system).
You need to log in before you can comment on or make changes to this bug.