Failures in 2.0.0/conformance2/transform_feedback/transform_feedback.html

RESOLVED FIXED in Firefox 51

Status

()

defect
P1
normal
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: jgilbert, Assigned: jgilbert)

Tracking

unspecified
mozilla53
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox51 fixed, firefox52 fixed, firefox53 fixed)

Details

(Whiteboard: gfx-noted)

Attachments

(1 attachment)

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 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 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 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 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 on attachment 8822349 [details]
Bug 1326159 - Update TF buffer restrictions. -

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

LGTM
Attachment #8822349 - Flags: review?(dmu) → review+
Pushed by jgilbert@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/aa8818f77045
Update TF buffer restrictions. - r=daoshengmu
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]:
Attachment #8822349 - Flags: approval-mozilla-beta?
Attachment #8822349 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/aa8818f77045
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
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.
Attachment #8822349 - Flags: approval-mozilla-beta?
Attachment #8822349 - Flags: approval-mozilla-beta+
Attachment #8822349 - Flags: approval-mozilla-aurora?
Attachment #8822349 - Flags: approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.