glBindTexImage2DCHROMIUM should retain pixel format.

glBindTexImage2DCHROMIUM should retain pixel format.

glBindTexImage2DCHROMIUM and glReleaseTexImage2DCHROMIUM will force the
texture format in the level info to GL_RGBA regardless of the internal
format used by the GLImage.

This CL address this by adding a GetFormat function to GLImage that
glBindTexImage2DCHROMIUM and glReleaseTexImage2DCHROMIUM will use when
updating the level info.

BUG=

Committed: https://crrev.com/55ddebb83836f9c4ef8b325050db65ead9807299
Cr-Commit-Position: refs/heads/master@{#330336}

[christiank, 2015-05-12 07:42:37]

christiank@opera.com changed reviewers:
+ jbauman@chromium.org, reveman@chromium.org

[christiank, 2015-05-12 07:42:37]

Hi,

While working on 1119723003 I noticed that glBindTexImage2DCHROMIUM will always
set the texture format to GL_RGBA when updating the level info.

It makes it hard to reflect on the format since it doesn't reflect the actual
texture format.

reveman: Do you want me to tie this to a bug? Maybe the tile compression bug?

[reveman, 2015-05-12 15:27:19]

lgtm with nits

Btw, do you mind updating (in a separate patch) the
gpu/GLES2/extensions/CHROMIUM/CHROMIUM_image.txt spec with the compressed
formats that are now supported?

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

https://codereview.chromium.org/1140683002/diff/1/ui/gl/gl_image.h#newcode30
ui/gl/gl_image.h:30: // Get the image pixel format.
Internal format is not guaranteed to match the exact pixel format. "Get the
internal format of the image."?

https://codereview.chromium.org/1140683002/diff/1/ui/gl/gl_image.h#newcode31
ui/gl/gl_image.h:31: virtual unsigned GetFormat() = 0;
nit: GetInternalFormat() as that's how we refer to this in CHROMIUM_image spec:

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

https://codereview.chromium.org/1140683002/diff/1/ui/gl/gl_image_egl.cc
File ui/gl/gl_image_egl.cc (right):

https://codereview.chromium.org/1140683002/diff/1/ui/gl/gl_image_egl.cc#newco...
ui/gl/gl_image_egl.cc:54: unsigned GLImageEGL::GetFormat() { return 0; }
Please return a valid default value here (say GL_RGBA) or not implement it as
part of GLImageEGL.

https://codereview.chromium.org/1140683002/diff/1/ui/gl/gl_image_linux_dma_bu...
File ui/gl/gl_image_linux_dma_buffer.h (right):

https://codereview.chromium.org/1140683002/diff/1/ui/gl/gl_image_linux_dma_bu...
ui/gl/gl_image_linux_dma_buffer.h:23: // Overridden from GLImageEGL:
nit: "// Overridden from GLImage:" as I think these comments usually refer to
the interface the defined the function

[christiank, 2015-05-13 09:02:23]

Sure, I'll update CHROMIUM_image.txt.

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

https://codereview.chromium.org/1140683002/diff/1/ui/gl/gl_image.h#newcode30
ui/gl/gl_image.h:30: // Get the image pixel format.
On 2015/05/12 15:27:19, reveman wrote:
> Internal format is not guaranteed to match the exact pixel format. "Get the
> internal format of the image."?

Alright, I was debating whether using GetInternalFormat was the right way to go
here. But I was unsure whether the concept existed for GLImage. Should now be
fixed.

[christiank, 2015-05-13 09:02:37]

The CQ bit was checked by christiank@opera.com to run a CQ dry run

[christiank, 2015-05-13 09:02:37]

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/1140683002/#ps20001
(title: "Rename to GetInternalFormat and address nits.")

[commit-bot: I haz the power, 2015-05-13 09:02:41]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1140683002/20001

[commit-bot: I haz the power, 2015-05-13 09:11:52]

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

[commit-bot: I haz the power, 2015-05-13 09:11:53]

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

[christiank, 2015-05-13 09:47:03]

The CQ bit was checked by christiank@opera.com to run a CQ dry run

[christiank, 2015-05-13 09:47:03]

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/1140683002/#ps40001
(title: "Add GetInternalFormat override to MockGLImage.")

[commit-bot: I haz the power, 2015-05-13 09:56:48]

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

[commit-bot: I haz the power, 2015-05-13 11:12:59]

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

[commit-bot: I haz the power, 2015-05-13 11:12:59]

Dry run: This issue passed the CQ dry run.

[christiank, 2015-05-13 11:20:38]

jbauman, would you please have a look?

[jbauman, 2015-05-13 21:00:05]

lgtm

[christiank, 2015-05-18 07:41:31]

The CQ bit was checked by christiank@opera.com

[commit-bot: I haz the power, 2015-05-18 07:41:52]

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

[commit-bot: I haz the power, 2015-05-18 08:56:42]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-05-18 11:34:01]

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