Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1750123002:
Upgrade Tex{Sub}Image{2|3}D to handle ES3 unpack parameters. (Closed)
|
Created:
4 years, 9 months ago by Zhenyao Mo Modified:
4 years, 9 months ago Reviewers:
piman CC:
chromium-reviews, piman+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionUpgrade Tex{Sub}Image{2|3}D to handle ES3 unpack parameters.
Also, this CL changed the SKIP_* unpack parameters to be fully handled
on the client side and never get sent down to the service side.
Also, this CL fixed a bug in ClearLevel3D. If an unpack buffer is bound,
unpack parameters have been submitted to the driver already. So when we
call TexSubImage3D to clear a 3D texture, we need to clear these parameters
if set by client, and restore them afterwards.
BUG=487841, 429053
TEST=gpu_unittests,webgl_conformance,webgl2_conformance
R=piman@chromium.org
CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel
Committed: https://crrev.com/4a658ee4067d5256885276c95a52ac393f11e2c0
Cr-Commit-Position: refs/heads/master@{#380752}
Patch Set 1 : #Patch Set 2 : #Patch Set 3 : #Patch Set 4 : #Patch Set 5 : #Patch Set 6 : working version #Patch Set 7 : #Patch Set 8 : #
Total comments: 15
Patch Set 9 : Clean rebase #Patch Set 10 : #Patch Set 11 : #Patch Set 12 : rebase after crrev.com/1785483002 #Patch Set 13 : Fix minor bugs #Patch Set 14 : rebase after crrev.com/1787503002 #Patch Set 15 : pure rebase #Patch Set 16 : Attemp to fix random crashes on windows #Messages
Total messages: 67 (33 generated)
Description was changed from
==========
Upgrade Tex{Sub}Image{2|3}D to handle ES3 unpack parameters.
Also, this CL changes the SKIP_* unpack parameters to be fully handled
on the client side and never get sent down to the service side.
BUG=487841,429053
TEST=gpu_unittests,webgl_conformance,webgl2_conformance
R=piman@chromium.org
==========
to
==========
Upgrade Tex{Sub}Image{2|3}D to handle ES3 unpack parameters.
Also, this CL changes the SKIP_* unpack parameters to be fully handled
on the client side and never get sent down to the service side.
BUG=487841,429053
TEST=gpu_unittests,webgl_conformance,webgl2_conformance
R=piman@chromium.org
CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel
==========
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/patch-status/1750123002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1750123002/1
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win8_chromium_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_ng/...)
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/patch-status/1750123002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1750123002/20001
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 to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1750123002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1750123002/40001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
Patchset #4 (id:60001) has been deleted
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/patch-status/1750123002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1750123002/80001
Description was changed from
==========
Upgrade Tex{Sub}Image{2|3}D to handle ES3 unpack parameters.
Also, this CL changes the SKIP_* unpack parameters to be fully handled
on the client side and never get sent down to the service side.
BUG=487841,429053
TEST=gpu_unittests,webgl_conformance,webgl2_conformance
R=piman@chromium.org
CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel
==========
to
==========
Upgrade Tex{Sub}Image{2|3}D to handle ES3 unpack parameters.
Also, this CL changed the SKIP_* unpack parameters to be fully handled
on the client side and never get sent down to the service side.
Also, this CL fixed a bug in ClearLevel3D. If an unpack buffer is bound,
unpack parameters have been submitted to the driver already. So when we
call TexSubImage3D to clear a 3D texture, we need to clear these parameters
if set by client, and restore them afterwards.
BUG=487841,429053
TEST=gpu_unittests,webgl_conformance,webgl2_conformance
R=piman@chromium.org
CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel
==========
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/patch-status/1750123002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1750123002/100001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
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/patch-status/1750123002/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1750123002/140001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
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/patch-status/1750123002/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1750123002/160001
Patchset #1 (id:1) has been deleted
Patchset #7 (id:160001) has been deleted
piman: PTAL We have to resolve the random crashes on win bot before landing this. The webgl conformance test (https://www.khronos.org/registry/webgl/sdk/tests/conformance2/textures/misc/t...) is written to make sure the code in this CL works well.
Dry run: Exceeded global retry quota
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
Patchset #7 (id:180001) has been deleted
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/patch-status/1750123002/200001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1750123002/200001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
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/patch-status/1750123002/220001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1750123002/220001
https://codereview.chromium.org/1750123002/diff/220001/gpu/command_buffer/ser... File gpu/command_buffer/service/gles2_cmd_decoder.cc (right): https://codereview.chromium.org/1750123002/diff/220001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_decoder.cc:9563: NOTREACHED(); nit: no NOTREACHED() since this can be reached by a malicious compromised renderer / nacl module. https://codereview.chromium.org/1750123002/diff/220001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_decoder.cc:11263: DCHECK_EQ(0u, pixels_shm_id); Is this DCHECK valid? We can't trust the client. We probably need to return kInvalidArguments if this happens. https://codereview.chromium.org/1750123002/diff/220001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_decoder.cc:11274: DCHECK_EQ(0u, pixels_shm_offset); Same here. https://codereview.chromium.org/1750123002/diff/220001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_decoder.cc:11356: DCHECK_EQ(0u, pixels_shm_id); Same as above. https://codereview.chromium.org/1750123002/diff/220001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_decoder.cc:11367: DCHECK_EQ(0u, pixels_shm_offset); And here. https://codereview.chromium.org/1750123002/diff/220001/gpu/command_buffer/ser... File gpu/command_buffer/service/texture_manager.cc (right): https://codereview.chromium.org/1750123002/diff/220001/gpu/command_buffer/ser... gpu/command_buffer/service/texture_manager.cc:2147: if (buffer_size % type_size != 0) { Why this check? The buffer size doesn't matter, as long as it contains enough data. https://codereview.chromium.org/1750123002/diff/220001/gpu/command_buffer/ser... gpu/command_buffer/service/texture_manager.cc:2312: if (buffer_size % type_size != 0) { Same here
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
Revised, but I still haven't figured out the win crash issue - can't reproduce locally. https://codereview.chromium.org/1750123002/diff/220001/gpu/command_buffer/ser... File gpu/command_buffer/service/gles2_cmd_decoder.cc (right): https://codereview.chromium.org/1750123002/diff/220001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_decoder.cc:9563: NOTREACHED(); On 2016/03/08 01:56:50, piman wrote: > nit: no NOTREACHED() since this can be reached by a malicious compromised > renderer / nacl module. Done. https://codereview.chromium.org/1750123002/diff/220001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_decoder.cc:11263: DCHECK_EQ(0u, pixels_shm_id); On 2016/03/08 01:56:50, piman wrote: > Is this DCHECK valid? We can't trust the client. We probably need to return > kInvalidArguments if this happens. Done. https://codereview.chromium.org/1750123002/diff/220001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_decoder.cc:11274: DCHECK_EQ(0u, pixels_shm_offset); On 2016/03/08 01:56:50, piman wrote: > Same here. Done. https://codereview.chromium.org/1750123002/diff/220001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_decoder.cc:11356: DCHECK_EQ(0u, pixels_shm_id); On 2016/03/08 01:56:50, piman wrote: > Same as above. Done. https://codereview.chromium.org/1750123002/diff/220001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_decoder.cc:11367: DCHECK_EQ(0u, pixels_shm_offset); On 2016/03/08 01:56:50, piman wrote: > And here. Done. https://codereview.chromium.org/1750123002/diff/220001/gpu/command_buffer/ser... File gpu/command_buffer/service/texture_manager.cc (right): https://codereview.chromium.org/1750123002/diff/220001/gpu/command_buffer/ser... gpu/command_buffer/service/texture_manager.cc:2147: if (buffer_size % type_size != 0) { On 2016/03/08 01:56:50, piman wrote: > Why this check? The buffer size doesn't matter, as long as it contains enough > data. See ES Spec 3.0.4 page 113, under $3.7 -> Unpacking. https://codereview.chromium.org/1750123002/diff/220001/gpu/command_buffer/ser... gpu/command_buffer/service/texture_manager.cc:2312: if (buffer_size % type_size != 0) { On 2016/03/08 01:56:50, piman wrote: > Same here Same answer. It is specified in a difference place other than Tex functions.
https://codereview.chromium.org/1750123002/diff/220001/gpu/command_buffer/ser... File gpu/command_buffer/service/texture_manager.cc (right): https://codereview.chromium.org/1750123002/diff/220001/gpu/command_buffer/ser... gpu/command_buffer/service/texture_manager.cc:2147: if (buffer_size % type_size != 0) { On 2016/03/09 18:41:05, Zhenyao Mo wrote: > On 2016/03/08 01:56:50, piman wrote: > > Why this check? The buffer size doesn't matter, as long as it contains enough > > data. > > See ES Spec 3.0.4 page 113, under $3.7 -> Unpacking. I still don't get it. The 2 requirements are: 1- the data doesn't go beyond the buffer 2- the data is aligned to the underlying type. Are you trying to do #2 here? you'd want to check the offset (args.pixels), not the buffer size.
On 2016/03/09 18:52:05, piman wrote: > https://codereview.chromium.org/1750123002/diff/220001/gpu/command_buffer/ser... > File gpu/command_buffer/service/texture_manager.cc (right): > > https://codereview.chromium.org/1750123002/diff/220001/gpu/command_buffer/ser... > gpu/command_buffer/service/texture_manager.cc:2147: if (buffer_size % type_size > != 0) { > On 2016/03/09 18:41:05, Zhenyao Mo wrote: > > On 2016/03/08 01:56:50, piman wrote: > > > Why this check? The buffer size doesn't matter, as long as it contains > enough > > > data. > > > > See ES Spec 3.0.4 page 113, under $3.7 -> Unpacking. > > I still don't get it. The 2 requirements are: > 1- the data doesn't go beyond the buffer > 2- the data is aligned to the underlying type. > > Are you trying to do #2 here? you'd want to check the offset (args.pixels), not > the buffer size. You are right. I misunderstood the spec. Updated.
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/patch-status/1750123002/300001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1750123002/300001
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 to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1750123002/340001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1750123002/340001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Patchset #14 (id:340001) has been deleted
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/patch-status/1750123002/400001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1750123002/400001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by zmo@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from piman@chromium.org Link to the patchset: https://codereview.chromium.org/1750123002/#ps400001 (title: "Attemp to fix random crashes on windows")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1750123002/400001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1750123002/400001
Message was sent while issue was closed.
Committed patchset #16 (id:400001)
Message was sent while issue was closed.
Description was changed from
==========
Upgrade Tex{Sub}Image{2|3}D to handle ES3 unpack parameters.
Also, this CL changed the SKIP_* unpack parameters to be fully handled
on the client side and never get sent down to the service side.
Also, this CL fixed a bug in ClearLevel3D. If an unpack buffer is bound,
unpack parameters have been submitted to the driver already. So when we
call TexSubImage3D to clear a 3D texture, we need to clear these parameters
if set by client, and restore them afterwards.
BUG=487841,429053
TEST=gpu_unittests,webgl_conformance,webgl2_conformance
R=piman@chromium.org
CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel
==========
to
==========
Upgrade Tex{Sub}Image{2|3}D to handle ES3 unpack parameters.
Also, this CL changed the SKIP_* unpack parameters to be fully handled
on the client side and never get sent down to the service side.
Also, this CL fixed a bug in ClearLevel3D. If an unpack buffer is bound,
unpack parameters have been submitted to the driver already. So when we
call TexSubImage3D to clear a 3D texture, we need to clear these parameters
if set by client, and restore them afterwards.
BUG=487841,429053
TEST=gpu_unittests,webgl_conformance,webgl2_conformance
R=piman@chromium.org
CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel
Committed: https://crrev.com/4a658ee4067d5256885276c95a52ac393f11e2c0
Cr-Commit-Position: refs/heads/master@{#380752}
==========
Message was sent while issue was closed.
Patchset 16 (id:??) landed as https://crrev.com/4a658ee4067d5256885276c95a52ac393f11e2c0 Cr-Commit-Position: refs/heads/master@{#380752}
Message was sent while issue was closed.
Post my findings for the random crashes on win bots observed in any CLs except the last one. 1) in these CLs, I did an optimization when TexImage is called with data = nullptr, I skip calculating the data size, (thus the variable holding data size is set to 0) 2) on top of ANGLE, we have workarounds for cubemaps that we internally allocate all faces to make it cube complete, where they use this data size (=0) to create a buffer that's all initialized to zero and feed it to all cube map faces. 3) apparently new char[0] returns non null, and feeding this pointer to TexImage caused random crashes (in driver I assume) because this pointer points to no data, but since it's non null, driver assumes it points to a buffer large enough to hold full pixels. |
