Allow textures to be moved from one GL context group to another.

Allow textures to be moved from one GL context group to another.

The function glProduceTexture(target, mailbox) removes a texture from a context group and puts it in a "mailbox" that is accessible to all context groups on the same channel.

Then glConsumeTexture(target, mailbox) adds the texture to a context group, possibly a different one, if the caller knows the name of the mailbox.

From the point of view of the caller, the texture objects themselves do not move, just the texels. In terms of the underlying GL, the TextureInfo service IDs are reassigned as necessary.

A texture in a mailbox is destroyed when the context group that put it there is destroyed.

Expected usage is:

// Context 1
glBindTexture(GL_TEXTURE_2D, foo);
glProduceTexture(GL_TEXTURE_2D, secret_name);
glFlush();

// Context 2
glBindTexture(GL_TEXTURE_2D, bar);
glConsumeTexture(GL_TEXTURE_2D, secret_name);

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

[apatrick_chromium, 2012-04-19 00:09:13]

This patch is not finished yet and it has only one test so far. Could you take a
look, especially at the changes in display.cc, texture_manager.cc and
gles2_cmd_decoder.cc?

Some concerns I have are, does this do what we need to get WebGL out of the
compositor's context? Does it work, e.g. is reassigning the service IDs of
TextureInfos a good idea. Is skipping the validators in gles2_cmd_decoder.cc
when calling ConsumeTexture okay?

[apatrick_chromium, 2012-04-19 20:36:21]

+piman

Gregg suggested you might have some thoughts on this.

[Ken Russell (switch to Gerrit), 2012-04-23 20:12:00]

Per our offline discussions, the design seems good overall, and will work for
the use cases we have in mind. It will definitely allow WebGL to be removed from
the compositor's share group as desired. I'm 99% sure it will work the same way
for Pepper 3D.

I wonder whether the GL should produce the mailbox names, but it seems OK to me
to require that the application produce them.

Need to clearly document the new entry points, in particular the size of the
mailbox names. Is 1024 bits overkill?

http://codereview.chromium.org/10106015/diff/9033/gpu/command_buffer/service/...
File gpu/command_buffer/service/display.h (right):

http://codereview.chromium.org/10106015/diff/9033/gpu/command_buffer/service/...
gpu/command_buffer/service/display.h:26: class GPU_EXPORT Display : public
base::RefCounted<Display> {
Is "Display" the best name? Maybe something more concrete, like
CrossContextSharedTextureManager?

http://codereview.chromium.org/10106015/diff/9033/gpu/command_buffer/service/...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

http://codereview.chromium.org/10106015/diff/9033/gpu/command_buffer/service/...
gpu/command_buffer/service/gles2_cmd_decoder.cc:742: // Wrapper for
SwapTexturesCHROMIUM.
Comment out of date? And below?

http://codereview.chromium.org/10106015/diff/9033/gpu/command_buffer/service/...
File gpu/command_buffer/service/gles2_cmd_decoder_unittest.cc (right):

http://codereview.chromium.org/10106015/diff/9033/gpu/command_buffer/service/...
gpu/command_buffer/service/gles2_cmd_decoder_unittest.cc:6749:
TEST_F(GLES2DecoderTest, ProduceAndConsumeTexture) {
Need a test which verifies the functionality across two contexts.

[gman1, 2012-04-23 21:44:10]

On Mon, Apr 23, 2012 at 1:12 PM, <kbr@chromium.org> wrote:

> Per our offline discussions, the design seems good overall, and will work
> for
> the use cases we have in mind. It will definitely allow WebGL to be
> removed from
> the compositor's share group as desired. I'm 99% sure it will work the
> same way
> for Pepper 3D.
>
> I wonder whether the GL should produce the mailbox names, but it seems OK
> to me
> to require that the application produce them.
>

Good point.

Yes? I'd have to look over the code again but it seems like you don't want
to leave that responsibility to the users of the API. Otherwise you risk
them doing something stupid like choosing names like "0", "1", "2" or
"foo", "bar", "baz", etc....


>
> Need to clearly document the new entry points, in particular the size of
> the
> mailbox names. Is 1024 bits overkill?
>
>
>
> http://codereview.chromium.**org/10106015/diff/9033/gpu/**
>
command_buffer/service/**display.h<http://codereview.chromium.org/10106015/diff/9033/gpu/command_buffer/service/display.h>
> File gpu/command_buffer/service/**display.h (right):
>
> http://codereview.chromium.**org/10106015/diff/9033/gpu/**
>
command_buffer/service/**display.h#newcode26<http://codereview.chromium.org/10106015/diff/9033/gpu/command_buffer/service/display.h#newcode26>
> gpu/command_buffer/service/**display.h:26: class GPU_EXPORT Display :
> public base::RefCounted<Display> {
> Is "Display" the best name? Maybe something more concrete, like
> CrossContextSharedTextureManag**er?
>
> http://codereview.chromium.**org/10106015/diff/9033/gpu/**
>
command_buffer/service/gles2_**cmd_decoder.cc<http://codereview.chromium.org/10106015/diff/9033/gpu/command_buffer/service/gles2_cmd_decoder.cc>
> File gpu/command_buffer/service/**gles2_cmd_decoder.cc (right):
>
> http://codereview.chromium.**org/10106015/diff/9033/gpu/**
>
command_buffer/service/gles2_**cmd_decoder.cc#newcode742<http://codereview.chromium.org/10106015/diff/9033/gpu/command_buffer/service/gles2_cmd_decoder.cc#newcode742>
> gpu/command_buffer/service/**gles2_cmd_decoder.cc:742: // Wrapper for
> SwapTexturesCHROMIUM.
> Comment out of date? And below?
>
> http://codereview.chromium.**org/10106015/diff/9033/gpu/**
>
command_buffer/service/gles2_**cmd_decoder_unittest.cc<http://codereview.chromium.org/10106015/diff/9033/gpu/command_buffer/service/gles2_cmd_decoder_unittest.cc>
> File gpu/command_buffer/service/**gles2_cmd_decoder_unittest.cc (right):
>
> http://codereview.chromium.**org/10106015/diff/9033/gpu/**
>
command_buffer/service/gles2_**cmd_decoder_unittest.cc#**newcode6749<http://codereview.chromium.org/10106015/diff/9033/gpu/command_buffer/service/gles2_cmd_decoder_unittest.cc#newcode6749>
> gpu/command_buffer/service/**gles2_cmd_decoder_unittest.cc:**6749:
> TEST_F(GLES2DecoderTest, ProduceAndConsumeTexture) {
> Need a test which verifies the functionality across two contexts.
>
>
http://codereview.chromium.**org/10106015/<http://codereview.chromium.org/101...
>

[apatrick_chromium, 2012-04-25 21:58:53]

Done sans documentation (which I'll do). PTAL.

[Ken Russell (switch to Gerrit), 2012-04-26 01:10:53]

Looks really good overall, in particular the thorough tests. Few minor comments.

https://chromiumcodereview.appspot.com/10106015/diff/17040/gpu/command_buffer...
File gpu/command_buffer/build_gles2_cmd_buffer.py (right):

https://chromiumcodereview.appspot.com/10106015/diff/17040/gpu/command_buffer...
gpu/command_buffer/build_gles2_cmd_buffer.py:868: 'data_type': 'char',
Can this use GLchar? Or could there be a typedef GLchar[32] GLmailboxCHROMIUM?

https://chromiumcodereview.appspot.com/10106015/diff/17040/gpu/command_buffer...
File gpu/command_buffer/common/constants.h (right):

https://chromiumcodereview.appspot.com/10106015/diff/17040/gpu/command_buffer...
gpu/command_buffer/common/constants.h:52: // Size of a mailbox name is bytes.
is -> in

https://chromiumcodereview.appspot.com/10106015/diff/17040/gpu/command_buffer...
gpu/command_buffer/common/constants.h:53: const int32 kMailboxSize = 32;
Unfortunate that this can't be implicit in the type passed to
GenMailboxCHROMIUM, etc.

https://chromiumcodereview.appspot.com/10106015/diff/17040/gpu/command_buffer...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

https://chromiumcodereview.appspot.com/10106015/diff/17040/gpu/command_buffer...
gpu/command_buffer/service/gles2_cmd_decoder.cc:750: // Wrapper for
SwapTexturesCHROMIUM.
Wrong comment.

https://chromiumcodereview.appspot.com/10106015/diff/17040/gpu/command_buffer...
gpu/command_buffer/service/gles2_cmd_decoder.cc:753: // Wrapper for
TexKeyCHROMIUM.
Wrong comment.

https://chromiumcodereview.appspot.com/10106015/diff/17040/gpu/command_buffer...
File gpu/command_buffer/tests/gl_unittests.cc (right):

https://chromiumcodereview.appspot.com/10106015/diff/17040/gpu/command_buffer...
gpu/command_buffer/tests/gl_unittests.cc:72: TEST_F(GLTest,
ProduceAndConsumeTexture) {
Nice test!

https://chromiumcodereview.appspot.com/10106015/diff/17040/gpu/command_buffer...
gpu/command_buffer/tests/gl_unittests.cc:76: char mailbox[32];
Really wish the "32" could be part of the type.

https://chromiumcodereview.appspot.com/10106015/diff/17040/third_party/khrono...
File third_party/khronos/GLES2/gl2ext.h (right):

https://chromiumcodereview.appspot.com/10106015/diff/17040/third_party/khrono...
third_party/khronos/GLES2/gl2ext.h:1985: /* GL_CHROMIUM_copy_texture */
Should be GL_CHROMIUM_texture_mailbox. Also, the mailbox size needs to be
defined in this file somehow. Minimally something like "#define
GL_MAILBOX_SIZE_CHROMIUM 32".

[greggman, 2012-04-26 18:09:18]

https://chromiumcodereview.appspot.com/10106015/diff/17040/gpu/command_buffer...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

https://chromiumcodereview.appspot.com/10106015/diff/17040/gpu/command_buffer...
gpu/command_buffer/service/gles2_cmd_decoder.cc:8564:
group_->mailbox_manager()->ProduceTexture(
I'm a little concerned that GenMailbox is optional. Does it matter?

https://chromiumcodereview.appspot.com/10106015/diff/17040/gpu/command_buffer...
File gpu/command_buffer/service/mailbox_manager.h (right):

https://chromiumcodereview.appspot.com/10106015/diff/17040/gpu/command_buffer...
gpu/command_buffer/service/mailbox_manager.h:22: // Identified a mailbox where a
texture definition can be stored for sharing
s/Identified/Identifies/ ?

[apatrick_chromium, 2012-04-26 21:17:40]

I added a constant to gl2ext.h for the size of the mailbox and tried to use it
to reduce redundancy.

The MailboxManager now signs each mailbox name using a private key so that it
can verify that names were produced with glGenMailbox.

I think I addressed everything. PTAL.

[Ken Russell (switch to Gerrit), 2012-04-26 22:34:14]

One semantic issue in glProduceTextureCHROMIUM. The others are minor comments.

http://codereview.chromium.org/10106015/diff/45007/gpu/command_buffer/common/...
File gpu/command_buffer/common/gles2_cmd_format.h (right):

http://codereview.chromium.org/10106015/diff/45007/gpu/command_buffer/common/...
gpu/command_buffer/common/gles2_cmd_format.h:38: typedef char GLchar;
This doesn't seem to be referenced anywhere else in this patch.

http://codereview.chromium.org/10106015/diff/45007/gpu/command_buffer/service...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

http://codereview.chromium.org/10106015/diff/45007/gpu/command_buffer/service...
gpu/command_buffer/service/gles2_cmd_decoder.cc:8562: return;
What happens to the texture in this case? Looks like it's in limbo. Either it
needs to remain untouched or it needs to be conceptually destroyed.

http://codereview.chromium.org/10106015/diff/45007/gpu/command_buffer/service...
File gpu/command_buffer/service/mailbox_manager.h (right):

http://codereview.chromium.org/10106015/diff/45007/gpu/command_buffer/service...
gpu/command_buffer/service/mailbox_manager.h:21: // transferring between
contexts that are not in the same context group. It is
"sharing transferring" -> "transferring textures"?

http://codereview.chromium.org/10106015/diff/45007/gpu/command_buffer/service...
File gpu/command_buffer/service/texture_definition.h (right):

http://codereview.chromium.org/10106015/diff/45007/gpu/command_buffer/service...
gpu/command_buffer/service/texture_definition.h:19: // context in using the same
GLShareGroup with the corresponding service ID.
"in using" -> "using"

http://codereview.chromium.org/10106015/diff/45007/gpu/command_buffer/tests/g...
File gpu/command_buffer/tests/gl_texture_mailbox_unittests.cc (right):

http://codereview.chromium.org/10106015/diff/45007/gpu/command_buffer/tests/g...
gpu/command_buffer/tests/gl_texture_mailbox_unittests.cc:131:
EXPECT_EQ(GL_INVALID_OPERATION, glGetError());
Ideally would also test whether the texture was destroyed or preserved during
this operation.

[apatrick_chromium, 2012-04-27 22:31:58]

I changed the mailbox name size to 512 bits to accommodate a 256-bit key and a
SHA256 hash without truncation. Also, used HMAC instead of SecureHash since the
former works in the sandbox.

http://codereview.chromium.org/10106015/diff/45007/gpu/command_buffer/common/...
File gpu/command_buffer/common/gles2_cmd_format.h (right):

http://codereview.chromium.org/10106015/diff/45007/gpu/command_buffer/common/...
gpu/command_buffer/common/gles2_cmd_format.h:38: typedef char GLchar;
On 2012/04/26 22:34:14, kbr wrote:
> This doesn't seem to be referenced anywhere else in this patch.

Done.

http://codereview.chromium.org/10106015/diff/45007/gpu/command_buffer/service...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

http://codereview.chromium.org/10106015/diff/45007/gpu/command_buffer/service...
gpu/command_buffer/service/gles2_cmd_decoder.cc:8562: return;
Fixed and added a test.

http://codereview.chromium.org/10106015/diff/45007/gpu/command_buffer/service...
File gpu/command_buffer/service/mailbox_manager.h (right):

http://codereview.chromium.org/10106015/diff/45007/gpu/command_buffer/service...
gpu/command_buffer/service/mailbox_manager.h:21: // transferring between
contexts that are not in the same context group. It is
On 2012/04/26 22:34:14, kbr wrote:
> "sharing transferring" -> "transferring textures"?

Done.

http://codereview.chromium.org/10106015/diff/45007/gpu/command_buffer/service...
File gpu/command_buffer/service/texture_definition.h (right):

http://codereview.chromium.org/10106015/diff/45007/gpu/command_buffer/service...
gpu/command_buffer/service/texture_definition.h:19: // context in using the same
GLShareGroup with the corresponding service ID.
On 2012/04/26 22:34:14, kbr wrote:
> "in using" -> "using"

Done.

http://codereview.chromium.org/10106015/diff/45007/gpu/command_buffer/tests/g...
File gpu/command_buffer/tests/gl_texture_mailbox_unittests.cc (right):

http://codereview.chromium.org/10106015/diff/45007/gpu/command_buffer/tests/g...
gpu/command_buffer/tests/gl_texture_mailbox_unittests.cc:131:
EXPECT_EQ(GL_INVALID_OPERATION, glGetError());
On 2012/04/26 22:34:14, kbr wrote:
> Ideally would also test whether the texture was destroyed or preserved during
> this operation.

Done.

http://codereview.chromium.org/10106015/diff/67005/gpu/command_buffer/service...
File gpu/command_buffer/service/mailbox_manager.cc (right):

http://codereview.chromium.org/10106015/diff/67005/gpu/command_buffer/service...
gpu/command_buffer/service/mailbox_manager.cc:19: base::RandBytes(private_key,
sizeof(private_key));
This actually is a cryptographically strong on all platforms.

http://codereview.chromium.org/10106015/diff/67005/gpu/command_buffer/service...
File gpu/command_buffer/service/mailbox_manager.h (right):

http://codereview.chromium.org/10106015/diff/67005/gpu/command_buffer/service...
gpu/command_buffer/service/mailbox_manager.h:18: #define
GL_MAILBOX_SIZE_CHROMIUM 64
Including GL header files in a header file causes trouble because there are
several different GL headers and they conflict: mesa's, ANGLE's, our GLES2
headers and gl_utils.h. So there is some redundancy for GL_MAINBOX_SIZE_CHROMIUM
and GLbyte.

[Ken Russell (switch to Gerrit), 2012-04-28 00:27:40]

LGTM; very nice. Couple of minor comments.

http://codereview.chromium.org/10106015/diff/67005/gpu/command_buffer/service...
File gpu/command_buffer/service/mailbox_manager.h (right):

http://codereview.chromium.org/10106015/diff/67005/gpu/command_buffer/service...
gpu/command_buffer/service/mailbox_manager.h:18: #define
GL_MAILBOX_SIZE_CHROMIUM 64
On 2012/04/27 22:31:58, apatrick_chromium wrote:
> Including GL header files in a header file causes trouble because there are
> several different GL headers and they conflict: mesa's, ANGLE's, our GLES2
> headers and gl_utils.h. So there is some redundancy for
GL_MAINBOX_SIZE_CHROMIUM
> and GLbyte.

OK. Consider adding comments to point to the files that need to be kept in sync.

http://codereview.chromium.org/10106015/diff/67005/gpu/command_buffer/tests/g...
File gpu/command_buffer/tests/gl_texture_mailbox_unittests.cc (right):

http://codereview.chromium.org/10106015/diff/67005/gpu/command_buffer/tests/g...
gpu/command_buffer/tests/gl_texture_mailbox_unittests.cc:138:
EXPECT_EQ(0x00000000U, ReadTexel(tex, 0, 0));
Consider filling the texture with a color first to better verify that its
contents are intact? Here and in the test below.

[commit-bot: I haz the power, 2012-05-01 01:08:43]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/apatrick@chromium.org/10106015/76004

[commit-bot: I haz the power, 2012-05-01 01:09:14]

Can't apply patch for file
gpu/command_buffer/tests/gl_texture_mailbox_unittests.cc.
While running patch -p0 --forward --force;
patching file gpu/command_buffer/tests/gl_texture_mailbox_unittests.cc
Hunk #1 FAILED at 2.
1 out of 1 hunk FAILED -- saving rejects to file
gpu/command_buffer/tests/gl_texture_mailbox_unittests.cc.rej

[commit-bot: I haz the power, 2012-05-01 19:47:21]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/apatrick@chromium.org/10106015/81052

[commit-bot: I haz the power, 2012-05-01 19:47:46]

Can't apply patch for file
gpu/command_buffer/tests/gl_texture_mailbox_unittests.cc.
While running patch -p0 --forward --force;
patching file gpu/command_buffer/tests/gl_texture_mailbox_unittests.cc
Hunk #1 FAILED at 2.
1 out of 1 hunk FAILED -- saving rejects to file
gpu/command_buffer/tests/gl_texture_mailbox_unittests.cc.rej

[commit-bot: I haz the power, 2012-05-01 20:03:17]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/apatrick@chromium.org/10106015/78020

[commit-bot: I haz the power, 2012-05-01 21:51:05]

Change committed as 134791