Closed Bug 1266799 Opened 8 years ago Closed 8 years ago

[e10s] Investigate slow fullscreen transition in content DOM for Yahoo music player

Categories

(Core :: DOM: Content Processes, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla49
Tracking Status
e10s m9+ ---
firefox48 --- fixed
firefox49 --- fixed

People

(Reporter: mconley, Assigned: mconley)

References

(Depends on 1 open bug)

Details

(Whiteboard: btpp-followup-2016-05-27)

Attachments

(1 file)

Spun out from bug 1264091.

We seem to get into a state where when the user triggers DOM fullscreen, the browser UI does the full screen transition, and once it completes, there is a gap of time where the web content does not go full screen, which is rather jarring.

According to Xidorn, there is a full-screen-api.transition.timeout that we must be hitting, which is causing the transition in the browser UI to complete in order to remain responsive to the user. We should find out what is happening in the content process that is causing it to be slow at doing the DOM fullscreen transition.
This is supplanting bug 1264091 as an e10s M9.
tracking-e10s: --- → m9+
Could this have something to do with resize perf? Related bugs - 
Bug 1255968 - Interruptible reflow is broken in e10s
Bug 1257869 - Window Resize of large page takes seconds to change viewport
Moving this to ? to find an owner.
See Also: → 1267568
Flags: needinfo?(mconley)
I believe this is caused by some JS on the Yahoo player page that's reacting to the mozfullscreenchange.

Here's a more minimal test case:

1) Visit https://output.jsbin.com/lituremiga
2) Start the video
3) Click on the fullscreen button to enter fullscreen
4) Once the transition completes, click on it again to exit fullscreen.

I've added a busy function for the mozfullscreenchange event, and that's blocking the content from completing its transition in time in the e10s case.

In the non-e10s case, the busy loop causes the whole browser to wait inside the "dark" fullscreen transition.

Neither is amazing, but perhaps the latter is preferable.
Flags: needinfo?(mconley)
(In reply to Mike Conley (:mconley) - Needinfo me! from comment #4)
> I believe this is caused by some JS on the Yahoo player page that's reacting
> to the mozfullscreenchange.
> 
> Here's a more minimal test case:
> 
> 1) Visit https://output.jsbin.com/lituremiga
> 2) Start the video
> 3) Click on the fullscreen button to enter fullscreen
> 4) Once the transition completes, click on it again to exit fullscreen.
> 
> I've added a busy function for the mozfullscreenchange event, and that's
> blocking the content from completing its transition in time in the e10s case.
> 
> In the non-e10s case, the busy loop causes the whole browser to wait inside
> the "dark" fullscreen transition.
> 
> Neither is amazing, but perhaps the latter is preferable.

This is arguable. There are certain users hate to be thrown in the dark for a long time, and want us to unblack the screen even if it is in an intermediate state.

The mozfullscreenchange event is dispatched inside refresh driver tick, and there is no guarantee that such tick would ever complete (e.g. if page is closed inside the tick), so there must be some kind of timeout anyway.
I wonder whether it is really a dup of bug 1243014.
See Also: → 1243014
Depends on: 1243014
This feel like an evangelism bug to me, and possibly a dev-doc-needed bug requesting that our MDN docs explicitly state you should not block on mozfullscreenchange.

https://developer.mozilla.org/en-US/docs/Web/API/Fullscreen_API
(In reply to Jim Mathies [:jimm] from comment #7)
> This feel like an evangelism bug to me, and possibly a dev-doc-needed bug
> requesting that our MDN docs explicitly state you should not block on
> mozfullscreenchange.
> 
> https://developer.mozilla.org/en-US/docs/Web/API/Fullscreen_API

It seems this page really would need an update. We've unprefixed this API in Firefox 47.
Relevant spec: https://fullscreen.spec.whatwg.org/#api

The event timing is actually defined in the spec to happen "As part of the next animation frame task" after we "Resize pending's top-level browsing context's document's viewport's dimensions to match the dimensions of the screen of the output device."
(In reply to Chris H-C :chutten from comment #9)
> Relevant spec: https://fullscreen.spec.whatwg.org/#api
> 
> The event timing is actually defined in the spec to happen "As part of the
> next animation frame task" after we "Resize pending's top-level browsing
> context's document's viewport's dimensions to match the dimensions of the
> screen of the output device."

Classic spec.

So my reading on this is that we're supposed to be firing this event _after_ we've done the fullscreen transition in the content area, no?

Xidorn, do you know?
Flags: needinfo?(bugzilla)
The "next animation frame task" after resize is exact the first frame we are in fullscreen, which we really want to finish before we unblack the screen. The spec is written that way to ensure that there is no intermediate state between non-fullscreened and fully-fullscreened.
Flags: needinfo?(bugzilla)
So we're firing the event too early? (Sorry if I'm coming off as dense - just trying to be absolutely clear)
Flags: needinfo?(bugzilla)
No, I believe we are firing the event at the right time. This event does block the first frame, which blocks unblacking.
Flags: needinfo?(bugzilla)
Interesting. Then I think other browsers might be playing by different rules.

Take Chrome, for example. For some reason, the demo page I wrote for this bug doesn't work for it, but pick any YouTube video using the HTML5 video player. Open the Chrome devtools console, and put in:

addEventListener("webkitfullscreenchange", function(e) {
  console.log("Got fullscreenchange");
  var then = Date.now();
  while(Date.now() - then < 2500) {};
  console.log("Done spinning");
});

Press enter, and then fullscreen the YouTube video. The video goes fullscreen right away, though there is a slight border around it for the 2.5 seconds that we're running the busy loop. We also exit immediately, even though the content process is blocked for 2.5 seconds.

Are they not to spec? Or are we?
Flags: needinfo?(bugzilla)
They don't, I believe. Blink doesn't follow the spec for quite a while.

Making the next frame block on this event so that the script has a chance to fix anything specific to fullscreen before it is shown to users is intuitively reasonable.
Flags: needinfo?(bugzilla)
So is this now a TE bug?
Flags: needinfo?(mconley)
Whiteboard: btpp-followup-2016-05-10
I don't know if it is, to be honest.

I just tested on Safari, and from the looks of things, they wait until the content transition is complete before doing this "zoom" effect, much like the application fullscreen effect. Exiting fullscreen shows a similar bit of jank while content sorts itself out - the zoom effect reverses, but the video consumes the entire browser space for a half a second or so before it can re-organize itself.

Edge expands the application to consume the fullscreen, and then after a short pause, fullscreens the video. It's not too smooth.

I don't know if this is a TE bug. This might be a UX bug - the effect in Firefox of fading down and fading up might be emphasizing the fact that content wasn't ready (since the fade up seems to imply "all set to go"). Perhaps this is a matter of increasing the timeout for fade-up slightly from its current setting. For example, if I set full-screen-api.transition.timeout to 1000 from its default of 500, the transition is beautifully done.

I wonder if we could / should collect some Telemetry to determine how often the timeout is being hit, and if so, if there's an acceptable value of full-screen-api.transition.timeout that would accommodate most of our users without keeping them in the dark too long.

ni?ing verdi, since he worked on bug 1129061.
Flags: needinfo?(mconley) → needinfo?(mverdi)
(In reply to Mike Conley (:mconley) - Needinfo me! from comment #17)
> I just tested on Safari, and from the looks of things, they wait until the
> content transition is complete before doing this "zoom" effect, much like
> the application fullscreen effect.

I wonder whether we can do this as well. I mean, at least start content side change at the beginning of transition, which would give us ~200ms more time to process things.

What I'm concerned is animations on the page driven by the refresh driver. Are we fine to freeze them before the screen goes completely black?
Depends on: 1271767
(In reply to Mike Conley (:mconley) - Needinfo me! from comment #4)
> 
> I've added a busy function for the mozfullscreenchange event, and that's
> blocking the content from completing its transition in time in the e10s case.
> 
> In the non-e10s case, the busy loop causes the whole browser to wait inside
> the "dark" fullscreen transition.
> 
> Neither is amazing, but perhaps the latter is preferable.

The e10s case is horrible. It looks like Firefox is completely broken. The non-e10s case is not ideal but it just looks like we're a little slow, not completely broken.

(In reply to Mike Conley (:mconley) - Needinfo me! from comment #17)
> Perhaps this is a matter of increasing the timeout for fade-up slightly from
> its current setting. For example, if I set
> full-screen-api.transition.timeout to 1000 from its default of 500, the
> transition is beautifully done.

I'm not sure what the timing of that looks like. Is it faster or slower than the non-e10s case above?

> 
> I wonder if we could / should collect some Telemetry to determine how often
> the timeout is being hit, and if so, if there's an acceptable value of
> full-screen-api.transition.timeout that would accommodate most of our users
> without keeping them in the dark too long.
> 

That seems reasonable if there isn't a way to prevent the problem to begin with.
Flags: needinfo?(mverdi)
Depends on: 1271160
No longer depends on: 1271767
triage note: telemetry probes just landed in nightly (bug 1271160), and should uplift to beta shortly.
Whiteboard: btpp-followup-2016-05-10 → btpp-followup-2016-05-27
Assignee: nobody → mconley
The Telemetry probes suggest that a wait of about 1.17s should account for about 95% of all use cases.

I'm going to suggest we just round that out to 1s, which should account for something like 93%-94% of use cases.
According to the Telemetry probes added in Bug 1271160, 1000ms should
account for ~94% of fullscreen transitions. The remaining ~6% tail is where
users might see the transition end and then content re-organize itself.

I think this is a big improvement over the original 500ms, which covers only
about ~80% of cases, according to Telemetry.

Review commit: https://reviewboard.mozilla.org/r/57264/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/57264/
Attachment #8759284 - Flags: review?(bugzilla)
Comment on attachment 8759284 [details]
Bug 1266799 - Increase DOM fullscreen timeout length to 1000ms.

https://reviewboard.mozilla.org/r/57264/#review54206
Attachment #8759284 - Flags: review?(bugzilla) → review+
Pushed by mconley@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/250ad26a44ea
Increase DOM fullscreen timeout length to 1000ms. r=xidorn
https://hg.mozilla.org/mozilla-central/rev/250ad26a44ea
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
Comment on attachment 8759284 [details]
Bug 1266799 - Increase DOM fullscreen timeout length to 1000ms.

Approval Request Comment
[Feature/regressing bug #]:

e10s

[User impact if declined]:

Users that have e10s enabled have a greater probability of seeing an ugly full page re-layout after making some element go fullscreen (like a video).

[Describe test coverage new/current, TreeHerder]:

There are internal tests for fullscreen that are all passing.

[Risks and why]: 

Very low risk - we're just bumping up a value in a pref to increase the amount of time we're willing to wait before finishing the fullscreen transition.

[String/UUID change made/needed]:

None.
Attachment #8759284 - Flags: approval-mozilla-aurora?
Comment on attachment 8759284 [details]
Bug 1266799 - Increase DOM fullscreen timeout length to 1000ms.

Improve e10s support, taking it
Attachment #8759284 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.