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

gpu/command_buffer/service/gles2_cmd_srgb_converter.cc

Index: gpu/command_buffer/service/gles2_cmd_srgb_converter.cc
diff --git a/gpu/command_buffer/service/gles2_cmd_srgb_converter.cc b/gpu/command_buffer/service/gles2_cmd_srgb_converter.cc
new file mode 100644
index 0000000000000000000000000000000000000000..9d23bc885d2c20708a03995db1def46dde3eea1b
--- /dev/null
+++ b/gpu/command_buffer/service/gles2_cmd_srgb_converter.cc
@@ -0,0 +1,479 @@
+// Copyright (c) 2016 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/gles2_cmd_srgb_converter.h"
+
+#include "gpu/command_buffer/service/texture_manager.h"
+#include "ui/gl/gl_version_info.h"
+
+namespace {
+
+void CompileShader(GLuint shader, const char* shader_source) {
+  glShaderSource(shader, 1, &shader_source, 0);
+  glCompileShader(shader);
+#ifndef NDEBUG
+  GLint compile_status;
+  glGetShaderiv(shader, GL_COMPILE_STATUS, &compile_status);
+  if (GL_TRUE != compile_status)
+    DLOG(ERROR) << "CopyTexImage: shader compilation failure.";
+#endif
+}
+
+}  // anonymous namespace
+
+namespace gpu {
+namespace gles2 {
+
+SRGBConverter::SRGBConverter(
+    const gles2::FeatureInfo* feature_info)
+    : feature_info_(feature_info) {
+}
+
+SRGBConverter::~SRGBConverter() {}
+
+void SRGBConverter::InitializeSRGBDecoder(
+    const gles2::GLES2Decoder* decoder) {
+  if (srgb_decoder_initialized_) {
+    return;
+  }
+
+  srgb_decoder_program_ = glCreateProgram();
+
+  // Compile the vertex shader
+  const char* vs_source =
+      "#version 150\n"
+      "out vec2 v_texcoord;\n"
+      "\n"
+      "void main()\n"
+      "{\n"
+      "    const vec2 quad_positions[6] = vec2[6]\n"
+      "    (\n"
+      "        vec2(0.0f, 0.0f),\n"
+      "        vec2(0.0f, 1.0f),\n"
+      "        vec2(1.0f, 0.0f),\n"
+      "\n"
+      "        vec2(0.0f, 1.0f),\n"
+      "        vec2(1.0f, 0.0f),\n"
+      "        vec2(1.0f, 1.0f)\n"
+      "    );\n"
+      "\n"
+      "    vec2 xy = vec2((quad_positions[gl_VertexID] * 2.0) - 1.0);\n"
+      "    gl_Position = vec4(xy, 0.0, 1.0);\n"
+      "    v_texcoord = quad_positions[gl_VertexID];\n"
+      "}\n";
+
+  GLuint vs = glCreateShader(GL_VERTEX_SHADER);
+  CompileShader(vs, vs_source);
+  glAttachShader(srgb_decoder_program_, vs);
+  glDeleteShader(vs);
+
+  // Compile the fragment shader
+  // Sampling from a srgb texture and drawing to a linear color buffer, it will
+  // convert the srgb color space to linear color space automatically.
+  // However, sampling from a linear texture and drawing to a srgb color buffer,
+  // it will not convert linear to srgb automatically.

[piman, 2016/09/07 23:08:33]

I don't think I understand the comment. The shader itself doesn't do anything,
just outputs the sampled texture.
Are you talking about the fixed function parts of the pipeline, like
sRGB->linear conversion on texture fetch prior to filtering, and linear->sRGB
conversion prior to writing pixel values? If so, why would one apply but not the
other one?

[yunchao, 2016/09/08 18:32:49]

I saw the some difference between these 2 situation, which is out of my
expectation: 
1) sampling from a srgb texture and drawing to a linear image, then it will do
srgb conversion (srgb -> linear) automatically. 
2) sampling from a linear texture and drawing to a srgb image, it will not do
the conversion (linear -> srgb) automatically. 
After re-read the spec carefully, I found that the difference comes from
sampling stage, not drawing or per-fragment stage in gpu pipeline. Both the gles
spec and the OGL spec says that: if current bound texture is a srgb texture,
then it should do the conversion (srgb -> linear) as a part of filtering during
sampling. See the gles spec in
https://www.khronos.org/registry/gles/specs/3.0/es_spec_3.0.4.pdf#nameddest=s...,
in section <srgb texture color conversion>. You can find the same words in OGL
spec. 
But there is no similar words to do a conversion from linear to srgb if the
texture is linear and the drawing backend has srgb image.

Well, I have updated the comments here. Hope that can state this clearly.

[piman, 2016/09/08 19:39:18]

For the output, the state of texture doesn't matter. All that matters is whether
the output texture is in sRGB format (see 4.1.8 sRGB Conversion in the ES 3.0
spec), and additionally, on desktop GL (as it matters here), whether
GL_FRAMEBUFFER_SRGB is enabled (see 17.3.7 sRGB Conversion in the GL 4.5 spec).

The code in GLES2Decoder explicitly disables GL_FRAMEBUFFER_SRGB before calling
these functions, so that's why the linear->sRGB conversion doesn't happen.

[yunchao, 2016/09/08 23:16:49]

(Maybe I am wrong here) I may have different opinion. the state of texture
matters. See the section <srgb texture color conversion>.  if the texture is
srgb color format, it will be converted to linear during sampling. So we don't
need to convert srgb to linear one more time in shader. 

It seems to me that the section <SRGB conversion> is applied during blending.
Here I am not talking about blending, but sampling. 
Yes, that's true. linear-> SRGB doesn't happen, so we need to use a shader to
emulate this behavior. 
However, what I want to explain is why SRGB->linear happened automatically, why
we don't need to use a function in fragment shader to decode srgb to linear
explicitly. 

You know. In the encoder program, we have a function named encode() to
explicitly encode linear to srgb. But in decoder program, there is no function
decoder() to explicitly decode srgb to linear. It is weird, if anybody else look
into this implementation of emulation a srgb converter.  

What I want to say is that I do not forget to do this. The reason is that
sampling from srgb will decode srgb to linear automatically. However, sampling
from linear to srgb will not encode linear to srgb automatically. This is also
the reason I want to keep the comment code below.

+  const char* fs_source =
+      "#version 150\n"
+      "uniform sampler2D u_source_texture;\n"
+      "in vec2 v_texcoord;\n"
+      "out vec4 output_color;\n"
+      "\n"
+      /* "float decode(float color)\n"
+      "{\n"
+      "    float decoded_color;\n"
+      "    if (color <= 0.04045) {\n"
+      "        decoded_color = color / 12.92;\n"
+      "    } else {\n"
+      "        decoded_color = (color + 0.055) / 1.055;\n"
+      "        decoded_color = pow(decoded_color, 2.4);\n"
+      "    }\n"
+      "    return decoded_color;\n"
+      "}\n"
+      "\n" */

[piman, 2016/09/07 23:08:32]

nit: remove commented code.

[yunchao, 2016/09/08 18:32:49]

I would like to keep the code here, which indicate that we are aware of the
conversion from srgb to linear.

[piman, 2016/09/08 19:39:17]

Please don't. It makes it confusing. The conversion is well explained in the
spec.

[yunchao, 2016/09/08 23:16:49]

(Again, maybe I am wrong here) sampling from srgb texture to a linear image will
convert srgb to linear automatically, but sampling from a linear texture to a
srgb image will not convert linear to srgb. This is a little weird. 
It is OK to remove the code if it is confusing...  I just want to say what I
concerned. Then let's make a good choice based on it. 

+      "void main()\n"
+      "{\n"
+      "    vec4 c = texture(u_source_texture, v_texcoord);\n"
+      //"    output_color = vec4(decode(c.r), decode(c.g), decode(c.b), c.a);\n"

[piman, 2016/09/07 23:08:32]

nit: remove commented code.

[yunchao, 2016/09/08 18:32:49]

I'd like to keep the code here.

[piman, 2016/09/08 19:39:17]

Please don't.

+      "    output_color = c;\n"
+      "}\n";
+
+  GLuint fs = glCreateShader(GL_FRAGMENT_SHADER);
+  CompileShader(fs, fs_source);
+  glAttachShader(srgb_decoder_program_, fs);
+  glDeleteShader(fs);
+
+  glLinkProgram(srgb_decoder_program_);
+#ifndef NDEBUG
+  GLint linked = 0;
+  glGetProgramiv(srgb_decoder_program_, GL_LINK_STATUS, &linked);
+  if (!linked) {
+    DLOG(ERROR) << "BlitFramebuffer: program link failure.";
+  }
+#endif
+
+  GLuint texture_uniform =
+      glGetUniformLocation(srgb_decoder_program_, "u_source_texture");
+  glUseProgram(srgb_decoder_program_);
+  glUniform1i(texture_uniform, 0);
+
+  glGenTextures(srgb_decoder_textures_.size(), srgb_decoder_textures_.data());
+  glActiveTexture(GL_TEXTURE0);
+  for (auto srgb_decoder_texture : srgb_decoder_textures_) {
+    glBindTexture(GL_TEXTURE_2D, srgb_decoder_texture);
+
+    // Use nearest, non-mipmapped sampling with the srgb decoder texture

[piman, 2016/09/07 23:08:33]

nit: you're using linear, not nearest. Fix comment?

[yunchao, 2016/09/08 18:32:49]

Done.

+    glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_MIN_FILTER, GL_LINEAR);
+    glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_MAG_FILTER, GL_LINEAR);
+    glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_WRAP_S, GL_CLAMP_TO_EDGE);
+    glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_WRAP_T, GL_CLAMP_TO_EDGE);
+  }
+
+  glGenFramebuffersEXT(1, &srgb_decoder_fbo_);
+  glGenVertexArraysOES(1, &srgb_decoder_vao_);
+
+  decoder->RestoreTextureUnitBindings(0);
+  decoder->RestoreActiveTexture();
+  decoder->RestoreProgramBindings();
+
+  srgb_decoder_initialized_ = true;
+}
+
+void SRGBConverter::InitializeSRGBEncoder(
+    const gles2::GLES2Decoder* decoder) {
+  if (srgb_encoder_initialized_) {
+    return;
+  }
+
+  srgb_encoder_program_ = glCreateProgram();
+
+  // Compile the vertex shader
+  const char* vs_source =
+      "#version 150\n"
+      "out vec2 v_texcoord;\n"
+      "\n"
+      "void main()\n"
+      "{\n"
+      "    const vec2 quad_positions[6] = vec2[6]\n"
+      "    (\n"
+      "        vec2(0.0f, 0.0f),\n"
+      "        vec2(0.0f, 1.0f),\n"
+      "        vec2(1.0f, 0.0f),\n"
+      "\n"
+      "        vec2(0.0f, 1.0f),\n"
+      "        vec2(1.0f, 0.0f),\n"
+      "        vec2(1.0f, 1.0f)\n"
+      "    );\n"
+      "\n"
+      "    vec2 xy = vec2((quad_positions[gl_VertexID] * 2.0) - 1.0);\n"
+      "    gl_Position = vec4(xy, 0.0, 1.0);\n"
+      "    v_texcoord = quad_positions[gl_VertexID];\n"
+      "}\n";

[piman, 2016/09/07 23:08:32]

This is the same VS as above for the decoder, can we share?

[yunchao, 2016/09/08 18:32:49]

That's true. Done.

+
+  GLuint vs = glCreateShader(GL_VERTEX_SHADER);
+  CompileShader(vs, vs_source);
+  glAttachShader(srgb_encoder_program_, vs);
+  glDeleteShader(vs);
+
+  // Compile the fragment shader
+  const char* fs_source =
+      "#version 150\n"
+      "uniform sampler2D u_source_texture;\n"
+      "in vec2 v_texcoord;\n"
+      "out vec4 output_color;\n"
+      "\n"
+      "float encode(float color)\n"

[piman, 2016/09/07 23:08:33]

If we're doing the linear->srgb conversion with a draw, why do we disable sRGB
output and do the conversion in the shader, as opposed to just enabling
FRAMEBUFFER_SRGB and using the fixed function hardware, with a trivial blit in
the shader (like in the srgb->linear case)?

[yunchao, 2016/09/08 18:32:49]

Prior to OGL 4.4, it can not support FRAMEBUFFER_SRGB flag at all. So we need to
use a shader to do this conversion manually. After OGL 4.4 (4.4 is included), we
indeed enable this flag and let the fixed function do the conversion. In that
situation, the srgb_conversion here will not be executed. 

You can refer to the related crbug at
https://bugs.chromium.org/p/chromium/issues/detail?id=634525, where Zhenyao have
listed the specifications about this feature in different OGL versions. 

[piman, 2016/09/08 19:39:18]

Why not? FRAMEBUFFER_SRGB is definitely supported for draws in GL 4.2 (see
https://www.opengl.org/registry/doc/glspec42.core.20120427.pdf section 4.1.8
sRGB Conversion). The bug mentions that in that version it just doesn't apply to
blits.

[yunchao, 2016/09/08 23:16:49]

Sorry, I am wrong. FRAMEBUFFER_SRGB exists as long as blitFramebuffer exists. In
desktop OGL prior to 4.4, that flag just doesn't apply to blits, so we need to
emulate the srgb conversion for blitFramebuffer. 

+      "{\n"
+      "    float encoded_color;\n"
+      "    if (color <= 0.0) {\n"
+      "        return 0.0;\n"
+      "    } else if (color < 0.0031308) {\n"
+      "        return color * 12.92;\n"
+      "    } else if (color < 1) {\n"
+      "        return pow(color, 0.41666) * 1.055 - 0.055;\n"
+      "    } else {\n"
+      "        return 1.0;\n"
+      "    }\n"
+      "}\n"
+      "\n"
+      "void main()\n"
+      "{\n"
+      "    vec4 c = texture(u_source_texture, v_texcoord);\n"
+      "    output_color = vec4(encode(c.r), encode(c.g), encode(c.b), c.a);\n"
+      "}\n";
+
+  GLuint fs = glCreateShader(GL_FRAGMENT_SHADER);
+  CompileShader(fs, fs_source);
+  glAttachShader(srgb_encoder_program_, fs);
+  glDeleteShader(fs);
+
+  glLinkProgram(srgb_encoder_program_);
+#ifndef NDEBUG
+  GLint linked = 0;
+  glGetProgramiv(srgb_encoder_program_, GL_LINK_STATUS, &linked);
+  if (!linked) {
+    DLOG(ERROR) << "SRGB Encoder for BlitFramebuffer: program link failure.";
+  }
+#endif
+
+  GLuint texture_uniform =
+      glGetUniformLocation(srgb_encoder_program_, "u_source_texture");
+  glUseProgram(srgb_encoder_program_);
+  glUniform1i(texture_uniform, 0);
+
+  glGenTextures(srgb_encoder_textures_.size(), srgb_encoder_textures_.data());
+  glActiveTexture(GL_TEXTURE0);
+  for (auto srgb_encoder_texture : srgb_encoder_textures_) {
+    glBindTexture(GL_TEXTURE_2D, srgb_encoder_texture);
+
+    // Use nearest, non-mipmapped sampling with the srgb encoder texture

[piman, 2016/09/07 23:08:32]

nit: ditto, nearest->linear

[yunchao, 2016/09/08 18:32:49]

Done.

+    glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_MIN_FILTER, GL_LINEAR);
+    glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_MAG_FILTER, GL_LINEAR);
+    glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_WRAP_S, GL_CLAMP_TO_EDGE);
+    glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_WRAP_T, GL_CLAMP_TO_EDGE);
+  }
+
+  glGenFramebuffersEXT(1, &srgb_encoder_fbo_);
+  glGenVertexArraysOES(1, &srgb_encoder_vao_);
+
+  decoder->RestoreTextureUnitBindings(0);
+  decoder->RestoreActiveTexture();
+  decoder->RestoreProgramBindings();
+
+  srgb_encoder_initialized_ = true;
+}
+
+void SRGBConverter::Destroy() {
+  if (srgb_decoder_initialized_) {
+    glDeleteProgram(srgb_decoder_program_);
+    srgb_decoder_program_ = 0;
+
+    glDeleteTextures(srgb_decoder_textures_.size(),
+                     srgb_decoder_textures_.data());
+    srgb_decoder_textures_.fill(0);
+
+    glDeleteFramebuffersEXT(1, &srgb_decoder_fbo_);
+    srgb_decoder_fbo_ = 0;
+
+    glDeleteVertexArraysOES(1, &srgb_decoder_vao_);
+    srgb_decoder_vao_ = 0;
+
+    srgb_decoder_initialized_ = false;
+  }
+
+  if (srgb_encoder_initialized_) {
+    glDeleteProgram(srgb_encoder_program_);
+    srgb_encoder_program_ = 0;
+
+    glDeleteTextures(srgb_encoder_textures_.size(),
+                     srgb_encoder_textures_.data());
+    srgb_encoder_textures_.fill(0);
+
+    glDeleteFramebuffersEXT(1, &srgb_encoder_fbo_);
+    srgb_encoder_fbo_ = 0;
+
+    glDeleteVertexArraysOES(1, &srgb_encoder_vao_);
+    srgb_encoder_vao_ = 0;
+
+    srgb_encoder_initialized_ = false;
+  }
+}
+
+void SRGBConverter::SRGBToLinear(
+    const gles2::GLES2Decoder* decoder,
+    GLint srcX0,
+    GLint srcY0,
+    GLint srcX1,
+    GLint srcY1,
+    GLint dstX0,
+    GLint dstY0,
+    GLint dstX1,
+    GLint dstY1,
+    GLbitfield mask,
+    GLenum filter,
+    const gfx::Size& framebuffer_size,
+    GLuint src_framebuffer,
+    GLenum src_framebuffer_internal_format,
+    GLuint dst_framebuffer,
+    GLenum dst_framebuffer_internal_format,
+    GLenum dst_framebuffer_format,
+    GLenum dst_framebuffer_type) {

[piman, 2016/09/07 23:08:33]

This could use a high-level comment about what we're doing:
1- copy+crop source sRGB FBO to first texture (sRGB)
2- convert first texture to second texture (linear) with a draw
3- do glBlitFramebuffer from second texture to destination FBO (linear)


Note, I think you could merge step 2 and step 3 by doing the blit as a draw from
first texture to destination FBO (srgb->linear conversion on texture fetch
happens before filtering, so it should filter correctly).

[yunchao, 2016/09/08 18:32:49]

Done.

[yunchao, 2016/09/08 18:32:49]

Good suggestions! Done.
Good suggestion! If the src image is a texture (instead of a renderbuffer), the
copy operation in step 1 can be removed too. We can sampling directly from the
src image. l will do these two optimizations in follow-up change.

+
+  DCHECK(srgb_decoder_initialized_);
+
+  // Copy the image from framebuffer to the first srgb decoder texture
+  // TODO(yunchao) If the read buffer is a fbo texture, we can sample
+  // directly from that texture. In this way, we can save gpu memory.
+  glBindFramebufferEXT(GL_FRAMEBUFFER, src_framebuffer);
+  glActiveTexture(GL_TEXTURE0);
+  glBindTexture(GL_TEXTURE_2D, srgb_decoder_textures_[0]);
+
+  // We should not copy pixels outside of the read framebuffer. If we read
+  // these pixels, they would become in-bound during BlitFramebuffer. However,
+  // Out-of-bounds pixels will be initialized to 0 in CopyTexSubImage. But they
+  // should read as if the GL_CLAMP_TO_EDGE texture mapping mode were applied
+  // during BlitFramebuffer when the filter is GL_LINEAR.
+  GLuint x = srcX1 > srcX0 ? srcX0 : srcX1;
+  GLuint width = srcX1 > srcX0 ? srcX1 - srcX0 : srcX0 - srcX1;
+  GLuint y = srcY1 > srcY0 ? srcY0 : srcY1;
+  GLuint height = srcY1 > srcY0 ? srcY1 - srcY0 : srcY0 - srcY1;
+  gfx::Rect c(0, 0, framebuffer_size.width(), framebuffer_size.height());
+  c.Intersect(gfx::Rect(x, y, width, height));
+  GLuint xoffset = c.x() - x;
+  GLuint yoffset = c.y() - y;
+  glCopyTexImage2D(GL_TEXTURE_2D, 0, src_framebuffer_internal_format,
+                   c.x(), c.y(), c.width(), c.height(), 0);
+
+  // Make a temporary framebuffer using the second srgb decoder texture to
+  // render the converted (linear to srgb) result to.

[piman, 2016/09/07 23:08:32]

nit: you're converting srgb to linear, not linear to srgb.

[yunchao, 2016/09/08 18:32:49]

Done.

+  glBindBuffer(GL_PIXEL_UNPACK_BUFFER, 0);
+
+  glBindTexture(GL_TEXTURE_2D, srgb_decoder_textures_[1]);
+  glTexImage2D(GL_TEXTURE_2D, 0, dst_framebuffer_internal_format,
+               c.width(), c.height(),
+               0, dst_framebuffer_format, dst_framebuffer_type, nullptr);
+  glBindFramebufferEXT(GL_FRAMEBUFFER, srgb_decoder_fbo_);
+  glFramebufferTexture2DEXT(GL_FRAMEBUFFER, GL_COLOR_ATTACHMENT0, GL_TEXTURE_2D,
+                            srgb_decoder_textures_[1], 0);
+
+  // Render to the second srgb decoder texture,
+  // sampling from the first srgb decoder texture.
+  glUseProgram(srgb_decoder_program_);
+  glViewport(0, 0, width, height);
+  glDisable(GL_SCISSOR_TEST);
+  glDisable(GL_DEPTH_TEST);
+  glDisable(GL_STENCIL_TEST);
+  glDisable(GL_CULL_FACE);
+  glColorMask(GL_TRUE, GL_TRUE, GL_TRUE, GL_TRUE);
+  glDepthMask(GL_FALSE);
+  glDisable(GL_BLEND);
+  glDisable(GL_DITHER);
+
+  glBindTexture(GL_TEXTURE_2D, srgb_decoder_textures_[0]);
+  glBindVertexArrayOES(srgb_decoder_vao_);
+
+  glDrawArrays(GL_TRIANGLES, 0, 6);
+
+  // Finally, bind the temporary framebuffer as read framebuffer,
+  // blit the converted texture in temp fbo to the destination texture
+  // in destination framebuffer.
+  glBindFramebufferEXT(GL_READ_FRAMEBUFFER, srgb_decoder_fbo_);
+  glBindFramebufferEXT(GL_DRAW_FRAMEBUFFER, dst_framebuffer);
+  // Note that the source region has been changed in temp framebuffer.
+  // The xoffset/yoffset can make bliting clamp to the correct edge if
+  // CLAMP_TO_EDGE is necessary.
+  glBlitFramebuffer(srcX0 < srcX1 ? 0 - xoffset : width - xoffset,
+                    srcY0 < srcY1 ? 0 - yoffset : height - yoffset,
+                    srcX0 < srcX1 ? width - xoffset : 0 - xoffset,
+                    srcY0 < srcY1 ? height - yoffset : 0 - yoffset,

[Zhenyao Mo, 2016/09/06 22:30:11]

Can you also expand your new conformance test to cover the cases where srcX0 >
srcX1 and srcY0 > srcY1?

[yunchao, 2016/09/07 03:46:26]

Sure, see the patch at https://github.com/KhronosGroup/WebGL/pull/2013

+                    dstX0, dstY0, dstX1, dstY1, mask, filter);
+
+  // Restore state
+  decoder->RestoreAllAttributes();
+  decoder->RestoreTextureUnitBindings(0);
+  decoder->RestoreActiveTexture();
+  decoder->RestoreProgramBindings();
+  decoder->RestoreBufferBindings();
+  decoder->RestoreFramebufferBindings();
+  decoder->RestoreGlobalState();
+}
+
+void SRGBConverter::LinearToSRGB(
+    const gles2::GLES2Decoder* decoder,
+    GLint srcX0,
+    GLint srcY0,
+    GLint srcX1,
+    GLint srcY1,
+    GLint dstX0,
+    GLint dstY0,
+    GLint dstX1,
+    GLint dstY1,
+    GLbitfield mask,
+    GLenum filter,
+    const gfx::Size& framebuffer_size,
+    GLuint src_framebuffer,
+    GLenum src_framebuffer_internal_format,
+    GLuint dst_framebuffer,
+    GLenum dst_framebuffer_internal_format,
+    GLenum dst_framebuffer_format,
+    GLenum dst_framebuffer_type) {

[piman, 2016/09/07 23:08:33]

ditto, worth a high-level comment about what we do here.

[yunchao, 2016/09/08 18:32:49]

Done.

+
+  DCHECK(srgb_encoder_initialized_);
+
+  // Copy the framebuffer to the first srgb encoder texture
+  glBindFramebufferEXT(GL_FRAMEBUFFER, src_framebuffer);
+  glActiveTexture(GL_TEXTURE0);
+  glBindTexture(GL_TEXTURE_2D, srgb_encoder_textures_[0]);
+
+  // We should not copy pixels outside of the read framebuffer. If we read
+  // these pixels, they would become in-bound during BlitFramebuffer. However,
+  // Out-of-bounds pixels will be initialized to 0 in CopyTexSubImage. But they
+  // should read as if the GL_CLAMP_TO_EDGE texture mapping mode were applied
+  // during BlitFramebuffer when the filter is GL_LINEAR.
+  GLuint x = srcX1 > srcX0 ? srcX0 : srcX1;
+  GLuint width = srcX1 > srcX0 ? srcX1 - srcX0 : srcX0 - srcX1;
+  GLuint y = srcY1 > srcY0 ? srcY0 : srcY1;
+  GLuint height = srcY1 > srcY0 ? srcY1 - srcY0 : srcY0 - srcY1;
+  gfx::Rect c(0, 0, framebuffer_size.width(), framebuffer_size.height());
+  c.Intersect(gfx::Rect(x, y, width, height));
+  GLuint xoffset = c.x() - x;
+  GLuint yoffset = c.y() - y;
+  glCopyTexImage2D(GL_TEXTURE_2D, 0, src_framebuffer_internal_format,
+                   c.x(), c.y(), c.width(), c.height(), 0);
+
+  // Make a temporary framebuffer using the second srgb encoder texture to
+  // render the converted (linear to srgb) result to.
+  glBindBuffer(GL_PIXEL_UNPACK_BUFFER, 0);
+
+  glBindTexture(GL_TEXTURE_2D, srgb_encoder_textures_[1]);
+  glTexImage2D(GL_TEXTURE_2D, 0, dst_framebuffer_internal_format,
+               c.width(), c.height(),
+               0, dst_framebuffer_format, dst_framebuffer_type, nullptr);
+  glBindFramebufferEXT(GL_FRAMEBUFFER, srgb_encoder_fbo_);
+  glFramebufferTexture2DEXT(GL_FRAMEBUFFER, GL_COLOR_ATTACHMENT0, GL_TEXTURE_2D,
+                            srgb_encoder_textures_[1], 0);
+
+  // Render to the second srgb encoder texture,
+  // sampling from the first srgb encoder texture.
+  glUseProgram(srgb_encoder_program_);
+  glViewport(0, 0, width, height);
+  glDisable(GL_SCISSOR_TEST);
+  glDisable(GL_DEPTH_TEST);
+  glDisable(GL_STENCIL_TEST);
+  glDisable(GL_CULL_FACE);
+  glColorMask(GL_TRUE, GL_TRUE, GL_TRUE, GL_TRUE);
+  glDepthMask(GL_FALSE);
+  glDisable(GL_BLEND);
+  glDisable(GL_DITHER);
+
+  glBindTexture(GL_TEXTURE_2D, srgb_encoder_textures_[0]);
+  glBindVertexArrayOES(srgb_encoder_vao_);
+
+  glDrawArrays(GL_TRIANGLES, 0, 6);
+
+  // Finally, bind the temporary framebuffer as read framebuffer,
+  // blit the converted texture in temp fbo to the destination texture
+  // in destination framebuffer.
+  glBindFramebufferEXT(GL_READ_FRAMEBUFFER, srgb_encoder_fbo_);
+  glBindFramebufferEXT(GL_DRAW_FRAMEBUFFER, dst_framebuffer);
+  // Note that the source region has been changed in temp framebuffer.
+  // The xoffset/yoffset can make bliting clamp to the correct edge if
+  // CLAMP_TO_EDGE is necessary.
+  glBlitFramebuffer(srcX0 < srcX1 ? 0 - xoffset : width - xoffset,

[piman, 2016/09/07 23:08:32]

If we don't trust glBlitFramebuffer to correctly handle sRGB, then we should do
it in linear space, not sRGB (otherwise filtering is incorrect), that is do
instead:
1- glBlitFramebuffer from source fbo (linear) to first texture (linear)
2- convert (draw) first texture to destination FBO (sRGB)

(you would size the first texture to the destination dimensions to make sure
step 2 maps 1:1)

It would also be more efficient (only 1 additional copy).

[yunchao, 2016/09/08 18:32:49]

You are correct. My implementation is wrong for encoding... I think the current
test cases doesn't cover this feature, blit between images with different size
(need filtering) with chromatic color. 
I will write a conformance test to cover this feature. 

code has been updated. 

+                    srcY0 < srcY1 ? 0 - yoffset : height - yoffset,
+                    srcX0 < srcX1 ? width - xoffset : 0 - xoffset,
+                    srcY0 < srcY1 ? height - yoffset : 0 - yoffset,
+                    dstX0, dstY0, dstX1, dstY1, mask, filter);
+
+  // Restore state
+  decoder->RestoreAllAttributes();
+  decoder->RestoreTextureUnitBindings(0);
+  decoder->RestoreActiveTexture();
+  decoder->RestoreProgramBindings();
+  decoder->RestoreBufferBindings();
+  decoder->RestoreFramebufferBindings();
+  decoder->RestoreGlobalState();
+}
+
+}  // namespace gles2.
+}  // namespace gpu