gpu: Mailbox emulation with EGLImage

gpu: Mailbox emulation with EGLImage

This adds support for sharing image buffers through EGLImage-like
semantics. It's made to look as if textures were shared through
mailboxes by creating multiple Texture instances and synchronizing
parameters and image handle etc. across MailboxManager instances (for
example across contexts and possibly GPU threads).
 
Since GL clients are async anyways, it only guarantees that changes to
the texture structure or filters become visible after the client has
waited on a sync point, while fences are used to synchronize read/write
access to the underlying buffer.
 
NOTRY=True
BUG=332146
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=256962

[piman, 2014-03-08 01:03:10]

https://codereview.chromium.org/180723023/diff/40001/gpu/command_buffer/servi...
File gpu/command_buffer/service/mailbox_manager.cc (right):

https://codereview.chromium.org/180723023/diff/40001/gpu/command_buffer/servi...
gpu/command_buffer/service/mailbox_manager.cc:18:
sync_(MailboxSynchronizer::GetInstance()) {
nit: it'd be nice to pass explicitly to avoid one more global (could be passed
explicitly to GpuChannelManager and InProcessCommandBuffer).

https://codereview.chromium.org/180723023/diff/40001/gpu/command_buffer/servi...
File gpu/command_buffer/service/mailbox_synchronizer.h (right):

https://codereview.chromium.org/180723023/diff/40001/gpu/command_buffer/servi...
gpu/command_buffer/service/mailbox_synchronizer.h:64: GlobalMailbox(const
TextureDefinition& definition)
nit: explicit

[no sievers, 2014-03-12 18:45:36]

+reveman for GLImage plumbing

Ok, should be pretty functional at this point.

I've added versioning to find out when a texture was modified to avoid redundant
updates (texture->saved definition or vice versa). This is actually necessary to
avoid texture pushes clobbering a definition with an older version (I added a
unit test for that).

It *should* handle producing different textures into existing mailboxes and all
of that now (I'm owing tests), although it could be probably made more
efficient.

It doesn't work well to track the fence as part of the definition, since we
really want to know about read and write access to a buffer. Instead I added the
fence synchronization at a different level, allowing for the model of multiple
readers with only one client modifying the pixels.

I had to add plumbing for GLImage to know when it's being updated
(glCopyTex(Sub)Image, glTexSubImage, glDrawArrays/Elements).

https://codereview.chromium.org/180723023/diff/40001/gpu/command_buffer/servi...
File gpu/command_buffer/service/mailbox_manager.cc (right):

https://codereview.chromium.org/180723023/diff/40001/gpu/command_buffer/servi...
gpu/command_buffer/service/mailbox_manager.cc:18:
sync_(MailboxSynchronizer::GetInstance()) {
On 2014/03/08 01:03:11, piman wrote:
> nit: it'd be nice to pass explicitly to avoid one more global (could be passed
> explicitly to GpuChannelManager and InProcessCommandBuffer).

I still need to do this.

https://codereview.chromium.org/180723023/diff/40001/gpu/command_buffer/servi...
File gpu/command_buffer/service/mailbox_synchronizer.h (right):

https://codereview.chromium.org/180723023/diff/40001/gpu/command_buffer/servi...
gpu/command_buffer/service/mailbox_synchronizer.h:64: GlobalMailbox(const
TextureDefinition& definition)
On 2014/03/08 01:03:11, piman wrote:
> nit: explicit

Done.

[reveman, 2014-03-12 19:18:03]

We currently call Will/DidUseTexImage when attaching texture to FBO. WillModify
replaces that with a more accurate notification so the right thing seems to be
to not call Will/DidUseTexImage when binding to FBO anymore. Wdyt?

https://codereview.chromium.org/180723023/diff/80001/ui/gl/gl_image.h
File ui/gl/gl_image.h (right):

https://codereview.chromium.org/180723023/diff/80001/ui/gl/gl_image.h#newcode43
ui/gl/gl_image.h:43: virtual void WillModifyPixels() {}
This assumes that GLImage is not buffered like GLImageGLX or GLImageShm. I think
this should be called WillModifyTexImage as it's not the underlaying image data
that is necessarily modified.

Actually, I think I'd prefer Read/Write instead of Use/Modify. Wdyt?

nit: move implementation to gl_image.cc for consistency.

https://codereview.chromium.org/180723023/diff/80001/ui/gl/gl_image.h#newcode50
ui/gl/gl_image.h:50: virtual void SetReleaseAfterUse();
Please change this too if you rename WillUseTexImage to WillReadTexImage.

[no sievers, 2014-03-12 20:22:12]

On 2014/03/12 19:18:03, reveman wrote:
> We currently call Will/DidUseTexImage when attaching texture to FBO.
WillModify
> replaces that with a more accurate notification so the right thing seems to be
> to not call Will/DidUseTexImage when binding to FBO anymore. Wdyt?
> 

For binding to an FBO it actually calls this immediately around
glFramebufferTexture2D() to make sure that the texture is valid when we attach
it to an FBO.

Will/DidModify happens around the actual draw calls to the FBO.

I think WillUseTexImage() is maybe a bit more general than WillRead(), but it's
only used in GLImageEGL for the 'release_after_use_' logic. There it makes sure
the texture is defined for any GL call that needs to use the texture, incl. for
example glFramebufferTexture2D() which is not a read operation. 

If we didn't have the need for release_after_use, we could split this up into
WillDidRead/WillDidWrite. Or we could split it up into 3 sets of functions :/

[reveman, 2014-03-12 20:33:16]

On 2014/03/12 20:22:12, sievers wrote:
> On 2014/03/12 19:18:03, reveman wrote:
> > We currently call Will/DidUseTexImage when attaching texture to FBO.
> WillModify
> > replaces that with a more accurate notification so the right thing seems to
be
> > to not call Will/DidUseTexImage when binding to FBO anymore. Wdyt?
> > 
> 
> For binding to an FBO it actually calls this immediately around
> glFramebufferTexture2D() to make sure that the texture is valid when we attach
> it to an FBO.
> 
> Will/DidModify happens around the actual draw calls to the FBO.
> 
> I think WillUseTexImage() is maybe a bit more general than WillRead(), but
it's
> only used in GLImageEGL for the 'release_after_use_' logic. There it makes
sure
> the texture is defined for any GL call that needs to use the texture, incl.
for
> example glFramebufferTexture2D() which is not a read operation. 
> 
> If we didn't have the need for release_after_use, we could split this up into
> WillDidRead/WillDidWrite. Or we could split it up into 3 sets of functions :/

Ok, let's keep Will/DidUse for now but rename Modify functions to
Will/DidModifyTexImage.

[no sievers, 2014-03-12 21:03:42]

On 2014/03/12 20:33:16, reveman wrote:
> On 2014/03/12 20:22:12, sievers wrote:
> > On 2014/03/12 19:18:03, reveman wrote:
> > > We currently call Will/DidUseTexImage when attaching texture to FBO.
> > WillModify
> > > replaces that with a more accurate notification so the right thing seems
to
> be
> > > to not call Will/DidUseTexImage when binding to FBO anymore. Wdyt?
> > > 
> > 
> > For binding to an FBO it actually calls this immediately around
> > glFramebufferTexture2D() to make sure that the texture is valid when we
attach
> > it to an FBO.
> > 
> > Will/DidModify happens around the actual draw calls to the FBO.
> > 
> > I think WillUseTexImage() is maybe a bit more general than WillRead(), but
> it's
> > only used in GLImageEGL for the 'release_after_use_' logic. There it makes
> sure
> > the texture is defined for any GL call that needs to use the texture, incl.
> for
> > example glFramebufferTexture2D() which is not a read operation. 
> > 
> > If we didn't have the need for release_after_use, we could split this up
into
> > WillDidRead/WillDidWrite. Or we could split it up into 3 sets of functions
:/
> 
> Ok, let's keep Will/DidUse for now but rename Modify functions to
> Will/DidModifyTexImage.

Done.

[reveman, 2014-03-12 22:53:02]

ui/gl/gl_image* lgtm

https://codereview.chromium.org/180723023/diff/120001/ui/gl/gl_image.h
File ui/gl/gl_image.h (right):

https://codereview.chromium.org/180723023/diff/120001/ui/gl/gl_image.h#newcode46
ui/gl/gl_image.h:46: virtual void DidModifyTexImage();
while here, do you mind making these and all functions above pure virtual? We
might as well have the compiler tell us when a function is not implemented
instead of default implementations with NOTIMPLEMENTED statements.
SetReleaseAfterUse can have a default implementation as it's a workaround.

[boliu, 2014-03-13 01:08:53]

android_webview lgtm

[piman, 2014-03-13 04:41:58]

https://codereview.chromium.org/180723023/diff/180001/gpu/command_buffer/serv...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

https://codereview.chromium.org/180723023/diff/180001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder.cc:359:
ScopedModifyPixels(TextureRef* ref);
nit: explicit

https://codereview.chromium.org/180723023/diff/180001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder.cc:378: ScopedRenderTo(Framebuffer*
framebuffer);
nit: explicit

https://codereview.chromium.org/180723023/diff/180001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder.cc:387: color_buffer_ =
framebuffer->GetAttachment(GL_COLOR_ATTACHMENT0);
Do we need color attachments 1..n? Maybe depth/stencil too?

https://codereview.chromium.org/180723023/diff/180001/gpu/command_buffer/serv...
File gpu/command_buffer/service/mailbox_manager.cc (right):

https://codereview.chromium.org/180723023/diff/180001/gpu/command_buffer/serv...
gpu/command_buffer/service/mailbox_manager.cc:40: DCHECK_EQ(0U,
texture->refs_.size()); // TODO
nit: trailing // TODO
From my reading of it, the DCHECK is valid, is there something missing?

https://codereview.chromium.org/180723023/diff/180001/gpu/command_buffer/serv...
File gpu/command_buffer/service/mailbox_synchronizer.cc (right):

https://codereview.chromium.org/180723023/diff/180001/gpu/command_buffer/serv...
gpu/command_buffer/service/mailbox_synchronizer.cc:118: // TODO(sievers)
nit: TODO? bug#?

https://codereview.chromium.org/180723023/diff/180001/gpu/command_buffer/serv...
gpu/command_buffer/service/mailbox_synchronizer.cc:127: TextureGroup* group =
it->second.group.get();
nit: it would be nice to give a name to it->second, here and below, e.g.
const TextureVersion& texture_version = it->second;

It's hard to follow what |it->second| is.

https://codereview.chromium.org/180723023/diff/180001/gpu/command_buffer/serv...
gpu/command_buffer/service/mailbox_synchronizer.cc:138: if
(group->definition.IsNewerThan(it->second.version))
Do we want IsNewerOrEqual? If versions are equal, we should skip quickly?

https://codereview.chromium.org/180723023/diff/180001/gpu/command_buffer/serv...
gpu/command_buffer/service/mailbox_synchronizer.cc:148: return;
What are the conditions under which this would happen? Is it something like
BindTexture(TEXTURE_2D, id);
ProduceTexture(TEXTURE_2D, mailbox);
InsertSyncPoint();
BindTexImage2DCHROMIUM(TEXTURE_2D, image);
InsertSyncPoint();

?

It seems like it could legitimately happen. However once this happens on any
(possibly untrusted) channel that uses the MailboxManager, it would stop updates
to some textures in the same MM (the ones that are further in the
MailboxToTextureMap), including on other channels... that seems wrong.

Do you really need to return here, or could you just continue to skip this
texture (if we can't handle it... though it'd be better if we could)?

https://codereview.chromium.org/180723023/diff/180001/gpu/command_buffer/serv...
File gpu/command_buffer/service/texture_definition.cc (right):

https://codereview.chromium.org/180723023/diff/180001/gpu/command_buffer/serv...
gpu/command_buffer/service/texture_definition.cc:114: NativeImageBufferEGL*
NativeImageBufferEGL::Create(GLuint texture_id) {
nit: return a scoped_refptr<NativeImageBufferEGL>

https://codereview.chromium.org/180723023/diff/180001/gpu/command_buffer/serv...
gpu/command_buffer/service/texture_definition.cc:121: }
Can these conditions happen? Should they just be DCHECKs?

https://codereview.chromium.org/180723023/diff/180001/gpu/command_buffer/serv...
gpu/command_buffer/service/texture_definition.cc:159: DCHECK_EQ(GL_NO_ERROR,
(GLint)glGetError());
nit: no c-style casts. Prefer static_cast or reinterpret_cast as appropriate.

https://codereview.chromium.org/180723023/diff/180001/gpu/command_buffer/serv...
gpu/command_buffer/service/texture_definition.cc:287: // of perf when there are
multiple draw calls per frame.
So, the risk is that if we're missing a sync point somewhere, we might deadlock
the GPU if we have one write and one read, and no glFlush in the middle.

Would it be possible to, say, not "publish" the write fence until we push? I.e.
make the write_fence a property of the TextureDefinition? We might even be able
to delay the inserting of the fence until the Push rather than the draw, since
otherwise you'll insert one fence for every draw, which seems heavy.

We would need to tag the NativeImageBuffer as "dirty" in DidWrite, then on Push,
look for dirty NativeImageBuffers (and treat them as a version update), and if
any, lazily insert a single fence. Then on Pull, give the fence (if any) to the
GLImageSync (we may need refcounting...), which will then wait on it before read
or write.

WDYT?

https://codereview.chromium.org/180723023/diff/180001/gpu/command_buffer/serv...
gpu/command_buffer/service/texture_definition.cc:383: if (image_)
DCHECK, or if? Chose one.

https://codereview.chromium.org/180723023/diff/180001/gpu/command_buffer/serv...
gpu/command_buffer/service/texture_definition.cc:385: glFlush();
DO you need this glFlush?

https://codereview.chromium.org/180723023/diff/180001/gpu/command_buffer/serv...
File gpu/command_buffer/service/texture_definition.h (right):

https://codereview.chromium.org/180723023/diff/180001/gpu/command_buffer/serv...
gpu/command_buffer/service/texture_definition.h:80: return (int)(version_ -
version) > 0;
int overflow = undefined. I'm afraid the compiler can legally treat this as
version_ != version.
How about version_ - version < 0x80000000 ?

https://codereview.chromium.org/180723023/diff/180001/gpu/command_buffer/serv...
gpu/command_buffer/service/texture_definition.h:114:
scoped_refptr<NativeImageBuffer> image_;
nit: image_buffer_ maybe? The callsites are confusing, you expect image_ to be a
GLImage.

https://codereview.chromium.org/180723023/diff/180001/gpu/command_buffer/serv...
File gpu/command_buffer/service/texture_manager.cc (right):

https://codereview.chromium.org/180723023/diff/180001/gpu/command_buffer/serv...
gpu/command_buffer/service/texture_manager.cc:812: }
Won't that break cube maps?

https://codereview.chromium.org/180723023/diff/180001/ui/gl/gl_fence.cc
File ui/gl/gl_fence.cc (right):

https://codereview.chromium.org/180723023/diff/180001/ui/gl/gl_fence.cc#newco...
ui/gl/gl_fence.cc:135: return new GLFenceARBSync(flush);
Because the NV_fence implementation degrades to a client wait for the server
wait, we should prefer the ARB_sync version.

[no sievers, 2014-03-13 20:50:15]

I've also fixed a bug with this sequence:

1) ctx1: Produce, Sync
2) ctx2: waitSync
3) ctx2: modify texture
4) ctx2: delete texture  
5) ctx2: insertSyncPoint

5) before 4) was working, but otherwise need to push a final update when the
texture is deleted.

https://codereview.chromium.org/180723023/diff/180001/gpu/command_buffer/serv...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

https://codereview.chromium.org/180723023/diff/180001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder.cc:359:
ScopedModifyPixels(TextureRef* ref);
On 2014/03/13 04:41:58, piman wrote:
> nit: explicit

Done.

https://codereview.chromium.org/180723023/diff/180001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder.cc:378: ScopedRenderTo(Framebuffer*
framebuffer);
On 2014/03/13 04:41:58, piman wrote:
> nit: explicit

Done.

https://codereview.chromium.org/180723023/diff/180001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder.cc:387: color_buffer_ =
framebuffer->GetAttachment(GL_COLOR_ATTACHMENT0);
On 2014/03/13 04:41:58, piman wrote:
> Do we need color attachments 1..n? Maybe depth/stencil too?

Done.

https://codereview.chromium.org/180723023/diff/180001/gpu/command_buffer/serv...
File gpu/command_buffer/service/mailbox_manager.cc (right):

https://codereview.chromium.org/180723023/diff/180001/gpu/command_buffer/serv...
gpu/command_buffer/service/mailbox_manager.cc:40: DCHECK_EQ(0U,
texture->refs_.size()); // TODO
On 2014/03/13 04:41:58, piman wrote:
> nit: trailing // TODO
> From my reading of it, the DCHECK is valid, is there something missing?

Done. I think I was wondering if there is maybe a nicer way to pass this unref'd
object around. Will leave as is for now.

https://codereview.chromium.org/180723023/diff/180001/gpu/command_buffer/serv...
File gpu/command_buffer/service/mailbox_synchronizer.cc (right):

https://codereview.chromium.org/180723023/diff/180001/gpu/command_buffer/serv...
gpu/command_buffer/service/mailbox_synchronizer.cc:118: // TODO(sievers)
On 2014/03/13 04:41:58, piman wrote:
> nit: TODO? bug#?

Done.

https://codereview.chromium.org/180723023/diff/180001/gpu/command_buffer/serv...
gpu/command_buffer/service/mailbox_synchronizer.cc:127: TextureGroup* group =
it->second.group.get();
On 2014/03/13 04:41:58, piman wrote:
> nit: it would be nice to give a name to it->second, here and below, e.g.
> const TextureVersion& texture_version = it->second;
> 
> It's hard to follow what |it->second| is.

Done.

https://codereview.chromium.org/180723023/diff/180001/gpu/command_buffer/serv...
gpu/command_buffer/service/mailbox_synchronizer.cc:138: if
(group->definition.IsNewerThan(it->second.version))
On 2014/03/13 04:41:58, piman wrote:
> Do we want IsNewerOrEqual? If versions are equal, we should skip quickly?

Good point. Done.

https://codereview.chromium.org/180723023/diff/180001/gpu/command_buffer/serv...
gpu/command_buffer/service/mailbox_synchronizer.cc:148: return;
On 2014/03/13 04:41:58, piman wrote:
> What are the conditions under which this would happen? Is it something like
> BindTexture(TEXTURE_2D, id);
> ProduceTexture(TEXTURE_2D, mailbox);
> InsertSyncPoint();
> BindTexImage2DCHROMIUM(TEXTURE_2D, image);
> InsertSyncPoint();
> 
> ?
> 
> It seems like it could legitimately happen. However once this happens on any
> (possibly untrusted) channel that uses the MailboxManager, it would stop
updates
> to some textures in the same MM (the ones that are further in the
> MailboxToTextureMap), including on other channels... that seems wrong.
> 
> Do you really need to return here, or could you just continue to skip this
> texture (if we can't handle it... though it'd be better if we could)?

Oops, thanks. The 'return' is a total typo, it should continue.

About handling this... I think in some cases we can theoretically. Here are the
existing Android textures that this would skip:

1) async uploads with EGLImage (for example tile textures):
For this we need to change the async upload code to generate compatible GLImage
attachments.

2) StreamTextures
I think we cannot handle this, since we cannot get to the EGLImage handle
internally allocated for the texture. And you cannot create another EGLImage out
of any EGLImage sibling.

3) gralloc 0-copy buffers (GLImageEGL):
These are compatible since they use EGLImage internally.

Overall, I'd probably want to pull out the synchronization code and
NativeImageBuffer so it can be made compatible for use with GLImageEGL.

Android Webview does not use 1), does not need to share 2) across share groups
(stream texture gets created in compositor share group and sharing video hw
texture with canvas/webgl involves a copy which can be made to work), and does
currently not need to put 3) into mailboxes. So for the time being we are good.

https://codereview.chromium.org/180723023/diff/180001/gpu/command_buffer/serv...
File gpu/command_buffer/service/texture_definition.cc (right):

https://codereview.chromium.org/180723023/diff/180001/gpu/command_buffer/serv...
gpu/command_buffer/service/texture_definition.cc:114: NativeImageBufferEGL*
NativeImageBufferEGL::Create(GLuint texture_id) {
On 2014/03/13 04:41:58, piman wrote:
> nit: return a scoped_refptr<NativeImageBufferEGL>

Done.

https://codereview.chromium.org/180723023/diff/180001/gpu/command_buffer/serv...
gpu/command_buffer/service/texture_definition.cc:121: }
On 2014/03/13 04:41:58, piman wrote:
> Can these conditions happen? Should they just be DCHECKs?

You mean no context or display, right?

https://codereview.chromium.org/180723023/diff/180001/gpu/command_buffer/serv...
gpu/command_buffer/service/texture_definition.cc:159: DCHECK_EQ(GL_NO_ERROR,
(GLint)glGetError());
On 2014/03/13 04:41:58, piman wrote:
> nit: no c-style casts. Prefer static_cast or reinterpret_cast as appropriate.

Done.

https://codereview.chromium.org/180723023/diff/180001/gpu/command_buffer/serv...
gpu/command_buffer/service/texture_definition.cc:287: // of perf when there are
multiple draw calls per frame.
On 2014/03/13 04:41:58, piman wrote:
> So, the risk is that if we're missing a sync point somewhere, we might
deadlock
> the GPU if we have one write and one read, and no glFlush in the middle.
> 
> Would it be possible to, say, not "publish" the write fence until we push?
I.e.
> make the write_fence a property of the TextureDefinition? We might even be
able
> to delay the inserting of the fence until the Push rather than the draw, since
> otherwise you'll insert one fence for every draw, which seems heavy.
> 
> We would need to tag the NativeImageBuffer as "dirty" in DidWrite, then on
Push,
> look for dirty NativeImageBuffers (and treat them as a version update), and if
> any, lazily insert a single fence. Then on Pull, give the fence (if any) to
the
> GLImageSync (we may need refcounting...), which will then wait on it before
read
> or write.
> 
> WDYT?

Can I owe this as an immediate follow-up?
I was also considering to expose this functionality in GLFence. Maybe the fence
could be notified about flushes by the GL api?

https://codereview.chromium.org/180723023/diff/180001/gpu/command_buffer/serv...
gpu/command_buffer/service/texture_definition.cc:383: if (image_)
On 2014/03/13 04:41:58, piman wrote:
> DCHECK, or if? Chose one.

Done. Should eventually handle incomplete textures, that later create an image
when defined.

https://codereview.chromium.org/180723023/diff/180001/gpu/command_buffer/serv...
gpu/command_buffer/service/texture_definition.cc:385: glFlush();
On 2014/03/13 04:41:58, piman wrote:
> DO you need this glFlush?

Most of the time probably not. There is one case I'm not sure about wrt
compatibility to share group mailbox semantics:

ctx1 is in driver share group A
ctx2a and ctx2b are in driver share group B

ctx1 defines the texture and flushes to make changes visible, then produces
ctx2a consumes and then shares with ctx2b

With existing mailbox semantics ctx2a does not need to flush because it's not
making changes to the texture before sharing it with ctx2b.
But with this we defined a new texture in ctx2a and it wouldn't be necessarily
visible to ctx2b, right?

https://codereview.chromium.org/180723023/diff/180001/gpu/command_buffer/serv...
File gpu/command_buffer/service/texture_definition.h (right):

https://codereview.chromium.org/180723023/diff/180001/gpu/command_buffer/serv...
gpu/command_buffer/service/texture_definition.h:80: return (int)(version_ -
version) > 0;
On 2014/03/13 04:41:58, piman wrote:
> int overflow = undefined. I'm afraid the compiler can legally treat this as
> version_ != version.
> How about version_ - version < 0x80000000 ?

Done. Inverted, since you pointed out that 'equal' can skip (not for Pull
though).

https://codereview.chromium.org/180723023/diff/180001/gpu/command_buffer/serv...
gpu/command_buffer/service/texture_definition.h:114:
scoped_refptr<NativeImageBuffer> image_;
On 2014/03/13 04:41:58, piman wrote:
> nit: image_buffer_ maybe? The callsites are confusing, you expect image_ to be
a
> GLImage.

Done.

https://codereview.chromium.org/180723023/diff/180001/gpu/command_buffer/serv...
File gpu/command_buffer/service/texture_manager.cc (right):

https://codereview.chromium.org/180723023/diff/180001/gpu/command_buffer/serv...
gpu/command_buffer/service/texture_manager.cc:812: }
On 2014/03/13 04:41:58, piman wrote:
> Won't that break cube maps?

I noticed recently that we actually don't support that correctly because we are
confused between texture target (GL_TEXTURE_CUBE_MAP) and face index.

I added an early-out in DoBindTexImage2DCHROMIUM() +
GLES2DecoderTest.BindTexImage2DCHROMIUMCubeMapNotAllowed for the former, while
the validators for glBindTexImage2D already take care of the latter (see
cmd_buffer_functions.txt specifying the argument as 'GLenumTextureBindTarget').

https://codereview.chromium.org/180723023/diff/180001/ui/gl/gl_fence.cc
File ui/gl/gl_fence.cc (right):

https://codereview.chromium.org/180723023/diff/180001/ui/gl/gl_fence.cc#newco...
ui/gl/gl_fence.cc:135: return new GLFenceARBSync(flush);
On 2014/03/13 04:41:58, piman wrote:
> Because the NV_fence implementation degrades to a client wait for the server
> wait, we should prefer the ARB_sync version.

Done in https://codereview.chromium.org/195763015

[piman, 2014-03-13 23:29:05]

LGTM except for some minor points.

https://codereview.chromium.org/180723023/diff/180001/gpu/command_buffer/serv...
File gpu/command_buffer/service/texture_definition.cc (right):

https://codereview.chromium.org/180723023/diff/180001/gpu/command_buffer/serv...
gpu/command_buffer/service/texture_definition.cc:121: }
On 2014/03/13 20:50:15, sievers wrote:
> On 2014/03/13 04:41:58, piman wrote:
> > Can these conditions happen? Should they just be DCHECKs?
> 
> You mean no context or display, right?

Yes, thanks.

https://codereview.chromium.org/180723023/diff/180001/gpu/command_buffer/serv...
gpu/command_buffer/service/texture_definition.cc:287: // of perf when there are
multiple draw calls per frame.
On 2014/03/13 20:50:15, sievers wrote:
> On 2014/03/13 04:41:58, piman wrote:
> > So, the risk is that if we're missing a sync point somewhere, we might
> deadlock
> > the GPU if we have one write and one read, and no glFlush in the middle.
> > 
> > Would it be possible to, say, not "publish" the write fence until we push?
> I.e.
> > make the write_fence a property of the TextureDefinition? We might even be
> able
> > to delay the inserting of the fence until the Push rather than the draw,
since
> > otherwise you'll insert one fence for every draw, which seems heavy.
> > 
> > We would need to tag the NativeImageBuffer as "dirty" in DidWrite, then on
> Push,
> > look for dirty NativeImageBuffers (and treat them as a version update), and
if
> > any, lazily insert a single fence. Then on Pull, give the fence (if any) to
> the
> > GLImageSync (we may need refcounting...), which will then wait on it before
> read
> > or write.
> > 
> > WDYT?
> 
> Can I owe this as an immediate follow-up?
> I was also considering to expose this functionality in GLFence. Maybe the
fence
> could be notified about flushes by the GL api?
> 

Maybe file a bug and reference here... It's not something I would typically want
to have in the GPU process, depending on client-side behavior for correctness. I
guess this would only happen on WebView for now where the security aspects are
different, but I don't want us to start relying on this in other cases.

https://codereview.chromium.org/180723023/diff/180001/gpu/command_buffer/serv...
gpu/command_buffer/service/texture_definition.cc:385: glFlush();
On 2014/03/13 20:50:15, sievers wrote:
> On 2014/03/13 04:41:58, piman wrote:
> > DO you need this glFlush?
> 
> Most of the time probably not. There is one case I'm not sure about wrt
> compatibility to share group mailbox semantics:
> 
> ctx1 is in driver share group A
> ctx2a and ctx2b are in driver share group B
> 
> ctx1 defines the texture and flushes to make changes visible, then produces
> ctx2a consumes and then shares with ctx2b
> 
> With existing mailbox semantics ctx2a does not need to flush because it's not
> making changes to the texture before sharing it with ctx2b.
> But with this we defined a new texture in ctx2a and it wouldn't be necessarily
> visible to ctx2b, right?

I see... arguably, ctx2a needs to flush after the Consume to share with ctx2b.
But in any case, that shouldn't necessarily be required for subsequent updates,
so I could see this.
Maybe add a comment?

https://codereview.chromium.org/180723023/diff/180001/gpu/command_buffer/serv...
File gpu/command_buffer/service/texture_definition.h (right):

https://codereview.chromium.org/180723023/diff/180001/gpu/command_buffer/serv...
gpu/command_buffer/service/texture_definition.h:80: return (int)(version_ -
version) > 0;
On 2014/03/13 20:50:15, sievers wrote:
> On 2014/03/13 04:41:58, piman wrote:
> > int overflow = undefined. I'm afraid the compiler can legally treat this as
> > version_ != version.
> > How about version_ - version < 0x80000000 ?
> 
> Done. Inverted, since you pointed out that 'equal' can skip (not for Pull
> though).

Why can't we skip for Pull?

https://codereview.chromium.org/180723023/diff/200001/gpu/command_buffer/serv...
File gpu/command_buffer/service/mailbox_synchronizer.cc (right):

https://codereview.chromium.org/180723023/diff/200001/gpu/command_buffer/serv...
gpu/command_buffer/service/mailbox_synchronizer.cc:194: if
(definition.IsOlderThan(it->second.version))
IsOlderOrEqual? ;)

You can just add a || definition.version == it->second.version

[no sievers, 2014-03-13 23:51:13]

https://codereview.chromium.org/180723023/diff/180001/gpu/command_buffer/serv...
File gpu/command_buffer/service/texture_definition.cc (right):

https://codereview.chromium.org/180723023/diff/180001/gpu/command_buffer/serv...
gpu/command_buffer/service/texture_definition.cc:287: // of perf when there are
multiple draw calls per frame.
On 2014/03/13 23:29:06, piman wrote:
> On 2014/03/13 20:50:15, sievers wrote:
> > On 2014/03/13 04:41:58, piman wrote:
> > > So, the risk is that if we're missing a sync point somewhere, we might
> > deadlock
> > > the GPU if we have one write and one read, and no glFlush in the middle.
> > > 
> > > Would it be possible to, say, not "publish" the write fence until we push?
> > I.e.
> > > make the write_fence a property of the TextureDefinition? We might even be
> > able
> > > to delay the inserting of the fence until the Push rather than the draw,
> since
> > > otherwise you'll insert one fence for every draw, which seems heavy.
> > > 
> > > We would need to tag the NativeImageBuffer as "dirty" in DidWrite, then on
> > Push,
> > > look for dirty NativeImageBuffers (and treat them as a version update),
and
> if
> > > any, lazily insert a single fence. Then on Pull, give the fence (if any)
to
> > the
> > > GLImageSync (we may need refcounting...), which will then wait on it
before
> > read
> > > or write.
> > > 
> > > WDYT?
> > 
> > Can I owe this as an immediate follow-up?
> > I was also considering to expose this functionality in GLFence. Maybe the
> fence
> > could be notified about flushes by the GL api?
> > 
> 
> Maybe file a bug and reference here... It's not something I would typically
want
> to have in the GPU process, depending on client-side behavior for correctness.
I
> guess this would only happen on WebView for now where the security aspects are
> different, but I don't want us to start relying on this in other cases.

Done.

https://codereview.chromium.org/180723023/diff/180001/gpu/command_buffer/serv...
gpu/command_buffer/service/texture_definition.cc:385: glFlush();
On 2014/03/13 23:29:06, piman wrote:
> On 2014/03/13 20:50:15, sievers wrote:
> > On 2014/03/13 04:41:58, piman wrote:
> > > DO you need this glFlush?
> > 
> > Most of the time probably not. There is one case I'm not sure about wrt
> > compatibility to share group mailbox semantics:
> > 
> > ctx1 is in driver share group A
> > ctx2a and ctx2b are in driver share group B
> > 
> > ctx1 defines the texture and flushes to make changes visible, then produces
> > ctx2a consumes and then shares with ctx2b
> > 
> > With existing mailbox semantics ctx2a does not need to flush because it's
not
> > making changes to the texture before sharing it with ctx2b.
> > But with this we defined a new texture in ctx2a and it wouldn't be
necessarily
> > visible to ctx2b, right?
> 
> I see... arguably, ctx2a needs to flush after the Consume to share with ctx2b.
> But in any case, that shouldn't necessarily be required for subsequent
updates,
> so I could see this.
> Maybe add a comment?

Done. Maybe we could be smarter about when the flush is needed.

https://codereview.chromium.org/180723023/diff/200001/gpu/command_buffer/serv...
File gpu/command_buffer/service/mailbox_synchronizer.cc (right):

https://codereview.chromium.org/180723023/diff/200001/gpu/command_buffer/serv...
gpu/command_buffer/service/mailbox_synchronizer.cc:194: if
(definition.IsOlderThan(it->second.version))
On 2014/03/13 23:29:06, piman wrote:
> IsOlderOrEqual? ;)
> 
> You can just add a || definition.version == it->second.version

Oops, you are right, thanks! Done.

[no sievers, 2014-03-13 23:52:31]

The CQ bit was checked by sievers@chromium.org

[commit-bot: I haz the power, 2014-03-13 23:53:15]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sievers@chromium.org/180723023/230001

[commit-bot: I haz the power, 2014-03-14 00:08:58]

Change committed as 256962