Bug 92932 – [chromium] Initialize impl-side SharedGraphicsContext after the LRC is initialized

• Status: RESOLVED
• Resolution: WONTFIX
• Component: Platform

[Nat Duca]

[chromium] Initialize impl-side SharedGraphicsContext after the LRC is initialized

[Nat Duca]

Created attachment 155947 [details]
Patch

[Nat Duca]

Hey Steven, is this kosher? I'd like to be able to get some state from the compositor graphics context to decide whether we'll even need the shared context. I can't do that when we initialize the shared context before the LRC init.

[Nat Duca]

Created attachment 155983 [details]
Patch

[Stephen White]

Comment on attachment 155983 [details]
Patch

Unless initializeContext() happens every frame, I don't think this will work, since needsSharedContext() checks needsFilterContext(), which is only set when LayerChromium::setFilters() is passed a non-empty filters list.  This might happen long after compositor context initialization.

You should probably try running the css3/filters layout tests with the threaded compositor enabled, at a minimum.  Or open chrome with the shared compositor, and go to any page without filters, then add them manually in the inspector and see if they show up.

[Nat Duca]

(In reply to comment #4)
> (From update of attachment 155983 [details])
> Unless initializeContext() happens every frame, I don't think this will work, since needsSharedContext() checks needsFilterContext(), which is only set when LayerChromium::setFilters() is passed a non-empty filters list.  This might happen long after compositor context initialization.

Is there a reason we do it this way, rather than making setFilters-first-call do the initialization?

Where its done now makes it very hard to do wkb.ug/92890

[Stephen White]

(In reply to comment #5)
> Is there a reason we do it this way, rather than making setFilters-first-call do the initialization?

Honestly, I can't remember (maybe threading issues?  is LayerChromium::setFilters() still on the main thread?).  James might remember this stuff better than me.  If you can make a direct call at LayerChromium::setFilters() time to create the context, then by all means go ahead and change it.  We do need to re-create the filter context on a lost context, though, so that still needs to work.  Currently we do that by bootstrapping off the compositor lost context machinery (since we assume that you won't lose one without losing the other).

It looks like this context is also now used for accelerated painting, but that should be safe to check anytime, since it just checks settings.

> Where its done now makes it very hard to do wkb.ug/92890

[Nat Duca]

(In reply to comment #6)
> Honestly, I can't remember (maybe threading issues?  is LayerChromium::setFilters() still on the main thread?).  James might remember this stuff better than me.  If you can make a direct call at LayerChromium::setFilters() time to create the context, then by all means go ahead and change it.

The restriction is that we need the context to be created before the compositor draws, right? Is there anything else in the compositor path that uses this outside of CCLayerTreeHostImpl::draw?

[Stephen White]

(In reply to comment #7)
> (In reply to comment #6)
> > Honestly, I can't remember (maybe threading issues?  is LayerChromium::setFilters() still on the main thread?).  James might remember this stuff better than me.  If you can make a direct call at LayerChromium::setFilters() time to create the context, then by all means go ahead and change it.
> 
> The restriction is that we need the context to be created before the compositor draws, right? Is there anything else in the compositor path that uses this outside of CCLayerTreeHostImpl::draw?

It looks like accelerated painting uses it now too, but that's new to me.  So you'll have to make sure that doesn't break.

The only other thing I remember from this particular maze was that getting the context created at the right time was really finnicky, e.g., checking for lost context can only be done on the impl thread, but the context can only be re-created on the main thread.  If LayerChromium::setFilters() is done on the main thread, then that part should be ok, maybe (modulo accelerated painting above).

And of course, no layout test bots run with threaded compositing enabled.  So there's no way to test this from a layout test.  (I suppose you could add threadedCompositing as a flag on the LayoutTestController, if you felt generous, and just turn it on for some filters tests to get some coverage).

[James Robinson]

LayerChromium::setFilters can be called before you know which CCLayerTreeHost the given LayerChromium is going to end up in, which might make things tricky for you as well.

[James Robinson]

Is forceSoftwareCompositing a per-LTH setting or global?  If it was global, I *think* you could do this in ::setFilters() (remember to also check that we're in threaded mode, but that is global)

[Nat Duca]

What I'm thinking I'll do is remove this conditional check on software mode, and make the creation of the context fail deeper in the context creation guts. Eg if you're in a software mode, you can't get 3D contexts back.

[Eric Seidel (no email)]

Comment on attachment 155983 [details]
Patch

Cleared review? from attachment 155983 [details] so that this bug does not appear in http://webkit.org/pending-review.  If you would like this patch reviewed, please attach it to a new bug (or re-open this bug before marking it for review again).