New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 655247 link

Starred by 1 user

Issue metadata

Status: Assigned
Owner:
Cc:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug

Blocking:
issue 667433



Sign in to add a comment

Add EXT_sRGB_write_control and EXT_texture_sRGB_decode support to the command buffer

Project Member Reported by ccameron@chromium.org, Oct 12 2016

Issue description

These extensions are used by Skia to do color-correct rendering.

GL_EXT_texture_sRGB_decode is a combined GL and GLES spec.
https://www.opengl.org/registry/specs/EXT/texture_sRGB_decode.txt
It is listed as supported on my particular Mac.

EXT_sRGB_write_control is a GLES spec.
https://www.khronos.org/registry/gles/extensions/EXT/EXT_sRGB_write_control.txt
The equivalent GL spec is ARB_framebuffer_sRGB ... but I think it was rolled into the core at some point.
https://www.opengl.org/registry/specs/ARB/framebuffer_sRGB.txt
 
Cc: bsalomon@chromium.org

Comment 2 by zmo@chromium.org, Oct 12 2016

Cc: yunchao...@intel.com

Comment 3 by zmo@chromium.org, Oct 12 2016

Cc: cwallez@chromium.org
FRAMBUFFER_SRGB (from EXT_sRGB_write_control) was added to core in 3.0 (section 4.1.9)
Cc: geoffl...@chromium.org
See also: anglebug.com/1383
Project Member

Comment 7 by bugdroid1@chromium.org, Oct 25 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/angle/angle/+/81c6b5776cd309b36cb39deafa9a6032685fde7d

commit 81c6b5776cd309b36cb39deafa9a6032685fde7d
Author: Geoff Lang <geofflang@chromium.org>
Date: Wed Oct 19 21:07:52 2016

Implement GL_EXT_texture_sRGB_decode for GL.

BUG=angleproject:1383
BUG=655247

Change-Id: I409b12e1ae418530576de5ec9ce26b7be5d91650
Reviewed-on: https://chromium-review.googlesource.com/400807
Reviewed-by: Jamie Madill <jmadill@chromium.org>
Commit-Queue: Geoff Lang <geofflang@chromium.org>

[modify] https://crrev.com/81c6b5776cd309b36cb39deafa9a6032685fde7d/src/libANGLE/Texture.cpp
[modify] https://crrev.com/81c6b5776cd309b36cb39deafa9a6032685fde7d/src/libANGLE/renderer/gl/SamplerGL.cpp
[modify] https://crrev.com/81c6b5776cd309b36cb39deafa9a6032685fde7d/src/libANGLE/Sampler.h
[modify] https://crrev.com/81c6b5776cd309b36cb39deafa9a6032685fde7d/src/tests/gl_tests/SRGBTextureTest.cpp
[modify] https://crrev.com/81c6b5776cd309b36cb39deafa9a6032685fde7d/src/libANGLE/renderer/gl/renderergl_utils.cpp
[modify] https://crrev.com/81c6b5776cd309b36cb39deafa9a6032685fde7d/src/libANGLE/Sampler.cpp
[modify] https://crrev.com/81c6b5776cd309b36cb39deafa9a6032685fde7d/src/libANGLE/queryutils.cpp
[modify] https://crrev.com/81c6b5776cd309b36cb39deafa9a6032685fde7d/src/tests/test_utils/gl_raii.h
[modify] https://crrev.com/81c6b5776cd309b36cb39deafa9a6032685fde7d/src/libANGLE/Caps.h
[modify] https://crrev.com/81c6b5776cd309b36cb39deafa9a6032685fde7d/src/libANGLE/angletypes.h
[modify] https://crrev.com/81c6b5776cd309b36cb39deafa9a6032685fde7d/src/libANGLE/Texture.h
[modify] https://crrev.com/81c6b5776cd309b36cb39deafa9a6032685fde7d/src/libANGLE/angletypes.cpp
[modify] https://crrev.com/81c6b5776cd309b36cb39deafa9a6032685fde7d/src/libANGLE/Caps.cpp
[modify] https://crrev.com/81c6b5776cd309b36cb39deafa9a6032685fde7d/src/libANGLE/renderer/gl/TextureGL.cpp
[modify] https://crrev.com/81c6b5776cd309b36cb39deafa9a6032685fde7d/src/libANGLE/validationES.cpp

Project Member

Comment 8 by bugdroid1@chromium.org, Oct 25 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/657fd99aed465ab7f6d7aa62775217cd571f97c7

commit 657fd99aed465ab7f6d7aa62775217cd571f97c7
Author: geofflang <geofflang@chromium.org>
Date: Tue Oct 25 22:19:26 2016

Roll ANGLE 3b5ffe5..af7f301

https://chromium.googlesource.com/angle/angle.git/+log/3b5ffe5..af7f301

BUG= chromium:639760 ,None,655247

TBR=jmadill@chromium.org

TEST=bots

CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.android:android_optional_gpu_tests_rel

Review-Url: https://codereview.chromium.org/2444383003
Cr-Commit-Position: refs/heads/master@{#427509}

[modify] https://crrev.com/657fd99aed465ab7f6d7aa62775217cd571f97c7/DEPS

Project Member

Comment 9 by bugdroid1@chromium.org, Oct 27 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/angle/angle/+/1d2c41d612abb740b7a7f50dfa8ff6878ca1a144

commit 1d2c41d612abb740b7a7f50dfa8ff6878ca1a144
Author: Geoff Lang <geofflang@chromium.org>
Date: Wed Oct 19 23:14:46 2016

Implement GL_EXT_sRGB_write_control for GL.

BUG=angleproject:1547
BUG=655247

Change-Id: I3f04ddc7032e4a47eb21ff3b8586c5b47415bb64
Reviewed-on: https://chromium-review.googlesource.com/400958
Commit-Queue: Geoff Lang <geofflang@chromium.org>
Reviewed-by: Jamie Madill <jmadill@chromium.org>

[modify] https://crrev.com/1d2c41d612abb740b7a7f50dfa8ff6878ca1a144/src/libANGLE/renderer/gl/StateManagerGL.h
[modify] https://crrev.com/1d2c41d612abb740b7a7f50dfa8ff6878ca1a144/src/libANGLE/renderer/gl/FramebufferGL.h
[modify] https://crrev.com/1d2c41d612abb740b7a7f50dfa8ff6878ca1a144/src/libANGLE/State.cpp
[modify] https://crrev.com/1d2c41d612abb740b7a7f50dfa8ff6878ca1a144/src/tests/deqp_support/deqp_gles2_test_expectations.txt
[modify] https://crrev.com/1d2c41d612abb740b7a7f50dfa8ff6878ca1a144/src/libANGLE/Context.cpp
[modify] https://crrev.com/1d2c41d612abb740b7a7f50dfa8ff6878ca1a144/src/libANGLE/State.h
[modify] https://crrev.com/1d2c41d612abb740b7a7f50dfa8ff6878ca1a144/src/libANGLE/Caps.h
[modify] https://crrev.com/1d2c41d612abb740b7a7f50dfa8ff6878ca1a144/src/libANGLE/renderer/gl/FramebufferGL.cpp
[modify] https://crrev.com/1d2c41d612abb740b7a7f50dfa8ff6878ca1a144/src/libANGLE/renderer/gl/StateManagerGL.cpp
[modify] https://crrev.com/1d2c41d612abb740b7a7f50dfa8ff6878ca1a144/src/libANGLE/renderer/gl/renderergl_utils.cpp
[modify] https://crrev.com/1d2c41d612abb740b7a7f50dfa8ff6878ca1a144/src/libANGLE/Caps.cpp
[modify] https://crrev.com/1d2c41d612abb740b7a7f50dfa8ff6878ca1a144/src/tests/angle_end2end_tests.gypi
[add] https://crrev.com/1d2c41d612abb740b7a7f50dfa8ff6878ca1a144/src/tests/gl_tests/SRGBFramebufferTest.cpp
[modify] https://crrev.com/1d2c41d612abb740b7a7f50dfa8ff6878ca1a144/src/libANGLE/ContextState.cpp
[modify] https://crrev.com/1d2c41d612abb740b7a7f50dfa8ff6878ca1a144/src/libANGLE/validationES.cpp

Project Member

Comment 10 by bugdroid1@chromium.org, Oct 27 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/a16bd358e88a73acb35843f44719783f24087234

commit a16bd358e88a73acb35843f44719783f24087234
Author: geofflang <geofflang@chromium.org>
Date: Thu Oct 27 21:18:24 2016

Roll ANGLE af7f301..1d2c41d

https://chromium.googlesource.com/angle/angle.git/+log/af7f301..1d2c41d

BUG= 658555 , chromium:659326 , 540829 ,655247

TBR=jmadill@chromium.org

TEST=bots

CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.android:android_optional_gpu_tests_rel

Review-Url: https://codereview.chromium.org/2457883002
Cr-Commit-Position: refs/heads/master@{#428138}

[modify] https://crrev.com/a16bd358e88a73acb35843f44719783f24087234/DEPS

Project Member

Comment 11 by bugdroid1@chromium.org, Oct 27 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/9bfaa6948251aae2a4f2b8ca178e8c9c3f5e78c3

commit 9bfaa6948251aae2a4f2b8ca178e8c9c3f5e78c3
Author: jam <jam@chromium.org>
Date: Thu Oct 27 21:49:44 2016

Revert of Roll ANGLE af7f301..1d2c41d (patchset #1 id:1 of https://codereview.chromium.org/2457883002/ )

Reason for revert:
breaks build
https://build.chromium.org/p/chromium.chrome/builders/Google%20Chrome%20Win/builds/11909/steps/compile/logs/stdio

Original issue's description:
> Roll ANGLE af7f301..1d2c41d
>
> https://chromium.googlesource.com/angle/angle.git/+log/af7f301..1d2c41d
>
> BUG= 658555 , chromium:659326 , 540829 ,655247
>
> TBR=jmadill@chromium.org
>
> TEST=bots
>
> CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.android:android_optional_gpu_tests_rel
>
> Committed: https://crrev.com/a16bd358e88a73acb35843f44719783f24087234
> Cr-Commit-Position: refs/heads/master@{#428138}

TBR=jmadill@chromium.org,geofflang@chromium.org
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG= 658555 , chromium:659326 , 540829 ,655247

Review-Url: https://codereview.chromium.org/2459753002
Cr-Commit-Position: refs/heads/master@{#428148}

[modify] https://crrev.com/9bfaa6948251aae2a4f2b8ca178e8c9c3f5e78c3/DEPS

Project Member

Comment 12 by bugdroid1@chromium.org, Oct 30 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/988aa562e76cacc7b6b39d9f7b33c25f5b7824f8

commit 988aa562e76cacc7b6b39d9f7b33c25f5b7824f8
Author: qiankun.miao <qiankun.miao@intel.com>
Date: Sun Oct 30 09:14:46 2016

Roll ANGLE af7f301..705a919

https://chromium.googlesource.com/angle/angle.git/+log/af7f301..705a919

BUG= chromium:659892 ,  540829 , 655247,  chromium:639760 ,  chromium:659326 ,  chromium:659326 ,  658555 

TEST=bots

CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.android:android_optional_gpu_tests_rel

Review-Url: https://codereview.chromium.org/2457243003
Cr-Commit-Position: refs/heads/master@{#428622}

[modify] https://crrev.com/988aa562e76cacc7b6b39d9f7b33c25f5b7824f8/DEPS

Blocking: -44872 667433
Project Member

Comment 14 by bugdroid1@chromium.org, Dec 1 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/a48dcecc89523bf034b2138c308fc959380c8028

commit a48dcecc89523bf034b2138c308fc959380c8028
Author: ccameron <ccameron@chromium.org>
Date: Thu Dec 01 17:10:07 2016

Add command buffer support for EXT_sRGB_write_control

This state is not set in the service side at glEnable/glDisable time.
Instead, it is set during framebuffer validation, to accommodate
driver bug workarounds on desktop.

Move the logic for updating framebuffer SRGB in the decoder to
a separate function, GLES2DecoderImpl::UpdateFramebufferSRGB.

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

Review-Url: https://codereview.chromium.org/2519603002
Cr-Commit-Position: refs/heads/master@{#435631}

[modify] https://crrev.com/a48dcecc89523bf034b2138c308fc959380c8028/gpu/command_buffer/build_gles2_cmd_buffer.py
[modify] https://crrev.com/a48dcecc89523bf034b2138c308fc959380c8028/gpu/command_buffer/service/context_state.cc
[modify] https://crrev.com/a48dcecc89523bf034b2138c308fc959380c8028/gpu/command_buffer/service/context_state.h
[modify] https://crrev.com/a48dcecc89523bf034b2138c308fc959380c8028/gpu/command_buffer/service/context_state_autogen.h
[modify] https://crrev.com/a48dcecc89523bf034b2138c308fc959380c8028/gpu/command_buffer/service/context_state_impl_autogen.h
[modify] https://crrev.com/a48dcecc89523bf034b2138c308fc959380c8028/gpu/command_buffer/service/feature_info.cc
[modify] https://crrev.com/a48dcecc89523bf034b2138c308fc959380c8028/gpu/command_buffer/service/feature_info.h
[modify] https://crrev.com/a48dcecc89523bf034b2138c308fc959380c8028/gpu/command_buffer/service/feature_info_unittest.cc
[modify] https://crrev.com/a48dcecc89523bf034b2138c308fc959380c8028/gpu/command_buffer/service/gles2_cmd_decoder.cc
[modify] https://crrev.com/a48dcecc89523bf034b2138c308fc959380c8028/gpu/command_buffer/service/gles2_cmd_decoder_autogen.h
[modify] https://crrev.com/a48dcecc89523bf034b2138c308fc959380c8028/gpu/command_buffer/service/gles2_cmd_decoder_unittest_0_autogen.h
[modify] https://crrev.com/a48dcecc89523bf034b2138c308fc959380c8028/gpu/command_buffer/service/gles2_cmd_decoder_unittest_base.cc
[modify] https://crrev.com/a48dcecc89523bf034b2138c308fc959380c8028/gpu/command_buffer/tests/gl_test_utils.cc

Project Member

Comment 15 by bugdroid1@chromium.org, Dec 1 2016

Project Member

Comment 16 by bugdroid1@chromium.org, Dec 1 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/e425b98e375135dd91099b29f00fcbc0cd8dc356

commit e425b98e375135dd91099b29f00fcbc0cd8dc356
Author: enne <enne@chromium.org>
Date: Thu Dec 01 23:48:26 2016

Revert of Add command buffer support for GL_EXT_texture_sRGB_decode (patchset #4 id:60001 of https://codereview.chromium.org/2539163003/ )

Reason for revert:
Patch that this depends on breaks gl_tests

BUG= 670487 

Original issue's description:
> Add command buffer support for GL_EXT_texture_sRGB_decode
>
> BUG=655247
> CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
>
> Committed: https://crrev.com/e0e73c7fd74ed796a0b57748fcc7c740d0a51220
> Cr-Commit-Position: refs/heads/master@{#435666}

TBR=zmo@chromium.org,ccameron@chromium.org
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG=655247

Review-Url: https://codereview.chromium.org/2544903003
Cr-Commit-Position: refs/heads/master@{#435770}

[modify] https://crrev.com/e425b98e375135dd91099b29f00fcbc0cd8dc356/gpu/command_buffer/build_gles2_cmd_buffer.py
[modify] https://crrev.com/e425b98e375135dd91099b29f00fcbc0cd8dc356/gpu/command_buffer/common/gles2_cmd_utils_autogen.h
[modify] https://crrev.com/e425b98e375135dd91099b29f00fcbc0cd8dc356/gpu/command_buffer/common/gles2_cmd_utils_implementation_autogen.h
[modify] https://crrev.com/e425b98e375135dd91099b29f00fcbc0cd8dc356/gpu/command_buffer/service/feature_info.cc
[modify] https://crrev.com/e425b98e375135dd91099b29f00fcbc0cd8dc356/gpu/command_buffer/service/gles2_cmd_validation_autogen.h
[modify] https://crrev.com/e425b98e375135dd91099b29f00fcbc0cd8dc356/gpu/command_buffer/service/gles2_cmd_validation_implementation_autogen.h
[modify] https://crrev.com/e425b98e375135dd91099b29f00fcbc0cd8dc356/gpu/command_buffer/service/texture_manager.cc

Project Member

Comment 17 by bugdroid1@chromium.org, Dec 1 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/25a5b0aa58a30bbbffe5f2bb197436044ce80832

commit 25a5b0aa58a30bbbffe5f2bb197436044ce80832
Author: enne <enne@chromium.org>
Date: Thu Dec 01 23:51:53 2016

Revert of Add command buffer support for EXT_sRGB_write_control (patchset #9 id:160001 of https://codereview.chromium.org/2519603002/ )

Reason for revert:
Breaks gl_tests on GPU FYI bots

BUG= 670487 

Original issue's description:
> Add command buffer support for EXT_sRGB_write_control
>
> This state is not set in the service side at glEnable/glDisable time.
> Instead, it is set during framebuffer validation, to accommodate
> driver bug workarounds on desktop.
>
> Move the logic for updating framebuffer SRGB in the decoder to
> a separate function, GLES2DecoderImpl::UpdateFramebufferSRGB.
>
> BUG=655247
> CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
>
> Committed: https://crrev.com/a48dcecc89523bf034b2138c308fc959380c8028
> Cr-Commit-Position: refs/heads/master@{#435631}

TBR=zmo@chromium.org,ccameron@chromium.org
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG=655247

Review-Url: https://codereview.chromium.org/2537343008
Cr-Commit-Position: refs/heads/master@{#435773}

[modify] https://crrev.com/25a5b0aa58a30bbbffe5f2bb197436044ce80832/gpu/command_buffer/build_gles2_cmd_buffer.py
[modify] https://crrev.com/25a5b0aa58a30bbbffe5f2bb197436044ce80832/gpu/command_buffer/service/context_state.cc
[modify] https://crrev.com/25a5b0aa58a30bbbffe5f2bb197436044ce80832/gpu/command_buffer/service/context_state.h
[modify] https://crrev.com/25a5b0aa58a30bbbffe5f2bb197436044ce80832/gpu/command_buffer/service/context_state_autogen.h
[modify] https://crrev.com/25a5b0aa58a30bbbffe5f2bb197436044ce80832/gpu/command_buffer/service/context_state_impl_autogen.h
[modify] https://crrev.com/25a5b0aa58a30bbbffe5f2bb197436044ce80832/gpu/command_buffer/service/feature_info.cc
[modify] https://crrev.com/25a5b0aa58a30bbbffe5f2bb197436044ce80832/gpu/command_buffer/service/feature_info.h
[modify] https://crrev.com/25a5b0aa58a30bbbffe5f2bb197436044ce80832/gpu/command_buffer/service/feature_info_unittest.cc
[modify] https://crrev.com/25a5b0aa58a30bbbffe5f2bb197436044ce80832/gpu/command_buffer/service/gles2_cmd_decoder.cc
[modify] https://crrev.com/25a5b0aa58a30bbbffe5f2bb197436044ce80832/gpu/command_buffer/service/gles2_cmd_decoder_autogen.h
[modify] https://crrev.com/25a5b0aa58a30bbbffe5f2bb197436044ce80832/gpu/command_buffer/service/gles2_cmd_decoder_unittest_0_autogen.h
[modify] https://crrev.com/25a5b0aa58a30bbbffe5f2bb197436044ce80832/gpu/command_buffer/service/gles2_cmd_decoder_unittest_base.cc
[modify] https://crrev.com/25a5b0aa58a30bbbffe5f2bb197436044ce80832/gpu/command_buffer/tests/gl_test_utils.cc

It was fun while it lasted :(.

Trying again!
Project Member

Comment 19 by bugdroid1@chromium.org, Dec 2 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/ddaa56a114ad4db2c6acc7d6c76d539587161165

commit ddaa56a114ad4db2c6acc7d6c76d539587161165
Author: ccameron <ccameron@chromium.org>
Date: Fri Dec 02 04:05:46 2016

Prep work for adding command buffer support for EXT_sRGB_write_control

This re-lands part of the work from crrev.com/435631, namely.

Move the logic for updating framebuffer SRGB in the decoder to
a separate function

TBR=zmo

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

Review-Url: https://codereview.chromium.org/2542343003
Cr-Commit-Position: refs/heads/master@{#435852}

[modify] https://crrev.com/ddaa56a114ad4db2c6acc7d6c76d539587161165/gpu/command_buffer/service/context_state.cc
[modify] https://crrev.com/ddaa56a114ad4db2c6acc7d6c76d539587161165/gpu/command_buffer/service/context_state.h
[modify] https://crrev.com/ddaa56a114ad4db2c6acc7d6c76d539587161165/gpu/command_buffer/service/gles2_cmd_decoder.cc
[modify] https://crrev.com/ddaa56a114ad4db2c6acc7d6c76d539587161165/gpu/command_buffer/service/gles2_cmd_decoder_unittest_base.cc

As a note, I think that when the decode extension landed, it broke the Skia command buffer bot: https://chromium-swarm.appspot.com/task?id=32d5e2cce4556a10&refresh=10

In particular, we're testing that mip-map generation (via glGenerateMipmap) respects the value of GL_TEXTURE_SRGB_DECODE_EXT - mip-maps should be gamma-correct when it's set to DECODE, and naively filtered when it's set to SKIP_DECODE.

I can't claim that this is required by the spec, but nearly all implementations that we've tested on behave this way.
For completeness, the mipmap thing was also caught on the chromium side. Just adding links to both bugs so we don't lose track of the connection:

https://bugs.chromium.org/p/chromium/issues/detail?id=634519
Cc: qiankun....@intel.com
Project Member

Comment 23 by bugdroid1@chromium.org, Dec 6 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/63b3546297d36805f698468ce6f9df2e77f1e6b6

commit 63b3546297d36805f698468ce6f9df2e77f1e6b6
Author: ccameron <ccameron@chromium.org>
Date: Tue Dec 06 09:47:11 2016

Add command buffer support for EXT_sRGB_write_control

This state is not set in the service side at glEnable/glDisable time.
Instead, it is set during framebuffer validation, to accommodate
driver bug workarounds on desktop.

Re-land of patch previously submitted at crrev.com/435631

TBR=zmo
BUG=655247
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel

Review-Url: https://codereview.chromium.org/2550203003
Cr-Commit-Position: refs/heads/master@{#436555}

[modify] https://crrev.com/63b3546297d36805f698468ce6f9df2e77f1e6b6/gpu/command_buffer/build_gles2_cmd_buffer.py
[modify] https://crrev.com/63b3546297d36805f698468ce6f9df2e77f1e6b6/gpu/command_buffer/service/context_state_autogen.h
[modify] https://crrev.com/63b3546297d36805f698468ce6f9df2e77f1e6b6/gpu/command_buffer/service/context_state_impl_autogen.h
[modify] https://crrev.com/63b3546297d36805f698468ce6f9df2e77f1e6b6/gpu/command_buffer/service/feature_info.cc
[modify] https://crrev.com/63b3546297d36805f698468ce6f9df2e77f1e6b6/gpu/command_buffer/service/feature_info.h
[modify] https://crrev.com/63b3546297d36805f698468ce6f9df2e77f1e6b6/gpu/command_buffer/service/feature_info_unittest.cc
[modify] https://crrev.com/63b3546297d36805f698468ce6f9df2e77f1e6b6/gpu/command_buffer/service/gles2_cmd_decoder.cc
[modify] https://crrev.com/63b3546297d36805f698468ce6f9df2e77f1e6b6/gpu/command_buffer/service/gles2_cmd_decoder_autogen.h
[modify] https://crrev.com/63b3546297d36805f698468ce6f9df2e77f1e6b6/gpu/command_buffer/tests/gl_test_utils.cc

Project Member

Comment 24 by bugdroid1@chromium.org, Dec 6 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/76bcf5b2adbf1f6321280e1bad91e443b8b43d2c

commit 76bcf5b2adbf1f6321280e1bad91e443b8b43d2c
Author: ccameron <ccameron@chromium.org>
Date: Tue Dec 06 21:41:48 2016

Add command buffer support for GL_EXT_texture_sRGB_decode

Direct re-land of crrev.com/435666

TBR=zmo
BUG=655247
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel

Review-Url: https://codereview.chromium.org/2560563002
Cr-Commit-Position: refs/heads/master@{#436742}

[modify] https://crrev.com/76bcf5b2adbf1f6321280e1bad91e443b8b43d2c/gpu/command_buffer/build_gles2_cmd_buffer.py
[modify] https://crrev.com/76bcf5b2adbf1f6321280e1bad91e443b8b43d2c/gpu/command_buffer/common/gles2_cmd_utils_autogen.h
[modify] https://crrev.com/76bcf5b2adbf1f6321280e1bad91e443b8b43d2c/gpu/command_buffer/common/gles2_cmd_utils_implementation_autogen.h
[modify] https://crrev.com/76bcf5b2adbf1f6321280e1bad91e443b8b43d2c/gpu/command_buffer/service/feature_info.cc
[modify] https://crrev.com/76bcf5b2adbf1f6321280e1bad91e443b8b43d2c/gpu/command_buffer/service/gles2_cmd_validation_autogen.h
[modify] https://crrev.com/76bcf5b2adbf1f6321280e1bad91e443b8b43d2c/gpu/command_buffer/service/gles2_cmd_validation_implementation_autogen.h
[modify] https://crrev.com/76bcf5b2adbf1f6321280e1bad91e443b8b43d2c/gpu/command_buffer/service/texture_manager.cc

Re-landing of this has again triggered a failure on Skia's command buffer bot: https://chromium-swarm.appspot.com/task?id=32f2eb5e5d2c1b10&refresh=10 (see #20). I can tell Skia to pretend that CB doesn't have GL_EXT_texture_sRGB_decode, but that will force Chromium to set a context option to get sRGB (and it breaks mixed legacy/color-correct rendering). I can't turn on our manual mip-mapping, because that depends on features that don't exist in ES2. Or I can just add another caps bit that says that sRGB decode control actually affects mip-map generation, but the up-shot is that we're losing the guarantee about how mips are built.

For reference, the failing bot is a Mac with an Intel 5100 GPU.
Interesting -- it looks like a GL error was generated. Did this happen on all Mac bots, or just that particular one?
I think the GL errors are unrelated - those happen even on our passing builds. (I suspect that some of our unit tests are doing something unsupported by command buffer). I don't think any other bots failed - but this is the only bot we run that tests command buffer. All others (including the other Mac bots) are using native desktop GL. More importantly, on those other bots, we get the actual device/driver information when we're determining our capabilities and workarounds, so we've blacklisted certain configurations where our bots weren't doing sRGB mip-mapping correctly:

- Intel
- ATI
- NVIDIA on Mac

With command buffer, we don't learn what the underlying HW is, so we can't implement our workaround. (And our workaround relies on ES3 capabilities, so we can't use it on command buffer anyway).
Cc: herb@chromium.org
We (Skia) are seeing more problems with mip-mapping. Our mip-map image tests on the commandbuffer bot are generating some very wrong results:

https://gold.skia.org/search?head=true&include=false&limit=50&neg=false&pos=false&query=config%3Dcommandbuffer%26source_type%3Dgm&unt=true

Best example is the first result from that page:

https://gold.skia.org/diff?test=downsamplebitmap_image_medium_mandrill_512.png&left=e6310ca1f321a5327c975d9833d87fb8&right=fc337aba5203ee2cff6e7d9a4dcdd001

With the extensions landed, we're going to be creating our textures as sRGB, then (in those tests) rendering to a non-sRGB surface, and using the decode extension to turn off sRGB decode of the texture. We are also calling glGenerateMipmap to generate the mipmaps. The images look like the resulting mipmaps are actually going through more 'linear -> sRGB' conversions than 'sRGB -> linear' (likely that the decode on read is disabled, but the encode on write isn't). It's apparent from that first result, because the image gets progressively lighter as you proceed down the mip-chain.

I can try changing things so we don't ever set the decode override while mip-mapping, but this isn't a long-term fix (and I'm still worried about state leak inside the command buffer).
Hmm. We have so many hacks+workarounds in the sRGB mipmap generation code, I wouldn't be surprised if something is going wrong.

Is this something that I can test locally on my machine to try to debug?
Yes, it should be possible. I think the steps are:

1) Start with a local checkout of Skia. Build Skia as normal:
  > gn gen out/Debug
  > ninja -C out/Debug

2) Use Skia's helper script to build and copy the command buffer DLL (this creates a dedicated GN configuration within your chromium checkout, to avoid interacting with any other local changes you may have):
  > python tools/build_command_buffer.py -c CHROME_DIR -o out/Debug

2b) To get the incorrect behavior back, you need to disable my hack. In skia/src/gpu/gl/GrGLCaps.cpp, look for:

      fSRGBDecodeDisableAffectsMipmaps = fSRGBDecodeDisableSupport &&
        kChromium_GrGLDriver != ctxInfo.driver();

... change that so that it just says:

     fSRGBDecodeDisableAffectsMipmaps = fSRGBDecodeDisableSupport;

That will restore the behavior where we try to use the SRGB_DECODE extension to control gamma-correctness of mipmaps before calling glGenerateMipmap.

3) At this point, all of Skia's GL tests and tools will support running against the command buffer (in addition to other GPU backends). (We try to dynamically load the command buffer shared library in our testing tools). You can run the test I mentioned specifically:
  > out/Debug/dm.exe --config commandbuffer --match downsamplebitmap_image_medium_mandrill_512 -w output_images

You can pass multiple configs to "--config", and all of the resulting images will be written to the "--output" directory. So running with "--config gl commandbuffer" will let you see the correct image, too.
If you normally build with goma you may want to add

--extra-gn-args use_goma=true --extra-ninja-args -j<large N>

to build_command_buffer.py
Taking another look at this (fell off the radar)

Comment 33 by herb@chromium.org, Aug 23 2017

Cc: -herb@chromium.org

Sign in to add a comment