Use message passing for BeginFrameAndCommitState and clean up forced commit logic

Use message passing for BeginFrameAndCommitState and clean up forced commit logic

This passes the BeginFrameAndCommitState struct along with messages instead of
relying on (racy) shared memory and slightly simplifies the forced commit logic.
This logic is for compositeAndReadback() and is trying to go through the normal
commit flow while the main thread is blocked. The normal commit flow is this:

1.) Main thread posts task to compositor thread requesting commit
2.) Compositor thread receives notification, after some delay posts task to main
to run the beginFrame logic
3.) Main thread runs the beginFrameTask, posts a blocking task to compositor
thread
  4.) With the main thread is blocked, compositor thread runs the beginFrameComplete
  task and signals the main thread to get on with life
5.) Compositor thread waits for an opportunity to draw (vsync) then goes back to
the initial state.


compositeAndReadback() requires that we run through the whole flow up through 5
without yielding the main thread at all. However, while it's doing this there
may very well be another beginFrame task pending on the main thread's message
queue. When this task does eventually run it needs to have a valid BFAC state
and the reply has to be expected on the compositor side.

This patch always passes the BFAC state on the beginFrame task and uses a null
BFAC state for compositeAndReadback-forced beginFrame()s. This means when we do
a compositeAndReadback we won't "see" state that has updated only on the
compositor thread, but that's fine since we'll apply that state once we process
the beginFrame task later on and BFAC state addition is additive. The tricky
bit from the compositor side is keeping track of whether there's a pending
beginFrame message after going through the forced commit flow. To keep track
of this I've added SchedulerStateMachine::m_expectImmediateBeginFrame that
keeps the scheduler in a state that expects a beginFrameComplete after
finishing the readback.

Typical flow:

1.) Main thread enters compositeAndReadback(), posts a blocking forceBeginFrame
task to compositor thread
2.) Compositor thread posts beginFrame task to main thread
3.) Compositor thread processes forceBeginFrame task, sets scheduler bits,
signals main thread to continue
4.) Main thread runs beginFrame(NULL), posts beginFrameComplete to compositor thread
5.) Compositor thread processes beginFrameComplete, runs through state machine up
to draw and then goes back to waiting for a reply to the beginFrame it posted in
step (2)
6.) Main thread issues sync readback, finishes compositeAndReadback()
7.) Main thread processes beginFrame message posted in (2), posts reply
8.) Profit!

Note that steps (2) and (3) can happen in any order.


BUG=158747
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=167537

[enne (OOO), 2012-11-02 18:02:57]

Does layer_tree_host_unittest.cc need to be extended with more
compositeAndReadback cases.  It looks like the coverage there is pretty minimal.

Also, I am not confident in my ability to review the scheduler changes.  I would
really like Nat to take a look at this.

http://codereview.chromium.org/11362054/diff/1/cc/scheduler_state_machine.cc
File cc/scheduler_state_machine.cc (right):

http://codereview.chromium.org/11362054/diff/1/cc/scheduler_state_machine.cc#...
cc/scheduler_state_machine.cc:304: if (m_expectImmediateBeginFrame)
Where does this get tested?

http://codereview.chromium.org/11362054/diff/1/cc/thread_proxy.cc
File cc/thread_proxy.cc (right):

http://codereview.chromium.org/11362054/diff/1/cc/thread_proxy.cc#newcode502
cc/thread_proxy.cc:502: beginFrameState->memoryAllocationLimitBytes =
m_layerTreeHostImpl->memoryAllocationLimitBytes();
Maybe move the DCHECK(memoryAllocationLimitBytes) you removed from layer tree
host to here instead?

http://codereview.chromium.org/11362054/diff/1/cc/thread_proxy.cc#newcode560
cc/thread_proxy.cc:560: base::TimeTicks frameBeginTime = beginFrameState ?
beginFrameState->monotonicFrameBeginTime : base::TimeTicks::Now();
This looks a little problematic.  When we get the second commit, would we then
updateAnimations backwards in time?

[nduca, 2012-11-05 07:08:02]

Can you walk through the sequence that causes the failure in the exisitng thing?
If you recall, we actually had a postTask based approach here and I switched us
to this because it did have benefits as well, especially as we go to
less-blocking impl-side stuff. I'm far from against changing back, but I'd
really like to understand the problem with the shared memory approach.

[jamesr, 2012-11-05 17:04:53]

On 2012/11/05 07:08:02, nduca wrote:
> Can you walk through the sequence that causes the failure in the exisitng
thing?
> If you recall, we actually had a postTask based approach here and I switched
us
> to this because it did have benefits as well, especially as we go to
> less-blocking impl-side stuff. I'm far from against changing back, but I'd
> really like to understand the problem with the shared memory approach.

The race is documented in the patch description.  Once we hit (5), the
compositor thread is in COMMIT_STATE_IDLE and able to start a frame whenever it
likes and the main thread is spinning its event loop with a beginFrame... task
sitting in it.  The fix here is to change the scheduler to keep track of if
there's a pending beginFrame after the compositeAndReadback sequence is
complete.

I think the fundamental problem with the shared memory approach is it's trying
to use shared memory (without synchronization primitives) as a signalling
mechanism.   That isn't necessary once the scheduler logic is fixed.

[jamesr, 2012-11-10 02:22:20]

nat - ping

[nduca, 2012-11-10 02:37:51]

> When this task does eventually run it needs to have a valid BFAC state
and the reply has to be expected on the compositor side.

Help me out with this bit. So in the current code, when the 2nd bfac task runs,
it is supposed to see no BFAC state object, causing it to early out.

Why isnt' that working?

[jamesr, 2012-11-12 03:03:44]

On 2012/11/10 02:37:51, nduca wrote:
> > When this task does eventually run it needs to have a valid BFAC state
> and the reply has to be expected on the compositor side.
> 
> Help me out with this bit. So in the current code, when the 2nd bfac task
runs,
> it is supposed to see no BFAC state object, causing it to early out.
> 
> Why isnt' that working?

After step 5 both threads are attempting to check the same member variable.  On
the main thread, the beginFrame task may run at any time.  On the compositor
thread, input handling/animation logic may call setNeedsCommit and enter
scheduledActionBeginFrame at any time.  Thus the main thread can null check
m_beginFrame... while the compositor thread is in the middle of initializing the
variable in scheduledActionBeginFrame.

[nduca, 2012-11-12 19:39:25]

Why can't we fix it with a judiciously placed mutex?

[jamesr, 2012-11-12 20:34:53]

On 2012/11/12 19:39:25, nduca wrote:
> Why can't we fix it with a judiciously placed mutex?

We can't guard the two racing functions (scheduledActionBeginFrame and
beginFrame) in mutexes because beginFrame makes sync calls to the compositor
thread.   I don't think that trying to use a mutex to fix this really makes
sense.  The problem here is that the impl-side state machine is in an invalid
state after the compositeAndReadback - it's not expecting a frame to post but
the main thread has a beginFrame task still pending.  You can't fix a logic
issue with a mutex.

[nduca, 2012-11-12 22:24:51]

LGTM. Might put a comment in scheduler state machine explaining on the new state
setter what it does, roughly.

[commit-bot: I haz the power, 2012-11-13 21:40:29]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jamesr@chromium.org/11362054/12001

[commit-bot: I haz the power, 2012-11-13 22:03:50]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jamesr@chromium.org/11362054/3003

[commit-bot: I haz the power, 2012-11-14 00:46:20]

Change committed as 167537