Bug 80100 - [chromium] CCThreadProxy context lost support
Summary: [chromium] CCThreadProxy context lost support
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nat Duca
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-03-01 22:17 PST by Nat Duca
Modified: 2012-03-06 20:58 PST (History)
5 users (show)

See Also:


Attachments
Patch (23.60 KB, patch)
2012-03-01 22:17 PST, Nat Duca
no flags Details | Formatted Diff | Diff
Patch (48.84 KB, patch)
2012-03-06 00:12 PST, Nat Duca
no flags Details | Formatted Diff | Diff
it works this time really (48.67 KB, patch)
2012-03-06 13:43 PST, Nat Duca
no flags Details | Formatted Diff | Diff
Integrate @kbrs didRecreate event delivery (51.71 KB, patch)
2012-03-06 13:58 PST, Nat Duca
no flags Details | Formatted Diff | Diff
Patch for landing (56.85 KB, patch)
2012-03-06 16:40 PST, Nat Duca
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Nat Duca 2012-03-01 22:17:08 PST
[chromium] CCThreadProxy context lost support
Comment 1 Nat Duca 2012-03-01 22:17:40 PST
Created attachment 129817 [details]
Patch
Comment 2 Nat Duca 2012-03-01 22:18:47 PST
I think this is mostly in a shape to get initial review feedback. James, seem sane?
Comment 3 James Robinson 2012-03-02 12:20:54 PST
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
Comment 4 James Robinson 2012-03-02 14:59:16 PST
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
Comment 5 Nat Duca 2012-03-06 00:12:30 PST
Created attachment 130312 [details]
Patch
Comment 6 WebKit Review Bot 2012-03-06 01:35:58 PST
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
Comment 7 Nat Duca 2012-03-06 13:43:46 PST
Created attachment 130435 [details]
it works this time really
Comment 8 Nat Duca 2012-03-06 13:58:41 PST
Created attachment 130437 [details]
Integrate @kbrs didRecreate event delivery
Comment 9 James Robinson 2012-03-06 15:16:00 PST
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
Comment 10 James Robinson 2012-03-06 15:47:25 PST
Comment on attachment 130437 [details]
Integrate @kbrs didRecreate event delivery

Ooh and the bots look happy too. R=me, but consider the feedback.
Comment 11 Nat Duca 2012-03-06 15:51:24 PST
> > 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.
Comment 12 James Robinson 2012-03-06 15:53:11 PST
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.
Comment 13 Nat Duca 2012-03-06 16:40:26 PST
Created attachment 130471 [details]
Patch for landing
Comment 14 WebKit Review Bot 2012-03-06 20:58:29 PST
Comment on attachment 130471 [details]
Patch for landing

Clearing flags on attachment: 130471

Committed r110010: <http://trac.webkit.org/changeset/110010>
Comment 15 WebKit Review Bot 2012-03-06 20:58:34 PST
All reviewed patches have been landed.  Closing bug.