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

Status: RESOLVED FIXED

(Core :: Graphics: CanvasWebGL, defect)

Description

[Kelsey Gilbert [:jgilbert]]

This appears like it should break, but clearly seems to be working. Let's find out why.

Comment 1

[Matt Woodrow (:mattwoodrow)]

Attachment: Pass the GLContext to WaitSync

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.

Comment 2

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

Comment 3

[Kelsey Gilbert [:jgilbert]]

Attachment: patch: Codify giving the factory a `nullptr` consGL, and set the consGL for each surface before WaitSync.

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.

Comment 4

[Kelsey Gilbert [:jgilbert]]

Backed out:
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=2defb982c769

Looks like a warnings-as-errors failure.
Also I got the bug number wrong. :C

Comment 5

[Kelsey Gilbert [:jgilbert]]

What's wrong with me. Still the wrong bug.

Comment 6

[Kelsey Gilbert [:jgilbert]]

Attachment: patch: null consGL means use glFinish. Setting consGL will make subsequent fencing faster (use fences)

Comment 7

[Matt Woodrow (:mattwoodrow)]

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.

Comment 8

[Kelsey Gilbert [:jgilbert]]

Attachment: patch: null consGL means use glFinish. Setting consGL will make subsequent fencing faster (use fences)

Fix and carry forward r+.

Comment 9

[Kelsey Gilbert [:jgilbert]]

Does this fix the race?

Comment 10

[Matt Woodrow (:mattwoodrow)]

Looks like it, yes.

Comment 11

[Kelsey Gilbert [:jgilbert]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6bf7f7e6e312

Comment 12

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/6bf7f7e6e312