gpu: Change Produce/ConsumeTexture to allow texture sharing

gpu: Change Produce/ConsumeTexture to allow texture sharing

This changes the semantics of ProduceTexture to not replacing the current texture by a dud, but instead keeping the existing one, that it also puts into the mailbox. It changes the semantics of ConsumeTexture to deleting the current texture, and replacing it by the mailbox contents (without taking it out of the mailbox). The texture becomes shared. The mailbox is now effectively a weak pointer onto the texture.

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

[Justin Novosad, 2013-04-29 14:29:18]

To successfully build on Windows:
In texture_manager.h, line 121 (function IsValid())

Change "return target()" to "return target() != 0;"

[piman, 2013-05-23 02:33:01]

gman/apatrick: gpu/
sievers: texture_image_transport_surface

The most subtle changes are in texture_image_transport_surface, the rest should
be fairly straightforward.

https://codereview.chromium.org/14188053/diff/62001/content/common/gpu/textur...
File content/common/gpu/texture_image_transport_surface.cc (left):

https://codereview.chromium.org/14188053/diff/62001/content/common/gpu/textur...
content/common/gpu/texture_image_transport_surface.cc:123: CreateBackTexture();
note: Construction moved to GetBackingFrameBufferObject because
CreateBackTexture needs the TextureManager to be created (to be able to create
the TextureRef), but the first MakeCurrent happens before that.

https://codereview.chromium.org/14188053/diff/62001/gpu/command_buffer/servic...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

https://codereview.chromium.org/14188053/diff/62001/gpu/command_buffer/servic...
gpu/command_buffer/service/gles2_cmd_decoder.cc:2410: back_buffer_has_stencil_ =
attrib_parser.stencil_size_ != 0 && v > 0;
These values are only used in the !offscreen case, so I moved them to this
block, and made sure the fbo (if needed) is created and current, because it
impacts the values.

[greggman, 2013-05-23 17:36:19]

https://codereview.chromium.org/14188053/diff/62001/content/common/gpu/textur...
File content/common/gpu/texture_image_transport_surface.cc (right):

https://codereview.chromium.org/14188053/diff/62001/content/common/gpu/textur...
content/common/gpu/texture_image_transport_surface.cc:203: glFlush();
This is probably a dumb question since I'm not clear on the entire flow but when
we get here we know this flush is for the correct context? Same on
ReleaseFrontTexture and ReleaseBackTexture

https://codereview.chromium.org/14188053/diff/62001/gpu/command_buffer/servic...
File gpu/command_buffer/service/texture_manager.cc (right):

https://codereview.chromium.org/14188053/diff/62001/gpu/command_buffer/servic...
gpu/command_buffer/service/texture_manager.cc:1027: DCHECK(result);
Seems like there needs to be some bookkeeping calls right? If you're inserting a
new texture and it's unrenderable the unrenderable count needs to be updated
etc...

I'm wondering it Consume would be better reusing the same ref? Something like

   
   Textures::Iter it = textures_.find(client_id);
   if (it != end()) {
        TextureRef ref = it->second
        ref->set_texture(texture);
   }

Then all the refs in this context are updated.

This would also auto delete the old texture is calling ref->set_texture release
the last reference

Right now it seems like this would get out of sync

  ctx2->genTex(1, &t);
  ctx2->bindTex(..., t);
  ctx2->framebufferTexture2D(,,,,t);
  ctx2->consume(...)

Because the framebuffer will have a scoped_refptr to the old TextureRef and so
will any texture_units 

Of course sadly, even if we do it that way the real FBO on the driver still has
the old texture bound and so do the texture units. Not sure what to do about
that. For the texture units we could set a flag to call
RestoreAllTextureUnitBindings before the next draw

For FBOs, we could make it an error to consume if the texture being replaced is
attached to anything.

https://codereview.chromium.org/14188053/diff/62001/gpu/command_buffer/servic...
File gpu/command_buffer/service/texture_manager.h (right):

https://codereview.chromium.org/14188053/diff/62001/gpu/command_buffer/servic...
gpu/command_buffer/service/texture_manager.h:411: GLuint client_id() const {
return client_id_; }
I'm not sure I follow the need for this. Is it only for testing? Then there's
also the reset_client_id as well but there's still going to be a mapping from
client_id to TextureRef in textures_.

[piman, 2013-05-23 18:02:37]

https://codereview.chromium.org/14188053/diff/62001/content/common/gpu/textur...
File content/common/gpu/texture_image_transport_surface.cc (right):

https://codereview.chromium.org/14188053/diff/62001/content/common/gpu/textur...
content/common/gpu/texture_image_transport_surface.cc:203: glFlush();
On 2013/05/23 17:36:19, greggman wrote:
> This is probably a dumb question since I'm not clear on the entire flow but
when
> we get here we know this flush is for the correct context? Same on
> ReleaseFrontTexture and ReleaseBackTexture

SwapBuffers and SetBackbufferAllocation are called directly from the decoder, so
the right context is current. That's true of most calls here.

It's not true for a couple of them though: BufferPresentedImpl is called from a
message (or a posted task if we need to wait for the sync point), and
SetFrontbufferAllocation is called from a posted task, so they need to make the
context current.

https://codereview.chromium.org/14188053/diff/62001/gpu/command_buffer/servic...
File gpu/command_buffer/service/texture_manager.cc (right):

https://codereview.chromium.org/14188053/diff/62001/gpu/command_buffer/servic...
gpu/command_buffer/service/texture_manager.cc:1027: DCHECK(result);
On 2013/05/23 17:36:19, greggman wrote:
> Seems like there needs to be some bookkeeping calls right? If you're inserting
a
> new texture and it's unrenderable the unrenderable count needs to be updated
> etc...

This is done when we create the TextureRef - it calls StartTracking in the
constructor.

> 
> I'm wondering it Consume would be better reusing the same ref? Something like
> 
>    
>    Textures::Iter it = textures_.find(client_id);
>    if (it != end()) {
>         TextureRef ref = it->second
>         ref->set_texture(texture);
>    }
> 
> Then all the refs in this context are updated.

The problem is state in other contexts and FBO attachments. The service id for a
texture ref changes, so if we want to do that, we need to keep track everywhere
that we need to do something in real GL terms - e.g. when we switch to a new
context, we need to rebind all texture units (or keep track of which one refers
to this texture ref), and reattach all FBOs.

> 
> This would also auto delete the old texture is calling ref->set_texture
release
> the last reference
> 
> Right now it seems like this would get out of sync
> 
>   ctx2->genTex(1, &t);
>   ctx2->bindTex(..., t);
>   ctx2->framebufferTexture2D(,,,,t);
>   ctx2->consume(...)
> 
> Because the framebuffer will have a scoped_refptr to the old TextureRef and so
> will any texture_units 
> 
> Of course sadly, even if we do it that way the real FBO on the driver still
has
> the old texture bound and so do the texture units. Not sure what to do about
> that. For the texture units we could set a flag to call
> RestoreAllTextureUnitBindings before the next draw
> 
> For FBOs, we could make it an error to consume if the texture being replaced
is
> attached to anything.

Right, my thinking is to make this explicit, and responsibility to the client:
consume deletes the current texture and creates a new one. It removes
implementation headaches, at the cost of what could be seen as a small quirk on
the client side. Delete has very well-defined semantics (resets units in current
context and attachments in current fbo, other ones keep a reference).

In practice, the quirk is not a problem: in our usage patterns, clients would
consume once, on a newly created texture. So I'm not sure it's worth it to add
implementation complexity for something that doesn't matter on the client side.

https://codereview.chromium.org/14188053/diff/62001/gpu/command_buffer/servic...
File gpu/command_buffer/service/texture_manager.h (right):

https://codereview.chromium.org/14188053/diff/62001/gpu/command_buffer/servic...
gpu/command_buffer/service/texture_manager.h:411: GLuint client_id() const {
return client_id_; }
On 2013/05/23 17:36:19, greggman wrote:
> I'm not sure I follow the need for this. Is it only for testing? Then there's
> also the reset_client_id as well but there's still going to be a mapping from
> client_id to TextureRef in textures_.  

It's there to be able to do the Delete in Consume. We could go through the list,
but since we're likely going to Consume somewhat often on a context with many
textures, this seems heavy.

reset_client_id is called when we remove the TextureRef from textures_. That
way, if we delete the texture in one context and the texture is bound in
another, if we switch to the other context and call Consume, we won't try to
delete an unrelated texture (if the client id was reused in the mean time).

[apatrick_chromium, 2013-05-23 18:37:11]

I like it. You will have to mostly rewrite the specification now :)

https://codereview.chromium.org/14188053/diff/62001/gpu/command_buffer/servic...
File gpu/command_buffer/service/mailbox_manager.cc (right):

https://codereview.chromium.org/14188053/diff/62001/gpu/command_buffer/servic...
gpu/command_buffer/service/mailbox_manager.cc:71: while (it != textures_.end())
{
This might now be a more common operation. Before this would only happen on
context group destruction. Now if happens whenever the last ref to a Texture
goes away. Could add a multiset with the reverse mapping Texture->TargetName or
something. Maybe not for this patch.

https://codereview.chromium.org/14188053/diff/62001/gpu/command_buffer/servic...
File gpu/command_buffer/service/texture_manager_unittest.cc (right):

https://codereview.chromium.org/14188053/diff/62001/gpu/command_buffer/servic...
gpu/command_buffer/service/texture_manager_unittest.cc:1433: Texture*
Save(TextureRef* texture_ref) {
Want to rename these to Produce/Consume as well?

[greggman, 2013-05-23 18:38:26]

https://codereview.chromium.org/14188053/diff/62001/gpu/command_buffer/servic...
File gpu/command_buffer/service/texture_manager.cc (right):

https://codereview.chromium.org/14188053/diff/62001/gpu/command_buffer/servic...
gpu/command_buffer/service/texture_manager.cc:1027: DCHECK(result);
On 2013/05/23 18:02:38, piman wrote:
> On 2013/05/23 17:36:19, greggman wrote:
> > Seems like there needs to be some bookkeeping calls right? If you're
inserting
> a
> > new texture and it's unrenderable the unrenderable count needs to be updated
> > etc...
> 
> This is done when we create the TextureRef - it calls StartTracking in the
> constructor.
> 
> > 
> > I'm wondering it Consume would be better reusing the same ref? Something
like
> > 
> >    
> >    Textures::Iter it = textures_.find(client_id);
> >    if (it != end()) {
> >         TextureRef ref = it->second
> >         ref->set_texture(texture);
> >    }
> > 
> > Then all the refs in this context are updated.
> 
> The problem is state in other contexts and FBO attachments. The service id for
a
> texture ref changes, so if we want to do that, we need to keep track
everywhere
> that we need to do something in real GL terms - e.g. when we switch to a new
> context, we need to rebind all texture units (or keep track of which one
refers
> to this texture ref), and reattach all FBOs.

I'm confused. TextureRefs are per context so changing a TextureRef in this
context to point to a new texture doesn't affect other contexts right?

It seems like making this change would delete about 20 lines of code from this
CL making it simpler. In GLES2DecoderImpl::::DoConsumeTextureCHROMIUM the call
to DeleteTextureHelper before calling texture_manager_->Consume and all the code
after to reset texture refs disappears as does the bind. It just becomes


if (!current_texture->IsAttachedToFramebuffer()) {
   texture-manager->Consume(client_id, texture_being_consumed);
   RestoreAllTextureUnitBindings();  
}

The code here just becomes

  TextureRef* ref = GetTexture(client_id);
  DCHECK(ref);
  ref->SetTexture(this, texture_to_consume);

And SetTexture is

TextureRef::SetTexture(TextureManager* manager, Texture* texture) {
  manager->StopTracking(this);
  texture_->RemoveTextureRef(this);  
  texture_ = texture;
  texture_->AddTexturRef(this);
  manager_->StartTracking(this);
}

And suddenly there's no path for the client to have weird situations where units
are pointing to the wrong stuff.

Does that make any sense or am I missing something?


> 
> > 
> > This would also auto delete the old texture is calling ref->set_texture
> release
> > the last reference
> > 
> > Right now it seems like this would get out of sync
> > 
> >   ctx2->genTex(1, &t);
> >   ctx2->bindTex(..., t);
> >   ctx2->framebufferTexture2D(,,,,t);
> >   ctx2->consume(...)
> > 
> > Because the framebuffer will have a scoped_refptr to the old TextureRef and
so
> > will any texture_units 
> > 
> > Of course sadly, even if we do it that way the real FBO on the driver still
> has
> > the old texture bound and so do the texture units. Not sure what to do about
> > that. For the texture units we could set a flag to call
> > RestoreAllTextureUnitBindings before the next draw
> > 
> > For FBOs, we could make it an error to consume if the texture being replaced
> is
> > attached to anything.
> 
> Right, my thinking is to make this explicit, and responsibility to the client:
> consume deletes the current texture and creates a new one. It removes
> implementation headaches, at the cost of what could be seen as a small quirk
on
> the client side. Delete has very well-defined semantics (resets units in
current
> context and attachments in current fbo, other ones keep a reference).
> 
> In practice, the quirk is not a problem: in our usage patterns, clients would
> consume once, on a newly created texture. So I'm not sure it's worth it to add
> implementation complexity for something that doesn't matter on the client
side.

[piman, 2013-05-23 19:32:32]

On Thu, May 23, 2013 at 11:38 AM, <gman@chromium.org> wrote:

>
> https://codereview.chromium.**org/14188053/diff/62001/gpu/**
>
command_buffer/service/**texture_manager.cc<https://codereview.chromium.org/14188053/diff/62001/gpu/command_buffer/service/texture_manager.cc>
> File gpu/command_buffer/service/**texture_manager.cc (right):
>
> https://codereview.chromium.**org/14188053/diff/62001/gpu/**
>
command_buffer/service/**texture_manager.cc#newcode1027<https://codereview.chromium.org/14188053/diff/62001/gpu/command_buffer/service/texture_manager.cc#newcode1027>
> gpu/command_buffer/service/**texture_manager.cc:1027: DCHECK(result);
> On 2013/05/23 18:02:38, piman wrote:
>
>> On 2013/05/23 17:36:19, greggman wrote:
>> > Seems like there needs to be some bookkeeping calls right? If you're
>>
> inserting
>
>> a
>> > new texture and it's unrenderable the unrenderable count needs to be
>>
> updated
>
>> > etc...
>>
>
>  This is done when we create the TextureRef - it calls StartTracking in
>>
> the
>
>> constructor.
>>
>
>  >
>> > I'm wondering it Consume would be better reusing the same ref?
>>
> Something like
>
>> >
>> >
>> >    Textures::Iter it = textures_.find(client_id);
>> >    if (it != end()) {
>> >         TextureRef ref = it->second
>> >         ref->set_texture(texture);
>> >    }
>> >
>> > Then all the refs in this context are updated.
>>
>
>  The problem is state in other contexts and FBO attachments. The
>>
> service id for a
>
>> texture ref changes, so if we want to do that, we need to keep track
>>
> everywhere
>
>> that we need to do something in real GL terms - e.g. when we switch to
>>
> a new
>
>> context, we need to rebind all texture units (or keep track of which
>>
> one refers
>
>> to this texture ref), and reattach all FBOs.
>>
>
> I'm confused. TextureRefs are per context so changing a TextureRef in
> this context to point to a new texture doesn't affect other contexts
> right?
>

TextureRefs are per share group, so it'll affect other contexts in the same
share group.


>
> It seems like making this change would delete about 20 lines of code
> from this CL making it simpler. In
> GLES2DecoderImpl::::**DoConsumeTextureCHROMIUM the call to
> DeleteTextureHelper before calling texture_manager_->Consume and all the
> code after to reset texture refs disappears as does the bind. It just
> becomes
>
>
> if (!current_texture->**IsAttachedToFramebuffer()) {
>    texture-manager->Consume(**client_id, texture_being_consumed);
>    RestoreAllTextureUnitBindings(**);
> }


> The code here just becomes
>
>   TextureRef* ref = GetTexture(client_id);
>   DCHECK(ref);
>   ref->SetTexture(this, texture_to_consume);
>
> And SetTexture is
>
> TextureRef::SetTexture(**TextureManager* manager, Texture* texture) {
>   manager->StopTracking(this);
>   texture_->RemoveTextureRef(**this);
>   texture_ = texture;
>   texture_->AddTexturRef(this);
>   manager_->StartTracking(this);
> }
>
> And suddenly there's no path for the client to have weird situations
> where units are pointing to the wrong stuff.
>
> Does that make any sense or am I missing something?


I actually had that in a previous version. This one I'm a lot more
confident is right.
Right now, on top of the other contexts, there is also the matter of the
async uploads, which are a bit bogus until crbug.com/240504 is fixed (right
now they're a per-TextureRef thing but affect the Texture. It becomes
possible to violate the assumption that only one TextureRef has an async
upload on the texture). I'd need to figure out if it's possible to fix this
as is, otherwise we have to wait for backer's work on this.
I also think it's actually easier to reason about on the client side ("my
texture will be deleted" vs "the call may fail if the texture is bound to a
FBO on another context"), but it's a matter of taste.


>
>
>
>  >
>> > This would also auto delete the old texture is calling
>>
> ref->set_texture
>
>> release
>> > the last reference
>> >
>> > Right now it seems like this would get out of sync
>> >
>> >   ctx2->genTex(1, &t);
>> >   ctx2->bindTex(..., t);
>> >   ctx2->framebufferTexture2D(,,,**,t);
>> >   ctx2->consume(...)
>> >
>> > Because the framebuffer will have a scoped_refptr to the old
>>
> TextureRef and so
>
>> > will any texture_units
>> >
>> > Of course sadly, even if we do it that way the real FBO on the
>>
> driver still
>
>> has
>> > the old texture bound and so do the texture units. Not sure what to
>>
> do about
>
>> > that. For the texture units we could set a flag to call
>> > RestoreAllTextureUnitBindings before the next draw
>> >
>> > For FBOs, we could make it an error to consume if the texture being
>>
> replaced
>
>> is
>> > attached to anything.
>>
>
>  Right, my thinking is to make this explicit, and responsibility to the
>>
> client:
>
>> consume deletes the current texture and creates a new one. It removes
>> implementation headaches, at the cost of what could be seen as a small
>>
> quirk on
>
>> the client side. Delete has very well-defined semantics (resets units
>>
> in current
>
>> context and attachments in current fbo, other ones keep a reference).
>>
>
>  In practice, the quirk is not a problem: in our usage patterns,
>>
> clients would
>
>> consume once, on a newly created texture. So I'm not sure it's worth
>>
> it to add
>
>> implementation complexity for something that doesn't matter on the
>>
> client side.
>
>
https://codereview.chromium.**org/14188053/<https://codereview.chromium.org/1...
>

[greggman, 2013-05-23 20:00:57]

On 2013/05/23 19:32:32, piman wrote:
> On Thu, May 23, 2013 at 11:38 AM, <mailto:gman@chromium.org> wrote:
> 
> >
> > https://codereview.chromium.**org/14188053/diff/62001/gpu/**
> >
>
command_buffer/service/**texture_manager.cc<https://codereview.chromium.org/14188053/diff/62001/gpu/command_buffer/service/texture_manager.cc>
> > File gpu/command_buffer/service/**texture_manager.cc (right):
> >
> > https://codereview.chromium.**org/14188053/diff/62001/gpu/**
> >
>
command_buffer/service/**texture_manager.cc#newcode1027<https://codereview.chromium.org/14188053/diff/62001/gpu/command_buffer/service/texture_manager.cc#newcode1027>
> > gpu/command_buffer/service/**texture_manager.cc:1027: DCHECK(result);
> > On 2013/05/23 18:02:38, piman wrote:
> >
> >> On 2013/05/23 17:36:19, greggman wrote:
> >> > Seems like there needs to be some bookkeeping calls right? If you're
> >>
> > inserting
> >
> >> a
> >> > new texture and it's unrenderable the unrenderable count needs to be
> >>
> > updated
> >
> >> > etc...
> >>
> >
> >  This is done when we create the TextureRef - it calls StartTracking in
> >>
> > the
> >
> >> constructor.
> >>
> >
> >  >
> >> > I'm wondering it Consume would be better reusing the same ref?
> >>
> > Something like
> >
> >> >
> >> >
> >> >    Textures::Iter it = textures_.find(client_id);
> >> >    if (it != end()) {
> >> >         TextureRef ref = it->second
> >> >         ref->set_texture(texture);
> >> >    }
> >> >
> >> > Then all the refs in this context are updated.
> >>
> >
> >  The problem is state in other contexts and FBO attachments. The
> >>
> > service id for a
> >
> >> texture ref changes, so if we want to do that, we need to keep track
> >>
> > everywhere
> >
> >> that we need to do something in real GL terms - e.g. when we switch to
> >>
> > a new
> >
> >> context, we need to rebind all texture units (or keep track of which
> >>
> > one refers
> >
> >> to this texture ref), and reattach all FBOs.
> >>
> >
> > I'm confused. TextureRefs are per context so changing a TextureRef in
> > this context to point to a new texture doesn't affect other contexts
> > right?
> >
> 
> TextureRefs are per share group, so it'll affect other contexts in the same
> share group.

That's what I was forgetting. Doh

I'm not super comfortable about the weird dangling references. I'm sure someone
in a year or two will commit a CL that assumes that things work and might just
be getting lucky that they do.

Even the GL spec requires just one rebinding to make all bindings work. In other
words, if we followed the GL spec this

   for (i = 0 to 10) {
    glActiveTexture(GL_TEXTURE0 + i);
    glBindTexture(GL_TEXTURE_2D, tex);
   }
   glConsumeTextureCHROMIM(tex, mailbox_id);
   glBindTexture(GL_TEXTURE_2D, tex);

Would make all the texture units work but in our case only the one of those 10
units will be correct.

If not now maybe we should make a todo to fix that? Set some flag per context
(could add ContextGroup::MarkAllContextsTextureUnitStateDirty() which could set
a flag in each decoder and then GLES2DecoderImpl::ApplyDirtyState could call
RestoreAllTextureUnitBindings if the flag is set.

That doesn't seem like a lot of code to add. Then would the suggested solution
work? There's still the issue of fbo attachments. We could restrict them now and
add RestoreAllFboAttachments later. 

What do you think?

> 
> 
> >
> > It seems like making this change would delete about 20 lines of code
> > from this CL making it simpler. In
> > GLES2DecoderImpl::::**DoConsumeTextureCHROMIUM the call to
> > DeleteTextureHelper before calling texture_manager_->Consume and all the
> > code after to reset texture refs disappears as does the bind. It just
> > becomes
> >
> >
> > if (!current_texture->**IsAttachedToFramebuffer()) {
> >    texture-manager->Consume(**client_id, texture_being_consumed);
> >    RestoreAllTextureUnitBindings(**);
> > }
> 
> 
> > The code here just becomes
> >
> >   TextureRef* ref = GetTexture(client_id);
> >   DCHECK(ref);
> >   ref->SetTexture(this, texture_to_consume);
> >
> > And SetTexture is
> >
> > TextureRef::SetTexture(**TextureManager* manager, Texture* texture) {
> >   manager->StopTracking(this);
> >   texture_->RemoveTextureRef(**this);
> >   texture_ = texture;
> >   texture_->AddTexturRef(this);
> >   manager_->StartTracking(this);
> > }
> >
> > And suddenly there's no path for the client to have weird situations
> > where units are pointing to the wrong stuff.
> >
> > Does that make any sense or am I missing something?
> 
> 
> I actually had that in a previous version. This one I'm a lot more
> confident is right.
> Right now, on top of the other contexts, there is also the matter of the
> async uploads, which are a bit bogus until crbug.com/240504 is fixed (right
> now they're a per-TextureRef thing but affect the Texture. It becomes
> possible to violate the assumption that only one TextureRef has an async
> upload on the texture). I'd need to figure out if it's possible to fix this
> as is, otherwise we have to wait for backer's work on this.
> I also think it's actually easier to reason about on the client side ("my
> texture will be deleted" vs "the call may fail if the texture is bound to a
> FBO on another context"), but it's a matter of taste.
> 
> 
> >
> >
> >
> >  >
> >> > This would also auto delete the old texture is calling
> >>
> > ref->set_texture
> >
> >> release
> >> > the last reference
> >> >
> >> > Right now it seems like this would get out of sync
> >> >
> >> >   ctx2->genTex(1, &t);
> >> >   ctx2->bindTex(..., t);
> >> >   ctx2->framebufferTexture2D(,,,**,t);
> >> >   ctx2->consume(...)
> >> >
> >> > Because the framebuffer will have a scoped_refptr to the old
> >>
> > TextureRef and so
> >
> >> > will any texture_units
> >> >
> >> > Of course sadly, even if we do it that way the real FBO on the
> >>
> > driver still
> >
> >> has
> >> > the old texture bound and so do the texture units. Not sure what to
> >>
> > do about
> >
> >> > that. For the texture units we could set a flag to call
> >> > RestoreAllTextureUnitBindings before the next draw
> >> >
> >> > For FBOs, we could make it an error to consume if the texture being
> >>
> > replaced
> >
> >> is
> >> > attached to anything.
> >>
> >
> >  Right, my thinking is to make this explicit, and responsibility to the
> >>
> > client:
> >
> >> consume deletes the current texture and creates a new one. It removes
> >> implementation headaches, at the cost of what could be seen as a small
> >>
> > quirk on
> >
> >> the client side. Delete has very well-defined semantics (resets units
> >>
> > in current
> >
> >> context and attachments in current fbo, other ones keep a reference).
> >>
> >
> >  In practice, the quirk is not a problem: in our usage patterns,
> >>
> > clients would
> >
> >> consume once, on a newly created texture. So I'm not sure it's worth
> >>
> > it to add
> >
> >> implementation complexity for something that doesn't matter on the
> >>
> > client side.
> >
> >
>
https://codereview.chromium.**org/14188053/%3Chttps://codereview.chromium.org...>
> >

[no sievers, 2013-05-24 01:31:38]

> sievers: texture_image_transport_surface

lgtm. very exciting....

What happens to the texture image transport surface's texture refs when the
context (group) is lost? Anything that can blow up?

https://codereview.chromium.org/14188053/diff/62001/content/common/gpu/textur...
File content/common/gpu/texture_image_transport_surface.cc (right):

https://codereview.chromium.org/14188053/diff/62001/content/common/gpu/textur...
content/common/gpu/texture_image_transport_surface.cc:287: if (backbuffer_ &&
!mailbox_name.empty()) {
When would backbuffer_ ever be invalid here?
We can't have deallocated it while the ack was pending.

https://codereview.chromium.org/14188053/diff/62001/content/common/gpu/textur...
content/common/gpu/texture_image_transport_surface.cc:292: if (!memcmp(&name,
&back_mailbox_name_, sizeof(name)))
nit: maybe comment that the browser skipped/returned the frame in that case.

https://codereview.chromium.org/14188053/diff/62001/gpu/command_buffer/servic...
File gpu/command_buffer/service/mailbox_manager.cc (right):

https://codereview.chromium.org/14188053/diff/62001/gpu/command_buffer/servic...
gpu/command_buffer/service/mailbox_manager.cc:11: #include
"gpu/command_buffer/service/gl_utils.h"
nit: I think gl_utils.h is not needed anymore.

https://codereview.chromium.org/14188053/diff/62001/gpu/command_buffer/servic...
File gpu/command_buffer/service/texture_manager_unittest.cc (right):

https://codereview.chromium.org/14188053/diff/62001/gpu/command_buffer/servic...
gpu/command_buffer/service/texture_manager_unittest.cc:1446: static const GLuint
kEmptyTextureServiceId;
nit: kEmptyTextureServiceId is not used anymore

[piman, 2013-05-24 02:32:22]

https://codereview.chromium.org/14188053/diff/62001/content/common/gpu/textur...
File content/common/gpu/texture_image_transport_surface.cc (right):

https://codereview.chromium.org/14188053/diff/62001/content/common/gpu/textur...
content/common/gpu/texture_image_transport_surface.cc:203: glFlush();
On 2013/05/23 18:02:38, piman wrote:
> On 2013/05/23 17:36:19, greggman wrote:
> > This is probably a dumb question since I'm not clear on the entire flow but
> when
> > we get here we know this flush is for the correct context? Same on
> > ReleaseFrontTexture and ReleaseBackTexture
> 
> SwapBuffers and SetBackbufferAllocation are called directly from the decoder,
so
> the right context is current. That's true of most calls here.
> 
> It's not true for a couple of them though: BufferPresentedImpl is called from
a
> message (or a posted task if we need to wait for the sync point), and
> SetFrontbufferAllocation is called from a posted task, so they need to make
the
> context current.

Added a bunch of DCHECKs in functions that call GL to make that more explicit.

https://codereview.chromium.org/14188053/diff/62001/content/common/gpu/textur...
content/common/gpu/texture_image_transport_surface.cc:287: if (backbuffer_ &&
!mailbox_name.empty()) {
On 2013/05/24 01:31:38, Daniel Sievers wrote:
> When would backbuffer_ ever be invalid here?
> We can't have deallocated it while the ack was pending.

I can't remember why I did this originally (I had several passes), but you're
right. I replaced it back to a DCHECK.

https://codereview.chromium.org/14188053/diff/62001/content/common/gpu/textur...
content/common/gpu/texture_image_transport_surface.cc:292: if (!memcmp(&name,
&back_mailbox_name_, sizeof(name)))
On 2013/05/24 01:31:38, Daniel Sievers wrote:
> nit: maybe comment that the browser skipped/returned the frame in that case.

Done. (also simplified a bit, I don't need to copy into a MailboxName to
compare).

https://codereview.chromium.org/14188053/diff/62001/gpu/command_buffer/servic...
File gpu/command_buffer/service/mailbox_manager.cc (right):

https://codereview.chromium.org/14188053/diff/62001/gpu/command_buffer/servic...
gpu/command_buffer/service/mailbox_manager.cc:11: #include
"gpu/command_buffer/service/gl_utils.h"
On 2013/05/24 01:31:38, Daniel Sievers wrote:
> nit: I think gl_utils.h is not needed anymore.

Done.

https://codereview.chromium.org/14188053/diff/62001/gpu/command_buffer/servic...
gpu/command_buffer/service/mailbox_manager.cc:71: while (it != textures_.end())
{
On 2013/05/23 18:37:11, apatrick_chromium wrote:
> This might now be a more common operation. Before this would only happen on
> context group destruction. Now if happens whenever the last ref to a Texture
> goes away. Could add a multiset with the reverse mapping Texture->TargetName
or
> something. Maybe not for this patch.

Done.

https://codereview.chromium.org/14188053/diff/62001/gpu/command_buffer/servic...
File gpu/command_buffer/service/texture_manager_unittest.cc (right):

https://codereview.chromium.org/14188053/diff/62001/gpu/command_buffer/servic...
gpu/command_buffer/service/texture_manager_unittest.cc:1433: Texture*
Save(TextureRef* texture_ref) {
On 2013/05/23 18:37:11, apatrick_chromium wrote:
> Want to rename these to Produce/Consume as well?

Done.

https://codereview.chromium.org/14188053/diff/62001/gpu/command_buffer/servic...
gpu/command_buffer/service/texture_manager_unittest.cc:1446: static const GLuint
kEmptyTextureServiceId;
On 2013/05/24 01:31:38, Daniel Sievers wrote:
> nit: kEmptyTextureServiceId is not used anymore

Done.

[piman, 2013-05-24 02:39:08]

On 2013/05/23 20:00:57, greggman wrote:
> On 2013/05/23 19:32:32, piman wrote:
> > On Thu, May 23, 2013 at 11:38 AM, <mailto:gman@chromium.org> wrote:
> > 
> > >
> > > https://codereview.chromium.**org/14188053/diff/62001/gpu/**
> > >
> >
>
command_buffer/service/**texture_manager.cc<https://codereview.chromium.org/14188053/diff/62001/gpu/command_buffer/service/texture_manager.cc>
> > > File gpu/command_buffer/service/**texture_manager.cc (right):
> > >
> > > https://codereview.chromium.**org/14188053/diff/62001/gpu/**
> > >
> >
>
command_buffer/service/**texture_manager.cc#newcode1027<https://codereview.chromium.org/14188053/diff/62001/gpu/command_buffer/service/texture_manager.cc#newcode1027>
> > > gpu/command_buffer/service/**texture_manager.cc:1027: DCHECK(result);
> > > On 2013/05/23 18:02:38, piman wrote:
> > >
> > >> On 2013/05/23 17:36:19, greggman wrote:
> > >> > Seems like there needs to be some bookkeeping calls right? If you're
> > >>
> > > inserting
> > >
> > >> a
> > >> > new texture and it's unrenderable the unrenderable count needs to be
> > >>
> > > updated
> > >
> > >> > etc...
> > >>
> > >
> > >  This is done when we create the TextureRef - it calls StartTracking in
> > >>
> > > the
> > >
> > >> constructor.
> > >>
> > >
> > >  >
> > >> > I'm wondering it Consume would be better reusing the same ref?
> > >>
> > > Something like
> > >
> > >> >
> > >> >
> > >> >    Textures::Iter it = textures_.find(client_id);
> > >> >    if (it != end()) {
> > >> >         TextureRef ref = it->second
> > >> >         ref->set_texture(texture);
> > >> >    }
> > >> >
> > >> > Then all the refs in this context are updated.
> > >>
> > >
> > >  The problem is state in other contexts and FBO attachments. The
> > >>
> > > service id for a
> > >
> > >> texture ref changes, so if we want to do that, we need to keep track
> > >>
> > > everywhere
> > >
> > >> that we need to do something in real GL terms - e.g. when we switch to
> > >>
> > > a new
> > >
> > >> context, we need to rebind all texture units (or keep track of which
> > >>
> > > one refers
> > >
> > >> to this texture ref), and reattach all FBOs.
> > >>
> > >
> > > I'm confused. TextureRefs are per context so changing a TextureRef in
> > > this context to point to a new texture doesn't affect other contexts
> > > right?
> > >
> > 
> > TextureRefs are per share group, so it'll affect other contexts in the same
> > share group.
> 
> That's what I was forgetting. Doh
> 
> I'm not super comfortable about the weird dangling references. I'm sure
someone
> in a year or two will commit a CL that assumes that things work and might just
> be getting lucky that they do.
> 
> Even the GL spec requires just one rebinding to make all bindings work. In
other
> words, if we followed the GL spec this
> 
>    for (i = 0 to 10) {
>     glActiveTexture(GL_TEXTURE0 + i);
>     glBindTexture(GL_TEXTURE_2D, tex);
>    }
>    glConsumeTextureCHROMIM(tex, mailbox_id);
>    glBindTexture(GL_TEXTURE_2D, tex);
> 
> Would make all the texture units work but in our case only the one of those 10
> units will be correct.
> 
> If not now maybe we should make a todo to fix that? Set some flag per context
> (could add ContextGroup::MarkAllContextsTextureUnitStateDirty() which could
set
> a flag in each decoder and then GLES2DecoderImpl::ApplyDirtyState could call
> RestoreAllTextureUnitBindings if the flag is set.
> 
> That doesn't seem like a lot of code to add. Then would the suggested solution
> work? There's still the issue of fbo attachments. We could restrict them now
and
> add RestoreAllFboAttachments later. 
> 
> What do you think?

Ok, I don't like arbitrary restrictions which can be hard to reason about, but I
agree that it would make the semantics a bit easier, so I'm convinced.
Right now, because of the async textures mess, it makes it hard, but for now I
restricted FBOs and added a TODO. Once 240504 is fixed I'll revisit. I don't
think we'll be adding clients that rely on one behavior or the other.

> 
> > 
> > 
> > >
> > > It seems like making this change would delete about 20 lines of code
> > > from this CL making it simpler. In
> > > GLES2DecoderImpl::::**DoConsumeTextureCHROMIUM the call to
> > > DeleteTextureHelper before calling texture_manager_->Consume and all the
> > > code after to reset texture refs disappears as does the bind. It just
> > > becomes
> > >
> > >
> > > if (!current_texture->**IsAttachedToFramebuffer()) {
> > >    texture-manager->Consume(**client_id, texture_being_consumed);
> > >    RestoreAllTextureUnitBindings(**);
> > > }
> > 
> > 
> > > The code here just becomes
> > >
> > >   TextureRef* ref = GetTexture(client_id);
> > >   DCHECK(ref);
> > >   ref->SetTexture(this, texture_to_consume);
> > >
> > > And SetTexture is
> > >
> > > TextureRef::SetTexture(**TextureManager* manager, Texture* texture) {
> > >   manager->StopTracking(this);
> > >   texture_->RemoveTextureRef(**this);
> > >   texture_ = texture;
> > >   texture_->AddTexturRef(this);
> > >   manager_->StartTracking(this);
> > > }
> > >
> > > And suddenly there's no path for the client to have weird situations
> > > where units are pointing to the wrong stuff.
> > >
> > > Does that make any sense or am I missing something?
> > 
> > 
> > I actually had that in a previous version. This one I'm a lot more
> > confident is right.
> > Right now, on top of the other contexts, there is also the matter of the
> > async uploads, which are a bit bogus until crbug.com/240504 is fixed (right
> > now they're a per-TextureRef thing but affect the Texture. It becomes
> > possible to violate the assumption that only one TextureRef has an async
> > upload on the texture). I'd need to figure out if it's possible to fix this
> > as is, otherwise we have to wait for backer's work on this.
> > I also think it's actually easier to reason about on the client side ("my
> > texture will be deleted" vs "the call may fail if the texture is bound to a
> > FBO on another context"), but it's a matter of taste.
> > 
> > 
> > >
> > >
> > >
> > >  >
> > >> > This would also auto delete the old texture is calling
> > >>
> > > ref->set_texture
> > >
> > >> release
> > >> > the last reference
> > >> >
> > >> > Right now it seems like this would get out of sync
> > >> >
> > >> >   ctx2->genTex(1, &t);
> > >> >   ctx2->bindTex(..., t);
> > >> >   ctx2->framebufferTexture2D(,,,**,t);
> > >> >   ctx2->consume(...)
> > >> >
> > >> > Because the framebuffer will have a scoped_refptr to the old
> > >>
> > > TextureRef and so
> > >
> > >> > will any texture_units
> > >> >
> > >> > Of course sadly, even if we do it that way the real FBO on the
> > >>
> > > driver still
> > >
> > >> has
> > >> > the old texture bound and so do the texture units. Not sure what to
> > >>
> > > do about
> > >
> > >> > that. For the texture units we could set a flag to call
> > >> > RestoreAllTextureUnitBindings before the next draw
> > >> >
> > >> > For FBOs, we could make it an error to consume if the texture being
> > >>
> > > replaced
> > >
> > >> is
> > >> > attached to anything.
> > >>
> > >
> > >  Right, my thinking is to make this explicit, and responsibility to the
> > >>
> > > client:
> > >
> > >> consume deletes the current texture and creates a new one. It removes
> > >> implementation headaches, at the cost of what could be seen as a small
> > >>
> > > quirk on
> > >
> > >> the client side. Delete has very well-defined semantics (resets units
> > >>
> > > in current
> > >
> > >> context and attachments in current fbo, other ones keep a reference).
> > >>
> > >
> > >  In practice, the quirk is not a problem: in our usage patterns,
> > >>
> > > clients would
> > >
> > >> consume once, on a newly created texture. So I'm not sure it's worth
> > >>
> > > it to add
> > >
> > >> implementation complexity for something that doesn't matter on the
> > >>
> > > client side.
> > >
> > >
> >
>
https://codereview.chromium.**org/14188053/%253Chttps://codereview.chromium.o...>
> > >

[piman, 2013-05-24 02:45:04]

On Thu, May 23, 2013 at 7:39 PM, <piman@chromium.org> wrote:

> On 2013/05/23 20:00:57, greggman wrote:
>
>> On 2013/05/23 19:32:32, piman wrote:
>> > On Thu, May 23, 2013 at 11:38 AM, <mailto:gman@chromium.org> wrote:
>> >
>> > >
>> > > https://codereview.chromium.****org/14188053/diff/62001/gpu/**
>> > >
>> >
>>
>
> command_buffer/service/****texture_manager.cc<https://**
> codereview.chromium.org/**14188053/diff/62001/gpu/**
>
command_buffer/service/**texture_manager.cc<https://codereview.chromium.org/14188053/diff/62001/gpu/command_buffer/service/texture_manager.cc>
> >
>
>> > > File gpu/command_buffer/service/****texture_manager.cc (right):
>> > >
>> > > https://codereview.chromium.****org/14188053/diff/62001/gpu/**
>> > >
>> >
>>
>
> command_buffer/service/****texture_manager.cc#**newcode1027<https://**
> codereview.chromium.org/**14188053/diff/62001/gpu/**
>
command_buffer/service/**texture_manager.cc#newcode1027<https://codereview.chromium.org/14188053/diff/62001/gpu/command_buffer/service/texture_manager.cc#newcode1027>
> **>
>
>> > > gpu/command_buffer/service/****texture_manager.cc:1027:
>> DCHECK(result);
>>
>> > > On 2013/05/23 18:02:38, piman wrote:
>> > >
>> > >> On 2013/05/23 17:36:19, greggman wrote:
>> > >> > Seems like there needs to be some bookkeeping calls right? If
>> you're
>> > >>
>> > > inserting
>> > >
>> > >> a
>> > >> > new texture and it's unrenderable the unrenderable count needs to
>> be
>> > >>
>> > > updated
>> > >
>> > >> > etc...
>> > >>
>> > >
>> > >  This is done when we create the TextureRef - it calls StartTracking
>> in
>> > >>
>> > > the
>> > >
>> > >> constructor.
>> > >>
>> > >
>> > >  >
>> > >> > I'm wondering it Consume would be better reusing the same ref?
>> > >>
>> > > Something like
>> > >
>> > >> >
>> > >> >
>> > >> >    Textures::Iter it = textures_.find(client_id);
>> > >> >    if (it != end()) {
>> > >> >         TextureRef ref = it->second
>> > >> >         ref->set_texture(texture);
>> > >> >    }
>> > >> >
>> > >> > Then all the refs in this context are updated.
>> > >>
>> > >
>> > >  The problem is state in other contexts and FBO attachments. The
>> > >>
>> > > service id for a
>> > >
>> > >> texture ref changes, so if we want to do that, we need to keep track
>> > >>
>> > > everywhere
>> > >
>> > >> that we need to do something in real GL terms - e.g. when we switch
>> to
>> > >>
>> > > a new
>> > >
>> > >> context, we need to rebind all texture units (or keep track of which
>> > >>
>> > > one refers
>> > >
>> > >> to this texture ref), and reattach all FBOs.
>> > >>
>> > >
>> > > I'm confused. TextureRefs are per context so changing a TextureRef in
>> > > this context to point to a new texture doesn't affect other contexts
>> > > right?
>> > >
>> >
>> > TextureRefs are per share group, so it'll affect other contexts in the
>> same
>> > share group.
>>
>
>  That's what I was forgetting. Doh
>>
>
>  I'm not super comfortable about the weird dangling references. I'm sure
>>
> someone
>
>> in a year or two will commit a CL that assumes that things work and might
>> just
>> be getting lucky that they do.
>>
>
>  Even the GL spec requires just one rebinding to make all bindings work. In
>>
> other
>
>> words, if we followed the GL spec this
>>
>
>      for (i = 0 to 10) {
>>      glActiveTexture(GL_TEXTURE0 + i);
>>      glBindTexture(GL_TEXTURE_2D, tex);
>>     }
>>     glConsumeTextureCHROMIM(tex, mailbox_id);
>>     glBindTexture(GL_TEXTURE_2D, tex);
>>
>
>  Would make all the texture units work but in our case only the one of
>> those 10
>> units will be correct.
>>
>
>  If not now maybe we should make a todo to fix that? Set some flag per
>> context
>> (could add ContextGroup::**MarkAllContextsTextureUnitStat**eDirty()
>> which could
>>
> set
>
>> a flag in each decoder and then GLES2DecoderImpl::**ApplyDirtyState
>> could call
>> RestoreAllTextureUnitBindings if the flag is set.
>>
>
>  That doesn't seem like a lot of code to add. Then would the suggested
>> solution
>> work? There's still the issue of fbo attachments. We could restrict them
>> now
>>
> and
>
>> add RestoreAllFboAttachments later.
>>
>
>  What do you think?
>>
>
> Ok, I don't like arbitrary restrictions which can be hard to reason about,
> but I
> agree that it would make the semantics a bit easier, so I'm convinced.
> Right now, because of the async textures mess, it makes it hard, but for
> now I
> restricted FBOs and added a TODO. Once 240504 is fixed I'll revisit. I
> don't
> think we'll be adding clients that rely on one behavior or the other.


And I filed  https://code.google.com/p/chromium/issues/detail?id=243603 for
that.


>
>
>  >
>> >
>> > >
>> > > It seems like making this change would delete about 20 lines of code
>> > > from this CL making it simpler. In
>> > > GLES2DecoderImpl::::****DoConsumeTextureCHROMIUM the call to
>> > > DeleteTextureHelper before calling texture_manager_->Consume and all
>> the
>> > > code after to reset texture refs disappears as does the bind. It just
>> > > becomes
>> > >
>> > >
>> > > if (!current_texture->****IsAttachedToFramebuffer()) {
>> > >    texture-manager->Consume(****client_id, texture_being_consumed);
>> > >    RestoreAllTextureUnitBindings(****);
>> > > }
>> >
>> >
>> > > The code here just becomes
>> > >
>> > >   TextureRef* ref = GetTexture(client_id);
>> > >   DCHECK(ref);
>> > >   ref->SetTexture(this, texture_to_consume);
>> > >
>> > > And SetTexture is
>> > >
>> > > TextureRef::SetTexture(****TextureManager* manager, Texture*
>> texture) {
>> > >   manager->StopTracking(this);
>> > >   texture_->RemoveTextureRef(****this);
>> > >   texture_ = texture;
>> > >   texture_->AddTexturRef(this);
>> > >   manager_->StartTracking(this);
>> > > }
>> > >
>> > > And suddenly there's no path for the client to have weird situations
>> > > where units are pointing to the wrong stuff.
>> > >
>> > > Does that make any sense or am I missing something?
>> >
>> >
>> > I actually had that in a previous version. This one I'm a lot more
>> > confident is right.
>> > Right now, on top of the other contexts, there is also the matter of the
>> > async uploads, which are a bit bogus until crbug.com/240504 is fixed
>> (right
>> > now they're a per-TextureRef thing but affect the Texture. It becomes
>> > possible to violate the assumption that only one TextureRef has an async
>> > upload on the texture). I'd need to figure out if it's possible to fix
>> this
>> > as is, otherwise we have to wait for backer's work on this.
>> > I also think it's actually easier to reason about on the client side
>> ("my
>> > texture will be deleted" vs "the call may fail if the texture is bound
>> to a
>> > FBO on another context"), but it's a matter of taste.
>> >
>> >
>> > >
>> > >
>> > >
>> > >  >
>> > >> > This would also auto delete the old texture is calling
>> > >>
>> > > ref->set_texture
>> > >
>> > >> release
>> > >> > the last reference
>> > >> >
>> > >> > Right now it seems like this would get out of sync
>> > >> >
>> > >> >   ctx2->genTex(1, &t);
>> > >> >   ctx2->bindTex(..., t);
>> > >> >   ctx2->framebufferTexture2D(,,,****,t);
>> > >> >   ctx2->consume(...)
>> > >> >
>> > >> > Because the framebuffer will have a scoped_refptr to the old
>> > >>
>> > > TextureRef and so
>> > >
>> > >> > will any texture_units
>> > >> >
>> > >> > Of course sadly, even if we do it that way the real FBO on the
>> > >>
>> > > driver still
>> > >
>> > >> has
>> > >> > the old texture bound and so do the texture units. Not sure what to
>> > >>
>> > > do about
>> > >
>> > >> > that. For the texture units we could set a flag to call
>> > >> > RestoreAllTextureUnitBindings before the next draw
>> > >> >
>> > >> > For FBOs, we could make it an error to consume if the texture being
>> > >>
>> > > replaced
>> > >
>> > >> is
>> > >> > attached to anything.
>> > >>
>> > >
>> > >  Right, my thinking is to make this explicit, and responsibility to
>> the
>> > >>
>> > > client:
>> > >
>> > >> consume deletes the current texture and creates a new one. It removes
>> > >> implementation headaches, at the cost of what could be seen as a
>> small
>> > >>
>> > > quirk on
>> > >
>> > >> the client side. Delete has very well-defined semantics (resets units
>> > >>
>> > > in current
>> > >
>> > >> context and attachments in current fbo, other ones keep a reference).
>> > >>
>> > >
>> > >  In practice, the quirk is not a problem: in our usage patterns,
>> > >>
>> > > clients would
>> > >
>> > >> consume once, on a newly created texture. So I'm not sure it's worth
>> > >>
>> > > it to add
>> > >
>> > >> implementation complexity for something that doesn't matter on the
>> > >>
>> > > client side.
>> > >
>> > >
>> >
>>
>
> https://codereview.chromium.****org/14188053/%253Chttps://code**
> review.chromium.org/14188053/ <http://codereview.chromium.org/14188053/>>
>
>> > >
>>
>
>
>
https://codereview.chromium.**org/14188053/<https://codereview.chromium.org/1...
>

[greggman, 2013-05-24 04:25:36]

lgtm

[greggman, 2013-05-24 17:51:53]

https://codereview.chromium.org/14188053/diff/85001/gpu/command_buffer/servic...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

https://codereview.chromium.org/14188053/diff/85001/gpu/command_buffer/servic...
gpu/command_buffer/service/gles2_cmd_decoder.cc:9922: // change the Texture
inside the TextureRef. We need crbug.com/240504 to be
I take it back. This code is fine as is. No need for the TODO. The way it is is
how GL works. Sorry for the confusion.  And, I don't think you need the check
above that it's not attached to an fbo.

Do we need a check that you don't mail something to yourself so you end up with
2 textures with the same service id in one texture manager?

[piman, 2013-05-24 17:58:47]

On Fri, May 24, 2013 at 10:51 AM, <gman@chromium.org> wrote:

>
> https://codereview.chromium.**org/14188053/diff/85001/gpu/**
>
command_buffer/service/gles2_**cmd_decoder.cc<https://codereview.chromium.org/14188053/diff/85001/gpu/command_buffer/service/gles2_cmd_decoder.cc>
> File gpu/command_buffer/service/**gles2_cmd_decoder.cc (right):
>
> https://codereview.chromium.**org/14188053/diff/85001/gpu/**
>
command_buffer/service/gles2_**cmd_decoder.cc#newcode9922<https://codereview.chromium.org/14188053/diff/85001/gpu/command_buffer/service/gles2_cmd_decoder.cc#newcode9922>
> gpu/command_buffer/service/**gles2_cmd_decoder.cc:9922: // change the
> Texture inside the TextureRef. We need crbug.com/240504 to be
> I take it back. This code is fine as is. No need for the TODO. The way
> it is is how GL works. Sorry for the confusion.  And, I don't think you
> need the check above that it's not attached to an fbo.
>
> Do we need a check that you don't mail something to yourself so you end
> up with 2 textures with the same service id in one texture manager?
>

Why is it a problem?


>
>
https://codereview.chromium.**org/14188053/<https://codereview.chromium.org/1...
>

[apatrick_chromium, 2013-05-24 18:02:10]

I think as things stand, we don't want multiple TextureRefs with the same
service_id in the same TextureManager because we have:

bool TextureManager::GetClientId(GLuint service_id, GLuint* client_id);

It would be awesome to see if we can remove that but I think not for this patch.
Maybe have Consume generate an error?

[piman, 2013-05-24 18:32:12]

On Fri, May 24, 2013 at 11:02 AM, <apatrick@chromium.org> wrote:

> I think as things stand, we don't want multiple TextureRefs with the same
> service_id in the same TextureManager because we have:
>
> bool TextureManager::GetClientId(**GLuint service_id, GLuint* client_id);
>
> It would be awesome to see if we can remove that but I think not for this
> patch.
> Maybe have Consume generate an error?
>

It would break some of the use cases that we have. Video, WebGL and Canvas
(in mailbox mode) currently create the texture and produce it in a context
that is shared with the consumer context (it might even be the same in the
video case). Same with asynchronous readback. We may be able to fully
separate the contexts eventually, but that's premature.

Let me see what I can find about GetClientId users. I don't think it
expresses something meaningful wrt client-side semantics, so it seems odd.


>
https://codereview.chromium.**org/14188053/<https://codereview.chromium.org/1...
>

[greggman, 2013-05-24 19:29:48]

On 2013/05/24 18:32:12, piman wrote:
> On Fri, May 24, 2013 at 11:02 AM, <mailto:apatrick@chromium.org> wrote:
> 
> > I think as things stand, we don't want multiple TextureRefs with the same
> > service_id in the same TextureManager because we have:
> >
> > bool TextureManager::GetClientId(**GLuint service_id, GLuint* client_id);
> >
> > It would be awesome to see if we can remove that but I think not for this
> > patch.
> > Maybe have Consume generate an error?
> >
> 
> It would break some of the use cases that we have. Video, WebGL and Canvas
> (in mailbox mode) currently create the texture and produce it in a context
> that is shared with the consumer context (it might even be the same in the
> video case). Same with asynchronous readback. We may be able to fully
> separate the contexts eventually, but that's premature.

I'm confused. There's no need for Produce/Consume if the textures are in the
same ContextGroup/TextureManager as they are already shared. So the only time
this is an issue is if you Produce a texture and Consume it back into the same
group, something that should never be done since it's not needed.

Seems like just calling GetClientId for the given service id in Consume and if
it finds something issuing an INVALID_OPERATION, "tried to share a texture with
yourself" would be enough.

> 
> Let me see what I can find about GetClientId users. I don't think it
> expresses something meaningful wrt client-side semantics, so it seems odd.
> 
> 
> >
>
https://codereview.chromium.**org/14188053/%3Chttps://codereview.chromium.org...>
> >

[piman, 2013-05-24 20:43:30]

On Fri, May 24, 2013 at 12:29 PM, <gman@chromium.org> wrote:

> On 2013/05/24 18:32:12, piman wrote:
>
>  On Fri, May 24, 2013 at 11:02 AM, <mailto:apatrick@chromium.org> wrote:
>>
>
>  > I think as things stand, we don't want multiple TextureRefs with the
>> same
>> > service_id in the same TextureManager because we have:
>> >
>> > bool TextureManager::GetClientId(****GLuint service_id, GLuint*
>> client_id);
>>
>> >
>> > It would be awesome to see if we can remove that but I think not for
>> this
>> > patch.
>> > Maybe have Consume generate an error?
>> >
>>
>
>  It would break some of the use cases that we have. Video, WebGL and Canvas
>> (in mailbox mode) currently create the texture and produce it in a context
>> that is shared with the consumer context (it might even be the same in the
>> video case). Same with asynchronous readback. We may be able to fully
>> separate the contexts eventually, but that's premature.
>>
>
> I'm confused. There's no need for Produce/Consume if the textures are in
> the
> same ContextGroup/TextureManager as they are already shared. So the only
> time
> this is an issue is if you Produce a texture and Consume it back into the
> same
> group, something that should never be done since it's not needed.
>


> Seems like just calling GetClientId for the given service id in Consume
> and if
> it finds something issuing an INVALID_OPERATION, "tried to share a texture
> with
> yourself" would be enough.


There's not strictly a need from the GL point of view, but we're switching
compositor and webkit APIs to use mailboxes instead of textures. From an
API standpoint, it shouldn't matter whether the contexts are the same or
not. Doing so would duplicate a bunch of APIs for the compositor - one for
textures, and one for mailboxes. I don't think that should be required.
Also the mailbox version is safer wrt context loss, so there's a good
reason to switch to it even without separating the share groups.

Semantically, it does not cause a problem with GL that 2 client ids point
to the same service id. Preventing it just adds a burden to the client.
It would also make the transition out of shared contexts for webgl harder,
since, at the same time, we need to change all the API calls in both blink
and chromium.
Also, since GetClientId does a linear search, I don't think that's a good
thing to do on Consume (we typically have 100s of textures in the share
group, if not more when canvas and webgl are involved).

So bottom line:
- we're making the semantics more quirky
- we're inducing excessive complexity on the client side
- we're making the service side more complex and slower


Regardless, I looked at all the call sites for GetClientId , and all but 1
should be fixable by passing a TextureRef around, instead of trying to find
it back from the service id. It'd make many calls more efficient anyway.
The only one left is RestoreTextureState, called
from CopyTextureCHROMIUMResourceManager::DoCopyTextureWithTransform. Turns
out this one:
- doesn't care about the client id, just about the Texture. We can replace
it by a GetTextureFromServiceId() call.
- is probably used incorrectly in some places,
notably AndroidVideoDecodeAccelerator::SendCurrentSurfaceToClient which
uses a service_id that is not in the manager, hence won't be able to
restore its state. Maybe it doesn't matter.


>
>
>  Let me see what I can find about GetClientId users. I don't think it
>> expresses something meaningful wrt client-side semantics, so it seems odd.
>>
>
>
>  >
>>
>
> https://codereview.chromium.****org/14188053/%3Chttps://codere**
> view.chromium.org/14188053/ <http://codereview.chromium.org/14188053/>>
>
>> >
>>
>
>
>
>
https://codereview.chromium.**org/14188053/<https://codereview.chromium.org/1...
>

[apatrick_chromium, 2013-05-24 20:51:58]

https://codereview.chromium.org/14188053/diff/85001/gpu/GLES2/extensions/CHRO...
File gpu/GLES2/extensions/CHROMIUM/CHROMIUM_texture_mailbox.txt (right):

https://codereview.chromium.org/14188053/diff/85001/gpu/GLES2/extensions/CHRO...
gpu/GLES2/extensions/CHROMIUM/CHROMIUM_texture_mailbox.txt:76: texture object is
deleted once all contexts deleted the texture name
deleted -> have deleted

https://codereview.chromium.org/14188053/diff/85001/gpu/GLES2/extensions/CHRO...
gpu/GLES2/extensions/CHROMIUM/CHROMIUM_texture_mailbox.txt:77: associated with
the texture object, and detached from all framebuffer
detached -> detached it

https://codereview.chromium.org/14188053/diff/85001/gpu/command_buffer/servic...
File gpu/command_buffer/service/mailbox_manager.cc (right):

https://codereview.chromium.org/14188053/diff/85001/gpu/command_buffer/servic...
gpu/command_buffer/service/mailbox_manager.cc:52: 
I believe we have discovered an interview question! I think something like this
works and it makes ProduceTexture shorter.

typedef std::map<Name, TextureReverseMapIt> TextureMap; // value is an iterator
typedef TextureMap::iterator TextureMapIt;

typedef std::multmap<Texture*, Name> TextureReverseMap;
typedef TextureReverseMap::iterator TextureReverseMapIt;

Texture* Consume(Name n) {
  // ...
  
  TextureMapIt m2t= mailbox_to_textures_.find(n);
  if (m2t == mailbox_to_textures_.end())
  return NULL;
	
  return m2t->second->first;
}

void ProduceTexture(Name n, Texture* t) {
  // ...

  TextureReverseMapIt t2m = texture_mailboxes_.insert(std::make_pair(t, n));
	
  TextureMapIt m2t= mailbox_to_textures_.find(n);
  if (m2t != mailbox_to_textures_.end())
  mailbox_to_textures_.erase(m2t->second);
  
  mailbox_to_textures_[n] = t2m;
}

void TextureDeleted(Texture* t) {
  std::pair<TextureReverseMap::iterator, TextureReverseMap::iterator> range =
  mailbox_to_textures_.equal_range(t);
  for (TextureReverseMap::iterator it = range.first, it != range.second; ++it) {
  mailbox_to_textures_.erase(it->second);
	
  mailbox_to_textures_.erase(range.first, range.last);
}

I'm not suggesting any name of signature changes. I only shortened / renamed
things for brevity.

[greggman, 2013-05-24 20:53:06]

On 2013/05/24 20:43:30, piman wrote:
> On Fri, May 24, 2013 at 12:29 PM, <mailto:gman@chromium.org> wrote:
> 
> > On 2013/05/24 18:32:12, piman wrote:
> >
> >  On Fri, May 24, 2013 at 11:02 AM, <mailto:apatrick@chromium.org> wrote:
> >>
> >
> >  > I think as things stand, we don't want multiple TextureRefs with the
> >> same
> >> > service_id in the same TextureManager because we have:
> >> >
> >> > bool TextureManager::GetClientId(****GLuint service_id, GLuint*
> >> client_id);
> >>
> >> >
> >> > It would be awesome to see if we can remove that but I think not for
> >> this
> >> > patch.
> >> > Maybe have Consume generate an error?
> >> >
> >>
> >
> >  It would break some of the use cases that we have. Video, WebGL and Canvas
> >> (in mailbox mode) currently create the texture and produce it in a context
> >> that is shared with the consumer context (it might even be the same in the
> >> video case). Same with asynchronous readback. We may be able to fully
> >> separate the contexts eventually, but that's premature.
> >>
> >
> > I'm confused. There's no need for Produce/Consume if the textures are in
> > the
> > same ContextGroup/TextureManager as they are already shared. So the only
> > time
> > this is an issue is if you Produce a texture and Consume it back into the
> > same
> > group, something that should never be done since it's not needed.
> >
> 
> 
> > Seems like just calling GetClientId for the given service id in Consume
> > and if
> > it finds something issuing an INVALID_OPERATION, "tried to share a texture
> > with
> > yourself" would be enough.
> 
> 
> There's not strictly a need from the GL point of view, but we're switching
> compositor and webkit APIs to use mailboxes instead of textures. From an
> API standpoint, it shouldn't matter whether the contexts are the same or
> not. Doing so would duplicate a bunch of APIs for the compositor - one for
> textures, and one for mailboxes. I don't think that should be required.
> Also the mailbox version is safer wrt context loss, so there's a good
> reason to switch to it even without separating the share groups.
> 
> Semantically, it does not cause a problem with GL that 2 client ids point
> to the same service id.

Sure it does: Examples

   glFramebufferTexture2D(...., render_target_texture);
   glBindTexture(GL_TEXTURE_2D, someother_texture);
   glDrawArrays(....)  

If render_target_texture and someother_texture have different client ids they
should be different textures. but in this case I'll be rendering to and reading
from the same texture 

Or this

   glBindTexture(GL_TEXTURE_2D, client_id_1);
   glTexImage2D(...., red);
   glBindTexture(GL_TEXTURE_2D, client_id_2);
   glTexImage2D(...., green);

If client_id_1 and client_id_2 are the same texture that's totally unexpected
behavior.


> Preventing it just adds a burden to the client.
> It would also make the transition out of shared contexts for webgl harder,
> since, at the same time, we need to change all the API calls in both blink
> and chromium.
> Also, since GetClientId does a linear search, I don't think that's a good
> thing to do on Consume (we typically have 100s of textures in the share
> group, if not more when canvas and webgl are involved).
> 
> So bottom line:
> - we're making the semantics more quirky
> - we're inducing excessive complexity on the client side
> - we're making the service side more complex and slower
> 
> 
> Regardless, I looked at all the call sites for GetClientId , and all but 1
> should be fixable by passing a TextureRef around, instead of trying to find
> it back from the service id. It'd make many calls more efficient anyway.
> The only one left is RestoreTextureState, called
> from CopyTextureCHROMIUMResourceManager::DoCopyTextureWithTransform. Turns
> out this one:
> - doesn't care about the client id, just about the Texture. We can replace
> it by a GetTextureFromServiceId() call.
> - is probably used incorrectly in some places,
> notably AndroidVideoDecodeAccelerator::SendCurrentSurfaceToClient which
> uses a service_id that is not in the manager, hence won't be able to
> restore its state. Maybe it doesn't matter.
> 
> 
> >
> >
> >  Let me see what I can find about GetClientId users. I don't think it
> >> expresses something meaningful wrt client-side semantics, so it seems odd.
> >>
> >
> >
> >  >
> >>
> >
> > https://codereview.chromium.****org/14188053/%253Chttps://codere**
> > view.chromium.org/14188053/ <http://codereview.chromium.org/14188053/>>
> >
> >> >
> >>
> >
> >
> >
> >
>
https://codereview.chromium.**org/14188053/%3Chttps://codereview.chromium.org...>
> >

[piman, 2013-05-24 21:11:57]

On Fri, May 24, 2013 at 1:53 PM, <gman@chromium.org> wrote:

> On 2013/05/24 20:43:30, piman wrote:
>
>  On Fri, May 24, 2013 at 12:29 PM, <mailto:gman@chromium.org> wrote:
>>
>
>  > On 2013/05/24 18:32:12, piman wrote:
>> >
>> >  On Fri, May 24, 2013 at 11:02 AM, <mailto:apatrick@chromium.org>
>> wrote:
>> >>
>> >
>> >  > I think as things stand, we don't want multiple TextureRefs with the
>> >> same
>> >> > service_id in the same TextureManager because we have:
>> >> >
>> >> > bool TextureManager::GetClientId(******GLuint service_id, GLuint*
>>
>> >> client_id);
>> >>
>> >> >
>> >> > It would be awesome to see if we can remove that but I think not for
>> >> this
>> >> > patch.
>> >> > Maybe have Consume generate an error?
>> >> >
>> >>
>> >
>> >  It would break some of the use cases that we have. Video, WebGL and
>> Canvas
>> >> (in mailbox mode) currently create the texture and produce it in a
>> context
>> >> that is shared with the consumer context (it might even be the same in
>> the
>> >> video case). Same with asynchronous readback. We may be able to fully
>> >> separate the contexts eventually, but that's premature.
>> >>
>> >
>> > I'm confused. There's no need for Produce/Consume if the textures are in
>> > the
>> > same ContextGroup/TextureManager as they are already shared. So the only
>> > time
>> > this is an issue is if you Produce a texture and Consume it back into
>> the
>> > same
>> > group, something that should never be done since it's not needed.
>> >
>>
>
>
>  > Seems like just calling GetClientId for the given service id in Consume
>> > and if
>> > it finds something issuing an INVALID_OPERATION, "tried to share a
>> texture
>> > with
>> > yourself" would be enough.
>>
>
>
>  There's not strictly a need from the GL point of view, but we're switching
>> compositor and webkit APIs to use mailboxes instead of textures. From an
>> API standpoint, it shouldn't matter whether the contexts are the same or
>> not. Doing so would duplicate a bunch of APIs for the compositor - one for
>> textures, and one for mailboxes. I don't think that should be required.
>> Also the mailbox version is safer wrt context loss, so there's a good
>> reason to switch to it even without separating the share groups.
>>
>
>  Semantically, it does not cause a problem with GL that 2 client ids point
>> to the same service id.
>>
>
> Sure it does: Examples
>
>    glFramebufferTexture2D(...., render_target_texture);
>    glBindTexture(GL_TEXTURE_2D, someother_texture);
>    glDrawArrays(....)
>
> If render_target_texture and someother_texture have different client ids
> they
> should be different textures. but in this case I'll be rendering to and
> reading
> from the same texture
>
> Or this
>
>    glBindTexture(GL_TEXTURE_2D, client_id_1);
>    glTexImage2D(...., red);
>    glBindTexture(GL_TEXTURE_2D, client_id_2);
>    glTexImage2D(...., green);
>
> If client_id_1 and client_id_2 are the same texture that's totally
> unexpected
> behavior.


It's just enough rope to hang oneself. I don't think it's a problem at all
in practice. It doesn't violate any GL semantics per se. The first case is
called out as undefined by the spec, the second case is well defined.
In that sense it's no different than if the textures were shared using
EGLImage or GLXPixmap.

I don't think we should be going out of our way to prevent *valid* and
*useful* behavior to prevent invalid ones.


>
>
>  Preventing it just adds a burden to the client.
>> It would also make the transition out of shared contexts for webgl harder,
>> since, at the same time, we need to change all the API calls in both blink
>> and chromium.
>> Also, since GetClientId does a linear search, I don't think that's a good
>> thing to do on Consume (we typically have 100s of textures in the share
>> group, if not more when canvas and webgl are involved).
>>
>
>  So bottom line:
>> - we're making the semantics more quirky
>> - we're inducing excessive complexity on the client side
>> - we're making the service side more complex and slower
>>
>
>
>  Regardless, I looked at all the call sites for GetClientId , and all but 1
>> should be fixable by passing a TextureRef around, instead of trying to
>> find
>> it back from the service id. It'd make many calls more efficient anyway.
>> The only one left is RestoreTextureState, called
>> from CopyTextureCHROMIUMResourceMan**ager::**DoCopyTextureWithTransform.
>> Turns
>> out this one:
>> - doesn't care about the client id, just about the Texture. We can replace
>> it by a GetTextureFromServiceId() call.
>> - is probably used incorrectly in some places,
>> notably AndroidVideoDecodeAccelerator:**:SendCurrentSurfaceToClient which
>> uses a service_id that is not in the manager, hence won't be able to
>> restore its state. Maybe it doesn't matter.
>>
>
>
>  >
>> >
>> >  Let me see what I can find about GetClientId users. I don't think it
>> >> expresses something meaningful wrt client-side semantics, so it seems
>> odd.
>> >>
>> >
>> >
>> >  >
>> >>
>> >
>> > https://codereview.chromium.******org/14188053/%253Chttps://**codere**
>> > view.chromium.org/14188053/
<http://codereview.chromium.**org/14188053/<http://codereview.chromium.org/141...
>> >>
>> >
>> >> >
>> >>
>> >
>> >
>> >
>> >
>>
>
> https://codereview.chromium.****org/14188053/%3Chttps://codere**
> view.chromium.org/14188053/ <http://codereview.chromium.org/14188053/>>
>
>> >
>>
>
>
>
>
https://codereview.chromium.**org/14188053/<https://codereview.chromium.org/1...
>

[piman, 2013-05-24 23:03:26]

I also removed TextureManager::GetClientId, replacing most calls by a
TextureRef::client_id(), except for RestoreTextureState that I replaced by
GetTextureForServiceId.
For DoGetFramebufferAttachmentParameteriv(GL_FRAMEBUFFER_ATTACHMENT_OBJECT_NAME)
I go directly to the attachment, which means I also introduced a client_id() on
Renderbuffer, making RenderbufferManager::GetClientId redundant.

I think it's overall a good thing, simplifying the code.

https://codereview.chromium.org/14188053/diff/85001/gpu/GLES2/extensions/CHRO...
File gpu/GLES2/extensions/CHROMIUM/CHROMIUM_texture_mailbox.txt (right):

https://codereview.chromium.org/14188053/diff/85001/gpu/GLES2/extensions/CHRO...
gpu/GLES2/extensions/CHROMIUM/CHROMIUM_texture_mailbox.txt:76: texture object is
deleted once all contexts deleted the texture name
On 2013/05/24 20:51:58, apatrick_chromium wrote:
> deleted -> have deleted

Done.

https://codereview.chromium.org/14188053/diff/85001/gpu/GLES2/extensions/CHRO...
gpu/GLES2/extensions/CHROMIUM/CHROMIUM_texture_mailbox.txt:77: associated with
the texture object, and detached from all framebuffer
On 2013/05/24 20:51:58, apatrick_chromium wrote:
> detached -> detached it

Done.

https://codereview.chromium.org/14188053/diff/85001/gpu/command_buffer/servic...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

https://codereview.chromium.org/14188053/diff/85001/gpu/command_buffer/servic...
gpu/command_buffer/service/gles2_cmd_decoder.cc:9922: // change the Texture
inside the TextureRef. We need crbug.com/240504 to be
On 2013/05/24 17:51:54, greggman wrote:
> I take it back. This code is fine as is. No need for the TODO. The way it is
is
> how GL works. Sorry for the confusion.  And, I don't think you need the check
> above that it's not attached to an fbo.

Done.

https://codereview.chromium.org/14188053/diff/85001/gpu/command_buffer/servic...
File gpu/command_buffer/service/mailbox_manager.cc (right):

https://codereview.chromium.org/14188053/diff/85001/gpu/command_buffer/servic...
gpu/command_buffer/service/mailbox_manager.cc:52: 
On 2013/05/24 20:51:58, apatrick_chromium wrote:
> I believe we have discovered an interview question! I think something like
this
> works and it makes ProduceTexture shorter.
> 
> typedef std::map<Name, TextureReverseMapIt> TextureMap; // value is an
iterator
> typedef TextureMap::iterator TextureMapIt;
> 
> typedef std::multmap<Texture*, Name> TextureReverseMap;
> typedef TextureReverseMap::iterator TextureReverseMapIt;
> 
> Texture* Consume(Name n) {
>   // ...
>   
>   TextureMapIt m2t= mailbox_to_textures_.find(n);
>   if (m2t == mailbox_to_textures_.end())
>   return NULL;
> 	
>   return m2t->second->first;
> }
> 
> void ProduceTexture(Name n, Texture* t) {
>   // ...
> 
>   TextureReverseMapIt t2m = texture_mailboxes_.insert(std::make_pair(t, n));
> 	
>   TextureMapIt m2t= mailbox_to_textures_.find(n);
>   if (m2t != mailbox_to_textures_.end())
>   mailbox_to_textures_.erase(m2t->second);
>   
>   mailbox_to_textures_[n] = t2m;
> }
> 
> void TextureDeleted(Texture* t) {
>   std::pair<TextureReverseMap::iterator, TextureReverseMap::iterator> range =
>   mailbox_to_textures_.equal_range(t);
>   for (TextureReverseMap::iterator it = range.first, it != range.second; ++it)
{
>   mailbox_to_textures_.erase(it->second);
> 	
>   mailbox_to_textures_.erase(range.first, range.last);
> }
> 
> I'm not suggesting any name of signature changes. I only shortened / renamed
> things for brevity.

Thanks for the suggestion! I did the map<mailbox, iterator> trick, and it does
simplify quite a bit. I also renamed the types for clarity.

[piman, 2013-05-29 00:08:00]

ping?


On Fri, May 24, 2013 at 4:03 PM, <piman@chromium.org> wrote:

> I also removed TextureManager::GetClientId, replacing most calls by a
> TextureRef::client_id(), except for RestoreTextureState that I replaced by
> GetTextureForServiceId.
> For DoGetFramebufferAttachmentPara**meteriv(GL_FRAMEBUFFER_**
> ATTACHMENT_OBJECT_NAME)
> I go directly to the attachment, which means I also introduced a
> client_id() on
> Renderbuffer, making RenderbufferManager::**GetClientId redundant.
>
> I think it's overall a good thing, simplifying the code.
>
>
>
> https://codereview.chromium.**org/14188053/diff/85001/gpu/**
>
GLES2/extensions/CHROMIUM/**CHROMIUM_texture_mailbox.txt<https://codereview.chromium.org/14188053/diff/85001/gpu/GLES2/extensions/CHROMIUM/CHROMIUM_texture_mailbox.txt>
> File gpu/GLES2/extensions/CHROMIUM/**CHROMIUM_texture_mailbox.txt (right):
>
> https://codereview.chromium.**org/14188053/diff/85001/gpu/**
>
GLES2/extensions/CHROMIUM/**CHROMIUM_texture_mailbox.txt#**newcode76<https://codereview.chromium.org/14188053/diff/85001/gpu/GLES2/extensions/CHROMIUM/CHROMIUM_texture_mailbox.txt#newcode76>
> gpu/GLES2/extensions/CHROMIUM/**CHROMIUM_texture_mailbox.txt:**76: texture
> object is deleted once all contexts deleted the texture name
> On 2013/05/24 20:51:58, apatrick_chromium wrote:
>
>> deleted -> have deleted
>>
>
> Done.
>
>
> https://codereview.chromium.**org/14188053/diff/85001/gpu/**
>
GLES2/extensions/CHROMIUM/**CHROMIUM_texture_mailbox.txt#**newcode77<https://codereview.chromium.org/14188053/diff/85001/gpu/GLES2/extensions/CHROMIUM/CHROMIUM_texture_mailbox.txt#newcode77>
> gpu/GLES2/extensions/CHROMIUM/**CHROMIUM_texture_mailbox.txt:**77:
> associated with the texture object, and detached from all framebuffer
> On 2013/05/24 20:51:58, apatrick_chromium wrote:
>
>> detached -> detached it
>>
>
> Done.
>
>
> https://codereview.chromium.**org/14188053/diff/85001/gpu/**
>
command_buffer/service/gles2_**cmd_decoder.cc<https://codereview.chromium.org/14188053/diff/85001/gpu/command_buffer/service/gles2_cmd_decoder.cc>
> File gpu/command_buffer/service/**gles2_cmd_decoder.cc (right):
>
> https://codereview.chromium.**org/14188053/diff/85001/gpu/**
>
command_buffer/service/gles2_**cmd_decoder.cc#newcode9922<https://codereview.chromium.org/14188053/diff/85001/gpu/command_buffer/service/gles2_cmd_decoder.cc#newcode9922>
> gpu/command_buffer/service/**gles2_cmd_decoder.cc:9922: // change the
> Texture inside the TextureRef. We need crbug.com/240504 to be
> On 2013/05/24 17:51:54, greggman wrote:
>
>> I take it back. This code is fine as is. No need for the TODO. The way
>>
> it is is
>
>> how GL works. Sorry for the confusion.  And, I don't think you need
>>
> the check
>
>> above that it's not attached to an fbo.
>>
>
> Done.
>
>
> https://codereview.chromium.**org/14188053/diff/85001/gpu/**
>
command_buffer/service/**mailbox_manager.cc<https://codereview.chromium.org/14188053/diff/85001/gpu/command_buffer/service/mailbox_manager.cc>
> File gpu/command_buffer/service/**mailbox_manager.cc (right):
>
> https://codereview.chromium.**org/14188053/diff/85001/gpu/**
>
command_buffer/service/**mailbox_manager.cc#newcode52<https://codereview.chromium.org/14188053/diff/85001/gpu/command_buffer/service/mailbox_manager.cc#newcode52>
> gpu/command_buffer/service/**mailbox_manager.cc:52:
> On 2013/05/24 20:51:58, apatrick_chromium wrote:
>
>> I believe we have discovered an interview question! I think something
>>
> like this
>
>> works and it makes ProduceTexture shorter.
>>
>
>  typedef std::map<Name, TextureReverseMapIt> TextureMap; // value is an
>>
> iterator
>
>> typedef TextureMap::iterator TextureMapIt;
>>
>
>  typedef std::multmap<Texture*, Name> TextureReverseMap;
>> typedef TextureReverseMap::iterator TextureReverseMapIt;
>>
>
>  Texture* Consume(Name n) {
>>    // ...
>>
>
>     TextureMapIt m2t= mailbox_to_textures_.find(n);
>>    if (m2t == mailbox_to_textures_.end())
>>    return NULL;
>>
>>    return m2t->second->first;
>> }
>>
>
>  void ProduceTexture(Name n, Texture* t) {
>>    // ...
>>
>
>     TextureReverseMapIt t2m =
>>
> texture_mailboxes_.insert(std:**:make_pair(t, n));
>
>>
>>    TextureMapIt m2t= mailbox_to_textures_.find(n);
>>    if (m2t != mailbox_to_textures_.end())
>>    mailbox_to_textures_.erase(**m2t->second);
>>
>
>     mailbox_to_textures_[n] = t2m;
>> }
>>
>
>  void TextureDeleted(Texture* t) {
>>    std::pair<TextureReverseMap::**iterator, TextureReverseMap::iterator>
>>
> range =
>
>>    mailbox_to_textures_.equal_**range(t);
>>    for (TextureReverseMap::iterator it = range.first, it !=
>>
> range.second; ++it) {
>
>>    mailbox_to_textures_.erase(it-**>second);
>>
>>    mailbox_to_textures_.erase(**range.first, range.last);
>> }
>>
>
>  I'm not suggesting any name of signature changes. I only shortened /
>>
> renamed
>
>> things for brevity.
>>
>
> Thanks for the suggestion! I did the map<mailbox, iterator> trick, and
> it does simplify quite a bit. I also renamed the types for clarity.
>
>
https://codereview.chromium.**org/14188053/<https://codereview.chromium.org/1...
>

[no sievers, 2013-05-29 16:47:13]

Regarding the multiple (client) texture ids mapping to the same texture: You can
already do the same in GL with
http://www.khronos.org/registry/gles/extensions/OES/OES_EGL_image.txt (also see
glBindTexImage2DCHROMIUM).
So I also feel like it shouldn't be a problem.

[greggman, 2013-05-29 18:27:51]

lgtm

[commit-bot: I haz the power, 2013-05-29 18:30:56]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/piman@chromium.org/14188053/103003

[commit-bot: I haz the power, 2013-05-29 18:31:04]

Failed to apply patch for content/common/gpu/texture_image_transport_surface.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file content/common/gpu/texture_image_transport_surface.cc
  Hunk #2 FAILED at 32.
  Hunk #3 succeeded at 94 (offset 1 line).
  Hunk #4 succeeded at 117 (offset 1 line).
  Hunk #5 succeeded at 131 (offset 1 line).
  Hunk #6 FAILED at 155.
  Hunk #7 succeeded at 187 (offset 3 lines).
  Hunk #8 succeeded at 203 with fuzz 2 (offset 4 lines).
  Hunk #9 succeeded at 218 (offset 4 lines).
  Hunk #10 succeeded at 229 (offset 4 lines).
  Hunk #11 succeeded at 243 (offset 5 lines).
  Hunk #12 succeeded at 265 (offset 5 lines).
  Hunk #13 succeeded at 287 (offset 5 lines).
  Hunk #14 succeeded at 339 (offset 5 lines).
  Hunk #15 succeeded at 430 (offset 5 lines).
  Hunk #16 succeeded at 448 (offset 5 lines).
  2 out of 16 hunks FAILED -- saving rejects to file
content/common/gpu/texture_image_transport_surface.cc.rej

Patch:       content/common/gpu/texture_image_transport_surface.cc
Index: content/common/gpu/texture_image_transport_surface.cc
diff --git a/content/common/gpu/texture_image_transport_surface.cc
b/content/common/gpu/texture_image_transport_surface.cc
index
858c023f700b9aaa2ba35f74f83cf4bdccfe7b27..766dac50b6a0af73b8a1c1bff9abe68beaf4dd2f
100644
--- a/content/common/gpu/texture_image_transport_surface.cc
+++ b/content/common/gpu/texture_image_transport_surface.cc
@@ -18,10 +18,12 @@
 #include "ui/gl/scoped_binders.h"
 
 using gpu::gles2::ContextGroup;
+using gpu::gles2::GLES2Decoder;
 using gpu::gles2::MailboxManager;
 using gpu::gles2::MailboxName;
-using gpu::gles2::TextureDefinition;
+using gpu::gles2::Texture;
 using gpu::gles2::TextureManager;
+using gpu::gles2::TextureRef;
 
 namespace content {
 
@@ -30,7 +32,7 @@ TextureImageTransportSurface::TextureImageTransportSurface(
     GpuCommandBufferStub* stub,
     const gfx::GLSurfaceHandle& handle)
       : fbo_id_(0),
-        backbuffer_(CreateTextureDefinition(gfx::Size(), 0)),
+        current_size_(1, 1),
         stub_destroyed_(false),
         backbuffer_suggested_allocation_(true),
         frontbuffer_suggested_allocation_(true),
@@ -91,41 +93,19 @@ bool TextureImageTransportSurface::DeferDraws() {
   return false;
 }
 
-bool TextureImageTransportSurface::Resize(const gfx::Size&) {
-  return true;
-}
-
 bool TextureImageTransportSurface::IsOffscreen() {
   return true;
 }
 
-bool TextureImageTransportSurface::OnMakeCurrent(gfx::GLContext* context) {
-  if (stub_destroyed_) {
-    // Early-exit so that we don't recreate the fbo. We still want to return
-    // true, so that the context is made current and the GLES2DecoderImpl can
-    // release its own resources.
-    return true;
-  }
-
-  context_ = context;
-
+unsigned int TextureImageTransportSurface::GetBackingFrameBufferObject() {
+  DCHECK(helper_->stub()->decoder()->GetGLContext()->IsCurrent(NULL));
   if (!fbo_id_) {
     glGenFramebuffersEXT(1, &fbo_id_);
     glBindFramebufferEXT(GL_FRAMEBUFFER, fbo_id_);
-    current_size_ = gfx::Size(1, 1);
     helper_->stub()->AddDestructionObserver(this);
-  }
-
-  // We could be receiving non-deferred GL commands, that is anything that does
-  // not need a framebuffer.
-  if (!backbuffer_->service_id() && !is_swap_buffers_pending_ &&
-      backbuffer_suggested_allocation_) {
     CreateBackTexture();
   }
-  return true;
-}
 
-unsigned int TextureImageTransportSurface::GetBackingFrameBufferObject() {
   return fbo_id_;
 }
 
@@ -136,7 +116,7 @@ bool
TextureImageTransportSurface::SetBackbufferAllocation(bool allocation) {
   backbuffer_suggested_allocation_ = allocation;
 
   if (backbuffer_suggested_allocation_) {
-    DCHECK(!backbuffer_->service_id());
+    DCHECK(!backbuffer_);
     CreateBackTexture();
   } else {
     ReleaseBackTexture();
@@ -150,9 +130,15 @@ void
TextureImageTransportSurface::SetFrontbufferAllocation(bool allocation) {
     return;
   frontbuffer_suggested_allocation_ = allocation;
 
-  if (!frontbuffer_suggested_allocation_) {
-    GpuHostMsg_AcceleratedSurfaceRelease_Params params;
-    helper_->SendAcceleratedSurfaceRelease(params);
+  // If a swapbuffers is in flight, wait for the ack before releasing the front
+  // buffer:
+  // - we don't know yet which texture the browser will want to keep
+  // - we want to ensure we don't destroy a texture that is in flight before
the
+  // browser got a reference on it.
+  if (!frontbuffer_suggested_allocation_ &&
+      !is_swap_buffers_pending_ &&
+      helper_->MakeCurrent()) {
+    ReleaseFrontTexture();
   }
 }
 
@@ -169,20 +155,21 @@ void* TextureImageTransportSurface::GetConfig() {
 }
 
 void TextureImageTransportSurface::OnResize(gfx::Size size) {
+  DCHECK_GE(size.width(), 1);
+  DCHECK_GE(size.height(), 1);
   current_size_ = size;
   CreateBackTexture();
 }
 
 void TextureImageTransportSurface::OnWillDestroyStub() {
+  DCHECK(helper_->stub()->decoder()->GetGLContext()->IsCurrent(NULL));
   helper_->stub()->RemoveDestructionObserver(this);
 
-  GpuHostMsg_AcceleratedSurfaceRelease_Params params;
-  helper_->SendAcceleratedSurfaceRelease(params);
-
-  ReleaseBackTexture();
-
   // We are losing the stub owning us, this is our last chance to clean up the
   // resources we allocated in the stub's context.
+  ReleaseBackTexture();
+  ReleaseFrontTexture();
+
   if (fbo_id_) {
     glDeleteFramebuffersEXT(1, &fbo_id_);
     CHECK_GL_ERROR();
@@ -198,12 +185,13 @@ void TextureImageTransportSurface::SetLatencyInfo(
 }
 
 bool TextureImageTransportSurface::SwapBuffers() {
+  DCHECK(helper_->stub()->decoder()->GetGLContext()->IsCurrent(NULL));
   DCHECK(backbuffer_suggested_allocation_);
 
   if (!frontbuffer_suggested_allocation_)
     return true;
 
-  if (!backbuffer_->service_id()) {
+  if (!backbuffer_) {
     LOG(ERROR) << "Swap without valid backing.";
     return true;
   }
@@ -212,14 +200,10 @@ bool TextureImageTransportSurface::SwapBuffers() {
   GpuHostMsg_AcceleratedSurfaceBuffersSwapped_Params params;
   params.size = backbuffer_size();
   params.mailbox_name.assign(
-      reinterpret_cast<const char*>(&mailbox_name_), sizeof(mailbox_name_));
+      reinterpret_cast<const char*>(&back_mailbox_name_),
+      sizeof(back_mailbox_name_));
 
   glFlush();
-  ProduceTexture();
-
-  // Do not allow destruction while we are still waiting for a swap ACK,
-  // so we do not leak a texture in the mailbox.
-  AddRef();
 
   params.latency_info = latency_info_;
   helper_->SendAcceleratedSurfaceBuffersSwapped(params);
@@ -231,6 +215,7 @@ bool TextureImageTransportSurface::SwapBuffers() {
 
 bool TextureImageTransportSurface::PostSubBuffer(
     int x, int y, int width, int height) {
+  DCHECK(helper_->stub()->decoder()->GetGLContext()->IsCurrent(NULL));
   DCHECK(backbuffer_suggested_allocation_);
   if (!frontbuffer_suggested_allocation_)
     return true;
@@ -241,7 +226,7 @@ bool TextureImageTransportSurface::PostSubBuffer(
   if (new_damage_rect.IsEmpty())
     return true;
 
-  if (!backbuffer_->service_id()) {
+  if (!backbuffer_) {
     LOG(ERROR) << "Swap without valid backing.";
     return true;
   }
@@ -254,14 +239,10 @@ bool TextureImageTransportSurface::PostSubBuffer(
   params.width = width;
   params.height = height;
   params.mailbox_name.assign(
-      reinterpret_cast<const char*>(&mailbox_name_), sizeof(mailbox_name_));
+      reinterpret_cast<const char*>(&back_mailbox_name_),
+      sizeof(back_mailbox_name_));
 
   glFlush();
-  ProduceTexture();
-
-  // Do not allow destruction while we are still waiting for a swap ACK,
-  // so we do not leak a texture in the mailbox.
-  AddRef();
 
   params.latency_info = latency_info_;
   helper_->SendAcceleratedSurfacePostSubBuffer(params);
@@ -280,11 +261,7 @@ std::string TextureImageTransportSurface::GetExtensions() {
 }
 
 gfx::Size TextureImageTransportSurface::GetSize() {
-  gfx::Size size = current_size_;
-
-  // OSMesa expects a non-zero size.
-  return gfx::Size(size.width() == 0 ? 1 : size.width(),
-                   size.height() == 0 ? 1 : size.height());
+  return current_size_;
 }
 
 void* TextureImageTransportSurface::GetHandle() {
@@ -306,45 +283,40 @@ void TextureImageTransportSurface::OnBufferPresented(
                    this,
                    params.mailbox_name));
   }
-
-  // Careful, we might get deleted now if we were only waiting for
-  // a final swap ACK.
-  Release();
 }
 
 void TextureImageTransportSurface::BufferPresentedImpl(
     const std::string& mailbox_name) {
-  DCHECK(!backbuffer_->service_id());
-  if (!mailbox_name.empty()) {
-    DCHECK(mailbox_name.length() == GL_MAILBOX_SIZE_CHROMIUM);
-    mailbox_name.copy(reinterpret_cast<char *>(&mailbox_name_),
-                      sizeof(MailboxName));
-    ConsumeTexture();
-  }
-
-  if (stub_destroyed_ && backbuffer_->service_id()) {
-    // TODO(sievers): Remove this after changes to the mailbox to take
ownership
-    // of the service ids.
-    DCHECK(context_.get() && surface_.get());
-    uint32 service_id = backbuffer_->ReleaseServiceId();
-    if (context_->MakeCurrent(surface_))
-      glDeleteTextures(1, &service_id);
-
-    return;
-  }
-
   DCHECK(is_swap_buffers_pending_);
   is_swap_buffers_pending_ = false;
-
   // We should not have allowed the backbuffer to be discarded while the ack
   // was pending.
   DCHECK(backbuffer_suggested_allocation_);
+  DCHECK(backbuffer_);
+
+  bool swap = true;
+  if (!mailbox_name.empty()) {
+    DCHECK(mailbox_name.length() == GL_MAILBOX_SIZE_CHROMIUM);
+    if (!memcmp(mailbox_name.data(),
+                &back_mailbox_name_,
+                mailbox_name.length())) {
+      // The browser has skipped the frame to unblock the GPU process, waiting
+      // for one of the right size, and returned the back buffer, so don't
swap.
+      swap = false;
+    }
+  }
+  if (swap) {
+    std::swap(backbuffer_, frontbuffer_);
+    std::swap(back_mailbox_name_, front_mailbox_name_);
+  }
 
   // We're …
(message too large)

[commit-bot: I haz the power, 2013-05-29 19:56:41]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/piman@chromium.org/14188053/140001

[commit-bot: I haz the power, 2013-05-29 22:28:11]

Change committed as 202992