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

gpu/command_buffer/service/gles2_cmd_decoder.cc

Index: gpu/command_buffer/service/gles2_cmd_decoder.cc
diff --git a/gpu/command_buffer/service/gles2_cmd_decoder.cc b/gpu/command_buffer/service/gles2_cmd_decoder.cc
index fe989b84063e61c99f06e0d44e9079a89a4d3450..7c9d6a2ef0039691f064254a14a5d6f0c2241009 100644
--- a/gpu/command_buffer/service/gles2_cmd_decoder.cc
+++ b/gpu/command_buffer/service/gles2_cmd_decoder.cc
@@ -1359,6 +1359,10 @@ class GLES2DecoderImpl : public GLES2Decoder, public ErrorStateClient {
   // framebuffer at the same time.
   bool CheckBoundFramebufferValid(const char* func_name);
 
+  // If one or more drawBuffers have srgb color format, enable framebuffer srgb.
+  // Otherwise, disable framebuffer srgb.
+  void EnableDisableFramebufferSRGBForDrawBuffers();
+
   // Checks if the current program exists and is valid. If not generates the
   // appropriate GL error.  Returns true if the current program is in a usable
   // state.
@@ -4184,18 +4188,6 @@ bool GLES2DecoderImpl::CheckBoundDrawFramebufferValid(const char* func_name) {
       framebuffer, target, GL_INVALID_FRAMEBUFFER_OPERATION, func_name);
   if (valid && !features().chromium_framebuffer_multisample)
     OnUseFramebuffer();
-  if (valid && feature_info_->feature_flags().desktop_srgb_support) {
-    // If framebuffer contains sRGB images, then enable FRAMEBUFFER_SRGB.
-    // Otherwise, disable FRAMEBUFFER_SRGB. Assume default fbo does not have
-    // sRGB image.
-    // In theory, we can just leave FRAMEBUFFER_SRGB on. However, many drivers
-    // behave incorrectly when all images are linear encoding, they still apply
-    // the sRGB conversion, but when at least one image is sRGB, then they
-    // behave correctly.
-    bool enable_framebuffer_srgb =
-        framebuffer && framebuffer->HasSRGBAttachments();

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

This is not correct. If the fbo has srgb color buffer, but they are not
drawBuffers, we should not enable framebuffer_srgb.

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

What we found is whether the sRGB image is currently active doesn't matter. 
However, if you want to fine-tuning it, that's OK. But why moving this out of
here and put it in all calls?  To me, putting it here is much cleaner.

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

That's true. It can draw correctly if there is sRGB image, no matter the sRGB
image is currently active or not. 
moving this outside of this function is because of fine tuning. Suppose that the
drawBuffer (clear or draw, etc) is depth buffer, it is not necessary to do this
check at all. Because the sRGB image is definitely inactive, we don't need to
check/change the framebuffer srgb state. 

-    state_.EnableDisableFramebufferSRGB(enable_framebuffer_srgb);
-  }
   return valid;
 }
 
@@ -4221,15 +4213,22 @@ bool GLES2DecoderImpl::CheckBoundFramebufferValid(const char* func_name) {
   Framebuffer* read_framebuffer = GetFramebufferInfoForTarget(target);
   valid = valid && CheckFramebufferValid(
       read_framebuffer, target, GL_INVALID_FRAMEBUFFER_OPERATION, func_name);
+  return valid;
+}
 
-  if (valid && feature_info_->feature_flags().desktop_srgb_support) {
-    bool enable_framebuffer_srgb =
-        (draw_framebuffer && draw_framebuffer->HasSRGBAttachments()) ||
-        (read_framebuffer && read_framebuffer->HasSRGBAttachments());

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

The same reason. If the fbo has srgb color buffer, but they are not
drawBuffers/readBuffer, we should not enable framebuffer_srgb.

-    state_.EnableDisableFramebufferSRGB(enable_framebuffer_srgb);
+void GLES2DecoderImpl::EnableDisableFramebufferSRGBForDrawBuffers() {
+  bool draw_buffers_has_srgb = false;
+  for (uint32_t ii = 0; ii < group_->max_draw_buffers(); ++ii) {
+    GLenum dst_format = GetBoundColorDrawBufferInternalFormat(
+        static_cast<GLint>(ii));
+    if (dst_format == 0)
+      continue;
+    if (GetColorEncodingFromInternalFormat(dst_format) == GL_SRGB) {
+      draw_buffers_has_srgb = true;
+      break;
+    }
   }
-
-  return valid;
+  state_.EnableDisableFramebufferSRGB(draw_buffers_has_srgb);
 }
 
 GLint GLES2DecoderImpl::GetColorEncodingFromInternalFormat(
@@ -6914,6 +6913,9 @@ error::Error GLES2DecoderImpl::DoClear(GLbitfield mask) {
             "can't be called on integer buffers");
         return error::kNoError;
       }
+      if (feature_info_->feature_flags().desktop_srgb_support) {
+        EnableDisableFramebufferSRGBForDrawBuffers();
+      }
     }
     glClear(mask);
   }
@@ -6940,6 +6942,11 @@ void GLES2DecoderImpl::DoClearBufferiv(
           "can only be called on signed integer buffers");
       return;
     }
+    if (feature_info_->feature_flags().desktop_srgb_support) {
+      bool draw_buffer_has_srgb =
+          GetColorEncodingFromInternalFormat(internal_format) == GL_SRGB;
+      state_.EnableDisableFramebufferSRGB(draw_buffer_has_srgb);
+    }
   } else {
     DCHECK(buffer == GL_STENCIL);
     if (drawbuffer != 0) {
@@ -6973,6 +6980,11 @@ void GLES2DecoderImpl::DoClearBufferuiv(
         "can only be called on unsigned integer buffers");
     return;
   }
+  if (feature_info_->feature_flags().desktop_srgb_support) {
+    bool draw_buffer_has_srgb =
+        GetColorEncodingFromInternalFormat(internal_format) == GL_SRGB;
+    state_.EnableDisableFramebufferSRGB(draw_buffer_has_srgb);
+  }
   MarkDrawBufferAsCleared(buffer, drawbuffer);
   glClearBufferuiv(buffer, drawbuffer, value);
 }
@@ -6997,6 +7009,11 @@ void GLES2DecoderImpl::DoClearBufferfv(
           "can only be called on float buffers");
       return;
     }
+    if (feature_info_->feature_flags().desktop_srgb_support) {
+      bool draw_buffer_has_srgb =
+          GetColorEncodingFromInternalFormat(internal_format) == GL_SRGB;
+      state_.EnableDisableFramebufferSRGB(draw_buffer_has_srgb);
+    }
   } else {
     DCHECK(buffer == GL_DEPTH);
     if (drawbuffer != 0) {
@@ -7511,6 +7528,9 @@ void GLES2DecoderImpl::DoBlitFramebufferCHROMIUM(
   GLenum src_format = GetBoundReadFrameBufferInternalFormat();
   GLenum src_type = GetBoundReadFrameBufferTextureType();
 
+  bool read_buffer_has_srgb =
+      GetColorEncodingFromInternalFormat(src_format) == GL_SRGB;
+  bool draw_buffers_has_srgb = false;
   if ((mask & GL_COLOR_BUFFER_BIT) != 0) {
     bool is_src_signed_int = GLES2Util::IsSignedIntegerFormat(src_format);
     bool is_src_unsigned_int = GLES2Util::IsUnsignedIntegerFormat(src_format);
@@ -7530,6 +7550,8 @@ void GLES2DecoderImpl::DoBlitFramebufferCHROMIUM(
       GLenum dst_type = GetBoundColorDrawBufferType(static_cast<GLint>(ii));
       if (dst_format == 0)
         continue;
+      if (GetColorEncodingFromInternalFormat(dst_format) == GL_SRGB)
+        draw_buffers_has_srgb = true;
       if (read_buffer_samples > 0 &&
           (src_sized_format !=
            GLES2Util::ConvertToSizedFormat(dst_format, dst_type))) {
@@ -7566,6 +7588,11 @@ void GLES2DecoderImpl::DoBlitFramebufferCHROMIUM(
     }
   }
 
+  if (feature_info_->feature_flags().desktop_srgb_support) {
+    bool enable_srgb = read_buffer_has_srgb || draw_buffers_has_srgb;
+    state_.EnableDisableFramebufferSRGB(enable_srgb);
+  }
+
   state_.SetDeviceCapabilityState(GL_SCISSOR_TEST, false);
   BlitFramebufferHelper(
       srcX0, srcY0, srcX1, srcY1, dstX0, dstY0, dstX1, dstY1, mask, filter);
@@ -9194,6 +9221,11 @@ error::Error GLES2DecoderImpl::DoDrawArrays(
   if (!CheckBoundDrawFramebufferValid(function_name)) {
     return error::kNoError;
   }
+
+  if (feature_info_->feature_flags().desktop_srgb_support) {
+    EnableDisableFramebufferSRGBForDrawBuffers();
+  }
+
   // We have to check this here because the prototype for glDrawArrays
   // is GLint not GLsizei.
   if (first < 0) {
@@ -9335,6 +9367,10 @@ error::Error GLES2DecoderImpl::DoDrawElements(const char* function_name,
     return error::kNoError;
   }
 
+  if (feature_info_->feature_flags().desktop_srgb_support) {
+    EnableDisableFramebufferSRGBForDrawBuffers();
+  }
+
   if (state_.bound_transform_feedback.get() &&
       state_.bound_transform_feedback->active() &&
       !state_.bound_transform_feedback->paused()) {