Bug 72688 – [chromium] Route willDraw/setNeedsRedraw to CCInputHandler and fix double-drawing issues that result

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

[Nat Duca]

[chromium] Route willDraw/setNeedsRedraw to CCInputHandler and fix double-drawing issues that result

[Nat Duca]

Created attachment 115737 [details]
Patch

[James Robinson]

Comment on attachment 115737 [details]
Patch

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

initial round of superficial comments. will have to think through actual logic when i'm slightly less tired

> Source/WebCore/ChangeLog:7
> +        perform requestAnimationFrame-style aniimations. The bulk of

animations

> Source/WebCore/platform/graphics/chromium/cc/CCInputHandler.h:35
> +// The CCInputHanlder is an way for the embedders to interact with

handler

is a way

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.h:55
> +class CCLayerTreeHostImpl : public CCInputHandlerTarget {

why Target instead of Client (or Delegate, which is less common in WebKit i believe)? dunno if we need yet another name for this pattern

> Source/WebCore/platform/graphics/chromium/cc/CCScheduler.h:89
> +    bool m_processingScheduledActions;

should this be #ifndef NDEBUG guarded? it's not used for logic other than ASSERTS

> Source/WebCore/platform/graphics/chromium/cc/CCThreadProxy.cpp:466
> +    m_inputHandlerOnImplThread->willDraw(WTF::monotonicallyIncreasingTime());

don't need WTF:: unless we shadow this function somewhere, it's in the global namespace (there's a using declaration in CurrentTime.h)

[James Robinson]

Comment on attachment 115737 [details]
Patch

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

I think the frame # logic can be simplified, although there may be a case i'm missing - see comment on CCScheduler::beginFrame()

Also found a second/millisecond confusion likely to make animations behave really weirdly. I kind of want to have separate types for these that aren't implicitly convertible so the compiler can catch these. Someday....

> Source/WebCore/platform/graphics/chromium/cc/CCFrameRateController.cpp:79
> +    m_numBeginFrameCalls++;

i'm not totally sure why this state (the frame count) lives here. What about having it be part of the CCSchedulerStateMachine and updated internally on vsync edges? I don't think anybody outside of the state machine needs to use this

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:287
> +    return WTF::monotonicallyIncreasingTime() * 1000.0;

no need for WTF:: here since this function is in the global namespace (unless we're shadowing it)

> Source/WebCore/platform/graphics/chromium/cc/CCScheduler.cpp:111
> +    m_stateMachine.setInsideVSync(frameNumber);
>      processScheduledActions();
> -    m_stateMachine.setInsideVSync(false);
> +    m_stateMachine.setOutsideVSync();

can we just move this block:

if (m_setNeedsRedrawAfterProcessingCommands)
  setNeedsRedraw()

to here? if it's after the vsync then it should trigger a draw on the next vsync and not this one without needing track frame #s

> Source/WebCore/platform/graphics/chromium/cc/CCSchedulerStateMachine.cpp:44
> +    bool canDraw = m_currentFrameNumber != m_lastFrameNumberWhereDrawWasCalled;

alternate proposal:

CCSchedulerStateMachine::setNeedsRedraw() can check m_insideVSync and if that is true set

> Source/WebCore/platform/graphics/chromium/cc/CCSchedulerStateMachine.h:111
> +    int m_currentFrameNumber;
>      bool m_needsRedraw;
>      bool m_needsForcedRedraw;
>      bool m_needsCommit;
>      bool m_updateMoreResourcesPending;
>      bool m_insideVSync;
> +    int m_lastFrameNumberWhereDrawWasCalled;

nit: the two new variables look like they should be buddies, can they live next to each other?

also similar types next to each other can help compilers pack the class more tightly (not that it's a big concern for this class, but a good general habit that most of WebKit follows)

>> Source/WebCore/platform/graphics/chromium/cc/CCThreadProxy.cpp:466
>> +    m_inputHandlerOnImplThread->willDraw(WTF::monotonicallyIncreasingTime());
> 
> don't need WTF:: unless we shadow this function somewhere, it's in the global namespace (there's a using declaration in CurrentTime.h)

This is in seconds, but the parameter name is suffixed with 'Ms'

[Nat Duca]

(In reply to comment #3)
> (From update of attachment 115737 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=115737&action=review
> 
> I think the frame # logic can be simplified, although there may be a case i'm missing - see comment on CCScheduler::beginFrame()

I agree that if we added some logic to the CCScheduler, we could avoid the frame numbering stuff.

However, the goal of the state machine is to contain the logic. The scheduler, in goal, should be mostly a passthrough. So I had considered the trick you suggested before, but it scared me in two ways:
1. The "falling edge" of the vsync incrementing a state value inside the state machine seemed scary to me.

An alternative way to fix reentrancy in the CCScheduler::processCommands is to updateState(action) before executing the scheduled action.

[Nat Duca]

Created attachment 115914 [details]
Patch

[Nat Duca]

I'm going to try triggering on falling edge of vsync. Stand by for another patch.

[Nat Duca]

Created attachment 116118 [details]
Patch

[James Robinson]

Just as a general comment I think frame numbers are probably easier for you to reason about since you have a lot of experience in code that uses them heavily, but since I don't have that background it can take a bit of extra effort to reason about them.  I think they can be really useful but solutions based on them might seem simpler to you than they do to me, at least at first glance.

[James Robinson]

Comment on attachment 116118 [details]
Patch

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

R=me

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:85
> +void CCLayerTreeHostImpl::animate(double frameBeginTime)

nit: frameBeginTimeMs please, to help out whoever ends up implementing this stub

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:291
> +    return WTF::monotonicallyIncreasingTime() * 1000.0;

nit: remove WTF:: prefix, this function's in the global namespace

> Source/WebKit/chromium/src/WebCompositorImpl.cpp:135
> +void WebCompositorImpl::willDraw(double frameBeginTime)

nit: frameBeginTimeMs please to make things clearer for whoever implements the stub

> Source/WebKit/chromium/tests/CCSchedulerStateMachineTest.cpp:223
> +            if (j == 1)
> +                state.didEnterVSync();

for symmetry, should this call didLeaveVSync() if j==0 ?

[Nat Duca]

Committed r100984: <http://trac.webkit.org/changeset/100984>