Avoid redundantly setting tex parameters

Avoid redundantly setting tex parameters

BUG=


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

[jamesr, 2012-11-12 18:12:17]

This is a little different from the other client-side state caching since this
is state on a texture object, not the context itself.  I doubt the command
buffer code will be able to avoid this call since it probably can't know if a
bindTexture succeeds or not on the client side.

Any suggestions for testing? I'm thinking of adding something to
resource_provider_unittest to verify that we call texParameteri() 4 times on
each texture we pass through

[enne (OOO), 2012-11-12 18:27:33]

Does this really need to happen for createFromExternalTextureId? Should the
caller be allowed/required to set the parameters that it desires?
(Canvas2DLayerBridge already does this.)

Re: testing.  That seems a little bit much like testing the implementation. 
Isn't it really more about verifying that we don't call it extra times as much
as it is that we call it at least once? I was picturing a drawing test with a
context that verified that these parameters got set exactly once?

[jamesr, 2012-11-12 19:23:45]

On 2012/11/12 18:27:33, enne wrote:
> Does this really need to happen for createFromExternalTextureId? Should the
> caller be allowed/required to set the parameters that it desires?
> (Canvas2DLayerBridge already does this.)

We get textures from lots of sources (pepper, h/w video decode paths, etc) that
are a bit harder to audit.  Seems safer to make one call when we adopt the
resource in to the compositor.

> Re: testing.  That seems a little bit much like testing the implementation. 
> Isn't it really more about verifying that we don't call it extra times as much
> as it is that we call it at least once? I was picturing a drawing test with a
> context that verified that these parameters got set exactly once?

We could also just yell at anyone who adds texParameteri calls to gl_renderer. 
I think the closest analogue (in terms of verifying that the draw stream doesn't
have calls that we think are expensive) would be
GLRendererTest2.initializationDoesNotMakeSynchronousCalls which IMO hasn't been
all that valuable.  In an ideal world our correctness tests would make sure that
we aren't missing any calls (since they would generate GL errors) and perf tests
would make sure we don't have unnecessary calls.

[enne (OOO), 2012-11-12 19:46:23]

On 2012/11/12 19:23:45, jamesr wrote:
> On 2012/11/12 18:27:33, enne wrote:
> > Does this really need to happen for createFromExternalTextureId? Should the
> > caller be allowed/required to set the parameters that it desires?
> > (Canvas2DLayerBridge already does this.)
> 
> We get textures from lots of sources (pepper, h/w video decode paths, etc)
that
> are a bit harder to audit.  Seems safer to make one call when we adopt the
> resource in to the compositor.

I guess I'm just not sure that everything wants the same clamp and filtering
settings.  It's probably a moot point.  We can go with this at first and change
it later if somebody needs flexibility later.

> We could also just yell at anyone who adds texParameteri calls to gl_renderer.


I like that idea a lot.

[enne (OOO), 2012-11-12 22:30:22]

Also, what are you going to do about tiled_layer_impl setting texture filter on
quads? Should this go away?

I will also point out that you will almost certainly break
compositing/checkerboard.html with this change.  (In fact, linux_layout_rel
backs me up here.) In the past various folks have bounced off trying to make
this work properly.  I personally think that we should be able to draw an
integer-translated image on the root layer pixel-for-pixel, but am not sure what
we need to do to make this happen.

(See also checkerboard test mention here:
http://codereview.chromium.org/11360205/)

[jamesr, 2012-11-12 22:35:33]

On 2012/11/12 22:30:22, enne wrote:
> Also, what are you going to do about tiled_layer_impl setting texture filter
on
> quads? Should this go away?

Oh! I just broke that.  Oops.  Currently it can't change since hasBorderTexels
blows away all tiles if it ever changes.  I could make it a construction-time
property of the resource and set it there.

[jamesr, 2012-11-13 01:46:06]

PTAL.  This will break compositing/checkerboard.html in the way we discussed,
but that's OK.  After this lands I'll rebaseline in WebKit and remove the
test_expectations.txt line.

I've added tests that we don't call texParameteri during the draw itself.  I
tried adding resource_provider tests that we set parameters as expected, but it
ended up being a big pain.  The way GL works is you can (and we do) set texture
parameters on an ID before the object is allocated, so I can't (trivally) reuse
the texture tracking in resource_provider_unittest.  I did verify that we never
create resources for GL_OES_EXTERNAL_TEXTURE, so at least that's good.

[enne (OOO), 2012-11-13 17:59:11]

lgtm except for the GL_OES_EXTERNAL_TEXTURE bit.  Can you walk me through why we
don't create resources for GL_OES_EXTERNAL_TEXTURE? It seems to me like we would
go down the willDraw => allocatePlaneData => allocateData =>
createResourceFromExternalTexture path for those frames.

[jamesr, 2012-11-13 18:08:34]

On 2012/11/13 17:59:11, enne wrote:
> lgtm except for the GL_OES_EXTERNAL_TEXTURE bit.  Can you walk me through why
we
> don't create resources for GL_OES_EXTERNAL_TEXTURE? It seems to me like we
would
> go down the willDraw => allocatePlaneData => allocateData =>
> createResourceFromExternalTexture path for those frames.

When m_format == GL_OES_EXTERNAL_TEXTURE, there aren't any planes so
allocatePlaneData/etc are no-ops.  We just stick the m_frame->texture_id()
directly in the quad itself.  
The end of VideoLayerImpl::willDrawInternal() has this:

    if (m_format == GL_TEXTURE_2D)
        m_externalTextureResource =
resourceProvider->createResourceFromExternalTexture(m_frame->texture_id());

[enne (OOO), 2012-11-13 18:34:15]

Oh, thanks, I missed the part where there are zero planes.  lgtm for real.

[commit-bot: I haz the power, 2012-11-13 19:37:53]

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

[commit-bot: I haz the power, 2012-11-13 19:59:21]

Sorry for I got bad news for ya.
Compile failed with a clobber build on linux_chromeos.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...
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-11-13 20:00:18]

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

[commit-bot: I haz the power, 2012-11-13 21:48:49]

Change committed as 167479