|
|
Created:
3 years, 7 months ago by yizhou.jiang Modified:
3 years, 7 months ago CC:
chromium-reviews, piman+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionFollow up immutable tex srgb emulation issue of generateMipmap.
when calculating max available mipmap level, as base level can
be set by client to any value, so there is an overflow danger.
In addition, when max level of srgb texture is less than full
mipmap level, the max level isn't generated.
BUG=712096
TEST=conformance2/textures/misc/tex-srgb-mipmap.html
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;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
Review-Url: https://codereview.chromium.org/2866663002
Cr-Commit-Position: refs/heads/master@{#471676}
Committed: https://chromium.googlesource.com/chromium/src/+/2e20e662edabca0927a21000850778eef038a7cf
Patch Set 1 #
Total comments: 4
Patch Set 2 : Address to yunchao's feedback #
Total comments: 2
Patch Set 3 : Address to Zhenyao's feedback #
Total comments: 2
Patch Set 4 : Address to Zhenyao's feedback #
Total comments: 1
Patch Set 5 : Address to Zhenyao's feedback #Patch Set 6 : Modify expectation file #
Messages
Total messages: 79 (66 generated)
Description was changed from ========== Follow up immutable tex srgb emulation issue of generateMipmap BUG=712096 ========== to ========== Follow up immutable tex srgb emulation issue of generateMipmap BUG=712096 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;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 checked by yizhou.jiang@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.
Description was changed from ========== Follow up immutable tex srgb emulation issue of generateMipmap BUG=712096 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;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 ========== Follow up immutable tex srgb emulation issue of generateMipmap BUG=712096 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;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 ==========
yizhou.jiang@intel.com changed reviewers: + jiajia.qin@intel.com, yunchao.he@intel.com
On 2017/05/05 07:47:52, yizhou.jiang wrote: > mailto:yizhou.jiang@intel.com changed reviewers: > + mailto:jiajia.qin@intel.com, mailto:yunchao.he@intel.com Yunchao and Jiajia, PTAL.
Some comments. Please add Jie, who will responsible WebGL 2.0.1 bug fixes. https://codereview.chromium.org/2866663002/diff/1/gpu/command_buffer/service/... File gpu/command_buffer/service/gles2_cmd_srgb_converter.cc (right): https://codereview.chromium.org/2866663002/diff/1/gpu/command_buffer/service/... gpu/command_buffer/service/gles2_cmd_srgb_converter.cc:427: (base_level + mipmap_levels - 1) > max_level Please check that max_level, not max_level - 1? https://codereview.chromium.org/2866663002/diff/1/gpu/command_buffer/service/... gpu/command_buffer/service/gles2_cmd_srgb_converter.cc:477: for (GLint level = base_level + 1; level <= max_mipmap_available_levels; I think it is <=, instead of <.
The CQ bit was checked by yizhou.jiang@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.
yizhou.jiang@intel.com changed reviewers: + yang.gu@intel.com
Thanks for your review yunchao and jiajia, please take another look. The CTS patch is pulling request(https://github.com/KhronosGroup/WebGL/pull/2395). https://codereview.chromium.org/2866663002/diff/1/gpu/command_buffer/service/... File gpu/command_buffer/service/gles2_cmd_srgb_converter.cc (right): https://codereview.chromium.org/2866663002/diff/1/gpu/command_buffer/service/... gpu/command_buffer/service/gles2_cmd_srgb_converter.cc:427: (base_level + mipmap_levels - 1) > max_level On 2017/05/05 08:01:48, yunchao wrote: > Please check that max_level, not max_level - 1? The variable name is quite confusing, so I change variable name max_mipmap_available_levels to max_mipmap_available_level. https://codereview.chromium.org/2866663002/diff/1/gpu/command_buffer/service/... gpu/command_buffer/service/gles2_cmd_srgb_converter.cc:477: for (GLint level = base_level + 1; level <= max_mipmap_available_levels; On 2017/05/05 08:01:48, yunchao wrote: > I think it is <=, instead of <. Done.
On 2017/05/08 08:28:56, yizhou.jiang wrote: > Thanks for your review yunchao and jiajia, please take another look. The CTS > patch is pulling request(https://github.com/KhronosGroup/WebGL/pull/2395). > > https://codereview.chromium.org/2866663002/diff/1/gpu/command_buffer/service/... > File gpu/command_buffer/service/gles2_cmd_srgb_converter.cc (right): > > https://codereview.chromium.org/2866663002/diff/1/gpu/command_buffer/service/... > gpu/command_buffer/service/gles2_cmd_srgb_converter.cc:427: (base_level + > mipmap_levels - 1) > max_level > On 2017/05/05 08:01:48, yunchao wrote: > > Please check that max_level, not max_level - 1? > > The variable name is quite confusing, so I change variable name > max_mipmap_available_levels to max_mipmap_available_level. > > https://codereview.chromium.org/2866663002/diff/1/gpu/command_buffer/service/... > gpu/command_buffer/service/gles2_cmd_srgb_converter.cc:477: for (GLint level = > base_level + 1; level <= max_mipmap_available_levels; > On 2017/05/05 08:01:48, yunchao wrote: > > I think it is <=, instead of <. > > Done. LGTM. Feel free to add Zhenyao and Ken to take a review.
Description was changed from ========== Follow up immutable tex srgb emulation issue of generateMipmap BUG=712096 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;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 ========== Follow up immutable tex srgb emulation issue of generateMipmap. When max level of srgb texture is less than full mipmap level, the max level isn't generated. BUG=712096 TEST=conformance2/textures/misc/tex-srgb-mipmap.html CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;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 ==========
yizhou.jiang@intel.com changed reviewers: + kbr@chromium.org, zmo@chromium.org
On 2017/05/08 08:49:32, yunchao wrote: > On 2017/05/08 08:28:56, yizhou.jiang wrote: > > Thanks for your review yunchao and jiajia, please take another look. The CTS > > patch is pulling request(https://github.com/KhronosGroup/WebGL/pull/2395). > > > > > https://codereview.chromium.org/2866663002/diff/1/gpu/command_buffer/service/... > > File gpu/command_buffer/service/gles2_cmd_srgb_converter.cc (right): > > > > > https://codereview.chromium.org/2866663002/diff/1/gpu/command_buffer/service/... > > gpu/command_buffer/service/gles2_cmd_srgb_converter.cc:427: (base_level + > > mipmap_levels - 1) > max_level > > On 2017/05/05 08:01:48, yunchao wrote: > > > Please check that max_level, not max_level - 1? > > > > The variable name is quite confusing, so I change variable name > > max_mipmap_available_levels to max_mipmap_available_level. > > > > > https://codereview.chromium.org/2866663002/diff/1/gpu/command_buffer/service/... > > gpu/command_buffer/service/gles2_cmd_srgb_converter.cc:477: for (GLint level = > > base_level + 1; level <= max_mipmap_available_levels; > > On 2017/05/05 08:01:48, yunchao wrote: > > > I think it is <=, instead of <. > > > > Done. > > LGTM. Feel free to add Zhenyao and Ken to take a review. @zmo @ken, the patch follow up immutable tex srgb emulation issue of generateMipmap. When max level of srgb texture is less than full mipmap level, the max level isn't generated. The CTS test is pulling request(https://github.com/KhronosGroup/WebGL/pull/2395). PTAL. Thanks!
https://codereview.chromium.org/2866663002/diff/20001/gpu/command_buffer/serv... File gpu/command_buffer/service/gles2_cmd_srgb_converter.cc (right): https://codereview.chromium.org/2866663002/diff/20001/gpu/command_buffer/serv... gpu/command_buffer/service/gles2_cmd_srgb_converter.cc:427: (base_level + mipmap_levels - 1) > max_level As I mentioned in the other CL, we need to use base::CheckedNumeric for this. base_level can be set by client to any value, so there is an overflow danger here.
The CQ bit was checked by yizhou.jiang@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: chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
The CQ bit was checked by yizhou.jiang@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...
Patchset #4 (id:60001) has been deleted
Dry run: Try jobs failed on following builders: mac_optional_gpu_tests_rel on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_optional_gpu_...)
The CQ bit was checked by yizhou.jiang@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...
Patchset #3 (id:40001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by yizhou.jiang@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.
@Zhenyao Please take another look. Thank you. https://codereview.chromium.org/2866663002/diff/20001/gpu/command_buffer/serv... File gpu/command_buffer/service/gles2_cmd_srgb_converter.cc (right): https://codereview.chromium.org/2866663002/diff/20001/gpu/command_buffer/serv... gpu/command_buffer/service/gles2_cmd_srgb_converter.cc:427: (base_level + mipmap_levels - 1) > max_level On 2017/05/08 17:25:29, Zhenyao Mo wrote: > As I mentioned in the other CL, we need to use base::CheckedNumeric for this. > base_level can be set by client to any value, so there is an overflow danger > here. I see,base_level + mipmap_level might overflow here.
https://codereview.chromium.org/2866663002/diff/80001/gpu/command_buffer/serv... File gpu/command_buffer/service/gles2_cmd_srgb_converter.cc (right): https://codereview.chromium.org/2866663002/diff/80001/gpu/command_buffer/serv... gpu/command_buffer/service/gles2_cmd_srgb_converter.cc:481: for (GLint level = base_level + 1; level <= max_mipmap_available_level; Sorry I failed to mention this earlier, but base_level + 1 and level ++ can also overflow.
The CQ bit was checked by yizhou.jiang@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...
yizhou.jiang@intel.com changed reviewers: + jie.chen@intel.com
yizhou.jiang@intel.com changed reviewers: + jie.a.chen@intel.com - jie.chen@intel.com
Description was changed from ========== Follow up immutable tex srgb emulation issue of generateMipmap. When max level of srgb texture is less than full mipmap level, the max level isn't generated. BUG=712096 TEST=conformance2/textures/misc/tex-srgb-mipmap.html CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;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 ========== Follow up immutable tex srgb emulation issue of generateMipmap. when calculating max available mipmap level, as base level can be set by client to any value, so there is an overflow danger. In addition, when max level of srgb texture is less than full mipmap level, the max level isn't generated. BUG=712096 TEST=conformance2/textures/misc/tex-srgb-mipmap.html CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;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: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by yizhou.jiang@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.
@Zhenyao, please take another look.Thanks. https://codereview.chromium.org/2866663002/diff/80001/gpu/command_buffer/serv... File gpu/command_buffer/service/gles2_cmd_srgb_converter.cc (right): https://codereview.chromium.org/2866663002/diff/80001/gpu/command_buffer/serv... gpu/command_buffer/service/gles2_cmd_srgb_converter.cc:481: for (GLint level = base_level + 1; level <= max_mipmap_available_level; On 2017/05/09 17:14:33, Zhenyao Mo wrote: > Sorry I failed to mention this earlier, but base_level + 1 and level ++ can also > overflow. Done.
https://codereview.chromium.org/2866663002/diff/100001/gpu/command_buffer/ser... File gpu/command_buffer/service/gles2_cmd_srgb_converter.cc (right): https://codereview.chromium.org/2866663002/diff/100001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_srgb_converter.cc:481: for (base::CheckedNumeric<GLint> level = base_level + 1; I am not sure if this can catch the overflow. My understanding is base_level + 1 is computed before assigning to level, so if overflow happens, you simply assign -1 to level, and level.IsValid() is still true even overflow has happened.
The CQ bit was checked by yizhou.jiang@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: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
The CQ bit was checked by yizhou.jiang@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: mac_optional_gpu_tests_rel on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_optional_gpu_...)
The CQ bit was checked by yizhou.jiang@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: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by yizhou.jiang@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.
lgtm
The CQ bit was checked by yizhou.jiang@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 yizhou.jiang@intel.com
The patchset sent to the CQ was uploaded after l-g-t-m from yunchao.he@intel.com, zmo@chromium.org Link to the patchset: https://codereview.chromium.org/2866663002/#ps140001 (title: "Modify expectation file")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch. Bot data: {"patchset_id": 140001, "attempt_start_ts": 1494826471892440, "parent_rev": "85a9925c6310cf115ea694e08d1ad332bfc1e09a", "commit_rev": "2e20e662edabca0927a21000850778eef038a7cf"}
Message was sent while issue was closed.
Description was changed from ========== Follow up immutable tex srgb emulation issue of generateMipmap. when calculating max available mipmap level, as base level can be set by client to any value, so there is an overflow danger. In addition, when max level of srgb texture is less than full mipmap level, the max level isn't generated. BUG=712096 TEST=conformance2/textures/misc/tex-srgb-mipmap.html CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;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 ========== Follow up immutable tex srgb emulation issue of generateMipmap. when calculating max available mipmap level, as base level can be set by client to any value, so there is an overflow danger. In addition, when max level of srgb texture is less than full mipmap level, the max level isn't generated. BUG=712096 TEST=conformance2/textures/misc/tex-srgb-mipmap.html CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;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 Review-Url: https://codereview.chromium.org/2866663002 Cr-Commit-Position: refs/heads/master@{#471676} Committed: https://chromium.googlesource.com/chromium/src/+/2e20e662edabca0927a210008507... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:140001) as https://chromium.googlesource.com/chromium/src/+/2e20e662edabca0927a210008507... |