|
|
Description[Command Buffer] enable/disable FRAMEBUFFER_SRGB only when sRGB image is active.
If there are sRGB images in fbo, but they are not selected as read/draw targets by
readBuffer or drawBuffers, it is not necessary to enable sRGB.
This small change can reduce the frequency of check and switch framebuffer srgb state.
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
Patch Set 1 #Patch Set 2 : remove unnecessary code #Patch Set 3 : encapusulate some code to a function #Patch Set 4 : framebuffer_srgb_ is not context-aware, set it to false during context restore #Patch Set 5 : framebuffer_srgb_ is not context-aware, set it to true during context restore #Patch Set 6 : encapusulate some code to a function #
Total comments: 10
Patch Set 7 : addressed zmo's feedback, small optimization and fix typos #
Total comments: 1
Patch Set 8 : typo #
Messages
Total messages: 68 (59 generated)
Description was changed from ========== We need to enable sRGB only when read buffer or draw buffe has sRGB. If fbo's attachments has sRGB but they are not selected as readBuffer or drawBuffers, it is not necessary to enable sRGB. BUG= ========== to ========== We need to enable sRGB only when read buffer or draw buffe has sRGB. If fbo's attachments has sRGB but they are not selected as readBuffer or drawBuffers, it is not necessary to enable sRGB. 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_optional_gpu_tests_rel on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_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...
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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_optional_gpu_tests_rel on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_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...
Description was changed from ========== We need to enable sRGB only when read buffer or draw buffe has sRGB. If fbo's attachments has sRGB but they are not selected as readBuffer or drawBuffers, it is not necessary to enable sRGB. 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] enable sRGB only when readBuffer or drawBuffers has sRGB color format. If there are sRGB attachments in fbo, but they are not selected as readBuffer or drawBuffers, it is not necessary to enable sRGB. 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 ==========
Description was changed from ========== [Command Buffer] enable sRGB only when readBuffer or drawBuffers has sRGB color format. If there are sRGB attachments in fbo, but they are not selected as readBuffer or drawBuffers, it is not necessary to enable sRGB. 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] enable sRGB only when readBuffer or drawBuffers has sRGB color format. If there are sRGB attachments in fbo, but they are not selected as read/draw target by readBuffer or drawBuffers, it is not necessary to enable sRGB. 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 ==========
Description was changed from ========== [Command Buffer] enable sRGB only when readBuffer or drawBuffers has sRGB color format. If there are sRGB attachments in fbo, but they are not selected as read/draw target by readBuffer or drawBuffers, it is not necessary to enable sRGB. 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] enable sRGB only when readBuffer or drawBuffers has sRGB color format. If there are sRGB attachments in fbo, but they are not selected as read/draw targets by readBuffer or drawBuffers, it is not necessary to enable sRGB. 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 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
The CQ bit was unchecked by yunchao.he@intel.com
Patchset #5 (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...
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_optional_gpu_tests_rel on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_optional_gpu_tests_rel on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_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...
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: This issue passed the CQ dry run.
Patchset #2 (id:20001) has been deleted
Patchset #2 (id:40001) has been deleted
Patchset #4 (id:120001) has been deleted
Patchset #4 (id:140001) has been deleted
yunchao.he@intel.com changed reviewers: + kbr@chromium.org, piman@chromium.org, qiankun.miao@intel.com, zmo@chromium.org
Description was changed from ========== [Command Buffer] enable sRGB only when readBuffer or drawBuffers has sRGB color format. If there are sRGB attachments in fbo, but they are not selected as read/draw targets by readBuffer or drawBuffers, it is not necessary to enable sRGB. 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] enable sRGB only when readBuffer or drawBuffers has sRGB color format. If there are sRGB attachments in fbo, but they are not selected as read/draw targets by readBuffer or drawBuffers, it is not necessary to enable sRGB. BUG= 637801 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 ==========
Some comments from https://codereview.chromium.org/2246823002/: I am looking at the sRGB related issue(emulate sRGB for core profile less than OGL 4.4 for BlitFramebuffer), I found that Chromium would check the srgb status for every draw call (BlitFramebuffer,drawArrays/drawElements and their variants, clear, clearBuffer, etc) by call into CheckBoundDrawFramebufferValid or CheckBoundFramebufferValid. So no matter what we set framebuffer_srgb status to be enabled/disabled for context init/restore,it would be correct in theory: If the srgb enable/disable state is wrong, it will be corrected before drawing. So I think there should be some other mistake in the code. Today, I found that there is a bug in Chromium: if the fbo have srgb color buffers(attachments), but they are not readBuffer/drawBuffers,chromium will enable srgb wrongly... PTAL. Thanks a lot!
See the comments inline. https://codereview.chromium.org/2268503002/diff/200001/gpu/command_buffer/ser... File gpu/command_buffer/service/context_state.cc (left): https://codereview.chromium.org/2268503002/diff/200001/gpu/command_buffer/ser... gpu/command_buffer/service/context_state.cc:503: } else if (framebuffer_srgb_ != prev_state->framebuffer_srgb_) { It is OK to keep Qiankun's code here. I just want to say the framebuffer_srgb status is not context-aware at all. The only thing we need to make sure is: keep the flag framebuffer_srgb_ to be consistent with what is really should be (glDisable or glEnable FRAMEBUFFER_SRGB) during context init/restore. https://codereview.chromium.org/2268503002/diff/200001/gpu/command_buffer/ser... File gpu/command_buffer/service/gles2_cmd_decoder.cc (left): https://codereview.chromium.org/2268503002/diff/200001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_decoder.cc:4196: framebuffer && framebuffer->HasSRGBAttachments(); This is not correct. If the fbo has srgb color buffer, but they are not drawBuffers, we should not enable framebuffer_srgb. https://codereview.chromium.org/2268503002/diff/200001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_decoder.cc:4228: (read_framebuffer && read_framebuffer->HasSRGBAttachments()); The same reason. If the fbo has srgb color buffer, but they are not drawBuffers/readBuffer, we should not enable framebuffer_srgb.
https://codereview.chromium.org/2268503002/diff/200001/gpu/command_buffer/ser... File gpu/command_buffer/service/context_state.cc (left): https://codereview.chromium.org/2268503002/diff/200001/gpu/command_buffer/ser... gpu/command_buffer/service/context_state.cc:503: } else if (framebuffer_srgb_ != prev_state->framebuffer_srgb_) { On 2016/08/24 14:08:51, yunchao wrote: > It is OK to keep Qiankun's code here. I just want to say the framebuffer_srgb > status is not context-aware at all. The only thing we need to make sure is: keep > the flag framebuffer_srgb_ to be consistent with what is really should be > (glDisable or glEnable FRAMEBUFFER_SRGB) during context init/restore. Agree with you.
https://codereview.chromium.org/2268503002/diff/200001/gpu/command_buffer/ser... File gpu/command_buffer/service/context_state.cc (right): https://codereview.chromium.org/2268503002/diff/200001/gpu/command_buffer/ser... gpu/command_buffer/service/context_state.cc:499: // FRAMEBUFFER_SRGB will be restored lazily at render time. This comment is wrong in your current implementation.
https://codereview.chromium.org/2268503002/diff/200001/gpu/command_buffer/ser... File gpu/command_buffer/service/context_state.cc (right): https://codereview.chromium.org/2268503002/diff/200001/gpu/command_buffer/ser... gpu/command_buffer/service/context_state.cc:503: EnableDisable(GL_FRAMEBUFFER_SRGB, framebuffer_srgb_); That's what we try to avoid. If we use virtual contexts, then every frame we switching contexts multiple times, at least between WebGL and compositor. The original design is to avoid enable/disable FRAMEBUFFER_SRGB per frame, but now it is per frame. https://codereview.chromium.org/2268503002/diff/200001/gpu/command_buffer/ser... File gpu/command_buffer/service/gles2_cmd_decoder.cc (left): https://codereview.chromium.org/2268503002/diff/200001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_decoder.cc:4196: framebuffer && framebuffer->HasSRGBAttachments(); On 2016/08/24 14:08:51, yunchao wrote: > This is not correct. If the fbo has srgb color buffer, but they are not > drawBuffers, we should not enable framebuffer_srgb. What we found is whether the sRGB image is currently active doesn't matter. However, if you want to fine-tuning it, that's OK. But why moving this out of here and put it in all calls? To me, putting it here is much cleaner.
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...
Description was changed from ========== [Command Buffer] enable sRGB only when readBuffer or drawBuffers has sRGB color format. If there are sRGB attachments in fbo, but they are not selected as read/draw targets by readBuffer or drawBuffers, it is not necessary to enable sRGB. BUG= 637801 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] enable sRGB only when readBuffer or drawBuffers has sRGB image enabled. If there are sRGB images in fbo, but they are not selected as read/draw targets by readBuffer or drawBuffers, it is not necessary to enable sRGB. This small change can reduce the frequency of check and switch framebuffer srgb state. BUG= 637801 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] enable sRGB only when readBuffer or drawBuffers has sRGB image enabled. If there are sRGB images in fbo, but they are not selected as read/draw targets by readBuffer or drawBuffers, it is not necessary to enable sRGB. This small change can reduce the frequency of check and switch framebuffer srgb state. BUG= 637801 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] enable sRGB only when readBuffer or drawBuffers has sRGB image enabled. If there are sRGB images in fbo, but they are not selected as read/draw targets by readBuffer or drawBuffers, it is not necessary to enable sRGB. This small change can reduce the frequency of check and switch framebuffer srgb state. 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 ==========
Thanks for your review, Zhenyao. You are correct, all bots can run correctly when framebuffer_srgb is enabled, but the srgb image is inactive. Although fine tuning to is correct too. So this change just reduce the check/switch frequency for framebuffe srgb state. Please take another look. Thanks! It is also OK to close this one if you think the fine-tuning is not necessary. https://codereview.chromium.org/2268503002/diff/200001/gpu/command_buffer/ser... File gpu/command_buffer/service/context_state.cc (right): https://codereview.chromium.org/2268503002/diff/200001/gpu/command_buffer/ser... gpu/command_buffer/service/context_state.cc:499: // FRAMEBUFFER_SRGB will be restored lazily at render time. On 2016/08/24 16:22:13, qiankun wrote: > This comment is wrong in your current implementation. That's true. https://codereview.chromium.org/2268503002/diff/200001/gpu/command_buffer/ser... gpu/command_buffer/service/context_state.cc:503: EnableDisable(GL_FRAMEBUFFER_SRGB, framebuffer_srgb_); On 2016/08/25 00:39:37, Zhenyao Mo wrote: > That's what we try to avoid. If we use virtual contexts, then every frame we > switching contexts multiple times, at least between WebGL and compositor. The > original design is to avoid enable/disable FRAMEBUFFER_SRGB per frame, but now > it is per frame. You are correct. The original code is better. https://codereview.chromium.org/2268503002/diff/200001/gpu/command_buffer/ser... File gpu/command_buffer/service/gles2_cmd_decoder.cc (left): https://codereview.chromium.org/2268503002/diff/200001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_decoder.cc:4196: framebuffer && framebuffer->HasSRGBAttachments(); On 2016/08/25 00:39:37, Zhenyao Mo wrote: > On 2016/08/24 14:08:51, yunchao wrote: > > This is not correct. If the fbo has srgb color buffer, but they are not > > drawBuffers, we should not enable framebuffer_srgb. > > What we found is whether the sRGB image is currently active doesn't matter. > However, if you want to fine-tuning it, that's OK. But why moving this out of > here and put it in all calls? To me, putting it here is much cleaner. That's true. It can draw correctly if there is sRGB image, no matter the sRGB image is currently active or not. moving this outside of this function is because of fine tuning. Suppose that the drawBuffer (clear or draw, etc) is depth buffer, it is not necessary to do this check at all. Because the sRGB image is definitely inactive, we don't need to check/change the framebuffer srgb state. https://codereview.chromium.org/2268503002/diff/220001/gpu/command_buffer/ser... File gpu/command_buffer/service/gles2_cmd_decoder.cc (left): https://codereview.chromium.org/2268503002/diff/220001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_decoder.cc:4228: read_framebuffer, target, GL_INVALID_FRAMEBUFFER_OPERATION, func_name); It is not necessary to check twice if the target is GL_FRAMEBUFFER.
The CQ bit was unchecked by yunchao.he@intel.com
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...
Description was changed from ========== [Command Buffer] enable sRGB only when readBuffer or drawBuffers has sRGB image enabled. If there are sRGB images in fbo, but they are not selected as read/draw targets by readBuffer or drawBuffers, it is not necessary to enable sRGB. This small change can reduce the frequency of check and switch framebuffer srgb state. 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] enable/disable FRAMEBUFFER_SRGB only when sRGB image is active. If there are sRGB images in fbo, but they are not selected as read/draw targets by readBuffer or drawBuffers, it is not necessary to enable sRGB. This small change can reduce the frequency of check and switch framebuffer srgb state. 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 ==========
I'm not an owner of this code, but I'm sorry, I don't think this is a good direction for the code. EnableDisableFramebufferSRGBForDrawBuffers is being called just before draw calls like DrawElements, and it iterates down all of the bound draw buffers, which is likely an expensive check. I think the way this is done in the current code is better.
On 2016/08/25 18:22:46, Ken Russell wrote: > I'm not an owner of this code, but I'm sorry, I don't think this is a good > direction for the code. EnableDisableFramebufferSRGBForDrawBuffers is being > called just before draw calls like DrawElements, and it iterates down all of the > bound draw buffers, which is likely an expensive check. I think the way this is > done in the current code is better. I agree with Ken. Also, you missed a bunch of calls, like instanced draw calls. That's why we put it in the frambuffer completeness check - to avoid making mistakes like this.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/08/25 18:25:55, Zhenyao Mo wrote: > On 2016/08/25 18:22:46, Ken Russell wrote: > > I'm not an owner of this code, but I'm sorry, I don't think this is a good > > direction for the code. EnableDisableFramebufferSRGBForDrawBuffers is being > > called just before draw calls like DrawElements, and it iterates down all of > the > > bound draw buffers, which is likely an expensive check. I think the way this > is > > done in the current code is better. > > I agree with Ken. > > Also, you missed a bunch of calls, like instanced draw calls. That's why we put > it in the frambuffer completeness check - to avoid making mistakes like this. To Ken, the draw buffers are cached in frame_buffer_manager, so I suppose this is not expensive. To Zhenyao, Instanced drawing are wrapped into corresponding DrawElements/DrawArrays, so no drawing APIs are missed. However, I indeed don't check/switch the framebuffer_srgb for nv_path_rendering APIs, like glStencilFillPathNV and so forth. Well, this change is not a must. I will close it. |