1177155 - Full screen video transition should be atomic

Status: RESOLVED FIXED

(Core :: General, defect)

Description

[Markus Stange [:mstange]]

When I press the fullscreen button of the new YouTube HTML5 video player, the transition to fullscreen happens in 5 (!) steps:

 0. Window is in its regular state.
 1. Menubar + dock hide
 2. The window is resized to fullscreen. Chrome is still visible, content looks resized but hasn't changed to its fullscreen appearance. (Also the video player looks wonky for some reason)
 3. Content has started applying its fullscreen stylesheet. The video area now covers the whole content viewport, but the video size hasn't been updated yet. Chrome is still visible.
 4. Chrome is hidden and the video is sized correctly, but still slightly mispositioned.
 5. The video position is updated to its final position.

This looks really bad. All of this should happen in one atomic screen update.
(Except maybe step 1; the system might not let us make that atomic with the window update.)

On OS X, when we resize the window, lots of things happen synchronously in that call: The window resize handler is called, then a synchronous paint is triggered, and once that paint has completed, the resize call completes and the screen is updated with the window at its new size with its new contents.
We need to make sure that we fire all the fullscreen state updates inside the resize handler, synchronously, and that the paint that is triggered actually paints the window with the fully updated fullscreen window contents. Then this should look much better.

I'm going to attach a few screenshots, grabbed from a screen recording I did with QuickTime.

Comment 1

[Markus Stange [:mstange]]

Attachment: base state

Comment 2

[Markus Stange [:mstange]]

Attachment: step 1

Comment 3

[Markus Stange [:mstange]]

Attachment: step 2

Comment 4

[Markus Stange [:mstange]]

Attachment: step 3

Comment 5

[Markus Stange [:mstange]]

Attachment: step 4

Comment 6

[Markus Stange [:mstange]]

Attachment: step 5

Comment 8

[Xidorn Quan [:xidorn] UTC+11]

I'll figure out what I can do to improve this later. Note that in bug 1160014, I'm implementing a cross-platform fullscreen transition, which would make the intermediate steps (almost) invisible to the user. But this bug is still worth to fix anyway even after we have that.

Comment 9

[Xidorn Quan [:xidorn] UTC+11]

Attachment: patch 1 - freeze pres shell and bypass resize reflow during fullscreen change

Comment 10

[Xidorn Quan [:xidorn] UTC+11]

Attachment: patch 2 - freeze pres shell and resize viewport in advance in content process

Comment 11

[Xidorn Quan [:xidorn] UTC+11]

The two patches makes the fullscreen change almost atomic.

I tested this on Windows, and the result is:

For non-e10s, there are two steps:
1. the window size changes, but the content is unchanged (this step is somehow inevitable)
2. everything reaches its final state

For e10s, there are three steps:
1. the window size changes, but the chrome and content is unchanged (this step is short on e10s because we only need to wait for the chrome)
2. the chrome is hidden but the content stays unchanged (the content process always changes after the parent)
3. everything reaches its final stage


It seems this change doesn't make bug 1187769 a lot better, though. It seems to me the reflow itself is not too slow. It would need further investigation about what can we do to shorten the duration of the change.

Comment 12

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8646746 [details] [diff] [review]
patch 1 - freeze pres shell and bypass resize reflow during fullscreen change

This is rather new way to use presshell freezing (so far it has been for bfcache).
Why do we want that and not for example docshell->SetIsActive(false|true)?
Though, also that has side effects.

If we just want to postpone the resize event, I'd prefer some separate mechanism for that (some simple flag?).
Reusing Freeze/Thaw for this feels scary, given that we do things 
like MaybeReleaseCapturingContent() and UpdateImageLockingState() there.

Comment 13

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8646747 [details] [diff] [review]
patch 2 - freeze pres shell and resize viewport in advance in content process

Same here.
And please don't have raw pointers. I don't see what guarantees mPressShell isn't deleted object in FullscreenChangePrepare().

Comment 14

[Xidorn Quan [:xidorn] UTC+11]

Attachment: patch 1 - defer resize reflow and freeze refresh driver in chrome process

Comment 15

[Xidorn Quan [:xidorn] UTC+11]

Attachment: patch 2 - notice pres shell and resize viewport in advance in content process

Comment 16

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8652172 [details] [diff] [review]
patch 1 - defer resize reflow and freeze refresh driver in chrome process


>+nsGlobalWindow::SetWidgetFullscreen(bool aIsFullscreen, nsIWidget* aWidget,
>+                                    nsIScreen* aScreen)
>+{
>+  MOZ_ASSERT(IsOuterWindow());
>+
>+  if (nsCOMPtr<nsIPresShell> presShell = mDocShell->GetPresShell()) {
>+    presShell->SetIsInFullscreenChange(true);
>+  }
So here we set some presShell to be in-fullscreen-change


>+  if (nsCOMPtr<nsIPresShell> presShell = mDocShell->GetPresShell()) {
>+    presShell->SetIsInFullscreenChange(false);
>+  }
and here we clear possibly some other presshell.

I think globalwindow should store a nsWeakPtr pointing to the presshell it set to be in-fullscreen-change and then
use that when calling SetIsInFullscreenChange(false)
(PresShell inherits nsSupportsWeakReference, so nsWeakPtr should work just fine)

r+ with that.

Comment 17

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8652172 [details] [diff] [review]
patch 1 - defer resize reflow and freeze refresh driver in chrome process

>+nsIPresShell::SetIsInFullscreenChange(bool aValue)
>+{
>+  if (mIsInFullscreenChange == aValue) {
>+    NS_WARNING(aValue ? "Pres shell has been in fullscreen change?" :
>+               "Pres shell is not in fullscreen change?");
>+    return;
>+  }
>+  mIsInFullscreenChange = aValue;
>+  if (aValue) {
>+    mPresContext->RefreshDriver()->Freeze();
>+  } else {
>+    mPresContext->RefreshDriver()->Thaw();
>+  }
oh and RefreshDriver() needs null check, unfortunately.
Layout doesn't follow the same rules as DOM where methods without Get* prefix never return null.
In layout code methods without Get* tend to return null once we've started to shutdown some object.
(I would r+ a patch renaming RefreshDriver() to GetRefreshDriver())

Comment 18

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8652173 [details] [diff] [review]
patch 2 - notice pres shell and resize viewport in advance in content process

># HG changeset patch
># User Xidorn Quan <[e-mail]>
># Date 1440483219 -36000
>#      Tue Aug 25 16:13:39 2015 +1000
># Node ID 88653923b63152f10e8bcaf1e69844f518c86034
># Parent  3b0b2bdc01ae6ec7b61cae6c0cc484f4cfa29583
>Bug 1177155 part 2 - Notify the fullscreen change and resize viewport in advance in content process.
>
>diff --git a/dom/base/nsDOMWindowUtils.cpp b/dom/base/nsDOMWindowUtils.cpp
>--- a/dom/base/nsDOMWindowUtils.cpp
>+++ b/dom/base/nsDOMWindowUtils.cpp
>@@ -3185,35 +3185,144 @@ nsDOMWindowUtils::RemoteFrameFullscreenR
> 
>   nsCOMPtr<nsIDocument> doc = GetDocument();
>   NS_ENSURE_STATE(doc);
> 
>   doc->RemoteFrameFullscreenReverted();
>   return NS_OK;
> }
> 
>+class MOZ_STACK_CLASS FullscreenChangePrepare
>+{
>+public:
>+  FullscreenChangePrepare(nsIPresShell* aPresShell,
>+                          const nsSize& aSize, nsSize* aOldSize = nullptr)
>+    : mPresShell(aPresShell)
>+  {
>+    if (mPresShell) {
>+      mPresShell->SetIsInFullscreenChange(true);
>+    }
>+    if (aSize.IsEmpty()) {
>+      return;
>+    }
>+    if (nsViewManager* viewManager = mPresShell->GetViewManager()) {
>+      if (aOldSize) {
>+        viewManager->GetWindowDimensions(&aOldSize->width, &aOldSize->height);
>+      }
>+      viewManager->SetWindowDimensions(aSize.width, aSize.height);
>+    }
>+  }
>+
>+  ~FullscreenChangePrepare()
>+  {
>+    if (mPresShell) {
>+      mPresShell->Thaw();
>+    }
This doesn't look right.
ctor does SetIsInFullscreenChange(true);, but
dtor Thaw();


>+  FullscreenChangePrepare _prepare
_ prefix? drop that.

Comment 19

[Xidorn Quan [:xidorn] UTC+11]

Attachment: patch 2 - notice pres shell and resize viewport in advance in content process

Comment 20

[Xidorn Quan [:xidorn] UTC+11]

(In reply to Olli Pettay [:smaug] (high review load) from comment #16)
> >+  if (nsCOMPtr<nsIPresShell> presShell = mDocShell->GetPresShell()) {
> >+    presShell->SetIsInFullscreenChange(false);
> >+  }
> and here we clear possibly some other presshell.
> 
> I think globalwindow should store a nsWeakPtr pointing to the presshell it
> set to be in-fullscreen-change and then
> use that when calling SetIsInFullscreenChange(false)
> (PresShell inherits nsSupportsWeakReference, so nsWeakPtr should work just
> fine)

I'm not a fan of adding too many bits to a common struct like nsGlobalWindow for this short-term usage... How can this case happen? Does that happen frequently? And when the presshell changes, does the old one as well as its refresh driver get destroyed?

If the old one no longer have any effect, I think it's fine to just leave this as-is. SetIsInFullscreenChange would simply throw a warning there and do nothing further. A comment there might be worth, though.

Comment 21

[Olli Pettay [:smaug][[e-mail]]]

You're dealing with outer window, so just loading a new page causes mDocShell->GetPresShell() to return a new object. I'd say that is a common case.
And the old one is either destroyed or it is in the bfcache.

Refreshdriver dies if we're dealing with top level page (and the old page isn't moving to bfcache).

Comment 22

[Xidorn Quan [:xidorn] UTC+11]

(In reply to Olli Pettay [:smaug] (high review load) from comment #21)
> You're dealing with outer window, so just loading a new page causes
> mDocShell->GetPresShell() to return a new object. I'd say that is a common
> case.

It could be a common case in general, but shouldn't be one for fullscreen change. Also, we are dealing with the root window here, which should normally be part of the chrome. Is it really a common case for that to be changed?

> And the old one is either destroyed or it is in the bfcache.

Then probably we should clear that flag when we move it to bfcache.

> Refreshdriver dies if we're dealing with top level page (and the old page
> isn't moving to bfcache).

It suddently makes me wonder, whether I really do the right thing here... So page may use a different refresh driver which is not frozen here?

Comment 23

[Olli Pettay [:smaug][[e-mail]]]

oh, hmm, chrome process. That might not be too bad then.
Could we at least assert that when SetIsInFullscreenChange(false); is called, the presshell is in fullscreenchange state?

Comment 24

[Olli Pettay [:smaug][[e-mail]]]

and we need to assert the process type too, and possibly that we're dealing with top level window object

Comment 25

[Xidorn Quan [:xidorn] UTC+11]

Attachment: patch 1 - defer resize reflow and freeze refresh driver in chrome process

Comment 26

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8652789 [details] [diff] [review]
patch 1 - defer resize reflow and freeze refresh driver in chrome process

>+nsGlobalWindow::SetWidgetFullscreen(bool aIsFullscreen, nsIWidget* aWidget,
>+                                    nsIScreen* aScreen)
>+{
>+  MOZ_ASSERT(IsOuterWindow());
>+  MOZ_ASSERT(this == GetTop(), "Only topmost window should call this");
Since GetTop() returns this if this is called on top level content window, add also
MOZ_ASSERT(!GetFrameElementInternal());

Comment 27

[Xidorn Quan [:xidorn] UTC+11]

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

Comment 28

[Xidorn Quan [:xidorn] UTC+11]

Hmm... It seems we cannot assert before SetIsInFullscreenChange(false)... Will investigate that tomorrow...

Comment 29

[Xidorn Quan [:xidorn] UTC+11]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=141d899f1359

Comment 30

[Xidorn Quan [:xidorn] UTC+11]

Attachment: patch 1 - defer resize reflow and freeze refresh driver in chrome process

The reason was that the path for entering fullscreen for Fullscree Mode didn't set InFullscreenChange flag, which triggered the assertion.

So the change in this patch is just moving that path to nsGlobalWindow::SetWidgetFullscreen as well.

Comment 31

[Xidorn Quan [:xidorn] UTC+11]

url:        https://hg.mozilla.org/integration/mozilla-inbound/rev/3e3948004ac1732cfe444d7d3a90a2a93111ed0d
changeset:  3e3948004ac1732cfe444d7d3a90a2a93111ed0d
user:       Xidorn Quan <[e-mail]>
date:       Thu Aug 27 23:14:49 2015 +1000
description:
Bug 1177155 part 1 - Defer resize reflow and freeze refresh driver during window fullscreen change. r=smaug

url:        https://hg.mozilla.org/integration/mozilla-inbound/rev/ef599e365a7b598a4286b64b544f3badec43e124
changeset:  ef599e365a7b598a4286b64b544f3badec43e124
user:       Xidorn Quan <[e-mail]>
date:       Thu Aug 27 23:14:49 2015 +1000
description:
Bug 1177155 part 2 - Notify the fullscreen change and resize viewport in advance in content process. r=smaug

Comment 32

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/3e3948004ac1
https://hg.mozilla.org/mozilla-central/rev/ef599e365a7b

Comment 33

[Xidorn Quan [:xidorn] UTC+11]

Comment on attachment 8653187 [details] [diff] [review]
patch 1 - defer resize reflow and freeze refresh driver in chrome process

Approval Request Comment
[Feature/regressing bug #]: fullscreen transition was implemented in bug 1160014
[User impact if declined]: they would see a longer time on black screen as indicated in bug 1187769
[Describe test coverage new/current, TreeHerder]: we have various tests for fullscreen for non-e10s. this patch shouldn't change any existing observable behavior so no new test is needed
[Risks and why]: there may be some risks because it delays resize reflow and freeze the refresh driver for fullscreen change, and thus if the reverse action doesn't run as expected, we may have some problem with the whole document then
[String/UUID change made/needed]: n/a

Comment 34

[Xidorn Quan [:xidorn] UTC+11]

Comment on attachment 8652569 [details] [diff] [review]
patch 2 - notice pres shell and resize viewport in advance in content process

As the comment above. But this is changes for e10s. If we do not ship e10s on Firefox 42, we probably don't need to uplift this patch at all.

Comment 35

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8653187 [details] [diff] [review]
patch 1 - defer resize reflow and freeze refresh driver in chrome process

OK, let's take it as it seems to be a recent regression.

Comment 36

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8652569 [details] [diff] [review]
patch 2 - notice pres shell and resize viewport in advance in content process

Even if we won't ship e10s in 42, we still have e10s enabled on the aurora channel. Taking it.

Comment 37

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/d81a0e85dcc8
https://hg.mozilla.org/releases/mozilla-aurora/rev/408d44ce14a1