Closed Bug 1310247 Opened 3 years ago Closed 3 years ago

WebGL 2.0 demo not work having useTransformFeedback enabled

Categories

(Core :: Canvas: WebGL, defect, P1)

51 Branch
defect

Tracking

()

RESOLVED FIXED
mozilla53
Tracking Status
firefox49 --- disabled
firefox50 --- disabled
firefox51 + fixed
firefox52 --- fixed
firefox53 --- fixed

People

(Reporter: bogdan_maris, Assigned: jgilbert)

References

Details

(Keywords: correctness, regression, Whiteboard: [gfx-noted])

Attachments

(4 files, 2 obsolete files)

[Affected versions]:
- latest Nightly 52.0a1
- latest Developer Edition 51.0a2

[Affected platforms]:
- Mac OS X 10.12
- Windows 7 32-bit

[Steps to reproduce]:
1. Start Firefox
2. Open http://toji.github.io/webgl2-crowd/
3. Check and uncheck useTransformFeedback

[Expected result]:
- Having Transform Feedback checked and unchecked the crowd is successfully seen. (I'm not sure of this expected result though)

[Actual result]:
- Crowd does not show having Transform Feedback checked

[Regression range]:
- Here is the regression range found using mozregression:

Last good revision: 4ebed327385b6827b9275c21e29f23b13aa92457
First bad revision: fa9844b0dee37aeb4c94d027f7c68a94721db320
Pushlog:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=4ebed327385b6827b9275c21e29f23b13aa92457&tochange=fa9844b0dee37aeb4c94d027f7c68a94721db320

I see a few bugs there Bug 1300946, Bug 1303879 and Bug 1303878

[Additional notes]:
- This is the Console output: http://pastebin.com/PABRTYSq
- Here is my Graphics from about:support

Features
Compositing	OpenGL
Asynchronous Pan/Zoom	wheel input enabled
WebGL Renderer	ATI Technologies Inc. -- AMD Radeon HD 6750M OpenGL Engine
WebGL2 Renderer	ATI Technologies Inc. -- AMD Radeon HD 6750M OpenGL Engine
Hardware H264 Decoding	Yes
Audio Backend	audiounit
GPU #1
Active	Yes
Vendor ID	0x1002
Device ID	0x6741
Diagnostics
AzureCanvasAccelerated	1
AzureCanvasBackend	skia
AzureContentBackend	skia
AzureFallbackCanvasBackend	none
TileHeight	512
TileWidth	512
QA Whiteboard: [qe-webgl2]
Attached patch fix WebGLRefCnt (obsolete) — Splinter Review
We got wrong mWebGLRefCnt in ValidateCanBindToTarget. That's because we store RefPtr<WebGLBuffer> in WebGLVertexAttribData and I think we can use raw pointer in WebGLVertexAttribData. Or alternatively, use another way to calculate bound number of the WebGLBuffer.
Attachment #8801664 - Flags: feedback?(jgilbert)
Keywords: correctness
Whiteboard: [gfx-noted]
Version: Trunk → 51 Branch
Assignee: nobody → ethlin
See Also: → 1316319
Track 51+ as this is a regression related to WebGL 2.
Attached patch Check the real bound buffers (obsolete) — Splinter Review
WebGLVertexAttribData also store the WebGLBuffer. So the ref count is not reliable. I think we should check the real bound buffers to get the number of bound.
Attachment #8811188 - Flags: review?(jgilbert)
Attachment #8801664 - Attachment is obsolete: true
Attachment #8801664 - Flags: feedback?(jgilbert)
(In reply to Ethan Lin[:ethlin] from comment #3)
> Created attachment 8811188 [details] [diff] [review]
> Check the real bound buffers
> 
> WebGLVertexAttribData also store the WebGLBuffer. So the ref count is not
> reliable. I think we should check the real bound buffers to get the number
> of bound.

I didn't consider some bound buffers, like mIndexedUniformBufferBindings and mBoundTransformFeedback->mIndexedBindings, which are hard to count in this way.
I add a variable in WebGLBuffer to record the number of bound. The number will change in BindBuffer / BindBufferBase / BindBufferRange. Some WebGLBuffers are bound in WebGLTransformFeedback, so the number also changes in the destructor of WebGLTransformFeedback.
Attachment #8811188 - Attachment is obsolete: true
Attachment #8811188 - Flags: review?(jgilbert)
Attachment #8811626 - Flags: review?(jgilbert)
Sorry to take this, but I think there's some spec-work needed.
I believe the restrictions in the WebGL 2 spec are wrong: They are either too lax, or too restrictive.

I'll sort this out with the working group.
I should have a patch tomorrow to serve as a WIP, which should fix this bug.
Assignee: ethlin → jgilbert
Comment on attachment 8811626 [details] [diff] [review]
calculate the number of bound for WebGLBuffer

Review of attachment 8811626 [details] [diff] [review]:
-----------------------------------------------------------------

I thought this would work, but I don't think it's actually sufficient.
Attachment #8811626 - Flags: review?(jgilbert) → review-
(In reply to Jeff Gilbert [:jgilbert] from comment #6)
> Sorry to take this, but I think there's some spec-work needed.
> I believe the restrictions in the WebGL 2 spec are wrong: They are either
> too lax, or too restrictive.
> 
> I'll sort this out with the working group.
> I should have a patch tomorrow to serve as a WIP, which should fix this bug.

Bug 1316319 should be the same problem. The attachment 8811626 [details] [diff] [review] also fixes bug 1316319. We can check it with your WIP.
Duplicate of this bug: 1316319
Priority: P4 → P1
Comment on attachment 8812399 [details]
Bug 1310247 - Check if buffers are bound for transform feedback. -

https://reviewboard.mozilla.org/r/94170/#review94412

::: dom/canvas/WebGLContext.cpp:2566
(Diff revision 1)
> +////
> +
> +const decltype(WebGLContext::mBuffersForUB)&
> +WebGLContext::BuffersForUB() const
> +{
> +    if (!mBuffersForUB_Dirty) {

Should be 'if (mBuffersForUB_Dirty)'.

::: dom/canvas/WebGLContext.cpp:2569
(Diff revision 1)
> +WebGLContext::BuffersForUB() const
> +{
> +    if (!mBuffersForUB_Dirty) {
> +        mBuffersForUB.clear();
> +        for (const auto& cur : mIndexedUniformBufferBindings) {
> +            mBuffersForUB.insert(cur.mBufferBinding.get());

We should ignore nullptr or nullptr will be counted as intersected buffer.

::: dom/canvas/WebGLTransformFeedback.cpp:53
(Diff revision 1)
> +    // "bound or in use for transform feedback".
> +    // Therefore, only the indexed bindings of the TFO count.
> +    if (mBuffersForTF_Dirty) {
> +        mBuffersForTF.clear();
> +        for (const auto& cur : mIndexedBindings) {
> +            mBuffersForTF.insert(cur.mBufferBinding.get());

We should ignore nullptr or nullptr will be counted as intersected buffer.
Comment on attachment 8812399 [details]
Bug 1310247 - Check if buffers are bound for transform feedback. -

https://reviewboard.mozilla.org/r/94170/#review94430
Comment on attachment 8812399 [details]
Bug 1310247 - Check if buffers are bound for transform feedback. -

https://reviewboard.mozilla.org/r/94170/#review94798
Attachment #8812399 - Flags: review?(ethlin) → review+
Blocks: 1316539
Comment on attachment 8813507 [details]
Bug 1310247 - Use pointers instead of references to prevent taking the address of temporary integers. -

https://reviewboard.mozilla.org/r/94926/#review95180
Attachment #8813507 - Flags: review?(ethlin) → review+
Pushed by jgilbert@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/52b48e6c648e
Check if buffers are bound for transform feedback. - r=ethlin
https://hg.mozilla.org/integration/mozilla-inbound/rev/9a514a653a23
Fix tests to match implementation for now.
https://hg.mozilla.org/integration/mozilla-inbound/rev/86d5fada043c
Use pointers instead of references to prevent taking the address of temporary integers. - r=ethlin
Comment on attachment 8812399 [details]
Bug 1310247 - Check if buffers are bound for transform feedback. -

Approval Request Comment
[Feature/regressing bug #]: webgl2
[User impact if declined]:
[Describe test coverage new/current, TreeHerder]:
[Risks and why]: 
[String/UUID change made/needed]:
Attachment #8812399 - Flags: approval-mozilla-beta?
Attachment #8812399 - Flags: approval-mozilla-aurora?
Comment on attachment 8813507 [details]
Bug 1310247 - Use pointers instead of references to prevent taking the address of temporary integers. -

Approval Request Comment
[Feature/regressing bug #]: webgl2
[User impact if declined]:
[Describe test coverage new/current, TreeHerder]:
[Risks and why]: 
[String/UUID change made/needed]:
Attachment #8813507 - Flags: approval-mozilla-beta?
Attachment #8813507 - Flags: approval-mozilla-aurora?
Comment on attachment 8813506 [details]
Bug 1310247 - Fix tests to match implementation for now.

Approval Request Comment
[Feature/regressing bug #]: webgl2
[User impact if declined]:
[Describe test coverage new/current, TreeHerder]:
[Risks and why]: 
[String/UUID change made/needed]:
Attachment #8813506 - Flags: approval-mozilla-beta?
Attachment #8813506 - Flags: approval-mozilla-aurora?
Comment on attachment 8812399 [details]
Bug 1310247 - Check if buffers are bound for transform feedback. -

Fix a WebGL 2 issue. Beta51+ and Aurora52+. Should be in 51 beta 4.
Attachment #8812399 - Flags: approval-mozilla-beta?
Attachment #8812399 - Flags: approval-mozilla-beta+
Attachment #8812399 - Flags: approval-mozilla-aurora?
Attachment #8812399 - Flags: approval-mozilla-aurora+
Attachment #8813506 - Flags: approval-mozilla-beta?
Attachment #8813506 - Flags: approval-mozilla-beta+
Attachment #8813506 - Flags: approval-mozilla-aurora?
Attachment #8813506 - Flags: approval-mozilla-aurora+
Attachment #8813507 - Flags: approval-mozilla-beta?
Attachment #8813507 - Flags: approval-mozilla-beta+
Attachment #8813507 - Flags: approval-mozilla-aurora?
Attachment #8813507 - Flags: approval-mozilla-aurora+
needs rebase for aurora
Flags: needinfo?(jgilbert)
You need to log in before you can comment on or make changes to this bug.