gpu: Refactor to support cross-channel shared textures

gpu: Refactor to support cross-channel shared textures

This CL doesn't change command semantics, but is refactoring the management of
textures on the service side.

Accounting for texture gets separated into the Texture class which wraps the
service id and the meta-data, vs the TextureRef class which is a reference to
that service id in a given context group (generally corresponding to the client
id). A Texture can now have multiple TextureRef referencing it (though in this
CL no code does yet).

TextureRef keeps the refcount, whereas the Texture is explicitly managed
(jointly owned by all the TextureRef).

The main functional change is that texture-related, per-TextureManager
meta-data, such as "Is there any texture that hasn't been cleared", "Is there
any non-renderable texture", or memory accounting is pushed from the Texture to
all the owning TextureManagers (since changes from one can affect other ones).

BUG=230137
TEST=gpu_unittests, gles2_conform_test, webgl conformance tests

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

[piman, 2013-05-14 01:34:50]

Sorry for the bomb-tastic patch...

https://codereview.chromium.org/14844004/diff/1/gpu/command_buffer/service/gl...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (left):

https://codereview.chromium.org/14844004/diff/1/gpu/command_buffer/service/gl...
gpu/command_buffer/service/gles2_cmd_decoder.cc:7542:
framebuffer_manager()->IncFramebufferStateChangeCount();
Note: this is done from SetLevelInfo now.

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

https://codereview.chromium.org/14844004/diff/1/gpu/command_buffer/service/gl...
gpu/command_buffer/service/gles2_cmd_decoder.cc:2773: clear_state_dirty_ = true;
Note on this because it may be non-obvious.

My understanding of the meaning of clear_state_dirty_ is that when we change a
FBO attachment, we may need to change things like color mask (assuming because
we don't trust the driver to do it?). clear_state_dirty_ is set to indicate that
we need to reset that state before drawing (and is cleared in
DoDrawArrays/Elements via ApplyDirtyState).
Current code does it for the current context whenever we change a texture that
may be bound to a FBO. However it should in theory also affect other contexts
that may also have this texture bound to a FBO, but I can't find code that does
it.

This would do the trick in that we assume that if we switch context then we need
to revalidate the current FBO state since it may have changed in another
context.


Note - I may be completely missing the mark here in which case please let me
know.

https://codereview.chromium.org/14844004/diff/1/gpu/command_buffer/service/gl...
gpu/command_buffer/service/gles2_cmd_decoder.cc:10343: if (!state) {
Note: previous code could call WaitForTransferCompletion with a NULL state, and
crash.

https://codereview.chromium.org/14844004/diff/1/gpu/command_buffer/service/te...
File gpu/command_buffer/service/texture_manager.cc (right):

https://codereview.chromium.org/14844004/diff/1/gpu/command_buffer/service/te...
gpu/command_buffer/service/texture_manager.cc:186: Texture::CanRenderCondition
Texture::GetCanRenderCondition() const {
Note: this function should encode the logic that was in Texture::CanRender.
Because it needs to return a feature-independent value, the order of tests
changed a bit, but it should be functionally identical (verified with tests).

https://codereview.chromium.org/14844004/diff/1/gpu/command_buffer/service/te...
File gpu/command_buffer/service/texture_manager.h (left):

https://codereview.chromium.org/14844004/diff/1/gpu/command_buffer/service/te...
gpu/command_buffer/service/texture_manager.h:107: }
Note: I removed this because it didn't seem to be ever called while returning
true.

deleted_ was set to true right after the client id was removed from the
TextureManager. All the callers of IsDeleted or IsValid would only call it on
the texture returned by TextureManager::GetTexture() which would return NULL on
the client id of a deleted texture.

If there is a use that I missed, we could re-introduce it, though it'd belong on
TextureRef.

[greggman, 2013-05-15 21:11:38]

Looks great! Much smaller than I expected.

I don't see any tests though. Should there at least be some simple unit tests
for things like

Making 2 TextureManagers that share a texture.
Deleting a texture from one texture mananger and it's still available in the
other
Making 2 TextureManagers each with a Framebuffer Manager that share a texture
then when updating a texture both framebuffer managers have their update count
incremented?

Maybe a functional test in gl_tests?

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

https://codereview.chromium.org/14844004/diff/1/gpu/command_buffer/service/gl...
gpu/command_buffer/service/gles2_cmd_decoder.cc:2773: clear_state_dirty_ = true;
On 2013/05/14 01:34:50, piman wrote:
> Note on this because it may be non-obvious.
> 
> My understanding of the meaning of clear_state_dirty_ is that when we change a
> FBO attachment, we may need to change things like color mask (assuming because
> we don't trust the driver to do it?). clear_state_dirty_ is set to indicate
that
> we need to reset that state before drawing (and is cleared in
> DoDrawArrays/Elements via ApplyDirtyState).
> Current code does it for the current context whenever we change a texture that
> may be bound to a FBO. However it should in theory also affect other contexts
> that may also have this texture bound to a FBO, but I can't find code that
does
> it.
> 
> This would do the trick in that we assume that if we switch context then we
need
> to revalidate the current FBO state since it may have changed in another
> context.
> 
> 
> Note - I may be completely missing the mark here in which case please let me
> know.

clear_state_dirty_ means the a bunch of state related to clearing has been
munged and needs to be restored before clearing/drawing.  Looking at the code
now it's possible it's being set to true in places that it no longer needs to be
set to true.

I honestly don't remember why it's being set after changing a texture attached
to an fbo :-(

[apatrick_chromium, 2013-05-15 21:26:18]

LGTM % gman's comments.

[piman, 2013-05-17 01:32:13]

PTAL

On 2013/05/15 21:11:38, greggman wrote:
> Looks great! Much smaller than I expected.

It's only half of the change :) The other half, which has semantic changes to
enable this client-side is smaller though.

> 
> I don't see any tests though. Should there at least be some simple unit tests
> for things like
> 
> Making 2 TextureManagers that share a texture.
> Deleting a texture from one texture mananger and it's still available in the
> other
> Making 2 TextureManagers each with a Framebuffer Manager that share a texture
> then when updating a texture both framebuffer managers have their update count
> incremented?

I added a few tests (see SharedTextureTest.*) for the service-side functionality
> 
> Maybe a functional test in gl_tests?

These will have to come with the other half of the CL, which will test
client-side behavior.

> 
>
https://codereview.chromium.org/14844004/diff/1/gpu/command_buffer/service/gl...
> File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):
> 
>
https://codereview.chromium.org/14844004/diff/1/gpu/command_buffer/service/gl...
> gpu/command_buffer/service/gles2_cmd_decoder.cc:2773: clear_state_dirty_ =
true;
> On 2013/05/14 01:34:50, piman wrote:
> > Note on this because it may be non-obvious.
> > 
> > My understanding of the meaning of clear_state_dirty_ is that when we change
a
> > FBO attachment, we may need to change things like color mask (assuming
because
> > we don't trust the driver to do it?). clear_state_dirty_ is set to indicate
> that
> > we need to reset that state before drawing (and is cleared in
> > DoDrawArrays/Elements via ApplyDirtyState).
> > Current code does it for the current context whenever we change a texture
that
> > may be bound to a FBO. However it should in theory also affect other
contexts
> > that may also have this texture bound to a FBO, but I can't find code that
> does
> > it.
> > 
> > This would do the trick in that we assume that if we switch context then we
> need
> > to revalidate the current FBO state since it may have changed in another
> > context.
> > 
> > 
> > Note - I may be completely missing the mark here in which case please let me
> > know.
> 
> clear_state_dirty_ means the a bunch of state related to clearing has been
> munged and needs to be restored before clearing/drawing.  Looking at the code
> now it's possible it's being set to true in places that it no longer needs to
be
> set to true.
> 
> I honestly don't remember why it's being set after changing a texture attached
> to an fbo :-(

My reading of it is this:
GLES2DecoderImpl::ApplyDirtyState sets (among other things) the color mask based
on whether current_draw_fbo.color_attachment0.internal_format has alpha or not.
It means that for any change in that chain we need to call ApplyDirtyState
before drawing:
1- if current draw buffer changes (glBindFramebuffer)
2- if its color attachment 0 changes (glFramebufferTexture2D)
3- if its internal format changes (gl*TexImage2D)

For 1 and 2 it's easy since you're always operating on the current FBO
For 3, you're operating on the texture, but you don't know if it's attached to
the current FBO or not. So as a proxy you use whether it's attached to any FBO
or not - worst case you just do redundant state changes. However before this
patch, it would only set clear_state_dirty_ on the current context, whereas the
effect could apply to all contexts where that texture might be bound to the
current FBO. Hence my proposed fix to always set clear_state_dirty_ on a context
switch, again worst case we just do redundant state changes.

That said, this is an obscure edge case, and it's a bit orthogonal to the shared
textures (except for the fact that now possibly more contexts can be affected).
I'm happy to leave it out of this patch if that's what you prefer (I don't think
it affects any test that we have).

[greggman, 2013-05-17 01:50:21]

On 2013/05/17 01:32:13, piman wrote:
> PTAL
> 
> On 2013/05/15 21:11:38, greggman wrote:
> > Looks great! Much smaller than I expected.
> 
> It's only half of the change :) The other half, which has semantic changes to
> enable this client-side is smaller though.
> 
> > 
> > I don't see any tests though. Should there at least be some simple unit
tests
> > for things like
> > 
> > Making 2 TextureManagers that share a texture.
> > Deleting a texture from one texture mananger and it's still available in the
> > other
> > Making 2 TextureManagers each with a Framebuffer Manager that share a
texture
> > then when updating a texture both framebuffer managers have their update
count
> > incremented?
> 
> I added a few tests (see SharedTextureTest.*) for the service-side
functionality
> > 
> > Maybe a functional test in gl_tests?
> 
> These will have to come with the other half of the CL, which will test
> client-side behavior.
> 
> > 
> >
>
https://codereview.chromium.org/14844004/diff/1/gpu/command_buffer/service/gl...
> > File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):
> > 
> >
>
https://codereview.chromium.org/14844004/diff/1/gpu/command_buffer/service/gl...
> > gpu/command_buffer/service/gles2_cmd_decoder.cc:2773: clear_state_dirty_ =
> true;
> > On 2013/05/14 01:34:50, piman wrote:
> > > Note on this because it may be non-obvious.
> > > 
> > > My understanding of the meaning of clear_state_dirty_ is that when we
change
> a
> > > FBO attachment, we may need to change things like color mask (assuming
> because
> > > we don't trust the driver to do it?). clear_state_dirty_ is set to
indicate
> > that
> > > we need to reset that state before drawing (and is cleared in
> > > DoDrawArrays/Elements via ApplyDirtyState).
> > > Current code does it for the current context whenever we change a texture
> that
> > > may be bound to a FBO. However it should in theory also affect other
> contexts
> > > that may also have this texture bound to a FBO, but I can't find code that
> > does
> > > it.
> > > 
> > > This would do the trick in that we assume that if we switch context then
we
> > need
> > > to revalidate the current FBO state since it may have changed in another
> > > context.
> > > 
> > > 
> > > Note - I may be completely missing the mark here in which case please let
me
> > > know.
> > 
> > clear_state_dirty_ means the a bunch of state related to clearing has been
> > munged and needs to be restored before clearing/drawing.  Looking at the
code
> > now it's possible it's being set to true in places that it no longer needs
to
> be
> > set to true.
> > 
> > I honestly don't remember why it's being set after changing a texture
attached
> > to an fbo :-(
> 
> My reading of it is this:
> GLES2DecoderImpl::ApplyDirtyState sets (among other things) the color mask
based
> on whether current_draw_fbo.color_attachment0.internal_format has alpha or
not.
> It means that for any change in that chain we need to call ApplyDirtyState
> before drawing:
> 1- if current draw buffer changes (glBindFramebuffer)
> 2- if its color attachment 0 changes (glFramebufferTexture2D)
> 3- if its internal format changes (gl*TexImage2D)
> 
> For 1 and 2 it's easy since you're always operating on the current FBO
> For 3, you're operating on the texture, but you don't know if it's attached to
> the current FBO or not. So as a proxy you use whether it's attached to any FBO
> or not - worst case you just do redundant state changes. However before this
> patch, it would only set clear_state_dirty_ on the current context, whereas
the
> effect could apply to all contexts where that texture might be bound to the
> current FBO. Hence my proposed fix to always set clear_state_dirty_ on a
context
> switch, again worst case we just do redundant state changes.
> 
> That said, this is an obscure edge case, and it's a bit orthogonal to the
shared
> textures (except for the fact that now possibly more contexts can be
affected).
> I'm happy to leave it out of this patch if that's what you prefer (I don't
think
> it affects any test that we have).

lgtm

Leave it in. Maybe file a bug that we should write some tests.

[piman, 2013-05-17 01:57:26]

On 2013/05/17 01:50:21, greggman wrote:
> On 2013/05/17 01:32:13, piman wrote:
> > PTAL
> > 
> > On 2013/05/15 21:11:38, greggman wrote:
> > > Looks great! Much smaller than I expected.
> > 
> > It's only half of the change :) The other half, which has semantic changes
to
> > enable this client-side is smaller though.
> > 
> > > 
> > > I don't see any tests though. Should there at least be some simple unit
> tests
> > > for things like
> > > 
> > > Making 2 TextureManagers that share a texture.
> > > Deleting a texture from one texture mananger and it's still available in
the
> > > other
> > > Making 2 TextureManagers each with a Framebuffer Manager that share a
> texture
> > > then when updating a texture both framebuffer managers have their update
> count
> > > incremented?
> > 
> > I added a few tests (see SharedTextureTest.*) for the service-side
> functionality
> > > 
> > > Maybe a functional test in gl_tests?
> > 
> > These will have to come with the other half of the CL, which will test
> > client-side behavior.
> > 
> > > 
> > >
> >
>
https://codereview.chromium.org/14844004/diff/1/gpu/command_buffer/service/gl...
> > > File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/14844004/diff/1/gpu/command_buffer/service/gl...
> > > gpu/command_buffer/service/gles2_cmd_decoder.cc:2773: clear_state_dirty_ =
> > true;
> > > On 2013/05/14 01:34:50, piman wrote:
> > > > Note on this because it may be non-obvious.
> > > > 
> > > > My understanding of the meaning of clear_state_dirty_ is that when we
> change
> > a
> > > > FBO attachment, we may need to change things like color mask (assuming
> > because
> > > > we don't trust the driver to do it?). clear_state_dirty_ is set to
> indicate
> > > that
> > > > we need to reset that state before drawing (and is cleared in
> > > > DoDrawArrays/Elements via ApplyDirtyState).
> > > > Current code does it for the current context whenever we change a
texture
> > that
> > > > may be bound to a FBO. However it should in theory also affect other
> > contexts
> > > > that may also have this texture bound to a FBO, but I can't find code
that
> > > does
> > > > it.
> > > > 
> > > > This would do the trick in that we assume that if we switch context then
> we
> > > need
> > > > to revalidate the current FBO state since it may have changed in another
> > > > context.
> > > > 
> > > > 
> > > > Note - I may be completely missing the mark here in which case please
let
> me
> > > > know.
> > > 
> > > clear_state_dirty_ means the a bunch of state related to clearing has been
> > > munged and needs to be restored before clearing/drawing.  Looking at the
> code
> > > now it's possible it's being set to true in places that it no longer needs
> to
> > be
> > > set to true.
> > > 
> > > I honestly don't remember why it's being set after changing a texture
> attached
> > > to an fbo :-(
> > 
> > My reading of it is this:
> > GLES2DecoderImpl::ApplyDirtyState sets (among other things) the color mask
> based
> > on whether current_draw_fbo.color_attachment0.internal_format has alpha or
> not.
> > It means that for any change in that chain we need to call ApplyDirtyState
> > before drawing:
> > 1- if current draw buffer changes (glBindFramebuffer)
> > 2- if its color attachment 0 changes (glFramebufferTexture2D)
> > 3- if its internal format changes (gl*TexImage2D)
> > 
> > For 1 and 2 it's easy since you're always operating on the current FBO
> > For 3, you're operating on the texture, but you don't know if it's attached
to
> > the current FBO or not. So as a proxy you use whether it's attached to any
FBO
> > or not - worst case you just do redundant state changes. However before this
> > patch, it would only set clear_state_dirty_ on the current context, whereas
> the
> > effect could apply to all contexts where that texture might be bound to the
> > current FBO. Hence my proposed fix to always set clear_state_dirty_ on a
> context
> > switch, again worst case we just do redundant state changes.
> > 
> > That said, this is an obscure edge case, and it's a bit orthogonal to the
> shared
> > textures (except for the fact that now possibly more contexts can be
> affected).
> > I'm happy to leave it out of this patch if that's what you prefer (I don't
> think
> > it affects any test that we have).
> 
> lgtm
> 
> Leave it in. Maybe file a bug that we should write some tests.

https://code.google.com/p/chromium/issues/detail?id=241654

[commit-bot: I haz the power, 2013-05-17 01:57:55]

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

[commit-bot: I haz the power, 2013-05-17 02:34:07]

Failed to trigger a try job on linux_chromeos
HTTP Error 400: Bad Request

[commit-bot: I haz the power, 2013-05-17 02:34:15]

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

[commit-bot: I haz the power, 2013-05-17 04:06:07]

Retried try job too often on win7_aura for step(s) ash_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win7_aura&...

[commit-bot: I haz the power, 2013-05-17 05:45:36]

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

[commit-bot: I haz the power, 2013-05-17 05:45:43]

Failed to apply patch for gpu/command_buffer/service/gles2_cmd_decoder.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file gpu/command_buffer/service/gles2_cmd_decoder.cc
  Hunk #27 succeeded at 5654 (offset 1 line).
  Hunk #28 succeeded at 5688 (offset 1 line).
  Hunk #29 succeeded at 5725 (offset 1 line).
  Hunk #30 succeeded at 6424 (offset 1 line).
  Hunk #31 succeeded at 7342 (offset 1 line).
  Hunk #32 succeeded at 7517 (offset 1 line).
  Hunk #33 succeeded at 7547 (offset 1 line).
  Hunk #34 succeeded at 7561 (offset 1 line).
  Hunk #35 succeeded at 7755 (offset 1 line).
  Hunk #36 succeeded at 7790 (offset 1 line).
  Hunk #37 succeeded at 7805 (offset 1 line).
  Hunk #38 succeeded at 7814 (offset 1 line).
  Hunk #39 succeeded at 7830 (offset 1 line).
  Hunk #40 succeeded at 7910 (offset 1 line).
  Hunk #41 succeeded at 7983 (offset 1 line).
  Hunk #42 succeeded at 8048 (offset 1 line).
  Hunk #43 succeeded at 8087 (offset 1 line).
  Hunk #44 succeeded at 8102 (offset 1 line).
  Hunk #45 succeeded at 8158 (offset 1 line).
  Hunk #46 succeeded at 8228 (offset 1 line).
  Hunk #47 succeeded at 8295 (offset 1 line).
  Hunk #48 succeeded at 8326 (offset 1 line).
  Hunk #49 succeeded at 8785 (offset 1 line).
  Hunk #50 succeeded at 9342 (offset 1 line).
  Hunk #51 FAILED at 9387.
  Hunk #52 succeeded at 9450 (offset 1 line).
  Hunk #53 succeeded at 9472 (offset 1 line).
  Hunk #54 succeeded at 9500 (offset 1 line).
  Hunk #55 succeeded at 9557 (offset 1 line).
  Hunk #56 succeeded at 9572 (offset 1 line).
  Hunk #57 succeeded at 9665 (offset 1 line).
  Hunk #58 succeeded at 9764 (offset 1 line).
  Hunk #59 succeeded at 9817 (offset 1 line).
  Hunk #60 succeeded at 9845 (offset 1 line).
  Hunk #61 succeeded at 9867 (offset 1 line).
  Hunk #62 succeeded at 9875 (offset 1 line).
  Hunk #63 succeeded at 9884 (offset 1 line).
  Hunk #64 succeeded at 9904 (offset 1 line).
  Hunk #65 succeeded at 9947 (offset 1 line).
  Hunk #66 succeeded at 9976 (offset 1 line).
  Hunk #67 succeeded at 9994 (offset 1 line).
  Hunk #68 succeeded at 10011 (offset 1 line).
  Hunk #69 succeeded at 10021 (offset 1 line).
  Hunk #70 succeeded at 10168 (offset 1 line).
  Hunk #71 succeeded at 10182 (offset 1 line).
  Hunk #72 succeeded at 10207 (offset 1 line).
  Hunk #73 succeeded at 10219 (offset 1 line).
  Hunk #74 succeeded at 10259 (offset 1 line).
  Hunk #75 succeeded at 10272 (offset 1 line).
  Hunk #76 succeeded at 10294 (offset 1 line).
  Hunk #77 succeeded at 10307 (offset 1 line).
  Hunk #78 succeeded at 10329 (offset 1 line).
  1 out of 78 hunks FAILED -- saving rejects to file
gpu/command_buffer/service/gles2_cmd_decoder.cc.rej

Patch:       gpu/command_buffer/service/gles2_cmd_decoder.cc
Index: gpu/command_buffer/service/gles2_cmd_decoder.cc
diff --git a/gpu/command_buffer/service/gles2_cmd_decoder.cc
b/gpu/command_buffer/service/gles2_cmd_decoder.cc
index
180a36d27a11b1db28da7c34ef4ee0268c5e7c25..f3fdc410355199fb916fa34aeeea5e822fd94f6a
100644
--- a/gpu/command_buffer/service/gles2_cmd_decoder.cc
+++ b/gpu/command_buffer/service/gles2_cmd_decoder.cc
@@ -693,13 +693,13 @@ class GLES2DecoderImpl : public GLES2Decoder {
   }
 
   // Creates a Texture for the given texture.
-  Texture* CreateTexture(
+  TextureRef* CreateTexture(
       GLuint client_id, GLuint service_id) {
     return texture_manager()->CreateTexture(client_id, service_id);
   }
 
   // Gets the texture info for the given texture. Returns NULL if none exists.
-  Texture* GetTexture(GLuint client_id) const {
+  TextureRef* GetTexture(GLuint client_id) const {
     return texture_manager()->GetTexture(client_id);
   }
 
@@ -1376,9 +1376,9 @@ class GLES2DecoderImpl : public GLES2Decoder {
   }
 
   // Gets the texture id for a given target.
-  Texture* GetTextureInfoForTarget(GLenum target) {
+  TextureRef* GetTextureInfoForTarget(GLenum target) {
     TextureUnit& unit = state_.texture_units[state_.active_texture_unit];
-    Texture* texture = NULL;
+    TextureRef* texture = NULL;
     switch (target) {
       case GL_TEXTURE_2D:
         texture = unit.bound_texture_2d;
@@ -1405,9 +1405,9 @@ class GLES2DecoderImpl : public GLES2Decoder {
     return texture;
   }
 
-  Texture* GetTextureInfoForTargetUnlessDefault(
+  TextureRef* GetTextureInfoForTargetUnlessDefault(
       GLenum target) {
-    Texture* texture = GetTextureInfoForTarget(target);
+    TextureRef* texture = GetTextureInfoForTarget(target);
     if (!texture)
       return NULL;
     if (texture == texture_manager()->GetDefaultTextureInfo(target))
@@ -1614,7 +1614,7 @@ class GLES2DecoderImpl : public GLES2Decoder {
   // The copy that is saved when SwapBuffers is called.
   scoped_ptr<BackFramebuffer> offscreen_saved_frame_buffer_;
   scoped_ptr<BackTexture> offscreen_saved_color_texture_;
-  scoped_refptr<Texture>
+  scoped_refptr<TextureRef>
       offscreen_saved_color_texture_info_;
 
   // The copy that is used as the destination for multi-sample resolves.
@@ -2234,25 +2234,25 @@ bool GLES2DecoderImpl::Initialize(
   for (uint32 tt = 0; tt < state_.texture_units.size(); ++tt) {
     glActiveTexture(GL_TEXTURE0 + tt);
     // We want the last bind to be 2D.
-    Texture* texture;
+    TextureRef* ref;
     if (features().oes_egl_image_external) {
-      texture = texture_manager()->GetDefaultTextureInfo(
+      ref = texture_manager()->GetDefaultTextureInfo(
           GL_TEXTURE_EXTERNAL_OES);
-      state_.texture_units[tt].bound_texture_external_oes = texture;
-      glBindTexture(GL_TEXTURE_EXTERNAL_OES, texture->service_id());
+      state_.texture_units[tt].bound_texture_external_oes = ref;
+      glBindTexture(GL_TEXTURE_EXTERNAL_OES, ref->service_id());
     }
     if (features().arb_texture_rectangle) {
-      texture = texture_manager()->GetDefaultTextureInfo(
+      ref = texture_manager()->GetDefaultTextureInfo(
           GL_TEXTURE_RECTANGLE_ARB);
-      state_.texture_units[tt].bound_texture_rectangle_arb = texture;
-      glBindTexture(GL_TEXTURE_RECTANGLE_ARB, texture->service_id());
+      state_.texture_units[tt].bound_texture_rectangle_arb = ref;
+      glBindTexture(GL_TEXTURE_RECTANGLE_ARB, ref->service_id());
     }
-    texture = texture_manager()->GetDefaultTextureInfo(GL_TEXTURE_CUBE_MAP);
-    state_.texture_units[tt].bound_texture_cube_map = texture;
-    glBindTexture(GL_TEXTURE_CUBE_MAP, texture->service_id());
-    texture = texture_manager()->GetDefaultTextureInfo(GL_TEXTURE_2D);
-    state_.texture_units[tt].bound_texture_2d = texture;
-    glBindTexture(GL_TEXTURE_2D, texture->service_id());
+    ref = texture_manager()->GetDefaultTextureInfo(GL_TEXTURE_CUBE_MAP);
+    state_.texture_units[tt].bound_texture_cube_map = ref;
+    glBindTexture(GL_TEXTURE_CUBE_MAP, ref->service_id());
+    ref = texture_manager()->GetDefaultTextureInfo(GL_TEXTURE_2D);
+    state_.texture_units[tt].bound_texture_2d = ref;
+    glBindTexture(GL_TEXTURE_2D, ref->service_id());
   }
   glActiveTexture(GL_TEXTURE0);
   CHECK_GL_ERROR();
@@ -2705,28 +2705,30 @@ void GLES2DecoderImpl::DeleteTexturesHelper(
   bool supports_separate_framebuffer_binds =
      features().chromium_framebuffer_multisample;
   for (GLsizei ii = 0; ii < n; ++ii) {
-    Texture* texture = GetTexture(client_ids[ii]);
-    if (texture && !texture->IsDeleted()) {
+    TextureRef* texture_ref = GetTexture(client_ids[ii]);
+    if (texture_ref) {
+      Texture* texture = texture_ref->texture();
       if (texture->IsAttachedToFramebuffer()) {
         clear_state_dirty_ = true;
       }
-      // Unbind texture from texture units.
+      // Unbind texture_ref from texture_ref units.
       for (size_t jj = 0; jj < state_.texture_units.size(); ++jj) {
-        state_.texture_units[jj].Unbind(texture);
+        state_.texture_units[jj].Unbind(texture_ref);
       }
       // Unbind from current framebuffers.
       if (supports_separate_framebuffer_binds) {
         if (state_.bound_read_framebuffer) {
           state_.bound_read_framebuffer->UnbindTexture(
-              GL_READ_FRAMEBUFFER_EXT, texture);
+              GL_READ_FRAMEBUFFER_EXT, texture_ref);
         }
         if (state_.bound_draw_framebuffer) {
           state_.bound_draw_framebuffer->UnbindTexture(
-              GL_DRAW_FRAMEBUFFER_EXT, texture);
+              GL_DRAW_FRAMEBUFFER_EXT, texture_ref);
         }
       } else {
         if (state_.bound_draw_framebuffer) {
-          state_.bound_draw_framebuffer->UnbindTexture(GL_FRAMEBUFFER,
texture);
+          state_.bound_draw_framebuffer->UnbindTexture(GL_FRAMEBUFFER,
+                                                       texture_ref);
         }
       }
       GLuint service_id = texture->service_id();
@@ -2771,6 +2773,8 @@ bool GLES2DecoderImpl::MakeCurrent() {
   if (workarounds().unbind_fbo_on_context_switch)
     RestoreFramebufferBindings();
 
+  clear_state_dirty_ = true;
+
   return true;
 }
 
@@ -2972,9 +2976,9 @@ void GLES2DecoderImpl::UpdateParentTextureInfo() {
     // Update the info about the offscreen saved color texture in the parent.
     // The reference to the parent is a weak pointer and will become null if
the
     // parent is later destroyed.
+    GLenum target = offscreen_saved_color_texture_info_->texture()->target();
     TextureManager* parent_texture_manager = parent_->texture_manager();
-    glBindTexture(offscreen_saved_color_texture_info_->target(),
-                  offscreen_saved_color_texture_info_->service_id());
+    glBindTexture(target, offscreen_saved_color_texture_info_->service_id());
     parent_texture_manager->SetLevelInfo(
         offscreen_saved_color_texture_info_,
         GL_TEXTURE_2D,
@@ -3011,10 +3015,8 @@ void GLES2DecoderImpl::UpdateParentTextureInfo() {
         offscreen_saved_color_texture_info_,
         GL_TEXTURE_WRAP_T,
         GL_CLAMP_TO_EDGE);
-    Texture* texture = GetTextureInfoForTarget(
-        offscreen_saved_color_texture_info_->target());
-    glBindT…
(message too large)

[piman, 2013-05-17 21:28:11]

This collided with https://chromiumcodereview.appspot.com/14828011
I rebased but the conflicts were non-trivial around the CanRender logic and
the SetStreamTexture flag.
I fixed GetCanRenderCondition accordingly, and made sure SetStreamTexture
calls UpdateCanRenderCondition().
PTAL.


On Thu, May 16, 2013 at 10:45 PM, <commit-bot@chromium.org> wrote:

> Failed to apply patch for gpu/command_buffer/service/**
> gles2_cmd_decoder.cc:
> While running patch -p1 --forward --force --no-backup-if-mismatch;
>   patching file gpu/command_buffer/service/**gles2_cmd_decoder.cc
>   Hunk #27 succeeded at 5654 (offset 1 line).
>   Hunk #28 succeeded at 5688 (offset 1 line).
>   Hunk #29 succeeded at 5725 (offset 1 line).
>   Hunk #30 succeeded at 6424 (offset 1 line).
>   Hunk #31 succeeded at 7342 (offset 1 line).
>   Hunk #32 succeeded at 7517 (offset 1 line).
>   Hunk #33 succeeded at 7547 (offset 1 line).
>   Hunk #34 succeeded at 7561 (offset 1 line).
>   Hunk #35 succeeded at 7755 (offset 1 line).
>   Hunk #36 succeeded at 7790 (offset 1 line).
>   Hunk #37 succeeded at 7805 (offset 1 line).
>   Hunk #38 succeeded at 7814 (offset 1 line).
>   Hunk #39 succeeded at 7830 (offset 1 line).
>   Hunk #40 succeeded at 7910 (offset 1 line).
>   Hunk #41 succeeded at 7983 (offset 1 line).
>   Hunk #42 succeeded at 8048 (offset 1 line).
>   Hunk #43 succeeded at 8087 (offset 1 line).
>   Hunk #44 succeeded at 8102 (offset 1 line).
>   Hunk #45 succeeded at 8158 (offset 1 line).
>   Hunk #46 succeeded at 8228 (offset 1 line).
>   Hunk #47 succeeded at 8295 (offset 1 line).
>   Hunk #48 succeeded at 8326 (offset 1 line).
>   Hunk #49 succeeded at 8785 (offset 1 line).
>   Hunk #50 succeeded at 9342 (offset 1 line).
>   Hunk #51 FAILED at 9387.
>   Hunk #52 succeeded at 9450 (offset 1 line).
>   Hunk #53 succeeded at 9472 (offset 1 line).
>   Hunk #54 succeeded at 9500 (offset 1 line).
>   Hunk #55 succeeded at 9557 (offset 1 line).
>   Hunk #56 succeeded at 9572 (offset 1 line).
>   Hunk #57 succeeded at 9665 (offset 1 line).
>   Hunk #58 succeeded at 9764 (offset 1 line).
>   Hunk #59 succeeded at 9817 (offset 1 line).
>   Hunk #60 succeeded at 9845 (offset 1 line).
>   Hunk #61 succeeded at 9867 (offset 1 line).
>   Hunk #62 succeeded at 9875 (offset 1 line).
>   Hunk #63 succeeded at 9884 (offset 1 line).
>   Hunk #64 succeeded at 9904 (offset 1 line).
>   Hunk #65 succeeded at 9947 (offset 1 line).
>   Hunk #66 succeeded at 9976 (offset 1 line).
>   Hunk #67 succeeded at 9994 (offset 1 line).
>   Hunk #68 succeeded at 10011 (offset 1 line).
>   Hunk #69 succeeded at 10021 (offset 1 line).
>   Hunk #70 succeeded at 10168 (offset 1 line).
>   Hunk #71 succeeded at 10182 (offset 1 line).
>   Hunk #72 succeeded at 10207 (offset 1 line).
>   Hunk #73 succeeded at 10219 (offset 1 line).
>   Hunk #74 succeeded at 10259 (offset 1 line).
>   Hunk #75 succeeded at 10272 (offset 1 line).
>   Hunk #76 succeeded at 10294 (offset 1 line).
>   Hunk #77 succeeded at 10307 (offset 1 line).
>   Hunk #78 succeeded at 10329 (offset 1 line).
>   1 out of 78 hunks FAILED -- saving rejects to file
> gpu/command_buffer/service/**gles2_cmd_decoder.cc.rej
>
> Patch:       gpu/command_buffer/service/**gles2_cmd_decoder.cc
> Index: gpu/command_buffer/service/**gles2_cmd_decoder.cc
> diff --git a/gpu/command_buffer/service/**gles2_cmd_decoder.cc
> b/gpu/command_buffer/service/**gles2_cmd_decoder.cc
> index
> 180a36d27a11b1db28da7c34ef4ee0**268c5e7c25..**
> f3fdc410355199fb916fa34aeeea5e**822fd94f6a
> 100644
> --- a/gpu/command_buffer/service/**gles2_cmd_decoder.cc
> +++ b/gpu/command_buffer/service/**gles2_cmd_decoder.cc
> @@ -693,13 +693,13 @@ class GLES2DecoderImpl : public GLES2Decoder {
>    }
>
>    // Creates a Texture for the given texture.
> -  Texture* CreateTexture(
> +  TextureRef* CreateTexture(
>        GLuint client_id, GLuint service_id) {
>      return texture_manager()->**CreateTexture(client_id, service_id);
>    }
>
>    // Gets the texture info for the given texture. Returns NULL if none
> exists.
> -  Texture* GetTexture(GLuint client_id) const {
> +  TextureRef* GetTexture(GLuint client_id) const {
>      return texture_manager()->GetTexture(**client_id);
>    }
>
> @@ -1376,9 +1376,9 @@ class GLES2DecoderImpl : public GLES2Decoder {
>    }
>
>    // Gets the texture id for a given target.
> -  Texture* GetTextureInfoForTarget(GLenum target) {
> +  TextureRef* GetTextureInfoForTarget(GLenum target) {
>      TextureUnit& unit = state_.texture_units[state_.**
> active_texture_unit];
> -    Texture* texture = NULL;
> +    TextureRef* texture = NULL;
>      switch (target) {
>        case GL_TEXTURE_2D:
>          texture = unit.bound_texture_2d;
> @@ -1405,9 +1405,9 @@ class GLES2DecoderImpl : public GLES2Decoder {
>      return texture;
>    }
>
> -  Texture* GetTextureInfoForTargetUnlessD**efault(
> +  TextureRef* GetTextureInfoForTargetUnlessD**efault(
>        GLenum target) {
> -    Texture* texture = GetTextureInfoForTarget(**target);
> +    TextureRef* texture = GetTextureInfoForTarget(**target);
>      if (!texture)
>        return NULL;
>      if (texture == texture_manager()->**GetDefaultTextureInfo(target))
> @@ -1614,7 +1614,7 @@ class GLES2DecoderImpl : public GLES2Decoder {
>    // The copy that is saved when SwapBuffers is called.
>    scoped_ptr<BackFramebuffer> offscreen_saved_frame_buffer_;
>    scoped_ptr<BackTexture> offscreen_saved_color_texture_**;
> -  scoped_refptr<Texture>
> +  scoped_refptr<TextureRef>
>        offscreen_saved_color_texture_**info_;
>
>    // The copy that is used as the destination for multi-sample resolves.
> @@ -2234,25 +2234,25 @@ bool GLES2DecoderImpl::Initialize(
>    for (uint32 tt = 0; tt < state_.texture_units.size(); ++tt) {
>      glActiveTexture(GL_TEXTURE0 + tt);
>      // We want the last bind to be 2D.
> -    Texture* texture;
> +    TextureRef* ref;
>      if (features().oes_egl_image_**external) {
> -      texture = texture_manager()->**GetDefaultTextureInfo(
> +      ref = texture_manager()->**GetDefaultTextureInfo(
>            GL_TEXTURE_EXTERNAL_OES);
> -      state_.texture_units[tt].**bound_texture_external_oes = texture;
> -      glBindTexture(GL_TEXTURE_**EXTERNAL_OES, texture->service_id());
> +      state_.texture_units[tt].**bound_texture_external_oes = ref;
> +      glBindTexture(GL_TEXTURE_**EXTERNAL_OES, ref->service_id());
>      }
>      if (features().arb_texture_**rectangle) {
> -      texture = texture_manager()->**GetDefaultTextureInfo(
> +      ref = texture_manager()->**GetDefaultTextureInfo(
>            GL_TEXTURE_RECTANGLE_ARB);
> -      state_.texture_units[tt].**bound_texture_rectangle_arb = texture;
> -      glBindTexture(GL_TEXTURE_**RECTANGLE_ARB, texture->service_id());
> +      state_.texture_units[tt].**bound_texture_rectangle_arb = ref;
> +      glBindTexture(GL_TEXTURE_**RECTANGLE_ARB, ref->service_id());
>      }
> -    texture = texture_manager()->**GetDefaultTextureInfo(GL_**
> TEXTURE_CUBE_MAP);
> -    state_.texture_units[tt].**bound_texture_cube_map = texture;
> -    glBindTexture(GL_TEXTURE_CUBE_**MAP, texture->service_id());
> -    texture = texture_manager()->**GetDefaultTextureInfo(GL_**
> TEXTURE_2D);
> -    state_.texture_units[tt].**bound_texture_2d = texture;
> -    glBindTexture(GL_TEXTURE_2D, texture->service_id());
> +    ref = texture_manager()->**GetDefaultTextureInfo(GL_**
> TEXTURE_CUBE_MAP);
> +    state_.texture_units[tt].**bound_texture_cube_map = ref;
> +    glBindTexture(GL_TEXTURE_CUBE_**MAP, ref->service_id());
> +    ref = texture_manager()->**GetDefaultTextureInfo(GL_**TEXTURE_2D);
> +    state_.texture_units[tt].**bound_texture_2d = ref;
> +    glBindTexture(GL_TEXTURE_2D, ref->service_id());
>    }
>    glActiveTexture(GL_TEXTURE0);
>    CHECK_GL_ERROR();
> @@ -2705,28 +2705,30 @@ void GLES2DecoderImpl::**DeleteTexturesHelper(
>    bool supports_separate_framebuffer_**binds =
>       features().chromium_**framebuffer_multisample;
>    for (GLsizei ii = 0; ii < n; ++ii) {
> -    Texture* texture = GetTexture(client_ids[ii]);
> -    if (texture && !texture->IsDeleted()) {
> +    TextureRef* texture_ref = GetTexture(client_ids[ii]);
> +    if (texture_ref) {
> +      Texture* texture = texture_ref->texture();
>        if (texture->**IsAttachedToFramebuffer()) {
>          clear_state_dirty_ = true;
>        }
> -      // Unbind texture from texture units.
> +      // Unbind texture_ref from texture_ref units.
>        for (size_t jj = 0; jj < state_.texture_units.size(); ++jj) {
> -        state_.texture_units[jj].**Unbind(texture);
> +        state_.texture_units[jj].**Unbind(texture_ref);
>        }
>        // Unbind from current framebuffers.
>        if (supports_separate_**framebuffer_binds) {
>          if (state_.bound_read_**framebuffer) {
>            state_.bound_read_framebuffer-**>UnbindTexture(
> -              GL_READ_FRAMEBUFFER_EXT, texture);
> +              GL_READ_FRAMEBUFFER_EXT, texture_ref);
>          }
>          if (state_.bound_draw_**framebuffer) {
>            state_.bound_draw_framebuffer-**>UnbindTexture(
> -              GL_DRAW_FRAMEBUFFER_EXT, texture);
> +              GL_DRAW_FRAMEBUFFER_EXT, texture_ref);
>          }
>        } else {
>          if (state_.bound_draw_**framebuffer) {
> -          state_.bound_draw_framebuffer-**>UnbindTexture(GL_FRAMEBUFFER,
> texture);
> +          state_.bound_draw_framebuffer-**>UnbindTexture(GL_FRAMEBUFFER,
> +                                                       texture_ref);
>          }
>        }
>        GLuint service_id = texture->service_id();
> @@ -2771,6 +2773,8 @@ bool GLES2DecoderImpl::MakeCurrent(**) {
>    if (workarounds().unbind_fbo_on_**context_switch)
>      RestoreFramebufferBindings();
>
> +  clear_state_dirty_ = true;
> +
>    return true;
>  }
>
> @@ -2972,9 +2976,9 @@ void GLES2DecoderImpl::**UpdateParentTextureInfo() {
>      // Update the info about the offscreen saved color texture in the
> parent.
>      // The reference to the parent is a weak pointer and will become null
> if
> the
>      // parent is later destroyed.
> +    GLenum target = offscreen_saved_color_texture_**
> info_->texture()->target();
>      TextureManager* parent_texture_manager = parent_->texture_manager();
> -    glBindTexture(offscreen_saved_**color_texture_info_->target(),
> -                  offscreen_saved_color_texture_**info_->service_id());
> +    glBindTexture(target, offscreen_saved_color_texture_**
> info_->service_id());
>      parent_texture_manager->**SetLevelInfo(
>          offscreen_saved_color_texture_**info_,
>          GL_TEXTURE_2D,
> @@ -3011,10 +3015,8 @@ void GLES2DecoderImpl::**UpdateParentTextureInfo()
> {
>          offscreen_saved_color_texture_**info_,
>          GL_TEXTURE_WRAP_T,
>          GL_CLAMP_TO_EDGE);
> -    Texture* texture = GetTextureInfoForTarget(
> -        offscreen_saved_color_texture_**info_->target());
> -    glBindT…
> (message too large)
>
>
https://chromiumcodereview.**appspot.com/14844004/<https://chromiumcodereview...
>

[greggman, 2013-05-17 22:14:27]

On 2013/05/17 21:28:11, piman wrote:
> This collided with https://chromiumcodereview.appspot.com/14828011
> I rebased but the conflicts were non-trivial around the CanRender logic and
> the SetStreamTexture flag.
> I fixed GetCanRenderCondition accordingly, and made sure SetStreamTexture
> calls UpdateCanRenderCondition().
> PTAL.

lgtm   Sorry about the conflict.

[commit-bot: I haz the power, 2013-05-18 01:48:27]

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

[commit-bot: I haz the power, 2013-05-18 09:19:51]

Change committed as 200981