Adding bindless variants mailbox produce/consume

Adding bindless variants mailbox produce/consume

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

[bajones, 2014-05-22 20:54:21]

Okay, this needs some cleanup but the basic implementation works quite nicely.
Cuts the canvas2d->texture regressions in half. Looking for some feedback at
this point:

- Function name suggestions? produceTextureBindlessCHROMIUM and
createAndConsumeTextureCHROMIUM feel clunky, but I don't have any better ideas.
- Command buffer advice? My changes to build_gles2_cmd_buffer.py feel ugly.
- Any other general feedback?

I need to also add some tests before it's commit worthy.

[danakj, 2014-05-22 21:29:03]

This probably needs to be updated too before landing:
https://code.google.com/p/chromium/codesearch#chromium/src/gpu/GLES2/extensio...

[bajones, 2014-05-22 21:33:24]

On 2014/05/22 21:29:03, danakj wrote:
> This probably needs to be updated too before landing:
>
https://code.google.com/p/chromium/codesearch#chromium/src/gpu/GLES2/extensio...

Good catch, thanks! I'll make sure to update that when we know what the
functions are actually going to be called.

[piman, 2014-05-22 21:53:57]

I can haz tests?

https://codereview.chromium.org/299043003/diff/1/content/common/gpu/client/we...
File content/common/gpu/client/webgraphicscontext3d_command_buffer_impl.h
(right):

https://codereview.chromium.org/299043003/diff/1/content/common/gpu/client/we...
content/common/gpu/client/webgraphicscontext3d_command_buffer_impl.h:600:
virtual void produceTextureBindlessCHROMIUM(WebGLId texture, WGC3Denum target,
nit: you'll also need these in WebGraphicsContext3DInProcessCommandBufferImpl

https://codereview.chromium.org/299043003/diff/1/gpu/GLES2/gl2extchromium.h
File gpu/GLES2/gl2extchromium.h (right):

https://codereview.chromium.org/299043003/diff/1/gpu/GLES2/gl2extchromium.h#n...
gpu/GLES2/gl2extchromium.h:59: GLuint texture, GLenum target, const GLbyte*
mailbox);
Nit: the GL-y way is to have target before texture.

https://codereview.chromium.org/299043003/diff/1/gpu/command_buffer/build_gle...
File gpu/command_buffer/build_gles2_cmd_buffer.py (right):

https://codereview.chromium.org/299043003/diff/1/gpu/command_buffer/build_gle...
gpu/command_buffer/build_gles2_cmd_buffer.py:1389: 'type': 'CreateAndConsume',
I'm not convinced it's worth creating a new type just for one function,
especially if there's still manual implementations anyway.

https://codereview.chromium.org/299043003/diff/1/gpu/command_buffer/service/g...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

https://codereview.chromium.org/299043003/diff/1/gpu/command_buffer/service/g...
gpu/command_buffer/service/gles2_cmd_decoder.cc:10223: Texture* produced =
texture_manager()->Produce(texture_ref);
You should check that target == produced->target(), otherwise bad things would
happen.

[bajones, 2014-05-22 22:07:10]

On 2014/05/22 21:53:57, piman wrote:
> I can haz tests?

You can haz. :) As I mentioned in my first comment, this needs tests before it's
commit worthy. What's been put up here is basically proof-of-concept code for
the sake of feedback.

> Nit: the GL-y way is to have target before texture.

Yeah, I waffled on this one. The key for me is that this is emulating Direct
State Access (http://www.opengl.org/registry/specs/EXT/direct_state_access.txt),
and those functions have their own pattern of always passing the object being
modified first. Case in point:

"glTextureImage2D(texobj, GL_TEXTURE_2D, 0, GL_RGB8, width, height, GL_RGB,
GL_FLOAT, data);"

That said, I have no real preference either way. Happy to switch it if you feel
strongly about it.

> gpu/command_buffer/build_gles2_cmd_buffer.py:1389: 'type': 'CreateAndConsume',
> I'm not convinced it's worth creating a new type just for one function,
> especially if there's still manual implementations anyway.

Agreed, but nothing else seemed to match what I needed. (At least that I can
see. Readability of this file is a whole different issue.) I do feel like now
that I've gotten it working I could probably do a handwritten version, though,
which is probably cleaner overall.

> gpu/command_buffer/service/gles2_cmd_decoder.cc:10223: Texture* produced =
> texture_manager()->Produce(texture_ref);
> You should check that target == produced->target(), otherwise bad things would
> happen.

Good call. I'll add that in the next revision.

[Ken Russell (switch to Gerrit), 2014-05-23 04:04:40]

Looks good though I haven't made it through the entire CL yet. Two minor
comments.

https://codereview.chromium.org/299043003/diff/1/gpu/GLES2/gl2extchromium.h
File gpu/GLES2/gl2extchromium.h (right):

https://codereview.chromium.org/299043003/diff/1/gpu/GLES2/gl2extchromium.h#n...
gpu/GLES2/gl2extchromium.h:58: GL_APICALL void GL_APIENTRY
glProduceTextureBindlessCHROMIUM(
I think the term "bindless" might cause confusion with the OpenGL concepts of
bindless textures. Could we choose another name? ProduceFromTextureCHROMIUM?
Something else? I think ideally the old entry point would be removed once all
call sites are switched to the new one. Perhaps it could just be temporarily
called glProduceTextureNewCHROMIUM or glProduceTexture2CHROMIUM. Then everyone
could be switched over to the new one; the old one could be replaced with the
new signature; everyone switched back; and the new temporary one deleted.

https://codereview.chromium.org/299043003/diff/1/gpu/GLES2/gl2extchromium.h#n...
gpu/GLES2/gl2extchromium.h:69: GLenum target, const GLbyte* mailbox);
The signature of the function pointer doesn't match the prototype above.

[bajones, 2014-06-02 17:07:46]

Addressed feedback from above, please take another look!

[Ken Russell (switch to Gerrit), 2014-06-02 22:46:53]

Very nice work. LGTM

https://codereview.chromium.org/299043003/diff/20001/gpu/GLES2/extensions/CHR...
File gpu/GLES2/extensions/CHROMIUM/CHROMIUM_texture_mailbox.txt (right):

https://codereview.chromium.org/299043003/diff/20001/gpu/GLES2/extensions/CHR...
gpu/GLES2/extensions/CHROMIUM/CHROMIUM_texture_mailbox.txt:114: Returns a new
texture name pointing to the texture object associated with
Consider expanding a little: "...new texture name associated with the given
target..."

https://codereview.chromium.org/299043003/diff/20001/gpu/command_buffer/commo...
File gpu/command_buffer/common/gles2_cmd_format_test_autogen.h (right):

https://codereview.chromium.org/299043003/diff/20001/gpu/command_buffer/commo...
gpu/command_buffer/common/gles2_cmd_format_test_autogen.h:3189: // TODO(gman):
Write test for CreateAndConsumeTextureCHROMIUMImmediate
It's poor that the autogenerated TODOs are assigned to someone no longer on the
team. Please file a bug about updating the scripts and regenerating all of the
autogenerated code.

https://codereview.chromium.org/299043003/diff/20001/gpu/command_buffer/servi...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

https://codereview.chromium.org/299043003/diff/20001/gpu/command_buffer/servi...
gpu/command_buffer/service/gles2_cmd_decoder.cc:10228: if (produced->target() !=
target) {
Is this new code path tested?

Do any other side-effects against the produced texture need to be undone in this
case?

https://codereview.chromium.org/299043003/diff/20001/gpu/command_buffer/servi...
File gpu/command_buffer/service/gles2_cmd_decoder_unittest_textures.cc (right):

https://codereview.chromium.org/299043003/diff/20001/gpu/command_buffer/servi...
gpu/command_buffer/service/gles2_cmd_decoder_unittest_textures.cc:1941:
GL_TEXTURE_2D, 1, GL_RGBA, 2, 4, 0, GL_RGBA, GL_UNSIGNED_BYTE, 0, 0);
Might be worth trying to use a different type and internal format for the second
texture to ensure that those are maintained properly during mailbox production.

[bajones, 2014-06-02 23:31:23]

Thanks for the feedback!

https://codereview.chromium.org/299043003/diff/20001/gpu/GLES2/extensions/CHR...
File gpu/GLES2/extensions/CHROMIUM/CHROMIUM_texture_mailbox.txt (right):

https://codereview.chromium.org/299043003/diff/20001/gpu/GLES2/extensions/CHR...
gpu/GLES2/extensions/CHROMIUM/CHROMIUM_texture_mailbox.txt:114: Returns a new
texture name pointing to the texture object associated with
On 2014/06/02 22:46:54, Ken Russell wrote:
> Consider expanding a little: "...new texture name associated with the given
> target..."

Done. Update coming in soon.

https://codereview.chromium.org/299043003/diff/20001/gpu/command_buffer/commo...
File gpu/command_buffer/common/gles2_cmd_format_test_autogen.h (right):

https://codereview.chromium.org/299043003/diff/20001/gpu/command_buffer/commo...
gpu/command_buffer/common/gles2_cmd_format_test_autogen.h:3189: // TODO(gman):
Write test for CreateAndConsumeTextureCHROMIUMImmediate
On 2014/06/02 22:46:54, Ken Russell wrote:
> It's poor that the autogenerated TODOs are assigned to someone no longer on
the
> team. Please file a bug about updating the scripts and regenerating all of the
> autogenerated code.

Good point. crbug.com/379951 :)

https://codereview.chromium.org/299043003/diff/20001/gpu/command_buffer/servi...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

https://codereview.chromium.org/299043003/diff/20001/gpu/command_buffer/servi...
gpu/command_buffer/service/gles2_cmd_decoder.cc:10228: if (produced->target() !=
target) {
On 2014/06/02 22:46:54, Ken Russell wrote:
> Is this new code path tested?

Informally. I'll add a unit test in the next CL revision.

> Do any other side-effects against the produced texture need to be undone in
this
> case?

texture_manager()->Produce(texture_ref); just returns the texture object
associated with texture_ref, it doesn't alter it at all. No side effects here.

https://codereview.chromium.org/299043003/diff/20001/gpu/command_buffer/servi...
File gpu/command_buffer/service/gles2_cmd_decoder_unittest_textures.cc (right):

https://codereview.chromium.org/299043003/diff/20001/gpu/command_buffer/servi...
gpu/command_buffer/service/gles2_cmd_decoder_unittest_textures.cc:1941:
GL_TEXTURE_2D, 1, GL_RGBA, 2, 4, 0, GL_RGBA, GL_UNSIGNED_BYTE, 0, 0);
On 2014/06/02 22:46:54, Ken Russell wrote:
> Might be worth trying to use a different type and internal format for the
second
> texture to ensure that those are maintained properly during mailbox
production.

Tests are fairly sparse for these APIs all around, not just the new ones. Maybe
it would be better to add a new issue centered around just improving test
coverage for the existing produce/consume calls?

[vmiura, 2014-06-03 00:00:50]

Just one issue.

https://codereview.chromium.org/299043003/diff/20001/gpu/command_buffer/clien...
File gpu/command_buffer/client/gles2_implementation.cc (right):

https://codereview.chromium.org/299043003/diff/20001/gpu/command_buffer/clien...
gpu/command_buffer/client/gles2_implementation.cc:3641: client_id, data);
I think we need the following Flush() here to ensure ids are synchronized across
share groups.  See GLES2Implementation::GenTextures.

  if (share_group_->bind_generates_resource())
    helper_->CommandBufferHelper::Flush();

[piman, 2014-06-04 11:09:44]

Implementation looks fine % vmiura's comment

As far as tests, you can also add something to GLTextureMailboxTest (integration
test). They're super easy to write and also demonstrate use cases.

[bajones, 2014-06-04 22:49:53]

On 2014/06/04 11:09:44, piman OOO back 2014-6-09 wrote:
> Implementation looks fine % vmiura's comment
> 
> As far as tests, you can also add something to GLTextureMailboxTest
(integration
> test). They're super easy to write and also demonstrate use cases.

Thanks for the suggestion! I hadn't seen that file yet.

Addressed all feedback, added new tests. PTAL!

[bajones, 2014-06-05 21:26:28]

Review Ping?

zmo@ or vmiura@: Can you take a look at the command buffer code?

[vmiura, 2014-06-05 21:48:51]

On 2014/06/05 21:26:28, bajones wrote:
> Review Ping?
> 
> zmo@ or vmiura@: Can you take a look at the command buffer code?

lgtm, thanks!

[bajones, 2014-06-05 23:17:47]

piman@ or sievers@: Could I get a quick review of gpu/GLES2?

[piman, 2014-06-09 14:37:25]

LGTM + nits

https://codereview.chromium.org/299043003/diff/40001/gpu/command_buffer/tests...
File gpu/command_buffer/tests/gl_texture_mailbox_unittest.cc (right):

https://codereview.chromium.org/299043003/diff/40001/gpu/command_buffer/tests...
gpu/command_buffer/tests/gl_texture_mailbox_unittest.cc:21: uint32
ReadTexel(GLuint id, GLint x, GLint y, GLenum format = GL_RGBA) {
nit: no default argument in Chromium style.

https://codereview.chromium.org/299043003/diff/40001/gpu/command_buffer/tests...
gpu/command_buffer/tests/gl_texture_mailbox_unittest.cc:376:
TEST_F(GLTextureMailboxTest, ProduceTextureDirectInvalideTarget) {
nit: Invalide->Invalid ?

[bajones, 2014-06-09 17:29:37]

The CQ bit was checked by bajones@chromium.org

[commit-bot: I haz the power, 2014-06-09 17:30:05]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/bajones@chromium.org/299043003/60001

[bajones, 2014-06-09 21:50:46]

The CQ bit was checked by bajones@chromium.org

[commit-bot: I haz the power, 2014-06-09 21:53:08]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/bajones@chromium.org/299043003/80001

[commit-bot: I haz the power, 2014-06-10 11:40:57]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_clang_dbg/bui...)

[commit-bot: I haz the power, 2014-06-10 13:15:40]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-06-10 13:15:42]

Try jobs failed on following builders:
  android_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_clang_dbg/bui...)

[bajones, 2014-06-10 17:21:12]

The CQ bit was checked by bajones@chromium.org

[commit-bot: I haz the power, 2014-06-10 17:23:31]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/bajones@chromium.org/299043003/80001

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

Change committed as 276068