[Command Buffer] enable/disable FRAMEBUFFER_SRGB only when sRGB image is active

gpu/command_buffer/service/context_state.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "gpu/command_buffer/service/context_state.h"
 
 #include <stddef.h>
 
 #include <cmath>
 
@@ skipping 477 matching lines @@
 
 void ContextState::RestoreState(const ContextState* prev_state) {
   RestoreAllTextureUnitBindings(prev_state);
   RestoreVertexAttribs();
   RestoreBufferBindings();
   RestoreRenderbufferBindings();
   RestoreProgramSettings(prev_state, true);
   RestoreIndexedUniformBufferBindings(prev_state);
   RestoreGlobalState(prev_state);
 
-  if (!prev_state) {
-    if (feature_info_->feature_flags().desktop_srgb_support) {
-      framebuffer_srgb_ = false;
-      glDisable(GL_FRAMEBUFFER_SRGB);
-    }
-  } else if (framebuffer_srgb_ != prev_state->framebuffer_srgb_) {

[yunchao, 2016/08/24 14:08:51]

It is OK to keep Qiankun's code here. I just want to say the framebuffer_srgb
status is not context-aware at all. The only thing we need to make sure is: keep
the flag framebuffer_srgb_ to be consistent with what is really should be
(glDisable or glEnable FRAMEBUFFER_SRGB) during context init/restore.  

[qiankun, 2016/08/24 16:21:00]

Agree with you.

+  if (prev_state && framebuffer_srgb_ != prev_state->framebuffer_srgb_) {
     // FRAMEBUFFER_SRGB will be restored lazily at render time.

[qiankun, 2016/08/24 16:22:13]

This comment is wrong in your current implementation.

[yunchao, 2016/08/25 16:29:51]

That's true.

     framebuffer_srgb_ = prev_state->framebuffer_srgb_;
   }
+  if (feature_info_->feature_flags().desktop_srgb_support) {
+    EnableDisable(GL_FRAMEBUFFER_SRGB, framebuffer_srgb_);

[Zhenyao Mo, 2016/08/25 00:39:37]

That's what we try to avoid.  If we use virtual contexts, then every frame we
switching contexts multiple times, at least between WebGL and compositor. The
original design is to avoid enable/disable FRAMEBUFFER_SRGB per frame, but now
it is per frame.

[yunchao, 2016/08/25 16:29:51]

You are correct. The original code is better.

+  }
 }
 
 ErrorState* ContextState::GetErrorState() {
   return error_state_.get();
 }
 
 void ContextState::EnableDisable(GLenum pname, bool enable) const {
   if (pname == GL_PRIMITIVE_RESTART_FIXED_INDEX &&
       feature_info_->feature_flags().emulate_primitive_restart_fixed_index) {
     // GLES2DecoderImpl::DoDrawElements can handle this situation
@@ skipping 190 matching lines @@
   UpdateUnpackParameters();
 }
 
 // Include the auto-generated part of this file. We split this because it means
 // we can easily edit the non-auto generated parts right here in this file
 // instead of having to edit some template or the code generator.
 #include "gpu/command_buffer/service/context_state_impl_autogen.h"
 
 }  // namespace gles2
 }  // namespace gpu