Closed Bug 1297965 Opened 8 years ago Closed 8 years ago

Enable ANGLE for WebGL2 on windows

Categories

(Core :: Graphics: CanvasWebGL, defect)

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

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla51
Tracking Flags:
Tracking Status
firefox50 --- fixed
firefox51 --- fixed

People

(Reporter: pchang, Assigned: mtseng)

References

Details

(Whiteboard: [gfx-noted])

Attachments

(4 files)

Bug 1297965 - Use ANGLE for WebGL2.
58 bytes, text/x-review-board-request
jgilbert
: review+
ritu
: approval-mozilla-aurora+
Details
Bug 1297965 - fail-if transform_feedback test case.
58 bytes, text/x-review-board-request
jgilbert
: review+
ritu
: approval-mozilla-aurora+
Details
Use ANGLE for WebGL2.
971 bytes, patch
Details | Diff | Splinter Review
fail-if transform_feedback test case.
3.16 KB, patch
Details | Diff | Splinter Review 
Right now we use native OpenGL for WebGL2 on windows.

https://dxr.mozilla.org/mozilla-central/source/dom/canvas/WebGLContext.cpp#731
Use this etherpad to track WebGL conformance failures  https://public.etherpad-mozilla.org/p/webgl2_angle
Whiteboard: [gfx-noted]
Depends on: 1299057
Comment on attachment 8786677 [details]
Bug 1297965 - Use ANGLE for WebGL2.

https://reviewboard.mozilla.org/r/75588/#review73690

This is not sufficient to protect us.
As the WebGL PR states, a buffer need only be bound to a "transform feedback binding point" to cause undefined values when read/written by another method.
However, the PR also adds a guarantee that BindBuffer should not work (INVALID_OP) if BindBufferBase/Range have already attached the buffer to a transform feedback binding point. This guarantee is in the WebGL2 spec still today.
We should just need to enforce this.

::: dom/canvas/WebGL2ContextBuffers.cpp:212
(Diff revision 1)
>          // writes might be consistent as long as transform feedback
>          // objects are not active, but neither GLES3.0 nor OpenGL 4.5
>          // spec guarantees this - just being bound for transform
>          // feedback is sufficient to cause undefined results.
>  
>          BindTransformFeedback(LOCAL_GL_TRANSFORM_FEEDBACK, nullptr);

This should be gl->fBindTransformFeedback.
Attachment #8786677 - Flags: review?(jgilbert) → review-
Yap, the PR also said, "an underlying OpenGL-based
WebGL implementation can ensure correct behavior by unbinding the buffer
from transform feedback prior to mapping it for reading, and then binding
it again for transform feedback once being done with reading."

But I'm not sure how to unbind the buffer from transform feedback.

Does calling "gl->fBindTransformFeedback(LOCAL_GL_TRANSFORM_FEEDBACK, 0)" means unbinding the buffer from transform feedback?

In the ANGLE implementation for transform feedback. They have a storage that store buffer binding info for each transform feedback. Consider following example:

gl.bindTransformFeedback(gl.TRANSFORM_FEEDBACK, tf)
gl.beginTransformFeedback()
... //do some transform feedback
gl.endTransformFeedback()

gl.bindBuffer(gl.TRANSFORM_FEEDBACK_BUFFER, buf)
gl.getBufferSubData(gl.TRANSFORM_FEEDBACK_BACK, 0, data)

So in the line "bindBuffer" we bind the "buf" to the "tf". Then next line getBufferSubData, we'll bind transform feedback to nullptr at first. In the mean time, ANGLE also change current transform feedback to default transform feedback. Then we call gl->fMapBufferRange in our implementation. But it will get buffer info from default transform feedback instead of "tf". So the test case will fail because we read from wrong buffer.

My patch here is rebind the buf to default transform feedback so we can read from correct buffer. Is the ANGLE implementation wrong in the transform feedback? Or there is something wrong here?

BTW, the case test is https://www.khronos.org/registry/webgl/sdk/tests/conformance2/transform_feedback/transform_feedback.html?webglVersion=2&quiet=0
Flags: needinfo?(jgilbert)
You patch doesn't detach it from any/all TF objects, so it's not guaranteed to work.
The PR adds restrictions so that no buffer can be bound if it's already attached to a TF binding point, so we shouldn't have to worry about detaching a buffer from TF objects, since if it's bound to the generic TF buffer binding, it can't have been bound to a TF object.
Flags: needinfo?(jgilbert)
Comment on attachment 8786677 [details]
Bug 1297965 - Use ANGLE for WebGL2.

https://reviewboard.mozilla.org/r/75588/#review73690

> This should be gl->fBindTransformFeedback.

Bind transform feedback won't work here. I submit another patch to rebind the buffer to ARRAY_BUFFER before mapping it for reading. Which match the WebGL PR suggested.
Oops, sorry I messed up the mozreview status.
Assignee: nobody → mtseng
Comment on attachment 8787534 [details]
Bug 1297965 - fail-if transform_feedback test case.

https://reviewboard.mozilla.org/r/76258/#review75290

::: dom/canvas/WebGL2ContextBuffers.cpp:213
(Diff revision 1)
>          // writes might be consistent as long as transform feedback
>          // objects are not active, but neither GLES3.0 nor OpenGL 4.5
>          // spec guarantees this - just being bound for transform
>          // feedback is sufficient to cause undefined results.
> -
> -        BindTransformFeedback(LOCAL_GL_TRANSFORM_FEEDBACK, nullptr);
> +        newTarget = LOCAL_GL_ARRAY_BUFFER;
> +        gl->fBindBuffer(newTarget, boundBuffer->mGLName);

gl->fBindTransformFeedback(this->mEmptyTF)
newTarget = LOCAL_GL_ARRAY_BUFFER;
gl->fBindBuffer(newTarget, boundBuffer->mGLName);

This stuff is a little weird, and we're actually not set up to handle it properly right now. I'll take a crack at it tomorrow.

It's relevent that Win+NV actually gets part of the spec wrong, but we can work around it.
Attachment #8787534 - Flags: review?(jgilbert) → review-
Depends on: 1300946
Comment on attachment 8787534 [details]
Bug 1297965 - fail-if transform_feedback test case.

https://reviewboard.mozilla.org/r/76258/#review75290

> gl->fBindTransformFeedback(this->mEmptyTF)
> newTarget = LOCAL_GL_ARRAY_BUFFER;
> gl->fBindBuffer(newTarget, boundBuffer->mGLName);
> 
> This stuff is a little weird, and we're actually not set up to handle it properly right now. I'll take a crack at it tomorrow.
> 
> It's relevent that Win+NV actually gets part of the spec wrong, but we can work around it.

Disable this test temporary.
Comment on attachment 8787534 [details]
Bug 1297965 - fail-if transform_feedback test case.

https://reviewboard.mozilla.org/r/76258/#review76080
Attachment #8787534 - Flags: review?(jgilbert) → review+
Comment on attachment 8786677 [details]
Bug 1297965 - Use ANGLE for WebGL2.

https://reviewboard.mozilla.org/r/75588/#review76082
Attachment #8786677 - Flags: review?(jgilbert) → review+
Comment on attachment 8786677 [details]
Bug 1297965 - Use ANGLE for WebGL2.

Approval Request Comment
With WebGL2 in FF50, we need this ANGLE update to fix and enable functionality.
Attachment #8786677 - Flags: approval-mozilla-aurora?
Comment on attachment 8787534 [details]
Bug 1297965 - fail-if transform_feedback test case.

See the previous patch.
Attachment #8787534 - Flags: approval-mozilla-aurora?
Comment on attachment 8786677 [details]
Bug 1297965 - Use ANGLE for WebGL2.

Let's do it, Aurora50+
Attachment #8786677 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8787534 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
The second patch needs rebasing for Aurora.
Flags: needinfo?(mtseng)
Blocks: 1302589
Patch for aurora.

MozReview-Commit-ID: A4oEZjToo3i
Patch for aurora.

MozReview-Commit-ID: 30pzBRyqS6s
I have rebased the patches to aurora. But there are 3 dependent bugs need to be uplifted first. So we should wait for all dependent bugs get approved.

All dependent are bug 1297924, bug 1299055 and bug 1299057

Thanks.
Flags: needinfo?(mtseng)
Backed out from Aurora because it depends on bug 1297924, which had to be backed out.

https://hg.mozilla.org/releases/mozilla-aurora/rev/2c332306c030
status-firefox50: fixed → affected
Blocks: 1297252
Blocks: 1297253
Blocks: 1289048
Blocks: 1289454
Blocks: 1296801
Blocks: 1297258
I did some local test. I think Bug 1293845 should also uplift to fix the ReadPixels assertion.
(In reply to Ethan Lin[:ethlin] from comment #34)
> I did some local test. I think Bug 1293845 should also uplift to fix the
> ReadPixels assertion.
Yes, this bug also fixed the mochitest failure in my local win 7.

Ritu, please help to approve the uplift request of bug 1293845.

At the same time, I submit another try to make sure we get green signals.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=b047cb2283d4
Flags: needinfo?(rkothari)
Done. The trees are locked now but in the meantime I've A+'d the patches for uplift.
Flags: needinfo?(rkothari)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: