gpu: Make glTexSubImage2D work with GL_SRGB_ALPHA on OpenGL

gpu: Make glTexSubImage2D work with GL_SRGB_ALPHA on OpenGL

Make glTexSubImage2D work when client passes GL_SRGB_ALPHA as the
format when running on OpenGL.

Considering GL_SRGB_ALPHA texture:
In OpenGL ES 2.0, Tex*Image2D format must be GL_SRGB_ALPHA (same as texture
internal format).
In OpenGL and OpenGL ES 3.0, format must not be GL_SRGB_ALPHA.

The format was handled correctly for glTexImage2D, but not for
glTexSubImage2D. The actual fix is to apply
TextureManager::AdjustTexFormat to glTexSubImage2D format.

Moves the glTexSubImage2D code from decoder to texture manager, and
tries to follow the structure of glTexImage2D. This is due to hoping to
reduce the amount of similar inconsistency bugs. The glTexSubImage3D,
CopySubTexture* and compressed call variants are still potential
sources of inconsistencies. The code duplication between glTexImage2D
and glTexSubImage2D is not addressed, either.

Changes the texture size of
Service/GLES2DecoderTest.TexSubImage2DBadArgs.  The check for behavior
"!pixels -> kOutOfBounds" was moved before argument validation. In the
test, there is a subtest that uses invalid type, GL_UNSIGNED_INT. This
causes the texture data to be bigger than SHM buffer, the unexpected
kOutOfBounds would be returned instead of normal GL error for invalid
type.

BUG=skia:2992
Committed: https://crrev.com/60da545176aed90d91874a456da2bac8b822c67d
Cr-Commit-Position: refs/heads/master@{#356787}

Committed: https://crrev.com/0c2dee787ace66b1f2d68cca0e19c2d3c82858f4
Cr-Commit-Position: refs/heads/master@{#358578}

[Kimmo Kinnunen, 2015-10-28 14:17:40]

kkinnunen@nvidia.com changed reviewers:
+ bajones@chromium.org, piman@chromium.org

[Kimmo Kinnunen, 2015-10-28 14:17:40]

bajones: would you be able to review GL_EXT_sRGB -related fix?
piman: added for .gyp/.gn files

[piman, 2015-10-28 20:16:16]

LGTM.
I would prefer if the AdjustTexFormat logic was moved to CustomTex[Sub]Image2D
in gl_gl_api_implementation.cc similarly to the rest of the logic to adapt
between ES and desktop GL, and if it used the GLVersionInfo to make the decision
rather than gfx::GetGLImplementation, so that it would be possible to unit test
both paths. But that all predates this patch, so I won't block this.

[bajones, 2015-10-28 20:23:26]

LGTM with a couple of nits

https://codereview.chromium.org/1426903002/diff/1/gpu/command_buffer/service/...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

https://codereview.chromium.org/1426903002/diff/1/gpu/command_buffer/service/...
gpu/command_buffer/service/gles2_cmd_decoder.cc:11161: return
error::kOutOfBounds;
Seems like kInvalidArguments might be a better error here?

https://codereview.chromium.org/1426903002/diff/1/gpu/command_buffer/service/...
File gpu/command_buffer/service/gles2_cmd_decoder_unittest_textures.cc (right):

https://codereview.chromium.org/1426903002/diff/1/gpu/command_buffer/service/...
gpu/command_buffer/service/gles2_cmd_decoder_unittest_textures.cc:223: const int
kHeight = 4;
What necessitated this change?

[Kimmo Kinnunen, 2015-10-29 06:21:18]

I'll not apply any nits and still commit. Feel free to revert if this is not ok,
or assign some further work if modifications are needed.

https://codereview.chromium.org/1426903002/diff/1/gpu/command_buffer/service/...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

https://codereview.chromium.org/1426903002/diff/1/gpu/command_buffer/service/...
gpu/command_buffer/service/gles2_cmd_decoder.cc:11161: return
error::kOutOfBounds;
On 2015/10/28 20:23:26, bajones wrote:
> Seems like kInvalidArguments might be a better error here?

The intention was not to change behavior wrt this. This was returning
kOutOfBounds before, and every similar place in the command buffer OutOfBounds
is returned.

https://codereview.chromium.org/1426903002/diff/1/gpu/command_buffer/service/...
File gpu/command_buffer/service/gles2_cmd_decoder_unittest_textures.cc (right):

https://codereview.chromium.org/1426903002/diff/1/gpu/command_buffer/service/...
gpu/command_buffer/service/gles2_cmd_decoder_unittest_textures.cc:223: const int
kHeight = 4;
On 2015/10/28 20:23:26, bajones wrote:
> What necessitated this change?

Explained already in the commit message. The data length of one of the test
cases exceeds unit test shm size, and kOutOfBounds is returned before an
validation error (16*8*4 components*4 bytes per component in gl_unsigned_int ==
2048).

https://codereview.chromium.org/1426903002/diff/1/gpu/command_buffer/service/...
gpu/command_buffer/service/gles2_cmd_decoder_unittest_textures.cc:269:
GL_UNSIGNED_INT,
This is the case that would fail with oob when the "!pixels" check is moved
before validation.

[Kimmo Kinnunen, 2015-10-29 06:22:38]

The CQ bit was checked by kkinnunen@nvidia.com

[commit-bot: I haz the power, 2015-10-29 06:22:49]

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

[commit-bot: I haz the power, 2015-10-29 06:24:44]

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

[commit-bot: I haz the power, 2015-10-29 06:24:45]

Try jobs failed on following builders:
  ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)

[Kimmo Kinnunen, 2015-10-29 06:57:46]

The CQ bit was checked by kkinnunen@nvidia.com

[Kimmo Kinnunen, 2015-10-29 06:57:47]

The patchset sent to the CQ was uploaded after l-g-t-m from
bajones@chromium.org, piman@chromium.org
Link to the patchset: https://codereview.chromium.org/1426903002/#ps20001
(title: "rebase")

[commit-bot: I haz the power, 2015-10-29 06:58:06]

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

[commit-bot: I haz the power, 2015-10-29 09:27:53]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2015-10-29 09:29:13]

Patchset 2 (id:??) landed as
https://crrev.com/60da545176aed90d91874a456da2bac8b822c67d
Cr-Commit-Position: refs/heads/master@{#356787}

[wychen, 2015-10-29 17:35:53]

A revert of this CL (patchset #2 id:20001) has been created in
https://codereview.chromium.org/1418113009/ by wychen@chromium.org.

The reason for reverting is: Caused Android test failure
(GLEXTSRGBTest.TexImageSRGBALPHAFormat)..

[boliu, 2015-10-29 17:47:39]

On 2015/10/29 17:35:53, wychen wrote:
> A revert of this CL (patchset #2 id:20001) has been created in
> https://codereview.chromium.org/1418113009/ by mailto:wychen@chromium.org.
> 
> The reason for reverting is: Caused Android test failure
> (GLEXTSRGBTest.TexImageSRGBALPHAFormat)..

FWIW this was a commit queue error. linux_android_rel_ng should not be green
here.

Newly added GLEXTSRGBTest.TexImageSRGBALPHAFormat is failling on
linux_android_rel_ng trybot:
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_android_r...

But then the bot tries to run that test *without* the patch, and then of course
that fails because said test doesn't exist! Then bot interpreted that as test
already failing, and allowed the patch through.

Filed crbug.com/549214 for that.

[Kimmo Kinnunen, 2015-10-30 09:52:39]

Description was changed from

==========
gpu: Make glTexSubImage2D work with GL_SRGB_ALPHA on OpenGL

Make glTexSubImage2D work when client passes GL_SRGB_ALPHA as the
format when running on OpenGL.

Considering GL_SRGB_ALPHA texture:
In OpenGL ES, Tex*Image2D format must be GL_SRGB_ALPHA (same as texture
internal format).
In OpenGL, format must not be GL_SRGB_ALPHA.

The format was handled correctly for glTexImage2D, but not for
glTexSubImage2D. The actual fix is to apply
TextureManager::AdjustTexFormat to glTexSubImage2D format.

Moves the glTexSubImage2D code from decoder to texture manager, and
tries to follow the structure of glTexImage2D. This is due to hoping to
reduce the amount of similar inconsistency bugs. The glTexSubImage3D,
CopySubTexture* and compressed call variants are still potential
sources of inconsistencies. The code duplication between glTexImage2D
and glTexSubImage2D is not addressed, either.

Changes the texture size of
Service/GLES2DecoderTest.TexSubImage2DBadArgs.  The check for behavior
"!pixels -> kOutOfBounds" was moved before argument validation. In the
test, there is a subtest that uses invalid type, GL_UNSIGNED_INT. This
causes the texture data to be bigger than SHM buffer, the unexpected
kOutOfBounds would be returned instead of normal GL error for invalid
type.

BUG=skia:2992

Committed: https://crrev.com/60da545176aed90d91874a456da2bac8b822c67d
Cr-Commit-Position: refs/heads/master@{#356787}
==========

to

==========
gpu: Make glTexSubImage2D work with GL_SRGB_ALPHA on OpenGL

Make glTexSubImage2D work when client passes GL_SRGB_ALPHA as the
format when running on OpenGL.

Considering GL_SRGB_ALPHA texture:
In OpenGL ES, Tex*Image2D format must be GL_SRGB_ALPHA (same as texture
internal format).
In OpenGL, format must not be GL_SRGB_ALPHA.

The format was handled correctly for glTexImage2D, but not for
glTexSubImage2D. The actual fix is to apply
TextureManager::AdjustTexFormat to glTexSubImage2D format.

Moves the glTexSubImage2D code from decoder to texture manager, and
tries to follow the structure of glTexImage2D. This is due to hoping to
reduce the amount of similar inconsistency bugs. The glTexSubImage3D,
CopySubTexture* and compressed call variants are still potential
sources of inconsistencies. The code duplication between glTexImage2D
and glTexSubImage2D is not addressed, either.

Changes the texture size of
Service/GLES2DecoderTest.TexSubImage2DBadArgs.  The check for behavior
"!pixels -> kOutOfBounds" was moved before argument validation. In the
test, there is a subtest that uses invalid type, GL_UNSIGNED_INT. This
causes the texture data to be bigger than SHM buffer, the unexpected
kOutOfBounds would be returned instead of normal GL error for invalid
type.

BUG=skia:2992

Committed: https://crrev.com/60da545176aed90d91874a456da2bac8b822c67d
Cr-Commit-Position: refs/heads/master@{#356787}
==========

[Kimmo Kinnunen, 2015-10-30 09:53:03]

The CQ bit was checked by kkinnunen@nvidia.com to run a CQ dry run

[commit-bot: I haz the power, 2015-10-30 09:53:16]

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

[commit-bot: I haz the power, 2015-10-30 10:54:21]

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

[commit-bot: I haz the power, 2015-10-30 10:54:22]

Dry run: This issue passed the CQ dry run.

[Kimmo Kinnunen, 2015-10-30 13:58:30]

Description was changed from

==========
gpu: Make glTexSubImage2D work with GL_SRGB_ALPHA on OpenGL

Make glTexSubImage2D work when client passes GL_SRGB_ALPHA as the
format when running on OpenGL.

Considering GL_SRGB_ALPHA texture:
In OpenGL ES, Tex*Image2D format must be GL_SRGB_ALPHA (same as texture
internal format).
In OpenGL, format must not be GL_SRGB_ALPHA.

The format was handled correctly for glTexImage2D, but not for
glTexSubImage2D. The actual fix is to apply
TextureManager::AdjustTexFormat to glTexSubImage2D format.

Moves the glTexSubImage2D code from decoder to texture manager, and
tries to follow the structure of glTexImage2D. This is due to hoping to
reduce the amount of similar inconsistency bugs. The glTexSubImage3D,
CopySubTexture* and compressed call variants are still potential
sources of inconsistencies. The code duplication between glTexImage2D
and glTexSubImage2D is not addressed, either.

Changes the texture size of
Service/GLES2DecoderTest.TexSubImage2DBadArgs.  The check for behavior
"!pixels -> kOutOfBounds" was moved before argument validation. In the
test, there is a subtest that uses invalid type, GL_UNSIGNED_INT. This
causes the texture data to be bigger than SHM buffer, the unexpected
kOutOfBounds would be returned instead of normal GL error for invalid
type.

BUG=skia:2992

Committed: https://crrev.com/60da545176aed90d91874a456da2bac8b822c67d
Cr-Commit-Position: refs/heads/master@{#356787}
==========

to

==========
gpu: Make glTexSubImage2D work with GL_SRGB_ALPHA on OpenGL

Make glTexSubImage2D work when client passes GL_SRGB_ALPHA as the
format when running on OpenGL.

Considering GL_SRGB_ALPHA texture:
In OpenGL ES 2.0, Tex*Image2D format must be GL_SRGB_ALPHA (same as texture
internal format).
In OpenGL and OpenGL ES 3.0, format must not be GL_SRGB_ALPHA.

The format was handled correctly for glTexImage2D, but not for
glTexSubImage2D. The actual fix is to apply
TextureManager::AdjustTexFormat to glTexSubImage2D format.

Moves the glTexSubImage2D code from decoder to texture manager, and
tries to follow the structure of glTexImage2D. This is due to hoping to
reduce the amount of similar inconsistency bugs. The glTexSubImage3D,
CopySubTexture* and compressed call variants are still potential
sources of inconsistencies. The code duplication between glTexImage2D
and glTexSubImage2D is not addressed, either.

Changes the texture size of
Service/GLES2DecoderTest.TexSubImage2DBadArgs.  The check for behavior
"!pixels -> kOutOfBounds" was moved before argument validation. In the
test, there is a subtest that uses invalid type, GL_UNSIGNED_INT. This
causes the texture data to be bigger than SHM buffer, the unexpected
kOutOfBounds would be returned instead of normal GL error for invalid
type.

BUG=skia:2992

Committed: https://crrev.com/60da545176aed90d91874a456da2bac8b822c67d
Cr-Commit-Position: refs/heads/master@{#356787}
==========

[Kimmo Kinnunen, 2015-11-02 12:21:14]

bajones: so there's a problem with Nexus5, Android 4.4.4. 
I filed it as: http://code.google.com/p/chromium/issues/detail?id=550292

Would it be ok to workaround it in the test?
The other option would be to disable EXT_sRGB for the configuration. I tried if
adjusting the glReadPixels format would workaround the error, but I couldn't
find any. The driver returns RGBA as the preferred format, but fails with it.

There's the problem that the real workaround becomes quite elaborate. ES 2.0
context is sort of ok, since we can disa

https://codereview.chromium.org/1426903002/diff/60001/gpu/command_buffer/test...
File gpu/command_buffer/tests/gl_ext_srgb_unittest.cc (right):

https://codereview.chromium.org/1426903002/diff/60001/gpu/command_buffer/test...
gpu/command_buffer/tests/gl_ext_srgb_unittest.cc:75: glReadPixels(0, 0, 1, 1,
GL_RGBA, GL_UNSIGNED_BYTE, pixel);
Is this workaround ok?
Other option would be to add a real gpu bug workaround to not expose GL_EXT_sRGB
for Adreno on Android version < 5.0.0.

[Kimmo Kinnunen, 2015-11-02 12:22:47]

On 2015/11/02 12:21:14, Kimmo Kinnunen wrote:
> There's the problem that the real workaround becomes quite elaborate. ES 2.0
> context is sort of ok, since we can disa

..ble the extension. For ES 3, one needs to check if the sized SRGB formats that
ES 3.0 supports have the same problem..

[piman, 2015-11-03 04:20:57]

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

[piman, 2015-11-03 04:20:58]

+sievers for opinion. I would prefer if we disabled the extension on that
configuration if it doesn't work fully because of driver bugs.

[piman, 2015-11-03 04:22:00]

(I think it's unlikely we'll expose ES3.0 / WebGL2 on such an old configuration)

[no sievers, 2015-11-03 17:43:12]

Blacklisting the extension sounds good, esp. if it was fixed in 5.0 (I'm
therefore guessing that Adreno 4xx also won't have the bug).

    {
      "id": xxx,
      "description": "...",
      "cr_bugs": [550292],
      "os": {
        "type": "android",
        "version": {
          "op": "<",
          "value": "5.0"
        }
      },
      "disabled_extensions": [
        "GL_EXT_sRGB"
      ]
    },

You can also match a gl_renderer string and/or check the gl(es) version, but it
sounds like that wouldn't be necessary here.

[no sievers, 2015-11-03 17:43:42]

On 2015/11/03 17:43:12, sievers wrote:
> Blacklisting the extension sounds good, esp. if it was fixed in 5.0 (I'm
> therefore guessing that Adreno 4xx also won't have the bug).
> 
>     {
>       "id": xxx,
>       "description": "...",
>       "cr_bugs": [550292],
>       "os": {
>         "type": "android",
>         "version": {
>           "op": "<",
>           "value": "5.0"
>         }
>       },
>       "disabled_extensions": [
>         "GL_EXT_sRGB"
>       ]
>     },
> 
> You can also match a gl_renderer string and/or check the gl(es) version, but
it
> sounds like that wouldn't be necessary here.

You do want to put this though :)

 "gl_vendor": "Qualcomm.*",

[Kimmo Kinnunen, 2015-11-04 08:47:28]

On 2015/11/03 17:43:12, sievers wrote:
> Blacklisting the extension sounds good, esp. if it was fixed in 5.0 (I'm
> therefore guessing that Adreno 4xx also won't have the bug).
> 

Done.

>       "disabled_extensions": [
>         "GL_EXT_sRGB"
>       ]

The disabled_extensions mechanism does not work on tests, and I have not yet had
enough time to fix it. I used workarounds for this.

[piman, 2015-11-04 18:07:23]

LGTM, thanks!

[Kimmo Kinnunen, 2015-11-05 06:26:11]

The CQ bit was checked by kkinnunen@nvidia.com

[Kimmo Kinnunen, 2015-11-05 06:26:12]

The patchset sent to the CQ was uploaded after l-g-t-m from bajones@chromium.org
Link to the patchset: https://codereview.chromium.org/1426903002/#ps100001
(title: "remove an unneeded hunk now that workarounds are used")

[commit-bot: I haz the power, 2015-11-05 06:26:24]

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

[commit-bot: I haz the power, 2015-11-05 07:42:27]

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

[commit-bot: I haz the power, 2015-11-05 07:42:28]

Try jobs failed on following builders:
  win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[Kimmo Kinnunen, 2015-11-06 11:22:08]

The CQ bit was checked by kkinnunen@nvidia.com

[commit-bot: I haz the power, 2015-11-06 11:22:31]

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

[commit-bot: I haz the power, 2015-11-06 12:33:31]

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

[commit-bot: I haz the power, 2015-11-06 12:33:32]

Try jobs failed on following builders:
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Kimmo Kinnunen, 2015-11-09 09:47:16]

The CQ bit was checked by kkinnunen@nvidia.com

[commit-bot: I haz the power, 2015-11-09 09:47:33]

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

[commit-bot: I haz the power, 2015-11-09 11:14:09]

Committed patchset #6 (id:100001)

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

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