Bug 68535 - [chromium] Revise zoom animator backend to use full transform instead of just scale.
Summary: [chromium] Revise zoom animator backend to use full transform instead of just...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: W. James MacLean
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-09-21 07:48 PDT by W. James MacLean
Modified: 2011-09-26 13:30 PDT (History)
6 users (show)

See Also:


Attachments
Patch (19.07 KB, patch)
2011-09-21 08:02 PDT, W. James MacLean
no flags Details | Formatted Diff | Diff
Patch (20.19 KB, patch)
2011-09-21 09:03 PDT, W. James MacLean
no flags Details | Formatted Diff | Diff
Patch (21.16 KB, patch)
2011-09-21 10:04 PDT, W. James MacLean
no flags Details | Formatted Diff | Diff
Patch (20.01 KB, patch)
2011-09-22 06:51 PDT, W. James MacLean
no flags Details | Formatted Diff | Diff
Patch (19.80 KB, patch)
2011-09-24 10:24 PDT, W. James MacLean
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description W. James MacLean 2011-09-21 07:48:37 PDT
[chromium] Revise zoom animator backend to use full transform instead of just scale.
Comment 1 W. James MacLean 2011-09-21 08:02:09 PDT
Created attachment 108160 [details]
Patch
Comment 2 W. James MacLean 2011-09-21 08:07:28 PDT
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 ...
Comment 3 Robert Kroeger 2011-09-21 08:28:31 PDT
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.
Comment 4 W. James MacLean 2011-09-21 08:34:56 PDT
(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?
Comment 5 W. James MacLean 2011-09-21 08:43:35 PDT
(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.
Comment 6 W. James MacLean 2011-09-21 08:49:57 PDT
> 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 ...
Comment 7 W. James MacLean 2011-09-21 09:03:00 PDT
Created attachment 108168 [details]
Patch
Comment 8 W. James MacLean 2011-09-21 09:08:07 PDT
(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 ...
Comment 9 W. James MacLean 2011-09-21 10:04:15 PDT
Created attachment 108173 [details]
Patch
Comment 10 W. James MacLean 2011-09-21 10:04:57 PDT
(In reply to comment #9)
> Created an attachment (id=108173) [details]
> Patch

Added missing chromium-gpu-linux test image expected output.
Comment 11 James Robinson 2011-09-21 11:41:26 PDT
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.
Comment 12 W. James MacLean 2011-09-21 12:00:14 PDT
(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.
Comment 13 W. James MacLean 2011-09-21 13:28:22 PDT
(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.)
Comment 14 W. James MacLean 2011-09-22 06:51:52 PDT
Created attachment 108327 [details]
Patch
Comment 15 W. James MacLean 2011-09-22 06:55:15 PDT
(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).
Comment 16 W. James MacLean 2011-09-24 10:24:15 PDT
Created attachment 108584 [details]
Patch
Comment 17 W. James MacLean 2011-09-24 10:26:09 PDT
(In reply to comment #16)
> Created an attachment (id=108584) [details]
> Patch

Remove unused function from CCLayerTreeHost.
Comment 18 Kenneth Russell 2011-09-26 10:42:38 PDT
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.
Comment 19 Kenneth Russell 2011-09-26 10:43:11 PDT
Looks fine overall. r=me
Comment 20 WebKit Review Bot 2011-09-26 13:30:20 PDT
Comment on attachment 108584 [details]
Patch

Clearing flags on attachment: 108584

Committed r95988: <http://trac.webkit.org/changeset/95988>
Comment 21 WebKit Review Bot 2011-09-26 13:30:25 PDT
All reviewed patches have been landed.  Closing bug.