Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2166923002:
Add unittests for InvalidateFramebuffer with DEPTH_STENCIL_ATTACHMENT (Closed)
DescriptionAdd unittests for InvalidateFramebuffer with DEPTH_STENCIL_ATTACHMENT
Unit tests for https://codereview.chromium.org/2149523002/.
BUG=628496, 624506
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/af3b3d8bb5281c929bcfd930cb6ebbfa3a5004c9
Cr-Commit-Position: refs/heads/master@{#407271}
Patch Set 1 #
Total comments: 5
Patch Set 2 : check glInvalidateFramebuffer #
Total comments: 2
Patch Set 3 : fix depth_stencil #Patch Set 4 : fix bots failures #
Total comments: 4
Patch Set 5 : fix typo #
Messages
Total messages: 29 (15 generated)
Description was changed from ========== Add unittests for InvalidateFramebuffer with DEPTH_STENCIL_ATTACGMENT BUG=628496 ========== to ========== Add unittests for InvalidateFramebuffer with DEPTH_STENCIL_ATTACGMENT BUG=628496 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 wrote this unit tests for https://codereview.chromium.org/2149523002/. PTAL. https://codereview.chromium.org/2166923002/diff/1/gpu/command_buffer/service/... File gpu/command_buffer/service/gles2_cmd_decoder_unittest_framebuffers.cc (right): https://codereview.chromium.org/2166923002/diff/1/gpu/command_buffer/service/... gpu/command_buffer/service/gles2_cmd_decoder_unittest_framebuffers.cc:3447: EXPECT_FALSE(framebuffer->HasUnclearedAttachment(GL_STENCIL_ATTACHMENT)); zhenyao, do you know how to check glInvalidateFramebuffer(target, 0, translated_attachments.get()) is called with 0 as the second parameter? It means no attachment is invalidated.
qiankun.miao@intel.com changed reviewers: + jiawei.shao@intel.com, kbr@chromium.org, zmo@chromium.org
https://codereview.chromium.org/2166923002/diff/1/gpu/command_buffer/service/... File gpu/command_buffer/service/gles2_cmd_decoder_unittest_framebuffers.cc (right): https://codereview.chromium.org/2166923002/diff/1/gpu/command_buffer/service/... gpu/command_buffer/service/gles2_cmd_decoder_unittest_framebuffers.cc:3447: EXPECT_FALSE(framebuffer->HasUnclearedAttachment(GL_STENCIL_ATTACHMENT)); On 2016/07/20 19:23:58, qiankun wrote: > zhenyao, do you know how to check glInvalidateFramebuffer(target, 0, > translated_attachments.get()) is called with 0 as the second parameter? It means > no attachment is invalidated. EXPECT_CALL(*gl_, InvalidateFramebuffer(target, 1, _)) replace 1 with 0.
https://codereview.chromium.org/2166923002/diff/1/gpu/command_buffer/service/... File gpu/command_buffer/service/gles2_cmd_decoder_unittest_framebuffers.cc (right): https://codereview.chromium.org/2166923002/diff/1/gpu/command_buffer/service/... gpu/command_buffer/service/gles2_cmd_decoder_unittest_framebuffers.cc:3447: EXPECT_FALSE(framebuffer->HasUnclearedAttachment(GL_STENCIL_ATTACHMENT)); On 2016/07/20 19:36:49, Zhenyao Mo wrote: > On 2016/07/20 19:23:58, qiankun wrote: > > zhenyao, do you know how to check glInvalidateFramebuffer(target, 0, > > translated_attachments.get()) is called with 0 as the second parameter? It > means > > no attachment is invalidated. > > EXPECT_CALL(*gl_, InvalidateFramebuffer(target, 1, _)) > > replace 1 with 0. This is used to check InvalidateFramebuffer in command buffer is called other than glInvalidateFramebuffer in driver. 1 makes sure InvaldateFramebuffer execute successfully.
https://codereview.chromium.org/2166923002/diff/1/gpu/command_buffer/service/... File gpu/command_buffer/service/gles2_cmd_decoder_unittest_framebuffers.cc (right): https://codereview.chromium.org/2166923002/diff/1/gpu/command_buffer/service/... gpu/command_buffer/service/gles2_cmd_decoder_unittest_framebuffers.cc:3447: EXPECT_FALSE(framebuffer->HasUnclearedAttachment(GL_STENCIL_ATTACHMENT)); On 2016/07/20 19:39:56, qiankun wrote: > On 2016/07/20 19:36:49, Zhenyao Mo wrote: > > On 2016/07/20 19:23:58, qiankun wrote: > > > zhenyao, do you know how to check glInvalidateFramebuffer(target, 0, > > > translated_attachments.get()) is called with 0 as the second parameter? It > > means > > > no attachment is invalidated. > > > > EXPECT_CALL(*gl_, InvalidateFramebuffer(target, 1, _)) > > > > replace 1 with 0. > > This is used to check InvalidateFramebuffer in command buffer is called other > than glInvalidateFramebuffer in driver. 1 makes sure InvaldateFramebuffer > execute successfully. No, this is the mock expectation for the underlying GL driver.
https://codereview.chromium.org/2166923002/diff/1/gpu/command_buffer/service/... File gpu/command_buffer/service/gles2_cmd_decoder_unittest_framebuffers.cc (right): https://codereview.chromium.org/2166923002/diff/1/gpu/command_buffer/service/... gpu/command_buffer/service/gles2_cmd_decoder_unittest_framebuffers.cc:3447: EXPECT_FALSE(framebuffer->HasUnclearedAttachment(GL_STENCIL_ATTACHMENT)); On 2016/07/20 19:41:44, Zhenyao Mo wrote: > On 2016/07/20 19:39:56, qiankun wrote: > > On 2016/07/20 19:36:49, Zhenyao Mo wrote: > > > On 2016/07/20 19:23:58, qiankun wrote: > > > > zhenyao, do you know how to check glInvalidateFramebuffer(target, 0, > > > > translated_attachments.get()) is called with 0 as the second parameter? It > > > means > > > > no attachment is invalidated. > > > > > > EXPECT_CALL(*gl_, InvalidateFramebuffer(target, 1, _)) > > > > > > replace 1 with 0. > > > > This is used to check InvalidateFramebuffer in command buffer is called other > > than glInvalidateFramebuffer in driver. 1 makes sure InvaldateFramebuffer > > execute successfully. > > No, this is the mock expectation for the underlying GL driver. Updated the code. I will try this unit_tests with https://codereview.chromium.org/2149523002/ and https://codereview.chromium.org/2171543002/ tomorrow.
https://codereview.chromium.org/2166923002/diff/20001/gpu/command_buffer/serv... File gpu/command_buffer/service/gles2_cmd_decoder_unittest_framebuffers.cc (right): https://codereview.chromium.org/2166923002/diff/20001/gpu/command_buffer/serv... gpu/command_buffer/service/gles2_cmd_decoder_unittest_framebuffers.cc:3447: EXPECT_FALSE(framebuffer->HasUnclearedAttachment(GL_STENCIL_ATTACHMENT)); Could you add a subsequent test which invalidates GL_DEPTH_STENCIL_ATTACHMENT and verifies EXPECT_TRUE for these two HasUnclearedAttachment calls?
Description was changed from ========== Add unittests for InvalidateFramebuffer with DEPTH_STENCIL_ATTACGMENT BUG=628496 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 ========== Add unittests for InvalidateFramebuffer with DEPTH_STENCIL_ATTACHMENT BUG=628496 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 ========== Add unittests for InvalidateFramebuffer with DEPTH_STENCIL_ATTACHMENT BUG=628496 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 ========== Add unittests for InvalidateFramebuffer with DEPTH_STENCIL_ATTACHMENT BUG=628496, 624506 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 ========== Add unittests for InvalidateFramebuffer with DEPTH_STENCIL_ATTACHMENT BUG=628496, 624506 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 ========== Add unittests for InvalidateFramebuffer with DEPTH_STENCIL_ATTACHMENT Unit tests for https://codereview.chromium.org/2149523002/. BUG=628496, 624506 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: + yunchao.he@intel.com
Patchset #4 (id:60001) has been deleted
Please review it again when you free. https://codereview.chromium.org/2166923002/diff/20001/gpu/command_buffer/serv... File gpu/command_buffer/service/gles2_cmd_decoder_unittest_framebuffers.cc (right): https://codereview.chromium.org/2166923002/diff/20001/gpu/command_buffer/serv... gpu/command_buffer/service/gles2_cmd_decoder_unittest_framebuffers.cc:3447: EXPECT_FALSE(framebuffer->HasUnclearedAttachment(GL_STENCIL_ATTACHMENT)); On 2016/07/20 23:44:08, Ken Russell wrote: > Could you add a subsequent test which invalidates GL_DEPTH_STENCIL_ATTACHMENT > and verifies EXPECT_TRUE for these two HasUnclearedAttachment calls? Done.
Thanks for updating the tests. LGTM https://codereview.chromium.org/2166923002/diff/80001/gpu/command_buffer/serv... File gpu/command_buffer/service/gles2_cmd_decoder.cc (right): https://codereview.chromium.org/2166923002/diff/80001/gpu/command_buffer/serv... gpu/command_buffer/service/gles2_cmd_decoder.cc:5798: // TODO(qiankun.miao@intel.com): We should only make DEPTH and STENCIL Typo? make -> mark? https://codereview.chromium.org/2166923002/diff/80001/gpu/command_buffer/serv... File gpu/command_buffer/service/gles2_cmd_decoder_unittest_framebuffers.cc (right): https://codereview.chromium.org/2166923002/diff/80001/gpu/command_buffer/serv... gpu/command_buffer/service/gles2_cmd_decoder_unittest_framebuffers.cc:3427: // TODO(qiankun.miao@intel.com): We should only make DEPTH and STENCIL make -> mark?
zmo: owner review. https://codereview.chromium.org/2166923002/diff/80001/gpu/command_buffer/serv... File gpu/command_buffer/service/gles2_cmd_decoder.cc (right): https://codereview.chromium.org/2166923002/diff/80001/gpu/command_buffer/serv... gpu/command_buffer/service/gles2_cmd_decoder.cc:5798: // TODO(qiankun.miao@intel.com): We should only make DEPTH and STENCIL On 2016/07/22 18:24:31, Ken Russell wrote: > Typo? make -> mark? Done. https://codereview.chromium.org/2166923002/diff/80001/gpu/command_buffer/serv... File gpu/command_buffer/service/gles2_cmd_decoder_unittest_framebuffers.cc (right): https://codereview.chromium.org/2166923002/diff/80001/gpu/command_buffer/serv... gpu/command_buffer/service/gles2_cmd_decoder_unittest_framebuffers.cc:3427: // TODO(qiankun.miao@intel.com): We should only make DEPTH and STENCIL On 2016/07/22 18:24:31, Ken Russell wrote: > make -> mark? Done.
On 2016/07/22 19:15:07, qiankun wrote: > zmo: owner review. > > https://codereview.chromium.org/2166923002/diff/80001/gpu/command_buffer/serv... > File gpu/command_buffer/service/gles2_cmd_decoder.cc (right): > > https://codereview.chromium.org/2166923002/diff/80001/gpu/command_buffer/serv... > gpu/command_buffer/service/gles2_cmd_decoder.cc:5798: // > mailto:TODO(qiankun.miao@intel.com): We should only make DEPTH and STENCIL > On 2016/07/22 18:24:31, Ken Russell wrote: > > Typo? make -> mark? > > Done. > > https://codereview.chromium.org/2166923002/diff/80001/gpu/command_buffer/serv... > File gpu/command_buffer/service/gles2_cmd_decoder_unittest_framebuffers.cc > (right): > > https://codereview.chromium.org/2166923002/diff/80001/gpu/command_buffer/serv... > gpu/command_buffer/service/gles2_cmd_decoder_unittest_framebuffers.cc:3427: // > mailto:TODO(qiankun.miao@intel.com): We should only make DEPTH and STENCIL > On 2016/07/22 18:24:31, Ken Russell wrote: > > make -> mark? > > Done. lgtm
The CQ bit was checked by zmo@chromium.org 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 zmo@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from kbr@chromium.org Link to the patchset: https://codereview.chromium.org/2166923002/#ps100001 (title: "fix typo")
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 ========== Add unittests for InvalidateFramebuffer with DEPTH_STENCIL_ATTACHMENT Unit tests for https://codereview.chromium.org/2149523002/. BUG=628496, 624506 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 ========== Add unittests for InvalidateFramebuffer with DEPTH_STENCIL_ATTACHMENT Unit tests for https://codereview.chromium.org/2149523002/. BUG=628496, 624506 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 #5 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== Add unittests for InvalidateFramebuffer with DEPTH_STENCIL_ATTACHMENT Unit tests for https://codereview.chromium.org/2149523002/. BUG=628496, 624506 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 ========== Add unittests for InvalidateFramebuffer with DEPTH_STENCIL_ATTACHMENT Unit tests for https://codereview.chromium.org/2149523002/. BUG=628496, 624506 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/af3b3d8bb5281c929bcfd930cb6ebbfa3a5004c9 Cr-Commit-Position: refs/heads/master@{#407271} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/af3b3d8bb5281c929bcfd930cb6ebbfa3a5004c9 Cr-Commit-Position: refs/heads/master@{#407271} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
