gpu: Add support for GLX_EXT_texture_from_pixmap extension.

gpu: Add support for GLX_EXT_texture_from_pixmap extension.

Implement CHROMIUM_texture_from_image. This extension behaves just like
EXT_texture_from_pixmap but uses chromium specific image identifiers rather
than platform specific pixmap IDs.

Add IPC message for creating an image identifier using a
gfx::PluginWindowHandle. Each GPU channel maintains a different set of
images and deleting an image will cause the internal image representation
to be freed once it's no longer bound to a texture.

BUG=132342
TEST=gpu_unittests --gtest_filter=TextureInfoTest.GetLevelImage:GLES2DecoderTest.BindTexImage2DCHROMIUM:GLES2DecoderTest.ReleaseTexImage2DCHROMIUM and manual

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

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

[reveman, 2012-06-12 22:27:09]

Hi,

this is required to land host window management support for aura. It's similar
to the IOSurface extension. Just uses Pixmap's and only supported with GLX of
course.

We might want to remove the XGetGeometry call in
PixmapGLSurfaceGLX::Initialize() and instead pass width/height/depth to
TexImagePixmap2DCHROMIUM to avoid an unnecessary round-trip to the X server.
Though the glXCreatePixmap call is likely more expensive and doing a round-trip
anyhow so I'm not sure it's worth it.

[piman, 2012-06-12 23:15:38]

couple of high-level comments.
It would be kinda nice to avoid #ifdef USE_X11 in common code. Maybe we can
delegate the X-specific calls to the current context or something?

https://chromiumcodereview.appspot.com/10543125/diff/1/gpu/command_buffer/ser...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

https://chromiumcodereview.appspot.com/10543125/diff/1/gpu/command_buffer/ser...
gpu/command_buffer/service/gles2_cmd_decoder.cc:2892:
static_cast<gfx::GLContextGLX*>(GetGLContext())->display(),
This is not a safe cast when based only on USE_X11: what about EGL?

https://chromiumcodereview.appspot.com/10543125/diff/1/ui/gl/gl_surface_glx.h
File ui/gl/gl_surface_glx.h (right):

https://chromiumcodereview.appspot.com/10543125/diff/1/ui/gl/gl_surface_glx.h...
ui/gl/gl_surface_glx.h:101: class GL_EXPORT PixmapGLSurfaceGLX : public
GLSurfaceGLX {
Do you really need to make this structure a GLSurface? The concept of a
GLSurface is only used to make contexts current on them. In this case you're
only using it to bind to a texture, so there's a bunch of stuff that doesn't
apply (SwapBuffers) and/or is coerced into wrong types
(GetHandle/GetConfig/...). It sounds like for the purpose of this CL you could
make a simpler class without all that baggage.

[google-reveman, 2012-06-12 23:46:18]

On Tue, Jun 12, 2012 at 7:15 PM,  <piman@chromium.org> wrote:
> couple of high-level comments.
> It would be kinda nice to avoid #ifdef USE_X11 in common code. Maybe we can
> delegate the X-specific calls to the current context or something?

I thought about that but chose the current approach to be somewhat
more consistent with glTexImageIOSurface2DCHROMIUM.

>
>
>
https://chromiumcodereview.appspot.com/10543125/diff/1/gpu/command_buffer/ser...
> File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):
>
>
https://chromiumcodereview.appspot.com/10543125/diff/1/gpu/command_buffer/ser...
> gpu/command_buffer/service/gles2_cmd_decoder.cc:2892:
> static_cast<gfx::GLContextGLX*>(GetGLContext())->display(),
> This is not a safe cast when based only on USE_X11: what about EGL?

There's a "gfx::GetGLImplementation() !=
gfx::kGLImplementationDesktopGL" check in
GLES2DecoderImpl::DoTexImagePixmap2DCHROMIUM that should make sure
this cast is always safe.

>
> https://chromiumcodereview.appspot.com/10543125/diff/1/ui/gl/gl_surface_glx.h
> File ui/gl/gl_surface_glx.h (right):
>
>
https://chromiumcodereview.appspot.com/10543125/diff/1/ui/gl/gl_surface_glx.h...
> ui/gl/gl_surface_glx.h:101: class GL_EXPORT PixmapGLSurfaceGLX : public
> GLSurfaceGLX {
> Do you really need to make this structure a GLSurface? The concept of a
> GLSurface is only used to make contexts current on them. In this case
> you're only using it to bind to a texture, so there's a bunch of stuff
> that doesn't apply (SwapBuffers) and/or is coerced into wrong types
> (GetHandle/GetConfig/...). It sounds like for the purpose of this CL you
> could make a simpler class without all that baggage.

Yup, doesn't need to be a GLSurface for this purpose. I'll remove the
GLSurfaceGLX inheritance if that's preferred.

[piman, 2012-06-12 23:58:50]

On Tue, Jun 12, 2012 at 4:46 PM, David Reveman <reveman@google.com> wrote:

> On Tue, Jun 12, 2012 at 7:15 PM,  <piman@chromium.org> wrote:
> > couple of high-level comments.
> > It would be kinda nice to avoid #ifdef USE_X11 in common code. Maybe we
> can
> > delegate the X-specific calls to the current context or something?
>
> I thought about that but chose the current approach to be somewhat
> more consistent with glTexImageIOSurface2DCHROMIUM.
>

I see.


>
> >
> >
> >
>
https://chromiumcodereview.appspot.com/10543125/diff/1/gpu/command_buffer/ser...
> > File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):
> >
> >
>
https://chromiumcodereview.appspot.com/10543125/diff/1/gpu/command_buffer/ser...
> > gpu/command_buffer/service/gles2_cmd_decoder.cc:2892:
> > static_cast<gfx::GLContextGLX*>(GetGLContext())->display(),
> > This is not a safe cast when based only on USE_X11: what about EGL?
>
> There's a "gfx::GetGLImplementation() !=
> gfx::kGLImplementationDesktopGL" check in
> GLES2DecoderImpl::DoTexImagePixmap2DCHROMIUM that should make sure
> this cast is always safe.
>

Maybe add a DCHECK here then?


>
> >
> >
> https://chromiumcodereview.appspot.com/10543125/diff/1/ui/gl/gl_surface_glx.h
> > File ui/gl/gl_surface_glx.h (right):
> >
> >
>
https://chromiumcodereview.appspot.com/10543125/diff/1/ui/gl/gl_surface_glx.h...
> > ui/gl/gl_surface_glx.h:101: class GL_EXPORT PixmapGLSurfaceGLX : public
> > GLSurfaceGLX {
> > Do you really need to make this structure a GLSurface? The concept of a
> > GLSurface is only used to make contexts current on them. In this case
> > you're only using it to bind to a texture, so there's a bunch of stuff
> > that doesn't apply (SwapBuffers) and/or is coerced into wrong types
> > (GetHandle/GetConfig/...). It sounds like for the purpose of this CL you
> > could make a simpler class without all that baggage.
>
> Yup, doesn't need to be a GLSurface for this purpose. I'll remove the
> GLSurfaceGLX inheritance if that's preferred.
>

I think that'd make it simpler (no ugly reinterpret_casts all over the
place).

Antoine

[greggman, 2012-06-13 00:15:51]

How do you get a pixmap id? Can a rouge program just start guessing ids?

https://chromiumcodereview.appspot.com/10543125/diff/1/gpu/command_buffer/ser...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

https://chromiumcodereview.appspot.com/10543125/diff/1/gpu/command_buffer/ser...
gpu/command_buffer/service/gles2_cmd_decoder.cc:1570:
TextureToPixmapGLSurfaceMap texture_to_pixmap_gl_surface_map_;
What happens if I use this texture from another context? The mapping only exists
in this context.

https://chromiumcodereview.appspot.com/10543125/diff/1/gpu/command_buffer/ser...
gpu/command_buffer/service/gles2_cmd_decoder.cc:9087: if (info ==
texture_manager()->GetDefaultTextureInfo(target)) {
Have you considered a new function

GetTextureInfoForTargetUnlessDefault 

to isolate this check?

https://chromiumcodereview.appspot.com/10543125/diff/1/gpu/command_buffer/ser...
gpu/command_buffer/service/gles2_cmd_decoder.cc:9125:
texture_manager()->SetLevelInfo(
So mapping to a pix map magically sets the format and the type and puts in
GL_BGRA as internal format but GL_RGBA as format which is explicitly not allowed
in OpenGL ES.

What other issues does this bring up? Can I still call glTexImage2D on a
pixmapped texture and change the format? glGenerateMipmap? glCopyTexImage2D?
glCompressedTexImage2D?

If I call glDeleteTexture on a mapped texture what happens?

[google-reveman, 2012-06-13 00:55:32]

On Tue, Jun 12, 2012 at 8:15 PM,  <gman@chromium.org> wrote:
> How do you get a pixmap id? Can a rouge program just start guessing ids?

In the current use case, aura gets pixmap IDs from redirecting
top-level X windows. It could be any valid X11 pixmap Id that is from
the same X server that the gpu process is using.

This is only for the browser compositor. I'm pretty sure we don't want
this exposed anywhere else. I assumed even the existing IOSurface
extension is not accessible to arbitrary programs, or is it?

>
>
>
>
https://chromiumcodereview.appspot.com/10543125/diff/1/gpu/command_buffer/ser...
> File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):
>
>
https://chromiumcodereview.appspot.com/10543125/diff/1/gpu/command_buffer/ser...
> gpu/command_buffer/service/gles2_cmd_decoder.cc:1570:
> TextureToPixmapGLSurfaceMap texture_to_pixmap_gl_surface_map_;
> What happens if I use this texture from another context? The mapping
> only exists in this context.

It's allowed (and required for our use case) to use the texture from a
different context. However, binding a texture to separate pixmaps from
different contexts is not supported. I think this is consistent with
our current IOSurface support.

>
>
https://chromiumcodereview.appspot.com/10543125/diff/1/gpu/command_buffer/ser...
> gpu/command_buffer/service/gles2_cmd_decoder.cc:9087: if (info ==
> texture_manager()->GetDefaultTextureInfo(target)) {
> Have you considered a new function
>
> GetTextureInfoForTargetUnlessDefault
>
> to isolate this check?

Good idea, I'll do that.

>
>
https://chromiumcodereview.appspot.com/10543125/diff/1/gpu/command_buffer/ser...
> gpu/command_buffer/service/gles2_cmd_decoder.cc:9125:
> texture_manager()->SetLevelInfo(
> So mapping to a pix map magically sets the format and the type and puts
> in GL_BGRA as internal format but GL_RGBA as format which is explicitly
> not allowed in OpenGL ES.

Oops, should have been the other way around to be consistent with
DoTexImageIOSurface2DCHROMIUM.

>
> What other issues does this bring up? Can I still call glTexImage2D on a
> pixmapped texture and change the format? glGenerateMipmap?
> glCopyTexImage2D? glCompressedTexImage2D?

Most of that is allowed except glGenerateMipmap which has undefined
behavior. The GLX_EXT_texture_from_pixmap spec is really what
determines the current behavior.

>
> If I call glDeleteTexture on a mapped texture what happens?

The pixmap is automatically release.

[apatrick_chromium, 2012-06-13 01:07:32]

The IOSurface support is broken so best not to copy it. I think the pixmap
reference should go in TextureManager::TextureInfo so that if another context in
the group deletes the texture the mapping from texture ID to pixmap does not
leak.

The XIDs will have to be secured somehow. You could add a browser to GPU process
IPC to put a texture bound to a pixmap in a mailbox and then use
CHROMIUM_texture_mailbox to push it into a particular context group.

[reveman, 2012-07-12 17:08:56]

On 2012/06/13 01:07:32, apatrick_chromium wrote:
> The IOSurface support is broken so best not to copy it. I think the pixmap
> reference should go in TextureManager::TextureInfo so that if another context
in
> the group deletes the texture the mapping from texture ID to pixmap does not
> leak.

OK, so with IOSurface support considered broken the latest patch is what I came
up with. Let me know what you think.

> 
> The XIDs will have to be secured somehow. You could add a browser to GPU
process
> IPC to put a texture bound to a pixmap in a mailbox and then use
> CHROMIUM_texture_mailbox to push it into a particular context group.

I haven't done anything to address this in the latest patch as I'm not sure I
understand exactly what needs to be done. Is a browser to GPU process IPC for
creating GLImage instances what I need to add? The texture I bind to a pixmap is
already in the correct context group (UI compositor group) so I don't see why
CHROMIUM_texture_mailbox would need to be used here. What am I missing?

[greggman, 2012-07-17 23:59:31]

On 2012/07/12 17:08:56, David Reveman wrote:
> On 2012/06/13 01:07:32, apatrick_chromium wrote:
> > The IOSurface support is broken so best not to copy it. I think the pixmap
> > reference should go in TextureManager::TextureInfo so that if another
context
> in
> > the group deletes the texture the mapping from texture ID to pixmap does not
> > leak.
> 
> OK, so with IOSurface support considered broken the latest patch is what I
came
> up with. Let me know what you think.
> 
> > 
> > The XIDs will have to be secured somehow. You could add a browser to GPU
> process
> > IPC to put a texture bound to a pixmap in a mailbox and then use
> > CHROMIUM_texture_mailbox to push it into a particular context group.
> 
> I haven't done anything to address this in the latest patch as I'm not sure I
> understand exactly what needs to be done. Is a browser to GPU process IPC for
> creating GLImage instances what I need to add? The texture I bind to a pixmap
is
> already in the correct context group (UI compositor group) so I don't see why
> CHROMIUM_texture_mailbox would need to be used here. What am I missing?

I mostly fine with this except I'd like to see how the ids are secured. As it
is, any process can map any id just by calling TexImagePixmap and passing in an
id. I'm not even sure what those ids mean.

[piman, 2012-07-18 00:08:40]

On 2012/07/17 23:59:31, greggman wrote:
> On 2012/07/12 17:08:56, David Reveman wrote:
> > On 2012/06/13 01:07:32, apatrick_chromium wrote:
> > > The IOSurface support is broken so best not to copy it. I think the pixmap
> > > reference should go in TextureManager::TextureInfo so that if another
> context
> > in
> > > the group deletes the texture the mapping from texture ID to pixmap does
not
> > > leak.
> > 
> > OK, so with IOSurface support considered broken the latest patch is what I
> came
> > up with. Let me know what you think.
> > 
> > > 
> > > The XIDs will have to be secured somehow. You could add a browser to GPU
> > process
> > > IPC to put a texture bound to a pixmap in a mailbox and then use
> > > CHROMIUM_texture_mailbox to push it into a particular context group.
> > 
> > I haven't done anything to address this in the latest patch as I'm not sure
I
> > understand exactly what needs to be done. Is a browser to GPU process IPC
for
> > creating GLImage instances what I need to add? The texture I bind to a
pixmap
> is
> > already in the correct context group (UI compositor group) so I don't see
why
> > CHROMIUM_texture_mailbox would need to be used here. What am I missing?
> 
> I mostly fine with this except I'd like to see how the ids are secured. As it
> is, any process can map any id just by calling TexImagePixmap and passing in
an
> id. I'm not even sure what those ids mean.

I think we should only expose the extension to the browser process, since that's
the only one that needs it - that way we don't need to secure ids one-by-one.

[greggman, 2012-07-18 01:22:11]

On 2012/07/18 00:08:40, piman wrote:
> On 2012/07/17 23:59:31, greggman wrote:
> > On 2012/07/12 17:08:56, David Reveman wrote:
> > > On 2012/06/13 01:07:32, apatrick_chromium wrote:
> > > > The IOSurface support is broken so best not to copy it. I think the
pixmap
> > > > reference should go in TextureManager::TextureInfo so that if another
> > context
> > > in
> > > > the group deletes the texture the mapping from texture ID to pixmap does
> not
> > > > leak.
> > > 
> > > OK, so with IOSurface support considered broken the latest patch is what I
> > came
> > > up with. Let me know what you think.
> > > 
> > > > 
> > > > The XIDs will have to be secured somehow. You could add a browser to GPU
> > > process
> > > > IPC to put a texture bound to a pixmap in a mailbox and then use
> > > > CHROMIUM_texture_mailbox to push it into a particular context group.
> > > 
> > > I haven't done anything to address this in the latest patch as I'm not
sure
> I
> > > understand exactly what needs to be done. Is a browser to GPU process IPC
> for
> > > creating GLImage instances what I need to add? The texture I bind to a
> pixmap
> > is
> > > already in the correct context group (UI compositor group) so I don't see
> why
> > > CHROMIUM_texture_mailbox would need to be used here. What am I missing?
> > 
> > I mostly fine with this except I'd like to see how the ids are secured. As
it
> > is, any process can map any id just by calling TexImagePixmap and passing in
> an
> > id. I'm not even sure what those ids mean.
> 
> I think we should only expose the extension to the browser process, since
that's
> the only one that needs it - that way we don't need to secure ids one-by-one.

That would work. Is there a way to distinguish a browser process context from a
non-browser process context currently or does something need to be added?

[piman, 2012-07-18 07:15:25]

On Tue, Jul 17, 2012 at 6:22 PM, <gman@chromium.org> wrote:

> On 2012/07/18 00:08:40, piman wrote:
>
>> On 2012/07/17 23:59:31, greggman wrote:
>> > On 2012/07/12 17:08:56, David Reveman wrote:
>> > > On 2012/06/13 01:07:32, apatrick_chromium wrote:
>> > > > The IOSurface support is broken so best not to copy it. I think the
>>
> pixmap
>
>> > > > reference should go in TextureManager::TextureInfo so that if
>> another
>> > context
>> > > in
>> > > > the group deletes the texture the mapping from texture ID to pixmap
>> does
>> not
>> > > > leak.
>> > >
>> > > OK, so with IOSurface support considered broken the latest patch is
>> what I
>> > came
>> > > up with. Let me know what you think.
>> > >
>> > > >
>> > > > The XIDs will have to be secured somehow. You could add a browser
>> to GPU
>> > > process
>> > > > IPC to put a texture bound to a pixmap in a mailbox and then use
>> > > > CHROMIUM_texture_mailbox to push it into a particular context group.
>> > >
>> > > I haven't done anything to address this in the latest patch as I'm not
>>
> sure
>
>> I
>> > > understand exactly what needs to be done. Is a browser to GPU process
>> IPC
>> for
>> > > creating GLImage instances what I need to add? The texture I bind to a
>> pixmap
>> > is
>> > > already in the correct context group (UI compositor group) so I don't
>> see
>> why
>> > > CHROMIUM_texture_mailbox would need to be used here. What am I
>> missing?
>> >
>> > I mostly fine with this except I'd like to see how the ids are secured.
>> As
>>
> it
>
>> > is, any process can map any id just by calling TexImagePixmap and
>> passing in
>> an
>> > id. I'm not even sure what those ids mean.
>>
>
>  I think we should only expose the extension to the browser process, since
>>
> that's
>
>> the only one that needs it - that way we don't need to secure ids
>> one-by-one.
>>
>
> That would work. Is there a way to distinguish a browser process context
> from a
> non-browser process context currently or does something need to be added?
>

It would need to be added. It could be a flag passed with the channel
creation IPC (it goes through the browser process, so the renderer can't
"fake" it).


>
>
https://chromiumcodereview.**appspot.com/10543125/<https://chromiumcodereview...
>

[reveman, 2012-10-08 18:12:35]

I'm about to pick up this work again, but rather than using a flag passed with
the channel creation IPC, I'm planning to add IPC for creating and destroying
GLImages. Binding pixmaps in the GPU process but managing the lifetime of the
pixmaps in the browser is fragile as some drivers don't handle situations when a
bound pixmap is destroyed very well.

By adding IPC for creating a GLImage from a window handle and making the GPU
process manage the lifetime of the pixmap things become much easier.

[piman, 2012-10-08 19:20:26]

On Mon, Oct 8, 2012 at 11:12 AM, <reveman@chromium.org> wrote:

> I'm about to pick up this work again, but rather than using a flag passed
> with
> the channel creation IPC, I'm planning to add IPC for creating and
> destroying
> GLImages. Binding pixmaps in the GPU process but managing the lifetime of
> the
> pixmaps in the browser is fragile as some drivers don't handle situations
> when a
> bound pixmap is destroyed very well.
>
> By adding IPC for creating a GLImage from a window handle and making the
> GPU
> process manage the lifetime of the pixmap things become much easier.
>

Note: in Aura there's 2 channels from the browser to the GPU process:
- one non-sync channel for control (creating client channels and "view"
contexts), exists on all platforms but can't send sync messages on it
- one sync channel for Aura's contexts

I don't know which channel you're planning on adding that IPC, but, from
the pov of the GPU process, the second one looks just like a renderer
channel. I mention this because you either need to get that second channel
tagged as "safe" to do the TFP IPCs, or you'll need to do it in the first
one. But if you do it in the first one you'll have extra synchronization
issues to deal with (there's no ordering between IPCs on different
channels).

Antoine


>
>
https://chromiumcodereview.**appspot.com/10543125/<https://chromiumcodereview...
>

[reveman, 2012-10-08 21:11:01]

On 2012/10/08 19:20:26, piman wrote:
> On Mon, Oct 8, 2012 at 11:12 AM, <mailto:reveman@chromium.org> wrote:
> 
> > I'm about to pick up this work again, but rather than using a flag passed
> > with
> > the channel creation IPC, I'm planning to add IPC for creating and
> > destroying
> > GLImages. Binding pixmaps in the GPU process but managing the lifetime of
> > the
> > pixmaps in the browser is fragile as some drivers don't handle situations
> > when a
> > bound pixmap is destroyed very well.
> >
> > By adding IPC for creating a GLImage from a window handle and making the
> > GPU
> > process manage the lifetime of the pixmap things become much easier.
> >
> 
> Note: in Aura there's 2 channels from the browser to the GPU process:
> - one non-sync channel for control (creating client channels and "view"
> contexts), exists on all platforms but can't send sync messages on it
> - one sync channel for Aura's contexts
> 
> I don't know which channel you're planning on adding that IPC, but, from
> the pov of the GPU process, the second one looks just like a renderer
> channel. I mention this because you either need to get that second channel
> tagged as "safe" to do the TFP IPCs, or you'll need to do it in the first
> one. But if you do it in the first one you'll have extra synchronization
> issues to deal with (there's no ordering between IPCs on different
> channels).
> 
> Antoine

Thanks for explaining. My plan is to put it on the non-sync channel. The
synchronization I have to deal with is somewhat similar to the X11
synchronization I already have to deal with so this shouldn't make things
significantly more complicated.

[apatrick_chromium, 2012-10-09 19:24:35]

I don't know if this is necessarily the way to go but, just to make sure you are
aware of this extension, there's this:

http://src.chromium.org/viewvc/chrome/trunk/src/gpu/GLES2/extensions/CHROMIUM...

One approach to securing the pixmap IDs, would be to have a trusted IPC from the
browser process to the GPU process that associated a pixmap with a mailbox. Then
the renderer processes could use glConsumeTextureCHROMIUM to securely bind them
to GL textures objects.

As things stand, I think an untrusted renderer can get access to an arbitrary
pixmap since the XIDs are global?

I much prefer the approach you are taking here by the way. Thanks for doing it.

[reveman, 2012-10-11 22:42:13]

PTAL

[apatrick_chromium, 2012-10-11 23:09:57]

I'll be out for a week so I defer to others. Don't feel you need to wait for an
L G from me.

I did a quick read through.

Identifying the images with an ID that is scoped to the channel seems like a
reasonable way of protecting them and much better than a raw XID.

I am slightly concerned that a plugin that would otherwise be isolated in its
own process and sandboxed but guesses an image ID can render to it and / or read
it back.

https://codereview.chromium.org/10543125/diff/18001/content/common/gpu/client...
File content/common/gpu/client/webgraphicscontext3d_command_buffer_impl.h
(right):

https://codereview.chromium.org/10543125/diff/18001/content/common/gpu/client...
content/common/gpu/client/webgraphicscontext3d_command_buffer_impl.h:576:
virtual void texImagePixmap2DCHROMIUM(WGC3Denum target, WGC3Duint pixmapId);
Nit: this file is Chrome style so pixmapId -> pixmap_id. There are some other
places as well.

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

https://codereview.chromium.org/10543125/diff/18001/gpu/command_buffer/servic...
gpu/command_buffer/service/texture_manager.cc:598: DCHECK_GE(level, 0);
Should these range checks be stronger than DCHECKs? It seems a buggy /
compromised renderer might pass out an out-of-ranger level.

[tfarina, 2012-10-12 01:40:48]

http://codereview.chromium.org/10543125/diff/47001/ui/gl/gl_image.h
File ui/gl/gl_image.h (right):

http://codereview.chromium.org/10543125/diff/47001/ui/gl/gl_image.h#newcode31
ui/gl/gl_image.h:31: static scoped_refptr<GLImage>
CreateGLImage(gfx::PluginWindowHandle window);
nit: place this before the constructor? so it isn't in a random position between
pure virtual and virtual methods?

[reveman, 2012-10-12 03:29:11]

https://codereview.chromium.org/10543125/diff/18001/content/common/gpu/client...
File content/common/gpu/client/webgraphicscontext3d_command_buffer_impl.h
(right):

https://codereview.chromium.org/10543125/diff/18001/content/common/gpu/client...
content/common/gpu/client/webgraphicscontext3d_command_buffer_impl.h:576:
virtual void texImagePixmap2DCHROMIUM(WGC3Denum target, WGC3Duint pixmapId);
On 2012/10/11 23:09:57, apatrick_chromium wrote:
> Nit: this file is Chrome style so pixmapId -> pixmap_id. There are some other
> places as well.

Fixed.

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

https://codereview.chromium.org/10543125/diff/18001/gpu/command_buffer/servic...
gpu/command_buffer/service/texture_manager.cc:598: DCHECK_GE(level, 0);
On 2012/10/11 23:09:57, apatrick_chromium wrote:
> Should these range checks be stronger than DCHECKs? It seems a buggy /
> compromised renderer might pass out an out-of-ranger level.

I think it should be consistent with SetLevelInfo. The decoder currently
protects against a bad value being passed to SetLevelInfo by calling
TextureManager::ValidForTarget before using a level value from the renderer with
this function. The same should be done for SetLevelImage() if the renderer is
ever allowed to specify the level here.

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

https://codereview.chromium.org/10543125/diff/47001/ui/gl/gl_image.h#newcode31
ui/gl/gl_image.h:31: static scoped_refptr<GLImage>
CreateGLImage(gfx::PluginWindowHandle window);
On 2012/10/12 01:40:49, tfarina wrote:
> nit: place this before the constructor? so it isn't in a random position
between
> pure virtual and virtual methods?

Moved it below virtual methods as that's consistent with GLSurface.

[reveman, 2012-10-12 03:45:58]

On 2012/10/11 23:09:57, apatrick_chromium wrote:
> I'll be out for a week so I defer to others. Don't feel you need to wait for
an
> L G from me.
> 
> I did a quick read through.
> 
> Identifying the images with an ID that is scoped to the channel seems like a
> reasonable way of protecting them and much better than a raw XID.
> 
> I am slightly concerned that a plugin that would otherwise be isolated in its
> own process and sandboxed but guesses an image ID can render to it and / or
read
> it back.

Images are only going to be used by the UI compositor and they are not shared
between GPU channels so a plugin or renderer should not be able to access them
even if they manage to guess an ID.

If we for some reason want to share these images with a renderer in the future,
the existing mailbox mechanism should allow that. Not sure why we would want to
do that though. Maybe we'll create an extension API that allows people to
implement customized UI compositing using WebGL :)

> 
>
https://codereview.chromium.org/10543125/diff/18001/content/common/gpu/client...
> File content/common/gpu/client/webgraphicscontext3d_command_buffer_impl.h
> (right):
> 
>
https://codereview.chromium.org/10543125/diff/18001/content/common/gpu/client...
> content/common/gpu/client/webgraphicscontext3d_command_buffer_impl.h:576:
> virtual void texImagePixmap2DCHROMIUM(WGC3Denum target, WGC3Duint pixmapId);
> Nit: this file is Chrome style so pixmapId -> pixmap_id. There are some other
> places as well.
> 
>
https://codereview.chromium.org/10543125/diff/18001/gpu/command_buffer/servic...
> File gpu/command_buffer/service/texture_manager.cc (right):
> 
>
https://codereview.chromium.org/10543125/diff/18001/gpu/command_buffer/servic...
> gpu/command_buffer/service/texture_manager.cc:598: DCHECK_GE(level, 0);
> Should these range checks be stronger than DCHECKs? It seems a buggy /
> compromised renderer might pass out an out-of-ranger level.

[piman, 2012-10-12 22:05:11]

https://codereview.chromium.org/10543125/diff/29052/content/browser/gpu/brows...
File content/browser/gpu/browser_gpu_channel_host_factory.cc (right):

https://codereview.chromium.org/10543125/diff/29052/content/browser/gpu/brows...
content/browser/gpu/browser_gpu_channel_host_factory.cc:197: image_id));
Note: there's still a race on destruction. Take this example:

CreateImage(XID, id, callback);

callback() {
  context->bindTexImage2D(id);
  context->drawQuad();
  context->releaseTexImage2D(id)
  DeleteImage(id);
}

In this case, the DeleteImage may be executed before the bindImage has a chance
to happen.

One way out is to have DeleteImage on the context's channel (it's similar to how
we create view contexts through the process channel, but they get deleted from
the context channel).
Another way is ask for a sync point here, and pass it along, so that on the GPU
process side you can wait for the referencing commands to be finished. It makes
it more verbose from the client's pov.

Depending on the image id allocation scheme though, there may still be races
with the first one. If image ids get re-used aggressively, you could get into:

[...]
DeleteImage(id);
CreateImage(XID, id, callback);

In the first case, CreateImage might be executed before the DeleteImage, and
hilarity ensues.

If ids aren't re-used before wrapping, we can probably construct an argument
that it can't happen (you'd need to exhaust memory to have 4B DeleteImage
messages in flight).
Something to note somewhere.

https://codereview.chromium.org/10543125/diff/29052/content/common/gpu/gpu_ch...
File content/common/gpu/gpu_channel.cc (right):

https://codereview.chromium.org/10543125/diff/29052/content/common/gpu/gpu_ch...
content/common/gpu/gpu_channel.cc:366: image_manager_->AddImage(image,
image_id);
Should we consider the case where image_id already exists?
Currently it will replace the image in the image_manager_ but textures may still
be referencing the old one. It may cause a bunch of surprises later.

While I assume the client would of course try to avoid that, if ids get re-used,
races can come in (see comments in browser_gpu_channel_host_factory).

https://codereview.chromium.org/10543125/diff/29052/content/renderer/render_t...
File content/renderer/render_thread_impl.cc (right):

https://codereview.chromium.org/10543125/diff/29052/content/renderer/render_t...
content/renderer/render_thread_impl.cc:968: const CreateImageCallback& callback)
{
NOTREACHED() ?

https://codereview.chromium.org/10543125/diff/29052/content/renderer/render_t...
content/renderer/render_thread_impl.cc:972: void
RenderThreadImpl::DeleteImage(int32 image_id) {
NOTREACHED() ?

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

https://codereview.chromium.org/10543125/diff/29052/gpu/command_buffer/servic...
gpu/command_buffer/service/gles2_cmd_decoder.cc:9495: GL_BGRA,
GL_UNSIGNED_INT_8_8_8_8_REV, true);
GL_BGRA, GL_UNSIGNED_INT_8_8_8_8_REV ?
Technically internal_format should be the same as format (gles2 constraint), and
I have a feeling GL_UNSIGNED_INT_8_8_8_8_REV should be GL_UNSIGNED_BYTE anyway.

Maybe it has no effect.

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

https://codereview.chromium.org/10543125/diff/29052/ui/gl/gl_image.h#newcode30
ui/gl/gl_image.h:30: // Bind image to texture.
Which texture?

https://codereview.chromium.org/10543125/diff/29052/ui/gl/gl_image_glx.cc
File ui/gl/gl_image_glx.cc (right):

https://codereview.chromium.org/10543125/diff/29052/ui/gl/gl_image_glx.cc#new...
ui/gl/gl_image_glx.cc:47: unsigned int width, height, border_width, depth;
nit: please initialize all local variables.

https://codereview.chromium.org/10543125/diff/29052/ui/gl/gl_image_glx.cc#new...
ui/gl/gl_image_glx.cc:68: int i;
nit move the = 0 initialization here.

https://codereview.chromium.org/10543125/diff/29052/ui/gl/gl_image_glx.cc#new...
ui/gl/gl_image_glx.cc:70: int value;
nit: initialize

https://codereview.chromium.org/10543125/diff/29052/ui/gl/gl_image_glx.cc#new...
ui/gl/gl_image_glx.cc:88: GLX_BIND_TO_TEXTURE_RGBA_EXT :
GLX_BIND_TO_TEXTURE_RGB_EXT,
so, techincally, we can have RGB windows in a 32 bit visual, in which case alpha
may be garbage. Should we inspect the window's visual instead of the resulting
pixmap's depth?

https://codereview.chromium.org/10543125/diff/29052/ui/gl/gl_image_glx.cc#new...
ui/gl/gl_image_glx.cc:113: }
Can we replace all this by glXChooseFBConfig?

https://codereview.chromium.org/10543125/diff/29052/ui/gl/gl_image_glx.cc#new...
ui/gl/gl_image_glx.cc:128: attribs.push_back(0);
Since the array is fixed size, can we just make this an array? It'll be both
more readable and more efficient, while avoiding the ugliness of passing a
pointer to the underlying storage ( &attribs.front() ).

https://codereview.chromium.org/10543125/diff/29052/webkit/gpu/webgraphicscon...
File webkit/gpu/webgraphicscontext3d_in_process_impl.cc (right):

https://codereview.chromium.org/10543125/diff/29052/webkit/gpu/webgraphicscon...
webkit/gpu/webgraphicscontext3d_in_process_impl.cc:1702: WGC3Denum target,
WGC3Dint imageId) {
NOTIMPLEMENTED(); here and below

[reveman, 2012-10-13 05:25:53]

https://codereview.chromium.org/10543125/diff/29052/content/browser/gpu/brows...
File content/browser/gpu/browser_gpu_channel_host_factory.cc (right):

https://codereview.chromium.org/10543125/diff/29052/content/browser/gpu/brows...
content/browser/gpu/browser_gpu_channel_host_factory.cc:197: image_id));
On 2012/10/12 22:05:11, piman wrote:
> Note: there's still a race on destruction. Take this example:
> 
> CreateImage(XID, id, callback);
> 
> callback() {
>   context->bindTexImage2D(id);
>   context->drawQuad();
>   context->releaseTexImage2D(id)
>   DeleteImage(id);
> }
> 
> In this case, the DeleteImage may be executed before the bindImage has a
chance
> to happen.
> 
> One way out is to have DeleteImage on the context's channel (it's similar to
how
> we create view contexts through the process channel, but they get deleted from
> the context channel).
> Another way is ask for a sync point here, and pass it along, so that on the
GPU
> process side you can wait for the referencing commands to be finished. It
makes
> it more verbose from the client's pov.
> 
> Depending on the image id allocation scheme though, there may still be races
> with the first one. If image ids get re-used aggressively, you could get into:
> 
> [...]
> DeleteImage(id);
> CreateImage(XID, id, callback);
> 
> In the first case, CreateImage might be executed before the DeleteImage, and
> hilarity ensues.
> 
> If ids aren't re-used before wrapping, we can probably construct an argument
> that it can't happen (you'd need to exhaust memory to have 4B DeleteImage
> messages in flight).
> Something to note somewhere.

I've added a sync point to DeleteImage. I liked that approach.

https://codereview.chromium.org/10543125/diff/29052/content/common/gpu/gpu_ch...
File content/common/gpu/gpu_channel.cc (right):

https://codereview.chromium.org/10543125/diff/29052/content/common/gpu/gpu_ch...
content/common/gpu/gpu_channel.cc:366: image_manager_->AddImage(image,
image_id);
On 2012/10/12 22:05:11, piman wrote:
> Should we consider the case where image_id already exists?
> Currently it will replace the image in the image_manager_ but textures may
still
> be referencing the old one. It may cause a bunch of surprises later.
> 
> While I assume the client would of course try to avoid that, if ids get
re-used,
> races can come in (see comments in browser_gpu_channel_host_factory).

I made it fail if an image_id already exists.

https://codereview.chromium.org/10543125/diff/29052/content/renderer/render_t...
File content/renderer/render_thread_impl.cc (right):

https://codereview.chromium.org/10543125/diff/29052/content/renderer/render_t...
content/renderer/render_thread_impl.cc:968: const CreateImageCallback& callback)
{
On 2012/10/12 22:05:11, piman wrote:
> NOTREACHED() ?

Done.

https://codereview.chromium.org/10543125/diff/29052/content/renderer/render_t...
content/renderer/render_thread_impl.cc:972: void
RenderThreadImpl::DeleteImage(int32 image_id) {
On 2012/10/12 22:05:11, piman wrote:
> NOTREACHED() ?

Done.

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

https://codereview.chromium.org/10543125/diff/29052/gpu/command_buffer/servic...
gpu/command_buffer/service/gles2_cmd_decoder.cc:9495: GL_BGRA,
GL_UNSIGNED_INT_8_8_8_8_REV, true);
On 2012/10/12 22:05:11, piman wrote:
> GL_BGRA, GL_UNSIGNED_INT_8_8_8_8_REV ?
> Technically internal_format should be the same as format (gles2 constraint),
and
> I have a feeling GL_UNSIGNED_INT_8_8_8_8_REV should be GL_UNSIGNED_BYTE
anyway.
> 
> Maybe it has no effect.

I don't think it has an effect but I changed to GL_RGBA and GL_UNSIGNED_BYTE. I
think I picked those other values so that it would be consistent with
DoTexImageIOSurface2DCHROMIUM but maybe we should change that function too.

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

https://codereview.chromium.org/10543125/diff/29052/ui/gl/gl_image.h#newcode30
ui/gl/gl_image.h:30: // Bind image to texture.
On 2012/10/12 22:05:11, piman wrote:
> Which texture?
Bind image to texture currently bound to GL_TEXTURE_2D target.

Updated comment.

https://codereview.chromium.org/10543125/diff/29052/ui/gl/gl_image_glx.cc
File ui/gl/gl_image_glx.cc (right):

https://codereview.chromium.org/10543125/diff/29052/ui/gl/gl_image_glx.cc#new...
ui/gl/gl_image_glx.cc:47: unsigned int width, height, border_width, depth;
On 2012/10/12 22:05:11, piman wrote:
> nit: please initialize all local variables.

Done.

https://codereview.chromium.org/10543125/diff/29052/ui/gl/gl_image_glx.cc#new...
ui/gl/gl_image_glx.cc:68: int i;
On 2012/10/12 22:05:11, piman wrote:
> nit move the = 0 initialization here.

Done.

https://codereview.chromium.org/10543125/diff/29052/ui/gl/gl_image_glx.cc#new...
ui/gl/gl_image_glx.cc:70: int value;
On 2012/10/12 22:05:11, piman wrote:
> nit: initialize

Done.

https://codereview.chromium.org/10543125/diff/29052/ui/gl/gl_image_glx.cc#new...
ui/gl/gl_image_glx.cc:88: GLX_BIND_TO_TEXTURE_RGBA_EXT :
GLX_BIND_TO_TEXTURE_RGB_EXT,
On 2012/10/12 22:05:11, piman wrote:
> so, techincally, we can have RGB windows in a 32 bit visual, in which case
alpha
> may be garbage. Should we inspect the window's visual instead of the resulting
> pixmap's depth?

Done.

https://codereview.chromium.org/10543125/diff/29052/ui/gl/gl_image_glx.cc#new...
ui/gl/gl_image_glx.cc:113: }
On 2012/10/12 22:05:11, piman wrote:
> Can we replace all this by glXChooseFBConfig?

Done.

https://codereview.chromium.org/10543125/diff/29052/ui/gl/gl_image_glx.cc#new...
ui/gl/gl_image_glx.cc:128: attribs.push_back(0);
On 2012/10/12 22:05:11, piman wrote:
> Since the array is fixed size, can we just make this an array? It'll be both
> more readable and more efficient, while avoiding the ugliness of passing a
> pointer to the underlying storage ( &attribs.front() ).

Done.

https://codereview.chromium.org/10543125/diff/29052/webkit/gpu/webgraphicscon...
File webkit/gpu/webgraphicscontext3d_in_process_impl.cc (right):

https://codereview.chromium.org/10543125/diff/29052/webkit/gpu/webgraphicscon...
webkit/gpu/webgraphicscontext3d_in_process_impl.cc:1702: WGC3Denum target,
WGC3Dint imageId) {
On 2012/10/12 22:05:11, piman wrote:
> NOTIMPLEMENTED(); here and below

Done.

[piman, 2012-10-13 18:00:37]

Ok, one last thing...

https://codereview.chromium.org/10543125/diff/60001/content/common/gpu/gpu_ch...
File content/common/gpu/gpu_channel_manager.cc (right):

https://codereview.chromium.org/10543125/diff/60001/content/common/gpu/gpu_ch...
content/common/gpu/gpu_channel_manager.cc:195: if (image_sync_point_)
I'm not convinced :)
It's possible - at least in theory - that the client would create 2 sync points,
but call DeleteImage on sync points in opposite order:

sync_point1 = context->insertSyncPoint();
[...]
sync_point2 = context->insertSyncPoint();
[...]
DeleteImage(id1, sync_point2);
DeleteImage(id2, sync_point1);
[...]
CreateImage(XID, id1, callback);

In this case, image_sync_point_ will be sync_point1, and you won't be waiting on
sync_point2 (if that's what you were trying to do).

https://codereview.chromium.org/10543125/diff/60001/content/common/gpu/gpu_ch...
content/common/gpu/gpu_channel_manager.cc:225: if (image_sync_point_)
nit: style requires braces (on both clauses) when one clause takes more than one
line.

[reveman, 2012-10-17 07:37:11]

PTAL

https://codereview.chromium.org/10543125/diff/60001/content/common/gpu/gpu_ch...
File content/common/gpu/gpu_channel_manager.cc (right):

https://codereview.chromium.org/10543125/diff/60001/content/common/gpu/gpu_ch...
content/common/gpu/gpu_channel_manager.cc:195: if (image_sync_point_)
On 2012/10/13 18:00:38, piman wrote:
> I'm not convinced :)
> It's possible - at least in theory - that the client would create 2 sync
points,
> but call DeleteImage on sync points in opposite order:
> 
> sync_point1 = context->insertSyncPoint();
> [...]
> sync_point2 = context->insertSyncPoint();
> [...]
> DeleteImage(id1, sync_point2);
> DeleteImage(id2, sync_point1);
> [...]
> CreateImage(XID, id1, callback);
> 
> In this case, image_sync_point_ will be sync_point1, and you won't be waiting
on
> sync_point2 (if that's what you were trying to do).

Latest patch puts all image ops in a queue and pops ops off the queue until an
op with a pending sync point is reached.

https://codereview.chromium.org/10543125/diff/60001/content/common/gpu/gpu_ch...
content/common/gpu/gpu_channel_manager.cc:225: if (image_sync_point_)
On 2012/10/13 18:00:38, piman wrote:
> nit: style requires braces (on both clauses) when one clause takes more than
one
> line.

Done.

[piman, 2012-10-17 17:46:20]

https://codereview.chromium.org/10543125/diff/55007/content/common/gpu/gpu_ch...
File content/common/gpu/gpu_channel_manager.cc (right):

https://codereview.chromium.org/10543125/diff/55007/content/common/gpu/gpu_ch...
content/common/gpu/gpu_channel_manager.cc:193: else
nit: style requires braces here because the statement spans multiple lines.
Also, it means you have to use braces on the other side of the else.

https://codereview.chromium.org/10543125/diff/55007/content/common/gpu/gpu_ch...
content/common/gpu/gpu_channel_manager.cc:221: if (sync_point)
nit: style requires braces

https://codereview.chromium.org/10543125/diff/55007/content/common/gpu/gpu_ch...
content/common/gpu/gpu_channel_manager.cc:226: &image_operations_.back()));
I don't think taking a pointer to the deque entry is safe here, since all
insertions/removals invalidate all iterators.

https://codereview.chromium.org/10543125/diff/55007/content/common/gpu/gpu_ch...
File content/common/gpu/gpu_channel_manager.h (right):

https://codereview.chromium.org/10543125/diff/55007/content/common/gpu/gpu_ch...
content/common/gpu/gpu_channel_manager.h:100: typedef std::pair<int32,
base::Closure> ImageOperation;
nit: we usually avoid std::pair except for implicit types. Rather, a simple
struct (with a trivial constructor if it's useful) is usually better because it
gives proper names to the fields. For example, here, it's very difficult to know
if the first parameter represents an image ID, a client ID or a sync point.
Where it gets used, we have to mentally translate "first" and "second" into
whichever field this is meant to be.

https://codereview.chromium.org/10543125/diff/55007/content/common/gpu/gpu_ch...
content/common/gpu/gpu_channel_manager.h:140: typedef std::deque<ImageOperation>
ImageOperationQueue;
nit: typedefs usually go together at the top of the "private" section.
(agreed this file isn't very consistent with this)

[greggman, 2012-10-17 18:32:16]

Looks good. 

I agree with Al though, there's a security issue in that any context (pepper or
compromised renderer) can just scan for GLImages by going

  for (GLuint ii = 0; ii < 10000000; ++ii) {
    glBindTexImage2DCHROMIUM(GL_TEXTURE_2D, ii);
    if (glGetError() == NO_ERROR) {
       // found a GLimage even if it belongs
       // to someone else.
    }
  }

Ideally, only images that a particular context has permission to see should be
accessible by that context.

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

https://codereview.chromium.org/10543125/diff/55007/gpu/command_buffer/servic...
gpu/command_buffer/service/gles2_cmd_decoder.cc:757: GLenum target,
style: indent 4

https://codereview.chromium.org/10543125/diff/55007/gpu/command_buffer/servic...
File gpu/command_buffer/service/image_manager.h (right):

https://codereview.chromium.org/10543125/diff/55007/gpu/command_buffer/servic...
gpu/command_buffer/service/image_manager.h:25: void
AddImage(scoped_refptr<gfx::GLImage> gl_image, int32 service_id);
Is there some reason to pass around scoped_refptrs? I thought generally we
passed raw pointers around since scoped_refptrs can be made from raw pointers
and because passing them around means lots of extra ref count manipulation.

[piman, 2012-10-17 18:35:27]

On Wed, Oct 17, 2012 at 11:32 AM, <gman@chromium.org> wrote:

> Looks good.
>
> I agree with Al though, there's a security issue in that any context
> (pepper or
> compromised renderer) can just scan for GLImages by going
>

The GLImage namespace is per-channel, so they can't, because renderers and
browsers don't share a channel?


>   for (GLuint ii = 0; ii < 10000000; ++ii) {
>     glBindTexImage2DCHROMIUM(GL_**TEXTURE_2D, ii);
>     if (glGetError() == NO_ERROR) {
>        // found a GLimage even if it belongs
>        // to someone else.
>     }
>   }
>
> Ideally, only images that a particular context has permission to see
> should be
> accessible by that context.
>
>
> https://codereview.chromium.**org/10543125/diff/55007/gpu/**
>
command_buffer/service/gles2_**cmd_decoder.cc<https://codereview.chromium.org/10543125/diff/55007/gpu/command_buffer/service/gles2_cmd_decoder.cc>
> File gpu/command_buffer/service/**gles2_cmd_decoder.cc (right):
>
> https://codereview.chromium.**org/10543125/diff/55007/gpu/**
>
command_buffer/service/gles2_**cmd_decoder.cc#newcode757<https://codereview.chromium.org/10543125/diff/55007/gpu/command_buffer/service/gles2_cmd_decoder.cc#newcode757>
> gpu/command_buffer/service/**gles2_cmd_decoder.cc:757: GLenum target,
> style: indent 4
>
> https://codereview.chromium.**org/10543125/diff/55007/gpu/**
>
command_buffer/service/image_**manager.h<https://codereview.chromium.org/10543125/diff/55007/gpu/command_buffer/service/image_manager.h>
> File gpu/command_buffer/service/**image_manager.h (right):
>
> https://codereview.chromium.**org/10543125/diff/55007/gpu/**
>
command_buffer/service/image_**manager.h#newcode25<https://codereview.chromium.org/10543125/diff/55007/gpu/command_buffer/service/image_manager.h#newcode25>
> gpu/command_buffer/service/**image_manager.h:25: void
> AddImage(scoped_refptr<gfx::**GLImage> gl_image, int32 service_id);
> Is there some reason to pass around scoped_refptrs? I thought generally
> we passed raw pointers around since scoped_refptrs can be made from raw
> pointers and because passing them around means lots of extra ref count
> manipulation.
>
>
https://codereview.chromium.**org/10543125/<https://codereview.chromium.org/1...
>

[greggman, 2012-10-17 20:33:55]

On 2012/10/17 18:35:27, piman wrote:
> On Wed, Oct 17, 2012 at 11:32 AM, <mailto:gman@chromium.org> wrote:
> 
> > Looks good.
> >
> > I agree with Al though, there's a security issue in that any context
> > (pepper or
> > compromised renderer) can just scan for GLImages by going
> >
> 
> The GLImage namespace is per-channel, so they can't, because renderers and
> browsers don't share a channel?
> 

Sorry I got confused.  Sounds good.

LGTM though I'd still like to hear about the scoped_refptr arguments vs raw
pointers issue out of curiosity.

[reveman, 2012-10-17 21:22:54]

https://codereview.chromium.org/10543125/diff/55007/content/common/gpu/gpu_ch...
File content/common/gpu/gpu_channel_manager.cc (right):

https://codereview.chromium.org/10543125/diff/55007/content/common/gpu/gpu_ch...
content/common/gpu/gpu_channel_manager.cc:193: else
On 2012/10/17 17:46:20, piman wrote:
> nit: style requires braces here because the statement spans multiple lines.
> Also, it means you have to use braces on the other side of the else.

Ah, braces required for multiple lines and not just multiple statements. Didn't
know that.

https://codereview.chromium.org/10543125/diff/55007/content/common/gpu/gpu_ch...
content/common/gpu/gpu_channel_manager.cc:226: &image_operations_.back()));
On 2012/10/17 17:46:20, piman wrote:
> I don't think taking a pointer to the deque entry is safe here, since all
> insertions/removals invalidate all iterators.

Oops, that's bad.

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

https://codereview.chromium.org/10543125/diff/55007/gpu/command_buffer/servic...
gpu/command_buffer/service/gles2_cmd_decoder.cc:757: GLenum target,
On 2012/10/17 18:32:16, greggman wrote:
> style: indent 4

Sure. We have 10 declarations with indent 2 above. I'll change all of them to
indent 4 as I don't like this to be inconsistent.

https://codereview.chromium.org/10543125/diff/55007/gpu/command_buffer/servic...
File gpu/command_buffer/service/image_manager.h (right):

https://codereview.chromium.org/10543125/diff/55007/gpu/command_buffer/servic...
gpu/command_buffer/service/image_manager.h:25: void
AddImage(scoped_refptr<gfx::GLImage> gl_image, int32 service_id);
On 2012/10/17 18:32:16, greggman wrote:
> Is there some reason to pass around scoped_refptrs? I thought generally we
> passed raw pointers around since scoped_refptrs can be made from raw pointers
> and because passing them around means lots of extra ref count manipulation.

You can have an image bound to multiple textures and deleting an image doesn't
affect current bindings. The image instance is deleted when the image_id is gone
and no texture bindings exist:

tex1 = context->createTexture();
CreateImage(XID, img1, callback1);

callback1() {
context->bindTexture(tex1);
context->bindTexImage2DCHROMIUM(img1);
DeleteImage(img1, context->insertSyncPoint());
context->drawArrays(..); // Draw image img1
}

[greggman, 2012-10-17 21:40:15]

On 2012/10/17 21:22:54, David Reveman wrote:
>
https://codereview.chromium.org/10543125/diff/55007/content/common/gpu/gpu_ch...
> File content/common/gpu/gpu_channel_manager.cc (right):
> 
>
https://codereview.chromium.org/10543125/diff/55007/content/common/gpu/gpu_ch...
> content/common/gpu/gpu_channel_manager.cc:193: else
> On 2012/10/17 17:46:20, piman wrote:
> > nit: style requires braces here because the statement spans multiple lines.
> > Also, it means you have to use braces on the other side of the else.
> 
> Ah, braces required for multiple lines and not just multiple statements.
Didn't
> know that.
> 
>
https://codereview.chromium.org/10543125/diff/55007/content/common/gpu/gpu_ch...
> content/common/gpu/gpu_channel_manager.cc:226: &image_operations_.back()));
> On 2012/10/17 17:46:20, piman wrote:
> > I don't think taking a pointer to the deque entry is safe here, since all
> > insertions/removals invalidate all iterators.
> 
> Oops, that's bad.
> 
>
https://codereview.chromium.org/10543125/diff/55007/gpu/command_buffer/servic...
> File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):
> 
>
https://codereview.chromium.org/10543125/diff/55007/gpu/command_buffer/servic...
> gpu/command_buffer/service/gles2_cmd_decoder.cc:757: GLenum target,
> On 2012/10/17 18:32:16, greggman wrote:
> > style: indent 4
> 
> Sure. We have 10 declarations with indent 2 above. I'll change all of them to
> indent 4 as I don't like this to be inconsistent.
> 
>
https://codereview.chromium.org/10543125/diff/55007/gpu/command_buffer/servic...
> File gpu/command_buffer/service/image_manager.h (right):
> 
>
https://codereview.chromium.org/10543125/diff/55007/gpu/command_buffer/servic...
> gpu/command_buffer/service/image_manager.h:25: void
> AddImage(scoped_refptr<gfx::GLImage> gl_image, int32 service_id);
> On 2012/10/17 18:32:16, greggman wrote:
> > Is there some reason to pass around scoped_refptrs? I thought generally we
> > passed raw pointers around since scoped_refptrs can be made from raw
pointers
> > and because passing them around means lots of extra ref count manipulation.
> 
> You can have an image bound to multiple textures and deleting an image doesn't
> affect current bindings. The image instance is deleted when the image_id is
gone
> and no texture bindings exist:
> 
> tex1 = context->createTexture();
> CreateImage(XID, img1, callback1);
> 
> callback1() {
> context->bindTexture(tex1);
> context->bindTexImage2DCHROMIUM(img1);
> DeleteImage(img1, context->insertSyncPoint());
> context->drawArrays(..); // Draw image img1
> }

I'm confused what that has to do with passing scoped_refptr around? With
scoped_refptrs this is fine

class Foo() {
  Thing* GetThing() { return t_.get(); }
  void SetThing(Thing* t) { t_.reset(t); }
  scoped_refptr<Thing> t_;
};

Foo a;
Foo b;

Thing* t = a.GetThing();
b.SetThing(t);

This is also ok

scoped_refptr<Thing*> t(a.GetThing());


No need to have GetThing or SetThing take a scoped_refptr. It only generates
more code.

The only caveat is if a function removes the last reference but that situation
is rare enough that it's not worth the other complications

[reveman, 2012-10-17 21:57:48]

On 2012/10/17 21:40:15, greggman wrote:
> On 2012/10/17 21:22:54, David Reveman wrote:
> >
>
https://codereview.chromium.org/10543125/diff/55007/content/common/gpu/gpu_ch...
> > File content/common/gpu/gpu_channel_manager.cc (right):
> > 
> >
>
https://codereview.chromium.org/10543125/diff/55007/content/common/gpu/gpu_ch...
> > content/common/gpu/gpu_channel_manager.cc:193: else
> > On 2012/10/17 17:46:20, piman wrote:
> > > nit: style requires braces here because the statement spans multiple
lines.
> > > Also, it means you have to use braces on the other side of the else.
> > 
> > Ah, braces required for multiple lines and not just multiple statements.
> Didn't
> > know that.
> > 
> >
>
https://codereview.chromium.org/10543125/diff/55007/content/common/gpu/gpu_ch...
> > content/common/gpu/gpu_channel_manager.cc:226: &image_operations_.back()));
> > On 2012/10/17 17:46:20, piman wrote:
> > > I don't think taking a pointer to the deque entry is safe here, since all
> > > insertions/removals invalidate all iterators.
> > 
> > Oops, that's bad.
> > 
> >
>
https://codereview.chromium.org/10543125/diff/55007/gpu/command_buffer/servic...
> > File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):
> > 
> >
>
https://codereview.chromium.org/10543125/diff/55007/gpu/command_buffer/servic...
> > gpu/command_buffer/service/gles2_cmd_decoder.cc:757: GLenum target,
> > On 2012/10/17 18:32:16, greggman wrote:
> > > style: indent 4
> > 
> > Sure. We have 10 declarations with indent 2 above. I'll change all of them
to
> > indent 4 as I don't like this to be inconsistent.
> > 
> >
>
https://codereview.chromium.org/10543125/diff/55007/gpu/command_buffer/servic...
> > File gpu/command_buffer/service/image_manager.h (right):
> > 
> >
>
https://codereview.chromium.org/10543125/diff/55007/gpu/command_buffer/servic...
> > gpu/command_buffer/service/image_manager.h:25: void
> > AddImage(scoped_refptr<gfx::GLImage> gl_image, int32 service_id);
> > On 2012/10/17 18:32:16, greggman wrote:
> > > Is there some reason to pass around scoped_refptrs? I thought generally we
> > > passed raw pointers around since scoped_refptrs can be made from raw
> pointers
> > > and because passing them around means lots of extra ref count
manipulation.
> > 
> > You can have an image bound to multiple textures and deleting an image
doesn't
> > affect current bindings. The image instance is deleted when the image_id is
> gone
> > and no texture bindings exist:
> > 
> > tex1 = context->createTexture();
> > CreateImage(XID, img1, callback1);
> > 
> > callback1() {
> > context->bindTexture(tex1);
> > context->bindTexImage2DCHROMIUM(img1);
> > DeleteImage(img1, context->insertSyncPoint());
> > context->drawArrays(..); // Draw image img1
> > }
> 
> I'm confused what that has to do with passing scoped_refptr around? With
> scoped_refptrs this is fine
> 
> class Foo() {
>   Thing* GetThing() { return t_.get(); }
>   void SetThing(Thing* t) { t_.reset(t); }
>   scoped_refptr<Thing> t_;
> };
> 
> Foo a;
> Foo b;
> 
> Thing* t = a.GetThing();
> b.SetThing(t);
> 
> This is also ok
> 
> scoped_refptr<Thing*> t(a.GetThing());
> 
> 
> No need to have GetThing or SetThing take a scoped_refptr. It only generates
> more code.
> 
> The only caveat is if a function removes the last reference but that situation
> is rare enough that it's not worth the other complications

Oh, sorry. I've been spending too much time in WebKit land and was thinking that
scoped_ptr==OwnPtr and scoped_refptr==RefPtr. All we need here is reference
counting. I'll update the patch.

[reveman, 2012-10-17 22:31:18]

On 2012/10/17 21:40:15, greggman wrote:
> On 2012/10/17 21:22:54, David Reveman wrote:
> >
>
https://codereview.chromium.org/10543125/diff/55007/content/common/gpu/gpu_ch...
> > File content/common/gpu/gpu_channel_manager.cc (right):
> > 
> >
>
https://codereview.chromium.org/10543125/diff/55007/content/common/gpu/gpu_ch...
> > content/common/gpu/gpu_channel_manager.cc:193: else
> > On 2012/10/17 17:46:20, piman wrote:
> > > nit: style requires braces here because the statement spans multiple
lines.
> > > Also, it means you have to use braces on the other side of the else.
> > 
> > Ah, braces required for multiple lines and not just multiple statements.
> Didn't
> > know that.
> > 
> >
>
https://codereview.chromium.org/10543125/diff/55007/content/common/gpu/gpu_ch...
> > content/common/gpu/gpu_channel_manager.cc:226: &image_operations_.back()));
> > On 2012/10/17 17:46:20, piman wrote:
> > > I don't think taking a pointer to the deque entry is safe here, since all
> > > insertions/removals invalidate all iterators.
> > 
> > Oops, that's bad.
> > 
> >
>
https://codereview.chromium.org/10543125/diff/55007/gpu/command_buffer/servic...
> > File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):
> > 
> >
>
https://codereview.chromium.org/10543125/diff/55007/gpu/command_buffer/servic...
> > gpu/command_buffer/service/gles2_cmd_decoder.cc:757: GLenum target,
> > On 2012/10/17 18:32:16, greggman wrote:
> > > style: indent 4
> > 
> > Sure. We have 10 declarations with indent 2 above. I'll change all of them
to
> > indent 4 as I don't like this to be inconsistent.
> > 
> >
>
https://codereview.chromium.org/10543125/diff/55007/gpu/command_buffer/servic...
> > File gpu/command_buffer/service/image_manager.h (right):
> > 
> >
>
https://codereview.chromium.org/10543125/diff/55007/gpu/command_buffer/servic...
> > gpu/command_buffer/service/image_manager.h:25: void
> > AddImage(scoped_refptr<gfx::GLImage> gl_image, int32 service_id);
> > On 2012/10/17 18:32:16, greggman wrote:
> > > Is there some reason to pass around scoped_refptrs? I thought generally we
> > > passed raw pointers around since scoped_refptrs can be made from raw
> pointers
> > > and because passing them around means lots of extra ref count
manipulation.
> > 
> > You can have an image bound to multiple textures and deleting an image
doesn't
> > affect current bindings. The image instance is deleted when the image_id is
> gone
> > and no texture bindings exist:
> > 
> > tex1 = context->createTexture();
> > CreateImage(XID, img1, callback1);
> > 
> > callback1() {
> > context->bindTexture(tex1);
> > context->bindTexImage2DCHROMIUM(img1);
> > DeleteImage(img1, context->insertSyncPoint());
> > context->drawArrays(..); // Draw image img1
> > }
> 
> I'm confused what that has to do with passing scoped_refptr around? With
> scoped_refptrs this is fine
> 
> class Foo() {
>   Thing* GetThing() { return t_.get(); }
>   void SetThing(Thing* t) { t_.reset(t); }
>   scoped_refptr<Thing> t_;
> };
> 
> Foo a;
> Foo b;
> 
> Thing* t = a.GetThing();
> b.SetThing(t);
> 
> This is also ok
> 
> scoped_refptr<Thing*> t(a.GetThing());
> 
> 
> No need to have GetThing or SetThing take a scoped_refptr. It only generates
> more code.
> 
> The only caveat is if a function removes the last reference but that situation
> is rare enough that it's not worth the other complications

So the platform abstraction code used for GLImage relies on scoped_refptr and
being able to return NULL. GLSurface/GLContext uses the same abstraction and I
prefer to keep this consistent. Is that OK?

[piman, 2012-10-18 03:13:53]

lgtm

[reveman, 2012-10-18 03:21:03]

Gregg, sorry for the scoped_refptr confusion. This latest patch should be
getting it right. Glad you pointed it out.

[commit-bot: I haz the power, 2012-10-18 03:21:24]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/reveman@chromium.org/10543125/79002

[commit-bot: I haz the power, 2012-10-18 05:15:57]

Change committed as 162654