Bug 95650 - REGRESSION (r123837): Full screen transition is broken at apple.com
Summary: REGRESSION (r123837): Full screen transition is broken at apple.com
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P1 Major
Assignee: Jer Noble
URL: http://www.apple.com/macbook-pro/#mac...
Keywords: InRadar, Regression
Depends on:
Blocks:
 
Reported: 2012-09-01 18:01 PDT by mitz
Modified: 2013-04-09 16:56 PDT (History)
10 users (show)

See Also:


Attachments
Patch (1.45 KB, patch)
2013-04-05 14:17 PDT, Jer Noble
simon.fraser: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description mitz 2012-09-01 18:01:42 PDT
To reproduce: navigate to the URL. When the video starts playing, click the full screen arrows in the playback controls. Observe the transition into full-screen playback. Press Esc to exit full screen mode and observe the transition. Rather than animate smoothly, the video content scale changes instantaneously, while remaining clipped to its original bounds. Its position the animates to the center of the screen, without resizing. Then the size changes abruptly.

This was caused by <http://trac.webkit.org/r123837>, the fix for bug 92220.
Comment 1 mitz 2012-09-01 18:02:20 PDT
<rdar://problem/12222130>
Comment 2 Mike Lawther 2012-09-03 00:07:43 PDT
I can reproduce this, but only with Safari. I tested with the shipping Safari 6.0 (7536.25) on MacOS 10.7, and with a locally built WebKit synced past r123837.

The transition to fullscreen is transitioning from a fixed width,height of 848,480 to a a percent based width,height of 100%,100%. I have not dived into the fullscreen or video player code, but I'm assuming 100% in this case is the extent of the monitor size.

From what I see, the blend of these is performed correctly - my monitor is 2560x1600 and the values appear correct, eg:

eval'd blending of (type Fixed, value 848.000000) to (type Percent, value 100.000000) at 13.0802% = 1071.932983px
eval'd blending of (type Fixed, value 480.000000) to (type Percent, value 100.000000) at 13.0802% = 626.498230px
[...]
eval'd blending of (type Fixed, value 848.000000) to (type Percent, value 100.000000) at 84.5118% = 2294.842529px
eval'd blending of (type Fixed, value 480.000000) to (type Percent, value 100.000000) at 84.5118% = 1426.532471px

Going back out of fullscreen, the numbers don't look the same, eg:

eval'd blending of (type Percent, value 100.000000) to (type Fixed, value 848.000000) at 45.7933% = 848.000000px
eval'd blending of (type percent, value 100.000000) to (type Fixed, value 480.000000) at 45.7933% = 480.000000px
[...]
eval'd blending of (type Percent, value 100.000000) to (type Fixed, value 848.000000) at 78.0345% = 848.000000px
eval'd blending of (type Percent, value 100.000000) to (type Fixed, value 480.000000) at 78.0345% = 480.000000px

It appears the container has already been resized to 848x480, and so the blended value is always 848,480.
Comment 3 mitz 2012-10-02 10:03:29 PDT
Mike, do you have any thoughts on how to address this regression? Should we just revert r123837?
Comment 4 Mike Lawther 2012-10-02 16:41:04 PDT
From what I can see in the digging I did above, the smooth transition itself is working correctly.

My gut feel is that there is a workaround in the existing fullscreen code to make the transition smooth. Now that the underlying transition is actually smooth, the assumptions in the workaround are no longer valid, leading to weirdness.

@jer.noble - does that sound likely? 

If not, I suggest we:

 - revert r123837 (it's a dead simple patch)
 - someone helps me write a test for the existing fullscreen transition, and we land this
 - re-land the smooth transition patch, making sure fullscreen doesn't break

How does that sound?
Comment 5 Jer Noble 2012-10-02 17:21:53 PDT
(In reply to comment #4)
> From what I can see in the digging I did above, the smooth transition itself is working correctly.
> 
> My gut feel is that there is a workaround in the existing fullscreen code to make the transition smooth. Now that the underlying transition is actually smooth, the assumptions in the workaround are no longer valid, leading to weirdness.
> 
> @jer.noble - does that sound likely? 

The blending is happening at the window server level.  We resize the view to take up the entire screen, then ask for the element's screen rect.  The window server then interpolates from the original screen rect to the final one.

Before r123837, the full screen element's renderer's contentsBox() returned the full screen rect.
After r123837, the full screen element's renderer's contentsBox() returns the original box.

> If not, I suggest we:
> 
>  - revert r123837 (it's a dead simple patch)
>  - someone helps me write a test for the existing fullscreen transition, and we land this

There are some helper functions in Internals that let you do processing between when full screen starts and ends.  That would be a good time to check the bounds of the full screen element.
Comment 6 Mike Lawther 2012-10-02 18:25:55 PDT
> The blending is happening at the window server level.  We resize the view to take up the entire screen, then ask for the element's screen rect.  The window server then interpolates from the original screen rect to the final one.
> 
> Before r123837, the full screen element's renderer's contentsBox() returned the full screen rect.
> After r123837, the full screen element's renderer's contentsBox() returns the original box.

This sounds like the assumption that has been violated. Prior to r123837, the transition between a fixed size and a percent would 'snap' instantly to the final state. 

Is the fix here is to simply remove the CSS transition upon the view resize? It sounds like you actually want it to be instantaneous.
Comment 7 Mike Lawther 2012-11-01 16:21:19 PDT
Hey Dan, Jer - did you try removing the CSS transition on the view resize? It really feels like this is the right solution here, as Jer points out in this case the blending is happening at the window server level.
Comment 8 Jer Noble 2012-11-01 16:30:52 PDT
(In reply to comment #7)
> Hey Dan, Jer - did you try removing the CSS transition on the view resize? It really feels like this is the right solution here, as Jer points out in this case the blending is happening at the window server level.

I'm confused.  Is there a CSS transition which is being added to the page through a style sheet?
Comment 9 Mike Lawther 2012-11-01 17:16:52 PDT
There is definitely a CSS transition happening there, evidenced both by what you describe, and the measurements I took in comment #2. It's a transition from a fixed size (eg a pixel size of 848x480) to a percentage size (100%x100%).

This is the assumption that I reckon has been violated - prior to r123837, a mixed unit transition would instantly 'snap' from begin to end, with no interpolation. And so the window server code provides the interpolation instead (I'm guessing animateFromRect in Source/WebCore/platform/mac/WebVideoFullscreenController.mm). Now, the CSS transition actually interpolates as well, and there are too many cooks in the animation kitchen.

I'm afraid I don't know where the CSS transition is coming from - I had a quick dig around in likely sounding culprits such as Source/WebCore/css/fullscreenQuickTime.css.

My Mac dev machine is currently hosed, so I can't check any of this stuff :(
Comment 10 Jer Noble 2013-04-05 14:17:27 PDT
Created attachment 196681 [details]
Patch
Comment 11 Jer Noble 2013-04-09 16:56:17 PDT
Committed r148065: <http://trac.webkit.org/changeset/148065>