Bug 68535 – [chromium] Revise zoom animator backend to use full transform instead of just scale.

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[W. James MacLean]

[chromium] Revise zoom animator backend to use full transform instead of just scale.

[W. James MacLean]

Created attachment 108160 [details]
Patch

[W. James MacLean]

A couple of comments questions:

1) LINEAR interpolation is turned on for GESTURE_EVENTS to prevent text from looking overly pixelated while zooming.

2) The pixel test changes slightly with LINEAR vs NEAREST ... will this be a problem? Currently features.gypi has 'ENABLE_GESTURE_EVENTS=1' but does anyone test with it off?

3) I assume I can do mac/mac-cg baselines once the patch is approved? Perhaps take new baselines off the bots ...

[Robert Kroeger]

Comment on attachment 108160 [details]
Patch

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

nits I think.

> Source/WebCore/page/Settings.h:611
> +        double m_zoomAnimatorPosX;

shouldn't these get a default value somewhere? Just to be safe.

> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:295
> +        // TODO(wjmaclean): Render some interesting texture in unrendered regions.

FIXME: in WebKit

> Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:202
> +            const GC3Dint filter = GraphicsContext3D::LINEAR;

Do we want to be smarter about this? I am not entirely sure of the implications.

[W. James MacLean]

(In reply to comment #3)
> (From update of attachment 108160 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=108160&action=review
> 
> nits I think.
> 
> > Source/WebCore/page/Settings.h:611
> > +        double m_zoomAnimatorPosX;

Good point. I've added initialisers that will show up when I load the next version of the patch.

> shouldn't these get a default value somewhere? Just to be safe.
> 
> > Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:295
> > +        // TODO(wjmaclean): Render some interesting texture in unrendered regions.
> 
> FIXME: in WebKit

Fixed.

> > Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:202
> > +            const GC3Dint filter = GraphicsContext3D::LINEAR;
> 
> Do we want to be smarter about this? I am not entirely sure of the implications.

I was wondering this too ... I'm not sure if the interpolation settings are allowed to change from one display of the texture to the next. If it is, this perhaps could be added to the TiledLayerChromium interface?

[W. James MacLean]

(In reply to comment #4)
> > > Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:202
> > > +            const GC3Dint filter = GraphicsContext3D::LINEAR;
> > 
> > Do we want to be smarter about this? I am not entirely sure of the implications.
> 
> I was wondering this too ... I'm not sure if the interpolation settings are allowed to change from one display of the texture to the next. If it is, this perhaps could be added to the TiledLayerChromium interface?

To expand on this a bit more:

- it causes very subtle changes to about 14 existing gpu layout tests (rebaselining should suffice)
- on most hardware I think the performance impact should be low or even negligible (although I may be wrong) ... and I don't know how big the fraction "1 - most" is.

Certainly turning it on/off as needed would be nice, if possible. Not sure yet how hard this would be to plumb.

[W. James MacLean]

> Certainly turning it on/off as needed would be nice, if possible. Not sure yet how hard this would be to plumb.

OK, TiledLayerChromium could ask it's LayerTreeHost if the current zoom transform isIdentity() ... will experiment with this ...

[W. James MacLean]

Created attachment 108168 [details]
Patch

[W. James MacLean]

(In reply to comment #7)
> Created an attachment (id=108168) [details]
> Patch

This patch plumbs from TiledLayerChromium to LayerTreeHost. The latter stores a boolean to indicate if the zoom transform is an identity, since presumably we test far more often than this value changes and the TransformationMatrix::isIdentity() function involves a lot of comparisons.

This *seems* (prima facie) to work OK on my EGL stack, but not sure if all GL implementions will like it. It reduces number of broken layout tests, but some other *seemingly unrelated* stuff is present ... need to investigate some more ...

[W. James MacLean]

Created attachment 108173 [details]
Patch

[W. James MacLean]

(In reply to comment #9)
> Created an attachment (id=108173) [details]
> Patch

Added missing chromium-gpu-linux test image expected output.

[James Robinson]

Comment on attachment 108173 [details]
Patch

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

> Source/WebCore/page/Settings.h:612
> +        double m_zoomAnimatorPosX;
> +        double m_zoomAnimatorPosY;

do you really need double precision? could this be an IntPoint (or FloatPoint if you need floating point values)

it's also unfortunate that this is growing into a bag of properties. Is there any reasonable way to consolidate these?

> Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:201
> +            const GC3Dint filter = (m_tiler->hasBorderTexels() || !layerTreeHost()->isZoomTransformIdentity()) ? GraphicsContext3D::LINEAR : GraphicsContext3D::NEAREST;

This seems very wrong.  If we are going to filter and apply a scale to tiled content then we have to have border texels or we'll generate rendering artifacts. It looks like this logic is attempting to apply filtering without uploading border texels which will not work.

It seems like in order for this to work you need to decide ahead of time whether you want to apply this animation and provide border texels for the root, or perhaps just always apply border texels on the root layer tiles and change the other logic that keys off of "do I have border texels?" as a proxy for "am I the root layer?" to be more explicit.

> Source/WebKit/chromium/src/WebViewImpl.cpp:2580
> +        zoomMatrix.translate3d(m_page->settings()->zoomAnimatorPosX(), m_page->settings()->zoomAnimatorPosY(), 0);

if you are doing a 2d translation then use translate(), don't use translate3d(..., ..., 0). the latter is less clear a the callsight and will do extra unnecessary math

> Source/WebKit/chromium/src/WebViewImpl.cpp:2581
> +        zoomMatrix.scale3d(m_page->settings()->zoomAnimatorScale(), m_page->settings()->zoomAnimatorScale(), 1);

you want .scale() here.

[W. James MacLean]

(In reply to comment #11)
> (From update of attachment 108173 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=108173&action=review
> 
> > Source/WebCore/page/Settings.h:612
> > +        double m_zoomAnimatorPosX;
> > +        double m_zoomAnimatorPosY;
> 
> do you really need double precision? could this be an IntPoint (or FloatPoint if you need floating point values)

We could use float here, although I figured since we were eventually targetting a TransformationMatrix double might make sense. I don't think IntPoint would make sense, and may cause the animation to be less smooth.

> it's also unfortunate that this is growing into a bag of properties. Is there any reasonable way to consolidate these?

Ultimately once the zoom front end code is in place I'd like to bypass Settings altogether if there's a better way to pass the values from FrameView down to WebViewImpl.

I've avoided putting more complex types into Settings as I think this will be a temporary pathway, although we could just make it a TransformationMatrix if it stays.

Let me know which route is preferred.

> > Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:201
> > +            const GC3Dint filter = (m_tiler->hasBorderTexels() || !layerTreeHost()->isZoomTransformIdentity()) ? GraphicsContext3D::LINEAR : GraphicsContext3D::NEAREST;
> 
> This seems very wrong.  If we are going to filter and apply a scale to tiled content then we have to have border texels or we'll generate rendering artifacts. It looks like this logic is attempting to apply filtering without uploading border texels which will not work.
> 
> It seems like in order for this to work you need to decide ahead of time whether you want to apply this animation and provide border texels for the root, or perhaps just always apply border texels on the root layer tiles and change the other logic that keys off of "do I have border texels?" as a proxy for "am I the root layer?" to be more explicit.

I guess we could always apply border texels to the root layer. Let me read through to see where to turn that on.

> > Source/WebKit/chromium/src/WebViewImpl.cpp:2580
> > +        zoomMatrix.translate3d(m_page->settings()->zoomAnimatorPosX(), m_page->settings()->zoomAnimatorPosY(), 0);
> 
> if you are doing a 2d translation then use translate(), don't use translate3d(..., ..., 0). the latter is less clear a the callsight and will do extra unnecessary math

Fixed.

> > Source/WebKit/chromium/src/WebViewImpl.cpp:2581
> > +        zoomMatrix.scale3d(m_page->settings()->zoomAnimatorScale(), m_page->settings()->zoomAnimatorScale(), 1);
> 
> you want .scale() here.

Fixed.

[W. James MacLean]

(In reply to comment #11)
> 
> It seems like in order for this to work you need to decide ahead of time whether you want to apply this animation and provide border texels for the root, or perhaps just always apply border texels on the root layer tiles and change the other logic that keys off of "do I have border texels?" as a proxy for "am I the root layer?" to be more explicit.

Two questions:

Do all non-root layers at present get border texels?

Who needs to know the difference between a root layer and a non-root layer? (I've looked at the code, but it's not very clear when we need to be able to distinguish if a layer is the root layer.)

[W. James MacLean]

Created attachment 108327 [details]
Patch

[W. James MacLean]

(In reply to comment #14)
> Created an attachment (id=108327) [details]
> Patch

I have removed the border texels & linear interpolation stuff and will submit it in another patch.

I have changed the parameter types in Settings from double to float, but not used FloatPoint as (1) this is only temporary, and (2) I don't know if we want to add FloatPoint.h just for this case (I will change this if it is deemed desirable).

[W. James MacLean]

Created attachment 108584 [details]
Patch

[W. James MacLean]

(In reply to comment #16)
> Created an attachment (id=108584) [details]
> Patch

Remove unused function from CCLayerTreeHost.

[Kenneth Russell]

Comment on attachment 108584 [details]
Patch

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

> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:296
> +        m_context->clearColor(0.25, 0.25, 0.25, 1);

It's okay to use braces here because the arms (including comments) aren't single-line.

[Kenneth Russell]

Looks fine overall. r=me

[WebKit Review Bot]

Comment on attachment 108584 [details]
Patch

Clearing flags on attachment: 108584

Committed r95988: <http://trac.webkit.org/changeset/95988>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.