Match html canvas which is transferred to OffscreenCanvas to CSS style

cc/layers/surface_layer_impl.cc

Index: cc/layers/surface_layer_impl.cc
diff --git a/cc/layers/surface_layer_impl.cc b/cc/layers/surface_layer_impl.cc
index 2289a29326f449746f68eb7ddff19fd3a0ba58bf..5e745648bc5e058137bf0a5f89524941f8df300c 100644
--- a/cc/layers/surface_layer_impl.cc
+++ b/cc/layers/surface_layer_impl.cc
@@ -68,7 +68,26 @@ void SurfaceLayerImpl::AppendQuads(RenderPass* render_pass,
 
   SharedQuadState* shared_quad_state =
       render_pass->CreateAndAppendSharedQuadState();
-  PopulateScaledSharedQuadState(shared_quad_state, surface_scale_);
+  gfx::Transform scaled_draw_transform =
+      draw_properties().target_space_transform;
+
+  float boundsWidth = bounds().width();

[danakj, 2016/11/21 23:37:22]

chromium style, though I'd drop these 4 temp vars they aren't adding any context
with their names and they are only used once?

[xlai (Olivia), 2016/11/22 16:55:59]

Done. I was trying to force them (integer) to float to prevent their fractional
part being discarded.

[danakj, 2016/11/22 19:19:13]

Ah, you can use a static cast for that, or use temps and a comment explaining.
You only need to make one of them (surface_size_ for instance) promoted to a
float for that to work.

+  float boundsHeight = bounds().height();
+  float surfaceSizeWidth = surface_size_.width();
+  float surfaceSizeHeight = surface_size_.height();
+  float scale_x = surfaceSizeWidth / boundsWidth;

[danakj, 2016/11/21 23:37:22]

why not invert these, and then you don't have to re-invert them below in the
call to Scale()?

Can you leave a comment explaining why this scale is being used? AIUI this is
the transform from the space inside the surface to the space of the surface
layer?

[xlai (Olivia), 2016/11/22 16:55:59]

Done.

+  float scale_y = surfaceSizeHeight / boundsHeight;
+  scaled_draw_transform.Scale(SK_MScalar1 / scale_x, SK_MScalar1 / scale_y);
+
+  gfx::Size scaled_bounds = gfx::ScaleToCeiledSize(bounds(), surface_scale_);

[danakj, 2016/11/21 23:37:22]

I'm very confused what the function of this surface_scale_ is at this point, and
having this not match the transform is very odd. Can you explain why it is not
matching?

The position/bounds of the layer say what space it should occupy in the embedder
of the SurfaceLayer. The transform creates a difference space where the content
is coming from, in this case based on the size of the surface that was being
produced. This scaled bounds should be in that space I would expect but they are
not here?

[xlai (Olivia), 2016/11/22 16:55:59]

Actually, this part of code is exactly same as the original code. It'll be more
clearer after I put this part of code as a peer function of the existing
PopulateScaledSharedQuadState(). The whole CL only changes the
quad_to_target_transform
in the SharedQuadState and not any other attributes.

It is a bit complicated to explain why. Let me list out the standard printout on
bounds and
surface_size on device-scale-factor=2 case, coupled with the css style change.
Let's say css forces canvas to be 400X100, intrinsic canvas size is 300X300.

(1) layer bounds() for canvas's layer is 800X200, which means it already takes
account the style change and is already double based on device_scale_factor.
(2) surface_size remains as 300X300 and surface_scale_ remains as 1. 

But! For the SurfaceLayer that already exists for a global webpage (before I
introduce canvas's SurfaceLayer):

(1) layer bounds() remains almost the same as the layer bounds() when
device_scale_factor=1.
(2) surface_size is double as the the surface_size in DSF=1
(3) surface_scale is 2.

To accommodate both cases (as they share the same part of code here),
scaled_bounds computation here is doing things right because:
(1) In canvas's case, bounds() is double, surface_scale_ is same, so the scaling
make the overall scaled_bounds double, which is right for DSF=2.
(2) In existing SurfaceLayer's case, bounds() is same, surface_scale_ is double,
so the scaling makes the overall scaled_bounds double, which is again right for
DSF=2.

You might ask why bounds(), surface_size_ and DSF, PDSF, surface_scale all look
different (in fact, opposite) in both cases. weiliangc@ and I are making a guess
that Chromium and Blink handle all these differently but exactly where are the
part handling this is far more complex than we thought and after doing an
intensive code search we still cannot see a full picture clearly. 
I have to admit that I figure out my computation based on the standard output
printing different variables (PDSF, DSF, bounds, etc) on different test cases on
high DPI and low DPI screens, couple with a lot of trial-and-error efforts to
make all of them correct.

[danakj, 2016/11/22 19:19:13]

My feeling is that it doesn't make sense to change one part and not the other,
as they are tied together.
This is true when UseZoomForDSF is true, but when it is false, the layer bounds
will stay 400x100.
So intrinsic canvas size does not change if your device-scale-factor changes, is
my take away here?


Let's start with canvas and css match and both are 100x100, with DSF=2. In that
case the canvas should cover 200x200 pixels in its target when it is drawn.

With UseZoomForDSF for a canvas:
- CSS size=100x100
- Bounds=200x200
- Surface size=100x100
- scale = surfacesize / bounds = 100/200 = 1/2
- transform = 1/scale = 2

This says that when going from the layer to its target everything grows by 2x so
the 100x100 pixels in the canvas will cover 200x200 pixels in the target, giving
what we want under DSF=2.


Without UseZoomForDSF for a canvas (did you try this mode is it true? this is
how you can change it
https://cs.chromium.org/chromium/src/content/common/content_switches_internal...
to use the mode):
- CSS size=100x100
- Bounds=100x100
- Surface size=100x100
- scale = surfacesize / bounds = 100/100 = 1
- transform = 1/scale = 1

Here we'll take the 100x100 pixels from the canvas and map them to 100x100
pixels in the target. This seems wrong we want it to cover 200x200 pixels in the
target.


With UseZoomForDSF for web/compositor content:
- CSS size=100x100
- Bounds=200x200
- Surface size=200x200
- scale = surfacesize / bounds = 200/200 = 1
- transform = 1/scale = 1

Here we have already DSF-scaled content of 200x200 and we transform it by
identity so we get 200x200 pixels in the target, great.

Without UseZoomForDSF for web/compositor content:
- CSS size=100x100
- Bounds=100x100
- Surface size=200x200
- scale = surfacesize / bounds = 200/100 = 2
- transform = 1/scale = 1/2

I think that things go awry here again. Web content is scaled so you have a
200x200 surface, but since we're not considering the DSF here we end scaling the
content by 1/2 to fill 100x100 pixels in the target which is incorrect.
This would be true for canvas also if UseZoomForDSF was disabled (which it is
for some platforms).
This is no longer canvas, which is why you're seeing this behave differently.
For compositor outputs, their intrinsic sizes double when the
device-scale-factor doubles. Intrinsic canvas size seems to differ here?
Same, this is because the intrinsic size of the content itself doubles for web
content when the device-scale-factor changes.
I claim above that it is doing something incorrect when UseZoomForDSF is false,
so would like to understand if I'm wrong about that or we need to handle this.
The documentation for quad_layer_bounds
https://cs.chromium.org/chromium/src/cc/quads/shared_quad_state.h?rcl=0&l=47
says that the rect is in the same space as the quad rects.

The quad rects
https://cs.chromium.org/chromium/src/cc/quads/draw_quad.h?rcl=0&l=55 are in a
space such that when you apply the transform you get the quad's output space in
its target.

I think that we need to change the rect computation here because you changed the
transform, which changes the space that the quad rects are defined in. But we
did not change the bounds/visible rect. If we change the space they are in, but
don't change them, then multiplying them by the transform will give a different
result.

We can enumerate the cases above again for these rects.

With UseZoomForDSF for a canvas:
- CSS size=100x100
- Bounds=200x200
- Surface size=100x100
- scale = surfacesize / bounds = 100/200 = 1/2
- transform = 1/scale = 2
- Surface scale=1
- DSF=1
- PaintedDSF=2

The quad rect and layer bounds are being scaled by 2 to find them in the target
space. However the layer rect we're giving is bounds*surfacescale = 200x200 * 1
= 200x200. When you apply the transform to that you get 400x400 in the target
which is not right. I think this must be the inverse of the transform to get the
right result. 200x200 * 1/2 = 100x100 which represents the intrinsic size of the
canvas correctly, and which when transformed to the target gives us the correct
200x200 pixels there.

Another way to think of this is that the "layer bounds" and "visible rect" here
should be in the space of the intrinsic canvas size. That is because the
transform is mapping pixels from the intrinsic canvas size to the area of the
target we want them to cover.


Without UseZoomForDSF for a canvas (did you try this mode is it true? this is
how you can change it
https://cs.chromium.org/chromium/src/content/common/content_switches_internal...
to use the mode):
- CSS size=100x100
- Bounds=100x100
- Surface size=100x100
- scale = surfacesize / bounds = 100/100 = 1
- transform = 1/scale = 1
- Surface scale=1
- DSF=2
- PaintedDSF=1

Here we were mapping the pixels incorrectly already I think, so the origin space
from the intrinsic canvas space was wrong. We have to fix that before we can
talk about the bounds/visible rect.

The correct transform here would be 2 again for the pixels to be drawn
correctly. Since the canvas intrinsic size is 100x100 but we want it to cover
200x200 in the target. Since the bounds are 100x100 they are already in the
origin space/intrinsic canvas space. We actually don't need to scale them at
all.


With UseZoomForDSF for web/compositor content:
- CSS size=100x100
- Bounds=200x200
- Surface size=200x200
- scale = surfacesize / bounds = 200/200 = 1
- transform = 1/scale = 1
- Surface scale=2
- DSF=1
- PaintedDSF=2

Here the intrinsic size of the content is 200x200 (note the surface scale=2). So
the bounds actually do not need to be scaled. Worth noting here that the old
code *did* scale the bounds here, which would make them 400x400 which is wrong.
I'm certain that has been pretty confusing here. The transform scale of 1 here
is right since we don't need to scale the pixels from the intrinsic size. The
bounds also don't need to be scaled.


Without UseZoomForDSF for web/compositor content:
- CSS size=100x100
- Bounds=100x100
- Surface size=200x200
- scale = surfacesize / bounds = 200/100 = 2
- transform = 1/scale = 1/2
- Surface scale=2
- DSF=2
- PaintedDSF=1

Here the bounds do not match the intrinsic size of the content (surface
scale=2). We can get the bounds to match the intrinsic size of the content by
scaling by the surface scale which is what the old code did. This is the case
that the old code was written for. The correct transform scale here would be 1
as the intrinsic size is already scaled, to get it drawn over 200x200 pixels in
the target.

However, since the bounds do not match the intrinsic size, we can't just use
them directly. That's why the old code scaled bounds by the surface scale.

In all four cases, from what I can see, if you multiply the bounds by "Surface
scale" / PaintedDSF, you get bounds in the space of the content / origin space /
intrinsic space.

And if you take 1 / surface size / bounds / DSF, you get a correct scale from
the pixels of the intrinsic space to the target space.

Does this hold for the case where the surface size (intrinsic size) is different
from the layer bounds?

UseZoomForDSF for canvas:
- CSS size=300x300
- Bounds=600x600
- Surface size=400x100
- Surface scale=1
- DSF=1
- PaintedDSF=2

Bounds * (Surface scale / PaintedDSF) = 300x300 <- Bounds in Origin space /
Intrinsic space. The intrinsic size is relative to these.

1 / (Surface size / bounds / DSF) = Scale[6/4,6/1] <- Transform from Origin
space to target space. If you multiply 400x100 by this you get 600x600 which is
the right number of pixels in the target.

Not-UseZoomForDSF for canvas:
- CSS size=300x300
- Bounds=300x300
- Surface size=400x100
- Surface scale=1
- DSF=2
- PaintedDSF=1

Bounds * (Surface scale / PaintedDSF) = 300x300  <- Note same result yay
1 / (Surface size / bounds / DSF) = Scale[6/4,6/1] <- Same result again

UseZoomForDSF for web/compositor:
- CSS size=300x300
- Bounds=600x600
- Surface size=800x200
- Surface scale=2
- DSF=1
- PaintedDSF=2

Bounds * (Surface scale / PaintedDSF) = 600x600 <- The intrinsic size of web
content is doubled compared to canvas since it's scaled by the DSF.
1 / (Surface size / bounds / DSF) = Scale[6/8,6/2] <- The transform scales by
1/2 of the above to account for the source pixels being twice as many, to cover
the space space in the target. 800x200 scaled by this gives 600x600 which is
what we want still.

Not-UseZoomForDSF for web/compositor:
- CSS size=300x300
- Bounds=300x300
- Surface size=800x200
- Surface scale=2
- DSF=2
- PaintedDSF=1

Bounds * (Surface scale / PaintedDSF) = 600x600 <- Same result, good.
1 / (Surface size / bounds / DSF) = Scale[6/8,6/2] <- Same again, yay.
I think we had 2 cases to consider before, and now we have 4.

Before:

UseZoomForDSF + web/compositor output
!UseZoomForDSF + web/compositor output

Now also:

UseZoomForDSF + canvas output
!UseZoomForDSF + canvas output

The difference between these from what you wrote above appears to be that the
web/compositor output intrinsic size is scaled by the DSF, but canvas' is not.

Also the code being wrong for UseZoomForDSF is a thing.

When you get the layer rect wrong what happens is we won't use AA on the edges
correctly when the surface is rotated. This doesn't happen much for existing use
cases so I can see that not being noticed.

When you get the visible rect wrong I'm not sure what happens for surface quads
right now. Other quads use this in GL renderer to avoid overdrawing, but I'm not
sure if it's used (right now) for surface quads.

+  gfx::Rect scaled_visible_layer_rect =
+      gfx::ScaleToEnclosingRect(visible_layer_rect(), surface_scale_);
+  scaled_visible_layer_rect.Intersect(gfx::Rect(scaled_bounds));
+
+  shared_quad_state->SetAll(

[danakj, 2016/11/21 23:37:22]

If you can't just add params toPopulateScaledSharedQuadState(),  I'd prefer this
move to a 2nd method as a peer of it, so the duplicated code is easier to find.

[xlai (Olivia), 2016/11/22 16:55:59]

Done.

+      scaled_draw_transform, scaled_bounds, scaled_visible_layer_rect,
+      draw_properties().clip_rect, draw_properties().is_clipped,
+      draw_properties().opacity, draw_blend_mode(), sorting_context_id_);
 
   if (!surface_id_.is_valid())
     return;