Add command buffer support for EXT_sRGB_write_control

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}

[ccameron, 2016-11-19 00:41:22]

Description was changed from

==========
Add command buffer support for EXT_sRGB_write_control

BUG=655247
==========

to

==========
Add command buffer support for EXT_sRGB_write_control

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
==========

[ccameron, 2016-11-19 00:42:36]

The CQ bit was checked by ccameron@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-19 00:43:19]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-19 01:29:19]

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

[commit-bot: I haz the power, 2016-11-19 01:29:20]

Dry run: Try jobs failed on following builders:
  mac_optional_gpu_tests_rel on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_optional_gpu_...)

[ccameron, 2016-11-27 23:10:43]

The CQ bit was checked by ccameron@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-27 23:10:54]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-28 00:23:09]

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

[commit-bot: I haz the power, 2016-11-28 00:23:09]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[ccameron, 2016-11-28 02:17:29]

The CQ bit was checked by ccameron@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-28 02:17:43]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[ccameron, 2016-11-28 03:06:25]

Description was changed from

==========
Add command buffer support for EXT_sRGB_write_control

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
==========

to

==========
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
==========

[ccameron, 2016-11-28 03:06:25]

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

[ccameron, 2016-11-28 06:04:58]

The CQ bit was checked by ccameron@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-28 06:05:09]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[ccameron, 2016-11-28 07:47:26]

ptal -- I ended up avoiding doing any substantial refactoring of the way things
worked, since the existing scheme works well

[commit-bot: I haz the power, 2016-11-28 08:37:26]

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

[commit-bot: I haz the power, 2016-11-28 08:37:28]

Dry run: This issue passed the CQ dry run.

[ccameron, 2016-11-29 17:36:59]

post-prandial ping

[ccameron, 2016-11-30 18:41:14]

ping again

[Zhenyao Mo, 2016-11-30 19:31:11]

Mostly looks good with some minor comments.

Sorry about the delay - I just un-buried myself out of emails. In the future,
feel free to ping me through gchat.

https://codereview.chromium.org/2519603002/diff/60001/gpu/command_buffer/serv...
File gpu/command_buffer/service/context_state.h (right):

https://codereview.chromium.org/2519603002/diff/60001/gpu/command_buffer/serv...
gpu/command_buffer/service/context_state.h:361: bool framebuffer_srgb_valid_ =
false;
This is nicer than the previous tracking mechanism

https://codereview.chromium.org/2519603002/diff/60001/gpu/command_buffer/serv...
File gpu/command_buffer/service/feature_info.cc (right):

https://codereview.chromium.org/2519603002/diff/60001/gpu/command_buffer/serv...
gpu/command_buffer/service/feature_info.cc:608:
extensions.Contains("GL_EXT_sRGB_write_control")) {
Don't does this if IsWebGLContext(). Otherwise you leak this enum to WebGL
context and you will need to go to multiple function entries points in
WebGLRenderingContext to filter out it.

https://codereview.chromium.org/2519603002/diff/60001/gpu/command_buffer/serv...
File gpu/command_buffer/service/feature_info_unittest.cc (right):

https://codereview.chromium.org/2519603002/diff/60001/gpu/command_buffer/serv...
gpu/command_buffer/service/feature_info_unittest.cc:265: // Note that because
GL_EXT_sRGB is a substring of
nit: remove this

https://codereview.chromium.org/2519603002/diff/60001/gpu/command_buffer/serv...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

https://codereview.chromium.org/2519603002/diff/60001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder.cc:4323:
needs_enable_disable_framebuffer_srgb = true;
You can DCHECK this because desktop_srgb_support should set
ext_srgb_write_control to true.

[ccameron, 2016-11-30 23:58:01]

Description was changed from

==========
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
==========

to

==========
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
==========

[ccameron, 2016-11-30 23:58:03]

The CQ bit was checked by ccameron@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-30 23:58:33]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[ccameron, 2016-11-30 23:59:44]

updated -- ptal

https://codereview.chromium.org/2519603002/diff/60001/gpu/command_buffer/serv...
File gpu/command_buffer/service/feature_info.cc (right):

https://codereview.chromium.org/2519603002/diff/60001/gpu/command_buffer/serv...
gpu/command_buffer/service/feature_info.cc:608:
extensions.Contains("GL_EXT_sRGB_write_control")) {
On 2016/11/30 19:31:11, Zhenyao Mo wrote:
> Don't does this if IsWebGLContext(). Otherwise you leak this enum to WebGL
> context and you will need to go to multiple function entries points in
> WebGLRenderingContext to filter out it.

Updated to track if we support srgb framebuffers, and only add support for
write_control if we do and we aren't in a WebGL context.

https://codereview.chromium.org/2519603002/diff/60001/gpu/command_buffer/serv...
File gpu/command_buffer/service/feature_info_unittest.cc (right):

https://codereview.chromium.org/2519603002/diff/60001/gpu/command_buffer/serv...
gpu/command_buffer/service/feature_info_unittest.cc:265: // Note that because
GL_EXT_sRGB is a substring of
On 2016/11/30 19:31:11, Zhenyao Mo wrote:
> nit: remove this

Done.

https://codereview.chromium.org/2519603002/diff/60001/gpu/command_buffer/serv...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

https://codereview.chromium.org/2519603002/diff/60001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder.cc:4323:
needs_enable_disable_framebuffer_srgb = true;
On 2016/11/30 19:31:11, Zhenyao Mo wrote:
> You can DCHECK this because desktop_srgb_support should set
> ext_srgb_write_control to true.

Done.

[Zhenyao Mo, 2016-12-01 00:05:02]

lgtm with one minor issue fixed

https://codereview.chromium.org/2519603002/diff/80001/gpu/command_buffer/serv...
File gpu/command_buffer/service/feature_info.cc (right):

https://codereview.chromium.org/2519603002/diff/80001/gpu/command_buffer/serv...
gpu/command_buffer/service/feature_info.cc:607: has_srgb_framebuffer_support =
true;
You should use gl_version_info_->is_es3 instead. context_type_ is the command
buffer context type, not the underlying driver context type.

[ccameron, 2016-12-01 00:39:02]

https://codereview.chromium.org/2519603002/diff/80001/gpu/command_buffer/serv...
File gpu/command_buffer/service/feature_info.cc (right):

https://codereview.chromium.org/2519603002/diff/80001/gpu/command_buffer/serv...
gpu/command_buffer/service/feature_info.cc:607: has_srgb_framebuffer_support =
true;
On 2016/12/01 00:05:02, Zhenyao Mo wrote:
> You should use gl_version_info_->is_es3 instead. context_type_ is the command
> buffer context type, not the underlying driver context type.

Done.

[ccameron, 2016-12-01 00:39:07]

The CQ bit was checked by ccameron@chromium.org

[ccameron, 2016-12-01 00:39:07]

The patchset sent to the CQ was uploaded after l-g-t-m from zmo@chromium.org
Link to the patchset: https://codereview.chromium.org/2519603002/#ps100001
(title: "Review feedback")

[commit-bot: I haz the power, 2016-12-01 00:39:39]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-01 02:00:20]

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

[commit-bot: I haz the power, 2016-12-01 02:00:21]

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

[ccameron, 2016-12-01 02:13:27]

The CQ bit was checked by ccameron@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-01 02:13:45]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[ccameron, 2016-12-01 02:14:34]

https://codereview.chromium.org/2519603002/diff/60001/gpu/command_buffer/serv...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

https://codereview.chromium.org/2519603002/diff/60001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder.cc:4323:
needs_enable_disable_framebuffer_srgb = true;
On 2016/11/30 23:59:44, ccameron wrote:
> On 2016/11/30 19:31:11, Zhenyao Mo wrote:
> > You can DCHECK this because desktop_srgb_support should set
> > ext_srgb_write_control to true.
> 
> Done.

Oops, now that I'm not enabling write control for WebGL contexts, this can't be
a DCHECK anymore.

[ccameron, 2016-12-01 02:14:42]

The CQ bit was unchecked by ccameron@chromium.org

[ccameron, 2016-12-01 02:15:05]

The CQ bit was checked by ccameron@chromium.org

[ccameron, 2016-12-01 02:15:06]

The patchset sent to the CQ was uploaded after l-g-t-m from zmo@chromium.org
Link to the patchset: https://codereview.chromium.org/2519603002/#ps120001
(title: "Remove DCHECK for write-control -- WebGL doesn't have it"")

[commit-bot: I haz the power, 2016-12-01 02:15:21]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-01 03:38:18]

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

[commit-bot: I haz the power, 2016-12-01 03:38:19]

Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[ccameron, 2016-12-01 08:10:11]

The CQ bit was checked by ccameron@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-01 08:10:28]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-01 08:23:40]

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

[commit-bot: I haz the power, 2016-12-01 08:23:41]

Dry run: Try jobs failed on following builders:
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)

[ccameron, 2016-12-01 09:37:09]

The CQ bit was checked by ccameron@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-01 09:37:15]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-01 10:28:10]

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

[commit-bot: I haz the power, 2016-12-01 10:28:11]

Dry run: This issue passed the CQ dry run.

[ccameron, 2016-12-01 17:04:43]

The CQ bit was checked by ccameron@chromium.org

[ccameron, 2016-12-01 17:04:43]

The patchset sent to the CQ was uploaded after l-g-t-m from zmo@chromium.org
Link to the patchset: https://codereview.chromium.org/2519603002/#ps160001
(title: "and without breaking compile")

[commit-bot: I haz the power, 2016-12-01 17:05:15]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-01 17:10:13]

CQ is committing da patch.

Bot data: {"patchset_id": 160001, "attempt_start_ts": 1480611883027690,
"parent_rev": "159c79b952bd2b80a13f4dfb5ad31ca60b0ca0e8", "commit_rev":
"8c83d1d289d131449d888937e57c8c144029c6dc"}

[commit-bot: I haz the power, 2016-12-01 17:10:46]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2016-12-01 17:13:06]

Description was changed from

==========
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
==========

to

==========
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}
==========

[commit-bot: I haz the power, 2016-12-01 17:13:08]

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

[enne (OOO), 2016-12-01 23:50:39]

A revert of this CL (patchset #9 id:160001) has been created in
https://codereview.chromium.org/2537343008/ by enne@chromium.org.

The reason for reverting is: Breaks gl_tests on GPU FYI bots

BUG=670487.