Closed Bug 1300946 Opened 8 years ago Closed 8 years ago

Fix transform feedback state handling

Categories

(Core :: Graphics: CanvasWebGL, defect)

Product:
Core
Component:
Graphics: CanvasWebGL
Version:
51 Branch
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla52
Tracking Flags:
Tracking Status
firefox50 --- fixed
firefox51 --- fixed
firefox52 --- fixed

People

(Reporter: jgilbert, Assigned: jgilbert)

References

Details

Attachments

(5 files)

Bug 1300946 - GetCurrentBinding is the wrong approach. -
58 bytes, text/x-review-board-request
jrmuizel
: review+
ritu
: approval-mozilla-aurora+
ritu
: approval-mozilla-beta+
Details
Bug 1300946 - Implement transform feedback. -
58 bytes, text/x-review-board-request
jrmuizel
: review+
Details
Bug 1300946 - Forbid simultaneous binding to TF and non-TF bind points. -
58 bytes, text/x-review-board-request
jrmuizel
: review+
Details
Bug 1300946 - Binding a deleted TFO should be INVALID_OP. -
58 bytes, text/x-review-board-request
ethlin
: review+
Details
Bug 1300946 - Only clear TFO indexed bindings on delete if TFO is inactive. -
58 bytes, text/x-review-board-request
ethlin
: review+
Details 
We plain get this wrong.

In particular, the TRANSFORM_FEEDBACK_BUFFER_BINDING should be attached to the TransformFeedback object. (though Win+NV's driver gets this wrong too. Easy enough to work around)
Assignee: nobody → jgilbert
Comment on attachment 8792650 [details]
Bug 1300946 - Binding a deleted TFO should be INVALID_OP. -

https://reviewboard.mozilla.org/r/79562/#review78280

The 'ValidateObjectAllowNull' always set 'InvalidValue' if the object is deleted, so we have to do additional checking here for returning 'InvalidOperation'.
Attachment #8792650 - Flags: review?(ethlin) → review+
(In reply to Ethan Lin[:ethlin] from comment #10)
> Comment on attachment 8792650 [details]
> Bug 1300946 - Binding a deleted TFO should be INVALID_OP. -
> 
> https://reviewboard.mozilla.org/r/79562/#review78280
> 
> The 'ValidateObjectAllowNull' always set 'InvalidValue' if the object is
> deleted, so we have to do additional checking here for returning
> 'InvalidOperation'.

Yep, it sucks. It's not a perf hit though. (this command isn't in any hot loops)
Comment on attachment 8792651 [details]
Bug 1300946 - Only clear TFO indexed bindings on delete if TFO is inactive. -

https://reviewboard.mozilla.org/r/79564/#review78304
Attachment #8792651 - Flags: review?(ethlin) → review+
Comment on attachment 8792648 [details]
Bug 1300946 - Implement transform feedback. -

https://reviewboard.mozilla.org/r/79558/#review78238

::: dom/canvas/WebGL2Context.cpp:218
(Diff revision 2)
> +
> +    ////
> +
> +    gl->fBindTransformFeedback(LOCAL_GL_TRANSFORM_FEEDBACK, 0);
> +    gl->fBindBuffer(LOCAL_GL_TRANSFORM_FEEDBACK_BUFFER, 0);
> +    */

This can go

::: dom/canvas/WebGL2ContextBuffers.cpp:147
(Diff revision 2)
> -     * the WebGL API to ensure that data are access
> -     * consistently. This applies even if the buffer is currently
> -     * bound to a transform feedback binding point.
> -     */
>  
> -    void* ptr = gl->fMapBufferRange(target, offset, data.LengthAllowShared(),
> +    const auto ptr = gl->fMapBufferRange(target, offset, data.LengthAllowShared(),

This hides the type of ptr which doesn't seem to have much value.

::: dom/canvas/WebGLContextBuffers.cpp:68
(Diff revision 2)
> +}
> +
> +WebGLBuffer*
> +WebGLContext::ValidateBufferSelection(const char* funcName, GLenum target)
> +{
> +    const auto& slot = ValidateBufferSlot(funcName, target);

Why do you want a reference to a pointer?

::: dom/canvas/WebGLContextBuffers.cpp:71
(Diff revision 2)
> +WebGLContext::ValidateBufferSelection(const char* funcName, GLenum target)
> +{
> +    const auto& slot = ValidateBufferSlot(funcName, target);
> +    if (!slot)
> +        return nullptr;
> +    const auto& buffer = *slot;

Same here. Also, this function and others like it would be easier to read using explicit types instead of 'auto'. Having to lookup the return type of ValidateBufferSlot to understand what's going on isn't so nice.

::: dom/canvas/WebGLContextBuffers.cpp:518
(Diff revision 2)
>                              const dom::ArrayBufferView& data)
>  {
>      BufferSubDataT(target, byteOffset, data);
>  }
>  
> +////////////////////////////////////////

What is the purpose of these breaks? How do you decide when to put them in?

::: dom/canvas/WebGLContextDraw.cpp
(Diff revision 2)
> +        return;
>  
>      {
>          ScopedMaskWorkaround autoMask(*this);
> -
> +        gl->fDrawElements(mode, vertCount, type,
> -        if (gl->IsSupported(gl::GLFeature::draw_range_elements)) {

Why is DrawRangeElements being dropped?

::: dom/canvas/WebGLProgram.h:92
(Diff revision 2)
>  
>      WebGLProgram* const prog;
>  
>      std::vector<AttribInfo> attribs;
>      std::vector<UniformInfo*> uniforms; // Owns its contents.
> -    std::vector<const UniformBlockInfo*> uniformBlocks; // Owns its contents.
> +    std::vector<UniformBlockInfo*> uniformBlocks; // Owns its contents.

Can you change these to UniquePtrs in a separate patch?

::: dom/canvas/WebGLProgram.h:99
(Diff revision 2)
>  
>      // Needed for draw call validation.
>      std::vector<UniformInfo*> uniformSamplers;
>  
> +    mutable std::vector<size_t> componentsPerTFVert;
> +    mutable std::set<WebGLTransformFeedback*> activeTFOs;

is this unused?

::: dom/canvas/WebGLProgram.h:100
(Diff revision 2)
>      // Needed for draw call validation.
>      std::vector<UniformInfo*> uniformSamplers;
>  
> +    mutable std::vector<size_t> componentsPerTFVert;
> +    mutable std::set<WebGLTransformFeedback*> activeTFOs;
> +    mutable std::vector<IndexedBufferBinding> uniformBlockBindings;

Is this used?
Comment on attachment 8792649 [details]
Bug 1300946 - Forbid simultaneous binding to TF and non-TF bind points. -

https://reviewboard.mozilla.org/r/79560/#review78610

::: dom/canvas/WebGLBuffer.cpp:194
(Diff revision 2)
>  {
>      return mCache->BeenUsedWithMultipleTypes();
>  }
>  
> +bool
> +WebGLBuffer::ValidateCanBindToTarget(const char* funcName, GLenum target)

Did anything change in this function? The diff doesn't say.
Comment on attachment 8792647 [details]
Bug 1300946 - GetCurrentBinding is the wrong approach. -

https://reviewboard.mozilla.org/r/79556/#review78612
Attachment #8792647 - Flags: review?(jmuizelaar) → review+
Comment on attachment 8792649 [details]
Bug 1300946 - Forbid simultaneous binding to TF and non-TF bind points. -

https://reviewboard.mozilla.org/r/79560/#review78610

> Did anything change in this function? The diff doesn't say.

The mBoundForTF bit is new.
Comment on attachment 8792648 [details]
Bug 1300946 - Implement transform feedback. -

https://reviewboard.mozilla.org/r/79558/#review78238

> Why do you want a reference to a pointer?

It decays into a value if given a value. `const auto&` is roughly "give this value a name, and don't make a copy if you con't need to".

> Same here. Also, this function and others like it would be easier to read using explicit types instead of 'auto'. Having to lookup the return type of ValidateBufferSlot to understand what's going on isn't so nice.

I guess? I generally have to look up the function I'm using anyways, to make sure I know what it's doing. This generally tells me what I'm being handed 'for free'.

> What is the purpose of these breaks? How do you decide when to put them in?

It's just a form of pseudo-whitespace separation. It's like paragraph breaks in prose.

> Why is DrawRangeElements being dropped?

It has no future, since we'll be moving to drawElements+robust_buffer_access_behavior soon.
Might as well drop the complexity now.

> Can you change these to UniquePtrs in a separate patch?

This is now bug 1304507.

> is this unused?

Yes.

> Is this used?

No.
Comment on attachment 8792648 [details]
Bug 1300946 - Implement transform feedback. -

https://reviewboard.mozilla.org/r/79558/#review79196
Attachment #8792648 - Flags: review?(jmuizelaar) → review+
Comment on attachment 8792649 [details]
Bug 1300946 - Forbid simultaneous binding to TF and non-TF bind points. -

https://reviewboard.mozilla.org/r/79560/#review78668
Attachment #8792649 - Flags: review?(jmuizelaar) → review+
Pushed by jgilbert@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a01c95a3bb4e
GetCurrentBinding is the wrong approach. - r=jrmuizel
https://hg.mozilla.org/integration/mozilla-inbound/rev/7a6514210303
Implement transform feedback. - r=jrmuizel
https://hg.mozilla.org/integration/mozilla-inbound/rev/ff3ec914ce4d
Forbid simultaneous binding to TF and non-TF bind points. - r=jrmuizel
https://hg.mozilla.org/integration/mozilla-inbound/rev/3c256ba175d7
Binding a deleted TFO should be INVALID_OP. - r=ethlin
https://hg.mozilla.org/integration/mozilla-inbound/rev/84f04631a9a2
Only clear TFO indexed bindings on delete if TFO is inactive. - r=ethlin
https://hg.mozilla.org/integration/mozilla-inbound/rev/fa9844b0dee3
TF test passes on Windows.
Comment on attachment 8792647 [details]
Bug 1300946 - GetCurrentBinding is the wrong approach. -

Approval Request Comment
[Feature/regressing bug #]: webgl2
[User impact if declined]: bad webgl2
[Describe test coverage new/current, TreeHerder]: conformance suite
[Risks and why]: low. This is pretty well understood.
[String/UUID change made/needed]: none

This applies to all patches in this bug.
Attachment #8792647 - Flags: approval-mozilla-aurora?
Comment on attachment 8792647 [details]
Bug 1300946 - GetCurrentBinding is the wrong approach. -

WebGL2 is planned for Beta50, let's stabilize this on Aurora51 and if there are no problems I plan to include this in 50.0b4 on 10/03.
Attachment #8792647 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Blocks: 1306480
I had issues uplifting these patches. Could we get a rebased version?
Flags: needinfo?(jgilbert)
Oh, guess I tried in the wrong order, doing this last worked.
Flags: needinfo?(jgilbert)
Comment on attachment 8792647 [details]
Bug 1300946 - GetCurrentBinding is the wrong approach. -

WebGL2 is planned for Fx50, Beta+ for all patches.
Attachment #8792647 - Flags: approval-mozilla-beta+
Depends on: 1313875
Depends on: 1315979
Depends on: 1316327
Depends on: 1321768
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: