Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2182443003:
WebGL 2: Fix bugs in negativetextureapi.html for Mac (Closed)
|
Created:
4 years, 5 months ago by yunchao Modified:
4 years, 4 months ago CC:
chromium-reviews, blink-reviews, darin-cc_chromium.org, haraken, jam, piman+watch_chromium.org, Yang Gu Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionWebGL 2: Fix bugs in negativetextureapi.html for Mac
BUG=631934
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/4771076dd784de40192c7249914a2fbb208e7a49
Cr-Commit-Position: refs/heads/master@{#408313}
Patch Set 1 : a small fix #Patch Set 2 : Update code per zmo's review: fix the bug in cmd buffer, instead of Blink WebGL #
Total comments: 6
Patch Set 3 : add tests into gpu_unittests for CopyTexSubImage3D #Patch Set 4 : code rebase #Patch Set 5 : small change #
Total comments: 17
Patch Set 6 : addressed feedbacks from zmo and qiankun #
Total comments: 9
Patch Set 7 : addressed zmo's feedback #
Messages
Total messages: 57 (39 generated)
Description was changed from ========== update webgl 2 expectation fix the bug in negativetextureapi.html for Mac Intel BUG= ========== to ========== update webgl 2 expectation fix the bug in negativetextureapi.html for Mac Intel BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel ==========
Description was changed from ========== update webgl 2 expectation fix the bug in negativetextureapi.html for Mac Intel BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel ========== to ========== update webgl 2 expectation fix the bug in negativetextureapi.html for Mac Intel 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 ========== update webgl 2 expectation fix the bug in negativetextureapi.html for Mac Intel 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 ========== WebGL 2: Fix bugs in negativetextureapi.html 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 ==========
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Description was changed from ========== WebGL 2: Fix bugs in negativetextureapi.html 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 ========== WebGL 2: Fix bugs in negativetextureapi.html BUG=565347 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, qiankun.miao@intel.com, zmo@chromium.org
A small change to fix bugs in negativetextureapi.html for some platforms. The corresponding item in webgl2_conformance_expectations .py has been updated too. PTAL. Thanks!
On 2016/07/26 05:28:40, yunchao wrote: > A small change to fix bugs in negativetextureapi.html for some platforms. > > The corresponding item in webgl2_conformance_expectations .py has been updated > too. > > PTAL. Thanks! Should we do the validation in command buffer instead?
On 2016/07/26 05:32:59, Zhenyao Mo wrote: > On 2016/07/26 05:28:40, yunchao wrote: > > A small change to fix bugs in negativetextureapi.html for some platforms. > > > > The corresponding item in webgl2_conformance_expectations .py has been updated > > too. > > > > PTAL. Thanks! > > Should we do the validation in command buffer instead? This is simple and direct. We can do this kind work in command buffer, but it would be a little complicated. Because we need to discard the auto-gen code for copyTexSubImage3D in GLES2Implementation and GLES2CmdDecoder, and write our own code for validation. It is up to you, Zhenyao. If you prefer to add more validation in command buffer, I can do this too.
On 2016/07/26 05:39:03, yunchao wrote: > On 2016/07/26 05:32:59, Zhenyao Mo wrote: > > On 2016/07/26 05:28:40, yunchao wrote: > > > A small change to fix bugs in negativetextureapi.html for some platforms. > > > > > > The corresponding item in webgl2_conformance_expectations .py has been > updated > > > too. > > > > > > PTAL. Thanks! > > > > Should we do the validation in command buffer instead? > > This is simple and direct. We can do this kind work in command buffer, but it > would be a little complicated. Because we need to discard the auto-gen code for > copyTexSubImage3D in GLES2Implementation and GLES2CmdDecoder, and write our own > code for validation. > > It is up to you, Zhenyao. If you prefer to add more validation in command > buffer, I can do this too. It's not a preference, but by design all commands going through command buffer have to be fully validated. 1, security concern, 2, webgl isn't the only user of command buffer, so other users may get incorrect behavior if you only validate in blink webgl.
On 2016/07/26 05:43:28, Zhenyao Mo wrote: > On 2016/07/26 05:39:03, yunchao wrote: > > On 2016/07/26 05:32:59, Zhenyao Mo wrote: > > > On 2016/07/26 05:28:40, yunchao wrote: > > > > A small change to fix bugs in negativetextureapi.html for some platforms. > > > > > > > > The corresponding item in webgl2_conformance_expectations .py has been > > updated > > > > too. > > > > > > > > PTAL. Thanks! > > > > > > Should we do the validation in command buffer instead? > > > > This is simple and direct. We can do this kind work in command buffer, but it > > would be a little complicated. Because we need to discard the auto-gen code > for > > copyTexSubImage3D in GLES2Implementation and GLES2CmdDecoder, and write our > own > > code for validation. > > > > It is up to you, Zhenyao. If you prefer to add more validation in command > > buffer, I can do this too. > > It's not a preference, but by design all commands going through command buffer > have to be fully validated. 1, security concern, 2, webgl isn't the only user of > command buffer, so other users may get incorrect behavior if you only validate > in blink webgl. That's true. Then I will add more validation code for copyTexSubImage3D. Parameters for this API is far from full validation by auto-generated code. Stay tuned. BTW, I think some other APIs whose validation code are totally auto-generated may have the same issue. I suppose that the auto-generated code is hard to cover all situations for a full validation. The only difference is the underlying HW can generate appropriate error for these APIs w/o full validation. I mean, by design it may be not a good way to do validation totally by auto-generated code (especially for GLES2Implementation and GLES2CmdDocoder). WDYT?
Patchset #2 (id:60001) 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: This issue passed the CQ dry run.
The current code looks correct, but you need more validation than what you put down right now. https://codereview.chromium.org/2182443003/diff/80001/gpu/command_buffer/serv... File gpu/command_buffer/service/gles2_cmd_decoder.cc (right): https://codereview.chromium.org/2182443003/diff/80001/gpu/command_buffer/serv... gpu/command_buffer/service/gles2_cmd_decoder.cc:12601: if (!texture_manager()->ValidForTarget(target, level, width, height, 1)) { Please look at CopyTexSubImage2D. You will need more validation here, for example, if the fbo is complete or not, if the src/dst formats are compatible, etc. https://codereview.chromium.org/2182443003/diff/80001/gpu/command_buffer/serv... gpu/command_buffer/service/gles2_cmd_decoder.cc:12604: "glCopyTexSubImage3D", "dimensions out of range"); nit: define a const char* function_name, so you don't have to repeat this string multiple times. https://codereview.chromium.org/2182443003/diff/80001/gpu/command_buffer/serv... File gpu/command_buffer/service/gles2_cmd_decoder_unittest_1_autogen.h (left): https://codereview.chromium.org/2182443003/diff/80001/gpu/command_buffer/serv... gpu/command_buffer/service/gles2_cmd_decoder_unittest_1_autogen.h:320: TEST_P(GLES2DecoderTest1, CopyTexSubImage3DValidArgs) { Please add a manual GLES3DecoderTest CopyTexSubImage3DValidArgs test in gles2_cmd_decoder_unittest_textures
Thanks for your quick review, Zhenyao. I will update the code accordingly. https://codereview.chromium.org/2182443003/diff/80001/gpu/command_buffer/serv... File gpu/command_buffer/service/gles2_cmd_decoder.cc (right): https://codereview.chromium.org/2182443003/diff/80001/gpu/command_buffer/serv... gpu/command_buffer/service/gles2_cmd_decoder.cc:12601: if (!texture_manager()->ValidForTarget(target, level, width, height, 1)) { On 2016/07/26 13:25:55, Zhenyao Mo wrote: > Please look at CopyTexSubImage2D. You will need more validation here, for > example, if the fbo is complete or not, if the src/dst formats are compatible, > etc. Acknowledged. https://codereview.chromium.org/2182443003/diff/80001/gpu/command_buffer/serv... gpu/command_buffer/service/gles2_cmd_decoder.cc:12604: "glCopyTexSubImage3D", "dimensions out of range"); On 2016/07/26 13:25:55, Zhenyao Mo wrote: > nit: define a const char* function_name, so you don't have to repeat this string > multiple times. Acknowledged. https://codereview.chromium.org/2182443003/diff/80001/gpu/command_buffer/serv... File gpu/command_buffer/service/gles2_cmd_decoder_unittest_1_autogen.h (left): https://codereview.chromium.org/2182443003/diff/80001/gpu/command_buffer/serv... gpu/command_buffer/service/gles2_cmd_decoder_unittest_1_autogen.h:320: TEST_P(GLES2DecoderTest1, CopyTexSubImage3DValidArgs) { On 2016/07/26 13:25:55, Zhenyao Mo wrote: > Please add a manual GLES3DecoderTest CopyTexSubImage3DValidArgs test in > gles2_cmd_decoder_unittest_textures Yeah. I am doing this, but have some bugs currently... :(
Patchset #3 (id:100001) 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: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-gn on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-gn/bui...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-gn on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-gn/...)
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...
Description was changed from ========== WebGL 2: Fix bugs in negativetextureapi.html BUG=565347 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 ========== WebGL 2: Fix bugs in negativetextureapi.html for Mac BUG=631934 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.
yunchao.he@intel.com changed reviewers: + piman@chromium.org
Zhenyao (and all), please take another look. The code has been updated. Some comments from http://crbug.com/631934: there are some other problems in the command buffer for CopyTex{Sub}Image{2D|3D} APIs. I want to improve these APIs step by step as below: 1) Add validation code for CopyTexSubImage3D to fix the bug for Mac. 2) Add more code to handle the out-of-bounds reading for CopyTexSubImage3D. 3) Do 1) and 2) for CopyTexImage3D in one step. 4) Remove duplicated code, unify all related APIs (CopyTex{Sub}Image{2D|3D}). This CL mainly do validation, in order to fix the bug for Mac.
Mostly looks good to me. https://codereview.chromium.org/2182443003/diff/160001/gpu/command_buffer/ser... File gpu/command_buffer/service/gles2_cmd_decoder.cc (right): https://codereview.chromium.org/2182443003/diff/160001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_decoder.cc:13297: height); extra space before "height" https://codereview.chromium.org/2182443003/diff/160001/gpu/command_buffer/ser... File gpu/command_buffer/service/gles2_cmd_decoder_unittest_3.cc (right): https://codereview.chromium.org/2182443003/diff/160001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_decoder_unittest_3.cc:119: void GLES2DecoderTestBase::SpecializedSetup<cmds::CopyTexSubImage3D, 0>( What's this setup used for? https://codereview.chromium.org/2182443003/diff/160001/gpu/command_buffer/ser... File gpu/command_buffer/service/gles2_cmd_decoder_unittest_textures.cc (right): https://codereview.chromium.org/2182443003/diff/160001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_decoder_unittest_textures.cc:1084: { {} is not required. https://codereview.chromium.org/2182443003/diff/160001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_decoder_unittest_textures.cc:1142: cmd.Init(kTarget, 1, 0, 0, 0, 0, 0, kWidth, kHeight); What's the format of read buffer here?
https://codereview.chromium.org/2182443003/diff/160001/gpu/command_buffer/ser... File gpu/command_buffer/service/gles2_cmd_decoder.cc (right): https://codereview.chromium.org/2182443003/diff/160001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_decoder.cc:13296: glCopyTexSubImage3D(target, level, xoffset, yoffset, zoffset, x, y, width, Here you still miss a bunch of stuff. 1) feedback look detection 2) emulation of unsized formats in core profile, 3) make sure we clear the 3D textures if it's uncleared. It's OK to do it in a followup CL, but at least add a TODO. https://codereview.chromium.org/2182443003/diff/160001/gpu/command_buffer/ser... File gpu/command_buffer/service/gles2_cmd_decoder_unittest_3.cc (right): https://codereview.chromium.org/2182443003/diff/160001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_decoder_unittest_3.cc:119: void GLES2DecoderTestBase::SpecializedSetup<cmds::CopyTexSubImage3D, 0>( On 2016/07/27 13:56:26, qiankun wrote: > What's this setup used for? I don't think this is necessary. For the manually written tests, all the setups can go into the test. https://codereview.chromium.org/2182443003/diff/160001/gpu/command_buffer/ser... File gpu/command_buffer/service/gles2_cmd_decoder_unittest_textures.cc (right): https://codereview.chromium.org/2182443003/diff/160001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_decoder_unittest_textures.cc:1111: CopyTexSubImage3D(kTarget, 1, 0, 0, 0, 0, 0, kWidth, kHeight)) Can you use kLevel, kXOffset, kYOffset, kZOffset, etc? Otherwise, code is hard to read. https://codereview.chromium.org/2182443003/diff/160001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_decoder_unittest_textures.cc:1128: cmd.Init(kTarget, 0, 0, 0, 0, 0, 0, kWidth, kHeight); Same here, please define constants and use it. https://codereview.chromium.org/2182443003/diff/160001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_decoder_unittest_textures.cc:1142: cmd.Init(kTarget, 1, 0, 0, 0, 0, 0, kWidth, kHeight); Same here, please define constants and use it. It's better to combine the BadArgs tests with the ValidArgs tests, so with all valid args, verify the command execution is good. Then everytime change on arg to something illegal and verify it fails.
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, Zhenyao and Qiankun. Code has been updated accordingly. PTAL. https://codereview.chromium.org/2182443003/diff/160001/gpu/command_buffer/ser... File gpu/command_buffer/service/gles2_cmd_decoder.cc (right): https://codereview.chromium.org/2182443003/diff/160001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_decoder.cc:13296: glCopyTexSubImage3D(target, level, xoffset, yoffset, zoffset, x, y, width, On 2016/07/27 17:38:29, Zhenyao Mo wrote: > Here you still miss a bunch of stuff. 1) feedback look detection 2) emulation of > unsized formats in core profile, 3) make sure we clear the 3D textures if it's > uncleared. > > It's OK to do it in a followup CL, but at least add a TODO. Done. https://codereview.chromium.org/2182443003/diff/160001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_decoder.cc:13297: height); On 2016/07/27 13:56:26, qiankun wrote: > extra space before "height" Done. https://codereview.chromium.org/2182443003/diff/160001/gpu/command_buffer/ser... File gpu/command_buffer/service/gles2_cmd_decoder_unittest_3.cc (right): https://codereview.chromium.org/2182443003/diff/160001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_decoder_unittest_3.cc:119: void GLES2DecoderTestBase::SpecializedSetup<cmds::CopyTexSubImage3D, 0>( On 2016/07/27 17:38:29, Zhenyao Mo wrote: > On 2016/07/27 13:56:26, qiankun wrote: > > What's this setup used for? > > I don't think this is necessary. For the manually written tests, all the setups > can go into the test. That's true. This code snippet is not necessary. Done. https://codereview.chromium.org/2182443003/diff/160001/gpu/command_buffer/ser... File gpu/command_buffer/service/gles2_cmd_decoder_unittest_textures.cc (right): https://codereview.chromium.org/2182443003/diff/160001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_decoder_unittest_textures.cc:1084: { On 2016/07/27 13:56:26, qiankun wrote: > {} is not required. Done. https://codereview.chromium.org/2182443003/diff/160001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_decoder_unittest_textures.cc:1111: CopyTexSubImage3D(kTarget, 1, 0, 0, 0, 0, 0, kWidth, kHeight)) On 2016/07/27 17:38:29, Zhenyao Mo wrote: > Can you use kLevel, kXOffset, kYOffset, kZOffset, etc? Otherwise, code is hard > to read. Done. https://codereview.chromium.org/2182443003/diff/160001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_decoder_unittest_textures.cc:1128: cmd.Init(kTarget, 0, 0, 0, 0, 0, 0, kWidth, kHeight); On 2016/07/27 17:38:29, Zhenyao Mo wrote: > Same here, please define constants and use it. Done. https://codereview.chromium.org/2182443003/diff/160001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_decoder_unittest_textures.cc:1142: cmd.Init(kTarget, 1, 0, 0, 0, 0, 0, kWidth, kHeight); On 2016/07/27 13:56:26, qiankun wrote: > What's the format of read buffer here? AFAIK, the default format/type of the default read buffer is RGB/UNSIGNED_BYTE. https://codereview.chromium.org/2182443003/diff/160001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_decoder_unittest_textures.cc:1142: cmd.Init(kTarget, 1, 0, 0, 0, 0, 0, kWidth, kHeight); On 2016/07/27 17:38:29, Zhenyao Mo wrote: > Same here, please define constants and use it. > > It's better to combine the BadArgs tests with the ValidArgs tests, so with all > valid args, verify the command execution is good. Then everytime change on arg > to something illegal and verify it fails. Good suggestion. It would be much clear. Done.
lgtm with nits https://codereview.chromium.org/2182443003/diff/180001/gpu/command_buffer/ser... File gpu/command_buffer/service/gles2_cmd_decoder.cc (right): https://codereview.chromium.org/2182443003/diff/180001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_decoder.cc:13298: // textures if it is uncleared, out-of-bounds reading, etc. Please do follow up. Can you also add a conformance test? Say you create a 3D texture without data, and then do CopyTexSubImage3D to one of the level/slice. At this point, in Chrome the texture is still marked as uninitialized. Now you try to render using the that slice. You will find it's been initialized to 0 instead of the data CopyTexSubImage3D wrote into. Once the test is added, we can get a CL to fix it. https://codereview.chromium.org/2182443003/diff/180001/gpu/command_buffer/ser... File gpu/command_buffer/service/gles2_cmd_decoder_unittest_textures.cc (right): https://codereview.chromium.org/2182443003/diff/180001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_decoder_unittest_textures.cc:1116: cmd.Init(kTarget, 1, 0, 0, 0, 0, 0, kWidth, kHeight); Same here, use kLevel etc https://codereview.chromium.org/2182443003/diff/180001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_decoder_unittest_textures.cc:1116: cmd.Init(kTarget, 1, 0, 0, 0, 0, 0, kWidth, kHeight); Here you need a comment that the FBO is RGB and UNSIGNED_BYTE. https://codereview.chromium.org/2182443003/diff/180001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_decoder_unittest_textures.cc:1192: cmd.Init(kTarget, kLevel, kXoffset, kYoffset, 2, nit: use kDepth instead of 2 is better.
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...
Thanks for your quick review, Zhenyao. https://codereview.chromium.org/2182443003/diff/180001/gpu/command_buffer/ser... File gpu/command_buffer/service/gles2_cmd_decoder.cc (right): https://codereview.chromium.org/2182443003/diff/180001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_decoder.cc:13298: // textures if it is uncleared, out-of-bounds reading, etc. On 2016/07/28 00:10:54, Zhenyao Mo wrote: > Please do follow up. Yes. In fact, I have already had a CL to handle the feedback loop issue. I will improve it and submit it to review today. > Can you also add a conformance test? Say you create a 3D > texture without data, and then do CopyTexSubImage3D to one of the level/slice. > At this point, in Chrome the texture is still marked as uninitialized. Now you > try to render using the that slice. You will find it's been initialized to 0 > instead of the data CopyTexSubImage3D wrote into. > > Once the test is added, we can get a CL to fix it. OK. I will add this test case in gpu_unittests by follow-up CL, as well as the code to fix it in the same CL. https://codereview.chromium.org/2182443003/diff/180001/gpu/command_buffer/ser... File gpu/command_buffer/service/gles2_cmd_decoder_unittest_textures.cc (right): https://codereview.chromium.org/2182443003/diff/180001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_decoder_unittest_textures.cc:1116: cmd.Init(kTarget, 1, 0, 0, 0, 0, 0, kWidth, kHeight); On 2016/07/28 00:10:54, Zhenyao Mo wrote: > Here you need a comment that the FBO is RGB and UNSIGNED_BYTE. Done. https://codereview.chromium.org/2182443003/diff/180001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_decoder_unittest_textures.cc:1116: cmd.Init(kTarget, 1, 0, 0, 0, 0, 0, kWidth, kHeight); On 2016/07/28 00:10:54, Zhenyao Mo wrote: > Same here, use kLevel etc Done. https://codereview.chromium.org/2182443003/diff/180001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_decoder_unittest_textures.cc:1192: cmd.Init(kTarget, kLevel, kXoffset, kYoffset, 2, On 2016/07/28 00:10:54, Zhenyao Mo wrote: > nit: use kDepth instead of 2 is better. Done.
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/2182443003/#ps200001 (title: "addressed zmo's feedback")
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 ========== WebGL 2: Fix bugs in negativetextureapi.html for Mac BUG=631934 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 ========== WebGL 2: Fix bugs in negativetextureapi.html for Mac BUG=631934 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:200001)
Message was sent while issue was closed.
Description was changed from ========== WebGL 2: Fix bugs in negativetextureapi.html for Mac BUG=631934 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 ========== WebGL 2: Fix bugs in negativetextureapi.html for Mac BUG=631934 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/4771076dd784de40192c7249914a2fbb208e7a49 Cr-Commit-Position: refs/heads/master@{#408313} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/4771076dd784de40192c7249914a2fbb208e7a49 Cr-Commit-Position: refs/heads/master@{#408313}
Message was sent while issue was closed.
https://codereview.chromium.org/2182443003/diff/180001/gpu/command_buffer/ser... File gpu/command_buffer/service/gles2_cmd_decoder.cc (right): https://codereview.chromium.org/2182443003/diff/180001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_decoder.cc:13298: // textures if it is uncleared, out-of-bounds reading, etc. On 2016/07/28 00:41:04, yunchao wrote: > On 2016/07/28 00:10:54, Zhenyao Mo wrote: > > Please do follow up. > > Yes. In fact, I have already had a CL to handle the feedback loop issue. I will > improve it and submit it to review today. > > > Can you also add a conformance test? Say you create a 3D > > texture without data, and then do CopyTexSubImage3D to one of the level/slice. > > > At this point, in Chrome the texture is still marked as uninitialized. Now you > > try to render using the that slice. You will find it's been initialized to 0 > > instead of the data CopyTexSubImage3D wrote into. > > > > Once the test is added, we can get a CL to fix it. > > OK. I will add this test case in gpu_unittests by follow-up CL, as well as the > code to fix it in the same CL. I suggest adding a webgl2 conformance test instead of a unit test.
Message was sent while issue was closed.
On 2016/07/28 05:13:26, Zhenyao Mo wrote: > https://codereview.chromium.org/2182443003/diff/180001/gpu/command_buffer/ser... > File gpu/command_buffer/service/gles2_cmd_decoder.cc (right): > > https://codereview.chromium.org/2182443003/diff/180001/gpu/command_buffer/ser... > gpu/command_buffer/service/gles2_cmd_decoder.cc:13298: // textures if it is > uncleared, out-of-bounds reading, etc. > On 2016/07/28 00:41:04, yunchao wrote: > > On 2016/07/28 00:10:54, Zhenyao Mo wrote: > > > Please do follow up. > > > > Yes. In fact, I have already had a CL to handle the feedback loop issue. I > will > > improve it and submit it to review today. > > > > > Can you also add a conformance test? Say you create a 3D > > > texture without data, and then do CopyTexSubImage3D to one of the > level/slice. > > > > > At this point, in Chrome the texture is still marked as uninitialized. Now > you > > > try to render using the that slice. You will find it's been initialized to 0 > > > instead of the data CopyTexSubImage3D wrote into. > > > > > > Once the test is added, we can get a CL to fix it. > > > > OK. I will add this test case in gpu_unittests by follow-up CL, as well as the > > code to fix it in the same CL. > > I suggest adding a webgl2 conformance test instead of a unit test. Got it. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
