Disable FRAMEBUFFER_SRGB when restoring from a null context state

Disable FRAMEBUFFER_SRGB when restoring from a null context state

In the following case, FRAMEBUFFER_SRGB state in driver will not be correct:
1. FRAMEBUFFER_SRGB is enabled for sRGB framebuffer.
2. restore from a null context, framebuffer_srgb_ will be false after restoring.
But FRAMEBUFFER_SRGB will not be changed if we call EnableDisableFramebufferSRGB(false)
to disable FRAMEBUFFER_SRGB, because the function will return early at
"if (framebuffer_srgb_ == enable)".

This CL disable FRAMEBUFFER_SRGB when restoring from a null context state.

BUG=540900, 637801
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/a8fe152031cea3017f7dd41990d7e46d49a2ce51
Cr-Commit-Position: refs/heads/master@{#413291}

[qiankun, 2016-08-15 05:32:01]

Description was changed from

==========
Disable FRAMEBUFFER_SRGB when restoring from a null context state

BUG=540900
==========

to

==========
Disable FRAMEBUFFER_SRGB when restoring from a null context state

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

[qiankun, 2016-08-15 08:59:20]

Description was changed from

==========
Disable FRAMEBUFFER_SRGB when restoring from a null context state

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

==========
Unset FRAMEBUFFER_SRGB when restoring from a null context state

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

[qiankun, 2016-08-15 09:09:53]

Description was changed from

==========
Unset FRAMEBUFFER_SRGB when restoring from a null context state

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

==========
Unset FRAMEBUFFER_SRGB when restoring from a null context state

In the following case, FRAMEBUFFER_SRGB state in driver will not be correct:
1. FRAMEBUFFER_SRGB is enabled for sRGB framebuffer. 
2. restore from a null context, framebuffer_srgb_ will be false after restoring.
But FRAMEBUFFER_SRGB will not be changed if we call
EnableDisableFramebufferSRGB(false)
to disable FRAMEBUFFER_SRGB, because the function will return early at 
"if (framebuffer_srgb_ == enable)".

This CL adds another state "FRAMEBUFFER_SRGB_UNDEFINED" to make sure
FRAMEBUFFER_SRGB
will be updated after resoring from a null context state. 

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

[qiankun, 2016-08-15 09:35:16]

Description was changed from

==========
Unset FRAMEBUFFER_SRGB when restoring from a null context state

In the following case, FRAMEBUFFER_SRGB state in driver will not be correct:
1. FRAMEBUFFER_SRGB is enabled for sRGB framebuffer. 
2. restore from a null context, framebuffer_srgb_ will be false after restoring.
But FRAMEBUFFER_SRGB will not be changed if we call
EnableDisableFramebufferSRGB(false)
to disable FRAMEBUFFER_SRGB, because the function will return early at 
"if (framebuffer_srgb_ == enable)".

This CL adds another state "FRAMEBUFFER_SRGB_UNDEFINED" to make sure
FRAMEBUFFER_SRGB
will be updated after resoring from a null context state. 

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

==========
Unset FRAMEBUFFER_SRGB when restoring from a null context state

In the following case, FRAMEBUFFER_SRGB state in driver will not be correct:
1. FRAMEBUFFER_SRGB is enabled for sRGB framebuffer. 
2. restore from a null context, framebuffer_srgb_ will be false after restoring.
But FRAMEBUFFER_SRGB will not be changed if we call
EnableDisableFramebufferSRGB(false)
to disable FRAMEBUFFER_SRGB, because the function will return early at 
"if (framebuffer_srgb_ == enable)".

This CL adds another state "FRAMEBUFFER_SRGB_UNDEFINED" to make sure
FRAMEBUFFER_SRGB
will be updated after resoring from a null context state. 

BUG=540900 637801
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
==========

[qiankun, 2016-08-15 09:35:54]

Description was changed from

==========
Unset FRAMEBUFFER_SRGB when restoring from a null context state

In the following case, FRAMEBUFFER_SRGB state in driver will not be correct:
1. FRAMEBUFFER_SRGB is enabled for sRGB framebuffer. 
2. restore from a null context, framebuffer_srgb_ will be false after restoring.
But FRAMEBUFFER_SRGB will not be changed if we call
EnableDisableFramebufferSRGB(false)
to disable FRAMEBUFFER_SRGB, because the function will return early at 
"if (framebuffer_srgb_ == enable)".

This CL adds another state "FRAMEBUFFER_SRGB_UNDEFINED" to make sure
FRAMEBUFFER_SRGB
will be updated after resoring from a null context state. 

BUG=540900 637801
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

==========
Unset FRAMEBUFFER_SRGB when restoring from a null context state

In the following case, FRAMEBUFFER_SRGB state in driver will not be correct:
1. FRAMEBUFFER_SRGB is enabled for sRGB framebuffer. 
2. restore from a null context, framebuffer_srgb_ will be false after restoring.
But FRAMEBUFFER_SRGB will not be changed if we call
EnableDisableFramebufferSRGB(false)
to disable FRAMEBUFFER_SRGB, because the function will return early at 
"if (framebuffer_srgb_ == enable)".

This CL adds another state "FRAMEBUFFER_SRGB_UNDEFINED" to make sure
FRAMEBUFFER_SRGB
will be updated after resoring from a null context state. 

BUG=540900 637801

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

[qiankun, 2016-08-15 09:36:15]

Description was changed from

==========
Unset FRAMEBUFFER_SRGB when restoring from a null context state

In the following case, FRAMEBUFFER_SRGB state in driver will not be correct:
1. FRAMEBUFFER_SRGB is enabled for sRGB framebuffer. 
2. restore from a null context, framebuffer_srgb_ will be false after restoring.
But FRAMEBUFFER_SRGB will not be changed if we call
EnableDisableFramebufferSRGB(false)
to disable FRAMEBUFFER_SRGB, because the function will return early at 
"if (framebuffer_srgb_ == enable)".

This CL adds another state "FRAMEBUFFER_SRGB_UNDEFINED" to make sure
FRAMEBUFFER_SRGB
will be updated after resoring from a null context state. 

BUG=540900 637801

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

==========
Unset FRAMEBUFFER_SRGB when restoring from a null context state

In the following case, FRAMEBUFFER_SRGB state in driver will not be correct:
1. FRAMEBUFFER_SRGB is enabled for sRGB framebuffer. 
2. restore from a null context, framebuffer_srgb_ will be false after restoring.
But FRAMEBUFFER_SRGB will not be changed if we call
EnableDisableFramebufferSRGB(false)
to disable FRAMEBUFFER_SRGB, because the function will return early at 
"if (framebuffer_srgb_ == enable)".

This CL adds another state "FRAMEBUFFER_SRGB_UNDEFINED" to make sure
FRAMEBUFFER_SRGB
will be updated after resoring from a null context state. 

BUG=540900, 637801

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

[qiankun, 2016-08-15 09:45:17]

qiankun.miao@intel.com changed reviewers:
+ kbr@chromium.org, piman@chromium.org, yunchao.he@intel.com, zmo@chromium.org

[qiankun, 2016-08-15 09:45:18]

I uploaded two patch sets to fix the attached bug.
Intuitively, PS1 is very simple. But it caused gl_tests failed on Windows.
INVALID_ENUM will be raised from GLVirtualContextsTest.VertexArrayObjectRestore
in gl_tests. But I didn't find the root cause for "INVALID_ENUM".
PS2 adds another state to make sure FRAMEBUFFER_SRGB is enabled or disabled
after command buffer restore from a null context.

[Zhenyao Mo, 2016-08-15 22:37:27]

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

https://codereview.chromium.org/2246823002/diff/20001/gpu/command_buffer/serv...
gpu/command_buffer/service/context_state.h:157: FRAMEBUFFER_SRGB_UNDEFINED,
Do we really need this?  If the prev_context is null, it means it's the context
initialization, which means FRAMEBUFFER_SRGB is disabled.

[qiankun, 2016-08-15 23:01:07]

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

https://codereview.chromium.org/2246823002/diff/20001/gpu/command_buffer/serv...
gpu/command_buffer/service/context_state.h:157: FRAMEBUFFER_SRGB_UNDEFINED,
On 2016/08/15 22:37:27, Zhenyao Mo wrote:
> Do we really need this?  If the prev_context is null, it means it's the
context
> initialization, which means FRAMEBUFFER_SRGB is disabled.

Yes, we do need this. See the bug this CL will fix:
https://bugs.chromium.org/p/chromium/issues/detail?id=637801. 

In context initialization, we should call glDisable(FRAMEBUFFER_SRGB) to do
disable because before this initialization other context may enable
FRAMEBUFFER_SRGB and EnableDisableFramebufferSRGB(false) will not do real
disable as I pointed in the description of this CL.

[Ken Russell (switch to Gerrit), 2016-08-16 01:58:45]

I defer to zmo@'s review. If this has been tested then it seems fine to me.

CC'ing ANGLE team in case similar handling is needed in ANGLE's context
implementation.

[Zhenyao Mo, 2016-08-16 17:16:48]

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

https://codereview.chromium.org/2246823002/diff/20001/gpu/command_buffer/serv...
gpu/command_buffer/service/context_state.h:157: FRAMEBUFFER_SRGB_UNDEFINED,
On 2016/08/15 23:01:06, qiankun wrote:
> On 2016/08/15 22:37:27, Zhenyao Mo wrote:
> > Do we really need this?  If the prev_context is null, it means it's the
> context
> > initialization, which means FRAMEBUFFER_SRGB is disabled.
> 
> Yes, we do need this. See the bug this CL will fix:
> https://bugs.chromium.org/p/chromium/issues/detail?id=637801. 
> 
> In context initialization, we should call glDisable(FRAMEBUFFER_SRGB) to do
> disable because before this initialization other context may enable
> FRAMEBUFFER_SRGB and EnableDisableFramebufferSRGB(false) will not do real
> disable as I pointed in the description of this CL.

This might fix the bug, but it's not correct.  If RestoreState() is called with
previous_state set to nullptr, the FramebufferSRGB isn't undefined.  It should
be initialized to false.  In this situation, you should just set it to disabled,
and call glDisable(FRAMEBUFFER_SRGB) to make sure of it.

[qiankun, 2016-08-16 21:30:09]

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

https://codereview.chromium.org/2246823002/diff/20001/gpu/command_buffer/serv...
gpu/command_buffer/service/context_state.h:157: FRAMEBUFFER_SRGB_UNDEFINED,
On 2016/08/16 17:16:48, Zhenyao Mo wrote:
> On 2016/08/15 23:01:06, qiankun wrote:
> > On 2016/08/15 22:37:27, Zhenyao Mo wrote:
> > > Do we really need this?  If the prev_context is null, it means it's the
> > context
> > > initialization, which means FRAMEBUFFER_SRGB is disabled.
> > 
> > Yes, we do need this. See the bug this CL will fix:
> > https://bugs.chromium.org/p/chromium/issues/detail?id=637801. 
> > 
> > In context initialization, we should call glDisable(FRAMEBUFFER_SRGB) to do
> > disable because before this initialization other context may enable
> > FRAMEBUFFER_SRGB and EnableDisableFramebufferSRGB(false) will not do real
> > disable as I pointed in the description of this CL.
> 
> This might fix the bug, but it's not correct.  If RestoreState() is called
with
> previous_state set to nullptr, the FramebufferSRGB isn't undefined.  It should
> be initialized to false.  In this situation, you should just set it to
disabled,
> and call glDisable(FRAMEBUFFER_SRGB) to make sure of it.

framebuffer_srgb_ is used to avoid redundant glEnable/glDisable call. In the
case you pointed,  setting framebuffer_srg b_ as FRAMEBUFFER_SRGB_UNDEFINED will
make the following EnableDisableFramebufferSRGB() do the real glEnable/glDisable
at render time.

[Zhenyao Mo, 2016-08-16 21:31:13]

On 2016/08/16 21:30:09, qiankun wrote:
>
https://codereview.chromium.org/2246823002/diff/20001/gpu/command_buffer/serv...
> File gpu/command_buffer/service/context_state.h (right):
> 
>
https://codereview.chromium.org/2246823002/diff/20001/gpu/command_buffer/serv...
> gpu/command_buffer/service/context_state.h:157: FRAMEBUFFER_SRGB_UNDEFINED,
> On 2016/08/16 17:16:48, Zhenyao Mo wrote:
> > On 2016/08/15 23:01:06, qiankun wrote:
> > > On 2016/08/15 22:37:27, Zhenyao Mo wrote:
> > > > Do we really need this?  If the prev_context is null, it means it's the
> > > context
> > > > initialization, which means FRAMEBUFFER_SRGB is disabled.
> > > 
> > > Yes, we do need this. See the bug this CL will fix:
> > > https://bugs.chromium.org/p/chromium/issues/detail?id=637801. 
> > > 
> > > In context initialization, we should call glDisable(FRAMEBUFFER_SRGB) to
do
> > > disable because before this initialization other context may enable
> > > FRAMEBUFFER_SRGB and EnableDisableFramebufferSRGB(false) will not do real
> > > disable as I pointed in the description of this CL.
> > 
> > This might fix the bug, but it's not correct.  If RestoreState() is called
> with
> > previous_state set to nullptr, the FramebufferSRGB isn't undefined.  It
should
> > be initialized to false.  In this situation, you should just set it to
> disabled,
> > and call glDisable(FRAMEBUFFER_SRGB) to make sure of it.
> 
> framebuffer_srgb_ is used to avoid redundant glEnable/glDisable call. In the
> case you pointed,  setting framebuffer_srg b_ as FRAMEBUFFER_SRGB_UNDEFINED
will
> make the following EnableDisableFramebufferSRGB() do the real
glEnable/glDisable
> at render time.

That's OK, since it's only context initialization time, not per frame.

[qiankun, 2016-08-16 21:42:46]

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

https://codereview.chromium.org/2246823002/diff/20001/gpu/command_buffer/serv...
gpu/command_buffer/service/context_state.h:157: FRAMEBUFFER_SRGB_UNDEFINED,
On 2016/08/16 17:16:48, Zhenyao Mo wrote:
> On 2016/08/15 23:01:06, qiankun wrote:
> > On 2016/08/15 22:37:27, Zhenyao Mo wrote:
> > > Do we really need this?  If the prev_context is null, it means it's the
> > context
> > > initialization, which means FRAMEBUFFER_SRGB is disabled.
> > 
> > Yes, we do need this. See the bug this CL will fix:
> > https://bugs.chromium.org/p/chromium/issues/detail?id=637801. 
> > 
> > In context initialization, we should call glDisable(FRAMEBUFFER_SRGB) to do
> > disable because before this initialization other context may enable
> > FRAMEBUFFER_SRGB and EnableDisableFramebufferSRGB(false) will not do real
> > disable as I pointed in the description of this CL.
> 
> This might fix the bug, but it's not correct.  If RestoreState() is called
with
> previous_state set to nullptr, the FramebufferSRGB isn't undefined.  It should
> be initialized to false.  In this situation, you should just set it to
disabled,
> and call glDisable(FRAMEBUFFER_SRGB) to make sure of it.

FRAMEBUFFER_SRGB is set dynamically on demand in current implementation. So it
doesn't matter if it's enabled or disabled at context initialization time. We
will set it properly according the content.

[Zhenyao Mo, 2016-08-16 21:55:35]

On 2016/08/16 21:42:46, qiankun wrote:
>
https://codereview.chromium.org/2246823002/diff/20001/gpu/command_buffer/serv...
> File gpu/command_buffer/service/context_state.h (right):
> 
>
https://codereview.chromium.org/2246823002/diff/20001/gpu/command_buffer/serv...
> gpu/command_buffer/service/context_state.h:157: FRAMEBUFFER_SRGB_UNDEFINED,
> On 2016/08/16 17:16:48, Zhenyao Mo wrote:
> > On 2016/08/15 23:01:06, qiankun wrote:
> > > On 2016/08/15 22:37:27, Zhenyao Mo wrote:
> > > > Do we really need this?  If the prev_context is null, it means it's the
> > > context
> > > > initialization, which means FRAMEBUFFER_SRGB is disabled.
> > > 
> > > Yes, we do need this. See the bug this CL will fix:
> > > https://bugs.chromium.org/p/chromium/issues/detail?id=637801. 
> > > 
> > > In context initialization, we should call glDisable(FRAMEBUFFER_SRGB) to
do
> > > disable because before this initialization other context may enable
> > > FRAMEBUFFER_SRGB and EnableDisableFramebufferSRGB(false) will not do real
> > > disable as I pointed in the description of this CL.
> > 
> > This might fix the bug, but it's not correct.  If RestoreState() is called
> with
> > previous_state set to nullptr, the FramebufferSRGB isn't undefined.  It
should
> > be initialized to false.  In this situation, you should just set it to
> disabled,
> > and call glDisable(FRAMEBUFFER_SRGB) to make sure of it.
> 
> FRAMEBUFFER_SRGB is set dynamically on demand in current implementation. So it
> doesn't matter if it's enabled or disabled at context initialization time. We
> will set it properly according the content.

That's not true.  We need it to be disabled all the time unless we need it
enabled for certain operations (and disable it again right after the operation).

At context initialization time, it is definitely disabled. If it's enabled, then
many operations will generate wrong colors.

[qiankun, 2016-08-16 22:21:08]

On 2016/08/16 21:55:35, Zhenyao Mo wrote:
> On 2016/08/16 21:42:46, qiankun wrote:
> >
>
https://codereview.chromium.org/2246823002/diff/20001/gpu/command_buffer/serv...
> > File gpu/command_buffer/service/context_state.h (right):
> > 
> >
>
https://codereview.chromium.org/2246823002/diff/20001/gpu/command_buffer/serv...
> > gpu/command_buffer/service/context_state.h:157: FRAMEBUFFER_SRGB_UNDEFINED,
> > On 2016/08/16 17:16:48, Zhenyao Mo wrote:
> > > On 2016/08/15 23:01:06, qiankun wrote:
> > > > On 2016/08/15 22:37:27, Zhenyao Mo wrote:
> > > > > Do we really need this?  If the prev_context is null, it means it's
the
> > > > context
> > > > > initialization, which means FRAMEBUFFER_SRGB is disabled.
> > > > 
> > > > Yes, we do need this. See the bug this CL will fix:
> > > > https://bugs.chromium.org/p/chromium/issues/detail?id=637801. 
> > > > 
> > > > In context initialization, we should call glDisable(FRAMEBUFFER_SRGB) to
> do
> > > > disable because before this initialization other context may enable
> > > > FRAMEBUFFER_SRGB and EnableDisableFramebufferSRGB(false) will not do
real
> > > > disable as I pointed in the description of this CL.
> > > 
> > > This might fix the bug, but it's not correct.  If RestoreState() is called
> > with
> > > previous_state set to nullptr, the FramebufferSRGB isn't undefined.  It
> should
> > > be initialized to false.  In this situation, you should just set it to
> > disabled,
> > > and call glDisable(FRAMEBUFFER_SRGB) to make sure of it.
> > 
> > FRAMEBUFFER_SRGB is set dynamically on demand in current implementation. So
it
> > doesn't matter if it's enabled or disabled at context initialization time.
We
> > will set it properly according the content.
> 
> That's not true.  We need it to be disabled all the time unless we need it
> enabled for certain operations (and disable it again right after the
operation).
> 
> At context initialization time, it is definitely disabled. If it's enabled,
then
> many operations will generate wrong colors.

In RestoreState(), though framebuffer_srgb_ is set to FRAMEBUFFER_SRGB_DISABLED,
FRAMEBUFFER_SRGB isn't really disabled.
glDisable should be called for FRAMEBUFFER_SRGB explicitly at context
initialization time. RestoreState() isn't the correct place to call glDisable,
see my Patch Set1.
Do you have some suggestions where to call glDisable(FRAMEBUFFER_SRGB)?

[Zhenyao Mo, 2016-08-16 22:33:04]

On 2016/08/16 22:21:08, qiankun wrote:
> On 2016/08/16 21:55:35, Zhenyao Mo wrote:
> > On 2016/08/16 21:42:46, qiankun wrote:
> > >
> >
>
https://codereview.chromium.org/2246823002/diff/20001/gpu/command_buffer/serv...
> > > File gpu/command_buffer/service/context_state.h (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/2246823002/diff/20001/gpu/command_buffer/serv...
> > > gpu/command_buffer/service/context_state.h:157:
FRAMEBUFFER_SRGB_UNDEFINED,
> > > On 2016/08/16 17:16:48, Zhenyao Mo wrote:
> > > > On 2016/08/15 23:01:06, qiankun wrote:
> > > > > On 2016/08/15 22:37:27, Zhenyao Mo wrote:
> > > > > > Do we really need this?  If the prev_context is null, it means it's
> the
> > > > > context
> > > > > > initialization, which means FRAMEBUFFER_SRGB is disabled.
> > > > > 
> > > > > Yes, we do need this. See the bug this CL will fix:
> > > > > https://bugs.chromium.org/p/chromium/issues/detail?id=637801. 
> > > > > 
> > > > > In context initialization, we should call glDisable(FRAMEBUFFER_SRGB)
to
> > do
> > > > > disable because before this initialization other context may enable
> > > > > FRAMEBUFFER_SRGB and EnableDisableFramebufferSRGB(false) will not do
> real
> > > > > disable as I pointed in the description of this CL.
> > > > 
> > > > This might fix the bug, but it's not correct.  If RestoreState() is
called
> > > with
> > > > previous_state set to nullptr, the FramebufferSRGB isn't undefined.  It
> > should
> > > > be initialized to false.  In this situation, you should just set it to
> > > disabled,
> > > > and call glDisable(FRAMEBUFFER_SRGB) to make sure of it.
> > > 
> > > FRAMEBUFFER_SRGB is set dynamically on demand in current implementation.
So
> it
> > > doesn't matter if it's enabled or disabled at context initialization time.
> We
> > > will set it properly according the content.
> > 
> > That's not true.  We need it to be disabled all the time unless we need it
> > enabled for certain operations (and disable it again right after the
> operation).
> > 
> > At context initialization time, it is definitely disabled. If it's enabled,
> then
> > many operations will generate wrong colors.
> 
> In RestoreState(), though framebuffer_srgb_ is set to
FRAMEBUFFER_SRGB_DISABLED,
> FRAMEBUFFER_SRGB isn't really disabled.
> glDisable should be called for FRAMEBUFFER_SRGB explicitly at context
> initialization time. RestoreState() isn't the correct place to call glDisable,
> see my Patch Set1.
> Do you have some suggestions where to call glDisable(FRAMEBUFFER_SRGB)?

In GLES2DecoderImpl::Initialize()?

[qiankun, 2016-08-17 14:29:56]

I updated the CL as we discussed to do glDisable(FRAMEBUFFER_SRGB) at context
initialization time. PTAL.

[Zhenyao Mo, 2016-08-17 17:21:09]

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

https://codereview.chromium.org/2246823002/diff/60001/gpu/command_buffer/serv...
gpu/command_buffer/service/context_state.cc:499: framebuffer_srgb_ =
FRAMEBUFFER_SRGB_UNDEFINED;
Per our discussion, this should be DISABLED.

Actually you can revert all the changes in context_state.h and context_state.cc.

[qiankun, 2016-08-18 00:19:56]

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

https://codereview.chromium.org/2246823002/diff/60001/gpu/command_buffer/serv...
gpu/command_buffer/service/context_state.cc:499: framebuffer_srgb_ =
FRAMEBUFFER_SRGB_UNDEFINED;
On 2016/08/17 17:21:09, Zhenyao Mo wrote:
> Per our discussion, this should be DISABLED.
> 
> Actually you can revert all the changes in context_state.h and
context_state.cc.

I still cannot revert these changes. There are still some situations where
context is restored from a null context but it isn't a context initialization.
glDisable(FRAMEBUFFER_SRGB) in context initialization doesn't help in such
situation.

[Zhenyao Mo, 2016-08-18 00:34:43]

On 2016/08/18 00:19:56, qiankun wrote:
>
https://codereview.chromium.org/2246823002/diff/60001/gpu/command_buffer/serv...
> File gpu/command_buffer/service/context_state.cc (right):
> 
>
https://codereview.chromium.org/2246823002/diff/60001/gpu/command_buffer/serv...
> gpu/command_buffer/service/context_state.cc:499: framebuffer_srgb_ =
> FRAMEBUFFER_SRGB_UNDEFINED;
> On 2016/08/17 17:21:09, Zhenyao Mo wrote:
> > Per our discussion, this should be DISABLED.
> > 
> > Actually you can revert all the changes in context_state.h and
> context_state.cc.
> 
> I still cannot revert these changes. There are still some situations where
> context is restored from a null context but it isn't a context initialization.
> glDisable(FRAMEBUFFER_SRGB) in context initialization doesn't help in such
> situation.

Really? Do you know what are these situations?

piman@: do you know on top of your head?

[piman, 2016-08-18 01:13:55]

On Wed, Aug 17, 2016 at 5:34 PM, <zmo@chromium.org> wrote:

> On 2016/08/18 00:19:56, qiankun wrote:
> >
> https://codereview.chromium.org/2246823002/diff/60001/gpu/
> command_buffer/service/context_state.cc
> > File gpu/command_buffer/service/context_state.cc (right):
> >
> >
> https://codereview.chromium.org/2246823002/diff/60001/gpu/
> command_buffer/service/context_state.cc#newcode499
> > gpu/command_buffer/service/context_state.cc:499: framebuffer_srgb_ =
> > FRAMEBUFFER_SRGB_UNDEFINED;
> > On 2016/08/17 17:21:09, Zhenyao Mo wrote:
> > > Per our discussion, this should be DISABLED.
> > >
> > > Actually you can revert all the changes in context_state.h and
> > context_state.cc.
> >
> > I still cannot revert these changes. There are still some situations
> where
> > context is restored from a null context but it isn't a context
> initialization.
> > glDisable(FRAMEBUFFER_SRGB) in context initialization doesn't help in
> such
> > situation.
>
> Really? Do you know what are these situations?
>
> piman@: do you know on top of your head?
>

prev_state would be null if we switched between real contexts, though I
don't think that happens in production (maybe in tests?).

In that case, we could choose to just make the current context consistent
with the value we want, rather than try to do it lazily?


> https://codereview.chromium.org/2246823002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[qiankun, 2016-08-18 07:04:34]

On 2016/08/18 00:34:43, Zhenyao Mo wrote:
> On 2016/08/18 00:19:56, qiankun wrote:
> > I still cannot revert these changes. There are still some situations where
> > context is restored from a null context but it isn't a context
initialization.
> > glDisable(FRAMEBUFFER_SRGB) in context initialization doesn't help in such
> > situation.
> 
> Really? Do you know what are these situations?

See the following call stack:
#0  (anonymous namespace)::(anonymous namespace)::ContextState::RestoreState
(this=0x315bc8263148, prev_state=0x0) at
../../gpu/command_buffer/service/context_state.cc:499
#1  0x0000564e3f3344be in (anonymous namespace)::(anonymous
namespace)::GLES2DecoderImpl::RestoreState (this=0x315bc8263020, prev_state=0x0)
at ../../gpu/command_buffer/service/gles2_cmd_
decoder.cc:5393
#2  0x0000564e3f2fd084 in (anonymous
namespace)::GLStateRestorerImpl::RestoreState (this=0x315bc7aecb60,
prev_state=0x0) at ../../gpu/command_buffer/service/gl_state_restorer_impl.cc:30
#3  0x0000564e38353b2d in (anonymous namespace)::GLContext::MakeVirtuallyCurrent
(this=0x315bc821ca20, virtual_context=0x315bc7eba200, surface=0x315bc79984a0) at
../../ui/gl/gl_context.cc
:231
#4  0x0000564e3f2fc844 in (anonymous namespace)::GLContextVirtual::MakeCurrent
(this=0x315bc7eba200, surface=0x315bc79984a0) at
../../gpu/command_buffer/service/gl_context_virtual.cc:38
#5  0x0000564e3f32d25f in (anonymous namespace)::(anonymous
namespace)::GLES2DecoderImpl::MakeCurrent (this=0x315bc8263020) at
../../gpu/command_buffer/service/gles2_cmd_decoder.cc:4090
#6  0x0000564e3e8703b0 in (anonymous
namespace)::GpuCommandBufferStub::MakeCurrent (this=0x315bc790ac60) at
../../gpu/ipc/service/gpu_command_buffer_stub.cc:385
#7  0x0000564e3e86f336 in (anonymous
namespace)::GpuCommandBufferStub::OnMessageReceived (this=0x315bc790ac60,
message=...) at ../../gpu/ipc/service/gpu_command_buffer_stub.cc:225
#8  0x0000564e3e84c65b in (anonymous namespace)::MessageRouter::RouteMessage
(this=0x315bc7878430, msg=...) at ../../ipc/message_router.cc:52
#9  0x0000564e3e8577a2 in (anonymous namespace)::GpuChannel::HandleMessageHelper
(this=0x315bc78783a0, msg=...) at ../../gpu/ipc/service/gpu_channel.cc:807

Here is the context state status change flow:
Open webgl-conformance-tests.html?version=2.0.0 in first tab:
in GLES2DecoderImpl::Initialize: context state = 0x67e196b27f8
in GLES2DecoderImpl::Initialize: context state = 0x67e19779cf8
in GLES2DecoderImpl::Initialize: context state = 0x67e19d9c0f8
in GLES2DecoderImpl::Initialize: context state = 0x67e19d9c7f8
in GLES2DecoderImpl::Initialize: context state = 0x67e1a536ef8
in GLES2DecoderImpl::Initialize: context state = 0x67e1a8267f8
in ContextState::RestoreState: context state = 0x67e1a8267f8 prev_state = (nil)
in ContextState::RestoreState: context state = 0x67e196b27f8 prev_state = (nil)

open a blank page in second tab:
in GLES2DecoderImpl::Initialize: context state = 0x67e1b0048f8
in GLES2DecoderImpl::Initialize: context state = 0x67e1adaf0f8
in GLES2DecoderImpl::Initialize: context state = 0x67e1a0301f8

open a page in second tab:
in GLES2DecoderImpl::Initialize: context state = 0x67e1a5398f8
in GLES2DecoderImpl::Initialize: context state = 0x67e1a03daf8
in ContextState::RestoreState: context state = 0x67e1a5398f8 prev_state = (nil)
in GLES2DecoderImpl::Initialize: context state = 0x67e1a8275f8

open a new page in the second tab:                                              
                                                                                
                         
in GLES2DecoderImpl::Initialize: context state = 0x67e1acdcef8
in ContextState::RestoreState: context state = 0x67e1a5398f8 prev_state = (nil)

You can see "context state = 0x67e1a5398f8" call RestoreState with a null
prev_state twice from above context state change list.

[Zhenyao Mo, 2016-08-18 21:46:51]

On 2016/08/18 07:04:34, qiankun wrote:
> On 2016/08/18 00:34:43, Zhenyao Mo wrote:
> > On 2016/08/18 00:19:56, qiankun wrote:
> > > I still cannot revert these changes. There are still some situations where
> > > context is restored from a null context but it isn't a context
> initialization.
> > > glDisable(FRAMEBUFFER_SRGB) in context initialization doesn't help in such
> > > situation.
> > 
> > Really? Do you know what are these situations?
> 
> See the following call stack:
> #0  (anonymous namespace)::(anonymous namespace)::ContextState::RestoreState
> (this=0x315bc8263148, prev_state=0x0) at
> ../../gpu/command_buffer/service/context_state.cc:499
> #1  0x0000564e3f3344be in (anonymous namespace)::(anonymous
> namespace)::GLES2DecoderImpl::RestoreState (this=0x315bc8263020,
prev_state=0x0)
> at ../../gpu/command_buffer/service/gles2_cmd_
> decoder.cc:5393
> #2  0x0000564e3f2fd084 in (anonymous
> namespace)::GLStateRestorerImpl::RestoreState (this=0x315bc7aecb60,
> prev_state=0x0) at
../../gpu/command_buffer/service/gl_state_restorer_impl.cc:30
> #3  0x0000564e38353b2d in (anonymous
namespace)::GLContext::MakeVirtuallyCurrent
> (this=0x315bc821ca20, virtual_context=0x315bc7eba200, surface=0x315bc79984a0)
at
> ../../ui/gl/gl_context.cc
> :231
> #4  0x0000564e3f2fc844 in (anonymous namespace)::GLContextVirtual::MakeCurrent
> (this=0x315bc7eba200, surface=0x315bc79984a0) at
> ../../gpu/command_buffer/service/gl_context_virtual.cc:38
> #5  0x0000564e3f32d25f in (anonymous namespace)::(anonymous
> namespace)::GLES2DecoderImpl::MakeCurrent (this=0x315bc8263020) at
> ../../gpu/command_buffer/service/gles2_cmd_decoder.cc:4090
> #6  0x0000564e3e8703b0 in (anonymous
> namespace)::GpuCommandBufferStub::MakeCurrent (this=0x315bc790ac60) at
> ../../gpu/ipc/service/gpu_command_buffer_stub.cc:385
> #7  0x0000564e3e86f336 in (anonymous
> namespace)::GpuCommandBufferStub::OnMessageReceived (this=0x315bc790ac60,
> message=...) at ../../gpu/ipc/service/gpu_command_buffer_stub.cc:225
> #8  0x0000564e3e84c65b in (anonymous namespace)::MessageRouter::RouteMessage
> (this=0x315bc7878430, msg=...) at ../../ipc/message_router.cc:52
> #9  0x0000564e3e8577a2 in (anonymous
namespace)::GpuChannel::HandleMessageHelper
> (this=0x315bc78783a0, msg=...) at ../../gpu/ipc/service/gpu_channel.cc:807
> 
> Here is the context state status change flow:
> Open webgl-conformance-tests.html?version=2.0.0 in first tab:
> in GLES2DecoderImpl::Initialize: context state = 0x67e196b27f8
> in GLES2DecoderImpl::Initialize: context state = 0x67e19779cf8
> in GLES2DecoderImpl::Initialize: context state = 0x67e19d9c0f8
> in GLES2DecoderImpl::Initialize: context state = 0x67e19d9c7f8
> in GLES2DecoderImpl::Initialize: context state = 0x67e1a536ef8
> in GLES2DecoderImpl::Initialize: context state = 0x67e1a8267f8
> in ContextState::RestoreState: context state = 0x67e1a8267f8 prev_state =
(nil)
> in ContextState::RestoreState: context state = 0x67e196b27f8 prev_state =
(nil)
> 
> open a blank page in second tab:
> in GLES2DecoderImpl::Initialize: context state = 0x67e1b0048f8
> in GLES2DecoderImpl::Initialize: context state = 0x67e1adaf0f8
> in GLES2DecoderImpl::Initialize: context state = 0x67e1a0301f8
> 
> open a page in second tab:
> in GLES2DecoderImpl::Initialize: context state = 0x67e1a5398f8
> in GLES2DecoderImpl::Initialize: context state = 0x67e1a03daf8
> in ContextState::RestoreState: context state = 0x67e1a5398f8 prev_state =
(nil)
> in GLES2DecoderImpl::Initialize: context state = 0x67e1a8275f8
> 
> open a new page in the second tab:                                            
 
>                                                                               
 
>                          
> in GLES2DecoderImpl::Initialize: context state = 0x67e1acdcef8
> in ContextState::RestoreState: context state = 0x67e1a5398f8 prev_state =
(nil)
> 
> You can see "context state = 0x67e1a5398f8" call RestoreState with a null
> prev_state twice from above context state change list.

See piman's comment.  Calling glDisable in Decoder::Initialize() is exactly what
he suggested, we just need to do it in call situations where prev_state is null.

Leaving it as UNDEFINED is buggy - it could be enabled or disabled, which is
definitely the wrong thing - it just hasn't manifested yet.  We have to know at
each moment the state of FRAMEBUFFER_SRGB.

[qiankun, 2016-08-18 23:04:54]

On 2016/08/18 21:46:51, Zhenyao Mo wrote:
> On 2016/08/18 07:04:34, qiankun wrote:
> > On 2016/08/18 00:34:43, Zhenyao Mo wrote:
> > > On 2016/08/18 00:19:56, qiankun wrote:
> > > > I still cannot revert these changes. There are still some situations
where
> > > > context is restored from a null context but it isn't a context
> > initialization.
> > > > glDisable(FRAMEBUFFER_SRGB) in context initialization doesn't help in
such
> > > > situation.
> > > 
> > > Really? Do you know what are these situations?
> > 
> > See the following call stack:
> > #0  (anonymous namespace)::(anonymous namespace)::ContextState::RestoreState
> > (this=0x315bc8263148, prev_state=0x0) at
> > ../../gpu/command_buffer/service/context_state.cc:499
> > #1  0x0000564e3f3344be in (anonymous namespace)::(anonymous
> > namespace)::GLES2DecoderImpl::RestoreState (this=0x315bc8263020,
> prev_state=0x0)
> > at ../../gpu/command_buffer/service/gles2_cmd_
> > decoder.cc:5393
> > #2  0x0000564e3f2fd084 in (anonymous
> > namespace)::GLStateRestorerImpl::RestoreState (this=0x315bc7aecb60,
> > prev_state=0x0) at
> ../../gpu/command_buffer/service/gl_state_restorer_impl.cc:30
> > #3  0x0000564e38353b2d in (anonymous
> namespace)::GLContext::MakeVirtuallyCurrent
> > (this=0x315bc821ca20, virtual_context=0x315bc7eba200,
surface=0x315bc79984a0)
> at
> > ../../ui/gl/gl_context.cc
> > :231
> > #4  0x0000564e3f2fc844 in (anonymous
namespace)::GLContextVirtual::MakeCurrent
> > (this=0x315bc7eba200, surface=0x315bc79984a0) at
> > ../../gpu/command_buffer/service/gl_context_virtual.cc:38
> > #5  0x0000564e3f32d25f in (anonymous namespace)::(anonymous
> > namespace)::GLES2DecoderImpl::MakeCurrent (this=0x315bc8263020) at
> > ../../gpu/command_buffer/service/gles2_cmd_decoder.cc:4090
> > #6  0x0000564e3e8703b0 in (anonymous
> > namespace)::GpuCommandBufferStub::MakeCurrent (this=0x315bc790ac60) at
> > ../../gpu/ipc/service/gpu_command_buffer_stub.cc:385
> > #7  0x0000564e3e86f336 in (anonymous
> > namespace)::GpuCommandBufferStub::OnMessageReceived (this=0x315bc790ac60,
> > message=...) at ../../gpu/ipc/service/gpu_command_buffer_stub.cc:225
> > #8  0x0000564e3e84c65b in (anonymous namespace)::MessageRouter::RouteMessage
> > (this=0x315bc7878430, msg=...) at ../../ipc/message_router.cc:52
> > #9  0x0000564e3e8577a2 in (anonymous
> namespace)::GpuChannel::HandleMessageHelper
> > (this=0x315bc78783a0, msg=...) at ../../gpu/ipc/service/gpu_channel.cc:807
> > 
> > Here is the context state status change flow:
> > Open webgl-conformance-tests.html?version=2.0.0 in first tab:
> > in GLES2DecoderImpl::Initialize: context state = 0x67e196b27f8
> > in GLES2DecoderImpl::Initialize: context state = 0x67e19779cf8
> > in GLES2DecoderImpl::Initialize: context state = 0x67e19d9c0f8
> > in GLES2DecoderImpl::Initialize: context state = 0x67e19d9c7f8
> > in GLES2DecoderImpl::Initialize: context state = 0x67e1a536ef8
> > in GLES2DecoderImpl::Initialize: context state = 0x67e1a8267f8
> > in ContextState::RestoreState: context state = 0x67e1a8267f8 prev_state =
> (nil)
> > in ContextState::RestoreState: context state = 0x67e196b27f8 prev_state =
> (nil)
> > 
> > open a blank page in second tab:
> > in GLES2DecoderImpl::Initialize: context state = 0x67e1b0048f8
> > in GLES2DecoderImpl::Initialize: context state = 0x67e1adaf0f8
> > in GLES2DecoderImpl::Initialize: context state = 0x67e1a0301f8
> > 
> > open a page in second tab:
> > in GLES2DecoderImpl::Initialize: context state = 0x67e1a5398f8
> > in GLES2DecoderImpl::Initialize: context state = 0x67e1a03daf8
> > in ContextState::RestoreState: context state = 0x67e1a5398f8 prev_state =
> (nil)
> > in GLES2DecoderImpl::Initialize: context state = 0x67e1a8275f8
> > 
> > open a new page in the second tab:                                          
 
>  
> >                                                                             
 
>  
> >                          
> > in GLES2DecoderImpl::Initialize: context state = 0x67e1acdcef8
> > in ContextState::RestoreState: context state = 0x67e1a5398f8 prev_state =
> (nil)
> > 
> > You can see "context state = 0x67e1a5398f8" call RestoreState with a null
> > prev_state twice from above context state change list.
> 
> See piman's comment.  Calling glDisable in Decoder::Initialize() is exactly
what
> he suggested, we just need to do it in call situations where prev_state is
null.
> 
> Leaving it as UNDEFINED is buggy - it could be enabled or disabled, which is
> definitely the wrong thing - it just hasn't manifested yet.  We have to know
at
> each moment the state of FRAMEBUFFER_SRGB.

Leaving framebuffer_srgb_ as FRAMEBUFFER_SRGB_UNDEFINED is not set
FRAMEBUFFER_SRGB flag as undefined. It just let next
EnableDisableFramebufferSRGB() call do glEnable or glDisable. It's harmless to
gl state. 

EnableDisableFramebufferSRGB() will set FRAMEBUFFER_SRGB as what we want. If we
don't use framebuffer_srgb_, it will set FRAMEBUFFER_SRGB flag every time. If we
keep updating framebuffer_srgb_, EnableDisableFramebufferSRGB() will not enable
FRAMEBUFFER_SRGB if it's already enabled or disable FRAMEBUFFER_SRGB if it's
already disabled.

[Zhenyao Mo, 2016-08-18 23:34:02]

On 2016/08/18 23:04:54, qiankun wrote:
> On 2016/08/18 21:46:51, Zhenyao Mo wrote:
> > On 2016/08/18 07:04:34, qiankun wrote:
> > > On 2016/08/18 00:34:43, Zhenyao Mo wrote:
> > > > On 2016/08/18 00:19:56, qiankun wrote:
> > > > > I still cannot revert these changes. There are still some situations
> where
> > > > > context is restored from a null context but it isn't a context
> > > initialization.
> > > > > glDisable(FRAMEBUFFER_SRGB) in context initialization doesn't help in
> such
> > > > > situation.
> > > > 
> > > > Really? Do you know what are these situations?
> > > 
> > > See the following call stack:
> > > #0  (anonymous namespace)::(anonymous
namespace)::ContextState::RestoreState
> > > (this=0x315bc8263148, prev_state=0x0) at
> > > ../../gpu/command_buffer/service/context_state.cc:499
> > > #1  0x0000564e3f3344be in (anonymous namespace)::(anonymous
> > > namespace)::GLES2DecoderImpl::RestoreState (this=0x315bc8263020,
> > prev_state=0x0)
> > > at ../../gpu/command_buffer/service/gles2_cmd_
> > > decoder.cc:5393
> > > #2  0x0000564e3f2fd084 in (anonymous
> > > namespace)::GLStateRestorerImpl::RestoreState (this=0x315bc7aecb60,
> > > prev_state=0x0) at
> > ../../gpu/command_buffer/service/gl_state_restorer_impl.cc:30
> > > #3  0x0000564e38353b2d in (anonymous
> > namespace)::GLContext::MakeVirtuallyCurrent
> > > (this=0x315bc821ca20, virtual_context=0x315bc7eba200,
> surface=0x315bc79984a0)
> > at
> > > ../../ui/gl/gl_context.cc
> > > :231
> > > #4  0x0000564e3f2fc844 in (anonymous
> namespace)::GLContextVirtual::MakeCurrent
> > > (this=0x315bc7eba200, surface=0x315bc79984a0) at
> > > ../../gpu/command_buffer/service/gl_context_virtual.cc:38
> > > #5  0x0000564e3f32d25f in (anonymous namespace)::(anonymous
> > > namespace)::GLES2DecoderImpl::MakeCurrent (this=0x315bc8263020) at
> > > ../../gpu/command_buffer/service/gles2_cmd_decoder.cc:4090
> > > #6  0x0000564e3e8703b0 in (anonymous
> > > namespace)::GpuCommandBufferStub::MakeCurrent (this=0x315bc790ac60) at
> > > ../../gpu/ipc/service/gpu_command_buffer_stub.cc:385
> > > #7  0x0000564e3e86f336 in (anonymous
> > > namespace)::GpuCommandBufferStub::OnMessageReceived (this=0x315bc790ac60,
> > > message=...) at ../../gpu/ipc/service/gpu_command_buffer_stub.cc:225
> > > #8  0x0000564e3e84c65b in (anonymous
namespace)::MessageRouter::RouteMessage
> > > (this=0x315bc7878430, msg=...) at ../../ipc/message_router.cc:52
> > > #9  0x0000564e3e8577a2 in (anonymous
> > namespace)::GpuChannel::HandleMessageHelper
> > > (this=0x315bc78783a0, msg=...) at ../../gpu/ipc/service/gpu_channel.cc:807
> > > 
> > > Here is the context state status change flow:
> > > Open webgl-conformance-tests.html?version=2.0.0 in first tab:
> > > in GLES2DecoderImpl::Initialize: context state = 0x67e196b27f8
> > > in GLES2DecoderImpl::Initialize: context state = 0x67e19779cf8
> > > in GLES2DecoderImpl::Initialize: context state = 0x67e19d9c0f8
> > > in GLES2DecoderImpl::Initialize: context state = 0x67e19d9c7f8
> > > in GLES2DecoderImpl::Initialize: context state = 0x67e1a536ef8
> > > in GLES2DecoderImpl::Initialize: context state = 0x67e1a8267f8
> > > in ContextState::RestoreState: context state = 0x67e1a8267f8 prev_state =
> > (nil)
> > > in ContextState::RestoreState: context state = 0x67e196b27f8 prev_state =
> > (nil)
> > > 
> > > open a blank page in second tab:
> > > in GLES2DecoderImpl::Initialize: context state = 0x67e1b0048f8
> > > in GLES2DecoderImpl::Initialize: context state = 0x67e1adaf0f8
> > > in GLES2DecoderImpl::Initialize: context state = 0x67e1a0301f8
> > > 
> > > open a page in second tab:
> > > in GLES2DecoderImpl::Initialize: context state = 0x67e1a5398f8
> > > in GLES2DecoderImpl::Initialize: context state = 0x67e1a03daf8
> > > in ContextState::RestoreState: context state = 0x67e1a5398f8 prev_state =
> > (nil)
> > > in GLES2DecoderImpl::Initialize: context state = 0x67e1a8275f8
> > > 
> > > open a new page in the second tab:                                        
 
>  
> >  
> > >                                                                           
 
>  
> >  
> > >                          
> > > in GLES2DecoderImpl::Initialize: context state = 0x67e1acdcef8
> > > in ContextState::RestoreState: context state = 0x67e1a5398f8 prev_state =
> > (nil)
> > > 
> > > You can see "context state = 0x67e1a5398f8" call RestoreState with a null
> > > prev_state twice from above context state change list.
> > 
> > See piman's comment.  Calling glDisable in Decoder::Initialize() is exactly
> what
> > he suggested, we just need to do it in call situations where prev_state is
> null.
> > 
> > Leaving it as UNDEFINED is buggy - it could be enabled or disabled, which is
> > definitely the wrong thing - it just hasn't manifested yet.  We have to know
> at
> > each moment the state of FRAMEBUFFER_SRGB.
> 
> Leaving framebuffer_srgb_ as FRAMEBUFFER_SRGB_UNDEFINED is not set
> FRAMEBUFFER_SRGB flag as undefined. It just let next
> EnableDisableFramebufferSRGB() call do glEnable or glDisable. It's harmless to
> gl state. 
> 
> EnableDisableFramebufferSRGB() will set FRAMEBUFFER_SRGB as what we want. If
we
> don't use framebuffer_srgb_, it will set FRAMEBUFFER_SRGB flag every time. If
we
> keep updating framebuffer_srgb_, EnableDisableFramebufferSRGB() will not
enable
> FRAMEBUFFER_SRGB if it's already enabled or disable FRAMEBUFFER_SRGB if it's
> already disabled.

Here is what I think: in general we should leave FRAMEBUFFER_SRGB disabled,
unless we need it for a few limited commands to get the correct behaviors.

At the moment we enable/disable lazily, so in the situation we switch context
from when it's enabled, it stays enabled for a long time.  This is concerning
and potentially bug prone and very bad if the bugs are triggered - and it could
turn Chrome grey.

So what I propose is: whenever we switch context, if it's enabled or unclear (no
prev_state), we forcefully disable it immediately.

What do you think?

[qiankun, 2016-08-18 23:40:37]

On 2016/08/18 23:34:02, Zhenyao Mo wrote:
> On 2016/08/18 23:04:54, qiankun wrote:
> > On 2016/08/18 21:46:51, Zhenyao Mo wrote:
> > > On 2016/08/18 07:04:34, qiankun wrote:
> > > > On 2016/08/18 00:34:43, Zhenyao Mo wrote:
> > > > > On 2016/08/18 00:19:56, qiankun wrote:
> > > > > > I still cannot revert these changes. There are still some situations
> > where
> > > > > > context is restored from a null context but it isn't a context
> > > > initialization.
> > > > > > glDisable(FRAMEBUFFER_SRGB) in context initialization doesn't help
in
> > such
> > > > > > situation.
> > > > > 
> > > > > Really? Do you know what are these situations?
> > > > 
> > > > See the following call stack:
> > > > #0  (anonymous namespace)::(anonymous
> namespace)::ContextState::RestoreState
> > > > (this=0x315bc8263148, prev_state=0x0) at
> > > > ../../gpu/command_buffer/service/context_state.cc:499
> > > > #1  0x0000564e3f3344be in (anonymous namespace)::(anonymous
> > > > namespace)::GLES2DecoderImpl::RestoreState (this=0x315bc8263020,
> > > prev_state=0x0)
> > > > at ../../gpu/command_buffer/service/gles2_cmd_
> > > > decoder.cc:5393
> > > > #2  0x0000564e3f2fd084 in (anonymous
> > > > namespace)::GLStateRestorerImpl::RestoreState (this=0x315bc7aecb60,
> > > > prev_state=0x0) at
> > > ../../gpu/command_buffer/service/gl_state_restorer_impl.cc:30
> > > > #3  0x0000564e38353b2d in (anonymous
> > > namespace)::GLContext::MakeVirtuallyCurrent
> > > > (this=0x315bc821ca20, virtual_context=0x315bc7eba200,
> > surface=0x315bc79984a0)
> > > at
> > > > ../../ui/gl/gl_context.cc
> > > > :231
> > > > #4  0x0000564e3f2fc844 in (anonymous
> > namespace)::GLContextVirtual::MakeCurrent
> > > > (this=0x315bc7eba200, surface=0x315bc79984a0) at
> > > > ../../gpu/command_buffer/service/gl_context_virtual.cc:38
> > > > #5  0x0000564e3f32d25f in (anonymous namespace)::(anonymous
> > > > namespace)::GLES2DecoderImpl::MakeCurrent (this=0x315bc8263020) at
> > > > ../../gpu/command_buffer/service/gles2_cmd_decoder.cc:4090
> > > > #6  0x0000564e3e8703b0 in (anonymous
> > > > namespace)::GpuCommandBufferStub::MakeCurrent (this=0x315bc790ac60) at
> > > > ../../gpu/ipc/service/gpu_command_buffer_stub.cc:385
> > > > #7  0x0000564e3e86f336 in (anonymous
> > > > namespace)::GpuCommandBufferStub::OnMessageReceived
(this=0x315bc790ac60,
> > > > message=...) at ../../gpu/ipc/service/gpu_command_buffer_stub.cc:225
> > > > #8  0x0000564e3e84c65b in (anonymous
> namespace)::MessageRouter::RouteMessage
> > > > (this=0x315bc7878430, msg=...) at ../../ipc/message_router.cc:52
> > > > #9  0x0000564e3e8577a2 in (anonymous
> > > namespace)::GpuChannel::HandleMessageHelper
> > > > (this=0x315bc78783a0, msg=...) at
../../gpu/ipc/service/gpu_channel.cc:807
> > > > 
> > > > Here is the context state status change flow:
> > > > Open webgl-conformance-tests.html?version=2.0.0 in first tab:
> > > > in GLES2DecoderImpl::Initialize: context state = 0x67e196b27f8
> > > > in GLES2DecoderImpl::Initialize: context state = 0x67e19779cf8
> > > > in GLES2DecoderImpl::Initialize: context state = 0x67e19d9c0f8
> > > > in GLES2DecoderImpl::Initialize: context state = 0x67e19d9c7f8
> > > > in GLES2DecoderImpl::Initialize: context state = 0x67e1a536ef8
> > > > in GLES2DecoderImpl::Initialize: context state = 0x67e1a8267f8
> > > > in ContextState::RestoreState: context state = 0x67e1a8267f8 prev_state
=
> > > (nil)
> > > > in ContextState::RestoreState: context state = 0x67e196b27f8 prev_state
=
> > > (nil)
> > > > 
> > > > open a blank page in second tab:
> > > > in GLES2DecoderImpl::Initialize: context state = 0x67e1b0048f8
> > > > in GLES2DecoderImpl::Initialize: context state = 0x67e1adaf0f8
> > > > in GLES2DecoderImpl::Initialize: context state = 0x67e1a0301f8
> > > > 
> > > > open a page in second tab:
> > > > in GLES2DecoderImpl::Initialize: context state = 0x67e1a5398f8
> > > > in GLES2DecoderImpl::Initialize: context state = 0x67e1a03daf8
> > > > in ContextState::RestoreState: context state = 0x67e1a5398f8 prev_state
=
> > > (nil)
> > > > in GLES2DecoderImpl::Initialize: context state = 0x67e1a8275f8
> > > > 
> > > > open a new page in the second tab:                                      
 
>  
> >  
> > >  
> > > >                                                                         
 
>  
> >  
> > >  
> > > >                          
> > > > in GLES2DecoderImpl::Initialize: context state = 0x67e1acdcef8
> > > > in ContextState::RestoreState: context state = 0x67e1a5398f8 prev_state
=
> > > (nil)
> > > > 
> > > > You can see "context state = 0x67e1a5398f8" call RestoreState with a
null
> > > > prev_state twice from above context state change list.
> > > 
> > > See piman's comment.  Calling glDisable in Decoder::Initialize() is
exactly
> > what
> > > he suggested, we just need to do it in call situations where prev_state is
> > null.
> > > 
> > > Leaving it as UNDEFINED is buggy - it could be enabled or disabled, which
is
> > > definitely the wrong thing - it just hasn't manifested yet.  We have to
know
> > at
> > > each moment the state of FRAMEBUFFER_SRGB.
> > 
> > Leaving framebuffer_srgb_ as FRAMEBUFFER_SRGB_UNDEFINED is not set
> > FRAMEBUFFER_SRGB flag as undefined. It just let next
> > EnableDisableFramebufferSRGB() call do glEnable or glDisable. It's harmless
to
> > gl state. 
> > 
> > EnableDisableFramebufferSRGB() will set FRAMEBUFFER_SRGB as what we want. If
> we
> > don't use framebuffer_srgb_, it will set FRAMEBUFFER_SRGB flag every time.
If
> we
> > keep updating framebuffer_srgb_, EnableDisableFramebufferSRGB() will not
> enable
> > FRAMEBUFFER_SRGB if it's already enabled or disable FRAMEBUFFER_SRGB if it's
> > already disabled.
> 
> Here is what I think: in general we should leave FRAMEBUFFER_SRGB disabled,
> unless we need it for a few limited commands to get the correct behaviors.
> 
> At the moment we enable/disable lazily, so in the situation we switch context
> from when it's enabled, it stays enabled for a long time.  This is concerning
> and potentially bug prone and very bad if the bugs are triggered - and it
could
> turn Chrome grey.
> 
> So what I propose is: whenever we switch context, if it's enabled or unclear
(no
> prev_state), we forcefully disable it immediately.
> 
> What do you think?

This makes sense. That's what I did in patch set1. But it failed on Windows bot.
I will dig it further.

[qiankun, 2016-08-19 08:28:12]

Description was changed from

==========
Unset FRAMEBUFFER_SRGB when restoring from a null context state

In the following case, FRAMEBUFFER_SRGB state in driver will not be correct:
1. FRAMEBUFFER_SRGB is enabled for sRGB framebuffer. 
2. restore from a null context, framebuffer_srgb_ will be false after restoring.
But FRAMEBUFFER_SRGB will not be changed if we call
EnableDisableFramebufferSRGB(false)
to disable FRAMEBUFFER_SRGB, because the function will return early at 
"if (framebuffer_srgb_ == enable)".

This CL adds another state "FRAMEBUFFER_SRGB_UNDEFINED" to make sure
FRAMEBUFFER_SRGB
will be updated after resoring from a null context state. 

BUG=540900, 637801

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

==========
Disable FRAMEBUFFER_SRGB when restoring from a null context state

In the following case, FRAMEBUFFER_SRGB state in driver will not be correct:
1. FRAMEBUFFER_SRGB is enabled for sRGB framebuffer. 
2. restore from a null context, framebuffer_srgb_ will be false after restoring.
But FRAMEBUFFER_SRGB will not be changed if we call
EnableDisableFramebufferSRGB(false)
to disable FRAMEBUFFER_SRGB, because the function will return early at 
"if (framebuffer_srgb_ == enable)".

This CL disable FRAMEBUFFER_SRGB when restoring from a null context state.

BUG=540900, 637801

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

[qiankun, 2016-08-19 10:20:03]

All bots passed now. Thanks very much for carefully review and discussion zmo@.
Please take another look.

[Zhenyao Mo, 2016-08-19 17:16:59]

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

https://codereview.chromium.org/2246823002/diff/80001/gpu/command_buffer/serv...
gpu/command_buffer/service/context_state.cc:500: glDisable(GL_FRAMEBUFFER_SRGB);
We need to set framebuffer_srgb_ to false here.

[qiankun, 2016-08-19 22:03:15]

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

https://codereview.chromium.org/2246823002/diff/80001/gpu/command_buffer/serv...
gpu/command_buffer/service/context_state.cc:500: glDisable(GL_FRAMEBUFFER_SRGB);
On 2016/08/19 17:16:59, Zhenyao Mo wrote:
> We need to set framebuffer_srgb_ to false here.

Done.

[qiankun, 2016-08-19 22:03:34]

The CQ bit was checked by qiankun.miao@intel.com to run a CQ dry run

[Zhenyao Mo, 2016-08-19 22:03:42]

lgtm

[commit-bot: I haz the power, 2016-08-19 22:04:28]

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

[piman, 2016-08-19 22:34:57]

lgtm

[commit-bot: I haz the power, 2016-08-19 23:54:20]

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

[commit-bot: I haz the power, 2016-08-19 23:54:21]

Dry run: This issue passed the CQ dry run.

[qiankun, 2016-08-19 23:58:15]

The CQ bit was checked by qiankun.miao@intel.com

[commit-bot: I haz the power, 2016-08-19 23:58:59]

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-08-20 00:05:03]

Description was changed from

==========
Disable FRAMEBUFFER_SRGB when restoring from a null context state

In the following case, FRAMEBUFFER_SRGB state in driver will not be correct:
1. FRAMEBUFFER_SRGB is enabled for sRGB framebuffer. 
2. restore from a null context, framebuffer_srgb_ will be false after restoring.
But FRAMEBUFFER_SRGB will not be changed if we call
EnableDisableFramebufferSRGB(false)
to disable FRAMEBUFFER_SRGB, because the function will return early at 
"if (framebuffer_srgb_ == enable)".

This CL disable FRAMEBUFFER_SRGB when restoring from a null context state.

BUG=540900, 637801

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

==========
Disable FRAMEBUFFER_SRGB when restoring from a null context state

In the following case, FRAMEBUFFER_SRGB state in driver will not be correct:
1. FRAMEBUFFER_SRGB is enabled for sRGB framebuffer. 
2. restore from a null context, framebuffer_srgb_ will be false after restoring.
But FRAMEBUFFER_SRGB will not be changed if we call
EnableDisableFramebufferSRGB(false)
to disable FRAMEBUFFER_SRGB, because the function will return early at 
"if (framebuffer_srgb_ == enable)".

This CL disable FRAMEBUFFER_SRGB when restoring from a null context state.

BUG=540900, 637801

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

[commit-bot: I haz the power, 2016-08-20 00:05:04]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-08-20 00:07:36]

Description was changed from

==========
Disable FRAMEBUFFER_SRGB when restoring from a null context state

In the following case, FRAMEBUFFER_SRGB state in driver will not be correct:
1. FRAMEBUFFER_SRGB is enabled for sRGB framebuffer. 
2. restore from a null context, framebuffer_srgb_ will be false after restoring.
But FRAMEBUFFER_SRGB will not be changed if we call
EnableDisableFramebufferSRGB(false)
to disable FRAMEBUFFER_SRGB, because the function will return early at 
"if (framebuffer_srgb_ == enable)".

This CL disable FRAMEBUFFER_SRGB when restoring from a null context state.

BUG=540900, 637801

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

==========
Disable FRAMEBUFFER_SRGB when restoring from a null context state

In the following case, FRAMEBUFFER_SRGB state in driver will not be correct:
1. FRAMEBUFFER_SRGB is enabled for sRGB framebuffer.
2. restore from a null context, framebuffer_srgb_ will be false after restoring.
But FRAMEBUFFER_SRGB will not be changed if we call
EnableDisableFramebufferSRGB(false)
to disable FRAMEBUFFER_SRGB, because the function will return early at
"if (framebuffer_srgb_ == enable)".

This CL disable FRAMEBUFFER_SRGB when restoring from a null context state.

BUG=540900, 637801

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/a8fe152031cea3017f7dd41990d7e46d49a2ce51
Cr-Commit-Position: refs/heads/master@{#413291}
==========

[commit-bot: I haz the power, 2016-08-20 00:07:44]

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

[yunchao, 2016-08-24 13:46:03]

On 2016/08/18 23:34:02, Zhenyao Mo wrote:
> On 2016/08/18 23:04:54, qiankun wrote:
> > On 2016/08/18 21:46:51, Zhenyao Mo wrote:
> > > On 2016/08/18 07:04:34, qiankun wrote:
> > > > On 2016/08/18 00:34:43, Zhenyao Mo wrote:
> > > > > On 2016/08/18 00:19:56, qiankun wrote:
> > > > > > I still cannot revert these changes. There are still some situations
> > where
> > > > > > context is restored from a null context but it isn't a context
> > > > initialization.
> > > > > > glDisable(FRAMEBUFFER_SRGB) in context initialization doesn't help
in
> > such
> > > > > > situation.
> > > > > 
> > > > > Really? Do you know what are these situations?
> > > > 
> > > > See the following call stack:
> > > > #0  (anonymous namespace)::(anonymous
> namespace)::ContextState::RestoreState
> > > > (this=0x315bc8263148, prev_state=0x0) at
> > > > ../../gpu/command_buffer/service/context_state.cc:499
> > > > #1  0x0000564e3f3344be in (anonymous namespace)::(anonymous
> > > > namespace)::GLES2DecoderImpl::RestoreState (this=0x315bc8263020,
> > > prev_state=0x0)
> > > > at ../../gpu/command_buffer/service/gles2_cmd_
> > > > decoder.cc:5393
> > > > #2  0x0000564e3f2fd084 in (anonymous
> > > > namespace)::GLStateRestorerImpl::RestoreState (this=0x315bc7aecb60,
> > > > prev_state=0x0) at
> > > ../../gpu/command_buffer/service/gl_state_restorer_impl.cc:30
> > > > #3  0x0000564e38353b2d in (anonymous
> > > namespace)::GLContext::MakeVirtuallyCurrent
> > > > (this=0x315bc821ca20, virtual_context=0x315bc7eba200,
> > surface=0x315bc79984a0)
> > > at
> > > > ../../ui/gl/gl_context.cc
> > > > :231
> > > > #4  0x0000564e3f2fc844 in (anonymous
> > namespace)::GLContextVirtual::MakeCurrent
> > > > (this=0x315bc7eba200, surface=0x315bc79984a0) at
> > > > ../../gpu/command_buffer/service/gl_context_virtual.cc:38
> > > > #5  0x0000564e3f32d25f in (anonymous namespace)::(anonymous
> > > > namespace)::GLES2DecoderImpl::MakeCurrent (this=0x315bc8263020) at
> > > > ../../gpu/command_buffer/service/gles2_cmd_decoder.cc:4090
> > > > #6  0x0000564e3e8703b0 in (anonymous
> > > > namespace)::GpuCommandBufferStub::MakeCurrent (this=0x315bc790ac60) at
> > > > ../../gpu/ipc/service/gpu_command_buffer_stub.cc:385
> > > > #7  0x0000564e3e86f336 in (anonymous
> > > > namespace)::GpuCommandBufferStub::OnMessageReceived
(this=0x315bc790ac60,
> > > > message=...) at ../../gpu/ipc/service/gpu_command_buffer_stub.cc:225
> > > > #8  0x0000564e3e84c65b in (anonymous
> namespace)::MessageRouter::RouteMessage
> > > > (this=0x315bc7878430, msg=...) at ../../ipc/message_router.cc:52
> > > > #9  0x0000564e3e8577a2 in (anonymous
> > > namespace)::GpuChannel::HandleMessageHelper
> > > > (this=0x315bc78783a0, msg=...) at
../../gpu/ipc/service/gpu_channel.cc:807
> > > > 
> > > > Here is the context state status change flow:
> > > > Open webgl-conformance-tests.html?version=2.0.0 in first tab:
> > > > in GLES2DecoderImpl::Initialize: context state = 0x67e196b27f8
> > > > in GLES2DecoderImpl::Initialize: context state = 0x67e19779cf8
> > > > in GLES2DecoderImpl::Initialize: context state = 0x67e19d9c0f8
> > > > in GLES2DecoderImpl::Initialize: context state = 0x67e19d9c7f8
> > > > in GLES2DecoderImpl::Initialize: context state = 0x67e1a536ef8
> > > > in GLES2DecoderImpl::Initialize: context state = 0x67e1a8267f8
> > > > in ContextState::RestoreState: context state = 0x67e1a8267f8 prev_state
=
> > > (nil)
> > > > in ContextState::RestoreState: context state = 0x67e196b27f8 prev_state
=
> > > (nil)
> > > > 
> > > > open a blank page in second tab:
> > > > in GLES2DecoderImpl::Initialize: context state = 0x67e1b0048f8
> > > > in GLES2DecoderImpl::Initialize: context state = 0x67e1adaf0f8
> > > > in GLES2DecoderImpl::Initialize: context state = 0x67e1a0301f8
> > > > 
> > > > open a page in second tab:
> > > > in GLES2DecoderImpl::Initialize: context state = 0x67e1a5398f8
> > > > in GLES2DecoderImpl::Initialize: context state = 0x67e1a03daf8
> > > > in ContextState::RestoreState: context state = 0x67e1a5398f8 prev_state
=
> > > (nil)
> > > > in GLES2DecoderImpl::Initialize: context state = 0x67e1a8275f8
> > > > 
> > > > open a new page in the second tab:                                      
 
>  
> >  
> > >  
> > > >                                                                         
 
>  
> >  
> > >  
> > > >                          
> > > > in GLES2DecoderImpl::Initialize: context state = 0x67e1acdcef8
> > > > in ContextState::RestoreState: context state = 0x67e1a5398f8 prev_state
=
> > > (nil)
> > > > 
> > > > You can see "context state = 0x67e1a5398f8" call RestoreState with a
null
> > > > prev_state twice from above context state change list.
> > > 
> > > See piman's comment.  Calling glDisable in Decoder::Initialize() is
exactly
> > what
> > > he suggested, we just need to do it in call situations where prev_state is
> > null.
> > > 
> > > Leaving it as UNDEFINED is buggy - it could be enabled or disabled, which
is
> > > definitely the wrong thing - it just hasn't manifested yet.  We have to
know
> > at
> > > each moment the state of FRAMEBUFFER_SRGB.
> > 
> > Leaving framebuffer_srgb_ as FRAMEBUFFER_SRGB_UNDEFINED is not set
> > FRAMEBUFFER_SRGB flag as undefined. It just let next
> > EnableDisableFramebufferSRGB() call do glEnable or glDisable. It's harmless
to
> > gl state. 
> > 
> > EnableDisableFramebufferSRGB() will set FRAMEBUFFER_SRGB as what we want. If
> we
> > don't use framebuffer_srgb_, it will set FRAMEBUFFER_SRGB flag every time.
If
> we
> > keep updating framebuffer_srgb_, EnableDisableFramebufferSRGB() will not
> enable
> > FRAMEBUFFER_SRGB if it's already enabled or disable FRAMEBUFFER_SRGB if it's
> > already disabled.
> 
> Here is what I think: in general we should leave FRAMEBUFFER_SRGB disabled,
> unless we need it for a few limited commands to get the correct behaviors.
> 
> At the moment we enable/disable lazily, so in the situation we switch context
> from when it's enabled, it stays enabled for a long time.  This is concerning
> and potentially bug prone and very bad if the bugs are triggered - and it
could
> turn Chrome grey.
> 
> So what I propose is: whenever we switch context, if it's enabled or unclear
(no
> prev_state), we forcefully disable it immediately.
> 
> What do you think?

Zhenyao and all, AFAICT, it is not true that the srgb enable state will be hold
for a long time. You know, I am looking at the sRGB related issue(emulate sRGB
for core profile less than OGL 4.4 for BlitFramebuffer), I found that we would
check the srgb status for every draw call (BlitFramebuffer,
drawArrays/drawElements and their variants, clear, clearBuffer, etc) by call
into CheckBoundDrawFramebufferValid or CheckBoundFramebufferValid. 
So no matter what we set to srgb enable/disable status for context init/restore,
it would be correct in theory: If the srgb enable/disable state is wrong, it
will be corrected before drawing. So I think there should be some other mistake
in the code. Today, I found that there is a bug in Chromium: if the fbo have
srgb color buffers(attachments), but they are not readBuffer/drawBuffers,
chromium will enable srgb wrongly... 
Please see this change: https://codereview.chromium.org/2268503002

[Zhenyao Mo, 2016-08-25 00:29:09]

On 2016/08/24 13:46:03, yunchao wrote:
> On 2016/08/18 23:34:02, Zhenyao Mo wrote:
> > On 2016/08/18 23:04:54, qiankun wrote:
> > > On 2016/08/18 21:46:51, Zhenyao Mo wrote:
> > > > On 2016/08/18 07:04:34, qiankun wrote:
> > > > > On 2016/08/18 00:34:43, Zhenyao Mo wrote:
> > > > > > On 2016/08/18 00:19:56, qiankun wrote:
> > > > > > > I still cannot revert these changes. There are still some
situations
> > > where
> > > > > > > context is restored from a null context but it isn't a context
> > > > > initialization.
> > > > > > > glDisable(FRAMEBUFFER_SRGB) in context initialization doesn't help
> in
> > > such
> > > > > > > situation.
> > > > > > 
> > > > > > Really? Do you know what are these situations?
> > > > > 
> > > > > See the following call stack:
> > > > > #0  (anonymous namespace)::(anonymous
> > namespace)::ContextState::RestoreState
> > > > > (this=0x315bc8263148, prev_state=0x0) at
> > > > > ../../gpu/command_buffer/service/context_state.cc:499
> > > > > #1  0x0000564e3f3344be in (anonymous namespace)::(anonymous
> > > > > namespace)::GLES2DecoderImpl::RestoreState (this=0x315bc8263020,
> > > > prev_state=0x0)
> > > > > at ../../gpu/command_buffer/service/gles2_cmd_
> > > > > decoder.cc:5393
> > > > > #2  0x0000564e3f2fd084 in (anonymous
> > > > > namespace)::GLStateRestorerImpl::RestoreState (this=0x315bc7aecb60,
> > > > > prev_state=0x0) at
> > > > ../../gpu/command_buffer/service/gl_state_restorer_impl.cc:30
> > > > > #3  0x0000564e38353b2d in (anonymous
> > > > namespace)::GLContext::MakeVirtuallyCurrent
> > > > > (this=0x315bc821ca20, virtual_context=0x315bc7eba200,
> > > surface=0x315bc79984a0)
> > > > at
> > > > > ../../ui/gl/gl_context.cc
> > > > > :231
> > > > > #4  0x0000564e3f2fc844 in (anonymous
> > > namespace)::GLContextVirtual::MakeCurrent
> > > > > (this=0x315bc7eba200, surface=0x315bc79984a0) at
> > > > > ../../gpu/command_buffer/service/gl_context_virtual.cc:38
> > > > > #5  0x0000564e3f32d25f in (anonymous namespace)::(anonymous
> > > > > namespace)::GLES2DecoderImpl::MakeCurrent (this=0x315bc8263020) at
> > > > > ../../gpu/command_buffer/service/gles2_cmd_decoder.cc:4090
> > > > > #6  0x0000564e3e8703b0 in (anonymous
> > > > > namespace)::GpuCommandBufferStub::MakeCurrent (this=0x315bc790ac60) at
> > > > > ../../gpu/ipc/service/gpu_command_buffer_stub.cc:385
> > > > > #7  0x0000564e3e86f336 in (anonymous
> > > > > namespace)::GpuCommandBufferStub::OnMessageReceived
> (this=0x315bc790ac60,
> > > > > message=...) at ../../gpu/ipc/service/gpu_command_buffer_stub.cc:225
> > > > > #8  0x0000564e3e84c65b in (anonymous
> > namespace)::MessageRouter::RouteMessage
> > > > > (this=0x315bc7878430, msg=...) at ../../ipc/message_router.cc:52
> > > > > #9  0x0000564e3e8577a2 in (anonymous
> > > > namespace)::GpuChannel::HandleMessageHelper
> > > > > (this=0x315bc78783a0, msg=...) at
> ../../gpu/ipc/service/gpu_channel.cc:807
> > > > > 
> > > > > Here is the context state status change flow:
> > > > > Open webgl-conformance-tests.html?version=2.0.0 in first tab:
> > > > > in GLES2DecoderImpl::Initialize: context state = 0x67e196b27f8
> > > > > in GLES2DecoderImpl::Initialize: context state = 0x67e19779cf8
> > > > > in GLES2DecoderImpl::Initialize: context state = 0x67e19d9c0f8
> > > > > in GLES2DecoderImpl::Initialize: context state = 0x67e19d9c7f8
> > > > > in GLES2DecoderImpl::Initialize: context state = 0x67e1a536ef8
> > > > > in GLES2DecoderImpl::Initialize: context state = 0x67e1a8267f8
> > > > > in ContextState::RestoreState: context state = 0x67e1a8267f8
prev_state
> =
> > > > (nil)
> > > > > in ContextState::RestoreState: context state = 0x67e196b27f8
prev_state
> =
> > > > (nil)
> > > > > 
> > > > > open a blank page in second tab:
> > > > > in GLES2DecoderImpl::Initialize: context state = 0x67e1b0048f8
> > > > > in GLES2DecoderImpl::Initialize: context state = 0x67e1adaf0f8
> > > > > in GLES2DecoderImpl::Initialize: context state = 0x67e1a0301f8
> > > > > 
> > > > > open a page in second tab:
> > > > > in GLES2DecoderImpl::Initialize: context state = 0x67e1a5398f8
> > > > > in GLES2DecoderImpl::Initialize: context state = 0x67e1a03daf8
> > > > > in ContextState::RestoreState: context state = 0x67e1a5398f8
prev_state
> =
> > > > (nil)
> > > > > in GLES2DecoderImpl::Initialize: context state = 0x67e1a8275f8
> > > > > 
> > > > > open a new page in the second tab:                                    
 
>  
> >  
> > >  
> > > >  
> > > > >                                                                       
 
>  
> >  
> > >  
> > > >  
> > > > >                          
> > > > > in GLES2DecoderImpl::Initialize: context state = 0x67e1acdcef8
> > > > > in ContextState::RestoreState: context state = 0x67e1a5398f8
prev_state
> =
> > > > (nil)
> > > > > 
> > > > > You can see "context state = 0x67e1a5398f8" call RestoreState with a
> null
> > > > > prev_state twice from above context state change list.
> > > > 
> > > > See piman's comment.  Calling glDisable in Decoder::Initialize() is
> exactly
> > > what
> > > > he suggested, we just need to do it in call situations where prev_state
is
> > > null.
> > > > 
> > > > Leaving it as UNDEFINED is buggy - it could be enabled or disabled,
which
> is
> > > > definitely the wrong thing - it just hasn't manifested yet.  We have to
> know
> > > at
> > > > each moment the state of FRAMEBUFFER_SRGB.
> > > 
> > > Leaving framebuffer_srgb_ as FRAMEBUFFER_SRGB_UNDEFINED is not set
> > > FRAMEBUFFER_SRGB flag as undefined. It just let next
> > > EnableDisableFramebufferSRGB() call do glEnable or glDisable. It's
harmless
> to
> > > gl state. 
> > > 
> > > EnableDisableFramebufferSRGB() will set FRAMEBUFFER_SRGB as what we want.
If
> > we
> > > don't use framebuffer_srgb_, it will set FRAMEBUFFER_SRGB flag every time.
> If
> > we
> > > keep updating framebuffer_srgb_, EnableDisableFramebufferSRGB() will not
> > enable
> > > FRAMEBUFFER_SRGB if it's already enabled or disable FRAMEBUFFER_SRGB if
it's
> > > already disabled.
> > 
> > Here is what I think: in general we should leave FRAMEBUFFER_SRGB disabled,
> > unless we need it for a few limited commands to get the correct behaviors.
> > 
> > At the moment we enable/disable lazily, so in the situation we switch
context
> > from when it's enabled, it stays enabled for a long time.  This is
concerning
> > and potentially bug prone and very bad if the bugs are triggered - and it
> could
> > turn Chrome grey.
> > 
> > So what I propose is: whenever we switch context, if it's enabled or unclear
> (no
> > prev_state), we forcefully disable it immediately.
> > 
> > What do you think?
> 
> Zhenyao and all, AFAICT, it is not true that the srgb enable state will be
hold
> for a long time. You know, I am looking at the sRGB related issue(emulate sRGB
> for core profile less than OGL 4.4 for BlitFramebuffer), I found that we would
> check the srgb status for every draw call (BlitFramebuffer,
> drawArrays/drawElements and their variants, clear, clearBuffer, etc) by call
> into CheckBoundDrawFramebufferValid or CheckBoundFramebufferValid. 
> So no matter what we set to srgb enable/disable status for context
init/restore,
> it would be correct in theory: If the srgb enable/disable state is wrong, it
> will be corrected before drawing. So I think there should be some other
mistake
> in the code. Today, I found that there is a bug in Chromium: if the fbo have
> srgb color buffers(attachments), but they are not readBuffer/drawBuffers,
> chromium will enable srgb wrongly... 
> Please see this change: https://codereview.chromium.org/2268503002

Even if we check every draw call, there are a lot of GL calls in between, so I
wouldn't say it's "not" a long time.

As for readBuffer/drawBuffers, what we found is as far as there is an sRGB
attachment in the fbo, whether it is active or not, turning on FRAMEBUFFER_SRGB
the driver behaves correctly.  That's why we didn't care in the first place.