Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2841503002:
Fix immutable tex srgb emulation issue of generateMipmap (Closed)
|
Created:
3 years, 8 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. |
DescriptionFix immutable tex srgb emulation issue of generateMipmap
During the emulation of srgb for generateMipmap, we tried to generate all
levels of the original texture. But if the texture is immutable texture,
we can't generate mipmaps exceed the original range.
Also, when the width and height are not equal, they should be at least one
during the emulation.
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
Review-Url: https://codereview.chromium.org/2841503002
Cr-Commit-Position: refs/heads/master@{#468238}
Committed: https://chromium.googlesource.com/chromium/src/+/a916ec8af80b660d45454ccdf5b88d97249327a8
Patch Set 1 : Fix immutable tex srgb emulation issue of generateMipmap #
Total comments: 2
Patch Set 2 : check texture maxLevel #Patch Set 3 : Address to qiankun's feedback #Patch Set 4 : Address to qiankun's feedback #Patch Set 5 : Modify expectation file #
Total comments: 1
Messages
Total messages: 54 (40 generated)
Description was changed from ========== Fix immutable tex srgb emulation issue of generateMipmap During the emulation of srgb for generateMipmap, we tried to generate all mipmaps of the original texture. But if the texture is immutable texture, we can't generate mipmaps bigger than the original levels. Also, the width or height is divided by 2 in every iteration. But they should be at least one if width and height are not equal. BUG=712096 ========== to ========== Fix immutable tex srgb emulation issue of generateMipmap During the emulation of srgb for generateMipmap, we tried to generate all mipmaps of the original texture. But if the texture is immutable texture, we can't generate mipmaps bigger than the original levels. Also, the width or height is divided by 2 in every iteration. But they should be at least one if width and height are not equal. 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: 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_...)
Patchset #1 (id:1) has been deleted
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 ========== Fix immutable tex srgb emulation issue of generateMipmap During the emulation of srgb for generateMipmap, we tried to generate all mipmaps of the original texture. But if the texture is immutable texture, we can't generate mipmaps bigger than the original levels. Also, the width or height is divided by 2 in every iteration. But they should be at least one if width and height are not equal. 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 ========== Fix immutable tex srgb emulation issue of generateMipmap During the emulation of srgb for generateMipmap, we tried to generate all mipmaps of the original texture. But if the texture is immutable texture, we can't generate mipmaps bigger than the original levels. Also, the width or height is divided by 2 in every iteration. But they should be at least one if width and height are not equal. 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: + qiankun.miao@intel.com, yunchao.he@intel.com
On 2017/04/24 09:55:00, yizhou.jiang wrote: > mailto:yizhou.jiang@intel.com changed reviewers: > + mailto:qiankun.miao@intel.com, mailto:yunchao.he@intel.com Yunchao and Qiankun, this patch fix immutable texture srgb emulation of generateMipmap. A related CTS test is under review. Please take a look.
https://codereview.chromium.org/2841503002/diff/20001/gpu/command_buffer/serv... File gpu/command_buffer/service/gles2_cmd_srgb_converter.cc (right): https://codereview.chromium.org/2841503002/diff/20001/gpu/command_buffer/serv... gpu/command_buffer/service/gles2_cmd_srgb_converter.cc:472: for (GLint level = base_level + 1; level < base_level + mipmap_levels; When base_level + mipmap_levels is larger than tex->max_level(), this isn't correct.
Description was changed from ========== Fix immutable tex srgb emulation issue of generateMipmap During the emulation of srgb for generateMipmap, we tried to generate all mipmaps of the original texture. But if the texture is immutable texture, we can't generate mipmaps bigger than the original levels. Also, the width or height is divided by 2 in every iteration. But they should be at least one if width and height are not equal. 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 ========== Fix immutable tex srgb emulation issue of generateMipmap During the emulation of srgb for generateMipmap, we tried to generate all levels of the original texture. But if the texture is immutable texture, we can't generate mipmaps exceed the original range. Also, when the width and height are not equal, they should be at least one during the emulation. 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 ==========
Patchset #2 (id:40001) has been deleted
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: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...) linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Patchset #2 (id:60001) has been deleted
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.
https://codereview.chromium.org/2841503002/diff/20001/gpu/command_buffer/serv... File gpu/command_buffer/service/gles2_cmd_srgb_converter.cc (right): https://codereview.chromium.org/2841503002/diff/20001/gpu/command_buffer/serv... gpu/command_buffer/service/gles2_cmd_srgb_converter.cc:472: for (GLint level = base_level + 1; level < base_level + mipmap_levels; On 2017/04/24 11:37:58, qiankun wrote: > When base_level + mipmap_levels is larger than tex->max_level(), this isn't > correct. Done.
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 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.
qiankun.miao@intel.com changed reviewers: + kbr@chromium.org, zmo@chromium.org
PTAL. Conformance test landed at: https://github.com/KhronosGroup/WebGL/pull/2382
Very nice. LGTM
On 2017/04/27 02:49:38, Ken Russell wrote: > Very nice. LGTM I upload a webgl roll-in patch(https://codereview.chromium.org/2841043002/). PTAL. Thanks!
On 2017/04/27 02:49:38, Ken Russell wrote: > Very nice. LGTM I upload a webgl roll-in patch(https://codereview.chromium.org/2841043002/). PTAL. Thanks!
On 2017/04/27 09:57:32, yizhou.jiang wrote: > On 2017/04/27 02:49:38, Ken Russell wrote: > > Very nice. LGTM > > I upload a webgl roll-in patch(https://codereview.chromium.org/2841043002/). > PTAL. Thanks! Your new WebGL conformance roll is in the CQ. Could you please update this CL once it lands and remove the suppression for the tex-srgb-mipmap test? Thanks. Feel free to CQ this afterward yourself -- it's already approved. Thanks.
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.
On 2017/04/28 18:39:06, Ken Russell wrote: > On 2017/04/27 09:57:32, yizhou.jiang wrote: > > On 2017/04/27 02:49:38, Ken Russell wrote: > > > Very nice. LGTM > > > > I upload a webgl roll-in patch(https://codereview.chromium.org/2841043002/). > > PTAL. Thanks! > > Your new WebGL conformance roll is in the CQ. Could you please update this CL > once it lands and remove the suppression for the tex-srgb-mipmap test? Thanks. > > Feel free to CQ this afterward yourself -- it's already approved. Thanks. Thanks a lot!
The CQ bit was checked by yizhou.jiang@intel.com
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/2841503002/#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": 1493464031259800,
"parent_rev": "31719eefcc4e06a6cb0b41364eb91d0857c2ce2f", "commit_rev":
"a916ec8af80b660d45454ccdf5b88d97249327a8"}
Message was sent while issue was closed.
Description was changed from ========== Fix immutable tex srgb emulation issue of generateMipmap During the emulation of srgb for generateMipmap, we tried to generate all levels of the original texture. But if the texture is immutable texture, we can't generate mipmaps exceed the original range. Also, when the width and height are not equal, they should be at least one during the emulation. 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 ========== Fix immutable tex srgb emulation issue of generateMipmap During the emulation of srgb for generateMipmap, we tried to generate all levels of the original texture. But if the texture is immutable texture, we can't generate mipmaps exceed the original range. Also, when the width and height are not equal, they should be at least one during the emulation. 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 Review-Url: https://codereview.chromium.org/2841503002 Cr-Commit-Position: refs/heads/master@{#468238} Committed: https://chromium.googlesource.com/chromium/src/+/a916ec8af80b660d45454ccdf5b8... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:140001) as https://chromium.googlesource.com/chromium/src/+/a916ec8af80b660d45454ccdf5b8...
Message was sent while issue was closed.
https://codereview.chromium.org/2841503002/diff/140001/gpu/command_buffer/ser... File gpu/command_buffer/service/gles2_cmd_srgb_converter.cc (right): https://codereview.chromium.org/2841503002/diff/140001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_srgb_converter.cc:427: (base_level + mipmap_levels) > max_level ? max_level There is an overflow issue here with base_level + mipmap_levels here.
Message was sent while issue was closed.
On 2017/05/04 16:55:44, Zhenyao Mo wrote: > https://codereview.chromium.org/2841503002/diff/140001/gpu/command_buffer/ser... > File gpu/command_buffer/service/gles2_cmd_srgb_converter.cc (right): > > https://codereview.chromium.org/2841503002/diff/140001/gpu/command_buffer/ser... > gpu/command_buffer/service/gles2_cmd_srgb_converter.cc:427: (base_level + > mipmap_levels) > max_level ? max_level > There is an overflow issue here with base_level + mipmap_levels here. Thank you Zhenyao. In line 476, the comparison is '<',so here isn't a overflow. However, If max level of a srgb texture is less than full mipmap level, the max level mipmap isn't generated. I have uploaded a cts test(https://github.com/KhronosGroup/WebGL/pull/2395) and a chromium patch(https://codereview.chromium.org/2866663002/#) to follow up this issue.
Message was sent while issue was closed.
On 2017/05/08 08:24:45, yizhou.jiang wrote: > On 2017/05/04 16:55:44, Zhenyao Mo wrote: > > > https://codereview.chromium.org/2841503002/diff/140001/gpu/command_buffer/ser... > > File gpu/command_buffer/service/gles2_cmd_srgb_converter.cc (right): > > > > > https://codereview.chromium.org/2841503002/diff/140001/gpu/command_buffer/ser... > > gpu/command_buffer/service/gles2_cmd_srgb_converter.cc:427: (base_level + > > mipmap_levels) > max_level ? max_level > > There is an overflow issue here with base_level + mipmap_levels here. > > Thank you Zhenyao. In line 476, the comparison is '<',so here isn't a overflow. > However, If max level of a srgb texture is less than full mipmap level, the > max level mipmap isn't generated. > I have uploaded a cts test(https://github.com/KhronosGroup/WebGL/pull/2395) > and a chromium patch(https://codereview.chromium.org/2866663002/#) to follow > up this issue. In WebGL2/ES3, base_level can be set by client to any non-negative integers. So base_level + 1 can definitely overflow. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
