Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2286593002:
[Command Buffer] emulate SRGB color format for BlitFramebuffer in OpenGL (Closed)
Description[Command Buffer] emulate SRGB color format for BlitFramebuffer in OpenGL.
In Desktop OGLs whose version < 4.4, they maybe don't enable srgb conversion
between srgb color format and linear color format. This change can emulate
srgb and do the conversion for machines with these OGL profiles.
BUG=634525
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
Committed: https://crrev.com/fd79169d48f8d31025b69c5dc05b7f00994a4f3d
Cr-Commit-Position: refs/heads/master@{#419396}
Patch Set 1 #Patch Set 2 : Fix build issue, run framebufferblit tests #Patch Set 3 : remove unnecessary code #Patch Set 4 : remove printf to upload code to review #Patch Set 5 : fix a small bug #Patch Set 6 : comment unnecessary code #Patch Set 7 : Remove unnecessary code, fix a build bug #Patch Set 8 : fix a small error #
Total comments: 41
Patch Set 9 : Addressed zmo@'s feedback #
Total comments: 8
Patch Set 10 : addressed feedbacks from zhenyao and qiankun #
Total comments: 8
Patch Set 11 : Addressed zmo@'s feedback #
Total comments: 36
Patch Set 12 : addressed piman@'s feedbacks, and rebased code #
Total comments: 1
Patch Set 13 : addressed piman's feedback #Patch Set 14 : addressed piman's feedback: add support to blit from srgb read buffer to srgb draw buffer, and consider filtering srgb image #
Total comments: 2
Patch Set 15 : addressed piman@'s feedback: don't use shader to do srgb conversion if possible #Patch Set 16 : code rebase #Patch Set 17 : addressed piman's feedback: use a trivial fragment shader, don't need to explicitly convert linear … #
Total comments: 6
Patch Set 18 : blitFramebuffer can resolove multisampled srgb image #Patch Set 19 : addressed piman@'s feedback: combine all srgb converters into one function #Patch Set 20 : code rebase #Patch Set 21 : addressed piman's feedback: reuse resources when decode/encode srgb to/from linear #
Total comments: 13
Patch Set 22 : addressed feedbacks from piman and kbr #Messages
Total messages: 171 (122 generated)
Description was changed from ========== [Command Buffer] emulate SRGB color format for GLES2DecoderImpl::DoBlitFramebuffer BUG= ========== to ========== [Command Buffer] emulate SRGB color format for GLES2DecoderImpl::DoBlitFramebuffer BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ==========
The CQ bit was checked by yunchao.he@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by yunchao.he@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #2 (id:20001) has been deleted
The CQ bit was checked by yunchao.he@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_optional_gpu_tests_rel on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_optional_gpu_...)
Patchset #4 (id:80001) has been deleted
The CQ bit was checked by yunchao.he@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #2 (id:40001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by yunchao.he@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by yunchao.he@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by yunchao.he@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by yunchao.he@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by yunchao.he@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #7 (id:180001) has been deleted
The CQ bit was checked by yunchao.he@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Exceeded global retry quota
Description was changed from ========== [Command Buffer] emulate SRGB color format for GLES2DecoderImpl::DoBlitFramebuffer BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ========== to ========== [Command Buffer] emulate SRGB color format for GLES2DecoderImpl::DoBlitFramebuffer BUG=634525 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ==========
yunchao.he@intel.com changed reviewers: + kbr@chromium.org, piman@chromium.org, qiankun.miao@intel.com, zmo@chromium.org
PTAL. Thanks a lot! Some explanations and concerns are inline. https://codereview.chromium.org/2286593002/diff/220001/content/test/gpu/gpu_t... File content/test/gpu/gpu_tests/webgl2_conformance_expectations.py (left): https://codereview.chromium.org/2286593002/diff/220001/content/test/gpu/gpu_t... content/test/gpu/gpu_tests/webgl2_conformance_expectations.py:227: self.Fail('deqp/functional/gles3/framebufferblit/conversion_28.html', These cases failed because of out-of-bounds bliting between float color format image. https://codereview.chromium.org/2286593002/diff/220001/content/test/gpu/gpu_t... File content/test/gpu/gpu_tests/webgl2_conformance_expectations.py (right): https://codereview.chromium.org/2286593002/diff/220001/content/test/gpu/gpu_t... content/test/gpu/gpu_tests/webgl2_conformance_expectations.py:260: ['mac', ('nvidia', 0xfe9)], bug=483282) The bots show that GPU info is Intel Iris OpenGL 4.1. And all failures are related to rgb5_a1 color format. srgb image related failures are fixed by this change. https://codereview.chromium.org/2286593002/diff/220001/gpu/command_buffer/ser... File gpu/command_buffer/service/gles2_cmd_decoder.cc (right): https://codereview.chromium.org/2286593002/diff/220001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_decoder.cc:7594: bool enable_srgb = read_buffer_has_srgb || draw_buffers_has_srgb; We need to traverse the read buffer and draw buffers to handle the TODO below: drawBuffers have srgb images, as well as non-srgb images during bliting. That patch is also ready, but it is a little tricky to cover all corner cases. Moreover, there is no test cases to cover this feature and expose potential errors. So I would like you to review the main part at first. https://codereview.chromium.org/2286593002/diff/220001/gpu/command_buffer/ser... File gpu/command_buffer/service/gles2_cmd_srgb_converter.h (right): https://codereview.chromium.org/2286593002/diff/220001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_srgb_converter.h:17: namespace gles2 { I wonder that why some classes are not under the gles2 namespace, for instance, gles2_cmd_clear_framebuffer.h and gles2_cmd_copy_tex_image.h. So does this one.
One more explanation. https://codereview.chromium.org/2286593002/diff/220001/gpu/command_buffer/ser... File gpu/command_buffer/service/gles2_cmd_srgb_converter.cc (right): https://codereview.chromium.org/2286593002/diff/220001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_srgb_converter.cc:93: " output_color = vec4(decode(c.r), decode(c.g), decode(c.b), c.a);\n" Obviously, the decode function in shader are not necessary. I will remove them. Just let you know that sampling from a srgb image and drawing to a non-srgb color buffer would enable srgb by default during drawing. However, sampling from a non-srgb image and drawing to a srgb color buffer would not enable srgb by default. Interesting...
zmo@chromium.org changed reviewers: + geofflang@chromium.org
yunchao, this is great! It still needs some work to get details correct (preferably by adding conformance tests first), but this has all the right designs. https://codereview.chromium.org/2286593002/diff/220001/content/test/gpu/gpu_t... File content/test/gpu/gpu_tests/webgl2_conformance_expectations.py (left): https://codereview.chromium.org/2286593002/diff/220001/content/test/gpu/gpu_t... content/test/gpu/gpu_tests/webgl2_conformance_expectations.py:209: self.Fail('deqp/functional/gles3/framebufferblit/conversion_04.html', What about Intel mesa? Should tests also pass there with this CL? https://codereview.chromium.org/2286593002/diff/220001/gpu/command_buffer/ser... File gpu/command_buffer/service/gles2_cmd_decoder.cc (right): https://codereview.chromium.org/2286593002/diff/220001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_decoder.cc:7599: !feature_info_->gl_version_info().is_desktop_core_profile || why core profile here? https://codereview.chromium.org/2286593002/diff/220001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_decoder.cc:7600: (read_buffer_has_srgb && draw_buffers_has_srgb && same_size) || Why same_size here? https://codereview.chromium.org/2286593002/diff/220001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_decoder.cc:7624: Framebuffer* read_framebuffer = GetFramebufferInfoForTarget(target); You should just use GetBoundReadFramebufferServiceId() and GetBoundDrawFramebufferServiceId(). The latter needs to be added though. https://codereview.chromium.org/2286593002/diff/220001/gpu/command_buffer/ser... File gpu/command_buffer/service/gles2_cmd_srgb_converter.cc (right): https://codereview.chromium.org/2286593002/diff/220001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_srgb_converter.cc:30: DCHECK(feature_info->gl_version_info().is_desktop_core_profile); Again, why desktop core profile only? https://codereview.chromium.org/2286593002/diff/220001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_srgb_converter.cc:62: "1.0);\n" nit: ugly formatting https://codereview.chromium.org/2286593002/diff/220001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_srgb_converter.cc:93: " output_color = vec4(decode(c.r), decode(c.g), decode(c.b), c.a);\n" On 2016/08/29 15:16:52, yunchao wrote: > Obviously, the decode function in shader are not necessary. I will remove them. > Just let you know that sampling from a srgb image and drawing to a non-srgb > color buffer would enable srgb by default during drawing. However, sampling from > a non-srgb image and drawing to a srgb color buffer would not enable srgb by > default. > > Interesting... As I pointed in the original bug, 4.2, 4.3, and 4.4 have different behaviors (although the drivers may not be spec conformant). For 4.2 or lower, we definitely need decoding, but if the Mac driver behaves differently, we will have to follow the driver behavior. https://codereview.chromium.org/2286593002/diff/220001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_srgb_converter.cc:123: glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_MAG_FILTER, GL_NEAREST); I think LINEAR is the correct filter. You should also set WRAP to CLAMP_TO_EDGE. https://codereview.chromium.org/2286593002/diff/220001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_srgb_converter.cc:163: "1.0);\n" nit: ugly formatting https://codereview.chromium.org/2286593002/diff/220001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_srgb_converter.cc:309: glCopyTexImage2D(GL_TEXTURE_2D, 0, src_framebuffer_internal_format, If the original fbo read_buffer is a texture, can we combine step 1 and 2? (if it's a lot of work, please add a TODO() and follow up). https://codereview.chromium.org/2286593002/diff/220001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_srgb_converter.cc:348: glBlitFramebuffer(srcX0 < srcX1 ? 0 : width, This looks incorrect. src region could be clamped, and then you need to clamp/shift the dst with the same scale to make sure you write to the right dst region. Be careful here, because src region and dst region could be opposite directions, for exampe, srcX0<srcX1 but dstX0>dstX1. You need to consider all these cases (I think you should write a webgl conformance test for all these cases first) https://codereview.chromium.org/2286593002/diff/220001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_srgb_converter.cc:352: dstX0, dstY0, dstX1, dstY1, mask, filter); I think you need to enable SCISSOR_TEST before writing to the dst region. Could you please add a webgl conformance test to verify the correct behavior? https://codereview.chromium.org/2286593002/diff/220001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_srgb_converter.cc:441: glBlitFramebuffer(srcX0 < srcX1 ? 0 : width, I think you need to enable SCISSOR_TEST before writing to the destination. https://codereview.chromium.org/2286593002/diff/220001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_srgb_converter.cc:445: dstX0, dstY0, dstX1, dstY1, mask, filter); Same here, dst region could be wrong if src region is clamped. https://codereview.chromium.org/2286593002/diff/220001/gpu/command_buffer/ser... File gpu/command_buffer/service/gles2_cmd_srgb_converter.h (right): https://codereview.chromium.org/2286593002/diff/220001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_srgb_converter.h:17: namespace gles2 { On 2016/08/29 14:33:47, yunchao wrote: > I wonder that why some classes are not under the gles2 namespace, for instance, > gles2_cmd_clear_framebuffer.h and gles2_cmd_copy_tex_image.h. So does this one. I think they should be under gpu/gles2, including this one.
geoff, FYI. We need the same mechanism in MANGLE.
https://codereview.chromium.org/2286593002/diff/220001/gpu/command_buffer/ser... File gpu/command_buffer/service/gles2_cmd_decoder.cc (right): https://codereview.chromium.org/2286593002/diff/220001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_decoder.cc:7606: state_.SetDeviceCapabilityState(GL_SCISSOR_TEST, false); This is a nasty situation. We define the CHROMIUM version of blitframebuffer extension as no SCISSOR_TEST, whereas the ES3 version should have SCISSOR_TEST. If you add a conformance test to verify SCISSOR_TEST and BlitFramebuffer, you will have to fix here also, i.e., branch out into CHROMIUM extension behavior and ES3 behavior,
https://codereview.chromium.org/2286593002/diff/220001/gpu/command_buffer/ser... File gpu/command_buffer/service/gles2_cmd_srgb_converter.cc (right): https://codereview.chromium.org/2286593002/diff/220001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_srgb_converter.cc:318: c.width(), c.height(), Thinking more about this, it seems to me that here it should be the destination width and height, because it is the only chance you can sample the texture using CLAMP_TO_EDGE and get the values right. Otherwise you won't be able to handle src region out-of-bounds situation. Same with the LinearToSRGB().
On 2016/08/30 00:22:05, Zhenyao Mo wrote: > https://codereview.chromium.org/2286593002/diff/220001/gpu/command_buffer/ser... > File gpu/command_buffer/service/gles2_cmd_srgb_converter.cc (right): > > https://codereview.chromium.org/2286593002/diff/220001/gpu/command_buffer/ser... > gpu/command_buffer/service/gles2_cmd_srgb_converter.cc:318: c.width(), > c.height(), > Thinking more about this, it seems to me that here it should be the destination > width and height, because it is the only chance you can sample the texture using > CLAMP_TO_EDGE and get the values right. Otherwise you won't be able to handle > src region out-of-bounds situation. > > Same with the LinearToSRGB(). Maybe not. I thought about this for a while. We can just copy the intersect region (read framebuffer size intersects with read region designated by blitframebuffer), and the final blitFramebuffer will clamp_to_edge if necessary. In OGL before 4.4, I think its blitFramebuffer just doesn't handle the srgb conversion issue (because they follow OGL spec, instead of GLES spec). But It can still handle the other features.
On 2016/08/30 01:23:14, yunchao wrote: > On 2016/08/30 00:22:05, Zhenyao Mo wrote: > > > https://codereview.chromium.org/2286593002/diff/220001/gpu/command_buffer/ser... > > File gpu/command_buffer/service/gles2_cmd_srgb_converter.cc (right): > > > > > https://codereview.chromium.org/2286593002/diff/220001/gpu/command_buffer/ser... > > gpu/command_buffer/service/gles2_cmd_srgb_converter.cc:318: c.width(), > > c.height(), > > Thinking more about this, it seems to me that here it should be the > destination > > width and height, because it is the only chance you can sample the texture > using > > CLAMP_TO_EDGE and get the values right. Otherwise you won't be able to handle > > src region out-of-bounds situation. > > > > Same with the LinearToSRGB(). > > Maybe not. I thought about this for a while. We can just copy the intersect > region (read framebuffer size intersects with read region designated by > blitframebuffer), and the final blitFramebuffer will clamp_to_edge if necessary. > > In OGL before 4.4, I think its blitFramebuffer just doesn't handle the srgb > conversion issue (because they follow OGL spec, instead of GLES spec). But It > can still handle the other features. That's fine, but then you will still need to do something because apparently MacOSX gets the out-of-bounds behaviors wrong.
The CQ bit was checked by yunchao.he@intel.com to run a CQ dry run
Description was changed from ========== [Command Buffer] emulate SRGB color format for GLES2DecoderImpl::DoBlitFramebuffer BUG=634525 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ========== to ========== [Command Buffer] emulate SRGB color format for BlitFramebuffer in OpenGL. In Desktop OGL whose version < 4.4. They may be not enable srgb conversion between srgb color format and linear color format. This change can emulate srgb and do the conversion for machines with these OGL profiles. BUG=634525 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ==========
Description was changed from ========== [Command Buffer] emulate SRGB color format for BlitFramebuffer in OpenGL. In Desktop OGL whose version < 4.4. They may be not enable srgb conversion between srgb color format and linear color format. This change can emulate srgb and do the conversion for machines with these OGL profiles. BUG=634525 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ========== to ========== [Command Buffer] emulate SRGB color format for BlitFramebuffer in OpenGL. In Desktop OGL whose version < 4.4, they may be not enable srgb conversion between srgb color format and linear color format. This change can emulate srgb and do the conversion for machines with these OGL profiles. BUG=634525 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ==========
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== [Command Buffer] emulate SRGB color format for BlitFramebuffer in OpenGL. In Desktop OGL whose version < 4.4, they may be not enable srgb conversion between srgb color format and linear color format. This change can emulate srgb and do the conversion for machines with these OGL profiles. BUG=634525 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ========== to ========== [Command Buffer] emulate SRGB color format for BlitFramebuffer in OpenGL. In Desktop OGLs whose version < 4.4, they maybe don't enable srgb conversion between srgb color format and linear color format. This change can emulate srgb and do the conversion for machines with these OGL profiles. BUG=634525 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_optional_gpu_tests_rel on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_optional_gpu_...)
The CQ bit was checked by yunchao.he@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #9 (id:240001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Some bugs in the shader. https://codereview.chromium.org/2286593002/diff/260001/gpu/command_buffer/ser... File gpu/command_buffer/service/gles2_cmd_srgb_converter.cc (right): https://codereview.chromium.org/2286593002/diff/260001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_srgb_converter.cc:85: " decoded_color = pow(decoded_color, 4.2);\n" 4.2 -> 2.4 https://codereview.chromium.org/2286593002/diff/260001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_srgb_converter.cc:94: " output_color = c;\n" Why output_color is assigned twice here? https://codereview.chromium.org/2286593002/diff/260001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_srgb_converter.cc:182: " if (color < 0.0031308) {\n" if (color <= 0.0) { return 0.0; } else if (color < 0.0031308) { ... } https://codereview.chromium.org/2286593002/diff/260001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_srgb_converter.cc:184: " } else {\n" else (color < 1.0) { ... } else { return 1.0 }
The CQ bit was checked by yunchao.he@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Thanks for your review, Zhenyao. Code has been updated. PTAL. https://codereview.chromium.org/2286593002/diff/220001/content/test/gpu/gpu_t... File content/test/gpu/gpu_tests/webgl2_conformance_expectations.py (left): https://codereview.chromium.org/2286593002/diff/220001/content/test/gpu/gpu_t... content/test/gpu/gpu_tests/webgl2_conformance_expectations.py:209: self.Fail('deqp/functional/gles3/framebufferblit/conversion_04.html', On 2016/08/29 23:36:50, Zhenyao Mo wrote: > What about Intel mesa? Should tests also pass there with this CL? Yes. With this change applied, test cases of framebufferblit for srgb (default version in Ubuntu 16.04 with mesa 3.3) can pass. But w/o this change, these cases failed. https://codereview.chromium.org/2286593002/diff/220001/gpu/command_buffer/ser... File gpu/command_buffer/service/gles2_cmd_decoder.cc (right): https://codereview.chromium.org/2286593002/diff/220001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_decoder.cc:7599: !feature_info_->gl_version_info().is_desktop_core_profile || On 2016/08/29 23:36:50, Zhenyao Mo wrote: > why core profile here? Removed them already. https://codereview.chromium.org/2286593002/diff/220001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_decoder.cc:7600: (read_buffer_has_srgb && draw_buffers_has_srgb && same_size) || On 2016/08/29 23:36:50, Zhenyao Mo wrote: > Why same_size here? When we blit a read buffer with srgb color format to draw buffers with srgb color format, if the size of src region is not the same as the size of the dst region, we need to filter. You know, srgb is non-linear, filtering against srgb color format is not correct (See qiankun's conformance test for generateMipmap against srgb: https://github.com/KhronosGroup/WebGL/pull/1951). In this situation, we need to convert srgb to linear, then blit and filter, finally, convert linear to srgb. However, if the src region is exactly the same size with the dst region, we can blit srgb read buffer to srgb draw buffers directly without filtering. https://codereview.chromium.org/2286593002/diff/220001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_decoder.cc:7606: state_.SetDeviceCapabilityState(GL_SCISSOR_TEST, false); On 2016/08/29 23:39:59, Zhenyao Mo wrote: > This is a nasty situation. We define the CHROMIUM version of blitframebuffer > extension as no SCISSOR_TEST, whereas the ES3 version should have SCISSOR_TEST. > If you add a conformance test to verify SCISSOR_TEST and BlitFramebuffer, you > will have to fix here also, i.e., branch out into CHROMIUM extension behavior > and ES3 behavior, That's true. The GLES spec says that blitFramebuffer should do scissor test per fragment operation. The original code has the same problem too. Seems that current CTS doesn't cover this feature. I will write conformance test and revisit this later. I have added a TODO for it. https://codereview.chromium.org/2286593002/diff/220001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_decoder.cc:7624: Framebuffer* read_framebuffer = GetFramebufferInfoForTarget(target); On 2016/08/29 23:36:50, Zhenyao Mo wrote: > You should just use GetBoundReadFramebufferServiceId() and > GetBoundDrawFramebufferServiceId(). The latter needs to be added though. Done. https://codereview.chromium.org/2286593002/diff/220001/gpu/command_buffer/ser... File gpu/command_buffer/service/gles2_cmd_srgb_converter.cc (right): https://codereview.chromium.org/2286593002/diff/220001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_srgb_converter.cc:30: DCHECK(feature_info->gl_version_info().is_desktop_core_profile); On 2016/08/29 23:36:50, Zhenyao Mo wrote: > Again, why desktop core profile only? I have removed it. https://codereview.chromium.org/2286593002/diff/220001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_srgb_converter.cc:62: "1.0);\n" On 2016/08/29 23:36:50, Zhenyao Mo wrote: > nit: ugly formatting Done. https://codereview.chromium.org/2286593002/diff/220001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_srgb_converter.cc:93: " output_color = vec4(decode(c.r), decode(c.g), decode(c.b), c.a);\n" On 2016/08/29 23:36:50, Zhenyao Mo wrote: > On 2016/08/29 15:16:52, yunchao wrote: > > Obviously, the decode function in shader are not necessary. I will remove > them. > > Just let you know that sampling from a srgb image and drawing to a non-srgb > > color buffer would enable srgb by default during drawing. However, sampling > from > > a non-srgb image and drawing to a srgb color buffer would not enable srgb by > > default. > > > > Interesting... > > As I pointed in the original bug, 4.2, 4.3, and 4.4 have different behaviors > (although the drivers may not be spec conformant). For 4.2 or lower, we > definitely need decoding, but if the Mac driver behaves differently, we will > have to follow the driver behavior. My MacOS is OGL 4.1, but it doesn't need to decode here. I read the OGL spec, it says that we should do srgb conversion during drawing per fragment operation. For instance, in the OGL 4.2 spec, see the figure 4.1 on page 284 at https://www.opengl.org/registry/doc/glspec42.core.20110822.withchanges.pdf. What I feel weird is that it just decode to linear(srgb > linear), but does not encode to srgb(linear > srgb) in MacOSX. I tested it on Mesa too. Linux mesa (Ubuntu 16.04 with 3.3 core profile) also follow the same rule. It will decode automatically, but does not encode. I would like to comment the code snippet, but do not remove them. https://codereview.chromium.org/2286593002/diff/220001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_srgb_converter.cc:123: glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_MAG_FILTER, GL_NEAREST); On 2016/08/29 23:36:50, Zhenyao Mo wrote: > I think LINEAR is the correct filter. > You should also set WRAP to CLAMP_TO_EDGE. Done. Currently there is neither filtering nor repeating during sampling. The during the copy, the src region is exactly the same size with the dst region. So it doesn't take effect. However, we can still write the code as you suggested, in case the scenario changed. https://codereview.chromium.org/2286593002/diff/220001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_srgb_converter.cc:163: "1.0);\n" On 2016/08/29 23:36:50, Zhenyao Mo wrote: > nit: ugly formatting Done. https://codereview.chromium.org/2286593002/diff/220001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_srgb_converter.cc:309: glCopyTexImage2D(GL_TEXTURE_2D, 0, src_framebuffer_internal_format, On 2016/08/29 23:36:50, Zhenyao Mo wrote: > If the original fbo read_buffer is a texture, can we combine step 1 and 2? (if > it's a lot of work, please add a TODO() and follow up). Add a TODO first, will revisit this soon. https://codereview.chromium.org/2286593002/diff/220001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_srgb_converter.cc:318: c.width(), c.height(), On 2016/08/30 00:22:05, Zhenyao Mo wrote: > Thinking more about this, it seems to me that here it should be the destination > width and height, because it is the only chance you can sample the texture using > CLAMP_TO_EDGE and get the values right. Otherwise you won't be able to handle > src region out-of-bounds situation. > > Same with the LinearToSRGB(). As we discussed, the real blitFramebuffer can clamp_to_edge. https://codereview.chromium.org/2286593002/diff/220001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_srgb_converter.cc:348: glBlitFramebuffer(srcX0 < srcX1 ? 0 : width, On 2016/08/29 23:36:50, Zhenyao Mo wrote: > This looks incorrect. src region could be clamped, and then you need to > clamp/shift the dst with the same scale to make sure you write to the right dst > region. > > Be careful here, because src region and dst region could be opposite directions, > for exampe, srcX0<srcX1 but dstX0>dstX1. You need to consider all these cases (I > think you should write a webgl conformance test for all these cases first) The same as the issue above. blitFramebuffer will clamp_to_edge when sampling from src region if necessary. https://codereview.chromium.org/2286593002/diff/220001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_srgb_converter.cc:352: dstX0, dstY0, dstX1, dstY1, mask, filter); On 2016/08/29 23:36:50, Zhenyao Mo wrote: > I think you need to enable SCISSOR_TEST before writing to the dst region. Could > you please add a webgl conformance test to verify the correct behavior? The linear color format has this problem too. I will revisit this issue. A TODO has been added into GLES2DecoderImpl::DoBlitFramebufferCHROMIUM. https://codereview.chromium.org/2286593002/diff/220001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_srgb_converter.cc:441: glBlitFramebuffer(srcX0 < srcX1 ? 0 : width, On 2016/08/29 23:36:50, Zhenyao Mo wrote: > I think you need to enable SCISSOR_TEST before writing to the destination. Acknowledged. https://codereview.chromium.org/2286593002/diff/220001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_srgb_converter.cc:445: dstX0, dstY0, dstX1, dstY1, mask, filter); On 2016/08/29 23:36:50, Zhenyao Mo wrote: > Same here, dst region could be wrong if src region is clamped. The same as the issue above. blitFramebuffer will clamp_to_edge when sampling from src region if necessary. https://codereview.chromium.org/2286593002/diff/220001/gpu/command_buffer/ser... File gpu/command_buffer/service/gles2_cmd_srgb_converter.h (right): https://codereview.chromium.org/2286593002/diff/220001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_srgb_converter.h:17: namespace gles2 { On 2016/08/29 23:36:50, Zhenyao Mo wrote: > On 2016/08/29 14:33:47, yunchao wrote: > > I wonder that why some classes are not under the gles2 namespace, for > instance, > > gles2_cmd_clear_framebuffer.h and gles2_cmd_copy_tex_image.h. So does this > one. > > I think they should be under gpu/gles2, including this one. Done.
The CQ bit was checked by yunchao.he@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #10 (id:280001) has been deleted
Thanks for your suggestions, Qiankun. https://codereview.chromium.org/2286593002/diff/260001/gpu/command_buffer/ser... File gpu/command_buffer/service/gles2_cmd_srgb_converter.cc (right): https://codereview.chromium.org/2286593002/diff/260001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_srgb_converter.cc:85: " decoded_color = pow(decoded_color, 4.2);\n" On 2016/08/30 16:38:42, qiankun wrote: > 4.2 -> 2.4 Done. https://codereview.chromium.org/2286593002/diff/260001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_srgb_converter.cc:94: " output_color = c;\n" On 2016/08/30 16:38:42, qiankun wrote: > Why output_color is assigned twice here? The decoder is not necessary at all. It is weird. See my comments with Zhenyao in the previous patchset. https://codereview.chromium.org/2286593002/diff/260001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_srgb_converter.cc:182: " if (color < 0.0031308) {\n" On 2016/08/30 16:38:42, qiankun wrote: > if (color <= 0.0) { > return 0.0; > } else if (color < 0.0031308) { > ... > } Done. https://codereview.chromium.org/2286593002/diff/260001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_srgb_converter.cc:184: " } else {\n" On 2016/08/30 16:38:42, qiankun wrote: > else (color < 1.0) { > ... > } else { > return 1.0 > } I am confused by the difference between decoder equation and encoder equation. The decoder (srgb > linear) don't have equation for any color channel that smaller than 0.0 or bigger that 1.0. So I don't add these unusual cases into the encoder too. However, today I found that srgb image should be unsigned fixed color buffer in OGL spec: "Note that only unsigned normalized fixed-point color buffers may be SRGB-encoded. ". But there are no such words about this in GLES spec. But it can explain that why decoder doesn't need to handle these unusual cases.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Any feedback about this one? Then I can proceed the follow-ups.
https://codereview.chromium.org/2286593002/diff/220001/gpu/command_buffer/ser... File gpu/command_buffer/service/gles2_cmd_decoder.cc (right): https://codereview.chromium.org/2286593002/diff/220001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_decoder.cc:7600: (read_buffer_has_srgb && draw_buffers_has_srgb && same_size) || On 2016/08/31 09:18:44, yunchao wrote: > On 2016/08/29 23:36:50, Zhenyao Mo wrote: > > Why same_size here? > > When we blit a read buffer with srgb color format to draw buffers with srgb > color format, if the size of src region is not the same as the size of the dst > region, we need to filter. You know, srgb is non-linear, filtering against srgb > color format is not correct (See qiankun's conformance test for generateMipmap > against srgb: https://github.com/KhronosGroup/WebGL/pull/1951). > > In this situation, we need to convert srgb to linear, then blit and filter, > finally, convert linear to srgb. > > However, if the src region is exactly the same size with the dst region, we can > blit srgb read buffer to srgb draw buffers directly without filtering. Our purpose is to emulate ES3 sRGB behavior, which GL 4.4 is the same, but older version isn't, they didn't perform decoding when read from sRGB formats. So even with the same size, you will still need to do some emulation. https://codereview.chromium.org/2286593002/diff/300001/content/test/gpu/gpu_t... File content/test/gpu/gpu_tests/webgl2_conformance_expectations.py (left): https://codereview.chromium.org/2286593002/diff/300001/content/test/gpu/gpu_t... content/test/gpu/gpu_tests/webgl2_conformance_expectations.py:209: self.Fail('deqp/functional/gles3/framebufferblit/conversion_04.html', The reason I asked about Mesa Intel is can we also remove the Linux Intel entries? https://codereview.chromium.org/2286593002/diff/300001/gpu/command_buffer/ser... File gpu/command_buffer/service/gles2_cmd_decoder.cc (right): https://codereview.chromium.org/2286593002/diff/300001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_decoder.cc:4308: if (offscreen_resolved_frame_buffer_.get()) { This is incorrect. It should be offscreen_target_frame_buffer_ for DRAW_FRAMEBUFFER. https://codereview.chromium.org/2286593002/diff/300001/gpu/command_buffer/ser... File gpu/command_buffer/service/gles2_cmd_srgb_converter.cc (right): https://codereview.chromium.org/2286593002/diff/300001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_srgb_converter.cc:365: dstX0, dstY0, dstX1, dstY1, mask, filter); I still think this is incorrect. At glCopyTexImage2D, the texture (srgb_decoder_textures_[0]) size is no longer the full size, but the clipped size. At glDawArrays, the texture (srgb_decoder_textures_[1]) also is the clipped size. Then you blit, let's say the clipped size is (cw, ch), and full size is (w, h), Now look at the target texture, [0-cw, 0-ch] are from the srgb_decoder_textures_[1], and [cw-w, ch-h] are from the border (because blit acts like CLAMP_TO_EDGE) But that may not always be the case. for example, srcX0 can be out of bounds, so for the target texture, the first a few pixels per line should be clamped to the left edge. Am I explaining the situation clearly? https://codereview.chromium.org/2286593002/diff/300001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_srgb_converter.cc:458: dstX0, dstY0, dstX1, dstY1, mask, filter); Same situation here.
The CQ bit was checked by yunchao.he@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Patchset #11 (id:320001) has been deleted
Thanks for your careful review, Zhenyao. Code has been updated, please take another look. https://codereview.chromium.org/2286593002/diff/220001/gpu/command_buffer/ser... File gpu/command_buffer/service/gles2_cmd_decoder.cc (right): https://codereview.chromium.org/2286593002/diff/220001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_decoder.cc:7600: (read_buffer_has_srgb && draw_buffers_has_srgb && same_size) || On 2016/09/01 19:28:33, Zhenyao Mo wrote: > On 2016/08/31 09:18:44, yunchao wrote: > > On 2016/08/29 23:36:50, Zhenyao Mo wrote: > > > Why same_size here? > > > > When we blit a read buffer with srgb color format to draw buffers with srgb > > color format, if the size of src region is not the same as the size of the dst > > region, we need to filter. You know, srgb is non-linear, filtering against > srgb > > color format is not correct (See qiankun's conformance test for generateMipmap > > against srgb: https://github.com/KhronosGroup/WebGL/pull/1951). > > > > In this situation, we need to convert srgb to linear, then blit and filter, > > finally, convert linear to srgb. > > > > However, if the src region is exactly the same size with the dst region, we > can > > blit srgb read buffer to srgb draw buffers directly without filtering. > > Our purpose is to emulate ES3 sRGB behavior, which GL 4.4 is the same, but older > version isn't, they didn't perform decoding when read from sRGB formats. So > even with the same size, you will still need to do some emulation. Yes, you are correct. I thought that if the size was the same (It was common in real case), we can disable framebuffer_srgb_ and blit-ed directly without any decode/encode. It would be an performance optimization. Unfortunately, the underlying driver may do encode during bliting even framebuffer_srgb_ flag was false, so it would get wrong result. The latest patchset removes this line, and always do encode/decode by emulation. https://codereview.chromium.org/2286593002/diff/300001/content/test/gpu/gpu_t... File content/test/gpu/gpu_tests/webgl2_conformance_expectations.py (left): https://codereview.chromium.org/2286593002/diff/300001/content/test/gpu/gpu_t... content/test/gpu/gpu_tests/webgl2_conformance_expectations.py:209: self.Fail('deqp/functional/gles3/framebufferblit/conversion_04.html', On 2016/09/01 19:28:33, Zhenyao Mo wrote: > The reason I asked about Mesa Intel is can we also remove the Linux Intel > entries? That's true. However, with this change applied, Intel mesa driver still have a couple of bugs about blitFramebuffer (They are not related to srgb issue). I have updated those entries accordingly. https://codereview.chromium.org/2286593002/diff/300001/gpu/command_buffer/ser... File gpu/command_buffer/service/gles2_cmd_decoder.cc (right): https://codereview.chromium.org/2286593002/diff/300001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_decoder.cc:4308: if (offscreen_resolved_frame_buffer_.get()) { On 2016/09/01 19:28:33, Zhenyao Mo wrote: > This is incorrect. It should be offscreen_target_frame_buffer_ for > DRAW_FRAMEBUFFER. Done. https://codereview.chromium.org/2286593002/diff/300001/gpu/command_buffer/ser... File gpu/command_buffer/service/gles2_cmd_srgb_converter.cc (right): https://codereview.chromium.org/2286593002/diff/300001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_srgb_converter.cc:365: dstX0, dstY0, dstX1, dstY1, mask, filter); On 2016/09/01 19:28:33, Zhenyao Mo wrote: > I still think this is incorrect. > > At glCopyTexImage2D, the texture (srgb_decoder_textures_[0]) size is no longer > the full size, but the clipped size. > > At glDawArrays, the texture (srgb_decoder_textures_[1]) also is the clipped > size. > > Then you blit, let's say the clipped size is (cw, ch), and full size is (w, h), > > Now look at the target texture, [0-cw, 0-ch] are from the > srgb_decoder_textures_[1], and [cw-w, ch-h] are from the border (because blit > acts like CLAMP_TO_EDGE) > > But that may not always be the case. for example, srcX0 can be out of bounds, > so for the target texture, the first a few pixels per line should be clamped to > the left edge. > > Am I explaining the situation clearly? Got it. You are correct. This algorithm would make it never clamp to the left edge and the bottom edge. CLAMP_TO_EDGE would always take effect on the top and right edge of the intersected read rectangle. The latest patchset has revised this. Well, I will write a conformance test to cover all possible situations when bliting outside of read framebuffer. Stay tuned. https://codereview.chromium.org/2286593002/diff/300001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_srgb_converter.cc:458: dstX0, dstY0, dstX1, dstY1, mask, filter); On 2016/09/01 19:28:33, Zhenyao Mo wrote: > Same situation here. Done.
Please see the conformance test at https://github.com/KhronosGroup/WebGL/pull/2010 to test blitframebuffer when bliting outside of read framebuffer.
lgtm with one request to expand the tests. Thanks for working on this challenging task. https://codereview.chromium.org/2286593002/diff/340001/gpu/command_buffer/ser... File gpu/command_buffer/service/gles2_cmd_srgb_converter.cc (right): https://codereview.chromium.org/2286593002/diff/340001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_srgb_converter.cc:368: srcY0 < srcY1 ? height - yoffset : 0 - yoffset, Can you also expand your new conformance test to cover the cases where srcX0 > srcX1 and srcY0 > srcY1?
piman: can you also take a look? I feel a second pair of eyes is necessary for this CL.
Thanks for your review, Zhenyao. https://codereview.chromium.org/2286593002/diff/340001/gpu/command_buffer/ser... File gpu/command_buffer/service/gles2_cmd_srgb_converter.cc (right): https://codereview.chromium.org/2286593002/diff/340001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_srgb_converter.cc:368: srcY0 < srcY1 ? height - yoffset : 0 - yoffset, On 2016/09/06 22:30:11, Zhenyao Mo wrote: > Can you also expand your new conformance test to cover the cases where srcX0 > > srcX1 and srcY0 > srcY1? Sure, see the patch at https://github.com/KhronosGroup/WebGL/pull/2013
https://codereview.chromium.org/2286593002/diff/340001/gpu/command_buffer/ser... File gpu/command_buffer/service/gles2_cmd_decoder.cc (right): https://codereview.chromium.org/2286593002/diff/340001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_decoder.cc:7651: if (draw_buffers_has_srgb) { What if both read and draw buffers have srgb? https://codereview.chromium.org/2286593002/diff/340001/gpu/command_buffer/ser... File gpu/command_buffer/service/gles2_cmd_srgb_converter.cc (right): https://codereview.chromium.org/2286593002/diff/340001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_srgb_converter.cc:75: // it will not convert linear to srgb automatically. 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? https://codereview.chromium.org/2286593002/diff/340001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_srgb_converter.cc:93: "\n" */ nit: remove commented code. https://codereview.chromium.org/2286593002/diff/340001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_srgb_converter.cc:97: //" output_color = vec4(decode(c.r), decode(c.g), decode(c.b), c.a);\n" nit: remove commented code. https://codereview.chromium.org/2286593002/diff/340001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_srgb_converter.cc:125: // Use nearest, non-mipmapped sampling with the srgb decoder texture nit: you're using linear, not nearest. Fix comment? https://codereview.chromium.org/2286593002/diff/340001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_srgb_converter.cc:171: "}\n"; This is the same VS as above for the decoder, can we share? https://codereview.chromium.org/2286593002/diff/340001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_srgb_converter.cc:185: "float encode(float color)\n" 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)? https://codereview.chromium.org/2286593002/diff/340001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_srgb_converter.cc:229: // Use nearest, non-mipmapped sampling with the srgb encoder texture nit: ditto, nearest->linear https://codereview.chromium.org/2286593002/diff/340001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_srgb_converter.cc:300: GLenum dst_framebuffer_type) { 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). https://codereview.chromium.org/2286593002/diff/340001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_srgb_converter.cc:328: // render the converted (linear to srgb) result to. nit: you're converting srgb to linear, not linear to srgb. https://codereview.chromium.org/2286593002/diff/340001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_srgb_converter.cc:399: GLenum dst_framebuffer_type) { ditto, worth a high-level comment about what we do here. https://codereview.chromium.org/2286593002/diff/340001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_srgb_converter.cc:462: glBlitFramebuffer(srcX0 < srcX1 ? 0 - xoffset : width - xoffset, 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).
The CQ bit was checked by yunchao.he@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
Patchset #12 (id:360001) has been deleted
The CQ bit was checked by yunchao.he@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thanks for your careful review, piman. Code has been updated, please take another look. Thanks a lot! https://codereview.chromium.org/2286593002/diff/340001/gpu/command_buffer/ser... File gpu/command_buffer/service/gles2_cmd_decoder.cc (right): https://codereview.chromium.org/2286593002/diff/340001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_decoder.cc:7651: if (draw_buffers_has_srgb) { On 2016/09/07 23:08:32, piman wrote: > What if both read and draw buffers have srgb? I would do this in follow-up changes. If both read and draw buffers have srgb image, we would convert srgb to linear, then blit, then convert blit-ed linear image to srgb. Look into the code, it would call SRGBToLinear and then call LinearToSRGB. Optionally, I can write a new function SRGBToSRGB(...). The later solution can have better performance, I suppose. A potential optimization may can be applied when the src region has exactly the same size with the dst region. We may can blit directly from srgb read buffe to srgb draw buffers. I will write conformance test and revisit this later. https://codereview.chromium.org/2286593002/diff/340001/gpu/command_buffer/ser... File gpu/command_buffer/service/gles2_cmd_srgb_converter.cc (right): https://codereview.chromium.org/2286593002/diff/340001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_srgb_converter.cc:75: // it will not convert linear to srgb automatically. On 2016/09/07 23:08:33, piman wrote: > 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? 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. https://codereview.chromium.org/2286593002/diff/340001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_srgb_converter.cc:93: "\n" */ On 2016/09/07 23:08:32, piman wrote: > nit: remove commented code. I would like to keep the code here, which indicate that we are aware of the conversion from srgb to linear. https://codereview.chromium.org/2286593002/diff/340001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_srgb_converter.cc:97: //" output_color = vec4(decode(c.r), decode(c.g), decode(c.b), c.a);\n" On 2016/09/07 23:08:32, piman wrote: > nit: remove commented code. I'd like to keep the code here. https://codereview.chromium.org/2286593002/diff/340001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_srgb_converter.cc:125: // Use nearest, non-mipmapped sampling with the srgb decoder texture On 2016/09/07 23:08:33, piman wrote: > nit: you're using linear, not nearest. Fix comment? Done. https://codereview.chromium.org/2286593002/diff/340001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_srgb_converter.cc:171: "}\n"; On 2016/09/07 23:08:32, piman wrote: > This is the same VS as above for the decoder, can we share? That's true. Done. https://codereview.chromium.org/2286593002/diff/340001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_srgb_converter.cc:185: "float encode(float color)\n" On 2016/09/07 23:08:33, piman wrote: > 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)? 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. https://codereview.chromium.org/2286593002/diff/340001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_srgb_converter.cc:229: // Use nearest, non-mipmapped sampling with the srgb encoder texture On 2016/09/07 23:08:32, piman wrote: > nit: ditto, nearest->linear Done. https://codereview.chromium.org/2286593002/diff/340001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_srgb_converter.cc:300: GLenum dst_framebuffer_type) { On 2016/09/07 23:08:33, piman wrote: > 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). Done. https://codereview.chromium.org/2286593002/diff/340001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_srgb_converter.cc:300: GLenum dst_framebuffer_type) { On 2016/09/07 23:08:33, piman wrote: > 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) > Good suggestions! Done. > > 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). 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. https://codereview.chromium.org/2286593002/diff/340001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_srgb_converter.cc:328: // render the converted (linear to srgb) result to. On 2016/09/07 23:08:32, piman wrote: > nit: you're converting srgb to linear, not linear to srgb. Done. https://codereview.chromium.org/2286593002/diff/340001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_srgb_converter.cc:399: GLenum dst_framebuffer_type) { On 2016/09/07 23:08:33, piman wrote: > ditto, worth a high-level comment about what we do here. Done. https://codereview.chromium.org/2286593002/diff/340001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_srgb_converter.cc:462: glBlitFramebuffer(srcX0 < srcX1 ? 0 - xoffset : width - xoffset, On 2016/09/07 23:08:32, piman wrote: > 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). 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.
https://codereview.chromium.org/2286593002/diff/340001/gpu/command_buffer/ser... File gpu/command_buffer/service/gles2_cmd_decoder.cc (right): https://codereview.chromium.org/2286593002/diff/340001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_decoder.cc:7651: if (draw_buffers_has_srgb) { On 2016/09/08 18:32:49, yunchao wrote: > On 2016/09/07 23:08:32, piman wrote: > > What if both read and draw buffers have srgb? > I would do this in follow-up changes. > > If both read and draw buffers have srgb image, we would convert srgb to linear, > then blit, then convert blit-ed linear image to srgb. Look into the code, it > would call SRGBToLinear and then call LinearToSRGB. How does that work? SRGBToLinear blits from the read FB to the draw FB. LinearToSRGB also blits from the read FB to the draw FB. So right now it would do a first incorrect blit, and then overwrite that with a second incorrect blit. > Optionally, I can write a > new function SRGBToSRGB(...). The later solution can have better performance, I > suppose. > > A potential optimization may can be applied when the src region has exactly the > same size with the dst region. We may can blit directly from srgb read buffe to > srgb draw buffers. I will write conformance test and revisit this later. https://codereview.chromium.org/2286593002/diff/340001/gpu/command_buffer/ser... File gpu/command_buffer/service/gles2_cmd_srgb_converter.cc (right): https://codereview.chromium.org/2286593002/diff/340001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_srgb_converter.cc:75: // it will not convert linear to srgb automatically. On 2016/09/08 18:32:49, yunchao wrote: > On 2016/09/07 23:08:33, piman wrote: > > 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? > > 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. 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. > > Well, I have updated the comments here. Hope that can state this clearly. https://codereview.chromium.org/2286593002/diff/340001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_srgb_converter.cc:93: "\n" */ On 2016/09/08 18:32:49, yunchao wrote: > On 2016/09/07 23:08:32, piman wrote: > > nit: remove commented code. > > I would like to keep the code here, which indicate that we are aware of the > conversion from srgb to linear. Please don't. It makes it confusing. The conversion is well explained in the spec. https://codereview.chromium.org/2286593002/diff/340001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_srgb_converter.cc:97: //" output_color = vec4(decode(c.r), decode(c.g), decode(c.b), c.a);\n" On 2016/09/08 18:32:49, yunchao wrote: > On 2016/09/07 23:08:32, piman wrote: > > nit: remove commented code. > > I'd like to keep the code here. Please don't. https://codereview.chromium.org/2286593002/diff/340001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_srgb_converter.cc:185: "float encode(float color)\n" On 2016/09/08 18:32:49, yunchao wrote: > On 2016/09/07 23:08:33, piman wrote: > > 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)? > > 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. 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. https://codereview.chromium.org/2286593002/diff/380001/gpu/command_buffer/ser... File gpu/command_buffer/service/gles2_cmd_srgb_converter.cc (right): https://codereview.chromium.org/2286593002/diff/380001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_srgb_converter.cc:36: const char* vs_source = nit: static, as to not pollute the namespace.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
@piman, thanks for your review. I explained my concerns for your comments. May be I am wrong, please correct me if so. Thanks a lot! https://codereview.chromium.org/2286593002/diff/340001/gpu/command_buffer/ser... File gpu/command_buffer/service/gles2_cmd_decoder.cc (right): https://codereview.chromium.org/2286593002/diff/340001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_decoder.cc:7651: if (draw_buffers_has_srgb) { On 2016/09/08 19:39:17, piman wrote: > On 2016/09/08 18:32:49, yunchao wrote: > > On 2016/09/07 23:08:32, piman wrote: > > > What if both read and draw buffers have srgb? > > I would do this in follow-up changes. > > > > If both read and draw buffers have srgb image, we would convert srgb to > linear, > > then blit, then convert blit-ed linear image to srgb. Look into the code, it > > would call SRGBToLinear and then call LinearToSRGB. > > How does that work? SRGBToLinear blits from the read FB to the draw FB. > LinearToSRGB also blits from the read FB to the draw FB. > So right now it would do a first incorrect blit, and then overwrite that with a > second incorrect blit. Yes, the current implementation is not correct here. It can pass the conformance test, only because that the conformance tests have the same size (both width and height) for src region and dst region when blit from srgb to srgb. So We have 2 solutions to resolve this issue. 1) create a temp fbo with linear texture as color buffer, then call SRGBToLinear to read from the source srgb image and draw to the temp fbo. After that, call LinearToSRGB to read from that temp fbo and draw to the target srgb image. 2) create a new function in srgb_converter, say SRGBToSRGB. then we can blit srgb to srgb in one shot, instead of call SRGBToLinear and then call LinearToSRGB. We can do potential optimizations. Apparently, solution 2) is better from the perspective of performance. However, here is a much more complicated scenario: what if drawbuffers have both linear images and srgb images? So I want to put that work in follow-up change after make a thorough thinking: Which is really better. So I add them into TODOs and fix them later. WDYT? If you want to impl the srgb to srgb bliting first, It is OK. I will impl srgb->srgb bliting in the next patchset. > > > Optionally, I can write a > > new function SRGBToSRGB(...). The later solution can have better performance, > I > > suppose. > > > > A potential optimization may can be applied when the src region has exactly > the > > same size with the dst region. We may can blit directly from srgb read buffe > to > > srgb draw buffers. I will write conformance test and revisit this later. > https://codereview.chromium.org/2286593002/diff/340001/gpu/command_buffer/ser... File gpu/command_buffer/service/gles2_cmd_srgb_converter.cc (right): https://codereview.chromium.org/2286593002/diff/340001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_srgb_converter.cc:75: // it will not convert linear to srgb automatically. On 2016/09/08 19:39:18, piman wrote: > On 2016/09/08 18:32:49, yunchao wrote: > > On 2016/09/07 23:08:33, piman wrote: > > > 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? > > > > 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. > > 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). (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. > > The code in GLES2Decoder explicitly disables GL_FRAMEBUFFER_SRGB before calling > these functions, so that's why the linear->sRGB conversion doesn't happen. 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. > > > > > Well, I have updated the comments here. Hope that can state this clearly. > https://codereview.chromium.org/2286593002/diff/340001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_srgb_converter.cc:93: "\n" */ On 2016/09/08 19:39:17, piman wrote: > On 2016/09/08 18:32:49, yunchao wrote: > > On 2016/09/07 23:08:32, piman wrote: > > > nit: remove commented code. > > > > I would like to keep the code here, which indicate that we are aware of the > > conversion from srgb to linear. > > Please don't. It makes it confusing. The conversion is well explained in the > spec. (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. https://codereview.chromium.org/2286593002/diff/340001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_srgb_converter.cc:185: "float encode(float color)\n" On 2016/09/08 19:39:18, piman wrote: > On 2016/09/08 18:32:49, yunchao wrote: > > On 2016/09/07 23:08:33, piman wrote: > > > 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)? > > > > 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. > > 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. 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.
I think there is a major confusion about how sRGB works, hence why this is more complicated than it needs to be. The shader always operates in linear space. There is no concept of linear->sRGB conversion at sampling time - it wouldn't make sense anyway, because filtering would be wrong. During texture sampling, either the source texture is one of the sRGB formats, in which case each texture sample is then converted to linear before filtering, or it is a linear format and no conversion occurs. Either way, data ends up in linear format before filtering, is filtered in linear space, and ends up as a linear float in the shader. This is what the ES3's "3.8.16 sRGB Texture Color Conversion" section (and corresponding section in desktop GL specs) refers to. The shader operates on the texture value in linear space - which allows it to perform linear operations such as lerps etc. It also outputs the color in linear space. At the back-end of the pipeline, the linear color value may then be converted to sRGB before writing to the corresponding attachment if: - the attachment is in one of the sRGB formats (equivalently, GL_FRAMEBUFFER_ATTACHMENT_COLOR_ENCODING is SRGB for that attachment) - and, if on desktop GL, GL_FRAMEBUFFER_SRGB is true. This last part is what the ES3's "4.1.8 sRGB Conversion" (and corresponding section in desktop GL specs) section refers to. This happens fully independently of blending (separately, sRGB also affects blending, that is explained in the ES3 "4.1.7 Blending" section, because the sample values from a sRGB attachment must be converted to linear before blending with the shader's output value, but that happens before the blended linear value is then converted back to sRGB). So, provided GL_FRAMEBUFFER_SRGB is enabled (on desktop), a single shader that reads a texture and directly outputs its value to its attachment (with no explicit conversion) can perfectly handle: - linear input and linear output (no conversion) - sRGB input and linear output (sampling decodes from sRGB to linear, which is passed to the output color, which is then written directly to attachment) - linear input and sRGB output (sampling gives linear value to shader, which is passed to the ouput color, which is converted to sRGB before writing to the attachment) - sRGB input and sRGB output (sampling decodes from sRGB to linear, which is passed to the output color, which is converted to sRGB before writing to the attachment) On Thu, Sep 8, 2016 at 4:16 PM, <yunchao.he@intel.com> wrote: > @piman, thanks for your review. I explained my concerns for your comments. > May > be I am wrong, please correct me if so. > > Thanks a lot! > > > https://codereview.chromium.org/2286593002/diff/340001/ > gpu/command_buffer/service/gles2_cmd_decoder.cc > File gpu/command_buffer/service/gles2_cmd_decoder.cc (right): > > https://codereview.chromium.org/2286593002/diff/340001/ > gpu/command_buffer/service/gles2_cmd_decoder.cc#newcode7651 > gpu/command_buffer/service/gles2_cmd_decoder.cc:7651: if > (draw_buffers_has_srgb) { > On 2016/09/08 19:39:17, piman wrote: > > On 2016/09/08 18:32:49, yunchao wrote: > > > On 2016/09/07 23:08:32, piman wrote: > > > > What if both read and draw buffers have srgb? > > > I would do this in follow-up changes. > > > > > > If both read and draw buffers have srgb image, we would convert srgb > to > > linear, > > > then blit, then convert blit-ed linear image to srgb. Look into the > code, it > > > would call SRGBToLinear and then call LinearToSRGB. > > > > How does that work? SRGBToLinear blits from the read FB to the draw > FB. > > LinearToSRGB also blits from the read FB to the draw FB. > > So right now it would do a first incorrect blit, and then overwrite > that with a > > second incorrect blit. > > Yes, the current implementation is not correct here. It can pass the > conformance test, only because that the conformance tests have the same > size (both width and height) for src region and dst region when blit > from srgb to srgb. > So We have 2 solutions to resolve this issue. > 1) create a temp fbo with linear texture as color buffer, then call > SRGBToLinear to read from the source srgb image and draw to the temp > fbo. After that, call LinearToSRGB to read from that temp fbo and draw > to the target srgb image. > 2) create a new function in srgb_converter, say SRGBToSRGB. then we can > blit srgb to srgb in one shot, instead of call SRGBToLinear and then > call LinearToSRGB. We can do potential optimizations. > Apparently, solution 2) is better from the perspective of performance. > However, here is a much more complicated scenario: what if drawbuffers > have both linear images and srgb images? So I want to put that work in > follow-up change after make a thorough thinking: Which is really better. > So I add them into TODOs and fix them later. > WDYT? If you want to impl the srgb to srgb bliting first, It is OK. I > will impl srgb->srgb bliting in the next patchset. > > > > > > Optionally, I can write a > > > new function SRGBToSRGB(...). The later solution can have better > performance, > > I > > > suppose. > > > > > > A potential optimization may can be applied when the src region has > exactly > > the > > > same size with the dst region. We may can blit directly from srgb > read buffe > > to > > > srgb draw buffers. I will write conformance test and revisit this > later. > > > > https://codereview.chromium.org/2286593002/diff/340001/ > gpu/command_buffer/service/gles2_cmd_srgb_converter.cc > File gpu/command_buffer/service/gles2_cmd_srgb_converter.cc (right): > > https://codereview.chromium.org/2286593002/diff/340001/ > gpu/command_buffer/service/gles2_cmd_srgb_converter.cc#newcode75 > gpu/command_buffer/service/gles2_cmd_srgb_converter.cc:75: // it will > not convert linear to srgb automatically. > On 2016/09/08 19:39:18, piman wrote: > > On 2016/09/08 18:32:49, yunchao wrote: > > > On 2016/09/07 23:08:33, piman wrote: > > > > 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? > > > > > > 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=section-3.8.16, > > > 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. > > > > 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). > > (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. > > > > > The code in GLES2Decoder explicitly disables GL_FRAMEBUFFER_SRGB > before calling > > these functions, so that's why the linear->sRGB conversion doesn't > happen. > 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. > > > > > > > > > Well, I have updated the comments here. Hope that can state this > clearly. > > > > https://codereview.chromium.org/2286593002/diff/340001/ > gpu/command_buffer/service/gles2_cmd_srgb_converter.cc#newcode93 > gpu/command_buffer/service/gles2_cmd_srgb_converter.cc:93: "\n" */ > On 2016/09/08 19:39:17, piman wrote: > > On 2016/09/08 18:32:49, yunchao wrote: > > > On 2016/09/07 23:08:32, piman wrote: > > > > nit: remove commented code. > > > > > > I would like to keep the code here, which indicate that we are aware > of the > > > conversion from srgb to linear. > > > > Please don't. It makes it confusing. The conversion is well explained > in the > > spec. > > (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. > > https://codereview.chromium.org/2286593002/diff/340001/ > gpu/command_buffer/service/gles2_cmd_srgb_converter.cc#newcode185 > gpu/command_buffer/service/gles2_cmd_srgb_converter.cc:185: "float > encode(float color)\n" > On 2016/09/08 19:39:18, piman wrote: > > On 2016/09/08 18:32:49, yunchao wrote: > > > On 2016/09/07 23:08:33, piman wrote: > > > > 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)? > > > > > > 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. > > > > 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. > > 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. > > https://codereview.chromium.org/2286593002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Excellent explanation about how sRGB works! I got it finally. It is much clear now. Thank your very much for your kind and deep explanation, @piman! I really appreciate it. I will update the code today. On 2016/09/08 23:52:29, piman wrote: > I think there is a major confusion about how sRGB works, hence why this is > more complicated than it needs to be. > > The shader always operates in linear space. There is no concept of > linear->sRGB conversion at sampling time - it wouldn't make sense anyway, > because filtering would be wrong. > > During texture sampling, either the source texture is one of the sRGB > formats, in which case each texture sample is then converted to linear > before filtering, or it is a linear format and no conversion occurs. Either > way, data ends up in linear format before filtering, is filtered in linear > space, and ends up as a linear float in the shader. This is what the ES3's > "3.8.16 sRGB Texture Color Conversion" section (and corresponding section > in desktop GL specs) refers to. > > The shader operates on the texture value in linear space - which allows it > to perform linear operations such as lerps etc. It also outputs the color > in linear space. > > At the back-end of the pipeline, the linear color value may then be > converted to sRGB before writing to the corresponding attachment if: > - the attachment is in one of the sRGB formats (equivalently, > GL_FRAMEBUFFER_ATTACHMENT_COLOR_ENCODING is SRGB for that attachment) > - and, if on desktop GL, GL_FRAMEBUFFER_SRGB is true. > > This last part is what the ES3's "4.1.8 sRGB Conversion" (and corresponding > section in desktop GL specs) section refers to. This happens fully > independently of blending (separately, sRGB also affects blending, that is > explained in the ES3 "4.1.7 Blending" section, because the sample values > from a sRGB attachment must be converted to linear before blending with the > shader's output value, but that happens before the blended linear value is > then converted back to sRGB). > > So, provided GL_FRAMEBUFFER_SRGB is enabled (on desktop), a single shader > that reads a texture and directly outputs its value to its attachment (with > no explicit conversion) can perfectly handle: > - linear input and linear output (no conversion) > - sRGB input and linear output (sampling decodes from sRGB to linear, which > is passed to the output color, which is then written directly to attachment) > - linear input and sRGB output (sampling gives linear value to shader, > which is passed to the ouput color, which is converted to sRGB before > writing to the attachment) > - sRGB input and sRGB output (sampling decodes from sRGB to linear, which > is passed to the output color, which is converted to sRGB before writing to > the attachment)
The CQ bit was checked by yunchao.he@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by yunchao.he@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Patchset #14 (id:420001) has been deleted
@piman (and all), I have updated the code to support bliting from srgb read buffer to srgb draw buffer. Could you take another look? Thanks a lot!
On 2016/09/09 14:34:58, yunchao wrote: > @piman (and all), I have updated the code to support bliting from srgb read > buffer to srgb draw buffer. > > Could you take another look? Thanks a lot! @piman and all, the test case to cover filtering srgb image(which means the width and height of src region are different from those in dst region) has been submitted at https://github.com/KhronosGroup/WebGL/pull/2023. With this patch applied, that conformance test can pass on MacOSX. Without this patch, it would fail.
https://codereview.chromium.org/2286593002/diff/440001/gpu/command_buffer/ser... File gpu/command_buffer/service/gles2_cmd_decoder.cc (right): https://codereview.chromium.org/2286593002/diff/440001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_decoder.cc:7727: state_.EnableDisableFramebufferSRGB(false); Once more. Why do you do disable GL_FRAMEBUFFER_SRGB here and do the conversion in the shader? Why not leaving GL_FRAMEBUFFER_SRGB enabled and use a trivial fragment shader?
The CQ bit was checked by yunchao.he@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Code has been updated per your suggestion, @piman. Could you take another look. Thanks a lot! (Please see the comments inline below. I am not quite sure whether I understand your suggestion correctly.) https://codereview.chromium.org/2286593002/diff/440001/gpu/command_buffer/ser... File gpu/command_buffer/service/gles2_cmd_decoder.cc (right): https://codereview.chromium.org/2286593002/diff/440001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_decoder.cc:7727: state_.EnableDisableFramebufferSRGB(false); On 2016/09/13 00:39:23, piman OOO back 2016-09-12 wrote: > Once more. Why do you do disable GL_FRAMEBUFFER_SRGB here and do the conversion > in the shader? > Why not leaving GL_FRAMEBUFFER_SRGB enabled and use a trivial fragment shader? I am not sure whether I understand you correctly. For desktop OGL whose version < 4.4, the GL_FRAMEBUFFER_SRGB flag doesn't apply to blitFramebuffer to decode srgb to linear (It might still apply to other APIs). For desktop OGL whose version < 4.2, the GL_FRAMEBUFFER_SRGB flag doesn't apply to blitFramebuffer to encode linear to srgb. So we need to build a particular shader to do the srgb conversion for these situations. Per zmo@'s suggestion, we can do the srgb conversion by shader when OGL version < 4.4 to unify decode/encode. However, there is another solution slightly different: use shader to decode srgb to linear when OGL version < 4.4, and use shader to encode linear to srgb when OGL version < 4.2. For all other situations, we use blitFramebuffe directly (there is no trivial shader too), and enable FRAMEBUFFER_SRGB flag for desktop OGL. Hope that I understand your suggestion correctly. Code has been updated accordingly. For srgb to srgb bliting, who need both decode and encode, I would like to use shader when desktop OGL version < 4.4.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_optional_gpu_tests_rel on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_optional_gpu_...)
On Tue, Sep 13, 2016 at 1:37 AM, <yunchao.he@intel.com> wrote: > > Code has been updated per your suggestion, @piman. Could you take another > look. > Thanks a lot! > > (Please see the comments inline below. I am not quite sure whether I > understand > your suggestion correctly.) > > > https://codereview.chromium.org/2286593002/diff/440001/ > gpu/command_buffer/service/gles2_cmd_decoder.cc > File gpu/command_buffer/service/gles2_cmd_decoder.cc (right): > > https://codereview.chromium.org/2286593002/diff/440001/ > gpu/command_buffer/service/gles2_cmd_decoder.cc#newcode7727 > gpu/command_buffer/service/gles2_cmd_decoder.cc:7727: > state_.EnableDisableFramebufferSRGB(false); > On 2016/09/13 00:39:23, piman OOO back 2016-09-12 wrote: > > Once more. Why do you do disable GL_FRAMEBUFFER_SRGB here and do the > conversion > > in the shader? > > Why not leaving GL_FRAMEBUFFER_SRGB enabled and use a trivial fragment > shader? > > I am not sure whether I understand you correctly. For desktop OGL whose > version < 4.4, the GL_FRAMEBUFFER_SRGB flag doesn't apply to > blitFramebuffer to decode srgb to linear (It might still apply to other > APIs). For desktop OGL whose version < 4.2, the GL_FRAMEBUFFER_SRGB flag > doesn't apply to blitFramebuffer to encode linear to srgb. > Yes, and that's not in question here. > So we need to > build a particular shader to do the srgb conversion for these > situations. > No, you don't need a shader to do sRGB conversion. You need a draw (as opposed to a blit), but that can be done with a trivial shader, ***provided you leave GL_FRAMEBUFFER_SRGB enabled***. If GL_FRAMEBUFFER_SRGB is on, the fixed function pipeline will do linear->sRGB conversion when you do a draw. I'm not sure how to describe this any better. > > Per zmo@'s suggestion, we can do the srgb conversion by shader when OGL > version < 4.4 to unify decode/encode. > > However, there is another solution slightly different: use shader to > decode srgb to linear when OGL version < 4.4, and use shader to encode > linear to srgb when OGL version < 4.2. > > For all other situations, we use blitFramebuffe directly (there is no > trivial shader too), and enable FRAMEBUFFER_SRGB flag for desktop OGL. > > Hope that I understand your suggestion correctly. Code has been updated > accordingly. > > For srgb to srgb bliting, who need both decode and encode, I would like > to use shader when desktop OGL version < 4.4. > > https://codereview.chromium.org/2286593002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
The CQ bit was checked by yunchao.he@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2016/09/13 16:16:20, piman OOO back 2016-09-12 wrote: > On Tue, Sep 13, 2016 at 1:37 AM, <mailto:yunchao.he@intel.com> wrote: > > > > > Code has been updated per your suggestion, @piman. Could you take another > > look. > > Thanks a lot! > > > > (Please see the comments inline below. I am not quite sure whether I > > understand > > your suggestion correctly.) > > > > > > https://codereview.chromium.org/2286593002/diff/440001/ > > gpu/command_buffer/service/gles2_cmd_decoder.cc > > File gpu/command_buffer/service/gles2_cmd_decoder.cc (right): > > > > https://codereview.chromium.org/2286593002/diff/440001/ > > gpu/command_buffer/service/gles2_cmd_decoder.cc#newcode7727 > > gpu/command_buffer/service/gles2_cmd_decoder.cc:7727: > > state_.EnableDisableFramebufferSRGB(false); > > On 2016/09/13 00:39:23, piman OOO back 2016-09-12 wrote: > > > Once more. Why do you do disable GL_FRAMEBUFFER_SRGB here and do the > > conversion > > > in the shader? > > > Why not leaving GL_FRAMEBUFFER_SRGB enabled and use a trivial fragment > > shader? > > > > I am not sure whether I understand you correctly. For desktop OGL whose > > version < 4.4, the GL_FRAMEBUFFER_SRGB flag doesn't apply to > > blitFramebuffer to decode srgb to linear (It might still apply to other > > APIs). For desktop OGL whose version < 4.2, the GL_FRAMEBUFFER_SRGB flag > > doesn't apply to blitFramebuffer to encode linear to srgb. > > > > Yes, and that's not in question here. > > > > So we need to > > build a particular shader to do the srgb conversion for these > > situations. > > > > No, you don't need a shader to do sRGB conversion. You need a draw (as > opposed to a blit), but that can be done with a trivial shader, ***provided > you leave GL_FRAMEBUFFER_SRGB enabled***. If GL_FRAMEBUFFER_SRGB is on, the > fixed function pipeline will do linear->sRGB conversion when you do a draw. > That's true. And the trivial fragment shader to encode linear to srgb is exactly the same with the fs to decode srgb to linear. So these two programs(srgb_decoder_program and srgb_encoder_program) can be combined into one. Code has been updated accordingly. Please take a look.
I came across a corner case for srgb decoding, do you have any suggestions, @piman? Please see the details inline. https://codereview.chromium.org/2286593002/diff/500001/gpu/command_buffer/ser... File gpu/command_buffer/service/gles2_cmd_srgb_converter.cc (right): https://codereview.chromium.org/2286593002/diff/500001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_srgb_converter.cc:251: glCopyTexImage2D(GL_TEXTURE_2D, 0, src_framebuffer_internal_format, If the read buffer is a multisampled renderbuffer with srgb color format, then this code would copy from a msaa renderbuffer to a texture. It seems to be wrong (any gles spec about this?). Do you have any suggestion to handle this corner case, @piman.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_optional_gpu_tests_rel on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_optional_gpu_...)
Thanks, this looks better already. I would like to try to reduce the code duplication, see suggestion below. https://codereview.chromium.org/2286593002/diff/500001/gpu/command_buffer/ser... File gpu/command_buffer/service/gles2_cmd_srgb_converter.cc (right): https://codereview.chromium.org/2286593002/diff/500001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_srgb_converter.cc:251: glCopyTexImage2D(GL_TEXTURE_2D, 0, src_framebuffer_internal_format, On 2016/09/14 16:11:33, yunchao wrote: > If the read buffer is a multisampled renderbuffer with srgb color format, then > this code would copy from a msaa renderbuffer to a texture. It seems to be wrong > (any gles spec about this?). > Do you have any suggestion to handle this corner case, @piman. I'm pretty sure that's the intended behavior - it would do the MSAA resolve, just like glBlitFramebuffer would. https://codereview.chromium.org/2286593002/diff/500001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_srgb_converter.cc:414: // During this step, color space is converted from linear to srgb. The 2 other functions could be considered a simplification of this one. SRGBToLinear skips step 4 (with 3 directly blitting to the destination FBO), and LinearToSRGB skips steps 1 and 2 (with 3 directly blitting from the source FBO). Could we unify? This would significantly reduce code duplication, both here and on the caller side - you could have a single SRGBConverter::Blit(bool needs_srgb_decode, bool needs_srgb_encode, ...) Relatedly, the conceptual separation of "srgb decoder" vs "srgb encoder" is a bit artificial at this point - they use the same shader, and many of the resources could be shared (e.g. vao, temporary textures, temporary FBO). Especially the textures, since we reallocate the images every time anyway (glCopyTexImage2D/glTexImage2D), there's not a lot of gains to be had by keeping them separate for the encoder step and the decoder step. Note, you only really need 2 textures overall, since step 3 could reuse texture 1 for the destination
The CQ bit was checked by yunchao.he@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
Patchset #20 (id:560001) has been deleted
Patchset #20 (id:580001) has been deleted
Patchset #20 (id:600001) has been deleted
The CQ bit was checked by yunchao.he@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thanks for your review, @piman. I have combined all srgb converters into one function (SRGBConverter::Blit). Please take another look. Thanks a lot! https://codereview.chromium.org/2286593002/diff/500001/gpu/command_buffer/ser... File gpu/command_buffer/service/gles2_cmd_srgb_converter.cc (right): https://codereview.chromium.org/2286593002/diff/500001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_srgb_converter.cc:251: glCopyTexImage2D(GL_TEXTURE_2D, 0, src_framebuffer_internal_format, On 2016/09/15 03:34:31, piman OOO back 2016-09-12 wrote: > On 2016/09/14 16:11:33, yunchao wrote: > > If the read buffer is a multisampled renderbuffer with srgb color format, then > > this code would copy from a msaa renderbuffer to a texture. It seems to be > wrong > > (any gles spec about this?). > > Do you have any suggestion to handle this corner case, @piman. > > I'm pretty sure that's the intended behavior - it would do the MSAA resolve, > just like glBlitFramebuffer would. Done. https://codereview.chromium.org/2286593002/diff/500001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_srgb_converter.cc:414: // During this step, color space is converted from linear to srgb. On 2016/09/15 03:34:31, piman OOO back 2016-09-12 wrote: > The 2 other functions could be considered a simplification of this one. > SRGBToLinear skips step 4 (with 3 directly blitting to the destination FBO), and > LinearToSRGB skips steps 1 and 2 (with 3 directly blitting from the source FBO). > > Could we unify? This would significantly reduce code duplication, both here and > on the caller side - you could have a single SRGBConverter::Blit(bool > needs_srgb_decode, bool needs_srgb_encode, ...) Done. Very good idea to remove the code duplication. > > Relatedly, the conceptual separation of "srgb decoder" vs "srgb encoder" is a > bit artificial at this point - they use the same shader, and many of the > resources could be shared (e.g. vao, temporary textures, temporary FBO). > Especially the textures, since we reallocate the images every time anyway > (glCopyTexImage2D/glTexImage2D), there's not a lot of gains to be had by keeping > them separate for the encoder step and the decoder step. Note, you only really > need 2 textures overall, since step 3 could reuse texture 1 for the destination I have thought about this. But I didn't reuse the texture 1. You know, it is necessary to get parameters need_encode and need_decode from the caller. So, I think keep the logic "srgb encoder" and "srgb decoder" would make the code clear: srgb -- (decode) -- linear -- (blit) -- linear -- (encode) -- srgb. Moreover, during the blit, we need two fbos, I named them decoder_fbo and encoder_fbo. If encoder_fbo's image (texture 3) re-use the one by decoder_fbo's image (texture 1). It is seems to be in a mess when read the code. After combine all srgb converters into one function, I think the code is already complicated now. WDYT, @piman. If you think it is better to reuse the texture 1, I will update the code accordingly.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by yunchao.he@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
@piman, I have updated the code as your suggestion: unify and share resources (textures, vao, etc). I think it is OK now. Please take another look. Thanks a lot! https://codereview.chromium.org/2286593002/diff/500001/gpu/command_buffer/ser... File gpu/command_buffer/service/gles2_cmd_srgb_converter.cc (right): https://codereview.chromium.org/2286593002/diff/500001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_srgb_converter.cc:414: // During this step, color space is converted from linear to srgb. On 2016/09/16 06:20:52, yunchao wrote: > On 2016/09/15 03:34:31, piman OOO back 2016-09-12 wrote: > > The 2 other functions could be considered a simplification of this one. > > SRGBToLinear skips step 4 (with 3 directly blitting to the destination FBO), > and > > LinearToSRGB skips steps 1 and 2 (with 3 directly blitting from the source > FBO). > > > > Could we unify? This would significantly reduce code duplication, both here > and > > on the caller side - you could have a single SRGBConverter::Blit(bool > > needs_srgb_decode, bool needs_srgb_encode, ...) > > Done. Very good idea to remove the code duplication. > > > > > Relatedly, the conceptual separation of "srgb decoder" vs "srgb encoder" is a > > bit artificial at this point - they use the same shader, and many of the > > resources could be shared (e.g. vao, temporary textures, temporary FBO). > > Especially the textures, since we reallocate the images every time anyway > > (glCopyTexImage2D/glTexImage2D), there's not a lot of gains to be had by > keeping > > them separate for the encoder step and the decoder step. Note, you only really > > need 2 textures overall, since step 3 could reuse texture 1 for the > destination > > I have thought about this. But I didn't reuse the texture 1. You know, it is > necessary to get parameters need_encode and need_decode from the caller. So, I > think keep the logic "srgb encoder" and "srgb decoder" would make the code > clear: > srgb -- (decode) -- linear -- (blit) -- linear -- (encode) -- srgb. > Moreover, during the blit, we need two fbos, I named them decoder_fbo and > encoder_fbo. If encoder_fbo's image (texture 3) re-use the one by decoder_fbo's > image (texture 1). It is seems to be in a mess when read the code. After > combine all srgb converters into one function, I think the code is already > complicated now. > > WDYT, @piman. If you think it is better to reuse the texture 1, I will update > the code accordingly. It is not necessary to associate texture and vao with decoder_fbo or encoder_fbo respectively. we can still have decoder_fbo and encoder_fbo, but use unified textures/vao/program, and share these resources between decoder_fbo and encoder_fbo. Code has been updated accordingly. I think it is OK now.
Thanks, this is great. LGTM after a couple of nits fixed. https://codereview.chromium.org/2286593002/diff/640001/gpu/command_buffer/ser... File gpu/command_buffer/service/gles2_cmd_decoder.cc (right): https://codereview.chromium.org/2286593002/diff/640001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_decoder.cc:7756: srgb_converter_->InitializeSRGBConverter(this); nit: maybe you can do that in the block above - since it will early-out the second time it runs. https://codereview.chromium.org/2286593002/diff/640001/gpu/command_buffer/ser... File gpu/command_buffer/service/gles2_cmd_srgb_converter.cc (right): https://codereview.chromium.org/2286593002/diff/640001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_srgb_converter.cc:286: glBindFramebufferEXT(GL_READ_FRAMEBUFFER, srgb_decoder_fbo_); nit: this is already done in the decode path (srgb_decoder_fbo_ gets bound to GL_FRAMEBUFFER, which means it's bound to both GL_READ_FRAMEBUFFER and GL_DRAW_FRAMEBUFFER). So maybe the else clause below could move to l.257, such that it goes if (decode) { // ... // [do the copy/crop] // ... glBindFramebufferEXT(GL_FRAMEBUFFER, srgb_decoder_fbo_); // ... // [do the decode draw] // ... } else { glBindFramebufferEXT(GL_READ_FRAMEBUFFER, src_framebuffer); }
geofflang@chromium.org changed reviewers: + cwallez@chromium.org
Appreciate piman thoroughly reviewing this -- sorry, just coming to this review for the first time. I defer to piman's review, but a couple of questions. https://codereview.chromium.org/2286593002/diff/640001/gpu/command_buffer/ser... File gpu/command_buffer/service/gles2_cmd_decoder.cc (right): https://codereview.chromium.org/2286593002/diff/640001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_decoder.cc:7711: read_buffer_samples > 0 || 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? https://codereview.chromium.org/2286593002/diff/640001/gpu/command_buffer/ser... File gpu/command_buffer/service/gles2_cmd_srgb_converter.cc (right): https://codereview.chromium.org/2286593002/diff/640001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_srgb_converter.cc:59: " vec2(1.0f, 1.0f)\n" Embedding the vertex positions in the vertex shader seems a little too clever -- is there a good reason to not just allocate a small buffer object and hook it up to the VAO you're already allocating?
BTW, thank you for this patch, Yunchao -- it's a nice piece of work and it looks good overall.
https://codereview.chromium.org/2286593002/diff/640001/gpu/command_buffer/ser... File gpu/command_buffer/service/gles2_cmd_decoder.cc (right): https://codereview.chromium.org/2286593002/diff/640001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_decoder.cc:7711: read_buffer_samples > 0 || On 2016/09/16 22:32:29, Ken Russell wrote: > 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? 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) https://codereview.chromium.org/2286593002/diff/640001/gpu/command_buffer/ser... File gpu/command_buffer/service/gles2_cmd_srgb_converter.cc (right): https://codereview.chromium.org/2286593002/diff/640001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_srgb_converter.cc:59: " vec2(1.0f, 1.0f)\n" On 2016/09/16 22:32:29, Ken Russell wrote: > Embedding the vertex positions in the vertex shader seems a little too clever -- > is there a good reason to not just allocate a small buffer object and hook it up > to the VAO you're already allocating? I'm actually ok with this. That's also done in the CopyTexImageResourceManager and other places. Personally, I think it is simpler, and actually likely more efficient, than managing a vertex buffer, its lifetime, its bindings, its updates.
https://codereview.chromium.org/2286593002/diff/640001/gpu/command_buffer/ser... File gpu/command_buffer/service/gles2_cmd_decoder.cc (right): https://codereview.chromium.org/2286593002/diff/640001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_decoder.cc:7711: read_buffer_samples > 0 || On 2016/09/16 22:53:29, piman OOO back 2016-09-12 wrote: > On 2016/09/16 22:32:29, Ken Russell wrote: > > 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? > > 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) 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. https://codereview.chromium.org/2286593002/diff/640001/gpu/command_buffer/ser... File gpu/command_buffer/service/gles2_cmd_srgb_converter.cc (right): https://codereview.chromium.org/2286593002/diff/640001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_srgb_converter.cc:59: " vec2(1.0f, 1.0f)\n" On 2016/09/16 22:53:29, piman OOO back 2016-09-12 wrote: > On 2016/09/16 22:32:29, Ken Russell wrote: > > Embedding the vertex positions in the vertex shader seems a little too clever > -- > > is there a good reason to not just allocate a small buffer object and hook it > up > > to the VAO you're already allocating? > > I'm actually ok with this. That's also done in the CopyTexImageResourceManager > and other places. Personally, I think it is simpler, and actually likely more > efficient, than managing a vertex buffer, its lifetime, its bindings, its > updates. OK, fine by me then.
https://codereview.chromium.org/2286593002/diff/640001/gpu/command_buffer/ser... File gpu/command_buffer/service/gles2_cmd_decoder.cc (right): https://codereview.chromium.org/2286593002/diff/640001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_decoder.cc:7711: read_buffer_samples > 0 || On 2016/09/16 23:19:42, Ken Russell wrote: > On 2016/09/16 22:53:29, piman OOO back 2016-09-12 wrote: > > On 2016/09/16 22:32:29, Ken Russell wrote: > > > 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? > > > > 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) > > 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. 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.
Patchset #22 (id:660001) has been deleted
Patchset #22 (id:680001) has been deleted
The CQ bit was checked by yunchao.he@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
Thanks for your review, @piman and @kbr. Code has been updated, please take another look. Thanks a lot! https://codereview.chromium.org/2286593002/diff/640001/gpu/command_buffer/ser... File gpu/command_buffer/service/gles2_cmd_decoder.cc (right): https://codereview.chromium.org/2286593002/diff/640001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_decoder.cc:7711: read_buffer_samples > 0 || On 2016/09/16 23:28:44, piman OOO back 2016-09-12 wrote: > On 2016/09/16 23:19:42, Ken Russell wrote: > > On 2016/09/16 22:53:29, piman OOO back 2016-09-12 wrote: > > > On 2016/09/16 22:32:29, Ken Russell wrote: > > > > 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? > > > > > > 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) > > > > 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. > > 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. 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. https://codereview.chromium.org/2286593002/diff/640001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_decoder.cc:7756: srgb_converter_->InitializeSRGBConverter(this); On 2016/09/16 20:49:27, piman OOO back 2016-09-12 wrote: > nit: maybe you can do that in the block above - since it will early-out the > second time it runs. Done. https://codereview.chromium.org/2286593002/diff/640001/gpu/command_buffer/ser... File gpu/command_buffer/service/gles2_cmd_srgb_converter.cc (right): https://codereview.chromium.org/2286593002/diff/640001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_srgb_converter.cc:286: glBindFramebufferEXT(GL_READ_FRAMEBUFFER, srgb_decoder_fbo_); On 2016/09/16 20:49:27, piman OOO back 2016-09-12 wrote: > nit: this is already done in the decode path (srgb_decoder_fbo_ gets bound to > GL_FRAMEBUFFER, which means it's bound to both GL_READ_FRAMEBUFFER and > GL_DRAW_FRAMEBUFFER). So maybe the else clause below could move to l.257, such > that it goes > > if (decode) { > // ... > // [do the copy/crop] > // ... > glBindFramebufferEXT(GL_FRAMEBUFFER, srgb_decoder_fbo_); > // ... > // [do the decode draw] > // ... > } else { > glBindFramebufferEXT(GL_READ_FRAMEBUFFER, src_framebuffer); > } > Done.
The CQ bit was checked by yunchao.he@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Merge this now.
The CQ bit was checked by yunchao.he@intel.com
The patchset sent to the CQ was uploaded after l-g-t-m from zmo@chromium.org, piman@chromium.org Link to the patchset: https://codereview.chromium.org/2286593002/#ps700001 (title: "addressed feedbacks from piman and kbr")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== [Command Buffer] emulate SRGB color format for BlitFramebuffer in OpenGL. In Desktop OGLs whose version < 4.4, they maybe don't enable srgb conversion between srgb color format and linear color format. This change can emulate srgb and do the conversion for machines with these OGL profiles. BUG=634525 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ========== to ========== [Command Buffer] emulate SRGB color format for BlitFramebuffer in OpenGL. In Desktop OGLs whose version < 4.4, they maybe don't enable srgb conversion between srgb color format and linear color format. This change can emulate srgb and do the conversion for machines with these OGL profiles. BUG=634525 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ==========
Message was sent while issue was closed.
Committed patchset #22 (id:700001)
Message was sent while issue was closed.
Description was changed from ========== [Command Buffer] emulate SRGB color format for BlitFramebuffer in OpenGL. In Desktop OGLs whose version < 4.4, they maybe don't enable srgb conversion between srgb color format and linear color format. This change can emulate srgb and do the conversion for machines with these OGL profiles. BUG=634525 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ========== to ========== [Command Buffer] emulate SRGB color format for BlitFramebuffer in OpenGL. In Desktop OGLs whose version < 4.4, they maybe don't enable srgb conversion between srgb color format and linear color format. This change can emulate srgb and do the conversion for machines with these OGL profiles. BUG=634525 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel Committed: https://crrev.com/fd79169d48f8d31025b69c5dc05b7f00994a4f3d Cr-Commit-Position: refs/heads/master@{#419396} ==========
Message was sent while issue was closed.
Patchset 22 (id:??) landed as https://crrev.com/fd79169d48f8d31025b69c5dc05b7f00994a4f3d Cr-Commit-Position: refs/heads/master@{#419396}
Message was sent while issue was closed.
@kbr and @piman, Here are some discussions about bliting multisampled srgb image. See the comments inline. Thanks a lot! Look forward to your insight. https://codereview.chromium.org/2286593002/diff/640001/gpu/command_buffer/ser... File gpu/command_buffer/service/gles2_cmd_decoder.cc (right): https://codereview.chromium.org/2286593002/diff/640001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_decoder.cc:7711: read_buffer_samples > 0 || On 2016/09/16 23:19:42, Ken Russell wrote: > On 2016/09/16 22:53:29, piman OOO back 2016-09-12 wrote: > > On 2016/09/16 22:32:29, Ken Russell wrote: > > > 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? > > > > 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) > > 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 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.
Message was sent while issue was closed.
I think this is causing GPU FYI failure: https://build.chromium.org/p/chromium.gpu.fyi/builders/Linux%20Release%20%28N... WebglConformance_deqp_functional_gles3_framebufferblit_conversion_04 et al
Message was sent while issue was closed.
On 2016/09/19 17:49:59, ccameron wrote: > I think this is causing GPU FYI failure: > > https://build.chromium.org/p/chromium.gpu.fyi/builders/Linux%20Release%20%28N... > WebglConformance_deqp_functional_gles3_framebufferblit_conversion_04 et al I'll investigate, probably the expectations should be adjusted.
Message was sent while issue was closed.
On Mon, Sep 19, 2016 at 12:25 AM, <yunchao.he@intel.com> wrote: > @kbr and @piman, > Here are some discussions about bliting multisampled srgb image. See the > comments inline. Thanks a lot! > > Look forward to your insight. > > > https://codereview.chromium.org/2286593002/diff/640001/gpu/ > command_buffer/service/gles2_cmd_decoder.cc > File gpu/command_buffer/service/gles2_cmd_decoder.cc (right): > > https://codereview.chromium.org/2286593002/diff/640001/gpu/ > command_buffer/service/gles2_cmd_decoder.cc#newcode7711 > gpu/command_buffer/service/gles2_cmd_decoder.cc:7711: > read_buffer_samples > 0 || > On 2016/09/16 23:19:42, Ken Russell wrote: > > On 2016/09/16 22:53:29, piman OOO back 2016-09-12 wrote: > > > On 2016/09/16 22:32:29, Ken Russell wrote: > > > > 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? > > > > > > 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) > > > > 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 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? > Yes, I think so. The ES3 restrictions mean that BlitFramebuffer + multisampling is only allowed for a straightforward resolve (no filtering, same format, same bounds, only source is MS). This should be ok in OpenGL 4.2, and besides, there's no other way to get a single-sample buffer out of the source, so no matter what we do we'd need a BlitFramebuffer sRGB->sRGB before doing anything else. So I agree that the right way to proceed is to: 1- check the restrictions for MS (need to check that the draw buffer are not MS if the source is MS - the rest seems already handled) 2- go directly to BlitFramebuffer > I have a conformance test for this two: > https://github.com/KhronosGroup/WebGL/pull/2034. PTAL. > > https://codereview.chromium.org/2286593002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org. |
