Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2329453002:
[Command buffer] During blit FB, It is invalid if src source and dst source have identical image (Closed)
Description[Command buffer] During blit framebuffer, It is invalid if src source and dst source have identical image
BUG=645490
TEST=conformance2/rendering/blitframebuffer-test.html
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/faeb732edb9e057db922bc1d623dd0a543b91ca0
Cr-Commit-Position: refs/heads/master@{#417896}
Patch Set 1 #
Total comments: 7
Patch Set 2 : addressed zmo@'s feedback #Patch Set 3 : addressed zmo@'s feedback: do not interate all drawbuffers to optimize the perf if possible #
Total comments: 2
Patch Set 4 : remove unnecessary code #Patch Set 5 : addressed zmo@'s feedback: break early if the readbuffer and drawbuffers has image identical #Patch Set 6 : Add a TODO per zmo@'s feedback #Patch Set 7 : update webgl2 expectation #
Messages
Total messages: 45 (34 generated)
Description was changed from ========== [Command buffer] During blit, It is invalid if src source and dst source have identical image BUG= ========== to ========== [Command buffer] During blit, It is invalid if src source and dst source have identical image 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: This issue passed the CQ dry run.
Description was changed from ========== [Command buffer] During blit, It is invalid if src source and dst source have identical image 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] During blit, It is invalid if src source and dst source have identical image 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 ==========
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] During blit, It is invalid if src source and dst source have identical image 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] During blit, It is invalid if src source and dst source have identical image BUG=645490 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 ==========
PTAL. Thanks a lot! I also updated the conformance test and add more cases to do a thorough test for this feature. It is at https://github.com/KhronosGroup/WebGL/pull/2019
Description was changed from ========== [Command buffer] During blit, It is invalid if src source and dst source have identical image BUG=645490 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] During blit framebuffer, It is invalid if src source and dst source have identical image BUG=645490 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 ==========
https://codereview.chromium.org/2329453002/diff/1/gpu/command_buffer/service/... File gpu/command_buffer/service/gles2_cmd_decoder.cc (right): https://codereview.chromium.org/2329453002/diff/1/gpu/command_buffer/service/... gpu/command_buffer/service/gles2_cmd_decoder.cc:1352: // Check that whether the currently bound draw framebuffer has image nit: remove that https://codereview.chromium.org/2329453002/diff/1/gpu/command_buffer/service/... gpu/command_buffer/service/gles2_cmd_decoder.cc:4355: for (uint32_t ii = 0; ii < group_->max_draw_buffers(); ++ii) { If you go through attachments of the draw_framebuffer, most time it's probably only one or two attachments, much less iterations. https://codereview.chromium.org/2329453002/diff/1/gpu/command_buffer/service/... gpu/command_buffer/service/gles2_cmd_decoder.cc:4376: if (depth_buffer_draw && nit: wrong indent
Patchset #2 (id:20001) has been deleted
Thanks for your quick review, Zhenyao. Code has been updated accordingly. Could you take another look... https://codereview.chromium.org/2329453002/diff/1/gpu/command_buffer/service/... File gpu/command_buffer/service/gles2_cmd_decoder.cc (right): https://codereview.chromium.org/2329453002/diff/1/gpu/command_buffer/service/... gpu/command_buffer/service/gles2_cmd_decoder.cc:1352: // Check that whether the currently bound draw framebuffer has image On 2016/09/09 16:29:57, Zhenyao Mo wrote: > nit: remove that Done. https://codereview.chromium.org/2329453002/diff/1/gpu/command_buffer/service/... gpu/command_buffer/service/gles2_cmd_decoder.cc:4355: for (uint32_t ii = 0; ii < group_->max_draw_buffers(); ++ii) { On 2016/09/09 16:29:57, Zhenyao Mo wrote: > If you go through attachments of the draw_framebuffer, most time it's probably > only one or two attachments, much less iterations. That's true. But we still need to iterate to cover unusual cases... https://codereview.chromium.org/2329453002/diff/1/gpu/command_buffer/service/... gpu/command_buffer/service/gles2_cmd_decoder.cc:4376: if (depth_buffer_draw && On 2016/09/09 16:29:57, Zhenyao Mo wrote: > nit: wrong indent 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...
Description was changed from ========== [Command buffer] During blit framebuffer, It is invalid if src source and dst source have identical image BUG=645490 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] During blit framebuffer, It is invalid if src source and dst source have identical image BUG=645490 TEST=conformance2/rendering/blitframebuffer-test.html 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 ==========
https://codereview.chromium.org/2329453002/diff/1/gpu/command_buffer/service/... File gpu/command_buffer/service/gles2_cmd_decoder.cc (right): https://codereview.chromium.org/2329453002/diff/1/gpu/command_buffer/service/... gpu/command_buffer/service/gles2_cmd_decoder.cc:4355: for (uint32_t ii = 0; ii < group_->max_draw_buffers(); ++ii) { On 2016/09/09 16:47:33, yunchao wrote: > On 2016/09/09 16:29:57, Zhenyao Mo wrote: > > If you go through attachments of the draw_framebuffer, most time it's probably > > only one or two attachments, much less iterations. > > That's true. But we still need to iterate to cover unusual cases... We should always optimize against most common use cases. BlitFramebuffer is on critical path: if webgl is created with antialias = true, then each frame we need to blit to a non-multisampled texture for compositing. So optimizing it is important.
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...
On 2016/09/09 17:39:32, Zhenyao Mo wrote: > https://codereview.chromium.org/2329453002/diff/1/gpu/command_buffer/service/... > File gpu/command_buffer/service/gles2_cmd_decoder.cc (right): > > https://codereview.chromium.org/2329453002/diff/1/gpu/command_buffer/service/... > gpu/command_buffer/service/gles2_cmd_decoder.cc:4355: for (uint32_t ii = 0; ii < > group_->max_draw_buffers(); ++ii) { > On 2016/09/09 16:47:33, yunchao wrote: > > On 2016/09/09 16:29:57, Zhenyao Mo wrote: > > > If you go through attachments of the draw_framebuffer, most time it's > probably > > > only one or two attachments, much less iterations. > > > > That's true. But we still need to iterate to cover unusual cases... > > We should always optimize against most common use cases. BlitFramebuffer is on > critical path: if webgl is created with antialias = true, then each frame we > need to blit to a non-multisampled texture for compositing. So optimizing it is > important. Yes. BlitFramebuffer is on critical path. I know that. I have updated the code to avoid iterate all drawbuffers. Could you take another look, Zhenyao? Thanks a lot!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm We should consider doing something similar like CheckFramebufferStatus(). We cache the validation results, and if read_framebuffer doesn't change, draw_framebuffer doesn't change, then use the cached status. But that could be future work.
https://codereview.chromium.org/2329453002/diff/60001/gpu/command_buffer/serv... File gpu/command_buffer/service/gles2_cmd_decoder.cc (right): https://codereview.chromium.org/2329453002/diff/60001/gpu/command_buffer/serv... gpu/command_buffer/service/gles2_cmd_decoder.cc:7700: is_feedback_loop = FeedbackLoopTrue; nit: break
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 yunchao.he@intel.com
Thanks for your quick review, Zhenyao. I added a TODO per your suggestion. I can do that optimization after bugs/failures are fixed. https://codereview.chromium.org/2329453002/diff/60001/gpu/command_buffer/serv... File gpu/command_buffer/service/gles2_cmd_decoder.cc (right): https://codereview.chromium.org/2329453002/diff/60001/gpu/command_buffer/serv... gpu/command_buffer/service/gles2_cmd_decoder.cc:7700: is_feedback_loop = FeedbackLoopTrue; On 2016/09/12 05:37:08, Zhenyao Mo wrote: > nit: break 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.
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
The patchset sent to the CQ was uploaded after l-g-t-m from zmo@chromium.org Link to the patchset: https://codereview.chromium.org/2329453002/#ps140001 (title: "update webgl2 expectation")
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] During blit framebuffer, It is invalid if src source and dst source have identical image BUG=645490 TEST=conformance2/rendering/blitframebuffer-test.html 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] During blit framebuffer, It is invalid if src source and dst source have identical image BUG=645490 TEST=conformance2/rendering/blitframebuffer-test.html 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 #7 (id:140001)
Message was sent while issue was closed.
Description was changed from ========== [Command buffer] During blit framebuffer, It is invalid if src source and dst source have identical image BUG=645490 TEST=conformance2/rendering/blitframebuffer-test.html 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] During blit framebuffer, It is invalid if src source and dst source have identical image BUG=645490 TEST=conformance2/rendering/blitframebuffer-test.html 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/faeb732edb9e057db922bc1d623dd0a543b91ca0 Cr-Commit-Position: refs/heads/master@{#417896} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/faeb732edb9e057db922bc1d623dd0a543b91ca0 Cr-Commit-Position: refs/heads/master@{#417896} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
