Bug 80100 – [chromium] CCThreadProxy context lost support

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Nat Duca]

[chromium] CCThreadProxy context lost support

[Nat Duca]

Created attachment 129817 [details]
Patch

[Nat Duca]

I think this is mostly in a shape to get initial review feedback. James, seem sane?

[James Robinson]

Comment on attachment 129817 [details]
Patch

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

Not fully-brained wrapped but what I think I understand so far looks good

> Source/WebCore/platform/graphics/chromium/cc/CCSchedulerStateMachine.cpp:47
> +    bool shouldDraw = (m_needsRedraw && m_insideVSync && m_visible && canDraw && m_canDraw && m_contextState == CONTEXT_ACTIVE) || m_needsForcedRedraw;

this is getting hairy. maybe this should be a helper function with the states listed out as if()+returns, or just broken out into more intermediate bools?

would context loss collapse more naturally into canDraw?

> Source/WebCore/platform/graphics/chromium/cc/CCSchedulerStateMachine.cpp:192
> +    if (m_contextState == CONTEXT_LOST || m_contextState == CONTEXT_RECREATING)

how can we hit this call without transitioning first to CONTEXT_ACTIVE?

> Source/WebCore/platform/graphics/chromium/cc/CCThreadProxy.cpp:230
> +                                          AllowCrossThreadAccess(&completion),

indentation here seems to have gone a bit off. i think aligning with the first parameter of the innermost call would be appropriate

[James Robinson]

Comment on attachment 129817 [details]
Patch

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

Logic looks pretty sound near as I can tell.

> Source/WebCore/ChangeLog:8
> +        No new tests. (OOPS!)

remember to take care of this

[Nat Duca]

Created attachment 130312 [details]
Patch

[WebKit Review Bot]

Comment on attachment 130312 [details]
Patch

Attachment 130312 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11835259

New failing tests:
platform/chromium/compositing/lost-compositor-context.html
platform/chromium/compositing/webgl-loses-compositor-context.html
platform/chromium/compositing/lost-compositor-context-with-rendersurface.html
platform/chromium/compositing/lost-compositor-context-with-video.html

[Nat Duca]

Created attachment 130435 [details]
it works this time really

[Nat Duca]

Created attachment 130437 [details]
Integrate @kbrs didRecreate event delivery

[James Robinson]

Comment on attachment 130437 [details]
Integrate @kbrs didRecreate event delivery

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

> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:208
> +    virtual void onContextLost()
> +    {
> +        m_client->didLoseContext();

is it worth having an adapter just do to the rename onContextLost -> didLoseContext? can't we just pick one name and have the LayerRendererChromiumClient satisfy the ContextLostCallback interface?

> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:212
> +    ContextLostCallbackAdapter(LayerRendererChromiumClient* client)

explicit.

> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:213
> +            : m_client(client) { }

indentation off

> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.h:72
> +    // Where possible, redraws are scissored to a damage region calculated from changes to
> +    // layer properties. This function overrides the damage region for the next draw cycle.

i think this comment wandered off from its true home (it was associated with setFullRootLayerDamage(), but that's not on this client interface). probably can be deleted completely now

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.h:137
> +    enum RecreateResult {

i know the scheduler follows a different convention but everywhere else in WebKit enum values are named like normal variables - i.e. CamelCased, not MACRO_STYLE

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:415
> +    if (wasRecreate)
> +        sendContextLostAndRestoredNotificationRecursive(m_rootLayerImpl.get());

this could be moved up next to the close() call to avoid the temporary.

the function is somewhat misnamed (IMO). it's notifying the CCLayerImpl tree that the context they were using was lost.  it doesn't actually matter whether we have a new context or not as far as the CCLayerImpls go - they just need to know that any resources that they had are no longer valid - so the ordering w.r.t. restoring the next context doesn't matter much.

> Source/WebCore/platform/graphics/chromium/cc/CCSchedulerStateMachine.cpp:42
> +    , m_contextState(CONTEXT_ACTIVE) { }

nit: if you move the { } at the end of line out to the next two lines you'll get 1 less line of diff every time this changes, and less chance of merge conflicts

> Source/WebCore/platform/graphics/chromium/cc/CCThreadProxy.h:98
> +    class ContextRecreationTimer : public CCTimer, CCTimerClient {

sucks that we need all this boilerplate to use a CCTimer :/

since this is stored via an OwnPtr<> can we just forward declare the type in the header and hide the implementation in the .cpp?

> Source/WebCore/platform/graphics/chromium/cc/CCThreadProxy.h:107
> +        enum Recreation { RecreationTickRate = 30 };

this could probably use a type-tag. I had assumed it was seconds, and 30 seconds seemed like a pretty long time to wait until I checked CCTimer.h!

> Source/WebCore/platform/graphics/chromium/cc/CCThreadProxy.h:110
> +        ContextRecreationTimer(CCThreadProxy* proxy) : CCTimer(CCProxy::mainThread(), this), m_proxy(proxy) { }

explicit.

expand the initialization out to be one per line

[James Robinson]

Comment on attachment 130437 [details]
Integrate @kbrs didRecreate event delivery

Ooh and the bots look happy too. R=me, but consider the feedback.

[Nat Duca]

> > Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:208
> > +    virtual void onContextLost()
> > +    {
> > +        m_client->didLoseContext();
> 
> is it worth having an adapter just do to the rename onContextLost -> didLoseContext? can't we just pick one name and have the LayerRendererChromiumClient satisfy the ContextLostCallback interface?

Yeah, if the purpose of this was just to do that conversion, that'd be a fair point. But, the adapter is also keeping the GC3D-basedness of the LRC out of the header. That's more the win I see. I could make LRC implement GraphicsContext3D::ContextLostCallback if you wanted to avoid the adapter...

WDYT?

sendContextLostAndRestoredNotificationRecursive(m_rootLayerImpl.get());
> the function is somewhat misnamed (IMO).

I changed the naming to be more normative.

> sucks that we need all this boilerplate to use a CCTimer :/

Hugely. I've got in the back of my head using the machinery of CCTask for method callbacks.

[James Robinson]

Ah, good point about isolation.  If only interfaces worked like they did in Go this would all go away. I agree that's probably enough to do an adapter (although LRC itself could be 'the adapter' as you point out).  Whichever you prefer.

[Nat Duca]

Created attachment 130471 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 130471 [details]
Patch for landing

Clearing flags on attachment: 130471

Committed r110010: <http://trac.webkit.org/changeset/110010>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.