Bug 96940 – A layout test to verify scaled composited layers have crisp text

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

[Dana Jansens]

A layout test to verify scaled composited layers have crisp text

[Dana Jansens]

Created attachment 164437 [details]
Patch

[James Robinson]

So the expectation here is that we'd raster at the higher scale instead of applying the transform in the compositor?  What behavior would you expect if the scale and translate were applied as one big matrix?

[Dana Jansens]

The expectation is that the contents of the composited layer contain the CSS transform, in exactly the same way as happens for device scale factor.

If we had a scale and transform, I would expect the text to be crisp because the backing texture of the layer is scaled, and the compositor would apply the scale+transform during rendering to put crisp text on the screen at the appropriate place.

[Dana Jansens]

(In reply to comment #3)
> The expectation is that the contents of the composited layer contain the CSS transform, in exactly the same way as happens for device scale factor.
> 
> If we had a scale and transform, I would expect the text to be crisp because the backing texture of the layer is scaled, and the compositor would apply the scale+transform during rendering to put crisp text on the screen at the appropriate place.

s/transform/translate/ in that second paragraph.

[Dana Jansens]

Created attachment 164444 [details]
Patch

this version of the test uses a translate instead of left/top. we can use this instead if you like.

[James Robinson]

That's not the point.  The question here is fundamentally about what scale to raster with when there's a compositor-applied transform.  For deviceScale you're right that we should raster at the post-scaled value so we end up 1:1 to screen.  For other transforms, it's much less clear what the right thing is.  Rastering at the highest scale we see and then trying to downsample in the compositor does produce the highest quality, but uses a lot of time and memory.  It sounds like you want to do a peephole optimization for the case where the compositing trigger and scale are on the same element but not address the more general case of a scale on an ancestor.  That seems fine, but I'd like you to precisely define what the parameters for this will be.

The other case to think about is when a scale is being animated (which is really common) - what scale do you want to raster at?

[Dana Jansens]

(In reply to comment #6)
> That's not the point.  The question here is fundamentally about what scale to raster with when there's a compositor-applied transform.  For deviceScale you're right that we should raster at the post-scaled value so we end up 1:1 to screen.  For other transforms, it's much less clear what the right thing is.  Rastering at the highest scale we see and then trying to downsample in the compositor does produce the highest quality, but uses a lot of time and memory.  It sounds like you want to do a peephole optimization for the case where the compositing trigger and scale are on the same element but not address the more general case of a scale on an ancestor.  That seems fine, but I'd like you to precisely define what the parameters for this will be.

Oh! I kinda assumed transform() included the parent but right, it does not.

It seems to me that it's a bug if we draw things fuzzy because of a webkit-transform on some ancestor (ie composited layer vs non), and that we should match what the output would look like if it was rendered in the NCCH instead.

> The other case to think about is when a scale is being animated (which is really common) - what scale do you want to raster at?

Right, Nat's concerned about causing a lot of painting, so I am proposing that we don't change the layer's contentsScale until animation completes, so we can avoid invalidations during the animation.

[Adrienne Walker]

Is there any way to just make this a ref test? One has a composited layer with a scale, the other has a scale but goes into the root?

[Simon Fraser (smfr)]

This sounds like a test for bug 27684. Why add a test before the bug is fixed?

[Dana Jansens]

The fix for chromium's compositor is happening here: https://codereview.chromium.org/10915313

[Simon Fraser (smfr)]

Can't we fix it in cross-platform code?

[James Robinson]

That's tricky - the cross-platform code gets a GraphicsContext passed in from the platform-dependent code via GraphicsLayerClient.  This context has to be sized + scaled correctly by the compositor implementation.

The question then becomes is the compositor implementation or cross-platform code in a better place to decide the scale+size for a given layer.  My inclination is it's the mostly compositor implementation, since only it can make intelligent choices about resource use vs. time vs. quality tradeoffs.  Cross-platform code can't really have any idea of what the impact of rastering at a higher resolution will be on a given backend or if it'll have any effect as all.

That said part of the work could definitely be done in cross-platform code depending on how the exact solution works.  For instance, if we want to make any scaling decisions based on animation progress I think doing it in cross-platform code makes sense.

[Simon Fraser (smfr)]

(In reply to comment #12)
> That's tricky - the cross-platform code gets a GraphicsContext passed in from the platform-dependent code via GraphicsLayerClient.  This context has to be sized + scaled correctly by the compositor implementation.

This is what GraphicsLayer::setContentsScale() is for.

> The question then becomes is the compositor implementation or cross-platform code in a better place to decide the scale+size for a given layer.  My inclination is it's the mostly compositor implementation, since only it can make intelligent choices about resource use vs. time vs. quality tradeoffs. 

True, but I think the cross-platform code is the best place to compute the ideal scale.

[Dana Jansens]

Created attachment 170012 [details]
Patch

Two tests, one with and one without a surface

[Adrienne Walker]

The font rasterization in that expected image looks really strange to my eyes.  Is that really the correct result?

Also, consider using a ref test, to check that font rasterization looks the same whether or not you create a render surface or not, and whether or not you create a layer or not.

[WebKit Review Bot]

Comment on attachment 170012 [details]
Patch

Attachment 170012 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/14500297

New failing tests:
platform/chromium/virtual/softwarecompositing/text-on-scaled-layer.html
platform/chromium/virtual/softwarecompositing/text-on-scaled-surface.html

[Dana Jansens]

(In reply to comment #15)
> The font rasterization in that expected image looks really strange to my eyes.  Is that really the correct result?

Probably not, this is from running it on precise. It's not fuzzy, so it's right in that regard.

> Also, consider using a ref test, to check that font rasterization looks the same whether or not you create a render surface or not, and whether or not you create a layer or not.

I'll have to learn about ref tests at last it seems. :)

[Dana Jansens]

Created attachment 170035 [details]
Patch

Reftests instead of pixel results

[Dana Jansens]

Created attachment 170039 [details]
Patch

Add softwarecompositing test expectations

[Adrienne Walker]

I'm confused why these are expected-mismatch and not expected.  Isn't the goal here to assert that the text with and without surfaces/layers is identical?

[Dana Jansens]

Oh, I misread, thanks.

[Dana Jansens]

Created attachment 170272 [details]
Patch

Proper ref tests that pass on my local machine

[Build Bot]

Comment on attachment 170272 [details]
Patch

Attachment 170272 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/14569065

New failing tests:
media/video-zoom.html
compositing/text-on-scaled-layer.html

[Dana Jansens]

Created attachment 170418 [details]
Patch

Add test expectations for mac

[Adrienne Walker]

Comment on attachment 170418 [details]
Patch

R=me.

[Dana Jansens]

Created attachment 170426 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 170426 [details]
Patch for landing

Clearing flags on attachment: 170426

Committed r132376: <http://trac.webkit.org/changeset/132376>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.