Implementing Blink-side validation for WebGL 2 Samplers

Implementing Blink-side validation for WebGL 2 Samplers

Does not allow the related tests to pass yet, needs some command buffer work for
that. That will come in a follow up patch.

BUG=563613
Committed: https://crrev.com/06d25d937636cb973a8e65f77d7cc189179785d2
Cr-Commit-Position: refs/heads/master@{#363058}

[bajones, 2015-12-01 23:26:03]

bajones@chromium.org changed reviewers:
+ kbr@chromium.org, zmo@chromium.org

[bajones, 2015-12-01 23:26:03]

PTAL. I want to leave the command buffer side of this to a separate CL for ease
of review. (It'll be bigger because it doesn't have and sampler state tracking
yet.)

[Zhenyao Mo, 2015-12-02 01:16:32]

I am not sure I agree with the philosophy of this CL.

I feel we should stick to the original design, i.e., compute all the flags per
texture update (now we need to call update() in more places, where a sampler
gets attached/detached, or an attached sampler is updated, and we probably need
to pass the sampler to the update() function.

It's a lot of work, but then it's per texture/sampler update, not per draw call
computation.

https://codereview.chromium.org/1490043002/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/webgl/WebGLTexture.h (right):

https://codereview.chromium.org/1490043002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/webgl/WebGLTexture.h:34: struct
WebGLSamplerState {
Maybe WebGLTexSamplingState?  SamplerState sounds like it should belong to
WebGLSampler.h

[Ken Russell (switch to Gerrit), 2015-12-02 01:49:27]

On 2015/12/02 01:16:32, Zhenyao Mo wrote:
> I am not sure I agree with the philosophy of this CL.
> 
> I feel we should stick to the original design, i.e., compute all the flags per
> texture update (now we need to call update() in more places, where a sampler
> gets attached/detached, or an attached sampler is updated, and we probably
need
> to pass the sampler to the update() function.
> 
> It's a lot of work, but then it's per texture/sampler update, not per draw
call
> computation.

There's a basic problem that the same texture can be used with two different
samplers. Some of the cached values can then no longer be kept in the texture.
They could plausibly be kept on the texture unit, or multiple caches could be
kept, some in the texture and some on the texture unit, which could be
overridden by the presence of the sampler.

I'm not sure what the best approach is. Agree that reducing the amount of
per-draw-call validation is the most important criterion, but we may need to
make incremental steps toward that goal.

Also, if we can delegate more of the incomplete texture handling to the command
buffer, that would be ideal. The only thing this code should need to do at this
point is handle differences in behavior between OpenGL ES and WebGL. For ES 2.0
that means that if the underlying implementation supports the NPOT texture
extension, this code needs to squelch that and forbid their usage; but WebGL 2.0
and ES 3.0 behave identically in this area.

>
https://codereview.chromium.org/1490043002/diff/1/third_party/WebKit/Source/m...
> File third_party/WebKit/Source/modules/webgl/WebGLTexture.h (right):
> 
>
https://codereview.chromium.org/1490043002/diff/1/third_party/WebKit/Source/m...
> third_party/WebKit/Source/modules/webgl/WebGLTexture.h:34: struct
> WebGLSamplerState {
> Maybe WebGLTexSamplingState?  SamplerState sounds like it should belong to
> WebGLSampler.h

[Ken Russell (switch to Gerrit), 2015-12-02 02:03:35]

I think the factoring of the state that resides both in textures and samplers
into a separate struct is good overall. Some questions.

https://codereview.chromium.org/1490043002/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/webgl/WebGLTexture.cpp (right):

https://codereview.chromium.org/1490043002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/webgl/WebGLTexture.cpp:305: if
(m_needToUseBlackTexture || !samplerState)
The test of "!samplerState" is really confusing given how intricate the logic is
around fetching the texture unit's sampler state for WebGL 1 vs. WebGL 2. Can
you organize this logic more carefully and perhaps split the validation between
WebGL 1 and 2? Can we add assertions about whether samplerState should be null
or non-null in certain situations?

https://codereview.chromium.org/1490043002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/webgl/WebGLTexture.cpp:308: if
(m_isWebGL2OrHigher && !m_isComplete && samplerState->minFilter != GL_NEAREST &&
samplerState->minFilter != GL_LINEAR)
If m_isWebGL2OrHigher then the underlying implementation is providing ES 3.0
semantics, so it should already be handling the case correctly if the texture's
not (mipmap) complete and it's using a filter that requires mipmaps. So can we
delegate this logic to the lower levels and just not have it here?

https://codereview.chromium.org/1490043002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/webgl/WebGLTexture.cpp:360: const LevelInfo&
base = m_info[0][m_baseLevel];
Why is this code duplicated (essentially) from WebGLTexture::update? Is that
only needed for WebGL 2.0 and above? At least please add a comment.

https://codereview.chromium.org/1490043002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/webgl/WebGLTexture.cpp:414: for (size_t ii =
0; ii < m_info.size() && m_isComplete; ++ii) {
Was the previous code wrong? It looks to me like m_isComplete was always true
coming in here, and if if was ever set to false, we'd break out of the loop. The
previous code looks wrong but this change looks redundant.

https://codereview.chromium.org/1490043002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/webgl/WebGLTexture.cpp:456: || (m_target ==
GL_TEXTURE_3D && m_samplerState.wrapR != GL_CLAMP_TO_EDGE)))
m_target will never be GL_TEXTURE_3D if (!m_isWebGL2OrHigher). Can this logic be
simplified?

[qiankun, 2015-12-02 15:28:58]

qiankun.miao@intel.com changed reviewers:
+ qiankun.miao@intel.com

[qiankun, 2015-12-02 15:28:58]

https://codereview.chromium.org/1490043002/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/webgl/WebGLTexture.cpp (right):

https://codereview.chromium.org/1490043002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/webgl/WebGLTexture.cpp:437: m_isComplete =
false;
If m_isComplete is set false here, we should not exist the loop. We should
continue to calculate status of m_isCubeComplete.

[qiankun, 2015-12-02 15:29:54]

On 2015/12/02 15:28:58, qiankun wrote:
>
https://codereview.chromium.org/1490043002/diff/1/third_party/WebKit/Source/m...
> File third_party/WebKit/Source/modules/webgl/WebGLTexture.cpp (right):
> 
>
https://codereview.chromium.org/1490043002/diff/1/third_party/WebKit/Source/m...
> third_party/WebKit/Source/modules/webgl/WebGLTexture.cpp:437: m_isComplete =
> false;
> If m_isComplete is set false here, we should not exist the loop. We should
> continue to calculate status of m_isCubeComplete.

exist -> exit

[bajones, 2015-12-02 19:50:27]

Thanks everyone for your feedback, online and off. Addressing feedback now,
although there was a weird glitch here that caused some unintended diffs. :(

I should have an updated CL up later today.

https://codereview.chromium.org/1490043002/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/webgl/WebGLTexture.cpp (right):

https://codereview.chromium.org/1490043002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/webgl/WebGLTexture.cpp:360: const LevelInfo&
base = m_info[0][m_baseLevel];
On 2015/12/02 02:03:35, Ken Russell wrote:
> Why is this code duplicated (essentially) from WebGLTexture::update? Is that
> only needed for WebGL 2.0 and above? At least please add a comment.

This wasn't intended to be part of this CL, and I'm honestly not sure where the
code came from. Rebasing/re-examining now to figure out what happened, but
thanks for catching it.

https://codereview.chromium.org/1490043002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/webgl/WebGLTexture.cpp:414: for (size_t ii =
0; ii < m_info.size() && m_isComplete; ++ii) {
On 2015/12/02 02:03:35, Ken Russell wrote:
> Was the previous code wrong? It looks to me like m_isComplete was always true
> coming in here, and if if was ever set to false, we'd break out of the loop.
The
> previous code looks wrong but this change looks redundant.

Same here, this wasn't a change I intentionally made.

[bajones, 2015-12-02 22:17:52]

Okay, Addressed everyone's feedback and removed the unintentional changes.
(Still not clear on how they got there. Bad rebase, maybe?)

PTAL!

[yunchao, 2015-12-03 02:01:24]

On 2015/12/02 22:17:52, bajones wrote:
> Okay, Addressed everyone's feedback and removed the unintentional changes.
> (Still not clear on how they got there. Bad rebase, maybe?)
> 
> PTAL!

Brandon and all, Guanxian has been doing the work for sampler and texture in
both Blink and command buffer for a while. I suppose that he can have a look.

[Zhenyao Mo, 2015-12-03 18:21:53]

LGTM

https://codereview.chromium.org/1490043002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.cpp
(right):

https://codereview.chromium.org/1490043002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.cpp:3227:
WebGLSampler* sampler = m_samplerUnits[unit];
assert that unit < m_samplerUnits.size()?

https://codereview.chromium.org/1490043002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp
(right):

https://codereview.chromium.org/1490043002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:2052:
WebGLTexture* texture = nullptr;
assert that unit < m_textureUnits.size()?

[bajones, 2015-12-03 18:38:50]

The CQ bit was checked by bajones@chromium.org

[bajones, 2015-12-03 18:38:50]

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/1490043002/#ps40001
(title: "Addressed zmo@'s new feedback")

[commit-bot: I haz the power, 2015-12-03 18:40:33]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1490043002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1490043002/40001

[commit-bot: I haz the power, 2015-12-03 21:10:39]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-12-03 21:12:17]

Description was changed from

==========
Implementing Blink-side validation for WebGL 2 Samplers

Does not allow the related tests to pass yet, needs some command buffer work for
that. That will come in a follow up patch.

BUG=563613
==========

to

==========
Implementing Blink-side validation for WebGL 2 Samplers

Does not allow the related tests to pass yet, needs some command buffer work for
that. That will come in a follow up patch.

BUG=563613

Committed: https://crrev.com/06d25d937636cb973a8e65f77d7cc189179785d2
Cr-Commit-Position: refs/heads/master@{#363058}
==========

[commit-bot: I haz the power, 2015-12-03 21:12:18]

Patchset 3 (id:??) landed as
https://crrev.com/06d25d937636cb973a8e65f77d7cc189179785d2
Cr-Commit-Position: refs/heads/master@{#363058}

[Ken Russell (switch to Gerrit), 2015-12-03 23:17:13]

Sorry for the delay re-reviewing this, but LGTM after the fact.