Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1306343007:
Fix base level for generateMipmap in blink (Closed)
DescriptionFix base level for generateMipmap in blink
Don't do base level clamp for generateMipmap. Generate error message for
generateMipmap if base level texture is not specified.
BUG=295792
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=201975
Patch Set 1 #
Total comments: 6
Patch Set 2 : large base level #
Total comments: 7
Patch Set 3 : add assertion #
Messages
Total messages: 21 (3 generated)
qiankun.miao@intel.com changed reviewers: + bajones@chromium.org, kbr@chromium.org, zmo@chromium.org
PTAL.
https://codereview.chromium.org/1306343007/diff/1/Source/modules/webgl/WebGLT... File Source/modules/webgl/WebGLTexture.cpp (right): https://codereview.chromium.org/1306343007/diff/1/Source/modules/webgl/WebGLT... Source/modules/webgl/WebGLTexture.cpp:403: return; should m_isComplete be set to false? https://codereview.chromium.org/1306343007/diff/1/Source/modules/webgl/WebGLT... Source/modules/webgl/WebGLTexture.cpp:408: if (m_baseLevel > maxLevel) { As I mentioned in the other CL, my reading of the spec is maxLevel should be clamped to [baseLevel, max], not generate an error instead.
https://codereview.chromium.org/1306343007/diff/1/Source/modules/webgl/WebGLT... File Source/modules/webgl/WebGLTexture.cpp (right): https://codereview.chromium.org/1306343007/diff/1/Source/modules/webgl/WebGLT... Source/modules/webgl/WebGLTexture.cpp:403: return; On 2015/09/07 21:35:17, Zhenyao Mo wrote: > should m_isComplete be set to false? Done. https://codereview.chromium.org/1306343007/diff/1/Source/modules/webgl/WebGLT... Source/modules/webgl/WebGLTexture.cpp:408: if (m_baseLevel > maxLevel) { On 2015/09/07 21:35:18, Zhenyao Mo wrote: > As I mentioned in the other CL, my reading of the spec is maxLevel should be > clamped to [baseLevel, max], not generate an error instead. Just set m_isComplete as false here other than returning false. This is limitation of texture completeness.
https://codereview.chromium.org/1306343007/diff/1/Source/modules/webgl/WebGLT... File Source/modules/webgl/WebGLTexture.cpp (right): https://codereview.chromium.org/1306343007/diff/1/Source/modules/webgl/WebGLT... Source/modules/webgl/WebGLTexture.cpp:408: if (m_baseLevel > maxLevel) { So I read the ES spec 3.0.4 one more time, here are my understandings. Section 3.8.10 Texture Minification, under "Mipmapping", page 158, "For immutable-format textures, level base is clamped to the range [0, levels − 1], level max is then clamped to the range [level base , levels − 1], and p is one less than levels, where levels is the parameter passed to TexStorage* for the texture object (see section 3.8.4). Otherwise p = log 2 (maxsize) + level base , and all arrays from level base through q = min{p, level max } must be defined, as discussed in section 3.8.13." Section 3.8.13 Texture Completeness says one necessary condition for a texture to be complete is level base <= level max. It didn't mention anything about clamping for immutable textures. kbr, bajones: inputs?
LGTM https://codereview.chromium.org/1306343007/diff/1/Source/modules/webgl/WebGLT... File Source/modules/webgl/WebGLTexture.cpp (right): https://codereview.chromium.org/1306343007/diff/1/Source/modules/webgl/WebGLT... Source/modules/webgl/WebGLTexture.cpp:408: if (m_baseLevel > maxLevel) { On 2015/09/08 20:45:24, Zhenyao Mo wrote: > So I read the ES spec 3.0.4 one more time, here are my understandings. > > Section 3.8.10 Texture Minification, under "Mipmapping", page 158, "For > immutable-format textures, level base is clamped to the range [0, levels − 1], > level max is then clamped to the range [level base , levels − 1], and p is one > less than levels, where levels is the parameter passed to TexStorage* for the > texture object (see section 3.8.4). Otherwise p = log 2 (maxsize) + level base , > and all arrays from level base through q = min{p, level max } must be defined, > as discussed in section 3.8.13." > > Section 3.8.13 Texture Completeness says one necessary condition for a texture > to be complete is level base <= level max. It didn't mention anything about > clamping for immutable textures. > > kbr, bajones: inputs? In Section 3.9.2 Shader Execution, Texture Access, it simply referes to Section 3.8.12 Texture Completeness. So let's respect Section 3.8.12's definition for all textures, including immutable textures.
Not sure this looks good. https://codereview.chromium.org/1306343007/diff/20001/Source/modules/webgl/We... File Source/modules/webgl/WebGLTexture.cpp (right): https://codereview.chromium.org/1306343007/diff/20001/Source/modules/webgl/We... Source/modules/webgl/WebGLTexture.cpp:223: size_t maxLevel = m_isWebGL2OrHigher ? std::min(m_maxLevel, m_baseLevel + levelCount - 1) : m_baseLevel + levelCount - 1; Is this code correct? It looks to me like it's possible for m_maxLevel to be set to an out of range value (less than m_baseLevel), since update() has determined that the texture is incomplete -- possibly for exactly this reason. It looks to me like the value has to be clamped, rather than set to the result of std::min.
https://codereview.chromium.org/1306343007/diff/20001/Source/modules/webgl/We... File Source/modules/webgl/WebGLTexture.cpp (right): https://codereview.chromium.org/1306343007/diff/20001/Source/modules/webgl/We... Source/modules/webgl/WebGLTexture.cpp:223: size_t maxLevel = m_isWebGL2OrHigher ? std::min(m_maxLevel, m_baseLevel + levelCount - 1) : m_baseLevel + levelCount - 1; On 2015/09/09 01:30:26, Ken Russell wrote: > Is this code correct? It looks to me like it's possible for m_maxLevel to be set > to an out of range value (less than m_baseLevel), since update() has determined > that the texture is incomplete -- possibly for exactly this reason. It looks to > me like the value has to be clamped, rather than set to the result of std::min. in canGenerateMipmaps() the case where m_maxLevel < m_baseLevel is already rejected. So here we can safely assume m_baseLevel <= m_maxLevel. Maybe adding an ASSERT() to make it apparent.
https://codereview.chromium.org/1306343007/diff/20001/Source/modules/webgl/We... File Source/modules/webgl/WebGLTexture.cpp (right): https://codereview.chromium.org/1306343007/diff/20001/Source/modules/webgl/We... Source/modules/webgl/WebGLTexture.cpp:223: size_t maxLevel = m_isWebGL2OrHigher ? std::min(m_maxLevel, m_baseLevel + levelCount - 1) : m_baseLevel + levelCount - 1; On 2015/09/09 01:34:48, Zhenyao Mo wrote: > On 2015/09/09 01:30:26, Ken Russell wrote: > > Is this code correct? It looks to me like it's possible for m_maxLevel to be > set > > to an out of range value (less than m_baseLevel), since update() has > determined > > that the texture is incomplete -- possibly for exactly this reason. It looks > to > > me like the value has to be clamped, rather than set to the result of > std::min. > > in canGenerateMipmaps() the case where m_maxLevel < m_baseLevel is already > rejected. So here we can safely assume m_baseLevel <= m_maxLevel. Maybe adding > an ASSERT() to make it apparent. Sorry, could you please point me to that code? Looking at WebGLTexture::canGenerateMipmaps below I don't see any examination of m_maxLevel. It is looked at in WebGLTexture::update() but if that condition holds true then the texture's marked incomplete, which causes this code path to be taken.
https://codereview.chromium.org/1306343007/diff/20001/Source/modules/webgl/We... File Source/modules/webgl/WebGLTexture.cpp (right): https://codereview.chromium.org/1306343007/diff/20001/Source/modules/webgl/We... Source/modules/webgl/WebGLTexture.cpp:223: size_t maxLevel = m_isWebGL2OrHigher ? std::min(m_maxLevel, m_baseLevel + levelCount - 1) : m_baseLevel + levelCount - 1; On 2015/09/09 01:30:26, Ken Russell wrote: > Is this code correct? It looks to me like it's possible for m_maxLevel to be set > to an out of range value (less than m_baseLevel), since update() has determined > that the texture is incomplete -- possibly for exactly this reason. It looks to > me like the value has to be clamped, rather than set to the result of std::min. As we discussed in https://github.com/KhronosGroup/WebGL/pull/1177, it doesn't require GenerateMipmap produce an error if level_max < levle_base. The spec doesn't define this behavior.
https://codereview.chromium.org/1306343007/diff/20001/Source/modules/webgl/We... File Source/modules/webgl/WebGLTexture.cpp (right): https://codereview.chromium.org/1306343007/diff/20001/Source/modules/webgl/We... Source/modules/webgl/WebGLTexture.cpp:223: size_t maxLevel = m_isWebGL2OrHigher ? std::min(m_maxLevel, m_baseLevel + levelCount - 1) : m_baseLevel + levelCount - 1; On 2015/09/09 02:09:32, qiankun wrote: > On 2015/09/09 01:30:26, Ken Russell wrote: > > Is this code correct? It looks to me like it's possible for m_maxLevel to be > set > > to an out of range value (less than m_baseLevel), since update() has > determined > > that the texture is incomplete -- possibly for exactly this reason. It looks > to > > me like the value has to be clamped, rather than set to the result of > std::min. > > As we discussed in https://github.com/KhronosGroup/WebGL/pull/1177, it doesn't > require GenerateMipmap produce an error if level_max < levle_base. The spec > doesn't define this behavior. The question is what the loop below will do if maxLevel is out of range, and what the state of the texture will be afterward. It looks to me like at best it will short-circuit the loop and set m_isComplete to true, which seems like wrong behavior. I was concerned that it would perform out-of-range accesses on m_info[ii]. Asserts would go a long way toward helping prove the code won't crash.
https://codereview.chromium.org/1306343007/diff/20001/Source/modules/webgl/We... File Source/modules/webgl/WebGLTexture.cpp (right): https://codereview.chromium.org/1306343007/diff/20001/Source/modules/webgl/We... Source/modules/webgl/WebGLTexture.cpp:223: size_t maxLevel = m_isWebGL2OrHigher ? std::min(m_maxLevel, m_baseLevel + levelCount - 1) : m_baseLevel + levelCount - 1; On 2015/09/09 01:59:33, Ken Russell wrote: > On 2015/09/09 01:34:48, Zhenyao Mo wrote: > > On 2015/09/09 01:30:26, Ken Russell wrote: > > > Is this code correct? It looks to me like it's possible for m_maxLevel to be > > set > > > to an out of range value (less than m_baseLevel), since update() has > > determined > > > that the texture is incomplete -- possibly for exactly this reason. It looks > > to > > > me like the value has to be clamped, rather than set to the result of > > std::min. > > > > in canGenerateMipmaps() the case where m_maxLevel < m_baseLevel is already > > rejected. So here we can safely assume m_baseLevel <= m_maxLevel. Maybe > adding > > an ASSERT() to make it apparent. > > Sorry, could you please point me to that code? Looking at > WebGLTexture::canGenerateMipmaps below I don't see any examination of > m_maxLevel. It is looked at in WebGLTexture::update() but if that condition > holds true then the texture's marked incomplete, which causes this code path to > be taken. My bad. The check is in update(), unrelated to generateMipmap(). The value actually might go out of range, because m_info[ii] is of size m_maxTextureLevel or m_maxCubeMapTextureLevel, however, compueteLevelCount could also return the same value if width/height/depth are each at its maximum value. So if m_baseLevel sets a value larger than 0, and m_maxLevel also is set to a huge value, then min() is still out of range.
On 2015/09/09 02:27:19, Zhenyao Mo wrote: > https://codereview.chromium.org/1306343007/diff/20001/Source/modules/webgl/We... > File Source/modules/webgl/WebGLTexture.cpp (right): > > https://codereview.chromium.org/1306343007/diff/20001/Source/modules/webgl/We... > Source/modules/webgl/WebGLTexture.cpp:223: size_t maxLevel = m_isWebGL2OrHigher > ? std::min(m_maxLevel, m_baseLevel + levelCount - 1) : m_baseLevel + levelCount > - 1; > On 2015/09/09 01:59:33, Ken Russell wrote: > > On 2015/09/09 01:34:48, Zhenyao Mo wrote: > > > On 2015/09/09 01:30:26, Ken Russell wrote: > > > > Is this code correct? It looks to me like it's possible for m_maxLevel to > be > > > set > > > > to an out of range value (less than m_baseLevel), since update() has > > > determined > > > > that the texture is incomplete -- possibly for exactly this reason. It > looks > > > to > > > > me like the value has to be clamped, rather than set to the result of > > > std::min. > > > > > > in canGenerateMipmaps() the case where m_maxLevel < m_baseLevel is already > > > rejected. So here we can safely assume m_baseLevel <= m_maxLevel. Maybe > > adding > > > an ASSERT() to make it apparent. > > > > Sorry, could you please point me to that code? Looking at > > WebGLTexture::canGenerateMipmaps below I don't see any examination of > > m_maxLevel. It is looked at in WebGLTexture::update() but if that condition > > holds true then the texture's marked incomplete, which causes this code path > to > > be taken. > > My bad. The check is in update(), unrelated to generateMipmap(). > > The value actually might go out of range, because m_info[ii] is of size > m_maxTextureLevel or m_maxCubeMapTextureLevel, however, compueteLevelCount could > also return the same value if width/height/depth are each at its maximum value. > So if m_baseLevel sets a value larger than 0, and m_maxLevel also is set to a > huge value, then min() is still out of range. Basically the maxLevel should be clamped to m_info[0].size().
https://codereview.chromium.org/1306343007/diff/20001/Source/modules/webgl/We... File Source/modules/webgl/WebGLTexture.cpp (right): https://codereview.chromium.org/1306343007/diff/20001/Source/modules/webgl/We... Source/modules/webgl/WebGLTexture.cpp:223: size_t maxLevel = m_isWebGL2OrHigher ? std::min(m_maxLevel, m_baseLevel + levelCount - 1) : m_baseLevel + levelCount - 1; On 2015/09/09 02:27:19, Zhenyao Mo wrote: > On 2015/09/09 01:59:33, Ken Russell wrote: > > On 2015/09/09 01:34:48, Zhenyao Mo wrote: > > > On 2015/09/09 01:30:26, Ken Russell wrote: > > > > Is this code correct? It looks to me like it's possible for m_maxLevel to > be > > > set > > > > to an out of range value (less than m_baseLevel), since update() has > > > determined > > > > that the texture is incomplete -- possibly for exactly this reason. It > looks > > > to > > > > me like the value has to be clamped, rather than set to the result of > > > std::min. > > > > > > in canGenerateMipmaps() the case where m_maxLevel < m_baseLevel is already > > > rejected. So here we can safely assume m_baseLevel <= m_maxLevel. Maybe > > adding > > > an ASSERT() to make it apparent. > > > > Sorry, could you please point me to that code? Looking at > > WebGLTexture::canGenerateMipmaps below I don't see any examination of > > m_maxLevel. It is looked at in WebGLTexture::update() but if that condition > > holds true then the texture's marked incomplete, which causes this code path > to > > be taken. > > My bad. The check is in update(), unrelated to generateMipmap(). > > The value actually might go out of range, because m_info[ii] is of size > m_maxTextureLevel or m_maxCubeMapTextureLevel, however, compueteLevelCount could > also return the same value if width/height/depth are each at its maximum value. > So if m_baseLevel sets a value larger than 0, and m_maxLevel also is set to a > huge value, then min() is still out of range. WebGLRenderingContextBase::validateTexFuncDimensions() will make sure each level has its owner max size, e.g. 16384 for level 0, 16384>>level for other levels. So maxLevel will not exceed m_info[ii].size().
On 2015/09/09 04:36:06, qiankun wrote: > https://codereview.chromium.org/1306343007/diff/20001/Source/modules/webgl/We... > File Source/modules/webgl/WebGLTexture.cpp (right): > > https://codereview.chromium.org/1306343007/diff/20001/Source/modules/webgl/We... > Source/modules/webgl/WebGLTexture.cpp:223: size_t maxLevel = m_isWebGL2OrHigher > ? std::min(m_maxLevel, m_baseLevel + levelCount - 1) : m_baseLevel + levelCount > - 1; > On 2015/09/09 02:27:19, Zhenyao Mo wrote: > > On 2015/09/09 01:59:33, Ken Russell wrote: > > > On 2015/09/09 01:34:48, Zhenyao Mo wrote: > > > > On 2015/09/09 01:30:26, Ken Russell wrote: > > > > > Is this code correct? It looks to me like it's possible for m_maxLevel > to > > be > > > > set > > > > > to an out of range value (less than m_baseLevel), since update() has > > > > determined > > > > > that the texture is incomplete -- possibly for exactly this reason. It > > looks > > > > to > > > > > me like the value has to be clamped, rather than set to the result of > > > > std::min. > > > > > > > > in canGenerateMipmaps() the case where m_maxLevel < m_baseLevel is already > > > > rejected. So here we can safely assume m_baseLevel <= m_maxLevel. Maybe > > > adding > > > > an ASSERT() to make it apparent. > > > > > > Sorry, could you please point me to that code? Looking at > > > WebGLTexture::canGenerateMipmaps below I don't see any examination of > > > m_maxLevel. It is looked at in WebGLTexture::update() but if that condition > > > holds true then the texture's marked incomplete, which causes this code path > > to > > > be taken. > > > > My bad. The check is in update(), unrelated to generateMipmap(). > > > > The value actually might go out of range, because m_info[ii] is of size > > m_maxTextureLevel or m_maxCubeMapTextureLevel, however, compueteLevelCount > could > > also return the same value if width/height/depth are each at its maximum > value. > > So if m_baseLevel sets a value larger than 0, and m_maxLevel also is set to a > > huge value, then min() is still out of range. > > WebGLRenderingContextBase::validateTexFuncDimensions() will make sure each level > has its owner max size, e.g. 16384 for level 0, 16384>>level for other levels. > So maxLevel will not exceed m_info[ii].size(). OK, thanks for the explanation. Could you please add an ASSERT to make sure it does not exceed m_info[ii].size()? Otherwise, LGTM
> > WebGLRenderingContextBase::validateTexFuncDimensions() will make sure each > level > > has its owner max size, e.g. 16384 for level 0, 16384>>level for other levels. > > So maxLevel will not exceed m_info[ii].size(). > > OK, thanks for the explanation. Could you please add an ASSERT to make sure it > does not exceed m_info[ii].size()? > > Otherwise, LGTM Done.
I defer to zmo's review.
The CQ bit was checked by qiankun.miao@intel.com
The patchset sent to the CQ was uploaded after l-g-t-m from zmo@chromium.org Link to the patchset: https://codereview.chromium.org/1306343007/#ps40001 (title: "add assertion")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1306343007/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1306343007/40001
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://src.chromium.org/viewvc/blink?view=rev&revision=201975 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
