|
|
Created:
4 years, 6 months ago by Zhenyao Mo Modified:
4 years, 6 months ago CC:
chromium-reviews, Ken Russell (switch to Gerrit), piman+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionImprove CopyTex{Sub}Image2D format validation.
1) Unify the validation code into a helper function.
2) In ES3, for sized formats, the read_format and internal_format's color bits
have to match exactly.
BUG=429053
TEST=gpu_unittests,webgl{2}_conformance
R=piman@chromium.org
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel;tryserver.chromium.win:win_optional_gpu_tests_rel
Committed: https://crrev.com/25966f2863c58fb04faa48986150c363d30be6fa
Cr-Commit-Position: refs/heads/master@{#397913}
Patch Set 1 #Patch Set 2 : with fix #
Total comments: 4
Patch Set 3 : fix #Patch Set 4 : #
Total comments: 3
Patch Set 5 : #
Messages
Total messages: 39 (16 generated)
Description was changed from ========== Improve CopyTex{Sub}Image2D format validation. 1) Unify the validation code into a helper function. 2) In ES3, for sized formats, the read_format and internal_format's color bits have to match exactly. BUG=429053 TEST=gpu_unittests,webgl{2}_conformance R=piman@chromium.org ========== to ========== Improve CopyTex{Sub}Image2D format validation. 1) Unify the validation code into a helper function. 2) In ES3, for sized formats, the read_format and internal_format's color bits have to match exactly. BUG=429053 TEST=gpu_unittests,webgl{2}_conformance R=piman@chromium.org CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel;tryserver.chromium.win:win_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/2029803002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2029803002/1
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Note that patch set 1 is refactoring, patch set 2 is adding the fix. piman: PTAL kbr, cwallez: FYI (feel free to review)
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/2029803002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2029803002/20001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
kbr@chromium.org changed reviewers: + kbr@chromium.org
Nice fix. LGTM for what it's worth.
On 2016/06/02 at 18:15:44, zmo wrote: > Note that patch set 1 is refactoring, patch set 2 is adding the fix. > > piman: PTAL > kbr, cwallez: FYI (feel free to review) lgtm, some small comments.
cwallez@chromium.org changed reviewers: + cwallez@chromium.org
https://codereview.chromium.org/2029803002/diff/20001/content/test/gpu/gpu_te... File content/test/gpu/gpu_tests/webgl2_conformance_expectations.py (left): https://codereview.chromium.org/2029803002/diff/20001/content/test/gpu/gpu_te... content/test/gpu/gpu_tests/webgl2_conformance_expectations.py:113: ['win'], bug=483282) likewise from Windows only to general. https://codereview.chromium.org/2029803002/diff/20001/gpu/command_buffer/comm... File gpu/command_buffer/common/gles2_cmd_utils.cc (right): https://codereview.chromium.org/2029803002/diff/20001/gpu/command_buffer/comm... gpu/command_buffer/common/gles2_cmd_utils.cc:1264: default: All the default cases could be UNREACHABLE()
https://codereview.chromium.org/2029803002/diff/20001/content/test/gpu/gpu_te... File content/test/gpu/gpu_tests/webgl2_conformance_expectations.py (left): https://codereview.chromium.org/2029803002/diff/20001/content/test/gpu/gpu_te... content/test/gpu/gpu_tests/webgl2_conformance_expectations.py:113: ['win'], bug=483282) On 2016/06/02 21:29:40, Corentin Wallez wrote: > likewise from Windows only to general. I actually did that :) https://codereview.chromium.org/2029803002/diff/20001/gpu/command_buffer/comm... File gpu/command_buffer/common/gles2_cmd_utils.cc (right): https://codereview.chromium.org/2029803002/diff/20001/gpu/command_buffer/comm... gpu/command_buffer/common/gles2_cmd_utils.cc:1264: default: On 2016/06/02 21:29:40, Corentin Wallez wrote: > All the default cases could be UNREACHABLE() Good idea. Done.
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/2029803002/40001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)
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/2029803002/60001
https://codereview.chromium.org/2029803002/diff/60001/gpu/command_buffer/comm... File gpu/command_buffer/common/gles2_cmd_utils.cc (right): https://codereview.chromium.org/2029803002/diff/60001/gpu/command_buffer/comm... gpu/command_buffer/common/gles2_cmd_utils.cc:1337: default: I think anything valid in FormatTypeValidator::kSupportedFormatTypes in texture_manager.cc can end up here, some combinations are exposed by extensions. E.g. GL_ALPHA/GL_UNSIGNED_BYTE, */GL_HALF_FLOAT_OES, */GL_FLOAT, GL_RED/*, GL_RG/*
https://codereview.chromium.org/2029803002/diff/60001/gpu/command_buffer/comm... File gpu/command_buffer/common/gles2_cmd_utils.cc (right): https://codereview.chromium.org/2029803002/diff/60001/gpu/command_buffer/comm... gpu/command_buffer/common/gles2_cmd_utils.cc:1337: default: On 2016/06/03 21:26:04, piman OOO back 2016-6-2 wrote: > I think anything valid in FormatTypeValidator::kSupportedFormatTypes in > texture_manager.cc can end up here, some combinations are exposed by extensions. > E.g. GL_ALPHA/GL_UNSIGNED_BYTE, */GL_HALF_FLOAT_OES, */GL_FLOAT, GL_RED/*, > GL_RG/* This is ES3 only, so all these extensions have been folded into core as sized formats. Except for the GL_ALPHA/GL_UNSIGNED_BYTE, which I left out and am fixing now.
https://codereview.chromium.org/2029803002/diff/60001/gpu/command_buffer/comm... File gpu/command_buffer/common/gles2_cmd_utils.cc (right): https://codereview.chromium.org/2029803002/diff/60001/gpu/command_buffer/comm... gpu/command_buffer/common/gles2_cmd_utils.cc:1337: default: On 2016/06/03 21:31:27, Zhenyao Mo wrote: > On 2016/06/03 21:26:04, piman OOO back 2016-6-2 wrote: > > I think anything valid in FormatTypeValidator::kSupportedFormatTypes in > > texture_manager.cc can end up here, some combinations are exposed by > extensions. > > E.g. GL_ALPHA/GL_UNSIGNED_BYTE, */GL_HALF_FLOAT_OES, */GL_FLOAT, GL_RED/*, > > GL_RG/* > > This is ES3 only, so all these extensions have been folded into core as sized > formats. Except for the GL_ALPHA/GL_UNSIGNED_BYTE, which I left out and am > fixing now. AFAICT, even if we enable ES3, we still enable e.g. GL_OES_texture_float, which means it's still valid for the client to call TexImage2D(GL_TEXTURE_2D, 0, GL_RGBA, ..., GL_RGBA, GL_FLOAT, NULL). I don't think we rename that GL_RGBA internal format to GL_RGBA32F anywhere (maybe we should when on ES3?)
On 2016/06/03 21:40:57, piman OOO back 2016-6-2 wrote: > https://codereview.chromium.org/2029803002/diff/60001/gpu/command_buffer/comm... > File gpu/command_buffer/common/gles2_cmd_utils.cc (right): > > https://codereview.chromium.org/2029803002/diff/60001/gpu/command_buffer/comm... > gpu/command_buffer/common/gles2_cmd_utils.cc:1337: default: > On 2016/06/03 21:31:27, Zhenyao Mo wrote: > > On 2016/06/03 21:26:04, piman OOO back 2016-6-2 wrote: > > > I think anything valid in FormatTypeValidator::kSupportedFormatTypes in > > > texture_manager.cc can end up here, some combinations are exposed by > > extensions. > > > E.g. GL_ALPHA/GL_UNSIGNED_BYTE, */GL_HALF_FLOAT_OES, */GL_FLOAT, GL_RED/*, > > > GL_RG/* > > > > This is ES3 only, so all these extensions have been folded into core as sized > > formats. Except for the GL_ALPHA/GL_UNSIGNED_BYTE, which I left out and am > > fixing now. > > AFAICT, even if we enable ES3, we still enable e.g. GL_OES_texture_float, which > means it's still valid for the client to call TexImage2D(GL_TEXTURE_2D, 0, > GL_RGBA, ..., GL_RGBA, GL_FLOAT, NULL). I don't think we rename that GL_RGBA > internal format to GL_RGBA32F anywhere (maybe we should when on ES3?) OK, this is a helper function, so I guess it's always OK to handle all cases. Validators should worry about which is allowed and which isn't.
Patchset #5 (id:80001) has been deleted
The CQ bit was checked by zmo@chromium.org to run a CQ dry run
Revised. Please take another look.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2029803002/100001
LGTM, thanks.
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, cwallez@chromium.org Link to the patchset: https://codereview.chromium.org/2029803002/#ps100001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2029803002/100001
Message was sent while issue was closed.
Committed patchset #5 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== Improve CopyTex{Sub}Image2D format validation. 1) Unify the validation code into a helper function. 2) In ES3, for sized formats, the read_format and internal_format's color bits have to match exactly. BUG=429053 TEST=gpu_unittests,webgl{2}_conformance R=piman@chromium.org CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel;tryserver.chromium.win:win_optional_gpu_tests_rel ========== to ========== Improve CopyTex{Sub}Image2D format validation. 1) Unify the validation code into a helper function. 2) In ES3, for sized formats, the read_format and internal_format's color bits have to match exactly. BUG=429053 TEST=gpu_unittests,webgl{2}_conformance R=piman@chromium.org CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel;tryserver.chromium.win:win_optional_gpu_tests_rel Committed: https://crrev.com/25966f2863c58fb04faa48986150c363d30be6fa Cr-Commit-Position: refs/heads/master@{#397913} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/25966f2863c58fb04faa48986150c363d30be6fa Cr-Commit-Position: refs/heads/master@{#397913} |