Full screen video transition should be atomic

RESOLVED FIXED in Firefox 42

Status

()

Core
General
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: mstange, Assigned: xidorn)

Tracking

(Blocks: 2 bugs)

Trunk
mozilla43
x86_64
Mac OS X
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox41 unaffected, firefox42 fixed, firefox43 fixed)

Details

Attachments

(8 attachments, 5 obsolete attachments)

3.06 MB, image/png
Details
3.06 MB, image/png
Details
3.35 MB, image/png
Details
1.97 MB, image/png
Details
6.22 MB, image/png
Details
6.27 MB, image/png
Details
4.40 KB, patch
smaug
: review+
Details | Diff | Splinter Review
9.25 KB, patch
smaug
: review+
Details | Diff | Splinter Review
(Reporter)

Description

3 years ago
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.
(Reporter)

Comment 1

3 years ago
Created attachment 8625809 [details]
base state
(Reporter)

Comment 2

3 years ago
Created attachment 8625810 [details]
step 1
(Reporter)

Comment 3

3 years ago
Created attachment 8625811 [details]
step 2
(Reporter)

Comment 4

3 years ago
Created attachment 8625812 [details]
step 3
(Reporter)

Comment 5

3 years ago
Created attachment 8625813 [details]
step 4
(Reporter)

Comment 6

3 years ago
Created attachment 8625814 [details]
step 5
(Assignee)

Updated

3 years ago
Blocks: 1121280
(Assignee)

Updated

3 years ago
Depends on: 1168705
(Assignee)

Updated

3 years ago
Duplicate of this bug: 1181904
(Assignee)

Comment 8

3 years ago
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.
(Assignee)

Comment 9

3 years ago
Created attachment 8646746 [details] [diff] [review]
patch 1 - freeze pres shell and bypass resize reflow during fullscreen change
Attachment #8646746 - Flags: review?(bugs)
(Assignee)

Comment 10

3 years ago
Created attachment 8646747 [details] [diff] [review]
patch 2 - freeze pres shell and resize viewport in advance in content process
Assignee: nobody → quanxunzhen
Attachment #8646747 - Flags: review?(bugs)
(Assignee)

Comment 11

3 years ago
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 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.
Attachment #8646746 - Flags: review?(bugs) → review-
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().
Attachment #8646747 - Flags: review?(bugs) → review-
(Assignee)

Comment 14

3 years ago
Created attachment 8652172 [details] [diff] [review]
patch 1 - defer resize reflow and freeze refresh driver in chrome process
Attachment #8646746 - Attachment is obsolete: true
(Assignee)

Updated

3 years ago
Attachment #8652172 - Flags: review?(bugs)
(Assignee)

Comment 15

3 years ago
Created attachment 8652173 [details] [diff] [review]
patch 2 - notice pres shell and resize viewport in advance in content process
Attachment #8646747 - Attachment is obsolete: true
Attachment #8652173 - Flags: review?(bugs)
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.
Attachment #8652172 - Flags: review?(bugs) → review+
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 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 <quanxunzhen@gmail.com>
># 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.
Attachment #8652173 - Flags: review?(bugs) → review-
(Assignee)

Comment 19

3 years ago
Created attachment 8652569 [details] [diff] [review]
patch 2 - notice pres shell and resize viewport in advance in content process
Attachment #8652173 - Attachment is obsolete: true
Attachment #8652569 - Flags: review?(bugs)

Updated

3 years ago
Attachment #8652569 - Flags: review?(bugs) → review+
(Assignee)

Comment 20

3 years ago
(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.
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).
(Assignee)

Comment 22

3 years ago
(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?
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?
and we need to assert the process type too, and possibly that we're dealing with top level window object
(Assignee)

Comment 25

3 years ago
Created attachment 8652789 [details] [diff] [review]
patch 1 - defer resize reflow and freeze refresh driver in chrome process
Attachment #8652172 - Attachment is obsolete: true
Attachment #8652789 - Flags: review?(bugs)
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());
Attachment #8652789 - Flags: review?(bugs) → review+
(Assignee)

Comment 28

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

Comment 30

3 years ago
Created attachment 8653187 [details] [diff] [review]
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.
Attachment #8652789 - Attachment is obsolete: true
Attachment #8653187 - Flags: review?(bugs)

Updated

3 years ago
Attachment #8653187 - Flags: review?(bugs) → review+
(Assignee)

Comment 31

3 years ago
url:        https://hg.mozilla.org/integration/mozilla-inbound/rev/3e3948004ac1732cfe444d7d3a90a2a93111ed0d
changeset:  3e3948004ac1732cfe444d7d3a90a2a93111ed0d
user:       Xidorn Quan <quanxunzhen@gmail.com>
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 <quanxunzhen@gmail.com>
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
https://hg.mozilla.org/mozilla-central/rev/3e3948004ac1
https://hg.mozilla.org/mozilla-central/rev/ef599e365a7b
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox43: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
(Assignee)

Comment 33

3 years ago
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
Attachment #8653187 - Flags: approval-mozilla-aurora?
(Assignee)

Comment 34

3 years ago
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.
Attachment #8652569 - Flags: approval-mozilla-aurora?
(Assignee)

Updated

3 years ago
Depends on: 1200766
status-firefox42: --- → affected
status-firefox41: affected → unaffected
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.
Attachment #8653187 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
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.
Attachment #8652569 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+

Updated

3 years ago
Depends on: 1204285
(Assignee)

Updated

3 years ago
Depends on: 1205194
(Assignee)

Updated

3 years ago
No longer depends on: 1204285
(Assignee)

Updated

3 years ago
Blocks: 708174

Updated

2 years ago
Depends on: 1238536
You need to log in before you can comment on or make changes to this bug.