Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1412883002:
Fix WebGL 2 texture renderability check. (Closed)
|
Created:
5 years, 2 months ago by Zhenyao Mo Modified:
5 years, 1 month ago CC:
chromium-reviews, darin-cc_chromium.org, jam, piman+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionFix WebGL 2 texture renderability check.
It should check from base_level and up, not 0 and up.
BUG=429053
TEST=conformance2/textures/misc/tex-mipmap-levels.html, gpu_unittests
R=bajones@chromium.org,kbr@chromium.org
Committed: https://crrev.com/493f20d34a57ae22da78f798235881ff39465ae0
Cr-Commit-Position: refs/heads/master@{#356475}
Patch Set 1 #Patch Set 2 : #Patch Set 3 : #Patch Set 4 : #Patch Set 5 : #Patch Set 6 : #
Total comments: 5
Patch Set 7 : working #
Total comments: 10
Patch Set 8 : fix asan crash #
Messages
Total messages: 42 (12 generated)
zmo@chromium.org changed reviewers: + sievers@chromium.org
Another one. PTAL.
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/1412883002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1412883002/1
LGTM
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
not ready for review yet. Decided to update more places with base_level thing.
On 2015/10/17 01:27:30, Zhenyao Mo wrote: > not ready for review yet. Decided to update more places with base_level thing. I wrote a CL to update base level support in command buffer at here: https://codereview.chromium.org/1409513002/. You may take a look. But it missed some tests now.
lgtm. It's fine if you'd like to add support for this incrementally. Maybe better than one large and risky patch that has untested code paths.
zmo@chromium.org changed reviewers: + piman@chromium.org
OK, please take a look again. I audited texture_manager.cc and think I covered all the places that are affected by base_level and max_level. @piman: can you also take a look.
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/1412883002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1412883002/100001
@qiankun, sorry our CLs got collided. Please also review my CL if you have time.
lgtm https://codereview.chromium.org/1412883002/diff/100001/gpu/command_buffer/ser... File gpu/command_buffer/service/texture_manager.cc (right): https://codereview.chromium.org/1412883002/diff/100001/gpu/command_buffer/ser... gpu/command_buffer/service/texture_manager.cc:803: face_info.num_mip_levels = std::min( nit: 2 spaces before std::min (should be 1). https://codereview.chromium.org/1412883002/diff/100001/gpu/command_buffer/ser... File gpu/command_buffer/service/texture_manager.h (right): https://codereview.chromium.org/1412883002/diff/100001/gpu/command_buffer/ser... gpu/command_buffer/service/texture_manager.h:252: GLsizei num_mip_levels; nit: can you add a comment indicating that this is relative to base_level? To avoid confusion with level_infos (which contains slots for all levels starting at 0).
LGTM again. Great work making this change complete. https://codereview.chromium.org/1412883002/diff/100001/content/test/gpu/gpu_t... File content/test/gpu/gpu_tests/webgl2_conformance_expectations.py (left): https://codereview.chromium.org/1412883002/diff/100001/content/test/gpu/gpu_t... content/test/gpu/gpu_tests/webgl2_conformance_expectations.py:96: self.Fail('conformance2/textures/misc/tex-mipmap-levels.html', bug=483282) Awesome work that this test is now passing.
LGTM https://codereview.chromium.org/1412883002/diff/100001/gpu/command_buffer/ser... File gpu/command_buffer/service/texture_manager.cc (right): https://codereview.chromium.org/1412883002/diff/100001/gpu/command_buffer/ser... gpu/command_buffer/service/texture_manager.cc:803: face_info.num_mip_levels = std::min( On 2015/10/22 01:18:36, piman (slow to review) wrote: > nit: 2 spaces before std::min (should be 1). Done. https://codereview.chromium.org/1412883002/diff/100001/gpu/command_buffer/ser... File gpu/command_buffer/service/texture_manager.h (right): https://codereview.chromium.org/1412883002/diff/100001/gpu/command_buffer/ser... gpu/command_buffer/service/texture_manager.h:252: GLsizei num_mip_levels; On 2015/10/22 01:18:36, piman (slow to review) wrote: > nit: can you add a comment indicating that this is relative to base_level? To > avoid confusion with level_infos (which contains slots for all levels starting > at 0). Done.
The CQ bit was checked by zmo@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bajones@chromium.org, kbr@chromium.org, piman@chromium.org Link to the patchset: https://codereview.chromium.org/1412883002/#ps120001 (title: "working")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1412883002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1412883002/120001
No idea why I put LGTM to my own CL
The CQ bit was unchecked by commit-bot@chromium.org
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_...)
qiankun.miao@intel.com changed reviewers: + qiankun.miao@intel.com
https://codereview.chromium.org/1412883002/diff/120001/gpu/command_buffer/ser... File gpu/command_buffer/service/texture_manager.cc (right): https://codereview.chromium.org/1412883002/diff/120001/gpu/command_buffer/ser... gpu/command_buffer/service/texture_manager.cc:483: DCHECK_GE(level, 0); We should check for level >= base_level_ at here and other places below. https://codereview.chromium.org/1412883002/diff/120001/gpu/command_buffer/ser... gpu/command_buffer/service/texture_manager.cc:904: if (level >= 0 && face_index < face_infos_.size() && Should be level >= base_level_? https://codereview.chromium.org/1412883002/diff/120001/gpu/command_buffer/ser... gpu/command_buffer/service/texture_manager.cc:929: if (level >= 0 && face_index < face_infos_.size() && Same here. https://codereview.chromium.org/1412883002/diff/120001/gpu/command_buffer/ser... gpu/command_buffer/service/texture_manager.cc:948: if (level >= 0 && face_index < face_infos_.size() && Same here. https://codereview.chromium.org/1412883002/diff/120001/gpu/command_buffer/ser... gpu/command_buffer/service/texture_manager.cc:1317: if (level >= 0 && face_index < face_infos_.size() && Same here.
Description was changed from ========== Fix WebGL 2 texture renderability check. It should check from base_level and up, not 0 and up. BUG=429053 TEST=conformance2/textures/misc/tex-mipmap-levels.html, gpu_unittests R=bajones@chromium.org,kbr@chromium.org ========== to ========== Fix WebGL 2 texture renderability check. It should check from base_level and up, not 0 and up. BUG=429053 TEST=conformance2/textures/misc/tex-mipmap-levels.html, gpu_unittests R=bajones@chromium.org,kbr@chromium.org ==========
On 2015/10/22 02:51:27, commit-bot: I haz the power wrote: > 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_...) dyen: could you please look at this failure of trace_test.Canvas2DRedBox on the Windows ATI tryserver? Is this a known problem?
On 2015/10/22 17:49:03, Ken Russell wrote: > On 2015/10/22 02:51:27, commit-bot: I haz the power wrote: > > 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_...) > > dyen: could you please look at this failure of trace_test.Canvas2DRedBox on the > Windows ATI tryserver? Is this a known problem? test name seemed to have changed, trybots are fixed now: https://codereview.chromium.org/1424433002/ feel free to try again
https://codereview.chromium.org/1412883002/diff/120001/gpu/command_buffer/ser... File gpu/command_buffer/service/texture_manager.cc (right): https://codereview.chromium.org/1412883002/diff/120001/gpu/command_buffer/ser... gpu/command_buffer/service/texture_manager.cc:483: DCHECK_GE(level, 0); On 2015/10/22 06:06:08, qiankun wrote: > We should check for level >= base_level_ at here and other places below. Not really. You can specify a tex level for framebuffer that's below base_level_ if base_level_ > 0. https://codereview.chromium.org/1412883002/diff/120001/gpu/command_buffer/ser... gpu/command_buffer/service/texture_manager.cc:929: if (level >= 0 && face_index < face_infos_.size() && On 2015/10/22 06:06:08, qiankun wrote: > Same here. Same here, you can specify texture at any level. It just won't affect rendering with mipmapping. https://codereview.chromium.org/1412883002/diff/120001/gpu/command_buffer/ser... gpu/command_buffer/service/texture_manager.cc:948: if (level >= 0 && face_index < face_infos_.size() && On 2015/10/22 06:06:08, qiankun wrote: > Same here. Same answer https://codereview.chromium.org/1412883002/diff/120001/gpu/command_buffer/ser... gpu/command_buffer/service/texture_manager.cc:1317: if (level >= 0 && face_index < face_infos_.size() && On 2015/10/22 06:06:08, qiankun wrote: > Same here. I think except for mipmaping/texture-completeness/rendering, texture level is not restricted by base_level.
lgtm https://codereview.chromium.org/1412883002/diff/120001/gpu/command_buffer/ser... File gpu/command_buffer/service/texture_manager.cc (right): https://codereview.chromium.org/1412883002/diff/120001/gpu/command_buffer/ser... gpu/command_buffer/service/texture_manager.cc:1317: if (level >= 0 && face_index < face_infos_.size() && On 2015/10/26 21:05:24, Zhenyao Mo wrote: > On 2015/10/22 06:06:08, qiankun wrote: > > Same here. > > I think except for mipmaping/texture-completeness/rendering, texture level is > not restricted by base_level. Thanks for explanation! That's right. BTW, when I read ES spec I found in some APIs such as TexImage*/TexSubImage*/CopyTexImage* parameter "level" specified the level-of-detail number. I am a little confused. Do you know the clear mean of "level-of-detail number"?
On 2015/10/27 06:32:50, qiankun wrote: > lgtm > > https://codereview.chromium.org/1412883002/diff/120001/gpu/command_buffer/ser... > File gpu/command_buffer/service/texture_manager.cc (right): > > https://codereview.chromium.org/1412883002/diff/120001/gpu/command_buffer/ser... > gpu/command_buffer/service/texture_manager.cc:1317: if (level >= 0 && face_index > < face_infos_.size() && > On 2015/10/26 21:05:24, Zhenyao Mo wrote: > > On 2015/10/22 06:06:08, qiankun wrote: > > > Same here. > > > > I think except for mipmaping/texture-completeness/rendering, texture level is > > not restricted by base_level. > > Thanks for explanation! That's right. > > BTW, when I read ES spec I found in some APIs such as > TexImage*/TexSubImage*/CopyTexImage* parameter "level" specified the > level-of-detail number. I am a little confused. Do you know the clear mean of > "level-of-detail number"? It's related with mipmapping. So the base level (the smallest LOD number) has the maximum texture size, and each level on top of that has half the lower level size.
On 2015/10/27 22:48:12, Zhenyao Mo wrote: > On 2015/10/27 06:32:50, qiankun wrote: > > lgtm > > > > > https://codereview.chromium.org/1412883002/diff/120001/gpu/command_buffer/ser... > > File gpu/command_buffer/service/texture_manager.cc (right): > > > > > https://codereview.chromium.org/1412883002/diff/120001/gpu/command_buffer/ser... > > gpu/command_buffer/service/texture_manager.cc:1317: if (level >= 0 && > face_index > > < face_infos_.size() && > > On 2015/10/26 21:05:24, Zhenyao Mo wrote: > > > > BTW, when I read ES spec I found in some APIs such as > > TexImage*/TexSubImage*/CopyTexImage* parameter "level" specified the > > level-of-detail number. I am a little confused. Do you know the clear mean of > > "level-of-detail number"? > > It's related with mipmapping. So the base level (the smallest LOD number) has > the maximum texture size, and each level on top of that has half the lower level > size. In ES3 manual page, for TexImage3D level param specifies the level-of-detail number. Level 0 is the base image level. Level n is the nth mipmap reduction image. Does this mean we should do translation for level: 0 -> base_level, level -> base_level + level?
On 2015/10/27 23:08:01, qiankun wrote: > On 2015/10/27 22:48:12, Zhenyao Mo wrote: > > On 2015/10/27 06:32:50, qiankun wrote: > > > lgtm > > > > > > > > > https://codereview.chromium.org/1412883002/diff/120001/gpu/command_buffer/ser... > > > File gpu/command_buffer/service/texture_manager.cc (right): > > > > > > > > > https://codereview.chromium.org/1412883002/diff/120001/gpu/command_buffer/ser... > > > gpu/command_buffer/service/texture_manager.cc:1317: if (level >= 0 && > > face_index > > > < face_infos_.size() && > > > On 2015/10/26 21:05:24, Zhenyao Mo wrote: > > > > > > > BTW, when I read ES spec I found in some APIs such as > > > TexImage*/TexSubImage*/CopyTexImage* parameter "level" specified the > > > level-of-detail number. I am a little confused. Do you know the clear mean > of > > > "level-of-detail number"? > > > > It's related with mipmapping. So the base level (the smallest LOD number) has > > the maximum texture size, and each level on top of that has half the lower > level > > size. > > In ES3 manual page, for TexImage3D level param specifies the level-of-detail > number. Level 0 is the base image level. Level n is the nth mipmap reduction > image. Does this mean we should do translation for level: 0 -> base_level, level > -> base_level + level? Not to speak for Mo, but I don't think so. The texture specification functions deal with absolute levels of detail. Later, the TEXTURE_BASE_LEVEL and TEXTURE_MAX_LEVEL can be specified to work with only a limited range of the texture's mipmap levels.
On 2015/10/27 23:22:11, Ken Russell wrote: > On 2015/10/27 23:08:01, qiankun wrote: > > On 2015/10/27 22:48:12, Zhenyao Mo wrote: > > > On 2015/10/27 06:32:50, qiankun wrote: > > > > lgtm > > > > > > > > > > > > > > https://codereview.chromium.org/1412883002/diff/120001/gpu/command_buffer/ser... > > > > File gpu/command_buffer/service/texture_manager.cc (right): > > > > > > > > > > > > > > https://codereview.chromium.org/1412883002/diff/120001/gpu/command_buffer/ser... > > > > gpu/command_buffer/service/texture_manager.cc:1317: if (level >= 0 && > > > face_index > > > > < face_infos_.size() && > > > > On 2015/10/26 21:05:24, Zhenyao Mo wrote: > > > > > > > > > > BTW, when I read ES spec I found in some APIs such as > > > > TexImage*/TexSubImage*/CopyTexImage* parameter "level" specified the > > > > level-of-detail number. I am a little confused. Do you know the clear mean > > of > > > > "level-of-detail number"? > > > > > > It's related with mipmapping. So the base level (the smallest LOD number) > has > > > the maximum texture size, and each level on top of that has half the lower > > level > > > size. > > > > In ES3 manual page, for TexImage3D level param specifies the level-of-detail > > number. Level 0 is the base image level. Level n is the nth mipmap reduction > > image. Does this mean we should do translation for level: 0 -> base_level, > level > > -> base_level + level? > > Not to speak for Mo, but I don't think so. The texture specification functions > deal with absolute levels of detail. Later, the TEXTURE_BASE_LEVEL and > TEXTURE_MAX_LEVEL can be specified to work with only a limited range of the > texture's mipmap levels. I agree with Ken.
The CQ bit was checked by zmo@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from kbr@chromium.org, bajones@chromium.org, qiankun.miao@intel.com, piman@chromium.org Link to the patchset: https://codereview.chromium.org/1412883002/#ps140001 (title: "fix asan crash")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1412883002/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1412883002/140001
On 2015/10/27 23:36:04, zmo wrote: > On 2015/10/27 23:22:11, Ken Russell wrote: > > On 2015/10/27 23:08:01, qiankun wrote: > > > On 2015/10/27 22:48:12, Zhenyao Mo wrote: > > > > On 2015/10/27 06:32:50, qiankun wrote: > > > > > lgtm > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/1412883002/diff/120001/gpu/command_buffer/ser... > > > > > File gpu/command_buffer/service/texture_manager.cc (right): > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/1412883002/diff/120001/gpu/command_buffer/ser... > > > > > gpu/command_buffer/service/texture_manager.cc:1317: if (level >= 0 && > > > > face_index > > > > > < face_infos_.size() && > > > > > On 2015/10/26 21:05:24, Zhenyao Mo wrote: > > > > > > > > > > > > > BTW, when I read ES spec I found in some APIs such as > > > > > TexImage*/TexSubImage*/CopyTexImage* parameter "level" specified the > > > > > level-of-detail number. I am a little confused. Do you know the clear > mean > > > of > > > > > "level-of-detail number"? > > > > > > > > It's related with mipmapping. So the base level (the smallest LOD number) > > has > > > > the maximum texture size, and each level on top of that has half the lower > > > level > > > > size. > > > > > > In ES3 manual page, for TexImage3D level param specifies the level-of-detail > > > number. Level 0 is the base image level. Level n is the nth mipmap reduction > > > image. Does this mean we should do translation for level: 0 -> base_level, > > level > > > -> base_level + level? > > > > Not to speak for Mo, but I don't think so. The texture specification functions > > deal with absolute levels of detail. Later, the TEXTURE_BASE_LEVEL and > > TEXTURE_MAX_LEVEL can be specified to work with only a limited range of the > > texture's mipmap levels. > > I agree with Ken. I see. Thank you, Ken and Mo.
Message was sent while issue was closed.
Committed patchset #8 (id:140001)
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/493f20d34a57ae22da78f798235881ff39465ae0 Cr-Commit-Position: refs/heads/master@{#356475}
Message was sent while issue was closed.
A revert of this CL (patchset #8 id:140001) has been created in https://codereview.chromium.org/1425943002/ by jmadill@chromium.org. The reason for reverting is: Failing on several configs, possibly has a bug: On Windows, triggers a bug in ANGLE, should have a skip suppression: http://build.chromium.org/p/chromium.gpu.fyi/builders/Win7%20Debug%20%28NVIDI... (assert in Renderer11.cpp in Debug) On Mac, fails for unknown reasons: http://build.chromium.org/p/chromium.gpu.fyi/builders/Mac%2010.10%20Release%2... http://build.chromium.org/p/chromium.gpu.fyi/builders/Mac%2010.9%20Debug%20%2... On Linux AMD, fails as well: http://build.chromium.org/p/chromium.gpu.fyi/builders/Linux%20Release%20%28AT... . |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
