Bug 71920 – [chromium] CCThreadProxy::finishAllRendering hangs if !visible

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

[Iain Merrick]

[chromium] CCThreadProxy::finishAllRendering hangs if !visible

[Iain Merrick]

Created attachment 114286 [details]
Patch

[Iain Merrick]

Still needs a unit test or layout test, so not ready for review.

Would still appreciate feedback on whether there's a cleaner fix.

[Nat Duca]

Comment on attachment 114286 [details]
Patch

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

This means that there's a hang in compositeAndReadback as well. That's the harder one to fix... what about making the scheduler have a setRedrawRequired to fix both issues?

> Source/WebCore/platform/graphics/chromium/cc/CCThreadProxy.cpp:315
> +        if (!m_schedulerOnImplThread->redrawPending() || !m_schedulerOnImplThread->visible()) {

Why null-check the completion event here if you just assigned to it?

[Iain Merrick]

Comment on attachment 114286 [details]
Patch

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

>> Source/WebCore/platform/graphics/chromium/cc/CCThreadProxy.cpp:315
>> +        if (!m_schedulerOnImplThread->redrawPending() || !m_schedulerOnImplThread->visible()) {
> 
> Why null-check the completion event here if you just assigned to it?

Because setNeedsRedraw() will prod the state machine, and (I think) that can run the callback and clear out the completion event.

At first I thought the bug was just that lines 310 and 311 were in the wrong order.

[Nat Duca]

> Because setNeedsRedraw() will prod the state machine, and (I think) that can run the callback and clear out the completion event.

Great point. You migth want to check the other cases where I stash a completion event away for a similar mistake... compositeAndReadback does this too, I think.

Any thoughts on the overall switching approach?

[Iain Merrick]

Summarising IM discussion: let's add a method to CCScheduler to force a redraw, even if the compositor isn't visible. Test that via the existing CCSchedulerStateMachineTest.

I like it, I'll update this patch along those lines.

[Iain Merrick]

Created attachment 114311 [details]
Added CCScheduler::forceRedraw()

[Iain Merrick]

Cheekily copied existing test.

Now I'm wondering whether we need to wait for vsync in this scenario. Could there be a case where we're hidden, and stop getting vsyncs, but still want to do a final draw?

[James Robinson]

Comment on attachment 114311 [details]
Added CCScheduler::forceRedraw()

For the readback case waiting for vsync doesn't seem to make much sense - we should just draw as soon as we get the request. I can't think of any other cases where we would want to draw when !visible, so I'd say that you really should have forceRedraw ignore vsync completely.  Nat?

[Nat Duca]

Comment on attachment 114311 [details]
Added CCScheduler::forceRedraw()

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

s/forceRedraw/setNeedsForcedRedraw/ to be in keeping with naming in the class, and also not imply that it will happen immediately.

I'm fine waiting for vsync. As written, its cleaner this way and this is far from a fast path.

> Source/WebCore/platform/graphics/chromium/cc/CCScheduler.h:67
> +    // As setNeedsRedraw(), but ensures the draw will definitely happen even if we are not visible.

s/definitely //? :) Though, otoh, I like how emphatic it is! :)

> Source/WebKit/chromium/tests/CCSchedulerStateMachineTest.cpp:169
> +// As TestNextActionDrawsOnVSync, but with setVisible(false) and forceRedraw().

Fold this test into TestNextActionDrawsOnVsync but add an inner loop to each that considers both forced and non-forced draws? Less test-maintenance burden that way?

[Iain Merrick]

Created attachment 114481 [details]
Renamed to setNeedsForcedRedraw, cleaned up test

[James Robinson]

Comment on attachment 114481 [details]
Renamed to setNeedsForcedRedraw, cleaned up test

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

I don't see any tests that verify that if visible = false and setNeedsForcedRedraw = true then we actually draw, which seems like the most valuable thing to test since that's the new functionality for this patch.  Other than the tests, though, this looks great.

> Source/WebKit/chromium/tests/CCSchedulerStateMachineTest.cpp:129
> +        // We should get exactly the same results in three scenarios:
> +        //  0. visible=true, setNeedsRedraw=true
> +        //  1. visible=true, setNeedsForcedRedraw=true
> +        //  2. visible=false, setNeedsForcedRedraw=true
> +        // The final case is covered by TestNoCommitStatesRedrawWhenInvisible.
> +        for (size_t j = 0; j < 3; ++j) {
> +            state.setNeedsRedraw(true);
> +            state.setNeedsForcedRedraw(j >= 1);
> +            state.setVisible(j <= 1);

I like trying to hit all the states but this is pretty hard to read.  A better way might be to have an array of structs each of which has the desired value for visible/setNeedsRedraw/setNeedsForcedRedraw and then iterate through that array.

[James Robinson]

Actually no - the tests do cover that state.

I have a minor cleanup to the tests then I think this is good to go.

[James Robinson]

Created attachment 114571 [details]
Patch

[James Robinson]

Created attachment 114588 [details]
makes layout tests not time out

[Nat Duca]

Comment on attachment 114588 [details]
makes layout tests not time out

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

> Source/WebCore/platform/graphics/chromium/cc/CCScheduler.cpp:74
> +    m_stateMachine.setInsideVSync(true);

Ew ew ew. We're blatantly NOT inside vsync. Can we keep this entirely inside the state machine? The scheduler is supposed to be largely a passthrough...

> Source/WebKit/chromium/tests/CCSchedulerStateMachineTest.cpp:122
> +

Hot.

[James Robinson]

Created attachment 114743 [details]
Patch

[James Robinson]

(In reply to comment #16)
> (From update of attachment 114588 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=114588&action=review
> 
> > Source/WebCore/platform/graphics/chromium/cc/CCScheduler.cpp:74
> > +    m_stateMachine.setInsideVSync(true);
> 
> Ew ew ew. We're blatantly NOT inside vsync. Can we keep this entirely inside the state machine? The scheduler is supposed to be largely a passthrough...
> 

Agree, I made forced redraw orthogonal to vsync and visibility and moved all the logic into the state machine in the most recent patch. PTAL.

[Nat Duca]

Comment on attachment 114743 [details]
Patch

lgtm, nice factoring.

[WebKit Review Bot]

Comment on attachment 114743 [details]
Patch

Clearing flags on attachment: 114743

Committed r100054: <http://trac.webkit.org/changeset/100054>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.