Closed Bug 874823 Opened 7 years ago Closed 7 years ago

OMTC appears to tell SharedSurfaceFactory that consGL is WebGL's gl.


(Core :: Canvas: WebGL, defect)

Not set





(Reporter: jgilbert, Assigned: jgilbert)




(1 file, 3 obsolete files)

This appears like it should break, but clearly seems to be working. Let's find out why.
Attached patch Pass the GLContext to WaitSync (obsolete) — Splinter Review
I think this bug only has issues when the two threads race. We have a crash on tinderbox (with the OMTC pref enabled) where the compositor thread is inside WaitSync(), and the content thread is inside Fence(). Both are using the same GLContext, and sadness ensues.

Attached patch is a draft that passes the consumer GLContext to WaitSync, rather than caching it when we create the factory.

One obvious issue is that it leaks GLContext into the SharedSurface abstraction, which is pretty ugly.

An alternative would be to have SwapConsumer *not* call WaitSync, and instead expect that caller to do this. We could then cast the returned SharedSurface to a SharedSurface_GLTexture (if appropriate) and call an alternative version of WaitSync.

Or we could try find a way to get the GLContext from the compositor when we initialize it, and cache it on the widget.
Attachment #754308 - Flags: feedback?(jgilbert)
Comment on attachment 754308 [details] [diff] [review]
Pass the GLContext to WaitSync

Review of attachment 754308 [details] [diff] [review]:

I think we're better off special-casing this for the GLTex subclass, rather than making a change to the ShSurf API.
Attachment #754308 - Flags: feedback?(jgilbert) → feedback-
Special-case this for the broken path. Support not knowing what consGL is until composition time, at which point we tell the ShSurf what its consGL should be. Assert that consGL is non-null at WaitSync time.
Attachment #754931 - Flags: review?(matt.woodrow)
Backed out:

Looks like a warnings-as-errors failure.
Also I got the bug number wrong. :C
What's wrong with me. Still the wrong bug.
Attachment #754308 - Attachment is obsolete: true
Attachment #754931 - Attachment is obsolete: true
Attachment #754931 - Flags: review?(matt.woodrow)
Attachment #757754 - Flags: review?(matt.woodrow)
Comment on attachment 757754 [details] [diff] [review]
patch: null consGL means use glFinish. Setting consGL will make subsequent fencing faster (use fences)

Review of attachment 757754 [details] [diff] [review]:

::: gfx/gl/SharedSurfaceGL.h
@@ +232,5 @@
> +
> +    // Custom:
> +    void SetConsumerGL(GLContext* consGL) {
> +        MOZ_ASSERT(consGL);
> +        mConsGL = consGL;

We should lock the mutex here too, or at least comment that we don't need to.
Attachment #757754 - Flags: review?(matt.woodrow) → review+
Fix and carry forward r+.
Assignee: nobody → jgilbert
Attachment #757754 - Attachment is obsolete: true
Attachment #757764 - Flags: review+
Does this fix the race?
Flags: needinfo?(matt.woodrow)
Looks like it, yes.
Flags: needinfo?(matt.woodrow)
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
Depends on: 882590
You need to log in before you can comment on or make changes to this bug.