[Command Buffer] emulate SRGB color format for BlitFramebuffer in OpenGL

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 86074df50854075905257ab394e55fb06e0683f7..472ecdc70a7de2cd5518ab897a9a6e863c8a4a50 100644
--- a/gpu/command_buffer/service/gles2_cmd_decoder.cc
+++ b/gpu/command_buffer/service/gles2_cmd_decoder.cc
@@ -44,6 +44,7 @@
 #include "gpu/command_buffer/service/gles2_cmd_copy_tex_image.h"
 #include "gpu/command_buffer/service/gles2_cmd_copy_texture_chromium.h"
 #include "gpu/command_buffer/service/gles2_cmd_decoder_passthrough.h"
+#include "gpu/command_buffer/service/gles2_cmd_srgb_converter.h"
 #include "gpu/command_buffer/service/gles2_cmd_validation.h"
 #include "gpu/command_buffer/service/gpu_preferences.h"
 #include "gpu/command_buffer/service/gpu_state_tracer.h"
@@ -813,10 +814,14 @@ class GLES2DecoderImpl : public GLES2Decoder, public ErrorStateClient {
   // or regular back buffer).
   gfx::Size GetBoundReadFramebufferSize();
 
-  // Get the service side ID for the bound read frame buffer.
+  // Get the service side ID for the bound read framebuffer.
   // If it's back buffer, 0 is returned.
   GLuint GetBoundReadFramebufferServiceId();
 
+  // Get the service side ID for the bound draw framebuffer.
+  // If it's back buffer, 0 is returned.
+  GLuint GetBoundDrawFramebufferServiceId();
+
   // Get the format/type of the currently bound frame buffer (either FBO or
   // regular back buffer).
   // If the color image is a renderbuffer, returns 0 for type.
@@ -2060,6 +2065,7 @@ class GLES2DecoderImpl : public GLES2Decoder, public ErrorStateClient {
 
   bool InitializeCopyTexImageBlitter(const char* function_name);
   bool InitializeCopyTextureCHROMIUM(const char* function_name);
+  bool InitializeSRGBConverter(const char* function_name);
   // Generate a member function prototype for each command in an automated and
   // typesafe way.
 #define GLES2_CMD_OP(name) \
@@ -2252,6 +2258,7 @@ class GLES2DecoderImpl : public GLES2Decoder, public ErrorStateClient {
       apply_framebuffer_attachment_cmaa_intel_;
   std::unique_ptr<CopyTexImageResourceManager> copy_tex_image_blit_;
   std::unique_ptr<CopyTextureCHROMIUMResourceManager> copy_texture_CHROMIUM_;
+  std::unique_ptr<SRGBConverter> srgb_converter_;
   std::unique_ptr<ClearFramebufferResourceManager> clear_framebuffer_blit_;
 
   // Cached values of the currently assigned viewport dimensions.
@@ -4234,14 +4241,6 @@ bool GLES2DecoderImpl::CheckBoundFramebufferValid(const char* func_name) {
   Framebuffer* read_framebuffer = GetFramebufferInfoForTarget(target);
   valid = valid && CheckFramebufferValid(
       read_framebuffer, target, GL_INVALID_FRAMEBUFFER_OPERATION, func_name);
-
-  if (valid && feature_info_->feature_flags().desktop_srgb_support) {
-    bool enable_framebuffer_srgb =
-        (draw_framebuffer && draw_framebuffer->HasSRGBAttachments()) ||
-        (read_framebuffer && read_framebuffer->HasSRGBAttachments());
-    state_.EnableDisableFramebufferSRGB(enable_framebuffer_srgb);
-  }
-
   return valid;
 }
 
@@ -4307,6 +4306,21 @@ GLuint GLES2DecoderImpl::GetBoundReadFramebufferServiceId() {
   return 0;
 }
 
+GLuint GLES2DecoderImpl::GetBoundDrawFramebufferServiceId() {
+  Framebuffer* framebuffer =
+    GetFramebufferInfoForTarget(GL_DRAW_FRAMEBUFFER_EXT);
+  if (framebuffer) {
+    return framebuffer->service_id();
+  }
+  if (offscreen_target_frame_buffer_.get()) {
+    return offscreen_target_frame_buffer_->id();
+  }
+  if (surface_.get()) {
+    return surface_->GetBackingFramebufferObject();
+  }
+  return 0;
+}
+
 GLenum GLES2DecoderImpl::GetBoundReadFramebufferTextureType() {
   Framebuffer* framebuffer =
     GetFramebufferInfoForTarget(GL_READ_FRAMEBUFFER_EXT);
@@ -4543,6 +4557,11 @@ void GLES2DecoderImpl::Destroy(bool have_context) {
       copy_texture_CHROMIUM_.reset();
     }
 
+    if (srgb_converter_.get()) {
+      srgb_converter_->Destroy();
+      srgb_converter_.reset();
+    }
+
     clear_framebuffer_blit_.reset();
 
     if (state_.current_program.get()) {
@@ -4640,6 +4659,7 @@ void GLES2DecoderImpl::Destroy(bool have_context) {
   apply_framebuffer_attachment_cmaa_intel_.reset();
   copy_tex_image_blit_.reset();
   copy_texture_CHROMIUM_.reset();
+  srgb_converter_.reset();
   clear_framebuffer_blit_.reset();
 
   if (query_manager_.get()) {
@@ -7593,12 +7613,17 @@ void GLES2DecoderImpl::DoBlitFramebufferCHROMIUM(
     }
   }
 
-  GLenum src_format = GetBoundReadFramebufferInternalFormat();
+  GLenum src_internal_format = GetBoundReadFramebufferInternalFormat();
   GLenum src_type = GetBoundReadFramebufferTextureType();
 
+  bool read_buffer_has_srgb =
+      GetColorEncodingFromInternalFormat(src_internal_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);
+    bool is_src_signed_int =
+        GLES2Util::IsSignedIntegerFormat(src_internal_format);
+    bool is_src_unsigned_int =
+        GLES2Util::IsUnsignedIntegerFormat(src_internal_format);
     DCHECK(!is_src_signed_int || !is_src_unsigned_int);
 
     if ((is_src_signed_int || is_src_unsigned_int) && filter == GL_LINEAR) {
@@ -7608,7 +7633,7 @@ void GLES2DecoderImpl::DoBlitFramebufferCHROMIUM(
     }
 
     GLenum src_sized_format =
-        GLES2Util::ConvertToSizedFormat(src_format, src_type);
+        GLES2Util::ConvertToSizedFormat(src_internal_format, src_type);
     DCHECK(read_framebuffer || (is_feedback_loop != FeedbackLoopUnknown));
     const Framebuffer::Attachment* read_buffer =
         is_feedback_loop == FeedbackLoopUnknown ?
@@ -7619,6 +7644,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))) {
@@ -7674,11 +7701,63 @@ void GLES2DecoderImpl::DoBlitFramebufferCHROMIUM(
     }
   }
 
-  state_.SetDeviceCapabilityState(GL_SCISSOR_TEST, false);
-  BlitFramebufferHelper(
-      srcX0, srcY0, srcX1, srcY1, dstX0, dstY0, dstX1, dstY1, mask, filter);
-  state_.SetDeviceCapabilityState(GL_SCISSOR_TEST,
+  bool enable_srgb =
+      (read_buffer_has_srgb || draw_buffers_has_srgb) &&
+      ((mask & GL_COLOR_BUFFER_BIT) != 0);
+  bool encode_srgb_only =
+      (draw_buffers_has_srgb && !read_buffer_has_srgb) &&
+      ((mask & GL_COLOR_BUFFER_BIT) != 0);
+  if (!enable_srgb ||
+      read_buffer_samples > 0 ||

[Ken Russell (switch to Gerrit), 2016/09/16 22:32:29]

Per https://bugs.chromium.org/p/chromium/issues/detail?id=634525#c4 , when the
source is a multisampled sRGB renderbuffer and the OpenGL version is less than
4.2, it's necessary to first blit to a temporary sRGB texture, and then use your
SRGBConverter class to blit from that temporary texture to the destination
texture.

It's fine by me to defer this work to a later patch, but could you please add a
TODO(yunchao) here indicating that this code isn't fully correct, and needs
tests of this code path?

[piman, 2016/09/16 22:53:29]

Ah, you're right. I was under the impression that the glCopyTexImage2D would do
the resolve, but that is not the case (it would GL_INVALID_OPERATION)

[Ken Russell (switch to Gerrit), 2016/09/16 23:19:42]

Maybe I'm missing something. This code path only executes BlitFramebuffer and
then returns; I don't see a glCopyTexImage2D being done. Assuming
BlitFramebuffer is not doing the desired thing in all cases from an (sRGB or
not) multisampled renderbuffer to an (sRGB or not) texture for older OpenGL
versions, that's the reason I thought more code would be needed.

[piman, 2016/09/16 23:28:44]

Oh, I thought you were referencing the code in the SRGBConverter which does the
glCopyTexImage2D.

If the source is sRGB, encode_srgb_only is false, so if we're on desktop 4.2 we
don't go through the simple glBlitFramebuffer path (see l.7714), but we use the
SRGBConverter.

Either way, yes this will need to be fixed, so a TODO would be good.

[yunchao, 2016/09/17 15:14:13]

Done. TODO has been added.

Yes, I think the current conformance test doesn't cover this feature. I will
submit corresponding conformance test, and fix the TODO too.  

[yunchao, 2016/09/19 07:25:00]

@piman and @kbr, I revisit this feature. multi-sampled srgb image can not blit
to linear image, and multi-sampled linear image can not blit to srgb image too.
Because if the read buffer is multi-sampled, the format/type of the read buffer
and draw buffer should be exactly the same. 

In addition, the src region and the dst region should be exactly the same too. 

So if the read buffer is multi-sampled, there are only two possible cases: 
1. blit multi-sampled linear image to a single-sampled linear image. (This case
is OK, I think. It is not under the discussion to emulate srgb for desktop OGL)
2. blit multi-sampled srgb image to a single-sampled srgb image (This is the
only case under discussion). 
So maybe we don't need to do any extra work here. Just call blitFramebuffer
directly. Because my code need to do like this: multi-sampled srgb image -
(copy) - srgb image - (decode) - linear image - (blit) - linear image - (encode)
- srgb image. 
But the first step is wrong, we can not copy a multi-sampled image to a
single-sampled image. Copy operation can not do resolution for multi-sampled
image. It will report INVALID_OPERATION. Considering that the src region and the
dst region is exactly the same (no filtering), and both the read buffer and draw
buffer are srgb image (no conversion). I suppose that the only work is resolve
multi-sampled image to a single-sampled image. I think the blitframebuffer
itself can do this underlying. 
WDYT? 
I have a conformance test for this two:
https://github.com/KhronosGroup/WebGL/pull/2034. PTAL. 

+      !feature_info_->feature_flags().desktop_srgb_support ||
+      gl_version_info().IsAtLeastGL(4, 4) ||
+      (gl_version_info().IsAtLeastGL(4, 2) && encode_srgb_only)) {
+    if (enable_srgb && gl_version_info().IsAtLeastGL(4, 2)) {
+      state_.EnableDisableFramebufferSRGB(enable_srgb);
+    }
+
+    // TODO(yunchao) Need to revisit here. In GLES spec, blitFramebuffer
+    // should do scissor test per fragment operation.
+    state_.SetDeviceCapabilityState(GL_SCISSOR_TEST, false);
+    BlitFramebufferHelper(
+        srcX0, srcY0, srcX1, srcY1, dstX0, dstY0, dstX1, dstY1, mask, filter);
+    state_.SetDeviceCapabilityState(GL_SCISSOR_TEST,
                                   state_.enable_flags.scissor_test);
+    return;
+  }
+
+  // emulate srgb for desktop core profile when GL version < 4.4
+  // TODO(yunchao): Need to handle this situation:
+  // There are multiple draw buffers. Some of them are srgb images.
+  // The others are not.
+  state_.EnableDisableFramebufferSRGB(true);
+  if (!InitializeSRGBConverter(func_name)) {
+    return;
+  }
+  GLenum src_format =
+      TextureManager::ExtractFormatFromStorageFormat(src_internal_format);
+  srgb_converter_->Blit(this, srcX0, srcY0, srcX1, srcY1,
+                        dstX0, dstY0, dstX1, dstY1,
+                        mask, filter,
+                        GetBoundReadFramebufferSize(),
+                        GetBoundReadFramebufferServiceId(),
+                        src_internal_format, src_format, src_type,
+                        GetBoundDrawFramebufferServiceId(),
+                        read_buffer_has_srgb, draw_buffers_has_srgb);
+}
+
+bool GLES2DecoderImpl::InitializeSRGBConverter(
+    const char* function_name) {
+  if (!srgb_converter_.get()) {
+    LOCAL_COPY_REAL_GL_ERRORS_TO_WRAPPER(function_name);
+    srgb_converter_.reset(
+        new SRGBConverter(feature_info_.get()));
+  }
+  srgb_converter_->InitializeSRGBConverter(this);

[piman, 2016/09/16 20:49:27]

nit: maybe you can do that in the block above - since it will early-out the
second time it runs.

[yunchao, 2016/09/17 15:14:13]

Done.

+  if (LOCAL_PEEK_GL_ERROR(function_name) != GL_NO_ERROR) {
+    return false;
+  }
+  return true;
 }
 
 void GLES2DecoderImpl::EnsureRenderbufferBound() {