1310247 - WebGL 2.0 demo not work having useTransformFeedback enabled

Status: RESOLVED FIXED

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

Description

[Bogdan Maris, Desktop QA]

[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

Comment 1

[Ethan Lin[:ethlin]]

Attachment: fix WebGLRefCnt

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.

Comment 2

[Gerry Chang [:gchang]]

Track 51+ as this is a regression related to WebGL 2.

Comment 3

[Ethan Lin[:ethlin]]

Attachment: 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.

Comment 4

[Ethan Lin[:ethlin]]

(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.

Comment 5

[Ethan Lin[:ethlin]]

Attachment: calculate the number of bound for WebGLBuffer

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.

Comment 6

[Kelsey Gilbert [: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.

Comment 7

[Kelsey Gilbert [: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.

Comment 8

[Ethan Lin[:ethlin]]

(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.

Comment 10

[Kelsey Gilbert [:jgilbert]]

Attachment: Bug 1310247 - Check if buffers are bound for transform feedback. -

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

Comment 11

[Ethan Lin[:ethlin]]

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 12

[Ethan Lin[:ethlin]]

Comment on attachment 8812399 [details]
Bug 1310247 - Check if buffers are bound for transform feedback. -

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

Comment 13

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8812399 [details]
Bug 1310247 - Check if buffers are bound for transform feedback. -

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/94170/diff/1-2/

Comment 14

[Ethan Lin[:ethlin]]

Comment on attachment 8812399 [details]
Bug 1310247 - Check if buffers are bound for transform feedback. -

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

Comment 15

[Kelsey Gilbert [:jgilbert]]

Attachment: Bug 1310247 - Fix tests to match implementation for now.

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

Comment 16

[Kelsey Gilbert [:jgilbert]]

Attachment: Bug 1310247 - Use pointers instead of references to prevent taking the address of temporary integers. -

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

Comment 17

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8812399 [details]
Bug 1310247 - Check if buffers are bound for transform feedback. -

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/94170/diff/2-3/

Comment 18

[Ethan Lin[:ethlin]]

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

Comment 19

[Pulsebot]

Pushed by [e-mail]:
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 20

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/52b48e6c648e
https://hg.mozilla.org/mozilla-central/rev/9a514a653a23
https://hg.mozilla.org/mozilla-central/rev/86d5fada043c

Comment 21

[Peter Chang[:pchang]]

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]:

Comment 22

[Peter Chang[:pchang]]

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]:

Comment 23

[Peter Chang[:pchang]]

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]:

Comment 24

[Gerry Chang [:gchang]]

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.

Comment 25

[Carsten Book [:Tomcat]]

needs rebase for aurora

Comment 26

[Kelsey Gilbert [:jgilbert]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/33ed433e98aa

Comment 27

[Kelsey Gilbert [:jgilbert]]

https://hg.mozilla.org/releases/mozilla-beta/rev/a0b2e78bf20d