Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2159183002:
Attaching an image to many color attachment points is unsupported (Closed)
DescriptionAttaching an image to many color attachment points is unsupported
See Section 5.34 Framebuffer color attachments in WebGL 2.0 spec.
BUG=628861
TEST=gpu_unittests conformance2/rendering/framebuffer-unsupported.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/db975cd18716681f50db94d36120ba50148604fe
Cr-Commit-Position: refs/heads/master@{#406613}
Patch Set 1 #
Total comments: 15
Patch Set 2 : fix zmo's comments #
Total comments: 4
Patch Set 3 : rebase only #Patch Set 4 : remove webgl2 condition #
Total comments: 2
Messages
Total messages: 21 (10 generated)
Description was changed from ========== Attaching an image to many color attachment points is unsupported See Section 5.34 Framebuffer color attachments in WebGL 2.0 spec. BUG=628861 ========== to ========== Attaching an image to many color attachment points is unsupported See Section 5.34 Framebuffer color attachments in WebGL 2.0 spec. BUG=628861 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 ========== Attaching an image to many color attachment points is unsupported See Section 5.34 Framebuffer color attachments in WebGL 2.0 spec. BUG=628861 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 ========== Attaching an image to many color attachment points is unsupported See Section 5.34 Framebuffer color attachments in WebGL 2.0 spec. BUG=628861 TEST=gpu_unittests conformance2/rendering/framebuffer-unsupported.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 ==========
qiankun.miao@intel.com changed reviewers: + geofflang@chromium.org, kbr@chromium.org, yunchao.he@intel.com, zmo@chromium.org
PTAL. The bots failures are fixed at: https://github.com/KhronosGroup/WebGL/pull/1919. We cannot land this CL until that pr is rolled in. Per the discussion in the bug 628861, there may be still a bug in ANGLE for d3d translation which should be fixed in ANGLE.
Mostly looks good with a few minor suggestions. https://codereview.chromium.org/2159183002/diff/1/gpu/command_buffer/service/... File gpu/command_buffer/service/framebuffer_manager.cc (right): https://codereview.chromium.org/2159183002/diff/1/gpu/command_buffer/service/... gpu/command_buffer/service/framebuffer_manager.cc:237: samples_ == other->samples() && samples should not be part of this. No matter what samples we use, it is still the same image. https://codereview.chromium.org/2159183002/diff/1/gpu/command_buffer/service/... gpu/command_buffer/service/framebuffer_manager.cc:239: target_ == other->target() && This makes me think, can you add some test cases in the conformance test to make sure if we attach different faces of the same cubemap level, or if we attach different layer of the same 3D image level, they are allowed? https://codereview.chromium.org/2159183002/diff/1/gpu/command_buffer/service/... gpu/command_buffer/service/framebuffer_manager.cc:452: it->first < GL_COLOR_ATTACHMENT0 + manager_->max_draw_buffers_) { max_color_attachments_ https://codereview.chromium.org/2159183002/diff/1/gpu/command_buffer/service/... gpu/command_buffer/service/framebuffer_manager.cc:455: i < GL_COLOR_ATTACHMENT0 + manager_->max_draw_buffers_; i++) { max_color_attachments_ https://codereview.chromium.org/2159183002/diff/1/gpu/command_buffer/service/... File gpu/command_buffer/service/framebuffer_manager.h (right): https://codereview.chromium.org/2159183002/diff/1/gpu/command_buffer/service/... gpu/command_buffer/service/framebuffer_manager.h:87: bool HasDuplicateColorAttachments() const; nit: HasDuplicatedAttachments. https://codereview.chromium.org/2159183002/diff/1/gpu/command_buffer/service/... File gpu/command_buffer/service/framebuffer_manager_unittest.cc (right): https://codereview.chromium.org/2159183002/diff/1/gpu/command_buffer/service/... gpu/command_buffer/service/framebuffer_manager_unittest.cc:1494: TEST_F(FramebufferInfoTest, HasDuplicateColorAttachments) { nit: HasDuplicatedAttachments https://codereview.chromium.org/2159183002/diff/1/gpu/command_buffer/service/... gpu/command_buffer/service/framebuffer_manager_unittest.cc:1504: ASSERT_TRUE(texture.get() != NULL); nit: nullptr https://codereview.chromium.org/2159183002/diff/1/gpu/command_buffer/service/... File gpu/command_buffer/service/gles2_cmd_decoder.cc (right): https://codereview.chromium.org/2159183002/diff/1/gpu/command_buffer/service/... gpu/command_buffer/service/gles2_cmd_decoder.cc:7150: if (framebuffer->HasDuplicateColorAttachments()) { This should go inside Framebuffer::IsPossiblyComplete().
Thanks for review. PTAL. https://codereview.chromium.org/2159183002/diff/1/gpu/command_buffer/service/... File gpu/command_buffer/service/framebuffer_manager.cc (right): https://codereview.chromium.org/2159183002/diff/1/gpu/command_buffer/service/... gpu/command_buffer/service/framebuffer_manager.cc:237: samples_ == other->samples() && On 2016/07/19 20:39:15, Zhenyao Mo wrote: > samples should not be part of this. No matter what samples we use, it is still > the same image. Done. https://codereview.chromium.org/2159183002/diff/1/gpu/command_buffer/service/... gpu/command_buffer/service/framebuffer_manager.cc:239: target_ == other->target() && On 2016/07/19 20:39:15, Zhenyao Mo wrote: > This makes me think, can you add some test cases in the conformance test to make > sure if we attach different faces of the same cubemap level, or if we attach > different layer of the same 3D image level, they are allowed? I will add test for this. https://codereview.chromium.org/2159183002/diff/1/gpu/command_buffer/service/... gpu/command_buffer/service/framebuffer_manager.cc:452: it->first < GL_COLOR_ATTACHMENT0 + manager_->max_draw_buffers_) { On 2016/07/19 20:39:15, Zhenyao Mo wrote: > max_color_attachments_ That's fine. I saw in WebGL 2.0 spec max_color_attachments_ must be equal to max_draw_buffers. The usage of these two variables are mixed. See other places in this file. https://codereview.chromium.org/2159183002/diff/1/gpu/command_buffer/service/... File gpu/command_buffer/service/framebuffer_manager.h (right): https://codereview.chromium.org/2159183002/diff/1/gpu/command_buffer/service/... gpu/command_buffer/service/framebuffer_manager.h:87: bool HasDuplicateColorAttachments() const; On 2016/07/19 20:39:15, Zhenyao Mo wrote: > nit: HasDuplicatedAttachments. Done. https://codereview.chromium.org/2159183002/diff/1/gpu/command_buffer/service/... File gpu/command_buffer/service/framebuffer_manager_unittest.cc (right): https://codereview.chromium.org/2159183002/diff/1/gpu/command_buffer/service/... gpu/command_buffer/service/framebuffer_manager_unittest.cc:1494: TEST_F(FramebufferInfoTest, HasDuplicateColorAttachments) { On 2016/07/19 20:39:15, Zhenyao Mo wrote: > nit: HasDuplicatedAttachments Done. https://codereview.chromium.org/2159183002/diff/1/gpu/command_buffer/service/... gpu/command_buffer/service/framebuffer_manager_unittest.cc:1504: ASSERT_TRUE(texture.get() != NULL); On 2016/07/19 20:39:15, Zhenyao Mo wrote: > nit: nullptr Done. BTW: These are many "NULL" under src/gpu/. Should we clean them? https://codereview.chromium.org/2159183002/diff/1/gpu/command_buffer/service/... File gpu/command_buffer/service/gles2_cmd_decoder.cc (right): https://codereview.chromium.org/2159183002/diff/1/gpu/command_buffer/service/... gpu/command_buffer/service/gles2_cmd_decoder.cc:7150: if (framebuffer->HasDuplicateColorAttachments()) { On 2016/07/19 20:39:15, Zhenyao Mo wrote: > This should go inside Framebuffer::IsPossiblyComplete(). Done. https://codereview.chromium.org/2159183002/diff/20001/gpu/command_buffer/serv... File gpu/command_buffer/service/framebuffer_manager.cc (right): https://codereview.chromium.org/2159183002/diff/20001/gpu/command_buffer/serv... gpu/command_buffer/service/framebuffer_manager.cc:738: if (feature_info->context_type() == CONTEXT_TYPE_WEBGL2) { Is it right only for webgl 2 context? I think gles should not apply this restriction.
https://codereview.chromium.org/2159183002/diff/20001/gpu/command_buffer/serv... File gpu/command_buffer/service/framebuffer_manager.cc (right): https://codereview.chromium.org/2159183002/diff/20001/gpu/command_buffer/serv... gpu/command_buffer/service/framebuffer_manager.cc:738: if (feature_info->context_type() == CONTEXT_TYPE_WEBGL2) { On 2016/07/20 08:53:33, qiankun wrote: > Is it right only for webgl 2 context? I think gles should not apply this > restriction. We need to apply it to ES3 contexts as well. If they are on top of D3D9, this limit will apply. You should remove the if condition here, since this should apply in ES2 / WEBGL1 for EXT_draw_buffers extension cases - we will need to update the WebGL extension spec also.
I updated the CL. Please review again. Thanks. https://codereview.chromium.org/2159183002/diff/20001/gpu/command_buffer/serv... File gpu/command_buffer/service/framebuffer_manager.cc (right): https://codereview.chromium.org/2159183002/diff/20001/gpu/command_buffer/serv... gpu/command_buffer/service/framebuffer_manager.cc:738: if (feature_info->context_type() == CONTEXT_TYPE_WEBGL2) { On 2016/07/20 13:55:44, Zhenyao Mo wrote: > On 2016/07/20 08:53:33, qiankun wrote: > > Is it right only for webgl 2 context? I think gles should not apply this > > restriction. > > We need to apply it to ES3 contexts as well. If they are on top of D3D9, this > limit will apply. > > You should remove the if condition here, since this should apply in ES2 / WEBGL1 > for EXT_draw_buffers extension cases - we will need to update the WebGL > extension spec also. I see. You told me to update extension case before. I will do it tomorrow. So, we still need to check if EXT_draw_buffers extension is available for ES2? ES2 doesn't have this limitation. Right? https://codereview.chromium.org/2159183002/diff/60001/gpu/command_buffer/serv... File gpu/command_buffer/service/framebuffer_manager_unittest.cc (right): https://codereview.chromium.org/2159183002/diff/60001/gpu/command_buffer/serv... gpu/command_buffer/service/framebuffer_manager_unittest.cc:1507: feature_info_->InitializeForTesting(CONTEXT_TYPE_OPENGLES3); Is it Ok for you to test GLES3 context now? I may update ES2/WebGL 1 + EXT_draw_buffers next time when conformance test for that is ready.
lgtm https://codereview.chromium.org/2159183002/diff/60001/gpu/command_buffer/serv... File gpu/command_buffer/service/framebuffer_manager_unittest.cc (right): https://codereview.chromium.org/2159183002/diff/60001/gpu/command_buffer/serv... gpu/command_buffer/service/framebuffer_manager_unittest.cc:1507: feature_info_->InitializeForTesting(CONTEXT_TYPE_OPENGLES3); On 2016/07/20 15:03:26, qiankun wrote: > Is it Ok for you to test GLES3 context now? I may update ES2/WebGL 1 + > EXT_draw_buffers next time when conformance test for that is ready. This is good. I think between this and the WebGL side testing, we are well covered.
https://codereview.chromium.org/2159183002/diff/20001/gpu/command_buffer/serv... File gpu/command_buffer/service/framebuffer_manager.cc (right): https://codereview.chromium.org/2159183002/diff/20001/gpu/command_buffer/serv... gpu/command_buffer/service/framebuffer_manager.cc:738: if (feature_info->context_type() == CONTEXT_TYPE_WEBGL2) { On 2016/07/20 15:03:26, qiankun wrote: > On 2016/07/20 13:55:44, Zhenyao Mo wrote: > > On 2016/07/20 08:53:33, qiankun wrote: > > > Is it right only for webgl 2 context? I think gles should not apply this > > > restriction. > > > > We need to apply it to ES3 contexts as well. If they are on top of D3D9, this > > limit will apply. > > > > You should remove the if condition here, since this should apply in ES2 / > WEBGL1 > > for EXT_draw_buffers extension cases - we will need to update the WebGL > > extension spec also. > > I see. You told me to update extension case before. I will do it tomorrow. > So, we still need to check if EXT_draw_buffers extension is available for ES2? > ES2 doesn't have this limitation. Right? On the command buffer side, we don't need to check for the extension. If it's not there, then there will be no more than one color attachment, and this check should just pass. This is not on a perf critical path, so this is OK.
The CQ bit was checked by qiankun.miao@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 qiankun.miao@intel.com
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 ========== Attaching an image to many color attachment points is unsupported See Section 5.34 Framebuffer color attachments in WebGL 2.0 spec. BUG=628861 TEST=gpu_unittests conformance2/rendering/framebuffer-unsupported.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 ========== Attaching an image to many color attachment points is unsupported See Section 5.34 Framebuffer color attachments in WebGL 2.0 spec. BUG=628861 TEST=gpu_unittests conformance2/rendering/framebuffer-unsupported.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 #4 (id:60001)
Message was sent while issue was closed.
CQ bit was unchecked.
Message was sent while issue was closed.
Description was changed from ========== Attaching an image to many color attachment points is unsupported See Section 5.34 Framebuffer color attachments in WebGL 2.0 spec. BUG=628861 TEST=gpu_unittests conformance2/rendering/framebuffer-unsupported.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 ========== Attaching an image to many color attachment points is unsupported See Section 5.34 Framebuffer color attachments in WebGL 2.0 spec. BUG=628861 TEST=gpu_unittests conformance2/rendering/framebuffer-unsupported.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/db975cd18716681f50db94d36120ba50148604fe Cr-Commit-Position: refs/heads/master@{#406613} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/db975cd18716681f50db94d36120ba50148604fe Cr-Commit-Position: refs/heads/master@{#406613} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
