cc: Fix R_8 GpuMemoryBuffers

cc: Fix GL_R8 GpuMemoryBuffers

Make GLImages use GL_RED instead of GL_R8 as the internal format,
because GL_R8 isn't supported as an internal format for most GL
functions and almost all GLES functions.

BUG=533677
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/ce99ede5349c82537e78d1b0cd9c70559550964c
Cr-Commit-Position: refs/heads/master@{#358225}

[ccameron, 2015-11-05 09:49:29]

ccameron@chromium.org changed reviewers:
+ reveman@chromium.org

[ccameron, 2015-11-05 09:49:29]

Couple more instances of this (found on try-jobs for switching to
GMB-by-default).

[reveman, 2015-11-05 13:46:01]

https://codereview.chromium.org/1434453008/diff/1/cc/resources/resource_forma...
File cc/resources/resource_format.cc (right):

https://codereview.chromium.org/1434453008/diff/1/cc/resources/resource_forma...
cc/resources/resource_format.cc:91: return GL_R8_EXT;
Can we have normal textures use GL_R8_EXT as well? I don't think there's any
good reason for why we should have to special case this for GLImage.

[ccameron, 2015-11-05 19:09:20]

https://codereview.chromium.org/1434453008/diff/1/cc/resources/resource_forma...
File cc/resources/resource_format.cc (right):

https://codereview.chromium.org/1434453008/diff/1/cc/resources/resource_forma...
cc/resources/resource_format.cc:91: return GL_R8_EXT;
On 2015/11/05 13:46:01, reveman wrote:
> Can we have normal textures use GL_R8_EXT as well? I don't think there's any
> good reason for why we should have to special case this for GLImage.

That was the first thing that I tried, but GL_R8_EXT doesn't go through the
various validators.

In particular, if we check out the GLES extension text:
https://www.khronos.org/registry/gles/extensions/EXT/EXT_texture_rg.txt
New Tokens
    Accepted by the <internalformat> parameter of TexImage2D and CopyTexImage2D,
    and the <format> parameter of TexImage2D, TexSubImage2D, and ReadPixels:
        RED_EXT                 0x1903
        RG_EXT                  0x8227
    Accepted by the <internalformat> parameter of RenderbufferStorage and
    RenderbufferStorageMultisampleAPPLE:
        R8_EXT                  0x8229
        RG8_EXT                 0x822B
So according to the extension spec, R8_EXT is not a valid argument for <format>
or <internalformat> for TexImage2D.

FWIW, for the related desktop GL extension text:
https://www.opengl.org/registry/specs/ARB/texture_rg.txt
It says that R8_ARB (which coincides with R8_EXT) is accepted as
<internalformat> but not <format>. I don't think that's relevant here because
the client code should be GLES and not desktop GL.

So I think that this is the right thing to do.

[reveman, 2015-11-05 19:17:59]

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

[reveman, 2015-11-05 19:18:01]

+dcastagna who added this GLImage format.

i'd like to keep the internal format usage consistent. ie. if we're going to use
RED_EXT for normal textures then we should also use that for GLImages.

[ccameron, 2015-11-05 19:54:16]

On 2015/11/05 19:18:01, reveman wrote:
> +dcastagna who added this GLImage format.
> 
> i'd like to keep the internal format usage consistent. ie. if we're going to
use
> RED_EXT for normal textures then we should also use that for GLImages.

I think this is a reasonable situation -- for GMBs, we want to be very specific
about the type (R8 is not the same as R16 or R16f). It seems analogous to the
way that R8_EXT is used for renderbuffers.

[reveman, 2015-11-05 20:02:29]

On 2015/11/05 at 19:54:16, ccameron wrote:
> On 2015/11/05 19:18:01, reveman wrote:
> > +dcastagna who added this GLImage format.
> > 
> > i'd like to keep the internal format usage consistent. ie. if we're going to
use
> > RED_EXT for normal textures then we should also use that for GLImages.
> 
> I think this is a reasonable situation -- for GMBs, we want to be very
specific about the type (R8 is not the same as R16 or R16f). It seems analogous
to the way that R8_EXT is used for renderbuffers.

The internalformat for images doesn't have to be anymore specific than for
textures. At the image level, R8 or R16 doesn't matter from the client's pov.
This format only defines what we can expect when sampling or copying from the
image. We already use GL_RGBA for both RGBA_8888 and RGBA_4444.

[ccameron, 2015-11-05 21:26:52]

On 2015/11/05 20:02:29, reveman wrote:
> On 2015/11/05 at 19:54:16, ccameron wrote:
> > On 2015/11/05 19:18:01, reveman wrote:
> > > +dcastagna who added this GLImage format.
> > > 
> > > i'd like to keep the internal format usage consistent. ie. if we're going
to
> use
> > > RED_EXT for normal textures then we should also use that for GLImages.
> > 
> > I think this is a reasonable situation -- for GMBs, we want to be very
> specific about the type (R8 is not the same as R16 or R16f). It seems
analogous
> to the way that R8_EXT is used for renderbuffers.
> 
> The internalformat for images doesn't have to be anymore specific than for
> textures. At the image level, R8 or R16 doesn't matter from the client's pov.
> This format only defines what we can expect when sampling or copying from the
> image. We already use GL_RGBA for both RGBA_8888 and RGBA_4444.

You're right -- I was thinking GMB, but this is for Image. Updated GLImage to
use GL_RED.

[ccameron, 2015-11-05 21:55:18]

On 2015/11/05 21:26:52, ccameron wrote:
> On 2015/11/05 20:02:29, reveman wrote:
> > On 2015/11/05 at 19:54:16, ccameron wrote:
> > > On 2015/11/05 19:18:01, reveman wrote:
> > > > +dcastagna who added this GLImage format.
> > > > 
> > > > i'd like to keep the internal format usage consistent. ie. if we're
going
> to
> > use
> > > > RED_EXT for normal textures then we should also use that for GLImages.
> > > 
> > > I think this is a reasonable situation -- for GMBs, we want to be very
> > specific about the type (R8 is not the same as R16 or R16f). It seems
> analogous
> > to the way that R8_EXT is used for renderbuffers.
> > 
> > The internalformat for images doesn't have to be anymore specific than for
> > textures. At the image level, R8 or R16 doesn't matter from the client's
pov.
> > This format only defines what we can expect when sampling or copying from
the
> > image. We already use GL_RGBA for both RGBA_8888 and RGBA_4444.
> 
> You're right -- I was thinking GMB, but this is for Image. Updated GLImage to
> use GL_RED.

(and, on the topic of red, the red on the bots is from a bizarre unrelated
failure)

[Daniele Castagna, 2015-11-05 22:00:41]

On 2015/11/05 at 21:55:18, ccameron wrote:
> On 2015/11/05 21:26:52, ccameron wrote:
> > On 2015/11/05 20:02:29, reveman wrote:
> > > On 2015/11/05 at 19:54:16, ccameron wrote:
> > > > On 2015/11/05 19:18:01, reveman wrote:
> > > > > +dcastagna who added this GLImage format.
> > > > > 
> > > > > i'd like to keep the internal format usage consistent. ie. if we're
going
> > to
> > > use
> > > > > RED_EXT for normal textures then we should also use that for GLImages.
> > > > 
> > > > I think this is a reasonable situation -- for GMBs, we want to be very
> > > specific about the type (R8 is not the same as R16 or R16f). It seems
> > analogous
> > > to the way that R8_EXT is used for renderbuffers.
> > > 
> > > The internalformat for images doesn't have to be anymore specific than for
> > > textures. At the image level, R8 or R16 doesn't matter from the client's
pov.
> > > This format only defines what we can expect when sampling or copying from
the
> > > image. We already use GL_RGBA for both RGBA_8888 and RGBA_4444.
> > 
> > You're right -- I was thinking GMB, but this is for Image. Updated GLImage
to
> > use GL_RED.
> 
> (and, on the topic of red, the red on the bots is from a bizarre unrelated
failure)

gpu_memory_buffer_video_frame_pool.cc:186 should be also updated.

[reveman, 2015-11-05 22:20:13]

this lgtm after updating the description although I would have preferred the
GetResourceTextureTarget change in a separate patch as that's not really related

[ccameron, 2015-11-05 22:24:23]

Description was changed from

==========
cc: Fix GL_R8 GpuMemoryBuffers

Change the conversion for RED_8 ResourceFormat from GL_RED_EXT to RED_8,
because RED_8 is what the GLImage and GpuMemoryBuffer functions expect.

Change a few instances of hard-coding GL_TEXTURE_2D in
VideoResourceUpdater to query the texture target from the resource, and
add a helper function to do this query this property. This is neccessary
now that the ResourceProvider decides the texture target to use on the
client's behalf.

BUG=533677
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
cc: Fix GL_R8 GpuMemoryBuffers

Make GLImages use GL_RED instead of GL_R8 as the internal format.

Change a few instances of hard-coding GL_TEXTURE_2D in
VideoResourceUpdater to query the texture target from the resource, and
add a helper function to do this query this property. This is neccessary
now that the ResourceProvider decides the texture target to use on the
client's behalf.

BUG=533677
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[ccameron, 2015-11-05 22:30:45]

On 2015/11/05 22:20:13, reveman wrote:
> this lgtm after updating the description although I would have preferred the
> GetResourceTextureTarget change in a separate patch as that's not really
related

Thanks -- yes, they arguably should be separated now that the color conversion
is bigger. I've split GetResourceTextureTarget off into
https://codereview.chromium.org/1406013007

[ccameron, 2015-11-05 22:31:17]

Description was changed from

==========
cc: Fix GL_R8 GpuMemoryBuffers

Make GLImages use GL_RED instead of GL_R8 as the internal format.

Change a few instances of hard-coding GL_TEXTURE_2D in
VideoResourceUpdater to query the texture target from the resource, and
add a helper function to do this query this property. This is neccessary
now that the ResourceProvider decides the texture target to use on the
client's behalf.

BUG=533677
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
cc: Fix GL_R8 GpuMemoryBuffers

Make GLImages use GL_RED instead of GL_R8 as the internal format,
because GL_R8 isn't supported as an internal format for most GL
functions and almost all GLES functions.


BUG=533677
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[ccameron, 2015-11-05 23:56:35]

ccameron@chromium.org changed reviewers:
+ dalecurtis@chromium.org, sievers@chromium.org

[ccameron, 2015-11-05 23:56:35]

Adding sievers@ and dalecurtis@ for stamps.

[DaleCurtis, 2015-11-05 23:59:23]

media/ lgtm

[no sievers, 2015-11-06 01:40:58]

lgtm

[ccameron, 2015-11-06 01:43:45]

Thanks!

[ccameron, 2015-11-06 01:43:52]

The CQ bit was checked by ccameron@chromium.org

[ccameron, 2015-11-06 01:43:53]

The patchset sent to the CQ was uploaded after l-g-t-m from reveman@chromium.org
Link to the patchset: https://codereview.chromium.org/1434453008/#ps40001
(title: "Remove GetResourceTextureTarget")

[commit-bot: I haz the power, 2015-11-06 01:44:32]

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

[commit-bot: I haz the power, 2015-11-06 01:49:54]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-11-06 01:50:51]

Patchset 3 (id:??) landed as
https://crrev.com/ce99ede5349c82537e78d1b0cd9c70559550964c
Cr-Commit-Position: refs/heads/master@{#358225}