1326159 - Failures in 2.0.0/conformance2/transform_feedback/transform_feedback.html

Status: RESOLVED FIXED

(Core :: Graphics: CanvasWebGL, defect, P1)

Description

[Kelsey Gilbert [:jgilbert]]

Reconsidering, our previous restriction choices are insufficient, though forbidding simultaneous TRANSFORM_FEEDBACK_BUFFER+ARRAY_BUFFER binding seems unnecessary.

Let's try without it and see what the damage is.

Comment 1

[Kelsey Gilbert [:jgilbert]]

Attachment: Bug 1326159 - Update TF buffer restrictions. -

Review commit: https://reviewboard.mozilla.org/r/101294/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/101294/

Comment 2

[Dzmitry Malyshau [:kvark]]

Comment on attachment 8822349 [details]
Bug 1326159 - Update TF buffer restrictions. -

https://reviewboard.mozilla.org/r/101294/#review101958

::: dom/canvas/WebGL2ContextBuffers.cpp:138
(Diff revision 1)
>  
>      if (byteLen) {
> -        const auto mappedBytes = gl->fMapBufferRange(target, srcByteOffset, glByteLen,
> +        const bool isTF = (target == LOCAL_GL_TRANSFORM_FEEDBACK_BUFFER);
> +        GLenum mapTarget = target;
> +        if (isTF) {
> +            gl->fBindTransformFeedback(LOCAL_GL_TRANSFORM_FEEDBACK, mEmptyTFO);

why do we need `mEmptuTFO` here if we can bind `0` instead?

::: dom/canvas/WebGLContext.h
(Diff revision 1)
>  
>      ////////////////////////////////////
>  
> -private:
> -    mutable bool mBuffersForUB_Dirty;
> -    mutable std::set<const WebGLBuffer*> mBuffersForUB;

nice to see these removed

::: dom/canvas/WebGLContextBuffers.cpp:487
(Diff revision 1)
>          if (bindPoint == buffer) {
> -            bindPoint = nullptr;
> +            WebGLBuffer::SetSlot(target, nullptr, &bindPoint);
>          }
>      };
>  
> -    fnClearIfBuffer(mBoundArrayBuffer);
> +    fnClearIfBuffer(0, mBoundArrayBuffer);

I don't think `0` is a valid parameter here, given the expected `GLenum` type. Perhaps, you want `LOCAL_GL_ARRAY_BUFFER` instead?

::: dom/canvas/WebGLContextBuffers.cpp:512
(Diff revision 1)
> +                                binding.mBufferBinding);
>              }
>          }
>  
>          for (auto& binding : mIndexedUniformBufferBindings) {
> -            fnClearIfBuffer(binding.mBufferBinding);
> +            fnClearIfBuffer(0, binding.mBufferBinding);

shouldn't this be `LOCAL_GL_UNIFORM_BUFFER`?

::: dom/canvas/WebGLProgram.cpp:719
(Diff revision 1)
>         }
>      }
>  
>      switch (pname) {
>      case LOCAL_GL_ATTACHED_SHADERS:
> +        return JS::NumberValue( int(bool(mVertShader.get())) + int(bool(mFragShader)) );

shouldn't we use `mFragShader.get()` here as well?

::: dom/canvas/WebGLVertexArray.cpp:39
(Diff revision 1)
> +}
> +
> +void
> +WebGLVertexArray::AddBufferBindCounts(int8_t addVal) const
> +{
> +    const GLenum target = 0; // Anything non-TF is fine.

This is similar to the previous cases, but with justification in the comment. I don't think it's sensible to have `GLenum` in the API that is treated as a `bool`.

Comment 3

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8822349 [details]
Bug 1326159 - Update TF buffer restrictions. -

https://reviewboard.mozilla.org/r/101294/#review101958

> why do we need `mEmptuTFO` here if we can bind `0` instead?

0 is the default TFO, which is a fully-functional TFO, which can have non-zero bindings.

Comment 4

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8822349 [details]
Bug 1326159 - Update TF buffer restrictions. -

https://reviewboard.mozilla.org/r/101294/#review101958

> I don't think `0` is a valid parameter here, given the expected `GLenum` type. Perhaps, you want `LOCAL_GL_ARRAY_BUFFER` instead?

I'm short-handing it. Really anything that's not TRANSFORM_FEEDBACK_BUFFER has the desired effect.

> shouldn't this be `LOCAL_GL_UNIFORM_BUFFER`?

I'm short-handing it.

> shouldn't we use `mFragShader.get()` here as well?

Since bool(mFragShader) works, I shouldn't use .get() on either.

> This is similar to the previous cases, but with justification in the comment. I don't think it's sensible to have `GLenum` in the API that is treated as a `bool`.

I think it's a little cleaner this way, as well as allowing passing `target` directly.
Otherwise we're passing blind true/false around in most of these cases, which is generally worse. ("what does `true` mean at this callsite?")

Comment 5

[Dzmitry Malyshau [:kvark]]

Comment on attachment 8822349 [details]
Bug 1326159 - Update TF buffer restrictions. -

https://reviewboard.mozilla.org/r/101294/#review101980

::: dom/canvas/WebGLVertexArray.cpp:39
(Diff revision 1)
> +}
> +
> +void
> +WebGLVertexArray::AddBufferBindCounts(int8_t addVal) const
> +{
> +    const GLenum target = 0; // Anything non-TF is fine.

Yes, I agree bool arguments in general are bad. But I didn't propose to change it to `bool`, I just said it's treated as one. So you might as well have an `enum` with `IsTF` and `NonTF` values or somethine.

Comment 6

[Daosheng Mu[:daoshengmu]]

Comment on attachment 8822349 [details]
Bug 1326159 - Update TF buffer restrictions. -

https://reviewboard.mozilla.org/r/101294/#review102004

LGTM

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/aa8818f77045
Update TF buffer restrictions. - r=daoshengmu

Comment 8

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8822349 [details]
Bug 1326159 - Update TF buffer restrictions. -

Approval Request Comment
[Feature/Bug causing the regression]: webgl2-blocker
[User impact if declined]:
[Is this code covered by automated tests?]:
[Has the fix been verified in Nightly?]:
[Needs manual test from QE? If yes, steps to reproduce]: 
[List of other uplifts needed for the feature/fix]:
[Is the change risky?]:
[Why is the change risky/not risky?]:
[String changes made/needed]:

Comment 9

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/aa8818f77045

Comment 10

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8822349 [details]
Bug 1326159 - Update TF buffer restrictions. -

Another fairly big change. Looks like we need this for WebGL2 enabling on 51.

Comment 11

[Kelsey Gilbert [:jgilbert]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/7368fa4424f5
https://hg.mozilla.org/releases/mozilla-beta/rev/405b825d5efd