Bug 72688 - [chromium] Route willDraw/setNeedsRedraw to CCInputHandler and fix double-drawing issues that result
Summary: [chromium] Route willDraw/setNeedsRedraw to CCInputHandler and fix double-dra...
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: 2011-11-17 21:51 PST by Nat Duca
Modified: 2011-11-22 01:34 PST (History)
3 users (show)

See Also:


Attachments
Patch (39.97 KB, patch)
2011-11-17 21:53 PST, Nat Duca
no flags Details | Formatted Diff | Diff
Patch (40.77 KB, patch)
2011-11-18 17:20 PST, Nat Duca
no flags Details | Formatted Diff | Diff
Patch (39.47 KB, patch)
2011-11-21 12:54 PST, Nat Duca
jamesr: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Nat Duca 2011-11-17 21:51:00 PST
[chromium] Route willDraw/setNeedsRedraw to CCInputHandler and fix double-drawing issues that result
Comment 1 Nat Duca 2011-11-17 21:53:58 PST
Created attachment 115737 [details]
Patch
Comment 2 James Robinson 2011-11-17 22:53:14 PST
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)
Comment 3 James Robinson 2011-11-18 15:50:55 PST
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'
Comment 4 Nat Duca 2011-11-18 16:45:31 PST
(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.
Comment 5 Nat Duca 2011-11-18 17:20:13 PST
Created attachment 115914 [details]
Patch
Comment 6 Nat Duca 2011-11-21 11:33:05 PST
I'm going to try triggering on falling edge of vsync. Stand by for another patch.
Comment 7 Nat Duca 2011-11-21 12:54:27 PST
Created attachment 116118 [details]
Patch
Comment 8 James Robinson 2011-11-21 12:56:47 PST
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.
Comment 9 James Robinson 2011-11-21 15:55:56 PST
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 ?
Comment 10 Nat Duca 2011-11-22 01:34:18 PST
Committed r100984: <http://trac.webkit.org/changeset/100984>