1375922 - Youtube UHD 4K Video stucks after bringing tab in foreground

Status: RESOLVED FIXED

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

Description

[Patrick Albrecht]

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.

Comment 1

[Mike Conley (:mconley) (:⚙️)]

Hey jwwang, I seem to recall you working on decoder / decoder shutdown. Is this a known bug?

Comment 2

[TMart]

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.

Comment 3

[Patrick Albrecht]

Maybe my Computer Details will also be useful:
Win10 Pro x64
16GB RAM
Nvidia GTX 980
Intel i5 6600k
512GB SSD

Comment 4

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

Kaku is the person for decoder shutdown.

Comment 5

[Tzuhao Kuo [:kaku]]

This is not a performance issue, but a bug of our seeking algorithm. See bug 1366999.

Comment 6

[TMart]

Is Youtube VP9 using a MP4 container? Isn't it WebM?

Comment 7

[Patrick Albrecht]

Just tested it again with the same profile:
For me it seems like it's using VP9 (video/webm; codecs="vp9")

Comment 8

[Tzuhao Kuo [:kaku]]

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.

Comment 9

[Jean-Yves Avenard [:jya]]

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

Comment 10

[Jean-Yves Avenard [:jya]]

it has nothing to do with bug 1366999.

This is MSE, there's no MP4 or webm demuxer involved when seeking

Comment 11

[Patrick Albrecht]

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.

Comment 12

[Blake Wu [:bwu][:blakewu]]

(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.

Comment 13

[TMart]

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.

Comment 14

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

IIRC, we had discussion to ensure this feature is turned on only when resume is fast enough in order not to irritate user experience.

Comment 15

[TMart]

FWIW Chrome also shows this behavior from time to time. Could be an interplay with graphics drivers.

Comment 16

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

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.

Comment 17

[TMart]

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?

Comment 18

[Patrick Albrecht]

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.

Comment 19

[TMart]

There exists a ticket for my various issues with GPU performance, but it was incoclusive: https://bugzilla.mozilla.org/show_bug.cgi?id=1333880

Comment 20

[Chris Pearce [:cpearce (Not reading bugmail)]]

(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.

Comment 21

[Patrick Albrecht]

Bug 1376838 seems to be the relevant bug :)

Comment 22

[Jean-Yves Avenard [:jya]]

You can either use Nightly or Beta for VP9 hardware decoding, for Beta, also set the pref media.wmf.vp9.force.enabled to true.

Comment 23

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

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.

Comment 24

[Tzuhao Kuo [:kaku]]

(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.

Comment 25

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

Attachment: Bug 1375922 - don't send frames to the compositor if the 1st one is a future frame.

A/V sync will be broken for the compositor will render the 1st frame
immediately without considering its timestamp.

Review commit: https://reviewboard.mozilla.org/r/154090/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/154090/

Comment 26

[Chris Pearce [:cpearce (Not reading bugmail)]]

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.

Comment 27

[Chris Pearce [:cpearce (Not reading bugmail)]]

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.

Comment 28

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

(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.

Comment 29

[Blake Wu [:bwu][:blakewu]]

Maybe we should not handle A/V sync in the first second in resuming. That may let users feel resuming is quicker.

Comment 30

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

Comment on attachment 8883152 [details]
Bug 1375922 - don't send frames to the compositor if the 1st one is a future frame.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/154090/diff/1-2/

Comment 31

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

Comment on attachment 8883152 [details]
Bug 1375922 - don't send frames to the compositor if the 1st one is a future frame.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/154090/diff/2-3/

Comment 32

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

Thanks for the review!

Comment 33

[Pulsebot]

Pushed by [e-mail]:
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

Comment 34

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/b29d1213b4a3

Comment 35

[Jean-Yves Avenard [:jya]]

(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

Comment 36

[Mike Conley (:mconley) (:⚙️)]

Hey Patrick,

Has this fix improved things for you?

Comment 37

[Patrick Albrecht]

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!

Comment 38

[TMart]

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).