Bug 78139 – [chromium] Resolve GraphicsContext3D use and re-enable filters in the threaded compositor

• Component: Canvas
• Status: RESOLVED FIXED

[Stephen White]

Currently, we use the SharedGraphicsContext3D for accelerated 2D <canvas> and for accelerated filters in the compositor.  This doesn't work in the threaded compositor case, since the two threads will race.  We should resolve this, and re-enable filters in the threaded compositor.

Either:

1)  We should create a new GraphicsContext3D for filter use in the threaded case, owned by the compositor and used in the compositor thread.
2)  We should start using the SharedGraphicsContext3D solely in the compositor thread by deferring all canvas rendering to the compositor thread, and remove the CCProxy::hasImplThread() check to re-enable filters.

[Stephen White]

Created attachment 129464 [details]
Patch

[Stephen White]

Not ready for review; just testing via EWS.

[James Robinson]

One (more) thing to think about: we might lose this context and if we do need to reinitialize it. We haven't bothered setting up any lost context recover for the SharedGraphicsContext3D since it's used for canvas 2d which can't recover in any useful way from a lost context, but that's not really true for filters.

[Stephen White]

(In reply to comment #3)
> One (more) thing to think about: we might lose this context and if we do need to reinitialize it. We haven't bothered setting up any lost context recover for the SharedGraphicsContext3D since it's used for canvas 2d which can't recover in any useful way from a lost context, but that's not really true for filters.

Good guess!  That's actually what I'm working on now (and why I didn't request a review).  :)  It would allow us to use filters on Windows XP, for example.  My guess is that I just need to set up the callback, and free the context on loss, so it gets recreated on the next draw.  Not sure what'll happen if it gets lost mid-draw, but apparently the compositor doesn't handle that either.

[James Robinson]

Yeah, in the compositor we just kick a new frame if we lose something midway.

[Stephen White]

Created attachment 129698 [details]
Patch

[James Robinson]

Comment on attachment 129698 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=129698&action=review

> Source/WebCore/platform/graphics/chromium/cc/CCRenderSurfaceFilters.cpp:197
> +        m_context = GraphicsContext3D::create(attributes, 0);

Check with sievers@, but I'm not sure if you can construct a new offscreen context from a non-main thread or not. For the compositor we always construct the context on the main thread and initialize it on the impl thread, but that case is a bit difference since it's a view context and not offscreen.

[Stephen White]

Created attachment 132635 [details]
Patch

[Stephen White]

(In reply to comment #8)
> Created an attachment (id=132635) [details]
> Patch

This version works in Release in both the threaded and non-threaded case, but still spits some asserts in Debug, since it doesn't like my making the context current on both the main and impl threads.  That's going to require some further ugliness in the lost context handling (which is probably not quite right in this patch).

I'm also not sure if the clone() method is really the right way to go, since it's pretty verbose and error-prone, and potentially costly.  I'd prefer to just make the FilterOperation be ThreadSafeRefCounted, and just hand them off to the compositor thread without copying.  That might incur a penalty in the non-threaded case, but I'd prefer to profile it and see if it really is a problem in practice.

[Build Bot]

Comment on attachment 132635 [details]
Patch

Attachment 132635 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/11989341

[James Robinson]

Comment on attachment 132635 [details]
Patch

For the compositor context we always create on the main thread and then hold off on the first makeContextCurrent() until impl-side.  Is there any reason you can't do that here?  I.e. is there some special error handling you want to do if you create a context for filters but can't make it current on the impl thread?

[Stephen White]

(In reply to comment #11)
> (From update of attachment 132635 [details])
> For the compositor context we always create on the main thread and then hold off on the first makeContextCurrent() until impl-side.  Is there any reason you can't do that here?  I.e. is there some special error handling you want to do if you create a context for filters but can't make it current on the impl thread?

It's the lost context handling that's problematic:  you can't detect it until you're on the impl thread (since you can't make current on the main thread), but then you have to request the main thread to re-create it (since you can't do so on the impl thread).  This explains the Rube Goldbergian contortions that the compositor has to go through, which I'd prefer to avoid if possible.  I was hoping to simply check for lost context at setFilters() time, but I guess that's not possible.

I suppose I'll have to add yet another mode to SharedGraphicsContext3D:  create, but don't touch.  Then check for lost context at filter draw time and... I don't know?  Give up and set a flag for the next draw?  Yuck.

[James Robinson]

(In reply to comment #12)
> (In reply to comment #11)
> > (From update of attachment 132635 [details] [details])
> > For the compositor context we always create on the main thread and then hold off on the first makeContextCurrent() until impl-side.  Is there any reason you can't do that here?  I.e. is there some special error handling you want to do if you create a context for filters but can't make it current on the impl thread?
> 
> It's the lost context handling that's problematic:  you can't detect it until you're on the impl thread (since you can't make current on the main thread), but then you have to request the main thread to re-create it (since you can't do so on the impl thread).  This explains the Rube Goldbergian contortions that the compositor has to go through, which I'd prefer to avoid if possible.  I was hoping to simply check for lost context at setFilters() time, but I guess that's not possible.
> 
> I suppose I'll have to add yet another mode to SharedGraphicsContext3D:  create, but don't touch.  Then check for lost context at filter draw time and... I don't know?  Give up and set a flag for the next draw?  Yuck.

What else would you do?

In practice we're pretty much always going to lose all contexts at the same time, so I would check both the compositor and filters contexts for loss in the same places and recreate both if they are lost.

[Stephen White]

(In reply to comment #13)
> (In reply to comment #12)
> > (In reply to comment #11)
> > > (From update of attachment 132635 [details] [details] [details])
> > > For the compositor context we always create on the main thread and then hold off on the first makeContextCurrent() until impl-side.  Is there any reason you can't do that here?  I.e. is there some special error handling you want to do if you create a context for filters but can't make it current on the impl thread?
> > 
> > It's the lost context handling that's problematic:  you can't detect it until you're on the impl thread (since you can't make current on the main thread), but then you have to request the main thread to re-create it (since you can't do so on the impl thread).  This explains the Rube Goldbergian contortions that the compositor has to go through, which I'd prefer to avoid if possible.  I was hoping to simply check for lost context at setFilters() time, but I guess that's not possible.
> > 
> > I suppose I'll have to add yet another mode to SharedGraphicsContext3D:  create, but don't touch.  Then check for lost context at filter draw time and... I don't know?  Give up and set a flag for the next draw?  Yuck.
> 
> What else would you do?

Ideally, I'd like to check for lost context in LayerChromium::pushPropertiesTo(), so that we'd know we had a good context before starting draw.  Then we'd only have to worry about the case where we lose context in mid-draw, which would seem to be a lot more rare.

> In practice we're pretty much always going to lose all contexts at the same time, so I would check both the compositor and filters contexts for loss in the same places and recreate both if they are lost.

I could do that, but I may not have the information at that point to know if the filter context is necessary, so I'd probably have to create it unconditionally.

[James Robinson]

(In reply to comment #14)
> (In reply to comment #13)
> > (In reply to comment #12)
> > > (In reply to comment #11)
> > > > (From update of attachment 132635 [details] [details] [details] [details])
> > > > For the compositor context we always create on the main thread and then hold off on the first makeContextCurrent() until impl-side.  Is there any reason you can't do that here?  I.e. is there some special error handling you want to do if you create a context for filters but can't make it current on the impl thread?
> > > 
> > > It's the lost context handling that's problematic:  you can't detect it until you're on the impl thread (since you can't make current on the main thread), but then you have to request the main thread to re-create it (since you can't do so on the impl thread).  This explains the Rube Goldbergian contortions that the compositor has to go through, which I'd prefer to avoid if possible.  I was hoping to simply check for lost context at setFilters() time, but I guess that's not possible.
> > > 
> > > I suppose I'll have to add yet another mode to SharedGraphicsContext3D:  create, but don't touch.  Then check for lost context at filter draw time and... I don't know?  Give up and set a flag for the next draw?  Yuck.
> > 
> > What else would you do?
> 
> Ideally, I'd like to check for lost context in LayerChromium::pushPropertiesTo(), so that we'd know we had a good context before starting draw.  Then we'd only have to worry about the case where we lose context in mid-draw, which would seem to be a lot more rare.

Afraid you can't do that.  We don't ever check for lost context mis-draw anyway, we put up completely frames and then see if it's gone.

> 
> > In practice we're pretty much always going to lose all contexts at the same time, so I would check both the compositor and filters contexts for loss in the same places and recreate both if they are lost.
> 
> I could do that, but I may not have the information at that point to know if the filter context is necessary, so I'd probably have to create it unconditionally.

Put the filter context bit on the host - it shouldn't be a layer thing anyway.

[Stephen White]

(In reply to comment #15)
> (In reply to comment #14)
> > (In reply to comment #13)
> > > (In reply to comment #12)
> > > > (In reply to comment #11)
> > > > > (From update of attachment 132635 [details] [details] [details] [details] [details])
> > > > > For the compositor context we always create on the main thread and then hold off on the first makeContextCurrent() until impl-side.  Is there any reason you can't do that here?  I.e. is there some special error handling you want to do if you create a context for filters but can't make it current on the impl thread?
> > > > 
> > > > It's the lost context handling that's problematic:  you can't detect it until you're on the impl thread (since you can't make current on the main thread), but then you have to request the main thread to re-create it (since you can't do so on the impl thread).  This explains the Rube Goldbergian contortions that the compositor has to go through, which I'd prefer to avoid if possible.  I was hoping to simply check for lost context at setFilters() time, but I guess that's not possible.
> > > > 
> > > > I suppose I'll have to add yet another mode to SharedGraphicsContext3D:  create, but don't touch.  Then check for lost context at filter draw time and... I don't know?  Give up and set a flag for the next draw?  Yuck.
> > > 
> > > What else would you do?
> > 
> > Ideally, I'd like to check for lost context in LayerChromium::pushPropertiesTo(), so that we'd know we had a good context before starting draw.  Then we'd only have to worry about the case where we lose context in mid-draw, which would seem to be a lot more rare.
> 
> Afraid you can't do that.  We don't ever check for lost context mis-draw anyway, we put up completely frames and then see if it's gone.
> 
> > 
> > > In practice we're pretty much always going to lose all contexts at the same time, so I would check both the compositor and filters contexts for loss in the same places and recreate both if they are lost.
> > 
> > I could do that, but I may not have the information at that point to know if the filter context is necessary, so I'd probably have to create it unconditionally.
> 
> Put the filter context bit on the host - it shouldn't be a layer thing anyway.

OK, thinking this through.  All the main thread can do is unconditionally create the context if it's told to.  There seem to be three states:

- filters not needed
- filters needed (first time) -> set the flag, but only if the filter context is NULL
  - must be checked in main thread otherwise the filter context won't be created by the time we get to commit, at which point it's too late and this draw is a bust
- filters needed (subsequent time), context was lost (but non-NULL) -> set the flag
  - can only be checked on impl thread and the flag set for next draw, but this draw is a bust

The second case is not so bad (if we're agreed that it's the main thread that should check for a NULL filter context and set the flag).

The third case seems wrong.  Even if the context was lost between draws, we don't detect this case until we're in mid-commit, by which point it's too late to back out and this draw is a bust.  (Correct me if I'm wrong about this -- maybe commit time is still early enough to create the context?)

Anyway this all seems needlessly complex, and would all be much easier if we could either create the context on the impl thread, or check for context loss on the main thread.  I have no idea how much would break if I were to do either one, though.

[Dana Jansens]

(In reply to comment #16)
> (In reply to comment #15)
> > (In reply to comment #14)
> > > (In reply to comment #13)
> > > > (In reply to comment #12)
> > > > > (In reply to comment #11)
> > > > > > (From update of attachment 132635 [details] [details] [details] [details] [details] [details])
> > > > > > For the compositor context we always create on the main thread and then hold off on the first makeContextCurrent() until impl-side.  Is there any reason you can't do that here?  I.e. is there some special error handling you want to do if you create a context for filters but can't make it current on the impl thread?
> > > > > 
> > > > > It's the lost context handling that's problematic:  you can't detect it until you're on the impl thread (since you can't make current on the main thread), but then you have to request the main thread to re-create it (since you can't do so on the impl thread).  This explains the Rube Goldbergian contortions that the compositor has to go through, which I'd prefer to avoid if possible.  I was hoping to simply check for lost context at setFilters() time, but I guess that's not possible.
> > > > > 
> > > > > I suppose I'll have to add yet another mode to SharedGraphicsContext3D:  create, but don't touch.  Then check for lost context at filter draw time and... I don't know?  Give up and set a flag for the next draw?  Yuck.
> > > > 
> > > > What else would you do?
> > > 
> > > Ideally, I'd like to check for lost context in LayerChromium::pushPropertiesTo(), so that we'd know we had a good context before starting draw.  Then we'd only have to worry about the case where we lose context in mid-draw, which would seem to be a lot more rare.
> > 
> > Afraid you can't do that.  We don't ever check for lost context mis-draw anyway, we put up completely frames and then see if it's gone.
> > 
> > > 
> > > > In practice we're pretty much always going to lose all contexts at the same time, so I would check both the compositor and filters contexts for loss in the same places and recreate both if they are lost.
> > > 
> > > I could do that, but I may not have the information at that point to know if the filter context is necessary, so I'd probably have to create it unconditionally.
> > 
> > Put the filter context bit on the host - it shouldn't be a layer thing anyway.
> 
> OK, thinking this through.  All the main thread can do is unconditionally create the context if it's told to.  There seem to be three states:
> 
> - filters not needed
> - filters needed (first time) -> set the flag, but only if the filter context is NULL
>   - must be checked in main thread otherwise the filter context won't be created by the time we get to commit, at which point it's too late and this draw is a bust
> - filters needed (subsequent time), context was lost (but non-NULL) -> set the flag
>   - can only be checked on impl thread and the flag set for next draw, but this draw is a bust
> 
> The second case is not so bad (if we're agreed that it's the main thread that should check for a NULL filter context and set the flag).
> 
> The third case seems wrong.  Even if the context was lost between draws, we don't detect this case until we're in mid-commit, by which point it's too late to back out and this draw is a bust.  (Correct me if I'm wrong about this -- maybe commit time is still early enough to create the context?)

Layers are updated on the main thread. Can you stick this check in where that happens? (Somewhere around CCLayerTreeHost::paintMaskAndReplicaForRenderSurface in the callstack?)

> Anyway this all seems needlessly complex, and would all be much easier if we could either create the context on the impl thread, or check for context loss on the main thread.  I have no idea how much would break if I were to do either one, though.

[Stephen White]

(In reply to comment #17)
> (In reply to comment #16)
> > (In reply to comment #15)
> > > (In reply to comment #14)
> > > > (In reply to comment #13)
> > > > > (In reply to comment #12)
> > > > > > (In reply to comment #11)
> > > > > > > (From update of attachment 132635 [details] [details] [details] [details] [details] [details] [details])
> > > > > > > For the compositor context we always create on the main thread and then hold off on the first makeContextCurrent() until impl-side.  Is there any reason you can't do that here?  I.e. is there some special error handling you want to do if you create a context for filters but can't make it current on the impl thread?
> > > > > > 
> > > > > > It's the lost context handling that's problematic:  you can't detect it until you're on the impl thread (since you can't make current on the main thread), but then you have to request the main thread to re-create it (since you can't do so on the impl thread).  This explains the Rube Goldbergian contortions that the compositor has to go through, which I'd prefer to avoid if possible.  I was hoping to simply check for lost context at setFilters() time, but I guess that's not possible.
> > > > > > 
> > > > > > I suppose I'll have to add yet another mode to SharedGraphicsContext3D:  create, but don't touch.  Then check for lost context at filter draw time and... I don't know?  Give up and set a flag for the next draw?  Yuck.
> > > > > 
> > > > > What else would you do?
> > > > 
> > > > Ideally, I'd like to check for lost context in LayerChromium::pushPropertiesTo(), so that we'd know we had a good context before starting draw.  Then we'd only have to worry about the case where we lose context in mid-draw, which would seem to be a lot more rare.
> > > 
> > > Afraid you can't do that.  We don't ever check for lost context mis-draw anyway, we put up completely frames and then see if it's gone.
> > > 
> > > > 
> > > > > In practice we're pretty much always going to lose all contexts at the same time, so I would check both the compositor and filters contexts for loss in the same places and recreate both if they are lost.
> > > > 
> > > > I could do that, but I may not have the information at that point to know if the filter context is necessary, so I'd probably have to create it unconditionally.
> > > 
> > > Put the filter context bit on the host - it shouldn't be a layer thing anyway.
> > 
> > OK, thinking this through.  All the main thread can do is unconditionally create the context if it's told to.  There seem to be three states:
> > 
> > - filters not needed
> > - filters needed (first time) -> set the flag, but only if the filter context is NULL
> >   - must be checked in main thread otherwise the filter context won't be created by the time we get to commit, at which point it's too late and this draw is a bust
> > - filters needed (subsequent time), context was lost (but non-NULL) -> set the flag
> >   - can only be checked on impl thread and the flag set for next draw, but this draw is a bust
> > 
> > The second case is not so bad (if we're agreed that it's the main thread that should check for a NULL filter context and set the flag).
> > 
> > The third case seems wrong.  Even if the context was lost between draws, we don't detect this case until we're in mid-commit, by which point it's too late to back out and this draw is a bust.  (Correct me if I'm wrong about this -- maybe commit time is still early enough to create the context?)
> 
> Layers are updated on the main thread. Can you stick this check in where that happens? (Somewhere around CCLayerTreeHost::paintMaskAndReplicaForRenderSurface in the callstack?)

No, I can't check for lost context on the main thread, since this requires making the context current, which can only be done on the impl thread.

> > Anyway this all seems needlessly complex, and would all be much easier if we could either create the context on the impl thread, or check for context loss on the main thread.  I have no idea how much would break if I were to do either one, though.

[Dana Jansens]

(In reply to comment #18)
> > Layers are updated on the main thread. Can you stick this check in where that happens? (Somewhere around CCLayerTreeHost::paintMaskAndReplicaForRenderSurface in the callstack?)
> 
> No, I can't check for lost context on the main thread, since this requires making the context current, which can only be done on the impl thread.

Ah, sorry, I see. Well, the main thread is blocked during commit, but it will conflict with future interests for out-of-process compositor.

[James Robinson]

If you can figure out a way to create the context from any thread that would definitely be ideal. Talk to Al Patrick about how to get to the right data structures and if this is feasible. Otherwise, if it simplifies things I'm pretty sure we never have to worry about the filter context being lostcompositor context being alive. If we lose one we lose them all and the frame is a bust no matter what.

On a phone, forgive the typos.

[Stephen White]

(In reply to comment #20)
> If you can figure out a way to create the context from any thread that would definitely be ideal. Talk to Al Patrick about how to get to the right data structures and if this is feasible. Otherwise, if it simplifies things I'm pretty sure we never have to worry about the filter context being lostcompositor context being alive. If we lose one we lose them all and the frame is a bust no matter what.

OK, I'm recreating the filter context in CCThreadProxy::recreateContext().  That'll probably work.

But the initial creation is still problematic:  my plan was to create the filter context in CCThreadProxy::initializeContext(), but I can't set the "needsFilterContext" flag from LayerChromium::setFilters(), since the layer has not been initialized with a non-NULL CCLayerTreeHost yet.  I can't set the flag in LayerChromium::setLayerTreeHost(), since CCThreadProxy::initializeContext() has already been called and the context already created.

(You are in a maze of twisty little restrictions, all different).

I'm at the point now where I'm thinking about just creating the filter context unconditionally in the threaded case.

[Stephen White]

Created attachment 132893 [details]
Patch

[Stephen White]

(In reply to comment #22)
> Created an attachment (id=132893) [details]
> Patch

OK, this version does the unconditional creation in the threaded case.  It doesn't seem to cause any asserts to fire, and generally seems to do the right thing on GPU process death.

[Stephen White]

Created attachment 133074 [details]
Patch

[Stephen White]

(In reply to comment #24)
> Created an attachment (id=133074) [details]
> Patch

This version is the same as the previous patch, but uses ThreadSafeRefCounted instead of RefCounted for FilterOperation.  This obviates the need for the clone() methods.  I prefer this version, due to lack of intrusiveness and potential for error, but it does potentially introduce a slowdown when ref'ing FilterOperation pointers in the non-threaded case.

[James Robinson]

Comment on attachment 133074 [details]
Patch

Always making a second context in threaded mode is too heavy.  Filters are (currently) quite rare and disabled by default, and contexts aren't super cheap.

Can you make it only create a context if we've ever seen filters in the process?  You could do this in some main thread part of the commit flow (perhaps paint time) to make sure it always happens when add the first filter in the process, and then keep the bit around as a static so it's automatically captured on compositors going forward and restored after recovery.

[David Levin]

Comment on attachment 133074 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=133074&action=review

Just a drive by threading comment.

> Source/WebCore/platform/graphics/gpu/SharedGraphicsContext3D.cpp:50
>      }

A blank line after the closing } for a function is more typical.

> Source/WebCore/platform/graphics/gpu/SharedGraphicsContext3D.cpp:78
> +    DEFINE_STATIC_LOCAL(SharedGraphicsContext3DImpl, impl, ());

If this is only intended for use on one particular thread, it would be good to add an assert here to verify that.
If not, DEFINE_STATIC_LOCAL isn't safe. (Ditto for SharedGraphicsContext3D::get)

[Stephen White]

(In reply to comment #26)
> (From update of attachment 133074 [details])
> Always making a second context in threaded mode is too heavy.  Filters are (currently) quite rare and disabled by default, and contexts aren't super cheap.
> 
> Can you make it only create a context if we've ever seen filters in the process?  You could do this in some main thread part of the commit flow (perhaps paint time) to make sure it always happens when add the first filter in the process, and then keep the bit around as a static so it's automatically captured on compositors going forward and restored after recovery.

I tried that, but failed for the reasons in comment #21 above:  LayerChromium::setFilters() is too early, since the CCLayerTreeHost is still NULL, and pushPropertiesTo() or LayerChromium::setLayerTreeHost() are too late, since the compositor context has already been created.

[James Robinson]

What's wrong with creating the shared context after the compositor context?

[Stephen White]

(In reply to comment #27)
> (From update of attachment 133074 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=133074&action=review
> 
> Just a drive by threading comment.
> 
> > Source/WebCore/platform/graphics/gpu/SharedGraphicsContext3D.cpp:50
> >      }
> 
> A blank line after the closing } for a function is more typical.

Fixed, thanks.

> > Source/WebCore/platform/graphics/gpu/SharedGraphicsContext3D.cpp:78
> > +    DEFINE_STATIC_LOCAL(SharedGraphicsContext3DImpl, impl, ());
> 
> If this is only intended for use on one particular thread, it would be good to add an assert here to verify that.
> If not, DEFINE_STATIC_LOCAL isn't safe. (Ditto for SharedGraphicsContext3D::get)

createForImplThread() is only called from the main thread, so I could assert that, and getForImplThread() is only called from the impl thread, but they both end up calling the DEFINE_STATIC_LOCAL.  create is always called first, and only called from the main thread (while the impl thread is blocked), and get is only called from the impl thread (but create() will never modify it while the impl thread is running, since it's done while recreating the compositor context -- James can you verify my thinking here?).  Unfortunately, I don't think I can verify this behaviour completely with asserts.

SharedGraphicsContext3D::get() is only called from the main thread.  Could be asserted, but anything you do with the context subsequently will assert anyway if it's not on the main thread.

[Stephen White]

(In reply to comment #29)
> What's wrong with creating the shared context after the compositor context?

Then we're back to comment #16:  the only thing we can do on the main thread is create the context unconditionally, so lost context must be detected on the compositor thread, then kicked back to the main thread for creation.  That's why I was hoping to bootstrap off of the compositor's context re-creation machinery, which is what I thought you were suggesting in comment #13.

I think I've proven that there's no way to detect that we'll need the filter context by the time the compositor context is created:  LayerChromium::setFilters() is too early (no compositor, no way to check for lost context) and pushPropertiesTo() is too late (we're on the impl thread, can't create a context).

Is there another call in LayerChromium, still on the main thread, that I can use?    If not, could we plumb one through somehow?

[James Robinson]

(In reply to comment #31)
> (In reply to comment #29)
> > What's wrong with creating the shared context after the compositor context?
> 
> Then we're back to comment #16:  the only thing we can do on the main thread is create the context unconditionally, so lost context must be detected on the compositor thread, then kicked back to the main thread for creation.  That's why I was hoping to bootstrap off of the compositor's context re-creation machinery, which is what I thought you were suggesting in comment #13.

Sure, but nothing about that implies that you have to unconditionally create a filter context.  The states are:

(A) Never seen a filter, do not need a filter context
(B) Have seen a filter, need to create a filter context
(C) Have a filter context, lost it

Transitions from A->B should set a global bit somewhere indicating that we'll need a filter context and create one.  Then whenever we go through the compositor context recovery process we can check the global bit and create a filter context as well.

> 
> I think I've proven that there's no way to detect that we'll need the filter context by the time the compositor context is created:  LayerChromium::setFilters() is too early (no compositor, no way to check for lost context) and pushPropertiesTo() is too late (we're on the impl thread, can't create a context).
> 
> Is there another call in LayerChromium, still on the main thread, that I can use?    If not, could we plumb one through somehow?

I think trying to do anything on a LayerChromium instances is only going to lead to problems.  "Need a filter context" is a bit of global state, not a per-layer state, so individual LayerChromiums shouldn't be involved with the process at all beyond setting a bit indicating "somebody needs filters".  Context recover is driven through the CCProxys and CCLayerTreeHost(Impl)s.

[Stephen White]

(In reply to comment #32)
> (In reply to comment #31)
> > (In reply to comment #29)
> > > What's wrong with creating the shared context after the compositor context?
> > 
> > Then we're back to comment #16:  the only thing we can do on the main thread is create the context unconditionally, so lost context must be detected on the compositor thread, then kicked back to the main thread for creation.  That's why I was hoping to bootstrap off of the compositor's context re-creation machinery, which is what I thought you were suggesting in comment #13.
> 
> Sure, but nothing about that implies that you have to unconditionally create a filter context.  The states are:
> 
> (A) Never seen a filter, do not need a filter context
> (B) Have seen a filter, need to create a filter context
> (C) Have a filter context, lost it
> 
> Transitions from A->B should set a global bit somewhere indicating that we'll need a filter context and create one.  Then whenever we go through the compositor context recovery process we can check the global bit and create a filter context as well.
> 
> > 
> > I think I've proven that there's no way to detect that we'll need the filter context by the time the compositor context is created:  LayerChromium::setFilters() is too early (no compositor, no way to check for lost context) and pushPropertiesTo() is too late (we're on the impl thread, can't create a context).
> > 
> > Is there another call in LayerChromium, still on the main thread, that I can use?    If not, could we plumb one through somehow?
> 
> I think trying to do anything on a LayerChromium instances is only going to lead to problems.  "Need a filter context" is a bit of global state, not a per-layer state, so individual LayerChromiums shouldn't be involved with the process at all beyond setting a bit indicating "somebody needs filters".  Context recover is driven through the CCProxys and CCLayerTreeHost(Impl)s.

Good point!

BTW, I wasn't trying to do anything on LayerChromium instances (beyond detecting filters).  I was just trying to get that info to the CCLayerTreeHost or the LayerRendererChromium in time.  But you're right, as long as we're using a global for the context, we can use a global for the "need a filter context" bit, and the CCLTH can just pick it up from there.

That said, after David's comments, I'm starting to feel that using a global for filter contexts is a bit fragile, thread-wise.  I'm pretty sure it's correct, but "pretty sure" doesn't fill me with confidence for threading stuff.  Since filters don't have to move between pages the way canvases do, maybe the SharedGraphicsContext3D model is the wrong one, and we should be pursuing a filter context per-CCLTH.

[Nat Duca]

Can I ask a stupid question?

Why the heck aren't we implementing filters directly in CC?

This dependency back to Ganesh for filtering is really wrong feeling to me.

[Stephen White]

(In reply to comment #34)
> Can I ask a stupid question?
> 
> Why the heck aren't we implementing filters directly in CC?

It's a fair question.

One reason is, we're not just calling this code from the compositor.  It's also being called from the generic code in the SVG filter implementation.  Take a look at platform/graphics/filters/skia if you want to see what I mean:  the nodes there are implemented in terms of the same Skia primitives, and are backend-agnostic (hardware or software).  This would be more tricky to do if we had to call out the compositor for the hardware side, and would still require us to maintain the software path.  Rather than simple chains of filters like the CSS shortcuts, they are full-fledged image processing networks, which would require a lot more infrastructure to be duplicated.

The other reason is, Skia already had functionality I needed:  color matrices, the Gaussian blur algorithm I had implemented for soft shadows, etc, as well as infrastructure such as the shader builder.  It seemed silly to reimplement the same algorithms in two places.  It's also a smaller module that's easier to build standalone and test in.

Another option I considered was actually starting a third project altogether.  If you look at it in terms of Apple layers, CC == CoreAnimation, Skia == CoreGraphics, and ??? == CoreImage.  Currently we're doing it inside Skia, but it could also be its own project, although it would lose the code-sharing mentioned above.

[Stephen White]

Nope, a global doesn't work.  printf debugging says:

CCThreadProxy()::initializeContext(), needsFilterContext false
LayerChromium::setFilters setting needsFilterContext true

Comes too late, since the context is already created.  I can't force context recreation in LayerChromium::setFilters(), since that would happen on every draw.

Sorry if I'm being overly dense; maybe there's a way to detect filters earlier.

[Stephen White]

Created attachment 133870 [details]
Patch

[Stephen White]

(In reply to comment #37)
> Created an attachment (id=133870) [details]
> Patch

This version handles the first-use case explicitly in beginFrame() (thanks to Dana for the idea).  At that point, we've seen filters, and set the flag.  We only create it if the flag is set and the filter context is NULL.  This should be safe, since if the context is NULL, the impl thread will have never used it (and the main thread is the only one which writes it).

[Dana Jansens]

Comment on attachment 133870 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=133870&action=review

> Source/WebCore/platform/graphics/chromium/cc/CCThreadProxy.cpp:174
> +    if (CCLayerTreeHost::needsFilterContext() && !SharedGraphicsContext3D::getForImplThread())
> +        if (!SharedGraphicsContext3D::createForImplThread())
> +            return false;

What does this do that beginFrame() won't?

> Source/WebCore/platform/graphics/chromium/cc/CCThreadProxy.cpp:434
> +    if (CCLayerTreeHost::needsFilterContext() && !SharedGraphicsContext3D::getForImplThread())

If recreation failed and its null, should this be trying to create again here?

[Stephen White]

Comment on attachment 133870 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=133870&action=review

>> Source/WebCore/platform/graphics/chromium/cc/CCThreadProxy.cpp:174
>> +            return false;
> 
> What does this do that beginFrame() won't?

Probably nothing; I was just hoping there was a way we could make it work here at a future time.  I'll remove it.

>> Source/WebCore/platform/graphics/chromium/cc/CCThreadProxy.cpp:434
>> +    if (CCLayerTreeHost::needsFilterContext() && !SharedGraphicsContext3D::getForImplThread())
> 
> If recreation failed and its null, should this be trying to create again here?

If recreation failed, recreateContext() early-returns false, which I think means we abort and won't get this far, but someone more savvy in this code should confirm that.

[Stephen White]

Created attachment 133881 [details]
Patch

[James Robinson]

Comment on attachment 133881 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=133881&action=review

The context management logic looks OK. I'm less sure about FilterOperations - can we look at that separately?

Left comments, mostly about naming/etc

> Source/WebCore/platform/graphics/chromium/LayerChromium.cpp:321
> +    CCLayerTreeHost::setNeedsFilterContext(true);

this could be parameterless, since it's only used for ->true transitions

> Source/WebCore/platform/graphics/chromium/cc/CCThreadProxy.cpp:430
> +    if (CCLayerTreeHost::needsFilterContext() && !SharedGraphicsContext3D::getForImplThread())

I think here an explicit "haveForImplThread()" might make this interface easier to understand / safer. with that, you could ASSERT() that the thread is correct in the getForImplThread() getter

> Source/WebCore/platform/graphics/chromium/cc/CCThreadProxy.cpp:431
> +        SharedGraphicsContext3D::createForImplThread();

if this fails for some reason (say out of some resource on the GPU process side) we'll silently stop rendering filters and keep attempting to create this context on every frame. is that what you want? I'm not sure if there is a valid alternative.

> Source/WebCore/platform/graphics/filters/FilterOperation.h:47
> +class FilterOperation : public ThreadSafeRefCounted<FilterOperation> {

This I feel less comfortable with - this might be OK for the FilterOps we support today, but it's not very future-proof.  For example, type CUSTOM is definitely not safe to destroy from a non-main thread and while we don't set ENABLE(CSS_SHADERS) today and may not for the immediate future, it's a subtle landmine to leave. It also looks like the CA Code copies data out of the filter ops, so if someone makes a change that makes RefCounting across threads break it'll break in chromium but not mac.

I'd prefer to change this in a different patch and/or take a more serious look at serializing the filter ops in our own code before crossing any thread boundaries.

> Source/WebCore/platform/graphics/gpu/SharedGraphicsContext3D.cpp:78
> +static PassRefPtr<GraphicsContext3D> getContextForImplThread(bool create)

even though it's just a static i think a two-state enum would be better than a bool

> Source/WebCore/platform/graphics/gpu/SharedGraphicsContext3D.cpp:86
> +    return getContextForImplThread(false);

if you had an explicit have..() call, you could add an ASSERT() here that this is only called on the impl thread

> Source/WebCore/platform/graphics/gpu/SharedGraphicsContext3D.cpp:91
> +    return getContextForImplThread(true);

can you add an ASSERT() that this is only called on the main thread?

> Source/WebCore/platform/graphics/gpu/SharedGraphicsContext3D.h:46
> +    static PassRefPtr<GraphicsContext3D> createForImplThread();

I think this would be better if it returned a bool indicating whether the creation is successful or not, since that's all the caller needs (if they want to use the context they can get() it).

[Stephen White]

Comment on attachment 133881 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=133881&action=review

>> Source/WebCore/platform/graphics/chromium/LayerChromium.cpp:321
>> +    CCLayerTreeHost::setNeedsFilterContext(true);
> 
> this could be parameterless, since it's only used for ->true transitions

Done.

>> Source/WebCore/platform/graphics/chromium/cc/CCThreadProxy.cpp:430
>> +    if (CCLayerTreeHost::needsFilterContext() && !SharedGraphicsContext3D::getForImplThread())
> 
> I think here an explicit "haveForImplThread()" might make this interface easier to understand / safer. with that, you could ASSERT() that the thread is correct in the getForImplThread() getter

Done.

>> Source/WebCore/platform/graphics/chromium/cc/CCThreadProxy.cpp:431
>> +        SharedGraphicsContext3D::createForImplThread();
> 
> if this fails for some reason (say out of some resource on the GPU process side) we'll silently stop rendering filters and keep attempting to create this context on every frame. is that what you want? I'm not sure if there is a valid alternative.

Yeah, I don't think there's much we can do here in the short term.

>> Source/WebCore/platform/graphics/filters/FilterOperation.h:47
>> +class FilterOperation : public ThreadSafeRefCounted<FilterOperation> {
> 
> This I feel less comfortable with - this might be OK for the FilterOps we support today, but it's not very future-proof.  For example, type CUSTOM is definitely not safe to destroy from a non-main thread and while we don't set ENABLE(CSS_SHADERS) today and may not for the immediate future, it's a subtle landmine to leave. It also looks like the CA Code copies data out of the filter ops, so if someone makes a change that makes RefCounting across threads break it'll break in chromium but not mac.
> 
> I'd prefer to change this in a different patch and/or take a more serious look at serializing the filter ops in our own code before crossing any thread boundaries.

OK, I'll (reluctantly) bring back the cloning from patch #3.

>> Source/WebCore/platform/graphics/gpu/SharedGraphicsContext3D.cpp:78
>> +static PassRefPtr<GraphicsContext3D> getContextForImplThread(bool create)
> 
> even though it's just a static i think a two-state enum would be better than a bool

Done.

>> Source/WebCore/platform/graphics/gpu/SharedGraphicsContext3D.cpp:86
>> +    return getContextForImplThread(false);
> 
> if you had an explicit have..() call, you could add an ASSERT() here that this is only called on the impl thread

Done.

>> Source/WebCore/platform/graphics/gpu/SharedGraphicsContext3D.cpp:91
>> +    return getContextForImplThread(true);
> 
> can you add an ASSERT() that this is only called on the main thread?

Done.

>> Source/WebCore/platform/graphics/gpu/SharedGraphicsContext3D.h:46
>> +    static PassRefPtr<GraphicsContext3D> createForImplThread();
> 
> I think this would be better if it returned a bool indicating whether the creation is successful or not, since that's all the caller needs (if they want to use the context they can get() it).

Done.

[Stephen White]

Created attachment 133935 [details]
Patch

[James Robinson]

Comment on attachment 133935 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=133935&action=review

I think the reference filter clone is wrong. Rest looks fine.

> Source/WebCore/platform/graphics/chromium/LayerChromium.cpp:466
> +        for (unsigned i = 0; i < m_filters.size(); ++i)

size_t would be slightly better (more to be warning-clean than to handle >4B filters)

> Source/WebCore/platform/graphics/filters/CustomFilterOperation.h:67
> +        ASSERT(false);

ASSERT_NOT_REACHED(). a comment explaining this condition wouldn't hurt.

would it be better to filer these node types out in our compositor code? we don't have this one #ifdef'd in for the chromium build at least, but people may add other filter types that we can't clone naively in the future that aren't #ifdef guarded

> Source/WebCore/platform/graphics/filters/FilterOperation.h:157
> +        return adoptRef(new ReferenceFilterOperation(m_reference, m_type));

are you sure this is safe? it's copying an AtomicString, which is typically a fairly thread-hostile thing. For instance the common atoms (like nullAtom, emptyAtom, etc) are stored in TLS and only valid to access from the main thread. things like "is this string the empty string?" will fail from non-main threads.

do we have any thread-side support for reference filters? should this be filtered out on the compositor side, perhaps?

> Source/WebCore/platform/graphics/gpu/SharedGraphicsContext3D.cpp:80
> +    GET, CREATE

WebKit style for enums is to use normal TypeNameStyle, not MACRO_STYLE

[Stephen White]

Comment on attachment 133935 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=133935&action=review

>> Source/WebCore/platform/graphics/chromium/LayerChromium.cpp:466
>> +        for (unsigned i = 0; i < m_filters.size(); ++i)
> 
> size_t would be slightly better (more to be warning-clean than to handle >4B filters)

Done.

>> Source/WebCore/platform/graphics/filters/CustomFilterOperation.h:67
>> +        ASSERT(false);
> 
> ASSERT_NOT_REACHED(). a comment explaining this condition wouldn't hurt.
> 
> would it be better to filer these node types out in our compositor code? we don't have this one #ifdef'd in for the chromium build at least, but people may add other filter types that we can't clone naively in the future that aren't #ifdef guarded

Done:  non-cloneable nodes return 0, and LayerChromium filters them out.

>> Source/WebCore/platform/graphics/filters/FilterOperation.h:157
>> +        return adoptRef(new ReferenceFilterOperation(m_reference, m_type));
> 
> are you sure this is safe? it's copying an AtomicString, which is typically a fairly thread-hostile thing. For instance the common atoms (like nullAtom, emptyAtom, etc) are stored in TLS and only valid to access from the main thread. things like "is this string the empty string?" will fail from non-main threads.
> 
> do we have any thread-side support for reference filters? should this be filtered out on the compositor side, perhaps?

Good catch; I didn't know that about the Atomic types.

We have no support for reference filters on thread-side (or anywhere, AFAIK).  Changed to return 0, so LayerChromium will filter it out.

>> Source/WebCore/platform/graphics/gpu/SharedGraphicsContext3D.cpp:80
>> +    GET, CREATE
> 
> WebKit style for enums is to use normal TypeNameStyle, not MACRO_STYLE

Fixed.

[Stephen White]

Created attachment 133949 [details]
Patch

[James Robinson]

Comment on attachment 133949 [details]
Patch

R=me, thanks for your patience

[Stephen White]

Committed r112187: <http://trac.webkit.org/changeset/112187>

[Stephen White]

Comment on attachment 133949 [details]
Patch

Clearing flags ('cos webkit-patch didn't).

[Zoltan Herczeg]

Why this include dependency is added SharedGraphicsContext3D: cc/CCProxy.h

[Zoltan Herczeg]

Can we lift this dependency? It seems only some asserts use it. And why do you add platform specific code to shared code?