emulate srgb format for generateMipmap

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
index ffbaf3059e5384432e264139e5a5b321788246b6..5c0f210b25455bf0d86dafcc7b0770c8cb8e71ae 100644
--- a/gpu/command_buffer/service/gles2_cmd_srgb_converter.cc
+++ b/gpu/command_buffer/service/gles2_cmd_srgb_converter.cc
@@ -322,5 +322,89 @@ void SRGBConverter::Blit(
   decoder->RestoreGlobalState();
 }
 
+void SRGBConverter::SRGBGenerateMipmap(const gles2::GLES2Decoder* decoder,
+                                       Texture* tex,
+                                       GLenum target) {

[Ken Russell (switch to Gerrit), 2016/09/21 01:00:14]

Could you add some high-level comments for this function? If I understand
correctly, the intent here is:

- Copy the base level of the sRGB texture |tex| into a linear (non-sRGB) texture
- Perform the glGenerateMipmap call against the linear texture
- Copy each mip level of the linear texture back into the sRGB texture |tex|

Is this correct?

piman did a more thorough review of this code than I could, but I wonder if you
are already going to the trouble of iterating down the mip levels and drawing
each one, then why not just generate the mipmap directly into |tex| by, at each
level:

1) Allocate |level + 1| of |tex|
2) Bind |level + 1| of |tex| to the draw framebuffer
3) Set the TEXTURE_BASE_LEVEL and TEXTURE_MAX_LEVEL of |tex| to |level|
4) Set |tex|'s TEXTURE_MIN_FILTER (and MAG_FILTER, while you're at it) to LINEAR
(this should be done outside the loop)
5) Draw |level| of |tex|, implicitly into |level + 1|

That would avoid the sRGB -> linear and linear -> sRGB conversions into
textures, which are lossy. It shouldn't cause a rendering feedback loop, either,
to the best of my knowledge.

[yizhou.jiang, 2016/09/23 04:32:06]

hi kbr, I'm not quite sure if I understand correctly. Did you mean to attach
every mipmap levels to a draw framebuffer firstly, then using glDrawBuffers
function to define draw buffers, thus we could draw all of the mipmap levels
into the original texture only sampling once instead of level by level copying? 
And I read opengl es spec 3.0, found that in section 4.4, FRAMEBUFFER OBJECTS.
It mentioned that "If the attachment sizes are not all identical, rendering will
be limited to the largest area that can fit in all of the attachments (an
intersection of rectangles having a lower left of (0, 0) and an upper right of
(width, height) for each attachment)." So maybe we can't attach color
attachments with different sizes to a framebuffer.

+  DCHECK(srgb_converter_initialized_);
+
+  GLsizei width;
+  GLsizei height;
+  GLsizei depth;
+  GLenum type = 0;
+  GLenum internal_format = 0;
+  GLsizei base_level = tex->base_level();
+  tex->GetLevelSize(target, base_level, &width, &height, &depth);
+  tex->GetLevelType(target, 0, &type, &internal_format);

[piman, 2016/09/20 21:53:30]

Should this be base_level instead of 0?

[yizhou.jiang, 2016/09/21 08:59:22]

Done.

+  const GLint mipmap_level =

[piman, 2016/09/20 21:53:30]

nit: maybe mipmap_levels, since it's a count, not an array index.

[yizhou.jiang, 2016/09/21 08:59:22]

Done.

+      TextureManager::ComputeMipMapCount(target, width, height, depth);
+
+  // copy tex to srgb_decoder_textures_[0] with srgb format
+  glBindTexture(target, tex->service_id());
+  glBindFramebufferEXT(GL_READ_FRAMEBUFFER, srgb_decoder_fbo_);
+  glFramebufferTexture2DEXT(GL_READ_FRAMEBUFFER, GL_COLOR_ATTACHMENT0,
+                            GL_TEXTURE_2D, tex->service_id(), 0);

[piman, 2016/09/20 21:53:30]

Should this be base_level instead of 0?

[yizhou.jiang, 2016/09/21 08:59:22]

Done.

+
+  glBindTexture(GL_TEXTURE_2D, srgb_converter_textures_[0]);
+  glCopyTexImage2D(GL_TEXTURE_2D, 0, internal_format, 0, 0, width, height, 0);

[piman, 2016/09/20 21:53:30]

Do we need to make that extra copy, could we use the original texture as a
source instead? By selecting GL_NEAREST as the min filter the draw would only
use the base_level mip, so you're sure to only convert the relevant level, and
then we could restore the min filter.

[yizhou.jiang, 2016/09/21 08:59:22]

I removed copy tex to srgb_decoder_textures_[0]

+
+  // bind srgb_decoder_textures_[1] to draw framebuffer
+  glBindTexture(GL_TEXTURE_2D, srgb_converter_textures_[1]);
+  glTexImage2D(GL_TEXTURE_2D, 0, GL_RGBA, width, height, 0, GL_RGBA,
+               GL_UNSIGNED_BYTE, nullptr);
+  glBindFramebufferEXT(GL_DRAW_FRAMEBUFFER, srgb_decoder_fbo_);
+  glFramebufferTexture2DEXT(GL_DRAW_FRAMEBUFFER, GL_COLOR_ATTACHMENT0,
+                            GL_TEXTURE_2D, srgb_converter_textures_[1], 0);
+
+  // bind texture with srgb format and render with srgb_converter_program_
+  glUseProgram(srgb_converter_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);
+
+  glBindVertexArrayOES(srgb_converter_vao_);
+  glActiveTexture(GL_TEXTURE0);
+  glBindTexture(GL_TEXTURE_2D, srgb_converter_textures_[0]);
+  glDrawArrays(GL_TRIANGLES, 0, 6);
+
+  // generateMipmap for tex and srgb_decoder_textures_[1]
+  glBindTexture(GL_TEXTURE_2D, tex->service_id());
+  glGenerateMipmapEXT(GL_TEXTURE_2D);

[piman, 2016/09/20 21:53:30]

Why this one, is it to generate the image levels on the source texture? It seems
wasteful to force the driver to do the scaling blits, just to throw away the
data, maybe we could just create them with glTexImage2D in the loop below just
before the glFramebufferTexture2DEXT?

[yizhou.jiang, 2016/09/21 08:59:22]

Done.

+  glBindTexture(GL_TEXTURE_2D, srgb_converter_textures_[1]);
+  glGenerateMipmapEXT(GL_TEXTURE_2D);
+
+  // bind tex with rgba format and render with srgb_converter_program_
+  glBindFramebufferEXT(GL_DRAW_FRAMEBUFFER, srgb_encoder_fbo_);
+  glBindTexture(GL_TEXTURE_2D, srgb_converter_textures_[1]);
+  glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_MIN_FILTER,
+                  GL_NEAREST_MIPMAP_NEAREST);
+  width >>= 1;
+  height >>= 1;
+
+  for (GLint level = base_level + 1; level < mipmap_level; ++level) {

[piman, 2016/09/20 21:53:30]

mipmap_level is computed as the number of levels including and above base_level.
For example, if base_level is 3 and width/height (for that level) are 8, then we
have:
level3 = 8x8
level4 = 4x4
level5 = 2x2
level6 = 1x1

But mipmap_level will be computed as 1+log_2(8) == 4.

So I think this loop should be:
for (GLint level = base_level + 1; level < base_level + mipmap_level; ++level)

[yizhou.jiang, 2016/09/21 08:59:22]

Done.

+    glFramebufferTexture2DEXT(GL_DRAW_FRAMEBUFFER, GL_COLOR_ATTACHMENT0,
+                              GL_TEXTURE_2D, tex->service_id(), level);
+
+    glViewport(0, 0, width, height);
+    glDrawArrays(GL_TRIANGLES, 0, 6);

[Ken Russell (switch to Gerrit), 2016/09/21 01:00:14]

What's the intent of this loop? Is it intended to do a level-by-level copy of
srgb_converter_textures_[1] into "tex", with NEAREST filtering at each level?

[yizhou.jiang, 2016/09/21 08:59:22]

Yes. I try to sampling converter_textures_[1] to convert to srgb format, then
draw mipmaps to the original "tex".

+    width >>= 1;
+    height >>= 1;
+  }
+
+  // Restore state
+  decoder->RestoreAllAttributes();
+  decoder->RestoreTextureUnitBindings(0);
+  decoder->RestoreActiveTexture();
+  decoder->RestoreProgramBindings();
+  decoder->RestoreBufferBindings();
+  decoder->RestoreFramebufferBindings();
+  decoder->RestoreGlobalState();
+}
+
 }  // namespace gles2.
 }  // namespace gpu