Allow using a larger-than-necessary texture as cached render pass backing

Allow using a larger-than-necessary texture as cached render pass backing

When moving a composited layer around the screen that requires a render pass,
it's not all that unusual for the required size to be slightly different from
frame to frame. This lets us use an oversized texture as the framebuffer
attachment.

BUG=161868
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=173112

[jamesr, 2012-11-20 00:09:06]

This provides a decent win on the test case mentioned in the linked bug - since
we're using the same fbo attachment on each frame, the service side doesn't need
to do nearly as many clear/checkFramebufferStatus calls.

[enne (OOO), 2012-11-20 00:20:15]

lgtm.  I looked through the quad drawing code, and I think this should be ok.

I know we don't have any automated testing, but for a manual test you could try
having DirectRenderer::useRenderPass create a too large texture when allocating
and then running some layout tests with render surfaces to see what if anything
goes askew.

[danakj, 2012-11-20 00:26:33]

http://codereview.chromium.org/11420079/diff/1/cc/direct_renderer.cc
File cc/direct_renderer.cc (right):

http://codereview.chromium.org/11420079/diff/1/cc/direct_renderer.cc#newcode140
cc/direct_renderer.cc:140: if (texture->id() && (!sizeAppropriate ||
texture->format() != requiredFormat))
I'm worried about the texture cacheing code. It makes assumptions that if the
texture size changes, then it knows to redraw the texture, and I'm not certain
if this change will make it not redraw the texture.

I am hoping we have tests that cover this.

[danakj, 2012-11-20 00:36:41]

http://codereview.chromium.org/11420079/diff/1/cc/direct_renderer.cc
File cc/direct_renderer.cc (right):

http://codereview.chromium.org/11420079/diff/1/cc/direct_renderer.cc#newcode140
cc/direct_renderer.cc:140: if (texture->id() && (!sizeAppropriate ||
texture->format() != requiredFormat))
Is it true that in order for the size of the render pass to change, that some
property must have caused damage to the renderPass, and so
contentsChangedSinceLastFrame() will be non-empty?

If that's the case then I think this is fine, and I can't imagine a good test
for it since the case can't happen.

[enne (OOO), 2012-11-20 00:39:45]

http://codereview.chromium.org/11420079/diff/1/cc/direct_renderer.cc
File cc/direct_renderer.cc (right):

http://codereview.chromium.org/11420079/diff/1/cc/direct_renderer.cc#newcode140
cc/direct_renderer.cc:140: if (texture->id() && (!sizeAppropriate ||
texture->format() != requiredFormat))
On 2012/11/20 00:36:41, danakj wrote:
> Is it true that in order for the size of the render pass to change, that some
> property must have caused damage to the renderPass, and so
> contentsChangedSinceLastFrame() will be non-empty?
> 
> If that's the case then I think this is fine, and I can't imagine a good test
> for it since the case can't happen.

Yeah, I think contentsChangedSinceLastFrame will correctly get this redrawn
without reallocating the texture.

In terms of testing, I think this is another case where a cc pixel test is
really needed.

[danakj, 2012-11-20 00:41:36]

http://codereview.chromium.org/11420079/diff/1/cc/direct_renderer.cc
File cc/direct_renderer.cc (right):

http://codereview.chromium.org/11420079/diff/1/cc/direct_renderer.cc#newcode140
cc/direct_renderer.cc:140: if (texture->id() && (!sizeAppropriate ||
texture->format() != requiredFormat))
On 2012/11/20 00:39:45, enne wrote:
> On 2012/11/20 00:36:41, danakj wrote:
> > Is it true that in order for the size of the render pass to change, that
some
> > property must have caused damage to the renderPass, and so
> > contentsChangedSinceLastFrame() will be non-empty?
> > 
> > If that's the case then I think this is fine, and I can't imagine a good
test
> > for it since the case can't happen.
> 
> Yeah, I think contentsChangedSinceLastFrame will correctly get this redrawn
> without reallocating the texture.
> 
> In terms of testing, I think this is another case where a cc pixel test is
> really needed.

Ya, Okay I've convinced myself that we'll be alright. If the surface's
contentRect() changes, then its surfacePropertyHasChanged = true. Then the
damageTracker sets damage to the entire surface bounds. So we win. LGTM.

[jamesr, 2012-11-20 00:49:15]

I've convinced myself it's correct as well, but if I run the layout tests with
this:

diff --git a/cc/direct_renderer.cc b/cc/direct_renderer.cc
index aba5614..95ca8a6 100644
--- a/cc/direct_renderer.cc
+++ b/cc/direct_renderer.cc
@@ -215,7 +215,9 @@ bool DirectRenderer::useRenderPass(DrawingFrame& frame,
const RenderPass* render
 
     CachedResource* texture = m_renderPassTextures.get(renderPass->id());
     DCHECK(texture);
-    if (!texture->id() && !texture->Allocate(Renderer::ImplPool,
renderPassTextureSize(renderPass), renderPassTextureFormat(renderPass),
ResourceProvider::TextureUsageFramebuffer))
+    gfx::Size big_size = renderPassTextureSize(renderPass);
+    big_size.Enlarge(10, 20);
+    if (!texture->id() && !texture->Allocate(Renderer::ImplPool, big_size,
renderPassTextureFormat(renderPass), ResourceProvider::TextureUsageFramebuffer))
         return false;
 
     return bindFramebufferToTexture(frame, texture, renderPass->outputRect());


I get a lot of pixel failures that look related.  Investigating...

[jamesr, 2012-11-20 02:30:33]

On 2012/11/20 00:20:15, enne wrote:
> lgtm.  I looked through the quad drawing code, and I think this should be ok.
> 
> I know we don't have any automated testing, but for a manual test you could
try
> having DirectRenderer::useRenderPass create a too large texture when
allocating
> and then running some layout tests with render surfaces to see what if
anything
> goes askew.

I think when we bind the render pass' texture to draw from we will need to
adjust the UVs since currently we assume the rect (0,0) -> (1,1) exactly matches
the render pass' size.  I'm pretty sure I can't fix this before going on
vacation but if anyone else wants to take over this patch feel free. Otherwise
I'll get at it when I get back.

[jamesr, 2012-12-12 22:34:47]

This patch seems to works.  It includes the patch from
https://codereview.chromium.org/11543013/ - once we decide if that's good or bad
I'll either back those changes out or post a diff against that branch.

[enne (OOO), 2012-12-12 23:41:54]

I like the mask uv RectF change.  That's much nicer too.  :)

https://codereview.chromium.org/11420079/diff/12004/cc/gl_renderer.cc
File cc/gl_renderer.cc (right):

https://codereview.chromium.org/11420079/diff/12004/cc/gl_renderer.cc#newcode634
cc/gl_renderer.cc:634: float tex_scale_y = quad->rect.height() /
static_cast<float>(contentsTexture->size().height());
Can you assert here that scale_foo is <= 1?

https://codereview.chromium.org/11420079/diff/12004/cc/gl_renderer.cc#newcode...
cc/gl_renderer.cc:1255: GLC(context(),
context()->uniform4f(program->vertexShader().texTransformLocation(),
It's kind of scary that this wasn't set before.

https://codereview.chromium.org/11420079/diff/12004/cc/gl_renderer_pixeltest.cc
File cc/gl_renderer_pixeltest.cc (right):

https://codereview.chromium.org/11420079/diff/12004/cc/gl_renderer_pixeltest....
cc/gl_renderer_pixeltest.cc:113: 0,     // mask_tex_coord_scale_x
I don't think this compiles.

https://codereview.chromium.org/11420079/diff/12004/cc/test/pixel_test_utils.cc
File cc/test/pixel_test_utils.cc (right):

https://codereview.chromium.org/11420079/diff/12004/cc/test/pixel_test_utils....
cc/test/pixel_test_utils.cc:42: WritePNGFile(gen_bmp, ref_img_path);
Is this intentional? (Also, the other change in this file, commented out?)

[jamesr, 2012-12-13 01:15:06]

https://codereview.chromium.org/11420079/diff/12004/cc/gl_renderer.cc
File cc/gl_renderer.cc (right):

https://codereview.chromium.org/11420079/diff/12004/cc/gl_renderer.cc#newcode634
cc/gl_renderer.cc:634: float tex_scale_y = quad->rect.height() /
static_cast<float>(contentsTexture->size().height());
On 2012/12/12 23:41:54, enne wrote:
> Can you assert here that scale_foo is <= 1?

Sure

https://codereview.chromium.org/11420079/diff/12004/cc/gl_renderer.cc#newcode...
cc/gl_renderer.cc:1255: GLC(context(),
context()->uniform4f(program->vertexShader().texTransformLocation(),
On 2012/12/12 23:41:54, enne wrote:
> It's kind of scary that this wasn't set before.

Not as scary as it seems - I'm adding this vertex uniform in this patch :)

https://codereview.chromium.org/11420079/diff/12004/cc/gl_renderer_pixeltest.cc
File cc/gl_renderer_pixeltest.cc (right):

https://codereview.chromium.org/11420079/diff/12004/cc/gl_renderer_pixeltest....
cc/gl_renderer_pixeltest.cc:113: 0,     // mask_tex_coord_scale_x
On 2012/12/12 23:41:54, enne wrote:
> I don't think this compiles.

Trybots agree!  I think I'll rework this test anyway to do one render of the
scene with a forced-oversized contents texture

https://codereview.chromium.org/11420079/diff/12004/cc/test/pixel_test_utils.cc
File cc/test/pixel_test_utils.cc (right):

https://codereview.chromium.org/11420079/diff/12004/cc/test/pixel_test_utils....
cc/test/pixel_test_utils.cc:42: WritePNGFile(gen_bmp, ref_img_path);
On 2012/12/12 23:41:54, enne wrote:
> Is this intentional? (Also, the other change in this file, commented out?)

no (although it's useful when debugging). removed

[jamesr, 2012-12-13 01:50:42]

This patch is meant to be on top of https://codereview.chromium.org/11543013/
(which is in the CQ) but otherwise is good to go.  Few comments:

1.) This allows any texture >= the pass size to be used.  This will use more
memory than strictly necessary in some cases, but given how rare passes are in
general and the fact that we drop the texture completely on the first frame that
doesn't have the pass I think this will be OK.
2.) I've added a somewhat odd enlargement factor to DirectRenderer for pass
textures and use that in the test.  It might be useful to let higher levels
(like LayerTreeHostImpl) decide pass texture sizes, but that'd currently be very
awkward since allocations happen implicitly during the draw pass that
DirectRenderer is responsible for.
3.) I'll have to check in cc/test/data/blue_yellow.png manually before landing
since the CQ can't handle binary files, but it's a 200x200 png with the left
half blue and the right half yellow.

[danakj, 2012-12-13 20:39:50]

I like this, LGTM. All the helpers functions in the pixeltests are awesome.

One nit about using FooForTesting() for methods only meant to be used in tests.
And I think enne@ should review the gl_renderer changes if they haven't already
\noideadog.

https://codereview.chromium.org/11420079/diff/12005/cc/direct_renderer.h
File cc/direct_renderer.h (right):

https://codereview.chromium.org/11420079/diff/12005/cc/direct_renderer.h#newc...
cc/direct_renderer.h:47: void setEnlargePassTextureAmount(gfx::Vector2d amount);
You can use ...ForTesting() suffix for compiler checks that we don't use this in
non-test code.

https://codereview.chromium.org/11420079/diff/12005/cc/gl_renderer.cc
File cc/gl_renderer.cc (right):

https://codereview.chromium.org/11420079/diff/12005/cc/gl_renderer.cc#newcode669
cc/gl_renderer.cc:669: 
extra \n

https://codereview.chromium.org/11420079/diff/12005/cc/gl_renderer_pixeltest.cc
File cc/gl_renderer_pixeltest.cc (right):

https://codereview.chromium.org/11420079/diff/12005/cc/gl_renderer_pixeltest....
cc/gl_renderer_pixeltest.cc:119: #if !defined(OS_ANDROID)
just curious why this? did the build there not get fixed yet?

[jamesr, 2012-12-13 20:48:34]

On 2012/12/13 20:39:50, danakj wrote:
> I like this, LGTM. All the helpers functions in the pixeltests are awesome.
> 
> One nit about using FooForTesting() for methods only meant to be used in
tests.
> And I think enne@ should review the gl_renderer changes if they haven't
already
> \noideadog.
> 
> https://codereview.chromium.org/11420079/diff/12005/cc/direct_renderer.h
> File cc/direct_renderer.h (right):
> 
>
https://codereview.chromium.org/11420079/diff/12005/cc/direct_renderer.h#newc...
> cc/direct_renderer.h:47: void setEnlargePassTextureAmount(gfx::Vector2d
amount);
> You can use ...ForTesting() suffix for compiler checks that we don't use this
in
> non-test code.

I like that, will do.

>
https://codereview.chromium.org/11420079/diff/12005/cc/gl_renderer_pixeltest....
> cc/gl_renderer_pixeltest.cc:119: #if !defined(OS_ANDROID)
> just curious why this? did the build there not get fixed yet?

Two reasons:
1.) Currently osmesa isn't set up to build on android, and even if it were it'd
probably be too slow to be useful.  If we use real GL drivers we'd be
susceptible to all sorts of tiny deltas due to different intermediate
resolutions and rounding.
2.) We don't have scripts set up to copy reference pngs to the device when
running cc_unittests (we do for cc_perftests for the layer trees, but those
don't run on the test bots).  This is easier to fix but I'd rather not add the
overhead to all test runs unless/until we need it.

Since these tests should behave the same everywhere, I don't think losing
coverage on android devices is a big deal.

[enne (OOO), 2012-12-13 20:48:45]

gl_renderer changes lgtm.

[commit-bot: I haz the power, 2012-12-13 21:27:19]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jamesr@chromium.org/11420079/23017

[commit-bot: I haz the power, 2012-12-13 23:47:04]

Sorry for I got bad news for ya.
Compile failed with a clobber build on win_rel.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
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-12-13 23:51:05]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jamesr@chromium.org/11420079/32003

[commit-bot: I haz the power, 2012-12-14 07:13:50]

Change committed as 173112

[Nico, 2013-11-09 06:51:53]

https://chromiumcodereview.appspot.com/11420079/diff/32003/cc/gl_renderer_pix...
File cc/gl_renderer_pixeltest.cc (right):

https://chromiumcodereview.appspot.com/11420079/diff/32003/cc/gl_renderer_pix...
cc/gl_renderer_pixeltest.cc:119: #if !defined(OS_ANDROID)
Is there a tracking bug for enabling this on android? (I realize this isn't the
CL that added this, but I got lost somewhere along the way while trying to trace
the origin of this, and it looks like this file was mostly touched by enne,
who's ccd on this review)

[jamesr, 2013-11-09 07:24:33]

Someone is working on enabling pixel tests on Android, yes. Eric or Daniel
should know the status.

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Daniel Sievers (Google), 2013-11-11 17:56:35]

https://codereview.chromium.org/23868030/ adds support for OSMesa on
Android. I can give it a try with that patch applied and see how things
work.


On Fri, Nov 8, 2013 at 11:24 PM, James Robinson <jamesr@chromium.org> wrote:

> Someone is working on enabling pixel tests on Android, yes. Eric or Daniel
> should know the status.
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Daniel Sievers (Google), 2013-11-11 20:17:22]

Looks promising. I can run and pass all cc pixel tests on N4 that run on
Linux after removing the ifdefs (123 tests with "*PixelTest*" filter).
Needs Peter's patch plus some gyp modifications and isolate file (so that
cc/tests/data is copied).


On Mon, Nov 11, 2013 at 9:56 AM, Daniel Sievers <sievers@google.com> wrote:

> https://codereview.chromium.org/23868030/ adds support for OSMesa on
> Android. I can give it a try with that patch applied and see how things
> work.
>
>
> On Fri, Nov 8, 2013 at 11:24 PM, James Robinson <jamesr@chromium.org>wrote:
>
>> Someone is working on enabling pixel tests on Android, yes. Eric or
>> Daniel should know the status.
>>
>
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Nico, 2013-11-17 08:20:48]

On Sat, Nov 9, 2013 at 6:51 AM, <thakis@chromium.org> wrote:

>
> https://chromiumcodereview.appspot.com/11420079/diff/32003/cc/gl_renderer_
> pixeltest.cc
> File cc/gl_renderer_pixeltest.cc (right):
>
> https://chromiumcodereview.appspot.com/11420079/diff/32003/cc/gl_renderer_
> pixeltest.cc#newcode119
> cc/gl_renderer_pixeltest.cc:119: #if !defined(OS_ANDROID)
> Is there a tracking bug for enabling this on android? (I realize this
> isn't the CL that added this, but I got lost somewhere along the way
> while trying to trace the origin of this, and it looks like this file
> was mostly touched by enne, who's ccd on this review)
>

enne: ^


>
> https://chromiumcodereview.appspot.com/11420079/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.