cc: Apply the layer's initial CSS scale to the contentsScale to render text at the right resolution.

cc: Apply the layer's initial CSS scale to the contentsScale to render text at the right resolution.

Currently the transform does not affect the scale of a layer's contents. This
means that when a layer with a scale on it becomes composited, its text is
rendered at 1x and then scaled up during rendering, which makes for very fuzzy
text.

This change applies the scale from the layer's CSS transform to the
contentsScale so that text is rendered at the same pixel resolution
as it will be drawn on screen.

Tests:
cc_unittests:CCLayerTreeHostCommonTest.verifyContentsScale
cc_unittests:CCLayerTreeHostCommonTest.verifyContentsScaleForSurfaces

BUG=149943
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=161472

[danakj, 2012-09-17 21:46:36]

Implemented the contentsScale setting into CCLTHCommon so that we can use the
combinedTransform.

[piman, 2012-09-17 23:53:37]

https://codereview.chromium.org/10915313/diff/11001/cc/CCMathUtil.cpp
File cc/CCMathUtil.cpp (right):

https://codereview.chromium.org/10915313/diff/11001/cc/CCMathUtil.cpp#newcode382
cc/CCMathUtil.cpp:382: return sqrtf(fabs(p1.x() - p2.x()) * fabs(p1.x() -
p2.x()) + fabs(p1.y() - p2.y()) * fabs(p1.y() - p2.y()));
nit: no needs for fabs here. x² == |x|²

[danakj, 2012-09-17 23:55:05]

https://codereview.chromium.org/10915313/diff/11001/cc/CCMathUtil.cpp
File cc/CCMathUtil.cpp (right):

https://codereview.chromium.org/10915313/diff/11001/cc/CCMathUtil.cpp#newcode382
cc/CCMathUtil.cpp:382: return sqrtf(fabs(p1.x() - p2.x()) * fabs(p1.x() -
p2.x()) + fabs(p1.y() - p2.y()) * fabs(p1.y() - p2.y()));
On 2012/09/17 23:53:37, piman wrote:
> nit: no needs for fabs here. x² == |x|²

o right :) thanks.

[jamesr, 2012-09-17 23:57:13]

I don't think we can afford to unconditionally raster at post-scale size -
there's a lot of content that relies on animating scale (either by CSS animation
or JS) on composited layers being no more expensive than scaling an
already-rastered-and-uploaded texture.

[danakj, 2012-09-22 15:44:49]

Here's a patch that will not regress RAF based animations, but will take care of
statically CSS-scaled content.

We store the initial CSS scale of the layer (this excludes device and page scale
factors) on LayerChromium. Then we are able to use this initial CSS scale,
combined with the current device and page scale factors, to compute the layer's
contentsScale

[enne (OOO), 2012-10-02 19:13:18]

I like this approach a lot more.  It solves the problem of compositing overlap
making contents look different.  It also prevents excessive repainting churn due
to animations.

The only thing I worry about is that upscaling something with an animation that
starts off with a small scale is now going to look worse, since we'll start it
at the small scaled size rather the pre-scaled original size of the contents. 
On the other hand, that'll use less memory.

jamesr, do you have any thoughts about this approach?

http://codereview.chromium.org/10915313/diff/20001/cc/CCLayerTreeHostCommon.cpp
File cc/CCLayerTreeHostCommon.cpp (right):

http://codereview.chromium.org/10915313/diff/20001/cc/CCLayerTreeHostCommon.c...
cc/CCLayerTreeHostCommon.cpp:343: WebTransformationMatrix flatTransform =
combinedTransform;
Do you want the combinedTransform here or the screen space transform?

http://codereview.chromium.org/10915313/diff/20001/cc/CCLayerTreeHostCommon.c...
cc/CCLayerTreeHostCommon.cpp:392: LayerSorter* layerSorter, int maxTextureSize,
float deviceScaleFactor, float pageScaleFactor, IntRect&
drawableContentRectOfSubtree)
How about an input struct to this function?

[jamesr, 2012-10-02 19:22:05]

I'm not sure I understand the interaction with pageScale here - how does that
work?  Is the pageScale baked in to the target raster scale?

[danakj, 2012-10-02 19:28:04]

On 2012/10/02 19:22:05, jamesr wrote:
> I'm not sure I understand the interaction with pageScale here - how does that
> work?  Is the pageScale baked in to the target raster scale?

Currently pagescale is baked into the CSS transform for layers other than the
NCCH, so we have to pull it out to get the layer's "pure" transform.

The NCCH has boundsContainPageScale() so we know about this, and we don't need
to pull out the pagescale from the transform there.

[danakj, 2012-10-02 19:52:50]

On 2012/10/02 19:13:18, enne wrote:
> I like this approach a lot more.  It solves the problem of compositing overlap
> making contents look different.  It also prevents excessive repainting churn
due
> to animations.
> 
> The only thing I worry about is that upscaling something with an animation
that
> starts off with a small scale is now going to look worse, since we'll start it
> at the small scaled size rather the pre-scaled original size of the contents. 
> On the other hand, that'll use less memory.

There is a workaround for devs, if they make the layer at their desired scale
off-screen, it'll keep that scale for rastering in the future. This trick also
could be used for something that appears at scale 1 but animates and stays at
some larger scale.

> jamesr, do you have any thoughts about this approach?
> 
>
http://codereview.chromium.org/10915313/diff/20001/cc/CCLayerTreeHostCommon.cpp
> File cc/CCLayerTreeHostCommon.cpp (right):
> 
>
http://codereview.chromium.org/10915313/diff/20001/cc/CCLayerTreeHostCommon.c...
> cc/CCLayerTreeHostCommon.cpp:343: WebTransformationMatrix flatTransform =
> combinedTransform;
> Do you want the combinedTransform here or the screen space transform?

Oooooh surfaces!

I want combinedTransform I think. This is the right scale to get pixel-perfect
results in the renderTarget's surface, right?

Say we have a layer owning a surface with scale=2. This doesn't change the
number of pixels in the layer's owned surface currently, so it does not change
the combinedTransform of its subtree.

If a child uses the screenspace transform instead, it would raster at the right
size to appear on the screen, but it would get down-sampled anyways into the
renderTarget's surface.

To make children of scaled surfaces work, we need to scale the surface size and
its sublayerMatrix accordingly!

>
http://codereview.chromium.org/10915313/diff/20001/cc/CCLayerTreeHostCommon.c...
> cc/CCLayerTreeHostCommon.cpp:392: LayerSorter* layerSorter, int
maxTextureSize,
> float deviceScaleFactor, float pageScaleFactor, IntRect&
> drawableContentRectOfSubtree)
> How about an input struct to this function?

Nom nom yaks. I don't mind doing this.

[jamesr, 2012-10-03 01:04:53]

Approach seems fine.

[danakj, 2012-10-03 03:04:40]

Ok this patch works with surfaces properly now! I tested with a layout test and
made all the unit tests pass. I need to add one more unit test still for scale
on surfaces I think, just to be explicit, though it is already being tested
quite nicely by the high dpi unit tests.

The approach is:
- The surface's "content" space (ie the space its drawTransform comes from) ==
the owning layer's content space. So we don't have to scale the drawTransform of
the owning layer at all anymore.
- Inside the subtree of a surface, we scale everything by the owning layer's
contentsScale so that everything ends up in that pixel space together.

Basically, we used to do this special case with device scale, but now we use the
contentsScale instead which includes the deviceScale.

After this change most draw transforms don't scale at all anymore since they
paint the number of pixels that will appear on the screen.

[danakj, 2012-10-03 03:05:39]

Oh, I still need to yakshave the struct business.

[danakj, 2012-10-03 14:12:44]

This should also fix the current problem of page scale not playing nice with
render surfaces. ie https://bugs.webkit.org/show_bug.cgi?id=98201

[Jeff Timanus, 2012-10-03 16:14:41]

On 2012/10/03 14:12:44, danakj wrote:
> This should also fix the current problem of page scale not playing nice with
> render surfaces. ie https://bugs.webkit.org/show_bug.cgi?id=98201

I just pulled in this change, and yes, it does fix the problem exposed by the
new test added in the above bug.

[danakj, 2012-10-03 17:46:29]

added test CCLayerTreeHostCommonTest.verifyContentsScaleForSurfaces

PTAL :)

[enne (OOO), 2012-10-03 19:52:38]

https://codereview.chromium.org/10915313/diff/34001/cc/CCLayerTreeHostCommon.cpp
File cc/CCLayerTreeHostCommon.cpp (left):

https://codereview.chromium.org/10915313/diff/34001/cc/CCLayerTreeHostCommon....
cc/CCLayerTreeHostCommon.cpp:536: layerDrawTransform.scale(deviceScaleFactor);
This block of code is highly intentional.

The "contentsScale" of the render surface should not always be equal to the
contentsScale of the owning layer.  What if the owning layer is a video and
doesn't needContentsScale(), but DSF != 1? What if the owning layer is an image
layer with gigantic contentBounds() but tiny bounds()?

This line right here is basically saying "If this surface were drawn with an
identity transform, the pixels in this surface are 1:1 with device pixels."

My intention was that in the future we could adjust this so that we could scale
the content size of the surface based on the amount of pixels that it actually
takes up to dynamically make better memory/quality trade-offs.

Can you add a unit test where the contentBounds / bounds ratio of a layer is
*not* the scale that we want to create the surface at?

[danakj, 2012-10-04 00:03:35]

On 2012/10/03 19:52:38, enne wrote:
>
https://codereview.chromium.org/10915313/diff/34001/cc/CCLayerTreeHostCommon.cpp
> File cc/CCLayerTreeHostCommon.cpp (left):
> 
>
https://codereview.chromium.org/10915313/diff/34001/cc/CCLayerTreeHostCommon....
> cc/CCLayerTreeHostCommon.cpp:536: layerDrawTransform.scale(deviceScaleFactor);
> This block of code is highly intentional.
> 
> The "contentsScale" of the render surface should not always be equal to the
> contentsScale of the owning layer.  What if the owning layer is a video and
> doesn't needContentsScale(), but DSF != 1? What if the owning layer is an
image
> layer with gigantic contentBounds() but tiny bounds()?

Your absolutely right, I don't want the contentBounds involved here. I've
switched it to be based on the current combinedTransform so that you get 1:1
with the pixels in any render surface and the final output on the screen.

> This line right here is basically saying "If this surface were drawn with an
> identity transform, the pixels in this surface are 1:1 with device pixels."
> 
> My intention was that in the future we could adjust this so that we could
scale
> the content size of the surface based on the amount of pixels that it actually
> takes up to dynamically make better memory/quality trade-offs.

We can choose to change this very simply in the CCLTHCommon by adjusting the
renderSurfaceSublayerScale, so yay.

> Can you add a unit test where the contentBounds / bounds ratio of a layer is
> *not* the scale that we want to create the surface at?

There was one but it was expecting the wrong things. See the
CCLayerTreeHostCommonTest.verifyContentsScaleForSurfaces test for a surface
which does not have scaled contentBounds.

[enne (OOO), 2012-10-05 19:05:16]

LGTM.  The changes to CCLayerTreeHostCommon look really great.

[danakj, 2012-10-05 19:08:34]

ty :)

[commit-bot: I haz the power, 2012-10-05 19:09:18]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/danakj@chromium.org/10915313/37001

[commit-bot: I haz the power, 2012-10-05 19:59:48]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/danakj@chromium.org/10915313/43003

[commit-bot: I haz the power, 2012-10-05 20:44:12]

Sorry for I got bad news for ya.
Compile failed with a clobber build.
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2012-10-06 17:10:38]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/danakj@chromium.org/10915313/55001

[commit-bot: I haz the power, 2012-10-06 17:34:30]

Sorry for I got bad news for ya.
Compile failed with a clobber build.
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[danakj, 2012-10-06 19:02:55]

I'm glad I ran layout bots. There's some rebasing needed but this would have
broken platform/chromium/compositing/3d-corners.html somehow, and the CQ
wouldn't have noticed at the moment.

[enne (OOO), 2012-10-11 22:28:14]

Changes lgtm.

[commit-bot: I haz the power, 2012-10-11 22:29:35]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/danakj@chromium.org/10915313/70001

[commit-bot: I haz the power, 2012-10-11 22:54:55]

Sorry for I got bad news for ya.
Compile failed with a clobber build.
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2012-10-11 23:34:40]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/danakj@chromium.org/10915313/80003

[commit-bot: I haz the power, 2012-10-12 01:41:19]

Change committed as 161472