Implement transform/clip support for Android WebView.

Implement transform/clip support for Android WebView.

Transforms are applied above the root-layer.  I fixed LTHCommon to forward
root-layer transforms to sublayers, as the RenderSurface-based logic was previously
clearing transforms and copying over only the scale portion.

The clip rect is treated as the viewport for the purposes of DrawQuads and Renderer
(this is required to avoid awful performance when the WebView is much larger than
the screen).  Because y-flipping the clip rect depends on knowledge of the true
surface size, I also needed to add a new OutputSurface::SurfaceSize() getter and refactored
viewport size throughout the Renderers to separate render-pass draw rect, glViewport rect,
and surface size.

Scale and translate transforms work with this patch, but rotation is still broken.

New tests: LayerTreeHostCommonTest.TransformAboveRootLayer,
GLRendererTest2.ScissorAndViewportWithinNonreshapableSurface,
RendererPixelTest/2* and 3*

NOTRY=true
BUG=230463
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=204650

[aelias_OOO_until_Jul13, 2013-05-25 02:34:55]

Hi Enne, PTAL.  This is getting clean and functional enough to start reviewing.

[joth, 2013-05-25 02:56:40]

Thanks for handling this aelias!

https://codereview.chromium.org/15579002/diff/5001/cc/output/output_surface_c...
File cc/output/output_surface_client.h (right):

https://codereview.chromium.org/15579002/diff/5001/cc/output/output_surface_c...
cc/output/output_surface_client.h:19: virtual void
SetDeviceTransformAndClip(gfx::Transform transform,
nit: transforms normally travel by const&
(so you can fwd declare rather than #include)

https://codereview.chromium.org/15579002/diff/5001/content/renderer/android/s...
File content/renderer/android/synchronous_compositor_output_surface.cc (right):

https://codereview.chromium.org/15579002/diff/5001/content/renderer/android/s...
content/renderer/android/synchronous_compositor_output_surface.cc:156: static
gfx::Transform adjustTransformForClip(gfx::Transform transform,
nits:

prefer namespace {} to static functions.

capitalize: AdjustTransformForClip()

cont& for transform

maybe comment why we need to remove the clip origin?

https://codereview.chromium.org/15579002/diff/5001/content/renderer/android/s...
content/renderer/android/synchronous_compositor_output_surface.cc:184: static
gfx::Rect clipFlipY(gfx::Rect clip, gfx::Size view_size) {
ditto - namespace {} / capitalize

https://codereview.chromium.org/15579002/diff/5001/content/renderer/android/s...
content/renderer/android/synchronous_compositor_output_surface.cc:199:
clipFlipY(clip, view_size));
any idea why we do need to flip Y for clip (in HW mode only) but not for the
SetNeedsRedrawRect() call that follows it?

[aelias_OOO_until_Jul13, 2013-05-25 03:33:39]

OK, all done.

On 2013/05/25 02:56:40, joth wrote:
> content/renderer/android/synchronous_compositor_output_surface.cc:199:
> clipFlipY(clip, view_size));
> any idea why we do need to flip Y for clip (in HW mode only) but not for the
> SetNeedsRedrawRect() call that follows it?

It's because we only use the size portion for the damage area, since CC's
viewport is rooted at the clip origin for the purpose of damage.  I clarified it
by passing in a gfx::Size as method argument instead of a rect.

[joth, 2013-05-26 00:24:39]

lgtm for content/renderer/android (and re. the other comments I made)

[enne (OOO), 2013-05-28 18:26:19]

+shawnsingh, danakj for transform-related changes FYI

Is it the case that this offset is relative to the current scroll offsets on the
page and is not some absolute offset?

It seems undesirable that fixed position elements will move offscreen here with
an offset because they are attached to the "real" viewport.  Moreover, I am
having a hard time seeing what the natural progression to implementing rotation
is after this.  Is there scale after that too?

I know you don't want to do this with layers and transforms, but it really seems
like you're trying to apply some transform below the clip layer.  If you did
that, then it seems like you wouldn't need all the changes to the renderer or
output surface client.

[aelias_OOO_until_Jul13, 2013-05-28 20:49:33]

On 2013/05/28 18:26:19, enne wrote:
> Is it the case that this offset is relative to the current scroll offsets on
the
> page and is not some absolute offset?

It's an absolute offset, it doesn't have much to do with scrolling.

> 
> It seems undesirable that fixed position elements will move offscreen here
with
> an offset because they are attached to the "real" viewport.

Agreed, this is still unresolved as of this patch.  I want to iterate towards
making the "real" viewport be the one specified by OutputSurfaceClient for
everything including fixed-pos, and basically stop using device_viewport_size_
(which is out of date).

 Moreover, I am
> having a hard time seeing what the natural progression to implementing
rotation
> is after this.  Is there scale after that too?

Rotation already kind of works in that the orientation is correct.  We just need
to replace the scissor rect with a stencil.

> 
> I know you don't want to do this with layers and transforms, but it really
seems
> like you're trying to apply some transform below the clip layer.  If you did
> that, then it seems like you wouldn't need all the changes to the renderer or
> output surface client.

Maybe the renderer changes can be avoided but the OutputSurfaceClient calls are
inevitable.  We need to be informed synchronously about the latest clip and
transform.

[enne (OOO), 2013-05-28 21:15:51]

On 2013/05/28 20:49:33, aelias wrote:
> On 2013/05/28 18:26:19, enne wrote:
> > Is it the case that this offset is relative to the current scroll offsets on
> the
> > page and is not some absolute offset?
> 
> It's an absolute offset, it doesn't have much to do with scrolling.

My question about scrolling is mostly related to the fixed position question. 
In some ways, I see scrolling the root layer (in device pixels) as somewhat
equivalent to moving around the viewport and the scissor rect by some offset.

> > It seems undesirable that fixed position elements will move offscreen here
> with
> > an offset because they are attached to the "real" viewport.
> 
> Agreed, this is still unresolved as of this patch.  I want to iterate towards
> making the "real" viewport be the one specified by OutputSurfaceClient for
> everything including fixed-pos, and basically stop using device_viewport_size_
> (which is out of date).

How is a rotation combined with fixed position elements supposed to interact? Do
the fixed position elements stay affixed to the unrotated viewport or to the
rotated clip layer?

>  Moreover, I am
> > having a hard time seeing what the natural progression to implementing
> rotation
> > is after this.  Is there scale after that too?
> 
> Rotation already kind of works in that the orientation is correct.  We just
need
> to replace the scissor rect with a stencil.

Are you saying the rotation applies above the clip layer? If so, wouldn't that
be handled by a render surface? If you wanted to use the stencil buffer then
that seems like an optimization that could be handled more generally in cc.

[aelias_OOO_until_Jul13, 2013-05-28 22:21:54]

OK, we chatted a bit more about fixed-pos and agreed they should stay attached
to the View size, i.e. clip layer.  If we attached them to the
OutputSurfaceClient clip rect, they would behave oddly when transforms are used
to animate the WebView off the screen.

On 2013/05/28 21:15:51, enne wrote:
> Are you saying the rotation applies above the clip layer?

Yes, but:

> If so, wouldn't that
> be handled by a render surface? If you wanted to use the stencil buffer then
> that seems like an optimization that could be handled more generally in cc.

For us, the OpenGL context will come in with an appropriate stencil buffer
already there.  So we don't need either a render surface nor populating the
stencil buffer within CC.  We just need to make DrawQuads for the largest
possible exposed area and then draw without scissor.

[aelias_OOO_until_Jul13, 2013-05-29 06:35:13]

Also:

On 2013/05/28 18:26:19, enne wrote:
> I know you don't want to do this with layers and transforms, but it really
seems
> like you're trying to apply some transform below the clip layer.

The transform needs to be above the clip layer.  (The relevant use case is the
whole WebView getting moved around during an animation.)  And the
OutputSurfaceClient clip needs to be above the transform.

Instead of modifying LTHCommon, I can also do it by changing the root-layer
sublayer matrix, if you think that's cleaner.  I tested that and it works fine.

With respect to the glViewport/windowMatrix changes, they could be avoided by
offsetting the root clip rect given to LTHCommon, but I when I tried this I ran
into some kind of hidden assumptions that the device viewport is rooted at (0,
0) rather than somewhere else.  Any thoughts on what those could be?

[aelias_OOO_until_Jul13, 2013-05-29 07:00:03]

On 2013/05/29 06:35:13, aelias wrote:
> With respect to the glViewport/windowMatrix changes, they could be avoided by
> offsetting the root clip rect given to LTHCommon, but I when I tried this I
ran
> into some kind of hidden assumptions that the device viewport is rooted at (0,
> 0) rather than somewhere else.  Any thoughts on what those could be?

Trying this approach again, some sort of glViewport change is mandatory
otherwise the view is cut off to the bottom left of the screen.  There are two
possible fixes, expand the glViewport size to be the entire device screen size,
or do the translation fix I did.  I think the translation fix is more elegant. 
And that's on top of fixing some other problems around
damage/calc-draw-properties that I haven't figured out the cause of yet.  So I'm
going to claim that the version I uploaded is the best way to do it.

[enne (OOO), 2013-05-29 20:34:12]

What are your thoughts about testing these changes? What do you think about unit
tests for the transform changes and at least 1 hardware/software pixel test for
the renderer changes?

https://codereview.chromium.org/15579002/diff/5003/cc/trees/layer_tree_host_c...
File cc/trees/layer_tree_host_common.cc (right):

https://codereview.chromium.org/15579002/diff/5003/cc/trees/layer_tree_host_c...
cc/trees/layer_tree_host_common.cc:801: const gfx::Transform&
next_sublayer_matrix,
I'm always skeptical about adding more parameters to this function.  Maybe I'm
misunderstanding things, but is there a reason you can't just apply this
transform at the top of the tree?

https://codereview.chromium.org/15579002/diff/5003/cc/trees/layer_tree_host_i...
File cc/trees/layer_tree_host_impl.h (right):

https://codereview.chromium.org/15579002/diff/5003/cc/trees/layer_tree_host_i...
cc/trees/layer_tree_host_impl.h:198: virtual gfx::Vector2d
DeviceViewportOffset() const OVERRIDE;
Unless I'm misunderstanding your requirements, it sounds like there are two
things you need from the output surface: a transform and a device viewport rect.
 However, I see viewport size, viewport offset, device clip, and device
transform.

Does it make sense to merge the viewport offset and size into just a rect? It
looks like that gets used in a number of places as a rect.

Also, is there a difference between viewport rect and device clip? Can these be
merged?

https://codereview.chromium.org/15579002/diff/5003/content/renderer/android/s...
File content/renderer/android/synchronous_compositor_output_surface.cc (right):

https://codereview.chromium.org/15579002/diff/5003/content/renderer/android/s...
content/renderer/android/synchronous_compositor_output_surface.cc:164: // CC's
cliprect is relative to the bottom-left in hardware mode and
This seems like an unfortunate inconsistency.  Does it seem like this could be
easily fixed?

[aelias_OOO_until_Jul13, 2013-05-29 21:13:35]

On 2013/05/29 20:34:12, enne wrote:
> What are your thoughts about testing these changes? What do you think about
unit
> tests for the transform changes and at least 1 hardware/software pixel test
for
> the renderer changes?
> 

OK, I'll start working on that.

>
https://codereview.chromium.org/15579002/diff/5003/cc/trees/layer_tree_host_c...
> File cc/trees/layer_tree_host_common.cc (right):
> 
>
https://codereview.chromium.org/15579002/diff/5003/cc/trees/layer_tree_host_c...
> cc/trees/layer_tree_host_common.cc:801: const gfx::Transform&
> next_sublayer_matrix,
> I'm always skeptical about adding more parameters to this function.  Maybe I'm
> misunderstanding things, but is there a reason you can't just apply this
> transform at the top of the tree?

Scale transforms can be applied at the top but translates have no effect there. 
Empirically, the origin of the draw seems to be the position of the root layer,
so if I translate the entire tree, nothing happens.  Does that match your
understanding of what ought to happen?

> 
>
https://codereview.chromium.org/15579002/diff/5003/cc/trees/layer_tree_host_i...
> File cc/trees/layer_tree_host_impl.h (right):
> 
>
https://codereview.chromium.org/15579002/diff/5003/cc/trees/layer_tree_host_i...
> cc/trees/layer_tree_host_impl.h:198: virtual gfx::Vector2d
> DeviceViewportOffset() const OVERRIDE;
> Unless I'm misunderstanding your requirements, it sounds like there are two
> things you need from the output surface: a transform and a device viewport
rect.
>  However, I see viewport size, viewport offset, device clip, and device
> transform.
> 
> Does it make sense to merge the viewport offset and size into just a rect? It
> looks like that gets used in a number of places as a rect.
> 
> Also, is there a difference between viewport rect and device clip? Can these
be
> merged?

device_viewport_size_ needs to be maintained separately as it refers to the
WebView size (I should probably rename it to view_size_ since "device" can now
be a misnomer.)  Other than than we need to maintain one rect for DeviceClip
(I'll merge it).

> 
>
https://codereview.chromium.org/15579002/diff/5003/content/renderer/android/s...
> File content/renderer/android/synchronous_compositor_output_surface.cc
(right):
> 
>
https://codereview.chromium.org/15579002/diff/5003/content/renderer/android/s...
> content/renderer/android/synchronous_compositor_output_surface.cc:164: // CC's
> cliprect is relative to the bottom-left in hardware mode and
> This seems like an unfortunate inconsistency.  Does it seem like this could be
> easily fixed?

We need to correct for the inconsistency somewhere given that OpenGL is
bottom-left origin and Skia is top-left.  The correction needs to be based on
the true GL target surface size, which Android WebView code has visibility into
but CC does not (there doesn't seem to be glGet call that provides it).  I
wanted to avoid plumbing in yet another concept of viewport size, so this seemed
like the best place to correct it.

[enne (OOO), 2013-05-29 21:33:15]

On 2013/05/29 21:13:35, aelias wrote:
> On 2013/05/29 20:34:12, enne wrote:
> > What are your thoughts about testing these changes? What do you think about
> unit
> > tests for the transform changes and at least 1 hardware/software pixel test
> for
> > the renderer changes?
> > 
> 
> OK, I'll start working on that.

Thanks.  I know writing tests can be a bit of a pain, but having some smoke
tests will make me a lot more confident about keeping this path functional.

> > I'm always skeptical about adding more parameters to this function.  Maybe
I'm
> > misunderstanding things, but is there a reason you can't just apply this
> > transform at the top of the tree?
> 
> Scale transforms can be applied at the top but translates have no effect
there. 
> Empirically, the origin of the draw seems to be the position of the root
layer,
> so if I translate the entire tree, nothing happens.  Does that match your
> understanding of what ought to happen?

That definitely does not match my understanding.  Is your viewport offset
somehow cancelling out the translation?

> > Unless I'm misunderstanding your requirements, it sounds like there are two
> > things you need from the output surface: a transform and a device viewport
> rect.
> >  However, I see viewport size, viewport offset, device clip, and device
> > transform.
> > 
> > Does it make sense to merge the viewport offset and size into just a rect?
It
> > looks like that gets used in a number of places as a rect.
> > 
> > Also, is there a difference between viewport rect and device clip? Can these
> be
> > merged?
> 
> device_viewport_size_ needs to be maintained separately as it refers to the
> WebView size (I should probably rename it to view_size_ since "device" can now
> be a misnomer.)  Other than than we need to maintain one rect for DeviceClip
> (I'll merge it).

My understanding here is that the GL viewport contains the webview (which is the
viewport given to cc), and the clip is equivalent to that webview viewport rect?
If so, why does the size and offset of the webview need to be maintained
separately?

> We need to correct for the inconsistency somewhere given that OpenGL is
> bottom-left origin and Skia is top-left.  The correction needs to be based on
> the true GL target surface size, which Android WebView code has visibility
into
> but CC does not (there doesn't seem to be glGet call that provides it).  I
> wanted to avoid plumbing in yet another concept of viewport size, so this
seemed
> like the best place to correct it.

If you passed the full viewport and the clip as a subset of that, then I suppose
you could defer the flip to cc. I know that Ganesh also does its own flip
internally too, so it's possible that you may have to end up going down that
path.  I'm ok with either cc or the Android WebView code managing the flip if
everything keeps working, but I wanted to mention this as a potential hazard.

[aelias_OOO_until_Jul13, 2013-05-29 21:51:12]

On 2013/05/29 21:33:15, enne wrote:
> > Scale transforms can be applied at the top but translates have no effect
> there. 
> > Empirically, the origin of the draw seems to be the position of the root
> layer,
> > so if I translate the entire tree, nothing happens.  Does that match your
> > understanding of what ought to happen?
> 
> That definitely does not match my understanding.  Is your viewport offset
> somehow cancelling out the translation?

It shouldn't -- I'll try to figure out what's going on here.

> > device_viewport_size_ needs to be maintained separately as it refers to the
> > WebView size (I should probably rename it to view_size_ since "device" can
now
> > be a misnomer.)  Other than than we need to maintain one rect for DeviceClip
> > (I'll merge it).
> 
> My understanding here is that the GL viewport contains the webview (which is
the
> viewport given to cc), and the clip is equivalent to that webview viewport
rect?
> If so, why does the size and offset of the webview need to be maintained
> separately?

WebView size is specified by the application developer, whereas the clip is
provided on every frame by the system.  The WebView size is what's told to Blink
and we also need to position fixed-pos elements within it.  It can be huge (say
10 screens high).  So we also need to be smart and only generate DrawQuads for
what's actually visible on the screen, and that's why we use the clip rect as
viewport size for CalcDrawProperties.

[joth, 2013-06-01 01:28:20]

https://codereview.chromium.org/15579002/diff/5003/content/renderer/android/s...
File content/renderer/android/synchronous_compositor_output_surface.cc (right):

https://codereview.chromium.org/15579002/diff/5003/content/renderer/android/s...
content/renderer/android/synchronous_compositor_output_surface.cc:195: gfx::Size
view_size,
view_size -> surface_size or device_size ?

[aelias_OOO_until_Jul13, 2013-06-04 07:16:14]

PTAL, I added two tests and restructured as discussed offline.  I think this
should be ready to land.

Adding jamesr@ for trivial OWNERS review of webkit/renderer/.

A few notes:
- To deal with the scissor y-flip within CC, I added a SurfaceSize() getter to
OutputSurface which normally caches what was passed in reshape(), but in the
WebView case a fixed value is returned instead.  I took the opportunity to clean
up viewport size stuff throughout the Renderers.
- LTHCommon was not correctly dealing with root-layer RenderSurface specialness
-- my change there amounts to a bugfix.
- I deleted
LayerPositionConstraintTest.ScrollCompensationForFixedPositionLayerThatHasNoContainer.
 Properly supporting root-layer transforms broke the assumptions of this test,
and I don't know why this test existed as we have no plans to ever structure the
layer tree that way in reality.

[jamesr, 2013-06-04 07:20:02]

On 2013/06/04 07:16:14, aelias wrote:
> Adding jamesr@ for trivial OWNERS review of webkit/renderer/.

enne@ has OWNERS there as well, are you still waiting for their review?  If so,
shouldn't need me.

[aelias_OOO_until_Jul13, 2013-06-04 07:24:37]

On 2013/06/04 07:20:02, jamesr wrote:
> enne@ has OWNERS there as well, are you still waiting for their review?  If
so,
> shouldn't need me.

Ah thanks, I didn't realize there was narrow subdirectory OWNERS.

[danakj, 2013-06-04 13:59:34]

On Tue, Jun 4, 2013 at 3:16 AM, <aelias@chromium.org> wrote:

> PTAL, I added two tests and restructured as discussed offline.  I think
> this
> should be ready to land.
>
> Adding jamesr@ for trivial OWNERS review of webkit/renderer/.
>
> A few notes:
> - To deal with the scissor y-flip within CC, I added a SurfaceSize()
> getter to
> OutputSurface which normally caches what was passed in reshape(), but in
> the
> WebView case a fixed value is returned instead.  I took the opportunity to
> clean
> up viewport size stuff throughout the Renderers.
> - LTHCommon was not correctly dealing with root-layer RenderSurface
> specialness
> -- my change there amounts to a bugfix.
> - I deleted
> LayerPositionConstraintTest.**ScrollCompensationForFixedPosi**
> tionLayerThatHasNoContainer.
>  Properly supporting root-layer transforms broke the assumptions of this
> test,
> and I don't know why this test existed as we have no plans to ever
> structure the
> layer tree that way in reality.
>

Maybe you could break this into a few CLs that each do one thing?


>
>
https://codereview.chromium.**org/15579002/<https://codereview.chromium.org/1...
>

[enne (OOO), 2013-06-04 17:26:03]

https://codereview.chromium.org/15579002/diff/44001/cc/layers/layer_position_...
File cc/layers/layer_position_constraint_unittest.cc (left):

https://codereview.chromium.org/15579002/diff/44001/cc/layers/layer_position_...
cc/layers/layer_position_constraint_unittest.cc:1034:
ScrollCompensationForFixedPositionLayerThatHasNoContainer) {
What do you mean by "we don't ever plan to structure the tree this way"?

https://codereview.chromium.org/15579002/diff/44001/cc/output/direct_renderer.cc
File cc/output/direct_renderer.cc (right):

https://codereview.chromium.org/15579002/diff/44001/cc/output/direct_renderer...
cc/output/direct_renderer.cc:215:
output_surface_->Reshape(client_->DeviceViewport().size(),
Should this only happen if the device viewport has changed?

https://codereview.chromium.org/15579002/diff/44001/cc/output/software_render...
File cc/output/software_renderer.cc (right):

https://codereview.chromium.org/15579002/diff/44001/cc/output/software_render...
cc/output/software_renderer.cc:155: frame, framebuffer_rect,
gfx::Rect(framebuffer_rect.size()), false);
I'm a little curious about this change.  When does this framebuffer rect not
have a position of zero? Should this function be taking in a gfx::Size instead
of a rect?

https://codereview.chromium.org/15579002/diff/44001/cc/output/software_render...
cc/output/software_renderer.cc:196: void
SoftwareRenderer::SetDrawViewport(gfx::Rect viewport) {}
How does the software renderer get away with ignoring the viewport?

https://codereview.chromium.org/15579002/diff/44001/cc/trees/layer_tree_host_...
File cc/trees/layer_tree_host_common.cc (right):

https://codereview.chromium.org/15579002/diff/44001/cc/trees/layer_tree_host_...
cc/trees/layer_tree_host_common.cc:1119: // The root layer isn't capable of
transforming its render surface since
Hmm.  I'm not sure about the "external to the CC instance" language.  Can you
instead say something like:

The root layer's render surface size is predetermined and so the root layer
can't directly support non-identity scale transforms.  It should just forward
top-level transforms to the rest of the tree.

https://codereview.chromium.org/15579002/diff/44001/cc/trees/layer_tree_host_...
cc/trees/layer_tree_host_common.cc:1122: sublayer_matrix = combined_transform;
What if the root layer also has a sublayer_matrix already? Can you add a test
for this?

https://codereview.chromium.org/15579002/diff/44001/content/browser/android/i...
File content/browser/android/in_process/synchronous_compositor_output_surface.cc
(right):

https://codereview.chromium.org/15579002/diff/44001/content/browser/android/i...
content/browser/android/in_process/synchronous_compositor_output_surface.cc:160:
// origin of the clip rect, but CC's draw origin starts at the clip.
Can you explain this? I would have expected with the glViewport calls with a
non-zero x/y that this would not be true.

[danakj, 2013-06-04 17:26:39]

https://codereview.chromium.org/15579002/diff/44001/cc/trees/layer_tree_host_...
File cc/trees/layer_tree_host_common.cc (left):

https://codereview.chromium.org/15579002/diff/44001/cc/trees/layer_tree_host_...
cc/trees/layer_tree_host_common.cc:1129:
DCHECK_EQ(render_surface_sublayer_scale.x(),
Can these checks stay in the IsRootLayer() block? A scale here would still be
meaningless and get silently dropped on the root layer (tho applied to its
children).

[danakj, 2013-06-04 17:31:30]

https://codereview.chromium.org/15579002/diff/44001/cc/trees/layer_tree_host_...
File cc/trees/layer_tree_host_common.cc (left):

https://codereview.chromium.org/15579002/diff/44001/cc/trees/layer_tree_host_...
cc/trees/layer_tree_host_common.cc:1129:
DCHECK_EQ(render_surface_sublayer_scale.x(),
On 2013/06/04 17:26:40, danakj wrote:
> Can these checks stay in the IsRootLayer() block? A scale here would still be
> meaningless and get silently dropped on the root layer (tho applied to its
> children).

enne pointed out that webview may want to actually scale the subtree. Instead of
setting the webview's scale on the root layer transform, can it be set on the
root layer's sublayer transform directly, so that no information is being
silently lost here (the root layer won't draw itself with a scale), and the
dchecks can remain?

[aelias_OOO_until_Jul13, 2013-06-04 17:54:19]

https://codereview.chromium.org/15579002/diff/44001/cc/layers/layer_position_...
File cc/layers/layer_position_constraint_unittest.cc (left):

https://codereview.chromium.org/15579002/diff/44001/cc/layers/layer_position_...
cc/layers/layer_position_constraint_unittest.cc:1034:
ScrollCompensationForFixedPositionLayerThatHasNoContainer) {
On 2013/06/04 17:26:03, enne wrote:
> What do you mean by "we don't ever plan to structure the tree this way"?

A fixed-pos layer will always have a clip layer above it in practice, so I
figured we could leave the case where there's no clip layer having undefined
behavior.

https://codereview.chromium.org/15579002/diff/44001/cc/output/direct_renderer.cc
File cc/output/direct_renderer.cc (right):

https://codereview.chromium.org/15579002/diff/44001/cc/output/direct_renderer...
cc/output/direct_renderer.cc:215:
output_surface_->Reshape(client_->DeviceViewport().size(),
On 2013/06/04 17:26:03, enne wrote:
> Should this only happen if the device viewport has changed?

I added a check within OutputSurface itself not to issue the reshape to the GL
context unless it's changed.  I think this is more robust than the
viewportChanged boolean we had before.

https://codereview.chromium.org/15579002/diff/44001/cc/output/software_render...
File cc/output/software_renderer.cc (right):

https://codereview.chromium.org/15579002/diff/44001/cc/output/software_render...
cc/output/software_renderer.cc:155: frame, framebuffer_rect,
gfx::Rect(framebuffer_rect.size()), false);
On 2013/06/04 17:26:03, enne wrote:
> I'm a little curious about this change.  When does this framebuffer rect not
> have a position of zero? Should this function be taking in a gfx::Size instead
> of a rect?

I'll investigate and consider changing the argument to gfx::Size.

https://codereview.chromium.org/15579002/diff/44001/cc/output/software_render...
cc/output/software_renderer.cc:196: void
SoftwareRenderer::SetDrawViewport(gfx::Rect viewport) {}
On 2013/06/04 17:26:03, enne wrote:
> How does the software renderer get away with ignoring the viewport?

The software renderer applies the window_matrix directly.  It gets the viewport
transformation that way instead.

https://codereview.chromium.org/15579002/diff/44001/cc/trees/layer_tree_host_...
File cc/trees/layer_tree_host_common.cc (left):

https://codereview.chromium.org/15579002/diff/44001/cc/trees/layer_tree_host_...
cc/trees/layer_tree_host_common.cc:1129:
DCHECK_EQ(render_surface_sublayer_scale.x(),


On 2013/06/04 17:31:30, danakj wrote:
> On 2013/06/04 17:26:40, danakj wrote:
> > Can these checks stay in the IsRootLayer() block? A scale here would still
be
> > meaningless and get silently dropped on the root layer (tho applied to its
> > children).
> 
> enne pointed out that webview may want to actually scale the subtree. Instead
of
> setting the webview's scale on the root layer transform, can it be set on the
> root layer's sublayer transform directly, so that no information is being
> silently lost here (the root layer won't draw itself with a scale), and the
> dchecks can remain?

Patch Set 1 above applied it directly to root-layer sublayer transform.  But we
were reluctant either to add another argument to LTHCommon or to mutate a field
on the root-layer that is normally only changed on commit.

Could you clarify what you mean by "lost information"?  Note that I added test
coverage that the root render-surface draw transform will remain identityMatrix,
if that helps.

https://codereview.chromium.org/15579002/diff/44001/cc/trees/layer_tree_host_...
File cc/trees/layer_tree_host_common.cc (right):

https://codereview.chromium.org/15579002/diff/44001/cc/trees/layer_tree_host_...
cc/trees/layer_tree_host_common.cc:1119: // The root layer isn't capable of
transforming its render surface since
On 2013/06/04 17:26:03, enne wrote:
> Hmm.  I'm not sure about the "external to the CC instance" language.  Can you
> instead say something like:
> 
> The root layer's render surface size is predetermined and so the root layer
> can't directly support non-identity scale transforms.  It should just forward
> top-level transforms to the rest of the tree.

Done.

https://codereview.chromium.org/15579002/diff/44001/cc/trees/layer_tree_host_...
cc/trees/layer_tree_host_common.cc:1122: sublayer_matrix = combined_transform;
On 2013/06/04 17:26:03, enne wrote:
> What if the root layer also has a sublayer_matrix already? Can you add a test
> for this?

It should be fine because that will be applied later in LTHC.  I'll add a test.

https://codereview.chromium.org/15579002/diff/44001/content/browser/android/i...
File content/browser/android/in_process/synchronous_compositor_output_surface.cc
(right):

https://codereview.chromium.org/15579002/diff/44001/content/browser/android/i...
content/browser/android/in_process/synchronous_compositor_output_surface.cc:160:
// origin of the clip rect, but CC's draw origin starts at the clip.
On 2013/06/04 17:26:03, enne wrote:
> Can you explain this? I would have expected with the glViewport calls with a
> non-zero x/y that this would not be true.

glViewport turns out to be a translate+scale operation, it's not really
analogous to scissor like we thought.  See
http://www.songho.ca/opengl/gl_transform.html

[danakj, 2013-06-04 18:02:50]

https://codereview.chromium.org/15579002/diff/44001/cc/layers/layer_position_...
File cc/layers/layer_position_constraint_unittest.cc (left):

https://codereview.chromium.org/15579002/diff/44001/cc/layers/layer_position_...
cc/layers/layer_position_constraint_unittest.cc:1034:
ScrollCompensationForFixedPositionLayerThatHasNoContainer) {
On 2013/06/04 17:54:20, aelias wrote:
> On 2013/06/04 17:26:03, enne wrote:
> > What do you mean by "we don't ever plan to structure the tree this way"?
> 
> A fixed-pos layer will always have a clip layer above it in practice, so I
> figured we could leave the case where there's no clip layer having undefined
> behavior.

Until the UI compositor does it? Should that be a consideration?

https://codereview.chromium.org/15579002/diff/44001/cc/trees/layer_tree_host_...
File cc/trees/layer_tree_host_common.cc (left):

https://codereview.chromium.org/15579002/diff/44001/cc/trees/layer_tree_host_...
cc/trees/layer_tree_host_common.cc:1129:
DCHECK_EQ(render_surface_sublayer_scale.x(),
On 2013/06/04 17:54:20, aelias wrote:
> 
> 
> On 2013/06/04 17:31:30, danakj wrote:
> > On 2013/06/04 17:26:40, danakj wrote:
> > > Can these checks stay in the IsRootLayer() block? A scale here would still
> be
> > > meaningless and get silently dropped on the root layer (tho applied to its
> > > children).
> > 
> > enne pointed out that webview may want to actually scale the subtree.
Instead
> of
> > setting the webview's scale on the root layer transform, can it be set on
the
> > root layer's sublayer transform directly, so that no information is being
> > silently lost here (the root layer won't draw itself with a scale), and the
> > dchecks can remain?
> 
> Patch Set 1 above applied it directly to root-layer sublayer transform.  But
we
> were reluctant either to add another argument to LTHCommon or to mutate a
field
> on the root-layer that is normally only changed on commit.
> 
> Could you clarify what you mean by "lost information"?  Note that I added test
> coverage that the root render-surface draw transform will remain
identityMatrix,
> if that helps.

Lost information: I mean that if you set a scale 2x on the root layer, it's
contents will still appear at 1x on the screen.

[aelias_OOO_until_Jul13, 2013-06-04 22:18:26]

PTAL, all comments addressed.

https://codereview.chromium.org/15579002/diff/44001/cc/layers/layer_position_...
File cc/layers/layer_position_constraint_unittest.cc (left):

https://codereview.chromium.org/15579002/diff/44001/cc/layers/layer_position_...
cc/layers/layer_position_constraint_unittest.cc:1034:
ScrollCompensationForFixedPositionLayerThatHasNoContainer) {
On 2013/06/04 18:02:50, danakj wrote:
> On 2013/06/04 17:54:20, aelias wrote:
> > On 2013/06/04 17:26:03, enne wrote:
> > > What do you mean by "we don't ever plan to structure the tree this way"?
> > 
> > A fixed-pos layer will always have a clip layer above it in practice, so I
> > figured we could leave the case where there's no clip layer having undefined
> > behavior.
> 
> Until the UI compositor does it? Should that be a consideration?

Anyway, since all we care about is some kind of consistent behavior, I brought
this back and changed this to test that fixed-pos elements are affixed to the
root layer instead.  I also simplified the LTHCommon code to avoid needing to
support the container == NULL case, passing the root_layer as starting
fixed_container instead.

https://codereview.chromium.org/15579002/diff/44001/cc/output/software_render...
File cc/output/software_renderer.cc (right):

https://codereview.chromium.org/15579002/diff/44001/cc/output/software_render...
cc/output/software_renderer.cc:155: frame, framebuffer_rect,
gfx::Rect(framebuffer_rect.size()), false);
On 2013/06/04 17:54:20, aelias wrote:
> On 2013/06/04 17:26:03, enne wrote:
> > I'm a little curious about this change.  When does this framebuffer rect not
> > have a position of zero? Should this function be taking in a gfx::Size
instead
> > of a rect?
> 
> I'll investigate and consider changing the argument to gfx::Size.

It needs to be a rect since framebuffer_rect is set from the clip rect,
therefore the draw is translated and we need to cancel that out in the
projection matrix.  The problem is that the variable name is misleading -- I
changed it to "target_rect" in all the Renderers.

https://codereview.chromium.org/15579002/diff/44001/cc/trees/layer_tree_host_...
File cc/trees/layer_tree_host_common.cc (left):

https://codereview.chromium.org/15579002/diff/44001/cc/trees/layer_tree_host_...
cc/trees/layer_tree_host_common.cc:1129:
DCHECK_EQ(render_surface_sublayer_scale.x(),
On 2013/06/04 18:02:50, danakj wrote:
> Lost information: I mean that if you set a scale 2x on the root layer, it's
> contents will still appear at 1x on the screen.

I changed my mind on this, I believe this use case will work properly after all
and I don't see why we need to DCHECK that the root layer doesn't draw content. 
root->target_space_transform is set to combined_transform earlier in LTHCommon,
and note that I put the 
"layer_draw_properties.target_space_transform.MakeIdentity();" into the else
clause below.  I added test coverage for the root layer's transform in my
LTHCommon test.

[aelias_OOO_until_Jul13, 2013-06-05 00:03:29]

Adding Antoine for OWNERS review of
content/renderer/gpu/mailbox_output_surface.cc.  To give some background, I
needed to make CC know about the true GL surface size to be able to correctly
flip the Y coordinates of the scissor rect.  This seems best expressed as a
getter on OutputSurface normally tracking the value passed in Reshape (except in
the WebView case).  MailboxOutputSurface needs a change as it overrided Reshape
and stores its own size.

[piman, 2013-06-05 00:52:07]

https://codereview.chromium.org/15579002/diff/69001/cc/output/output_surface.cc
File cc/output/output_surface.cc (right):

https://codereview.chromium.org/15579002/diff/69001/cc/output/output_surface....
cc/output/output_surface.cc:120: if (context3d_)
nit: brackets needed.

https://codereview.chromium.org/15579002/diff/69001/cc/output/output_surface.h
File cc/output/output_surface.h (right):

https://codereview.chromium.org/15579002/diff/69001/cc/output/output_surface....
cc/output/output_surface.h:115: float device_scale_factor_;
nit: needs initializer in the constructors.

https://codereview.chromium.org/15579002/diff/69001/content/renderer/gpu/mail...
File content/renderer/gpu/mailbox_output_surface.cc (right):

https://codereview.chromium.org/15579002/diff/69001/content/renderer/gpu/mail...
content/renderer/gpu/mailbox_output_surface.cc:107: surface_size_ = size;
Also set device_scale_factor_ so that it doesn't get out-of-date.

Not the fault of this CL, but implementation inheritance here is very messy &
fragile. We should have separated out the OutputSurface interface from the
common backed-by-a-context's-default-surface implementation.

[aelias_OOO_until_Jul13, 2013-06-05 00:59:02]

Agreed on the fragility, filed http://crbug.com/246861 to track.

https://codereview.chromium.org/15579002/diff/69001/cc/output/output_surface.h
File cc/output/output_surface.h (right):

https://codereview.chromium.org/15579002/diff/69001/cc/output/output_surface....
cc/output/output_surface.h:115: float device_scale_factor_;
On 2013/06/05 00:52:08, piman wrote:
> nit: needs initializer in the constructors.

Good catch, thanks.

[piman, 2013-06-05 01:05:00]

lgtm

[piman, 2013-06-05 01:05:49]

On 2013/06/05 01:05:00, piman wrote:
> lgtm

(note: I haven't done a full review of this CL, make sure enne/danakj are
happy).

[enne (OOO), 2013-06-05 16:39:06]

https://codereview.chromium.org/15579002/diff/57033/cc/output/direct_renderer.cc
File cc/output/direct_renderer.cc (right):

https://codereview.chromium.org/15579002/diff/57033/cc/output/direct_renderer...
cc/output/direct_renderer.cc:115:
scissor_rect_in_canvas_space.Offset(client_->DeviceViewport().x(),
My only remaining concern here is double-checking that I understand the
relationship between DeviceViewport (externally given or not) and SurfaceSize().
 When I talked to you previously, it sounded like there was no clear
container/subset relationship between these two, i.e. you could have a viewport
larger than the surface or a surface larger than the viewport.

If that's the case, it seems like this offset might be wrong and that maybe the
viewport needs to be the intersection of external viewport and surface size. 
(Or, if I'm wrong, maybe we need a dcheck that one is contained in the other?)

In either case, can you add some comments about what the relationship is between
viewport and surface size to the relevant header?

https://codereview.chromium.org/15579002/diff/57033/cc/output/direct_renderer...
cc/output/direct_renderer.cc:347: if (FlippedFramebuffer())
style nit: {}

[danakj, 2013-06-05 17:40:44]

Everything looks good except the readback stuff, I think. If the origin of the
viewport isn't 0,0 then readbacks seem like they'll get the wrong stuff?

https://codereview.chromium.org/15579002/diff/57033/cc/output/direct_renderer.cc
File cc/output/direct_renderer.cc (right):

https://codereview.chromium.org/15579002/diff/57033/cc/output/direct_renderer...
cc/output/direct_renderer.cc:113: if (!frame->current_texture) {
This check is done twice in this function. Can you make a temp bool with a nice
name so we don't go repeating ourselves like that?

https://codereview.chromium.org/15579002/diff/57033/cc/output/direct_renderer...
cc/output/direct_renderer.cc:115:
scissor_rect_in_canvas_space.Offset(client_->DeviceViewport().x(),
nit: or, scissor_rect_in_canvas_space +=
client_->DeviceViewport().OffsetFromOrigin();

https://codereview.chromium.org/15579002/diff/57033/cc/output/gl_renderer.cc
File cc/output/gl_renderer.cc (right):

https://codereview.chromium.org/15579002/diff/57033/cc/output/gl_renderer.cc#...
cc/output/gl_renderer.cc:2158: 0,
Should this be using the ViewportRect's x/y values? Should we be storing the
current framebuffer as a rect instead of just the size?

Can you add a test with a readback that would have a non-0 ViewportRect?

https://codereview.chromium.org/15579002/diff/57033/cc/output/gl_renderer.cc#...
cc/output/gl_renderer.cc:2186: context_->readPixels(rect.x(),
Should this be offset by the current_framebuffer rect's origin?

https://codereview.chromium.org/15579002/diff/57033/cc/output/software_render...
File cc/output/software_renderer.cc (right):

https://codereview.chromium.org/15579002/diff/57033/cc/output/software_render...
cc/output/software_renderer.cc:446: current_canvas_->readPixels(bitmap.get(), 0,
0);
Should this be offset by the ViewportRect's origin?

Can you add a readback unit test with a non-0 origin for the viewport rect for
this too?

https://codereview.chromium.org/15579002/diff/57033/cc/output/software_render...
cc/output/software_renderer.cc:454: output_device_->CopyToBitmap(rect,
&subset_bitmap);
does this rect need to be offset?

[aelias_OOO_until_Jul13, 2013-06-06 08:57:49]

OK, I made readback work and added "ExpandedViewport" versions of the
RendererPixelTest suites, so now we can run all those tests in a WebView-style
configuration.  They all pass except some 2 alpha-related ones in software that
I disabled for now (some off-by-one problem, maybe fringing).

I ended up renaming MoveScissorToWindowSpace to MoveFromDrawToWindowSpace and
reused the same method for readback rect transformation.  In order to have the
relevant rects available at readback time, I stash them at InitializeMatrices
time (renamed to InitializeViewport).  Overall the code is cleaner and more
centralized now.

https://codereview.chromium.org/15579002/diff/57033/cc/output/direct_renderer.cc
File cc/output/direct_renderer.cc (right):

https://codereview.chromium.org/15579002/diff/57033/cc/output/direct_renderer...
cc/output/direct_renderer.cc:115:
scissor_rect_in_canvas_space.Offset(client_->DeviceViewport().x(),
On 2013/06/05 16:39:06, enne wrote:
> My only remaining concern here is double-checking that I understand the
> relationship between DeviceViewport (externally given or not) and
SurfaceSize().
>  When I talked to you previously, it sounded like there was no clear
> container/subset relationship between these two, i.e. you could have a
viewport
> larger than the surface or a surface larger than the viewport.
> 
> If that's the case, it seems like this offset might be wrong and that maybe
the
> viewport needs to be the intersection of external viewport and surface size. 
> (Or, if I'm wrong, maybe we need a dcheck that one is contained in the other?)
> 
> In either case, can you add some comments about what the relationship is
between
> viewport and surface size to the relevant header?

I added a DCHECK in DirectRenderer::UseRenderPass that surface size contains
device viewport; as it's the clip rect for WebView, it's guaranteed to be
contained.  The one that is not guaranteed to be contained is the viewport size
coming from the main thread (which is not used by the Renderers when
external_viewport_ is set).  I also wrote a comment in renderer.h.

[danakj, 2013-06-06 19:03:12]

https://codereview.chromium.org/15579002/diff/99001/cc/output/direct_renderer.h
File cc/output/direct_renderer.h (right):

https://codereview.chromium.org/15579002/diff/99001/cc/output/direct_renderer...
cc/output/direct_renderer.h:128: // For use in coordinate conversion, this
stores the current render-pass
Perviously all temporary data that only lived for the scope of drawing the frame
would live in the DrawingFrame struct. Should these move there?

[danakj, 2013-06-06 19:06:00]

https://codereview.chromium.org/15579002/diff/99001/cc/output/direct_renderer.h
File cc/output/direct_renderer.h (right):

https://codereview.chromium.org/15579002/diff/99001/cc/output/direct_renderer...
cc/output/direct_renderer.h:128: // For use in coordinate conversion, this
stores the current render-pass
On 2013/06/06 19:03:13, danakj wrote:
> Perviously all temporary data that only lived for the scope of drawing the
frame
> would live in the DrawingFrame struct. Should these move there?

No I guess this is not possible since the readback can happen outside of drawing
a frame and it wants this data. Maybe the comment here is a bit misleading. What
are these values once the frame has finished drawing and there is no "current
render pass"?

[enne (OOO), 2013-06-06 19:07:48]

lgtm.  Thanks for the pixel tests.  :)

https://codereview.chromium.org/15579002/diff/99001/cc/output/direct_renderer.cc
File cc/output/direct_renderer.cc (right):

https://codereview.chromium.org/15579002/diff/99001/cc/output/direct_renderer...
cc/output/direct_renderer.cc:88: DCHECK_LE(viewport_rect.bottom(),
surface_size.height());
DCHECK_GT(viewport_rect.x(), 0)
DCHECK_GT(viewport_rect.y(), 0)

https://codereview.chromium.org/15579002/diff/99001/cc/output/renderer_pixelt...
File cc/output/renderer_pixeltest.cc (right):

https://codereview.chromium.org/15579002/diff/99001/cc/output/renderer_pixelt...
cc/output/renderer_pixeltest.cc:214: // FIXME(aelias): Alpha value off by 1 in
expanded-viewport software renderer.
Why doesn't the fuzzy comparator handle this?

https://codereview.chromium.org/15579002/diff/99001/cc/test/pixel_test.cc
File cc/test/pixel_test.cc (right):

https://codereview.chromium.org/15579002/diff/99001/cc/test/pixel_test.cc#new...
cc/test/pixel_test.cc:195: if (device)
style: {}

[aelias_OOO_until_Jul13, 2013-06-06 19:10:58]

https://codereview.chromium.org/15579002/diff/99001/cc/output/direct_renderer.h
File cc/output/direct_renderer.h (right):

https://codereview.chromium.org/15579002/diff/99001/cc/output/direct_renderer...
cc/output/direct_renderer.h:128: // For use in coordinate conversion, this
stores the current render-pass
On 2013/06/06 19:06:01, danakj wrote:
> On 2013/06/06 19:03:13, danakj wrote:
> > Perviously all temporary data that only lived for the scope of drawing the
> frame
> > would live in the DrawingFrame struct. Should these move there?
> 
> No I guess this is not possible since the readback can happen outside of
drawing
> a frame and it wants this data. Maybe the comment here is a bit misleading.
What
> are these values once the frame has finished drawing and there is no "current
> render pass"?

It ends up being the root render pass since that one's done last, which luckily
is what we want.  I had the same worry, although it was a pre-existing problem
since GLRenderer::current_renderer_size_ used to be used for this purpose.  Let
me know if you have any better ideas for where to put them.

[danakj, 2013-06-06 19:16:44]

LGTM2 with some clarification of that comment

[danakj, 2013-06-06 19:18:13]

On Thu, Jun 6, 2013 at 3:10 PM, <aelias@chromium.org> wrote:

>
> https://codereview.chromium.**org/15579002/diff/99001/cc/**
>
output/direct_renderer.h<https://codereview.chromium.org/15579002/diff/99001/cc/output/direct_renderer.h>
> File cc/output/direct_renderer.h (right):
>
> https://codereview.chromium.**org/15579002/diff/99001/cc/**
>
output/direct_renderer.h#**newcode128<https://codereview.chromium.org/15579002/diff/99001/cc/output/direct_renderer.h#newcode128>
> cc/output/direct_renderer.h:**128: // For use in coordinate conversion,
> this stores the current render-pass
> On 2013/06/06 19:06:01, danakj wrote:
>
>> On 2013/06/06 19:03:13, danakj wrote:
>> > Perviously all temporary data that only lived for the scope of
>>
> drawing the
>
>> frame
>> > would live in the DrawingFrame struct. Should these move there?
>>
>
>  No I guess this is not possible since the readback can happen outside
>>
> of drawing
>
>> a frame and it wants this data. Maybe the comment here is a bit
>>
> misleading. What
>
>> are these values once the frame has finished drawing and there is no
>>
> "current
>
>> render pass"?
>>
>
> It ends up being the root render pass since that one's done last, which
> luckily is what we want.  I had the same worry, although it was a
> pre-existing problem since GLRenderer::current_renderer_**size_ used to be
> used for this purpose.  Let me know if you have any better ideas for
> where to put them.
>

I'll move them into the DrawingFrame when all readbacks are done via draw
and not after. I'd just like the comment to be more precise.


>
>
https://codereview.chromium.**org/15579002/<https://codereview.chromium.org/1...
>

[aelias_OOO_until_Jul13, 2013-06-06 22:24:50]

OK, all nits addressed.  I'll land when the trybots go green.  Thanks for all
the quick reviews!

https://codereview.chromium.org/15579002/diff/99001/cc/output/renderer_pixelt...
File cc/output/renderer_pixeltest.cc (right):

https://codereview.chromium.org/15579002/diff/99001/cc/output/renderer_pixelt...
cc/output/renderer_pixeltest.cc:214: // FIXME(aelias): Alpha value off by 1 in
expanded-viewport software renderer.
On 2013/06/06 19:07:49, enne wrote:
> Why doesn't the fuzzy comparator handle this?

Ah, it was running the exact comparator because it was specializing for the
exact template type.  I activated the fuzzy comparator and reenabled these
tests, thanks.

[commit-bot: I haz the power, 2013-06-06 23:18:28]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/aelias@chromium.org/15579002/115001

[commit-bot: I haz the power, 2013-06-06 23:23:36]

Change committed as 204650

[piman, 2013-06-10 22:41:42]

https://chromiumcodereview.appspot.com/15579002/diff/115001/cc/output/direct_...
File cc/output/direct_renderer.cc (right):

https://chromiumcodereview.appspot.com/15579002/diff/115001/cc/output/direct_...
cc/output/direct_renderer.cc:214: client_->DeviceScaleFactor());
Note: this change triggered crbug.com/248213 because we now resize the back
buffer before we recreate it (e.g. after a tab was hidden).
https://codereview.chromium.org/16431008/ fixes the bug on
TextureImageTransportSurface (we don't want to rely on client-side behavior for
correctness), but I'm not sure if there could be subtle issues on other
platforms.

[aelias_OOO_until_Jul13, 2013-06-10 22:49:09]

https://chromiumcodereview.appspot.com/15579002/diff/115001/cc/output/direct_...
File cc/output/direct_renderer.cc (right):

https://chromiumcodereview.appspot.com/15579002/diff/115001/cc/output/direct_...
cc/output/direct_renderer.cc:214: client_->DeviceScaleFactor());
Sorry about that.  This is probably due to the ordering change relative to
EnsureBackbuffer.  We could make EnsureBackbuffer virtual and call it from
DirectRenderer.