Add command buffer support for GL_RGB CHROMIUM image emulation.

Add command buffer support for GL_RGB CHROMIUM image emulation.

The CHROMIUM image is always created with internal format GL_RGBA. The caller of
CreateGpuMemoryBufferImageCHROMIUM is responsible for most emulation. The only
support that the command buffer provides is that calls to copyTexImage2D and
copyTexSubImage2D will perform parameter validation as if the internal format
were RGB.

BUG=595948
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel;tryserver.chromium.win:win_optional_gpu_tests_rel

Committed: https://crrev.com/6e4845661eba8e56f36aa32c788f859ebafc8403
Cr-Commit-Position: refs/heads/master@{#387779}

[erikchen, 2016-04-06 23:08:34]

Description was changed from

==========
Add command buffer support for GL_RGB CHROMIUM image emulation.

The CHROMIUM image is always created with internal format GL_RGBA. The caller of
CreateGpuMemoryBufferImageCHROMIUM is responsible for most emulation. The only
support that the command buffer provides is that calls to copyTexImage2D and
copyTexSubImage2D will perform parameter validation as if the internal format
were RGB.

BUG=
==========

to

==========
Add command buffer support for GL_RGB CHROMIUM image emulation.

The CHROMIUM image is always created with internal format GL_RGBA. The caller of
CreateGpuMemoryBufferImageCHROMIUM is responsible for most emulation. The only
support that the command buffer provides is that calls to copyTexImage2D and
copyTexSubImage2D will perform parameter validation as if the internal format
were RGB.

BUG=
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel;tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[erikchen, 2016-04-06 23:26:29]

erikchen@chromium.org changed reviewers:
+ kbr@chromium.org, reveman@chromium.org

[erikchen, 2016-04-06 23:26:30]

kbr, reveman: Please review.

https://codereview.chromium.org/1870483003/diff/20001/gpu/GLES2/extensions/CH...
File gpu/GLES2/extensions/CHROMIUM/CHROMIUM_gpu_memory_buffer_image.txt (right):

https://codereview.chromium.org/1870483003/diff/20001/gpu/GLES2/extensions/CH...
gpu/GLES2/extensions/CHROMIUM/CHROMIUM_gpu_memory_buffer_image.txt:58: perform
paramter validation as if the internal format were RGB.
Assuming that everyone is happy with the general idea of this approach, I think
there are cleaner ways of supporting this mechanism in the CHROMIUM image API. 

1. Add an additional parameter to CreateGpuMemoryBufferImageCHROMIUM (bool
RGBEmulation).
2. Add an additional parameter to BindTexImage2DCHROMIUM (bool RGBEmulation).
3. Add a new function MarkImageAsRGBEmulationCHROMIUM(GLuint imageID)
4. [Keep mechanism here].

I'm not sure which one reveman@ will prefer, so I've held off on adding the
plumbing for a new API call. Maybe (3), based on
https://bugs.chromium.org/p/chromium/issues/detail?id=598050#c2, and the desire
to keep this set of APIs as close as possible to EGL image extension APIs?

[Ken Russell (switch to Gerrit), 2016-04-07 00:00:50]

On 2016/04/06 23:26:30, erikchen wrote:
> kbr, reveman: Please review.
> 
>
https://codereview.chromium.org/1870483003/diff/20001/gpu/GLES2/extensions/CH...
> File gpu/GLES2/extensions/CHROMIUM/CHROMIUM_gpu_memory_buffer_image.txt
(right):
> 
>
https://codereview.chromium.org/1870483003/diff/20001/gpu/GLES2/extensions/CH...
> gpu/GLES2/extensions/CHROMIUM/CHROMIUM_gpu_memory_buffer_image.txt:58: perform
> paramter validation as if the internal format were RGB.
> Assuming that everyone is happy with the general idea of this approach, I
think
> there are cleaner ways of supporting this mechanism in the CHROMIUM image API.

> 
> 1. Add an additional parameter to CreateGpuMemoryBufferImageCHROMIUM (bool
> RGBEmulation).
> 2. Add an additional parameter to BindTexImage2DCHROMIUM (bool RGBEmulation).
> 3. Add a new function MarkImageAsRGBEmulationCHROMIUM(GLuint imageID)
> 4. [Keep mechanism here].
> 
> I'm not sure which one reveman@ will prefer, so I've held off on adding the
> plumbing for a new API call. Maybe (3), based on
> https://bugs.chromium.org/p/chromium/issues/detail?id=598050#c2, and the
desire
> to keep this set of APIs as close as possible to EGL image extension APIs?

Is BindTexImage2DCHROMIUM documented? It doesn't appear in
src/gpu/GLES2/extensions/CHROMIUM/ .

(1) sounds good to me personally, since it's an essential property of the image,
and shouldn't change when it's bound to different GL textures. If adding the
parameter is undesirable then (3) sounds good.

[reveman, 2016-04-07 13:00:56]

https://codereview.chromium.org/1870483003/diff/20001/gpu/GLES2/extensions/CH...
File gpu/GLES2/extensions/CHROMIUM/CHROMIUM_gpu_memory_buffer_image.txt (right):

https://codereview.chromium.org/1870483003/diff/20001/gpu/GLES2/extensions/CH...
gpu/GLES2/extensions/CHROMIUM/CHROMIUM_gpu_memory_buffer_image.txt:58: perform
paramter validation as if the internal format were RGB.
On 2016/04/06 at 23:26:30, erikchen wrote:
> Assuming that everyone is happy with the general idea of this approach, I
think there are cleaner ways of supporting this mechanism in the CHROMIUM image
API. 
> 
> 1. Add an additional parameter to CreateGpuMemoryBufferImageCHROMIUM (bool
RGBEmulation).
> 2. Add an additional parameter to BindTexImage2DCHROMIUM (bool RGBEmulation).
> 3. Add a new function MarkImageAsRGBEmulationCHROMIUM(GLuint imageID)
> 4. [Keep mechanism here].
> 
> I'm not sure which one reveman@ will prefer, so I've held off on adding the
plumbing for a new API call. Maybe (3), based on
https://bugs.chromium.org/p/chromium/issues/detail?id=598050#c2, and the desire
to keep this set of APIs as close as possible to EGL image extension APIs?

I don't think we should have workarounds as part of extension specs. It should
be limited to the implementation. Why do we need to mark the image as using RGB
emulation? Can that not just be hardcoded as emulated on MacOS if format is
equal to RGB?

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

https://codereview.chromium.org/1870483003/diff/20001/ui/gl/gl_image.h#newcode82
ui/gl/gl_image.h:82: virtual bool EmulatingRGB() const;
Maybe I'm just being defensive of the code I've designed, written and maintains
but I'd really like to see this be restricted to the command buffer
client/service somehow. This lower level ui/gl code should not be aware of
concepts such as command buffer client and command buffer service.

Can we instead just hard code this in the command buffer service as all glimages
with RGB format on MacOS being emulated and not touch the GLImage interface?

[Ken Russell (switch to Gerrit), 2016-04-07 21:41:15]

I think we should proceed with the current direction of Erik's CL. Please see
below.

https://codereview.chromium.org/1870483003/diff/20001/gpu/GLES2/extensions/CH...
File gpu/GLES2/extensions/CHROMIUM/CHROMIUM_gpu_memory_buffer_image.txt (right):

https://codereview.chromium.org/1870483003/diff/20001/gpu/GLES2/extensions/CH...
gpu/GLES2/extensions/CHROMIUM/CHROMIUM_gpu_memory_buffer_image.txt:58: perform
paramter validation as if the internal format were RGB.
On 2016/04/07 13:00:56, reveman wrote:
> On 2016/04/06 at 23:26:30, erikchen wrote:
> > Assuming that everyone is happy with the general idea of this approach, I
> think there are cleaner ways of supporting this mechanism in the CHROMIUM
image
> API. 
> > 
> > 1. Add an additional parameter to CreateGpuMemoryBufferImageCHROMIUM (bool
> RGBEmulation).
> > 2. Add an additional parameter to BindTexImage2DCHROMIUM (bool
RGBEmulation).
> > 3. Add a new function MarkImageAsRGBEmulationCHROMIUM(GLuint imageID)
> > 4. [Keep mechanism here].
> > 
> > I'm not sure which one reveman@ will prefer, so I've held off on adding the
> plumbing for a new API call. Maybe (3), based on
> https://bugs.chromium.org/p/chromium/issues/detail?id=598050#c2, and the
desire
> to keep this set of APIs as close as possible to EGL image extension APIs?
> 
> I don't think we should have workarounds as part of extension specs. It should
> be limited to the implementation. Why do we need to mark the image as using
RGB
> emulation? Can that not just be hardcoded as emulated on MacOS if format is
> equal to RGB? 

It seems reasonable to mention errata in the extension spec. Unfortunately, as
has already been discussed at length on this bug and other CLs, it is
impractical to completely hide the knowledge from users of this extension that
GL_RGB type GLImages are emulated with the GL_RGBA type. Therefore this should
be documented.

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

https://codereview.chromium.org/1870483003/diff/20001/ui/gl/gl_image.h#newcode82
ui/gl/gl_image.h:82: virtual bool EmulatingRGB() const;
On 2016/04/07 13:00:56, reveman wrote:
> Maybe I'm just being defensive of the code I've designed, written and
maintains
> but I'd really like to see this be restricted to the command buffer
> client/service somehow. This lower level ui/gl code should not be aware of
> concepts such as command buffer client and command buffer service.
> 
> Can we instead just hard code this in the command buffer service as all
glimages
> with RGB format on MacOS being emulated and not touch the GLImage interface?

The comments don't need to reference the command buffer, but one way or another,
it seems that GLImage is going to have to know that it was allocated with type
GL_RGB and actually allocated a GL_RGBA type image internally. It seems
inappropriate to keep this knowledge entirely outside the GLImage class, when
the contract explicitly states that GL_RGB type images are supported, but this
is infeasible on the Mac platform.

I don't have a strong opinion on how this is implemented but it seems
appropriate to me that one of the following be done:

1) there be some kind of query on GLImage either for the requested and actually
allocated formats
2) there be some knowledge that the GLImage was allocated with GL_RGB format,
but GL_RGBA was actually allocated

David, I hope you'll not push back on recording this information in the GLImage
instance, since it looks like it would otherwise mean maintaining an entirely
separate hash table. Looking again at Erik's changes to texture_manager.cc, it
seems that the best (most localized, etc.) place to record this information is
on the GLImage instance itself.

[reveman, 2016-04-08 15:59:39]

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

https://codereview.chromium.org/1870483003/diff/20001/ui/gl/gl_image.h#newcode82
ui/gl/gl_image.h:82: virtual bool EmulatingRGB() const;
On 2016/04/07 at 21:41:14, Ken Russell wrote:
> On 2016/04/07 13:00:56, reveman wrote:
> > Maybe I'm just being defensive of the code I've designed, written and
maintains
> > but I'd really like to see this be restricted to the command buffer
> > client/service somehow. This lower level ui/gl code should not be aware of
> > concepts such as command buffer client and command buffer service.
> > 
> > Can we instead just hard code this in the command buffer service as all
glimages
> > with RGB format on MacOS being emulated and not touch the GLImage interface?
> 
> The comments don't need to reference the command buffer, but one way or
another, it seems that GLImage is going to have to know that it was allocated
with type GL_RGB and actually allocated a GL_RGBA type image internally. It
seems inappropriate to keep this knowledge entirely outside the GLImage class,
when the contract explicitly states that GL_RGB type images are supported, but
this is infeasible on the Mac platform.
> 
> I don't have a strong opinion on how this is implemented but it seems
appropriate to me that one of the following be done:
> 
> 1) there be some kind of query on GLImage either for the requested and
actually allocated formats
> 2) there be some knowledge that the GLImage was allocated with GL_RGB format,
but GL_RGBA was actually allocated
> 
> David, I hope you'll not push back on recording this information in the
GLImage instance, since it looks like it would otherwise mean maintaining an
entirely separate hash table. Looking again at Erik's changes to
texture_manager.cc, it seems that the best (most localized, etc.) place to
record this information is on the GLImage instance itself.

Why do we need to record it at all? Isn't EmulatingRGB going to be true for all
GLImage instances on Mac where internalformat is RGB?

[erikchen, 2016-04-08 16:40:19]

On 2016/04/08 15:59:39, reveman wrote:
> https://codereview.chromium.org/1870483003/diff/20001/ui/gl/gl_image.h
> File ui/gl/gl_image.h (right):
> 
>
https://codereview.chromium.org/1870483003/diff/20001/ui/gl/gl_image.h#newcode82
> ui/gl/gl_image.h:82: virtual bool EmulatingRGB() const;
> On 2016/04/07 at 21:41:14, Ken Russell wrote:
> > On 2016/04/07 13:00:56, reveman wrote:
> > > Maybe I'm just being defensive of the code I've designed, written and
> maintains
> > > but I'd really like to see this be restricted to the command buffer
> > > client/service somehow. This lower level ui/gl code should not be aware of
> > > concepts such as command buffer client and command buffer service.
> > > 
> > > Can we instead just hard code this in the command buffer service as all
> glimages
> > > with RGB format on MacOS being emulated and not touch the GLImage
interface?
> > 
> > The comments don't need to reference the command buffer, but one way or
> another, it seems that GLImage is going to have to know that it was allocated
> with type GL_RGB and actually allocated a GL_RGBA type image internally. It
> seems inappropriate to keep this knowledge entirely outside the GLImage class,
> when the contract explicitly states that GL_RGB type images are supported, but
> this is infeasible on the Mac platform.
> > 
> > I don't have a strong opinion on how this is implemented but it seems
> appropriate to me that one of the following be done:
> > 
> > 1) there be some kind of query on GLImage either for the requested and
> actually allocated formats
> > 2) there be some knowledge that the GLImage was allocated with GL_RGB
format,
> but GL_RGBA was actually allocated
> > 
> > David, I hope you'll not push back on recording this information in the
> GLImage instance, since it looks like it would otherwise mean maintaining an
> entirely separate hash table. Looking again at Erik's changes to
> texture_manager.cc, it seems that the best (most localized, etc.) place to
> record this information is on the GLImage instance itself.
> 
> Why do we need to record it at all? Isn't EmulatingRGB going to be true for
all
> GLImage instances on Mac where internalformat is RGB?

The command buffer service needs to be able to distinguish between RGBA textures
and emulated RGB textures, both of which appear to the command buffer service
with RGBA internal format. This requires an extra bit of information to be
stored *somewhere*.

[erikchen, 2016-04-11 16:28:49]

On 2016/04/08 16:40:19, erikchen wrote:
> On 2016/04/08 15:59:39, reveman wrote:
> > https://codereview.chromium.org/1870483003/diff/20001/ui/gl/gl_image.h
> > File ui/gl/gl_image.h (right):
> > 
> >
>
https://codereview.chromium.org/1870483003/diff/20001/ui/gl/gl_image.h#newcode82
> > ui/gl/gl_image.h:82: virtual bool EmulatingRGB() const;
> > On 2016/04/07 at 21:41:14, Ken Russell wrote:
> > > On 2016/04/07 13:00:56, reveman wrote:
> > > > Maybe I'm just being defensive of the code I've designed, written and
> > maintains
> > > > but I'd really like to see this be restricted to the command buffer
> > > > client/service somehow. This lower level ui/gl code should not be aware
of
> > > > concepts such as command buffer client and command buffer service.
> > > > 
> > > > Can we instead just hard code this in the command buffer service as all
> > glimages
> > > > with RGB format on MacOS being emulated and not touch the GLImage
> interface?
> > > 
> > > The comments don't need to reference the command buffer, but one way or
> > another, it seems that GLImage is going to have to know that it was
allocated
> > with type GL_RGB and actually allocated a GL_RGBA type image internally. It
> > seems inappropriate to keep this knowledge entirely outside the GLImage
class,
> > when the contract explicitly states that GL_RGB type images are supported,
but
> > this is infeasible on the Mac platform.
> > > 
> > > I don't have a strong opinion on how this is implemented but it seems
> > appropriate to me that one of the following be done:
> > > 
> > > 1) there be some kind of query on GLImage either for the requested and
> > actually allocated formats
> > > 2) there be some knowledge that the GLImage was allocated with GL_RGB
> format,
> > but GL_RGBA was actually allocated
> > > 
> > > David, I hope you'll not push back on recording this information in the
> > GLImage instance, since it looks like it would otherwise mean maintaining an
> > entirely separate hash table. Looking again at Erik's changes to
> > texture_manager.cc, it seems that the best (most localized, etc.) place to
> > record this information is on the GLImage instance itself.
> > 
> > Why do we need to record it at all? Isn't EmulatingRGB going to be true for
> all
> > GLImage instances on Mac where internalformat is RGB?
> 
> The command buffer service needs to be able to distinguish between RGBA
textures
> and emulated RGB textures, both of which appear to the command buffer service
> with RGBA internal format. This requires an extra bit of information to be
> stored *somewhere*.

reveman: Ping?

[erikchen, 2016-04-12 23:54:51]

The CQ bit was checked by erikchen@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-04-12 23:55:12]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1870483003/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1870483003/40001

[erikchen, 2016-04-13 00:48:41]

Description was changed from

==========
Add command buffer support for GL_RGB CHROMIUM image emulation.

The CHROMIUM image is always created with internal format GL_RGBA. The caller of
CreateGpuMemoryBufferImageCHROMIUM is responsible for most emulation. The only
support that the command buffer provides is that calls to copyTexImage2D and
copyTexSubImage2D will perform parameter validation as if the internal format
were RGB.

BUG=
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel;tryserver.chromium.win:win_optional_gpu_tests_rel
==========

to

==========
Add command buffer support for GL_RGB CHROMIUM image emulation.

The CHROMIUM image is always created with internal format GL_RGBA. The caller of
CreateGpuMemoryBufferImageCHROMIUM is responsible for most emulation. The only
support that the command buffer provides is that calls to copyTexImage2D and
copyTexSubImage2D will perform parameter validation as if the internal format
were RGB.

BUG=595948
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel;tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[commit-bot: I haz the power, 2016-04-13 01:21:30]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-04-13 01:21:31]

Dry run: This issue passed the CQ dry run.

[erikchen, 2016-04-13 01:29:55]

reveman: PTAL

[reveman, 2016-04-13 22:13:13]

sorry for the delay

ui/gl/gl_image* lgtm

[erikchen, 2016-04-13 22:30:36]

kbr: PTAL

Note that reveman@ prefers to not change the CHROMIUM image API.

[Ken Russell (switch to Gerrit), 2016-04-13 23:21:06]

LGTM too. Thanks for pushing this forward.

[erikchen, 2016-04-13 23:22:22]

erikchen@chromium.org changed reviewers:
+ ccameron@chromium.org, piman@chromium.org

[erikchen, 2016-04-13 23:22:23]

piman: Please review gpu/
ccameron: Please review ui/gfx/mac/io_surface.cc

[erikchen, 2016-04-13 23:23:15]

erikchen@chromium.org changed reviewers:
+ tsepez@chromium.org

[erikchen, 2016-04-13 23:23:16]

tsepez: Please review  gpu/ipc/common/gpu_memory_buffer_support.cc.

[Tom Sepez, 2016-04-13 23:31:44]

lgtm

[piman, 2016-04-14 01:05:43]

Could you add a unit test for the GLES2 code for when an image returns true to
EmulatingRGB?

https://codereview.chromium.org/1870483003/diff/40001/gpu/GLES2/extensions/CH...
File gpu/GLES2/extensions/CHROMIUM/CHROMIUM_gpu_memory_buffer_image.txt (right):

https://codereview.chromium.org/1870483003/diff/40001/gpu/GLES2/extensions/CH...
gpu/GLES2/extensions/CHROMIUM/CHROMIUM_gpu_memory_buffer_image.txt:59: the
internal format were RGB.
How is the client supposed to know? Does this mean we'll have #ifdefs OS_MACOSX
to deal with this? Can we instead at least add a cap bit (in gpu::Capabilities)
/ extension string to explain when the workaround is in place?

https://codereview.chromium.org/1870483003/diff/40001/gpu/GLES2/extensions/CH...
gpu/GLES2/extensions/CHROMIUM/CHROMIUM_gpu_memory_buffer_image.txt:59: the
internal format were RGB.
Actually, it looks like ReadPixels is affected too.

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

https://codereview.chromium.org/1870483003/diff/40001/gpu/command_buffer/serv...
gpu/command_buffer/service/texture_manager.cc:1627: if (level_info.image &&
level_info.image->EmulatingRGB())
EmulatingRGB is constant for the lifetime of the image. So could we evaluate
this in SetLevelImageInternal?

[erikchen, 2016-04-14 01:20:10]

https://codereview.chromium.org/1870483003/diff/40001/gpu/GLES2/extensions/CH...
File gpu/GLES2/extensions/CHROMIUM/CHROMIUM_gpu_memory_buffer_image.txt (right):

https://codereview.chromium.org/1870483003/diff/40001/gpu/GLES2/extensions/CH...
gpu/GLES2/extensions/CHROMIUM/CHROMIUM_gpu_memory_buffer_image.txt:59: the
internal format were RGB.
On 2016/04/14 01:05:43, piman wrote:
> Actually, it looks like ReadPixels is affected too.

For reference, this is the companion CL that implements the client side
workaround:
https://codereview.chromium.org/1856933002/

There's already an #ifdef OS_MACOSX, because the target needs to be RECTANGLE
instead of 2D. If you want, we could expose 2 capabilities, one for RGB
workaround, and one for RECTANGLE instead of 2D, and then we could remove the
#ifdef from DrawingBuffer.cpp

[piman, 2016-04-14 02:54:36]

On 2016/04/14 01:20:10, erikchen wrote:
>
https://codereview.chromium.org/1870483003/diff/40001/gpu/GLES2/extensions/CH...
> File gpu/GLES2/extensions/CHROMIUM/CHROMIUM_gpu_memory_buffer_image.txt
(right):
> 
>
https://codereview.chromium.org/1870483003/diff/40001/gpu/GLES2/extensions/CH...
> gpu/GLES2/extensions/CHROMIUM/CHROMIUM_gpu_memory_buffer_image.txt:59: the
> internal format were RGB.
> On 2016/04/14 01:05:43, piman wrote:
> > Actually, it looks like ReadPixels is affected too.
> 
> For reference, this is the companion CL that implements the client side
> workaround:
> https://codereview.chromium.org/1856933002/
> 
> There's already an #ifdef OS_MACOSX, because the target needs to be RECTANGLE
> instead of 2D. If you want, we could expose 2 capabilities, one for RGB
> workaround, and one for RECTANGLE instead of 2D, and then we could remove the
> #ifdef from DrawingBuffer.cpp

It would be nice to remove the ifdef - if done as a cap it also means we can do
this as a gpu workaround, iow possibly only on some subset of drivers/OSX
versions.
For the texture target, I think we have a way to query it already. reveman@
would know? Otherwise, a cap bit is possible

[erikchen, 2016-04-14 21:24:47]

piman: PTAL

I added a unit test. I ended up not adding a capability because there's no good
mechanism for plumbing that information into WebKit.

https://codereview.chromium.org/1870483003/diff/40001/gpu/GLES2/extensions/CH...
File gpu/GLES2/extensions/CHROMIUM/CHROMIUM_gpu_memory_buffer_image.txt (right):

https://codereview.chromium.org/1870483003/diff/40001/gpu/GLES2/extensions/CH...
gpu/GLES2/extensions/CHROMIUM/CHROMIUM_gpu_memory_buffer_image.txt:59: the
internal format were RGB.
On 2016/04/14 01:20:10, erikchen wrote:
> On 2016/04/14 01:05:43, piman wrote:
> > Actually, it looks like ReadPixels is affected too.
> 
> For reference, this is the companion CL that implements the client side
> workaround:
> https://codereview.chromium.org/1856933002/
> 
> There's already an #ifdef OS_MACOSX, because the target needs to be RECTANGLE
> instead of 2D. If you want, we could expose 2 capabilities, one for RGB
> workaround, and one for RECTANGLE instead of 2D, and then we could remove the
> #ifdef from DrawingBuffer.cpp

I started adding a capability, but I couldn't find a good mechanism for passing
capabilities through to WebKit. Right now, most flags that alter behavior get
passed in during WebKit initialization in the form of WebRuntimeFeatures. I
guess we could add a new public API, or somehow expose the capability through
OpenGL interface, but neither of those sounds very appealing, given that all
we're really doing is plumbing an #ifdef from gpu/command_buffer/service to
WebKit.

Or maybe I missed an existing mechanism for passing this information?

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

https://codereview.chromium.org/1870483003/diff/40001/gpu/command_buffer/serv...
gpu/command_buffer/service/texture_manager.cc:1627: if (level_info.image &&
level_info.image->EmulatingRGB())
On 2016/04/14 01:05:43, piman wrote:
> EmulatingRGB is constant for the lifetime of the image. So could we evaluate
> this in SetLevelImageInternal?

Done.

[erikchen, 2016-04-14 21:25:04]

The CQ bit was checked by erikchen@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-04-14 21:25:41]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1870483003/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1870483003/60001

[piman, 2016-04-14 21:51:36]

piman@chromium.org changed reviewers:
+ danakj@chromium.org

[piman, 2016-04-14 21:51:37]

For the plumbing: +danakj who changed a bunch of this recently, but if I'm not
mistaken, what you want is:
- add a gpu::Capabilities GetCapabilities() to WebGraphicsContext3DProvider
- implement in WebGraphicsContext3DProviderImpl to return
provider_->ContextCapabilities().gpu

That seems fairly straightforward.

https://codereview.chromium.org/1870483003/diff/60001/gpu/command_buffer/serv...
File gpu/command_buffer/service/gles2_cmd_decoder_unittest_textures.cc (right):

https://codereview.chromium.org/1870483003/diff/60001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder_unittest_textures.cc:3743:
cmd.Init(target, level, GL_RGBA, 0, 0, width, height);
Can you add a check that GL_RGB does work?

[commit-bot: I haz the power, 2016-04-14 21:53:55]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-04-14 21:53:56]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[danakj, 2016-04-14 21:54:50]

On Thu, Apr 14, 2016 at 2:51 PM, <piman@chromium.org> wrote:

> For the plumbing: +danakj who changed a bunch of this recently, but if I'm
> not
> mistaken, what you want is:
> - add a gpu::Capabilities GetCapabilities() to WebGraphicsContext3DProvider
>

Blink style name, getCapabilities()


> - implement in WebGraphicsContext3DProviderImpl to return
> provider_->ContextCapabilities().gpu
>

yep! Thanks


>
> That seems fairly straightforward.
>


>
>
>
>
https://codereview.chromium.org/1870483003/diff/60001/gpu/command_buffer/serv...
> File gpu/command_buffer/service/gles2_cmd_decoder_unittest_textures.cc
> (right):
>
>
>
https://codereview.chromium.org/1870483003/diff/60001/gpu/command_buffer/serv...
> gpu/command_buffer/service/gles2_cmd_decoder_unittest_textures.cc:3743:
> cmd.Init(target, level, GL_RGBA, 0, 0, width, height);
> Can you add a check that GL_RGB does work?
>
> https://codereview.chromium.org/1870483003/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[erikchen, 2016-04-15 00:17:06]

piman: PTAL

I also added a new capability and plumbed it through
WebGraphicsContext3DProvider.h, though it is currently unused.

(It will be used by: https://codereview.chromium.org/1856933002/)

https://codereview.chromium.org/1870483003/diff/60001/gpu/command_buffer/serv...
File gpu/command_buffer/service/gles2_cmd_decoder_unittest_textures.cc (right):

https://codereview.chromium.org/1870483003/diff/60001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder_unittest_textures.cc:3743:
cmd.Init(target, level, GL_RGBA, 0, 0, width, height);
On 2016/04/14 21:51:37, piman wrote:
> Can you add a check that GL_RGB does work?

Done.

[piman, 2016-04-15 02:00:27]

thanks, LGTM!

[erikchen, 2016-04-15 02:56:43]

The CQ bit was checked by erikchen@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-04-15 02:56:59]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1870483003/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1870483003/80001

[commit-bot: I haz the power, 2016-04-15 03:27:46]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-04-15 03:27:48]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[erikchen, 2016-04-15 17:29:52]

erikchen@chromium.org changed reviewers:
+ dcheng@chromium.org

[erikchen, 2016-04-15 17:29:53]

dcheng: Please review third_party/WebKit/public/

piman: I updated the texture signature to include the "emulating_rgb_" member.
You're welcome to take another look, but I'm not expecting you to.

[erikchen, 2016-04-15 17:30:12]

The CQ bit was checked by erikchen@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-04-15 17:30:44]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1870483003/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1870483003/100001

[commit-bot: I haz the power, 2016-04-15 18:37:06]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-04-15 18:37:07]

Dry run: Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[erikchen, 2016-04-15 18:54:07]

https://codereview.chromium.org/1870483003/diff/100001/gpu/command_buffer/ser...
File gpu/command_buffer/service/texture_manager.cc (right):

https://codereview.chromium.org/1870483003/diff/100001/gpu/command_buffer/ser...
gpu/command_buffer/service/texture_manager.cc:878: emulating_rgb_ = true;
piman: Request for comment.

This code is never run on non-Mac platforms.

MSVC doesn't like this:
"""
e:\b\build\slave\win\build\src\gpu\command_buffer\service\texture_manager.cc(876)
: error C2220: warning treated as error - no 'object' file generated
e:\b\build\slave\win\build\src\gpu\command_buffer\service\texture_manager.cc(876)
: warning C4702: unreachable code
"""

I can either:
1) Add some #pragmas to disable the warning on Windows
2) #ifdef out the code on Windows.
3) #ifdef out the code on non-OSX.

Normally I'd go with (2) and not even bother with a second request for review,
but you seem to have strong opinions about the presence of #ifdefs. Did you have
any opinions on this subject?

[dcheng, 2016-04-15 19:11:18]

https://codereview.chromium.org/1870483003/diff/100001/third_party/WebKit/pub...
File third_party/WebKit/public/platform/WebGraphicsContext3DProvider.h (right):

https://codereview.chromium.org/1870483003/diff/100001/third_party/WebKit/pub...
third_party/WebKit/public/platform/WebGraphicsContext3DProvider.h:39: struct
Capabilities;
I don't think you can use a forward declaration if something is being returned
by value.

[dcheng, 2016-04-15 19:12:53]

https://codereview.chromium.org/1870483003/diff/100001/third_party/WebKit/pub...
File third_party/WebKit/public/platform/WebGraphicsContext3DProvider.h (right):

https://codereview.chromium.org/1870483003/diff/100001/third_party/WebKit/pub...
third_party/WebKit/public/platform/WebGraphicsContext3DProvider.h:39: struct
Capabilities;
On 2016/04/15 at 19:11:17, dcheng wrote:
> I don't think you can use a forward declaration if something is being returned
by value.

(Rather, this feels a bit like an IWYU violation. It's true that callers will
always have to include this, but it feels weird.)

[dcheng, 2016-04-15 19:14:16]

Actually, I guess this is OK so just ignore everything I just said. LGTM.

[piman, 2016-04-15 19:20:23]

piman@chromium.org changed reviewers:
+ brucedawson@chromium.org

[piman, 2016-04-15 19:20:24]

+brucedawson for MSVC advice

https://codereview.chromium.org/1870483003/diff/100001/gpu/command_buffer/ser...
File gpu/command_buffer/service/texture_manager.cc (right):

https://codereview.chromium.org/1870483003/diff/100001/gpu/command_buffer/ser...
gpu/command_buffer/service/texture_manager.cc:878: emulating_rgb_ = true;
On 2016/04/15 18:54:07, erikchen wrote:
> piman: Request for comment.
> 
> This code is never run on non-Mac platforms.
> 
> MSVC doesn't like this:
> """
>
e:\b\build\slave\win\build\src\gpu\command_buffer\service\texture_manager.cc(876)
> : error C2220: warning treated as error - no 'object' file generated
>
e:\b\build\slave\win\build\src\gpu\command_buffer\service\texture_manager.cc(876)
> : warning C4702: unreachable code
> """
> 
> I can either:
> 1) Add some #pragmas to disable the warning on Windows
> 2) #ifdef out the code on Windows.
> 3) #ifdef out the code on non-OSX.
> 
> Normally I'd go with (2) and not even bother with a second request for review,
> but you seem to have strong opinions about the presence of #ifdefs. Did you
have
> any opinions on this subject?

Why is this not run on non-mac platforms? Is it because the compiler is able to
see that there's no implementation of GLImage that returns true here?

I would prefer disabling the warning, ifdefs seem out of place. It's none of
your business, compiler. Optimize it, fine, but don't warn.

[brucedawson, 2016-04-15 19:49:09]

Suppressing the warning seems fine. There are a few options:

#pragma warning(push)
#pragma warning(disable : 4702)
/* Function goes here */
#pragma warning(pop)

Or you could just disable the warning for the entire file - tightly scoping it
isn't crucial.

You can also try #pragma warning(suppress : 4702) which just suppresses it for
the next line, but that can be a bit finicky.

I assume that the #pragmas will themselves trigger warnings on other platforms
so some #ifdef _MSVC may be necessary anyway. Sigh...

Or you could disable it at the project level in the gyp/gn files.

Whichever method you use be sure to add a comment explaining what 4702 is and
why you are suppressing it (pet peeve).

[piman, 2016-04-15 20:10:50]

On Fri, Apr 15, 2016 at 12:49 PM, <brucedawson@chromium.org> wrote:

> Suppressing the warning seems fine. There are a few options:
>
> #pragma warning(push)
> #pragma warning(disable : 4702)
> /* Function goes here */
> #pragma warning(pop)
>
> Or you could just disable the warning for the entire file - tightly
> scoping it
> isn't crucial.
>
> You can also try #pragma warning(suppress : 4702) which just suppresses it
> for
> the next line, but that can be a bit finicky.
>
> I assume that the #pragmas will themselves trigger warnings on other
> platforms
> so some #ifdef _MSVC may be necessary anyway. Sigh...
>
> Or you could disable it at the project level in the gyp/gn files.
>
> Whichever method you use be sure to add a comment explaining what 4702 is
> and
> why you are suppressing it (pet peeve).
>

Actually, the warning happens at compile time, not link time. How can the
compiler know at that point that there is no derived class that returns
true?
Also, it happens on l.876, not 878. Does the compiler believe face_infos_
is empty? (it's not)
This is highly suspicious.


>
> https://codereview.chromium.org/1870483003/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[erikchen, 2016-04-15 21:14:41]

A pair of missing braces meant that the operator++ implicit in the range based
for loop would never be reached.

Now I'm surprised this wasn't caught on any other platforms.

[ccameron, 2016-04-15 21:37:28]

ui/gfx/mac/io_surface.cc lgtm

[erikchen, 2016-04-15 21:37:42]

The CQ bit was checked by erikchen@chromium.org

[erikchen, 2016-04-15 21:37:42]

The patchset sent to the CQ was uploaded after l-g-t-m from tsepez@chromium.org,
kbr@chromium.org, reveman@chromium.org, piman@chromium.org, dcheng@chromium.org
Link to the patchset: https://codereview.chromium.org/1870483003/#ps120001
(title: "Add missing braces.")

[brucedawson, 2016-04-15 21:37:44]

On 2016/04/15 21:14:41, erikchen wrote:
> A pair of missing braces meant that the operator++ implicit in the range based
> for loop would never be reached.
> 
> Now I'm surprised this wasn't caught on any other platforms.

Whoa. Nice catch. I'm annoyed that I didn't see that. The new gcc/clang (I can't
remember which) -Wmisleading-indentation warning would have caught this - I
guess we don't have it yet?

Also, piman@, you said "It's none of your business, compiler. Optimize it, fine,
but don't warn." - I think you owe VC++ an apology :-)

[commit-bot: I haz the power, 2016-04-15 21:38:24]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1870483003/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1870483003/120001

[piman, 2016-04-15 22:03:14]

On Fri, Apr 15, 2016 at 2:37 PM, <brucedawson@chromium.org> wrote:

> On 2016/04/15 21:14:41, erikchen wrote:
> > A pair of missing braces meant that the operator++ implicit in the range
> based
> > for loop would never be reached.
> >
> > Now I'm surprised this wasn't caught on any other platforms.
>
> Whoa. Nice catch. I'm annoyed that I didn't see that. The new gcc/clang (I
> can't
> remember which) -Wmisleading-indentation warning would have caught this - I
> guess we don't have it yet?
>
> Also, piman@, you said "It's none of your business, compiler. Optimize
> it, fine,
> but don't warn." - I think you owe VC++ an apology :-)
>

I'm sincerely sorry, VC++.

LGTM on the fix and nice catch.



>
>
> https://codereview.chromium.org/1870483003/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[commit-bot: I haz the power, 2016-04-16 01:23:48]

Description was changed from

==========
Add command buffer support for GL_RGB CHROMIUM image emulation.

The CHROMIUM image is always created with internal format GL_RGBA. The caller of
CreateGpuMemoryBufferImageCHROMIUM is responsible for most emulation. The only
support that the command buffer provides is that calls to copyTexImage2D and
copyTexSubImage2D will perform parameter validation as if the internal format
were RGB.

BUG=595948
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel;tryserver.chromium.win:win_optional_gpu_tests_rel
==========

to

==========
Add command buffer support for GL_RGB CHROMIUM image emulation.

The CHROMIUM image is always created with internal format GL_RGBA. The caller of
CreateGpuMemoryBufferImageCHROMIUM is responsible for most emulation. The only
support that the command buffer provides is that calls to copyTexImage2D and
copyTexSubImage2D will perform parameter validation as if the internal format
were RGB.

BUG=595948
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel;tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[commit-bot: I haz the power, 2016-04-16 01:23:58]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-04-16 01:25:02]

Description was changed from

==========
Add command buffer support for GL_RGB CHROMIUM image emulation.

The CHROMIUM image is always created with internal format GL_RGBA. The caller of
CreateGpuMemoryBufferImageCHROMIUM is responsible for most emulation. The only
support that the command buffer provides is that calls to copyTexImage2D and
copyTexSubImage2D will perform parameter validation as if the internal format
were RGB.

BUG=595948
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel;tryserver.chromium.win:win_optional_gpu_tests_rel
==========

to

==========
Add command buffer support for GL_RGB CHROMIUM image emulation.

The CHROMIUM image is always created with internal format GL_RGBA. The caller of
CreateGpuMemoryBufferImageCHROMIUM is responsible for most emulation. The only
support that the command buffer provides is that calls to copyTexImage2D and
copyTexSubImage2D will perform parameter validation as if the internal format
were RGB.

BUG=595948
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel;tryserver.chromium.win:win_optional_gpu_tests_rel

Committed: https://crrev.com/6e4845661eba8e56f36aa32c788f859ebafc8403
Cr-Commit-Position: refs/heads/master@{#387779}
==========

[commit-bot: I haz the power, 2016-04-16 01:25:04]

Patchset 7 (id:??) landed as
https://crrev.com/6e4845661eba8e56f36aa32c788f859ebafc8403
Cr-Commit-Position: refs/heads/master@{#387779}

[reveman, 2016-04-18 22:57:42]

On 2016/04/14 at 02:54:36, piman wrote:
> On 2016/04/14 01:20:10, erikchen wrote:
> >
https://codereview.chromium.org/1870483003/diff/40001/gpu/GLES2/extensions/CH...
> > File gpu/GLES2/extensions/CHROMIUM/CHROMIUM_gpu_memory_buffer_image.txt
(right):
> > 
> >
https://codereview.chromium.org/1870483003/diff/40001/gpu/GLES2/extensions/CH...
> > gpu/GLES2/extensions/CHROMIUM/CHROMIUM_gpu_memory_buffer_image.txt:59: the
> > internal format were RGB.
> > On 2016/04/14 01:05:43, piman wrote:
> > > Actually, it looks like ReadPixels is affected too.
> > 
> > For reference, this is the companion CL that implements the client side
> > workaround:
> > https://codereview.chromium.org/1856933002/
> > 
> > There's already an #ifdef OS_MACOSX, because the target needs to be
RECTANGLE
> > instead of 2D. If you want, we could expose 2 capabilities, one for RGB
> > workaround, and one for RECTANGLE instead of 2D, and then we could remove
the
> > #ifdef from DrawingBuffer.cpp
> 
> It would be nice to remove the ifdef - if done as a cap it also means we can
do this as a gpu workaround, iow possibly only on some subset of drivers/OSX
versions.
> For the texture target, I think we have a way to query it already. reveman@
would know? Otherwise, a cap bit is possible

https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/gp...
can be used to find out what the texture target should be for a native GMB.
That's also shared with each renderer and ends up in cc::ResourceProvider as
ResourceProvider::GetTextureImageTarget. Some plumbing might be needed to get
that to DrawingBuffer.cpp but that is probably the right thing to do.