gpu: Expose internal format R8 instead of RED.

gpu: Expose internal format R8 instead of RED.

As noted in crrev.com/1703153002 GL_R8 sized internalformat should be preferred to
GL_RED.

This CL lets the users of the command buffer use GL_R8, and it takes care of
converting it to GL_RED when not supported by the underlying GL context.

BUG=
CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel

Committed: https://crrev.com/7b277c596c517ce4a5d0985ace0da1806f2470f4
Cr-Commit-Position: refs/heads/master@{#377222}

[Daniele Castagna, 2016-02-18 16:53:09]

Description was changed from

==========
Use internal format R8 instead of RED.

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

to

==========
Use internal format R8 instead of RED.

BUG=
CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[Daniele Castagna, 2016-02-19 22:30:22]

Description was changed from

==========
Use internal format R8 instead of RED.

BUG=
CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel
==========

to

==========
Use internal format R8 instead of RED.

As noted in crrev.com/1703153002 GL_R8 sized internalformat should be preferred
to
GL_RED.

This CL lets the users of the command buffer use GL_R8, and it takes care of
converting it to GL_RED when not supported by the underlying GL context.


BUG=
CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[Daniele Castagna, 2016-02-20 00:51:25]

Description was changed from

==========
Use internal format R8 instead of RED.

As noted in crrev.com/1703153002 GL_R8 sized internalformat should be preferred
to
GL_RED.

This CL lets the users of the command buffer use GL_R8, and it takes care of
converting it to GL_RED when not supported by the underlying GL context.


BUG=
CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel
==========

to

==========
gpu: Expose internal format R8 instead of RED.

As noted in crrev.com/1703153002 GL_R8 sized internalformat should be preferred
to
GL_RED.

This CL lets the users of the command buffer use GL_R8, and it takes care of
converting it to GL_RED when not supported by the underlying GL context.


BUG=
CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[Daniele Castagna, 2016-02-20 00:51:54]

dcastagna@chromium.org changed reviewers:
+ reveman@chromium.org, zmo@chromium.org

[reveman, 2016-02-20 01:07:55]

lgtm % nits

https://codereview.chromium.org/1708263002/diff/80001/cc/resources/resource_f...
File cc/resources/resource_format.cc (right):

https://codereview.chromium.org/1708263002/diff/80001/cc/resources/resource_f...
cc/resources/resource_format.cc:91: if (format == RED_8)
nit: return (format == RED_8) ? GL_R8_EXT : GLDataFormat(format);

https://codereview.chromium.org/1708263002/diff/80001/ui/gl/gl_image_io_surfa...
File ui/gl/gl_image_io_surface.mm (right):

https://codereview.chromium.org/1708263002/diff/80001/ui/gl/gl_image_io_surfa...
ui/gl/gl_image_io_surface.mm:111: const gfx::GLVersionInfo* version_info =
Can this case be implemented this way?

return gfx::GLContext::GetCurrent()->GetVersionInfo()->IsES3Capable()
    ? GL_R8 : GL_RED;

https://codereview.chromium.org/1708263002/diff/80001/ui/gl/gl_image_memory.cc
File ui/gl/gl_image_memory.cc (right):

https://codereview.chromium.org/1708263002/diff/80001/ui/gl/gl_image_memory.c...
ui/gl/gl_image_memory.cc:102: const gfx::GLVersionInfo* version_info =
ditto

[Daniele Castagna, 2016-02-20 01:16:16]

https://codereview.chromium.org/1708263002/diff/80001/cc/resources/resource_f...
File cc/resources/resource_format.cc (right):

https://codereview.chromium.org/1708263002/diff/80001/cc/resources/resource_f...
cc/resources/resource_format.cc:91: if (format == RED_8)
On 2016/02/20 at 01:07:55, reveman wrote:
> nit: return (format == RED_8) ? GL_R8_EXT : GLDataFormat(format);

Done.

https://codereview.chromium.org/1708263002/diff/80001/ui/gl/gl_image_io_surfa...
File ui/gl/gl_image_io_surface.mm (right):

https://codereview.chromium.org/1708263002/diff/80001/ui/gl/gl_image_io_surfa...
ui/gl/gl_image_io_surface.mm:111: const gfx::GLVersionInfo* version_info =
On 2016/02/20 at 01:07:55, reveman wrote:
> Can this case be implemented this way?
> 
> return gfx::GLContext::GetCurrent()->GetVersionInfo()->IsES3Capable()
>     ? GL_R8 : GL_RED;

Done.

https://codereview.chromium.org/1708263002/diff/80001/ui/gl/gl_image_memory.cc
File ui/gl/gl_image_memory.cc (right):

https://codereview.chromium.org/1708263002/diff/80001/ui/gl/gl_image_memory.c...
ui/gl/gl_image_memory.cc:102: const gfx::GLVersionInfo* version_info =
On 2016/02/20 at 01:07:55, reveman wrote:
> ditto

Done.

[Zhenyao Mo, 2016-02-22 22:53:08]

https://codereview.chromium.org/1708263002/diff/100001/gpu/command_buffer/cli...
File gpu/command_buffer/client/gles2_implementation.cc (right):

https://codereview.chromium.org/1708263002/diff/100001/gpu/command_buffer/cli...
gpu/command_buffer/client/gles2_implementation.cc:5674: case GL_R8:
Please also keep GL_RED

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

https://codereview.chromium.org/1708263002/diff/100001/gpu/command_buffer/ser...
gpu/command_buffer/service/gles2_cmd_decoder.cc:13161: source_internal_format ==
GL_R8 || source_internal_format == GL_ALPHA ||
Please also keep GL_RED

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

https://codereview.chromium.org/1708263002/diff/100001/gpu/command_buffer/ser...
gpu/command_buffer/service/image_factory.cc:22: case GL_R8:
Please also keep GL_RED

https://codereview.chromium.org/1708263002/diff/100001/ui/gl/gl_image_io_surf...
File ui/gl/gl_image_io_surface.mm (right):

https://codereview.chromium.org/1708263002/diff/100001/ui/gl/gl_image_io_surf...
ui/gl/gl_image_io_surface.mm:72: case GL_R8:
Please also keep GL_RED

https://codereview.chromium.org/1708263002/diff/100001/ui/gl/gl_image_memory.cc
File ui/gl/gl_image_memory.cc (right):

https://codereview.chromium.org/1708263002/diff/100001/ui/gl/gl_image_memory....
ui/gl/gl_image_memory.cc:29: case GL_R8:
Please also keep GL_RED

https://codereview.chromium.org/1708263002/diff/100001/ui/gl/gl_image_memory....
ui/gl/gl_image_memory.cc:131: return GL_RED;
Why not let TextureFormat() deal with it here?

[Daniele Castagna, 2016-02-22 22:53:43]

https://codereview.chromium.org/1708263002/diff/120001/gpu/command_buffer/ser...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

https://codereview.chromium.org/1708263002/diff/120001/gpu/command_buffer/ser...
gpu/command_buffer/service/gles2_cmd_decoder.cc:13161: source_internal_format ==
GL_R8 || source_internal_format == GL_RED ||
Also allowed GL_RED as source internal format in copyTextureCHROMIUM as
originally suggested by zmo@.

[Zhenyao Mo, 2016-02-22 22:54:39]

In general it looks good.

I suggest we keep internal function validations flexible, so we don't have to
worry about ES2/ES3 differences.  The ES2/ES3 APIs will have the right
validations, so invalid enums for the platform won't pass through.

[Daniele Castagna, 2016-02-22 23:16:33]

https://codereview.chromium.org/1708263002/diff/100001/gpu/command_buffer/cli...
File gpu/command_buffer/client/gles2_implementation.cc (right):

https://codereview.chromium.org/1708263002/diff/100001/gpu/command_buffer/cli...
gpu/command_buffer/client/gles2_implementation.cc:5674: case GL_R8:
On 2016/02/22 at 22:53:08, Zhenyao Mo wrote:
> Please also keep GL_RED

As discussed offline, it's fine to limit GMB images to only support GL_R8 and
not RED internal format.

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

https://codereview.chromium.org/1708263002/diff/100001/gpu/command_buffer/ser...
gpu/command_buffer/service/gles2_cmd_decoder.cc:13161: source_internal_format ==
GL_R8 || source_internal_format == GL_ALPHA ||
On 2016/02/22 at 22:53:08, Zhenyao Mo wrote:
> Please also keep GL_RED

Done.

https://codereview.chromium.org/1708263002/diff/100001/ui/gl/gl_image_memory.cc
File ui/gl/gl_image_memory.cc (right):

https://codereview.chromium.org/1708263002/diff/100001/ui/gl/gl_image_memory....
ui/gl/gl_image_memory.cc:131: return GL_RED;
On 2016/02/22 at 22:53:08, Zhenyao Mo wrote:
> Why not let TextureFormat() deal with it here?

TextureFormat returns GL_R8 for ES3.
IIUC DataFormat (that is passed to teximage2d as format) can't be R8 but should
be RED in ES3:
https://www.khronos.org/opengles/sdk/docs/man3/html/glTexImage2D.xhtml
I might be wrong and this seems pretty confusing since they allowed sized
formats in ES2 (they had to since format had to be the same as internalformat)
but it seems like in ES3 they restricted the values for format.

[reveman, 2016-02-22 23:20:25]

On 2016/02/22 at 22:54:39, zmo wrote:
> In general it looks good.
> 
> I suggest we keep internal function validations flexible, so we don't have to
worry about ES2/ES3 differences.  The ES2/ES3 APIs will have the right
validations, so invalid enums for the platform won't pass through.

Why do we have to allow two different enums for GL_R8 in chromium specific
extensions that should only be used by chromium code?

CHROMIUM_copy_texture [1] and CHROMIUM_image consistently list GL_R8 as the only
supported format after this change. If we have chromium code that doesn't use
these values then I think we should fix that instead of allowing both.

[1]
https://code.google.com/p/chromium/codesearch#chromium/src/gpu/GLES2/extensio...

[Zhenyao Mo, 2016-02-22 23:48:12]

On 2016/02/22 23:20:25, reveman wrote:
> On 2016/02/22 at 22:54:39, zmo wrote:
> > In general it looks good.
> > 
> > I suggest we keep internal function validations flexible, so we don't have
to
> worry about ES2/ES3 differences.  The ES2/ES3 APIs will have the right
> validations, so invalid enums for the platform won't pass through.
> 
> Why do we have to allow two different enums for GL_R8 in chromium specific
> extensions that should only be used by chromium code?
> 
> CHROMIUM_copy_texture [1] and CHROMIUM_image consistently list GL_R8 as the
only
> supported format after this change. If we have chromium code that doesn't use
> these values then I think we should fix that instead of allowing both.

In theory these extension text should be written against both ES2 and ES3, and
in ES2 GL_RED is supported internalformat, and in ES3, GL_R8 is supported
internalformat.  I feel it's better to be consistent with ES2/ES3 traditions, so
there is no confusion moving forward.

I don't mind the code to be a bit more flexible and allow R8/RED in both
ES2/ES3.

> 
> [1]
>
https://code.google.com/p/chromium/codesearch#chromium/src/gpu/GLES2/extensio...

[Zhenyao Mo, 2016-02-22 23:56:52]

lgtm

I don't want to hold your CL from landing, so if reveman feels strongly about
it, you can land patch set 5.

[reveman, 2016-02-23 00:05:20]

On 2016/02/22 at 23:48:12, zmo wrote:
> On 2016/02/22 23:20:25, reveman wrote:
> > On 2016/02/22 at 22:54:39, zmo wrote:
> > > In general it looks good.
> > > 
> > > I suggest we keep internal function validations flexible, so we don't have
to
> > worry about ES2/ES3 differences.  The ES2/ES3 APIs will have the right
> > validations, so invalid enums for the platform won't pass through.
> > 
> > Why do we have to allow two different enums for GL_R8 in chromium specific
> > extensions that should only be used by chromium code?
> > 
> > CHROMIUM_copy_texture [1] and CHROMIUM_image consistently list GL_R8 as the
only
> > supported format after this change. If we have chromium code that doesn't
use
> > these values then I think we should fix that instead of allowing both.
> 
> In theory these extension text should be written against both ES2 and ES3, and
in ES2 GL_RED is supported internalformat, and in ES3, GL_R8 is supported
internalformat.  I feel it's better to be consistent with ES2/ES3 traditions, so
there is no confusion moving forward.
> 
> I don't mind the code to be a bit more flexible and allow R8/RED in both
ES2/ES3.
> 

hm, does all our chromium specific extensions need to support both ES2 and ES3?
I feel like it would save us a lot of work to just have to write extensions for
one version of GLES and handle the translation on the service side if that's
possible.

[Daniele Castagna, 2016-02-23 21:35:01]

On 2016/02/22 at 23:56:52, zmo wrote:
> lgtm
> 
> I don't want to hold your CL from landing, so if reveman feels strongly about
it, you can land patch set 5.

I'm landing patch set 6 (like 5 but it addresses reveman's first round of
comments.)
zmo@, please let me know at any time if you want to re-address this R8/RED issue
and I'm happy to work on it.

[Daniele Castagna, 2016-02-23 21:35:07]

The CQ bit was checked by dcastagna@chromium.org

[Daniele Castagna, 2016-02-23 21:35:08]

The patchset sent to the CQ was uploaded after l-g-t-m from
reveman@chromium.org, zmo@chromium.org
Link to the patchset: https://codereview.chromium.org/1708263002/#ps140001
(title: "Revert to 'Address reveman's comments'.")

[commit-bot: I haz the power, 2016-02-23 21:42:42]

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

[commit-bot: I haz the power, 2016-02-23 22:00:26]

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

[commit-bot: I haz the power, 2016-02-23 22:00:27]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Daniele Castagna, 2016-02-23 22:01:27]

dcastagna@chromium.org changed reviewers:
+ dalecurtis@chromium.org

[Daniele Castagna, 2016-02-23 22:01:27]

+dalecurtis for ownership on media.

[DaleCurtis, 2016-02-24 04:08:00]

lgtm

[Daniele Castagna, 2016-02-24 04:44:03]

The CQ bit was checked by dcastagna@chromium.org

[commit-bot: I haz the power, 2016-02-24 04:46:08]

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

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

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2016-02-24 04:56:18]

Description was changed from

==========
gpu: Expose internal format R8 instead of RED.

As noted in crrev.com/1703153002 GL_R8 sized internalformat should be preferred
to
GL_RED.

This CL lets the users of the command buffer use GL_R8, and it takes care of
converting it to GL_RED when not supported by the underlying GL context.


BUG=
CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel
==========

to

==========
gpu: Expose internal format R8 instead of RED.

As noted in crrev.com/1703153002 GL_R8 sized internalformat should be preferred
to
GL_RED.

This CL lets the users of the command buffer use GL_R8, and it takes care of
converting it to GL_RED when not supported by the underlying GL context.

BUG=
CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel

Committed: https://crrev.com/7b277c596c517ce4a5d0985ace0da1806f2470f4
Cr-Commit-Position: refs/heads/master@{#377222}
==========

[commit-bot: I haz the power, 2016-02-24 04:56:19]

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

[Daniele Castagna, 2016-02-25 20:22:47]

A revert of this CL (patchset #8 id:140001) has been created in
https://codereview.chromium.org/1741513002/ by dcastagna@chromium.org.

The reason for reverting is: This broke videos on windows: crbug.com/589775.