Bug 93292 – [Chromium] Pinch Zoom prototype tracking issue.

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Jeff Timanus]

This issue is to track the work required to implement accelerated pinch-zoom in the Chromium compositor.

[Jeff Timanus]

Created attachment 156756 [details]
Hacking around - first protypes

[Jeff Timanus]

Comment on attachment 156756 [details]
Hacking around - first protypes

View in context: https://bugs.webkit.org/attachment.cgi?id=156756&action=review

This change is very rough, but I wanted to upload it to be explicit about the changes I'm trying to make.  Should this be the beginning of the correct direction?

Does your recent change, https://bugs.webkit.org/attachment.cgi?id=155910, have an impact on the plans discussed?

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:768
> +#if 0

I removed this call, as part of the first steps to correct the scrolling behaviour, as we discussed.

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:1045
> +#if 0

I found that the resulting zoom behaviour is very jumpy when removing these optimization calls.  Is this potentially expected?  Is the recalculation of the zoom causing layout to be performed?

[Alexandre Elias]

(In reply to comment #2)
> (From update of attachment 156756 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=156756&action=review
> 
> This change is very rough, but I wanted to upload it to be explicit about the changes I'm trying to make.  Should this be the beginning of the correct direction?

Seems fine.  Right now you just removed pageScaleFactor entirely instead of changing the way it's applied though (any pinch zoom you still see is due to pageScaleDelta only).  Next thing is to actually apply it the new way in both calls to CCLayerTreeHostCommon::calculateDrawTransforms().

> 
> Does your recent change, https://bugs.webkit.org/attachment.cgi?id=155910, have an impact on the plans discussed?

No change to the plans.  It's a future-looking cleanup that will allow doing the coordinate switch on Android, but it doesn't have much impact on you.

> 
> > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:768
> > +#if 0
> 
> I removed this call, as part of the first steps to correct the scrolling behaviour, as we discussed.
> 
> > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:1045
> > +#if 0
> 
> I found that the resulting zoom behaviour is very jumpy when removing these optimization calls.  Is this potentially expected?  Is the recalculation of the zoom causing layout to be performed?

By removing that whole block, the current pinch state is being sent to the main thread incrementally as fast as possible.  So any bugs in the pinch implementation will be seen continually, instead of only when you let go of the pinch.  If there are no bugs, then you will just see a lot of checkerboard on the edges while zooming out.  Anyway, I recommend only #if 0'ing these lines:

#if 0
if (m_pinchGestureActive)
    computePinchZoomDeltas(scrollInfo.get());
else if (m_pageScaleAnimation.get())
    computeDoubleTapZoomDeltas(scrollInfo.get());
#endif

as these are the actually coordinate-space sensitive optimizations.

[Jeff Timanus]

Created attachment 158123 [details]
Patch attempting to apply zoom without Frame::frameScaleFactor.

[Jeff Timanus]

Created attachment 158129 [details]
Effect of zoom out from patch 158123.

[Jeff Timanus]

Created attachment 159943 [details]
Patch that updates (in a very hacky way), LayerChromium::contentsScale.

[Jeff Timanus]

Created attachment 160035 [details]
Apply scale to root scrolling layer.

This patch finally gets a basic pinch-zoom working without using a CSS-scale.

 - The LayerChromium::contentsScale is assigned for all layer descendents of the root scroll layer, so that the tiles are painted at the proper scale.
 - The page-scale-factor is installed as a transform on the root scrolling layer in the main thread, just before calculating the drawing transforms.  The previous approach of applying the scale to the entire tree was the root cause of the viewport snapping to a smaller size on pinch-zoom release.

This is still a prototyping patch, but demonstrates that a non-CSS zoom is possible.

Glaring problems that still need correction:
 - How to separate this zooming behaviour from the old zoom?  Some ports likely want to preserve frameScale, etc.  
 - LCD text is used when the page is rendered during the interactive zoom - this can cause colour sparkling.  LCD text needs to be disabled when zoom starts.
 - The scroll position is totally broken after a pinch release, and needs to be corrected.

[Adrienne Walker]

(In reply to comment #6)
> Created an attachment (id=159943) [details]
> Patch that updates (in a very hacky way), LayerChromium::contentsScale.

If you don't mind, I think I'd like to have this in a separate prerequisite patch, ideally one that also removes the contentsScale-related code from GraphicsLayerChromium and WebContentLayer.

I think there is definitely more to it than what you do here, because although this handles page scale, you're calling updateLayerScale with just the PSF, whereas GraphicsLayerChromium sets the contentsScale to DSF * PSF.  Although, as another wrinkle, I think this is different on the NCCH layer.

[Jeff Timanus]

(In reply to comment #8)
> (In reply to comment #6)
> > Created an attachment (id=159943) [details] [details]
> > Patch that updates (in a very hacky way), LayerChromium::contentsScale.
> 
> If you don't mind, I think I'd like to have this in a separate prerequisite patch, ideally one that also removes the contentsScale-related code from GraphicsLayerChromium and WebContentLayer.
> 
> I think there is definitely more to it than what you do here, because although this handles page scale, you're calling updateLayerScale with just the PSF, whereas GraphicsLayerChromium sets the contentsScale to DSF * PSF.  Although, as another wrinkle, I think this is different on the NCCH layer.

I'm looking into the correct way to remove the contentsScale code from these classes, but I'm unsure of the correct location to relocate this scale.

My plan is to remove the scale, and try to place it in the CCLTH, with the hope that the unit tests will highlight the regressions due to corner cases.  

Is my present solution of applying it during CCLayerTreeHost::updateLayers(...) reasonable?  (Assuming that I also apply the device scale factor correctly.)

I'm still not quite sure how to deal with the different scaling behaviour in the NonCompositedContentHost, but am discussing the issue with danakj@.

[Adrienne Walker]

(In reply to comment #9)
> (In reply to comment #8)
> > (In reply to comment #6)
> > > Created an attachment (id=159943) [details] [details] [details]
> > > Patch that updates (in a very hacky way), LayerChromium::contentsScale.
> > 
> > If you don't mind, I think I'd like to have this in a separate prerequisite patch, ideally one that also removes the contentsScale-related code from GraphicsLayerChromium and WebContentLayer.
> > 
> > I think there is definitely more to it than what you do here, because although this handles page scale, you're calling updateLayerScale with just the PSF, whereas GraphicsLayerChromium sets the contentsScale to DSF * PSF.  Although, as another wrinkle, I think this is different on the NCCH layer.
> 
> I'm looking into the correct way to remove the contentsScale code from these classes, but I'm unsure of the correct location to relocate this scale.

Those classes are just the indirect callers of LayerChromium::setContentsScale. Moving the calling code to somewhere like CCLTH or CCLTHCommon where you can call it on every layer every update seems like the right way to relocate this.

The only downside of setting contents scale late in the update pass is that any invalidations you get are in the wrong space (because contentBounds is wrong). However, it looks like when you change the contents scale, then we correctly invalidate the entire layer.  Those invalidations are now all moot, regardless of whether you set the contents scale before or after the invalidations.  So, I think it's still correct to set contents scale ta the beginning of update.

> My plan is to remove the scale, and try to place it in the CCLTH, with the hope that the unit tests will highlight the regressions due to corner cases.  

That will get you most of the way there.  I don't think we have good testing for the NonCompositedContentHost other than via layout tests.  I think (but am not 100% positive) that we have some device scale compositing layout tests, so maybe you're covered there too.

> Is my present solution of applying it during CCLayerTreeHost::updateLayers(...) reasonable?  (Assuming that I also apply the device scale factor correctly.)

Seems pretty reasonable.  I would either put it there or add it as part of the "calculateDrawTransforms" pass in CCLayerTreeHostCommon where we already recurse through the entire layer tree and update a bunch of derived layer state.

[Dana Jansens]

(In reply to comment #10)
> That will get you most of the way there.  I don't think we have good testing for the NonCompositedContentHost other than via layout tests.  I think (but am not 100% positive) that we have some device scale compositing layout tests, so maybe you're covered there too.

Turning on DSF layout tests is still being worked on I believe. tdanderson@ should know the state of it all though.

> > Is my present solution of applying it during CCLayerTreeHost::updateLayers(...) reasonable?  (Assuming that I also apply the device scale factor correctly.)
> 
> Seems pretty reasonable.  I would either put it there or add it as part of the "calculateDrawTransforms" pass in CCLayerTreeHostCommon where we already recurse through the entire layer tree and update a bunch of derived layer state.

calcDraw will be harder because this is a main-thread-only construct. So updateLayers is probably the best place.

[Jeff Timanus]

Created attachment 163130 [details]
Alpha version of the new pinch zoom algorithm.

Patch indicating a general idea of the new pinch zoom algorithm.

Style and variable naming still need to be addressed, but there are functional issues that I'm still trying to track down.  The naming of the blue-box viewport is to correspond with the terminology of the new design.

High-level summary:
 - On pinch, the Impl thread maintains a 'blue box' viewport.  The entire page is scaled, and this viewport moves within the scaled page.  
 - Movement of the blue-box viewport is not reflected by the current scroll-bar position.  This viewport can only be positioned via a pinch-zoom move, or during a scroll-to-edge-of-page event.
 - During scroll, the main page is scrolled first.  If it is not possible for the page to scroll, then the blue box viewport applies the scroll.

I'd like to request some assistance resolving bugs in the following areas.  Please look for in-line comments for further explanation.
 - The center of the pinch zoom is not correct.  I believe this is because of either the order that the transformations are applied, or because I'm confusing the coordinates in which the layers reside.
 - The performance of the scroll is very bad.  I think this is because I'm somehow triggering re-painting of the tiles.
 - I am unable to scroll to the bottom of the page.  This is likely a max-scroll position problem.

[Jeff Timanus]

Comment on attachment 163130 [details]
Alpha version of the new pinch zoom algorithm.

View in context: https://bugs.webkit.org/attachment.cgi?id=163130&action=review

I have a few questions about the correctness of the approach that I've taken here.  Can reviewers/experts please remark on if this is the correct path?

The focus here is the architecture of the change.  I would like correct style, and bike-shed the variable names in a subsequent review.

> WebKit/chromium/src/WebViewImpl.cpp:212
> +const float WebView::minPageScaleFactor = 1.0;

For the moment, I modified this constant to prevent zoom-out.  This simplifies the scrolling logic, for the moment.

Zoom-in is much more critical in terms of feature-set.

> WebCore/platform/graphics/chromium/cc/CCThreadProxy.cpp:479
> +    m_pendingBeginFrameRequest->blueBox = m_layerTreeHostImpl->blueBox();

When beginning a new frame, push the blue box coordinates to the main thread, so that the translation offsets can be used in the calculation of the tiles that need to be drawn.

Note that I do not follow the m_sentPageScaleFactor model of the CCLayerTreeHostImpl class.  I send the real-time position of the box.  This should be safe, because a translation will potentially only result in new tiles at the boundary of the viewport being painted, correct?

> WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:323
> +        m_rootScrollLayerImpl->setTransform(scaleMatrix);

Apply the transformation to position the 'blue box' viewport in the impl thread to the root scroll layer.

I'm concerned that there may be a problem here with the order of transformations.  During pinch zoom, the transformation is first applied via the page scale delta, and then the application of this matrix.  After pinch zoom, the delta is pushed into m_pageScale, and so the entire scaling is performed in this matrix.

> WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:920
> +    maxScroll.scale(1 / (m_deviceScaleFactor * m_pageScale));

This is not sufficient to allow scrolling to the bottom of the page.  Is there another scroll factor that needs to be updated?  I'm presently debugging this.

> WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:1032
> +        viewport->applyScroll(unscrolled);

Apply the portion of the scroll that was outside of the m_maxScrollPosition to the blue-box viewport, to allow scrolling into the zoomed in portion of the page.

> WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:502
> +    if (false /*layer->boundsContainPageScale()*/)

I've come across a problem here with the current page scale implementation.

Respecting the boundsContainPageScale prevents the root scroll layer from being re-painted at the correct resolution after pinch zoom release.  I don't think that it's safe to blindly disregard this setting.

My understanding is that the NCCH is corresponding to the root scroll layer.  Because of this the page-scale semantics may need to be changed.  These layers need to be re-painted at the correct resolution, and the only means I know of to do this is via page scale.  

Can someone chime in on if it's possible to entirely remove the boundsContainPageScale flag, so that scale is applied uniformly?

> WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:541
> +                              -m_blueBoxLocation.y());

From how I understand the system, the calculateDrawTransforms(...) routine is invoked to compute the set of tiles needed for display.

I pass the page scale and blue box position to ensure that the correct set of tiles are ready for use in the impl thread.

Question:  Are there performance/correctness concerns with this approach?

[Jeff Timanus]

There are other correctness issues with this change:  

Flash elements are not scaled properly during the zoom.

For example, watching a clip on youtube has the following behaviour:
 - During pinch zoom there is no change to the scale/positioning of the flash player
 - On pinch release, the flash element is re-positioned, but at an incorrect location.  The scale is still not modified.

Strangly, some flash elements still render correctly:
 - The pre-roll ad on this flash game scales properly during pinch zoom.
 - Once the game starts, the game itself exhibits similar problems to the youtube issue above.
 - http://www.addictinggames.com/action-games/soul-searchin-game.jsp

Also, fixed position elements are not scrolled at the same rate as the page when a page scale is applied.

[Jeff Timanus]

(In reply to comment #14)
> There are other correctness issues with this change:  
> 
> Flash elements are not scaled properly during the zoom.
> 
> For example, watching a clip on youtube has the following behaviour:
>  - During pinch zoom there is no change to the scale/positioning of the flash player
>  - On pinch release, the flash element is re-positioned, but at an incorrect location.  The scale is still not modified.
> 
> Strangly, some flash elements still render correctly:
>  - The pre-roll ad on this flash game scales properly during pinch zoom.
>  - Once the game starts, the game itself exhibits similar problems to the youtube issue above.
>  - http://www.addictinggames.com/action-games/soul-searchin-game.jsp
> 
> Also, fixed position elements are not scrolled at the same rate as the page when a page scale is applied.

Shawn, can you please have a look at this?  I'm reviewing the scroll compensation path in CCLayerTreeHostCommon to determine if it is not taking page scale into account, but have found any solid leads.

[Dana Jansens]

Comment on attachment 163130 [details]
Alpha version of the new pinch zoom algorithm.

View in context: https://bugs.webkit.org/attachment.cgi?id=163130&action=review

> WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:117
> +        overflow.setWidth(m_redBounds.width() - localBounds.maxX());

Negative overflow width? (I guess this is unused atm)

> WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:321
> +        scaleMatrix.scale(m_pageScale);
> +#if 1
> +        scaleMatrix.translate(-m_blueBox.location().x(),
> +                              -m_blueBox.location().y());

FYI in this order, the translate will also get scaled.

>> WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:323
>> +        m_rootScrollLayerImpl->setTransform(scaleMatrix);
> 
> Apply the transformation to position the 'blue box' viewport in the impl thread to the root scroll layer.
> 
> I'm concerned that there may be a problem here with the order of transformations.  During pinch zoom, the transformation is first applied via the page scale delta, and then the application of this matrix.  After pinch zoom, the delta is pushed into m_pageScale, and so the entire scaling is performed in this matrix.

I think we should apply the scaling the same way we apply deviceScaleFactor, by passing it to calcDrawTransforms. Not by setting a transform on any layers here. You should be able to just include it with the m_deviceScaleFactor being passed to calcDraw below.

I think the translation should go in the same way, and it can be set on the deviceScaleTransform inside CCLTHCommon, instead of setting it on a layer also. In terms of ordering, I think it would be:
deviceScaleTransform.scale(deviceScaleFactor)
deviceScaleTransform.scale(pageScaleFactor)
deviceScaleTransform.translate(-blueBoxOffset)

This is assuming the blueBox position is in logical pixels (ie. redBox pixels/layout pixels)

> WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:913
> -            viewBounds.scale(m_deviceScaleFactor);
> +            viewBounds.scale(m_deviceScaleFactor * m_pageScale);

I think viewBounds should be the number of physical pixels that can be displayed. So it should not be scaled by pageScale.

>> WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:920
>> +    maxScroll.scale(1 / (m_deviceScaleFactor * m_pageScale));
> 
> This is not sufficient to allow scrolling to the bottom of the page.  Is there another scroll factor that needs to be updated?  I'm presently debugging this.

This should be correct. You're getting a size in the layer's contentSpace, which will include the pageScale now. I think maybe taking out the pageScale above should fix things up?

maxScroll is "in content pixels" might be better said than "in physical pixels". And scroll positions are "in logical pixels".

> WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:1066
> -    pendingDelta.scale(m_deviceScaleFactor);
> +    pendingDelta.scale(m_deviceScaleFactor * m_pageScale);

This scale should really be moved into the scrollLayerWithScreenSpaceDelta() function, given the name of the function. But I'm now getting confused about scrollOffset being content pixels or logical pixels and seeing inconsistency, I will keep reading to figure this out.

[Dana Jansens]

Comment on attachment 163130 [details]
Alpha version of the new pinch zoom algorithm.

View in context: https://bugs.webkit.org/attachment.cgi?id=163130&action=review

>> WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:1066
>> +    pendingDelta.scale(m_deviceScaleFactor * m_pageScale);
> 
> This scale should really be moved into the scrollLayerWithScreenSpaceDelta() function, given the name of the function. But I'm now getting confused about scrollOffset being content pixels or logical pixels and seeing inconsistency, I will keep reading to figure this out.

After more reading I think this line is in the wrong place, but it's doing the "right thing" in the wrong place. This assumes that the screenSpaceTransform() for the layer has a pageScale built in, which it will, so yay. But its wrong if m_scrollDeltaIsInScreenSpace is false.

>> WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:502
>> +    if (false /*layer->boundsContainPageScale()*/)
> 
> I've come across a problem here with the current page scale implementation.
> 
> Respecting the boundsContainPageScale prevents the root scroll layer from being re-painted at the correct resolution after pinch zoom release.  I don't think that it's safe to blindly disregard this setting.
> 
> My understanding is that the NCCH is corresponding to the root scroll layer.  Because of this the page-scale semantics may need to be changed.  These layers need to be re-painted at the correct resolution, and the only means I know of to do this is via page scale.  
> 
> Can someone chime in on if it's possible to entirely remove the boundsContainPageScale flag, so that scale is applied uniformly?

We should remove it, as the goal of this change is to make all layers have !boundsContainPageScale(). Also stop setting the flag to true in NCCH again (I know you just added that :))

[Jeff Timanus]

Comment on attachment 163130 [details]
Alpha version of the new pinch zoom algorithm.

View in context: https://bugs.webkit.org/attachment.cgi?id=163130&action=review

>> WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:117
>> +        overflow.setWidth(m_redBounds.width() - localBounds.maxX());
> 
> Negative overflow width? (I guess this is unused atm)

Good point.  I think there may be an error here.

This code path is used for when the user is pinch-zoom-moving (scrolling while pinched) to position the blue box viewport.  If the viewport is adjacent to the edge of the red-viewport, then the scroll overflow will be used to attempt to scroll the root scroll layer.

You are correct, the overflow should be positive in this case, and below.

>> WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:321
>> +                              -m_blueBox.location().y());
> 
> FYI in this order, the translate will also get scaled.

Thanks for the remark.

This was a problem with the translation offsets.  Because they were being scaled, the blue box was scrolling at a rate proportional to the scale - hence it would not 'stick' to the user's fingerpress.  I believe this may be in relation to your comment in CCLayerTreeHostImpl::scrollBy.

However, by adding a scaling factor of 1/m_pageScale to the transformation, I am getting incorrect results on pinch-release.  I believe this may be due to the scale moving from m_pageScaleDelta to m_pageScale, and the order of transformations in CCLTHC.  Investigating.

>>> WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:323
>>> +        m_rootScrollLayerImpl->setTransform(scaleMatrix);
>> 
>> Apply the transformation to position the 'blue box' viewport in the impl thread to the root scroll layer.
>> 
>> I'm concerned that there may be a problem here with the order of transformations.  During pinch zoom, the transformation is first applied via the page scale delta, and then the application of this matrix.  After pinch zoom, the delta is pushed into m_pageScale, and so the entire scaling is performed in this matrix.
> 
> I think we should apply the scaling the same way we apply deviceScaleFactor, by passing it to calcDrawTransforms. Not by setting a transform on any layers here. You should be able to just include it with the m_deviceScaleFactor being passed to calcDraw below.
> 
> I think the translation should go in the same way, and it can be set on the deviceScaleTransform inside CCLTHCommon, instead of setting it on a layer also. In terms of ordering, I think it would be:
> deviceScaleTransform.scale(deviceScaleFactor)
> deviceScaleTransform.scale(pageScaleFactor)
> deviceScaleTransform.translate(-blueBoxOffset)
> 
> This is assuming the blueBox position is in logical pixels (ie. redBox pixels/layout pixels)

Yes, the assignment of the transform here feels hacky to me.

My concerns with passing it into calcDrawTransforms are the following:
 a) Only the root-scrolling-layer should be scaled, which means that the recursive descent in calculateDrawingTransforms would have to pick out this layer to apply the transformation.
 b) My earlier patches in this issue tried this approach of combining the page-scale-factor directly into the device-scale-factor, and found it to be problematic because it scales the entire layer-tree.  See the attached image on the issue to observe the effect.

>> WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:913
>> +            viewBounds.scale(m_deviceScaleFactor * m_pageScale);
> 
> I think viewBounds should be the number of physical pixels that can be displayed. So it should not be scaled by pageScale.

Yes, I think you are right here.

Removed.

>>> WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:920
>>> +    maxScroll.scale(1 / (m_deviceScaleFactor * m_pageScale));
>> 
>> This is not sufficient to allow scrolling to the bottom of the page.  Is there another scroll factor that needs to be updated?  I'm presently debugging this.
> 
> This should be correct. You're getting a size in the layer's contentSpace, which will include the pageScale now. I think maybe taking out the pageScale above should fix things up?
> 
> maxScroll is "in content pixels" might be better said than "in physical pixels". And scroll positions are "in logical pixels".

Noted.

Unfortunately, the above suggestion did not resolve the scroll-to-page-boundaries issue.

>>> WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:1066
>>> +    pendingDelta.scale(m_deviceScaleFactor * m_pageScale);
>> 
>> This scale should really be moved into the scrollLayerWithScreenSpaceDelta() function, given the name of the function. But I'm now getting confused about scrollOffset being content pixels or logical pixels and seeing inconsistency, I will keep reading to figure this out.
> 
> After more reading I think this line is in the wrong place, but it's doing the "right thing" in the wrong place. This assumes that the screenSpaceTransform() for the layer has a pageScale built in, which it will, so yay. But its wrong if m_scrollDeltaIsInScreenSpace is false.

Good point.  I'll look into moving it.

>>> WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:502
>>> +    if (false /*layer->boundsContainPageScale()*/)
>> 
>> I've come across a problem here with the current page scale implementation.
>> 
>> Respecting the boundsContainPageScale prevents the root scroll layer from being re-painted at the correct resolution after pinch zoom release.  I don't think that it's safe to blindly disregard this setting.
>> 
>> My understanding is that the NCCH is corresponding to the root scroll layer.  Because of this the page-scale semantics may need to be changed.  These layers need to be re-painted at the correct resolution, and the only means I know of to do this is via page scale.  
>> 
>> Can someone chime in on if it's possible to entirely remove the boundsContainPageScale flag, so that scale is applied uniformly?
> 
> We should remove it, as the goal of this change is to make all layers have !boundsContainPageScale(). Also stop setting the flag to true in NCCH again (I know you just added that :))

Will there be larger ramifications by removing it?  Will other assumptions in NCCH be broken?

[Dana Jansens]

Comment on attachment 163130 [details]
Alpha version of the new pinch zoom algorithm.

View in context: https://bugs.webkit.org/attachment.cgi?id=163130&action=review

>>>> WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:1066
>>>> +    pendingDelta.scale(m_deviceScaleFactor * m_pageScale);
>>> 
>>> This scale should really be moved into the scrollLayerWithScreenSpaceDelta() function, given the name of the function. But I'm now getting confused about scrollOffset being content pixels or logical pixels and seeing inconsistency, I will keep reading to figure this out.
>> 
>> After more reading I think this line is in the wrong place, but it's doing the "right thing" in the wrong place. This assumes that the screenSpaceTransform() for the layer has a pageScale built in, which it will, so yay. But its wrong if m_scrollDeltaIsInScreenSpace is false.
> 
> Good point.  I'll look into moving it.

Im making a bug regarding this line, there are deeper issues afoot.

>>>> WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:502
>>>> +    if (false /*layer->boundsContainPageScale()*/)
>>> 
>>> I've come across a problem here with the current page scale implementation.
>>> 
>>> Respecting the boundsContainPageScale prevents the root scroll layer from being re-painted at the correct resolution after pinch zoom release.  I don't think that it's safe to blindly disregard this setting.
>>> 
>>> My understanding is that the NCCH is corresponding to the root scroll layer.  Because of this the page-scale semantics may need to be changed.  These layers need to be re-painted at the correct resolution, and the only means I know of to do this is via page scale.  
>>> 
>>> Can someone chime in on if it's possible to entirely remove the boundsContainPageScale flag, so that scale is applied uniformly?
>> 
>> We should remove it, as the goal of this change is to make all layers have !boundsContainPageScale(). Also stop setting the flag to true in NCCH again (I know you just added that :))
> 
> Will there be larger ramifications by removing it?  Will other assumptions in NCCH be broken?

I don't think so as long as the contents size for the layer is correct. You should verify that view->contentsSize() is not being affected by page scale anymore in WebViewImpl::updateLayerTreeViewport.

Also WebViewImpl::didChangeContentsSize should not have anything to do with page scale anymore, but this is inside ENABLE(VIEWPORT) so maybe we dont care about it at the moment.

[Jeff Timanus]

Created attachment 163238 [details]
Remove references to blue-box position from the main thread.

From a discussion with danakj@, we decided that it would be simpler if the main thread prepared all of the necessary tiles for the pinch zoom.  

Previously, the main thread applied a scale corresponding to the page-scale, and translation corresponding to the blue-box viewport to the set of drawing transforms.  This ensured that only those tiles visible from the zoomed viewport would be painted, at the expense of some complexity.  By removing these transforms, all the tiles are prepared at a resolution scaled by the page scale.

The downside is that for high-scale-factors many tiles are prepared, when only a few are visible in the blue-box viewport.  For a v1 implementation, this is an acceptable trade-off.

[Jeff Timanus]

Created attachment 163364 [details]
Pinch zoom with correct fixed position offsets.

This patch shows how to resolve the problems with regards to the following:
 - Fixed position element positions during scroll.
 - Scrolling to the end of the page.

The previous problems were a result of the order of transformations in CCLTHC.  

During pinch zoom, the scroll is applied after the application of the page scale delta.  This meant that scroll offsets are multiplied by the page scale delta.  In my previous patches, I had been using the layer's transform to apply the page scale to the layer.  This transform happens after the translation for scrolling.  As a result, after pinch, when the delta is reset to 1, the scroll offsets were no longer being scaled.  It was not possible to scroll to the extents of the page because the transform was incorrect.

Because I now scroll the root-scroll-layer at the correct rate with respect to the page scale, the positioning of the fixed-position-elements is also correct.  They were already moving at the correct scrolling rate.

Fallout from this change is the following issue:
 - scrolling in screen-space is incorrect, because I did not modify the code to take into consideration these new transforms.
 - The use of CCLayerTreeHostImpl::contentBounds changed when updating the max scroll position.  Now all scrolling is performed entirely in layout space.  The page scale should have no effect on the current scroll offset, or the maximum extents of the scroll.  (Is this an ok assumption?)

Remaining high-level issues:
 - Scaling of flash elements is still broken, as previously noted.
 - I still disregard the boundsContainContentScale setting in CCLayerTreeHost to ensure that the tiles are re-painted at the correct resolution for the scale.

[Jeff Timanus]

(In reply to comment #21)
> Created an attachment (id=163364) [details]
> Pinch zoom with correct fixed position offsets.
> 
> This patch shows how to resolve the problems with regards to the following:
>  - Fixed position element positions during scroll.
>  - Scrolling to the end of the page.
> 
> The previous problems were a result of the order of transformations in CCLTHC.  
> 
> During pinch zoom, the scroll is applied after the application of the page scale delta.  This meant that scroll offsets are multiplied by the page scale delta.  In my previous patches, I had been using the layer's transform to apply the page scale to the layer.  This transform happens after the translation for scrolling.  As a result, after pinch, when the delta is reset to 1, the scroll offsets were no longer being scaled.  It was not possible to scroll to the extents of the page because the transform was incorrect.
> 
> Because I now scroll the root-scroll-layer at the correct rate with respect to the page scale, the positioning of the fixed-position-elements is also correct.  They were already moving at the correct scrolling rate.
> 
> Fallout from this change is the following issue:
>  - scrolling in screen-space is incorrect, because I did not modify the code to take into consideration these new transforms.
>  - The use of CCLayerTreeHostImpl::contentBounds changed when updating the max scroll position.  Now all scrolling is performed entirely in layout space.  The page scale should have no effect on the current scroll offset, or the maximum extents of the scroll.  (Is this an ok assumption?)
> 
> Remaining high-level issues:
>  - Scaling of flash elements is still broken, as previously noted.

Manual testing has found that flapper-flash content works correctly.

Windowed plugins will not work with this scaling approach as-is without a means  to explicitly sent the zoom/translation transform on each plugin window.

>  - I still disregard the boundsContainContentScale setting in CCLayerTreeHost to ensure that the tiles are re-painted at the correct resolution for the scale.

[Alexandre Elias]

Here is jam@'s initial patch for Aura NPAPI plugin support: https://codereview.chromium.org/10905122/

We chatted offline and he told me that the scrolling jank in this approach is not currently seen as a major concern, considering the rarity of NPAPI plugins.  I'm guessing bugginess with pinch zoom may not be either.

In the long run, the solution to both scrolling jank and pinch zoom is to attach root scroll/pageScaleFactor as metadata to ubercompositor frames.  Then they can be exported to RenderWidgetHost as frames arrive and used to adjust plugin size/scale.  So maybe we can wait until ubercompositor is shipping before solving the NPAPI problem.

[James Robinson]

We aren't supporting any impl-thread manipulations (scrolling, zooming, etc) with NPAPI plugins.  They'll update eventually, but for now we don't think it's worth effort getting them working smoothly.  NPAPI is the old and busted.  PPAPI is the new hotness and should work fine.

[Jeff Timanus]

Created attachment 163466 [details]
Working V1 prototype.

Patch that corrects the screen-space scrolling issues that were present in previous patches.  

The problem was that the page-scale was being applied to the scroll delta, but because all scrolling is now in layout space, this factor is not needed.  Note that I haven't tested this change extensively with --force-device-scale-factor, so there may be lurking issues.

Given the previous comments on NPAPI scaling, I think that this patch represents a reasonable state for an initial check-in, on which we can iterate/bug-fix.  I'm thinking something along the lines of #if USE(COMPOSITOR_PINCH_ZOOM) to selectively enable the patch.  I will need some guidance on how to best test this behaviour.

I will start a separate patch to see if I can remove the use of LayerChromium::boundsContainPageScale(...).  If we can submit this patch with a conditional #define, then removing boundsContainPageScale may not be a strict prerequisite.

Question:  Do we need to preserve the CSS-transform based pinch implementation, even after this patch has fully landed and is enabled?

[Dana Jansens]

(In reply to comment #25)
> Created an attachment (id=163466) [details]
> Working V1 prototype.
> 
> Patch that corrects the screen-space scrolling issues that were present in previous patches.  
> 
> The problem was that the page-scale was being applied to the scroll delta, but because all scrolling is now in layout space, this factor is not needed.  Note that I haven't tested this change extensively with --force-device-scale-factor, so there may be lurking issues.

Nice job on all the fixes!

> Given the previous comments on NPAPI scaling, I think that this patch represents a reasonable state for an initial check-in, on which we can iterate/bug-fix.  I'm thinking something along the lines of #if USE(COMPOSITOR_PINCH_ZOOM) to selectively enable the patch.  I will need some guidance on how to best test this behaviour.

There are page-scale layout tests already, but they test the non-compositor-based path currently. We should do something for page-scale similar to what is being done for device-scale: https://bugs.webkit.org/show_bug.cgi?id=90192

I think adding some layout tests that cover both features together would be admirable.

> I will start a separate patch to see if I can remove the use of LayerChromium::boundsContainPageScale(...).  If we can submit this patch with a conditional #define, then removing boundsContainPageScale may not be a strict prerequisite.

Instead of an ifdef, how about we add a WebSettings value "appliesPageScaleFactorInCompositor" similar to device scale factor? Aura/mac/win8 can set this to true. Android can set this to false until they don't need it anymore. Can the changes to scrolling be done separately as a pre-req to this patch so that they can be reviewed in their own right?

When WebSettings::appliesPageScaleFactorInCompositor is true, then boundsContainPageScale() is false for the NCCH layer.

> Question:  Do we need to preserve the CSS-transform based pinch implementation, even after this patch has fully landed and is enabled?

We should keep it until android is done with it so we don't diverge from their tree.

[Dana Jansens]

Comment on attachment 163466 [details]
Working V1 prototype.

View in context: https://bugs.webkit.org/attachment.cgi?id=163466&action=review

> WebKit/chromium/src/WebViewImpl.cpp:212
> -const float WebView::minPageScaleFactor = 0.25;
> +const float WebView::minPageScaleFactor = 1.0;

Hardcoding this will break layout tests. It should not be changed here, chromium can set its limits appropriately.

> WebKit/chromium/src/WebViewImpl.cpp:3917
> +        WebPoint scrollPoint(scrollOffset.width, scrollOffset.height);
> +        setPageScaleFactor(pageScaleFactor() * pageScaleDelta, scrollPoint);

This should only be done if applyPageScaleInCompositor is true. @aelias this change would break android for now, wouldn't it?

> WebCore/page/Frame.cpp:956
> +    return 1;
> +#if 0

If the WebSettings::appliesPageScaleInCompositor is moved into WebCore::Settings, then we could use it here to decide to return 1.

> WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:872
> +    IntSize scaledContentSize = contentSize();
> +    scaledContentSize.scale(1 / m_pageScale);

This can give you off-by-ones! When converting between bounds-contentBounds, you must use the ratio between the two, both for width and height independently. Why not use a function that returns bounds() instead of contentBounds()?

> WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:874
> +    IntSize maxScroll = scaledContentSize - expandedIntSize(viewBounds);

The viewBounds is currently scaled by device scale factor. So you can't compare it to this logical pixels value 1:1. Why do you want this function to not work in content space anymore?

> WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:1092
> +    // Compute the appliation of the delta with respect to the current page zoom of the page.
> +    move.scale(1 / m_pageScale);

Please don't work in a coordinate space that includes device scale but not page scale. Use them both together (physical pixels) or use neither (logical pixels). I know this code is currently a bit inconsistent, but it is attempting to work in content space. If you want to include the device scale in the working coordinate space, then I'd prefer that you also include the page scale.

> WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:485
> -    if (layer->boundsContainPageScale())
> +    if (false /*layer->boundsContainPageScale()*/)

This means boundsContainPageScale() is wrong on the NCCH layer. It should be false if we are applying page scale in the compositor, since its bounds will not contain the page scale anymore.

[Alexandre Elias]

> > WebKit/chromium/src/WebViewImpl.cpp:3917
> > +        WebPoint scrollPoint(scrollOffset.width, scrollOffset.height);
> > +        setPageScaleFactor(pageScaleFactor() * pageScaleDelta, scrollPoint);
> 
> This should only be done if applyPageScaleInCompositor is true. @aelias this change would break android for now, wouldn't it?

Yes, it would.  Please make the behavior change conditional on applyXYZScaleFactorInCompositor or your new ENABLE flag.

Thanks for keeping Android in mind.  Yes, we need to keep the old path around for now.  When Android gets around to transitioning to the new one, I'll delete the old behavior.

[Alexandre Elias]

Comment on attachment 163466 [details]
Working V1 prototype.

View in context: https://bugs.webkit.org/attachment.cgi?id=163466&action=review

Very promising patch, a few minor comments.

> WebCore/platform/graphics/chromium/cc/CCLayerImpl.h:205
> +    float pageScale() const { return m_pageScale; }

Could we call it pageScaleFactor everywhere?  It was my inconsistency in the first place, but we should take the opportunity to fix it.

> WebCore/platform/graphics/chromium/cc/CCLayerImpl.h:365
> +    float m_pageScale;

Now that we are up to three of these values we should probably collapse them into a precomputed WebTransformationMatrix m_implTransform.  (By "impl" I mean that it's set directly on the impl thread and not synchronized from the main-thread layer.)  It's getting to be too much specialized stuff just for the root scroll layer.  And "m_localOffset" is an unclear name I'd like to get rid of.

> WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.h:86
> +private:

Could we move CCLayerTreeHostImpl::m_pageScaleDelta into this class as well?

> WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.h:87
> +    float m_scaleFactor;

Rename these to m_pageScaleFactor and m_pinchViewportScrollDelta.

[Jeff Timanus]

Created attachment 163676 [details]
Rename pageScale, and easy comments addressed.

Patch for sharing with other team members.  Please do not review.

[James Robinson]

If you don't want something reviewed all you have to do is not set "review?".  If you're using webkit-patch, pass the --no-review flag.

[Jeff Timanus]

Created attachment 163735 [details]
Addition of Settings flag to disable page-scale pinch zoom.

I updated this change to reflect GTFO.

The corresponding CC changes are now tracked in the following issue: https://codereview.chromium.org/10916279

I added a new setting to the various Settings classes, applyPageScaleFactorInCompositor, and selectively disabled the behaviour that I introduced based on its setting.

This flag will be controlled by a chromium command line argument, added in the corresponding chromium change.

[Dana Jansens]

Comment on attachment 163735 [details]
Addition of Settings flag to disable page-scale pinch zoom.

View in context: https://bugs.webkit.org/attachment.cgi?id=163735&action=review

> WebKit/chromium/src/WebViewImpl.cpp:3802
> +            if (m_webSettings->applyPageScaleFactorInCompositor())
> +                m_minimumPageScaleFactor = 1;

This will still make it hard to run the layout tests with the new page scale method since they test < 1. The right place for this is around here, based on pinch being enabled: http://code.google.com/searchframe#OAMlx_jo-ck/src/content/renderer/render_view_impl.cc&l=2962

> WebCore/page/Frame.cpp:957
> +    if (page->settings()->applyPageScaleFactorInCompositor())
> +        return 1;

There's a null check for page below but it is being dereffed here, so the ordering is off. It should be null checked before this deref.

[Jeff Timanus]

Created attachment 163877 [details]
Address review comments from danakj.

[WebKit Review Bot]

Please wait for approval from [e-mail], [e-mail], [e-mail], [e-mail] or [e-mail] before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

[Dana Jansens]

Comment on attachment 163877 [details]
Address review comments from danakj.

Thanks twiz, the only piece missing from my pov is that NCCH is still setting boundsContainPageScale() when they do not (ie when applyPageScaleInCompositor is true).

[Adam Barth]

Assuming folks like the rest of this change, the API change LGTM.

[Jeff Timanus]

Created attachment 163914 [details]
Correct NCCH to respect applyPageScaleFactorInCompositor setting.

[Jeff Timanus]

Created attachment 163916 [details]
Set of failing layout tests.

Attaching a set of layout test failures wrt the changes in NCCH.

If this patch is applied, and enabled, then the following compositing tests will fail.  (Note that this is without the corresponding change in the chromium side.)

Unexpected flakiness: image-only failures (2)
  compositing/geometry/fixed-position-iframe-composited-page-scale.html = IMAGE+TEXT IMAGE PASS
  compositing/geometry/fixed-position-transform-composited-page-scale.html = IMAGE+TEXT IMAGE PASS


Regressions: Unexpected text failures : (1)
  compositing/geometry/object-clip-rects-assertion.html = TEXT

Regressions: Unexpected image-only failures : (2)
  compositing/geometry/fixed-position-composited-page-scale.html = IMAGE
  compositing/geometry/fixed-position-iframe-composited-page-scale-down.html = IMAGE

Since the flag is disabled by default, these tests are not an immediate issue, and will be resolved when the chromium compositor change lands.

[James Robinson]

You shouldn't cause a test to start being flaky - that's pretty much always the sign of a bug.  If the other tests are definitely going to fail then mark them in TestExpectations

[Jeff Timanus]

(In reply to comment #40)
> You shouldn't cause a test to start being flaky - that's pretty much always the sign of a bug.  If the other tests are definitely going to fail then mark them in TestExpectations

This change is a no-op.  The failures I mentioned above were to indicate the failures that would occur, if applyPageScaleFactorInCompositor were enabled.

No new tests fail when the change is applied with the flag disabled.

[Jeff Timanus]

Created attachment 165142 [details]
Rebaseline to 129227.

[Dana Jansens]

Comment on attachment 165142 [details]
Rebaseline to 129227.

View in context: https://bugs.webkit.org/attachment.cgi?id=165142&action=review

Should we land this first so we have the ability to turn the feature on, regardless of the compositor page scaling being landable yet?

> Source/WebKit/chromium/ChangeLog:1118
> +>>>>>>> .r128636

merge error

[Jeff Timanus]

Created attachment 165152 [details]
Correct merge error.

[Jeff Timanus]

(In reply to comment #43)
> (From update of attachment 165142 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=165142&action=review
> 
> Should we land this first so we have the ability to turn the feature on, regardless of the compositor page scaling being landable yet?

Yes, I think we should land this change.  However, it may be worth double checking that the use of the flag in this change is fully compatible with the corresponding cr change.

On the CR side change the major missing components are unittests for this new path.

> 
> > Source/WebKit/chromium/ChangeLog:1118
> > +>>>>>>> .r128636
> 
> merge error

[Jeff Timanus]

Created attachment 165441 [details]
Rebaseline to 129404

[Jeff Timanus]

Created attachment 165699 [details]
Stop applying page-scale factor to scroll offsets in WebViewImpl.

[Jeff Timanus]

Created attachment 165878 [details]
Add WebCompositor::setPageScalePinchZoomEnabled(...) settings exports.

[Jeff Timanus]

(In reply to comment #48)
> Created an attachment (id=165878) [details]
> Add WebCompositor::setPageScalePinchZoomEnabled(...) settings exports.

These exports are required to pass the pinch-zoom-in-compositor flag to the instance of the CC in the chromium repository.

See comments on the corresponding cr issue for full details:  https://codereview.chromium.org/10916279/#msg12

[Jeff Timanus]

Created attachment 166532 [details]
Rebaseline to 130047.

[Jeff Timanus]

Comment on attachment 165878 [details]
Add WebCompositor::setPageScalePinchZoomEnabled(...) settings exports.

I think this version of the change is ready for landing.  Can a reviewer please take a look?

There should not be any more changes on the Chromium side of this work that will require modifications here.

[Alexandre Elias]

Comment on attachment 166532 [details]
Rebaseline to 130047.

LGTM.  James, can you review?

[James Robinson]

Comment on attachment 166532 [details]
Rebaseline to 130047.

R=me

[WebKit Review Bot]

Comment on attachment 166532 [details]
Rebaseline to 130047.

Clearing flags on attachment: 166532

Committed r130321: <http://trac.webkit.org/changeset/130321>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Eric Seidel (no email)]

Comment on attachment 165878 [details]
Add WebCompositor::setPageScalePinchZoomEnabled(...) settings exports.

Cleared review? from obsolete attachment 165878 [details] so that this bug does not appear in http://webkit.org/pending-review.  If you would like this patch reviewed, please attach it to a new bug (or re-open this bug before marking it for review again).