If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

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

RESOLVED FIXED in mozilla24

Status

()

Core
Canvas: WebGL
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: jgilbert, Assigned: jgilbert)

Tracking

unspecified
mozilla24
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Assignee)

Description

4 years ago
This appears like it should break, but clearly seems to be working. Let's find out why.
Blocks: 756601
Blocks: 875232
Created attachment 754308 [details] [diff] [review]
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.
Attachment #754308 - Flags: feedback?(jgilbert)
(Assignee)

Comment 2

4 years ago
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-
(Assignee)

Comment 3

4 years ago
Created attachment 754931 [details] [diff] [review]
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.
Attachment #754931 - Flags: review?(matt.woodrow)
(Assignee)

Comment 4

4 years ago
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
(Assignee)

Comment 5

4 years ago
What's wrong with me. Still the wrong bug.
(Assignee)

Comment 6

4 years ago
Created attachment 757754 [details] [diff] [review]
patch: null consGL means use glFinish. Setting consGL will make subsequent fencing faster (use fences)
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+
(Assignee)

Comment 8

4 years ago
Created attachment 757764 [details] [diff] [review]
patch: null consGL means use glFinish. Setting consGL will make subsequent fencing faster (use fences)

Fix and carry forward r+.
Assignee: nobody → jgilbert
Attachment #757754 - Attachment is obsolete: true
Attachment #757764 - Flags: review+
(Assignee)

Comment 9

4 years ago
Does this fix the race?
Flags: needinfo?(matt.woodrow)
Looks like it, yes.
Flags: needinfo?(matt.woodrow)
(Assignee)

Comment 11

4 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/6bf7f7e6e312
https://hg.mozilla.org/mozilla-central/rev/6bf7f7e6e312
Status: NEW → RESOLVED
Last Resolved: 4 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.