cc: Add readback and forced draw states to the Scheduler

cc: Add readback and forced draw states to the Scheduler.

Readback doesn't have all its steps synchronized properly with
impl side painting enabled. This patch prevents the readback
commit from being swapped to screen and avoids using non-readback
commits for readback by adding a SynchronousReadbackState.

This patch also gives forced draws due to checkerboarding its
own ForcedReadback state that behaves independently of readback.

Previously readback and forced draws shared drawing mechanisms,
but we want readbacks to occur ASAP while we want forced draws
to occur inline with the normal frame scheduling.

BUG=276082
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=221854

[brianderson, 2013-08-27 05:01:55]

I haven't looked at tests yet and this doesn't quite right for me yet, but I'm
open to early feedback.

[brianderson, 2013-08-27 22:27:29]

PTAL. Test changes still in progress, but this patch should otherwise be close
to its final form.

[brianderson, 2013-08-28 02:52:08]

All tests, other than the Scheduler unit tests themselves, are passing now.

https://codereview.chromium.org/23503003/diff/3001/cc/scheduler/scheduler_sta...
File cc/scheduler/scheduler_state_machine.cc (right):

https://codereview.chromium.org/23503003/diff/3001/cc/scheduler/scheduler_sta...
cc/scheduler/scheduler_state_machine.cc:269: if (output_surface_state_ ==
OUTPUT_SURFACE_WAITING_FOR_FIRST_ACTIVATION)
This case is no longer needed because of the ShouldBeginOutputSurfaceCreation
changes.

https://codereview.chromium.org/23503003/diff/3001/cc/scheduler/scheduler_sta...
cc/scheduler/scheduler_state_machine.cc:281: output_surface_state_ ==
OUTPUT_SURFACE_WAITING_FOR_FIRST_COMMIT)
These cases are no longer needed because of the ShouldBeginOutputSurfaceCreation
changes.

https://codereview.chromium.org/23503003/diff/3001/cc/scheduler/scheduler_sta...
cc/scheduler/scheduler_state_machine.cc:307: if (output_surface_state_ ==
OUTPUT_SURFACE_WAITING_FOR_FIRST_ACTIVATION)
This is no longer needed because of changes to the abort draw / forced
activation timing and because of the following line in UpdateStateOnDraw:
else if (commit_state_ == COMMIT_STATE_WAITING_FOR_FIRST_DRAW &&
!has_pending_tree_)

https://codereview.chromium.org/23503003/diff/13001/cc/scheduler/scheduler_st...
File cc/scheduler/scheduler_state_machine.cc (right):

https://codereview.chromium.org/23503003/diff/13001/cc/scheduler/scheduler_st...
cc/scheduler/scheduler_state_machine.cc:327: return
!active_tree_has_been_drawn_;
I changed this because not drawing the active tree will only block something if
it hasn't been drawn before.

[brianderson, 2013-08-28 03:54:34]

All tests have been fixed now. PTAL.

[brianderson, 2013-08-29 19:23:09]

https://codereview.chromium.org/23503003/diff/18001/cc/scheduler/scheduler_st...
File cc/scheduler/scheduler_state_machine_unittest.cc (left):

https://codereview.chromium.org/23503003/diff/18001/cc/scheduler/scheduler_st...
cc/scheduler/scheduler_state_machine_unittest.cc:111:
TEST(SchedulerStateMachineTest, TestSetForcedRedrawDoesNotSetsNormalRedraw) {
I removed this test because ToT has been setting needs_redraw_ at a later point
in the readback process anyway, even if needs_redraw_ isn't set at this point.
This causes more redraws than strictly necessary after a readback, but doesn't
cause any incorrectness bugs.

Fixing the test and the needs_redraw_ logic is going to be hairy to get correct
though, and I would like to solve it in a separate patch (assuming we actually
care about performance of CompositeAndReadback):

1) If both the readback commit and replacement commit are aborted, needs_redraw_
should not change value during that entire period.
2) If the readback commit is aborted, but the replacement commit is not aborted,
needs_redraw_ should be set to true.
3) If the readback commit is not aborted, but the replacement commit is aborted,
needs_redraw_ should also still be set to true.
4) If both the readback commit and replacement commit are not aborted, it's
possible that we still don't need a redraw because the replacement commit could
have been aborted if we didn't have the readback commit. This will be the
hardest case to make perfect.

[danakj, 2013-08-29 19:46:46]

On Thu, Aug 29, 2013 at 3:23 PM, <brianderson@chromium.org> wrote:

> Fixing the test and the needs_redraw_ logic is going to be hairy to get
> correct though, and I would like to solve it in a separate patch
> (assuming we actually care about performance of CompositeAndReadback):
>

IMO we do not care at all about its performance.

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[danakj, 2013-08-30 15:56:41]

https://codereview.chromium.org/23503003/diff/18001/cc/scheduler/scheduler.cc
File cc/scheduler/scheduler.cc (right):

https://codereview.chromium.org/23503003/diff/18001/cc/scheduler/scheduler.cc...
cc/scheduler/scheduler.cc:232: client_->ScheduledActionDrawAndReadback();
How about a DrawAndReadback() method above that can DCHECK the result values are
as expected?

https://codereview.chromium.org/23503003/diff/18001/cc/scheduler/scheduler_st...
File cc/scheduler/scheduler_state_machine.cc (right):

https://codereview.chromium.org/23503003/diff/18001/cc/scheduler/scheduler_st...
cc/scheduler/scheduler_state_machine.cc:38: active_tree_has_been_drawn_(true),
This is a bit magical. Can we do this another way?

https://codereview.chromium.org/23503003/diff/18001/cc/scheduler/scheduler_st...
cc/scheduler/scheduler_state_machine.cc:205:
minor_state->SetInteger("last_frame_number_where_draw_was_called",
change this too?

https://codereview.chromium.org/23503003/diff/18001/cc/scheduler/scheduler_st...
cc/scheduler/scheduler_state_machine.cc:238: bool
SchedulerStateMachine::HasDrawnThisFrame() const {
HasDrawnAndSwappedThisFrame?

https://codereview.chromium.org/23503003/diff/18001/cc/scheduler/scheduler_st...
cc/scheduler/scheduler_state_machine.cc:300: if (!active_tree_has_been_drawn_ ||
has_pending_tree_)
what if the active tree is invalid and can't be drawn?

https://codereview.chromium.org/23503003/diff/18001/cc/scheduler/scheduler_st...
cc/scheduler/scheduler_state_machine.cc:323: // from occuring. If we are waiting
for the first draw, then perfom the
This comment is wrong now?

https://codereview.chromium.org/23503003/diff/18001/cc/scheduler/scheduler_st...
cc/scheduler/scheduler_state_machine.cc:360: // If this is our first activation,
we don't want to activate early.
We now have two conditions that trump "Forcing" an activate. Can you explain in
a comment for each why it's ok to ignore the Force state? Maybe some words like
"can't" or "must"?

https://codereview.chromium.org/23503003/diff/18001/cc/scheduler/scheduler_st...
cc/scheduler/scheduler_state_machine.cc:460: else if (readback_state_ ==
READBACK_STATE_WAITING_FOR_DRAW_AND_READBACK)
Seems like this one should come first, we don't want to swap to the screen
during a readback until the replacement commit? If you agree, can you add a
test?

Oh, but I see your CheckInvariants below. Can you explain that?

https://codereview.chromium.org/23503003/diff/18001/cc/scheduler/scheduler_st...
cc/scheduler/scheduler_state_machine.cc:499: forced_redraw_state_ ==
FORCED_REDRAW_STATE_WAITING_FOR_COMMIT ||
I don't think we commit for forced redraws when not visible, do we?

https://codereview.chromium.org/23503003/diff/18001/cc/scheduler/scheduler_st...
cc/scheduler/scheduler_state_machine.cc:500: output_surface_state_ ==
OUTPUT_SURFACE_WAITING_FOR_FIRST_COMMIT);
Same here?

https://codereview.chromium.org/23503003/diff/18001/cc/scheduler/scheduler_st...
cc/scheduler/scheduler_state_machine.cc:533:
DCHECK(active_tree_has_been_drawn_);
Can you explain?

https://codereview.chromium.org/23503003/diff/18001/cc/scheduler/scheduler_st...
cc/scheduler/scheduler_state_machine.cc:571: if (readback_state_ ==
READBACK_STATE_IDLE) {
What if the readback state is READBACK_STATE_REPLACEMENT_COMMIT_ACTIVATING then?
The comment seems to call out explicitly that we should advance then?

https://codereview.chromium.org/23503003/diff/18001/cc/scheduler/scheduler_st...
cc/scheduler/scheduler_state_machine.cc:593: // was not aborted or if we in a
readback or forced draw.
nit: "if we in a"

https://codereview.chromium.org/23503003/diff/18001/cc/scheduler/scheduler_st...
cc/scheduler/scheduler_state_machine.cc:676: active_tree_has_been_drawn_ = true;
This bool seems to now be shadowed by 3 enum states: all the waiting for draw
states. Do we need it?

https://codereview.chromium.org/23503003/diff/18001/cc/scheduler/scheduler_st...
File cc/scheduler/scheduler_state_machine.h (right):

https://codereview.chromium.org/23503003/diff/18001/cc/scheduler/scheduler_st...
cc/scheduler/scheduler_state_machine.h:66: READBACK_STATE_FORCED_COMMIT_PENDING,
This state enums are all different from each other in their verbage, and I am
finding it tricky to read and understand.

CommitState has FRAME_IN_PROGRESS... Forced redraw has WAITING_FOR_COMMIT...
This one has COMMIT_PENDING (and COMMIT_REQUESTED).

WDYT of..

READBACK_STATE_IDLE
READBACK_STATE_NEEDS_BEGIN_FRAME
READBACK_STATE_WAITING_FOR_COMMIT
READBACK_STATE_WAITING_FOR_ACTIVATION
READBACK_STATE_WAITING_FOR_DRAW_AND_READBACK
READBACK_STATE_WAITING_FOR_REPLACEMENT_COMMIT
READBACK_STATE_WAITING_FOR_REPLACEMENT_ACTIVATION


FORCED_REDRAW_STATE_IDLE
FORCED_REDRAW_STATE_WAITING_FOR_COMMIT
FORCED_REDRAW_STATE_WAITING_FOR_ACTIVATION
FORCED_REDRAW_STATE_WAITING_FOR_DRAW



All of this said, I'm not sure I like moving the "needs_foo" state into this
enum. That's not a pattern we do elsewhere. needs states have been bools, with
enums acting as "we're waiting for this" not "we need to do this". Maybe you
could convince me why that's a good idea?

https://codereview.chromium.org/23503003/diff/18001/cc/trees/thread_proxy.cc
File cc/trees/thread_proxy.cc (right):

https://codereview.chromium.org/23503003/diff/18001/cc/trees/thread_proxy.cc#...
cc/trees/thread_proxy.cc:145: // main thread, which  will be called after this
CompositeAndReadback
nit: double space

https://codereview.chromium.org/23503003/diff/18001/cc/trees/thread_proxy.cc#...
cc/trees/thread_proxy.cc:983: result.did_swap = false;
should we set did_readback false? or should we remove these two and rely on the
constructor?

https://codereview.chromium.org/23503003/diff/18001/cc/trees/thread_proxy.cc#...
cc/trees/thread_proxy.cc:1066: } else if (draw_frame && swap_requested) {
Are you saying you expect this function to be called with swap_requested =
false, and drawing_for_readback = false?

[brianderson, 2013-09-03 22:41:11]

https://codereview.chromium.org/23503003/diff/18001/cc/scheduler/scheduler.cc
File cc/scheduler/scheduler.cc (right):

https://codereview.chromium.org/23503003/diff/18001/cc/scheduler/scheduler.cc...
cc/scheduler/scheduler.cc:232: client_->ScheduledActionDrawAndReadback();
On 2013/08/30 15:56:41, danakj wrote:
> How about a DrawAndReadback() method above that can DCHECK the result values
are
> as expected?

I avoided checking for success because I read somewhere in the code that the
readback could fail in some cases and that would be ok. Is readback always
expected to succeed? Or did you just want to check that did_readback is true if
the draw succeeded?

https://codereview.chromium.org/23503003/diff/18001/cc/scheduler/scheduler_st...
File cc/scheduler/scheduler_state_machine.cc (right):

https://codereview.chromium.org/23503003/diff/18001/cc/scheduler/scheduler_st...
cc/scheduler/scheduler_state_machine.cc:38: active_tree_has_been_drawn_(true),
On 2013/08/30 15:56:41, danakj wrote:
> This is a bit magical. Can we do this another way?

This is to avoid aborting a draw before the first commit, which is innocuous but
weird and not expected. I've found various ways to avoid aborting that first
draw in the past, but this solution is always so tempting when I can't think of
something right away. In this patch, I should be able to condition the aborted
draw on did_create_and_initialize_first_output_surface_.

https://codereview.chromium.org/23503003/diff/18001/cc/scheduler/scheduler_st...
cc/scheduler/scheduler_state_machine.cc:205:
minor_state->SetInteger("last_frame_number_where_draw_was_called",
On 2013/08/30 15:56:41, danakj wrote:
> change this too?

OK.

https://codereview.chromium.org/23503003/diff/18001/cc/scheduler/scheduler_st...
cc/scheduler/scheduler_state_machine.cc:238: bool
SchedulerStateMachine::HasDrawnThisFrame() const {
On 2013/08/30 15:56:41, danakj wrote:
> HasDrawnAndSwappedThisFrame?

Ok. That would match the surrounding code better.

https://codereview.chromium.org/23503003/diff/18001/cc/scheduler/scheduler_st...
cc/scheduler/scheduler_state_machine.cc:300: if (!active_tree_has_been_drawn_ ||
has_pending_tree_)
On 2013/08/30 15:56:41, danakj wrote:
> what if the active tree is invalid and can't be drawn?

The draw will be aborted and will set active_tree_has_been_drawn_ to true, which
will unblock activations, which will allow has_pending_tree_ to also go false.

https://codereview.chromium.org/23503003/diff/18001/cc/scheduler/scheduler_st...
cc/scheduler/scheduler_state_machine.cc:360: // If this is our first activation,
we don't want to activate early.
On 2013/08/30 15:56:41, danakj wrote:
> We now have two conditions that trump "Forcing" an activate. Can you explain
in
> a comment for each why it's ok to ignore the Force state? Maybe some words
like
> "can't" or "must"?

Will do.

https://codereview.chromium.org/23503003/diff/18001/cc/scheduler/scheduler_st...
cc/scheduler/scheduler_state_machine.cc:460: else if (readback_state_ ==
READBACK_STATE_WAITING_FOR_DRAW_AND_READBACK)
On 2013/08/30 15:56:41, danakj wrote:
> Seems like this one should come first, we don't want to swap to the screen
> during a readback until the replacement commit? If you agree, can you add a
> test?
> 
> Oh, but I see your CheckInvariants below. Can you explain that?

I will go through the code and make sure that the readback logic is always
handled first, unless it absolutely cannot be, since it generally takes
precedence over everything else anyway.

In this case, the order does not matter because of the reason you alluded to in
CheckInvariants(). If we are in READBACK_STATE_WAITING_FOR_DRAW_AND_READBACK, we
cannot be in FORCED_REDRAW_STATE_WAITING_FOR_DRAW because the readback commit
must never be swapped to screen. This is enforced in UpdateStateOnCommit by only
advancing the other states that might cause a swap when the readback state is
idle.

https://codereview.chromium.org/23503003/diff/18001/cc/scheduler/scheduler_st...
cc/scheduler/scheduler_state_machine.cc:499: forced_redraw_state_ ==
FORCED_REDRAW_STATE_WAITING_FOR_COMMIT ||
On 2013/08/30 15:56:41, danakj wrote:
> I don't think we commit for forced redraws when not visible, do we?

Good catch. The main thread would just abort the commit anway. This version of
the patch shouldn't commit in these cases anymore, so I will remove them from
the DCHECK.

https://codereview.chromium.org/23503003/diff/18001/cc/scheduler/scheduler_st...
cc/scheduler/scheduler_state_machine.cc:533:
DCHECK(active_tree_has_been_drawn_);
On 2013/08/30 15:56:41, danakj wrote:
> Can you explain?

I added logic to flush the pipeline before starting output surface
initialization to avoid really complicated corner cases where we want to do all
of the following at the same time:
1) Abort a draw.
2) Force an activation.
3) Initialize the output surface.
4) Perform a readback.

These DCHECKs verify that the pipeline was actually flushed and that we only
have to worry about doing 3 and 4 at the same time.

https://codereview.chromium.org/23503003/diff/18001/cc/scheduler/scheduler_st...
cc/scheduler/scheduler_state_machine.cc:571: if (readback_state_ ==
READBACK_STATE_IDLE) {
On 2013/08/30 15:56:41, danakj wrote:
> What if the readback state is READBACK_STATE_REPLACEMENT_COMMIT_ACTIVATING
then?
> The comment seems to call out explicitly that we should advance then?

Good catch again. I should include READBACK_STATE_REPLACEMENT_COMMIT_ACTIVATING
in this if statement as well.

https://codereview.chromium.org/23503003/diff/18001/cc/scheduler/scheduler_st...
cc/scheduler/scheduler_state_machine.cc:593: // was not aborted or if we in a
readback or forced draw.
On 2013/08/30 15:56:41, danakj wrote:
> nit: "if we in a"

Done.

https://codereview.chromium.org/23503003/diff/18001/cc/scheduler/scheduler_st...
cc/scheduler/scheduler_state_machine.cc:676: active_tree_has_been_drawn_ = true;
On 2013/08/30 15:56:41, danakj wrote:
> This bool seems to now be shadowed by 3 enum states: all the waiting for draw
> states. Do we need it?

This probably could be removed at ToT, but the upcoming patches to start the
next commit before the previous commit has been drawn will de-shadow the commit
state from active_tree_has_been_drawn_. Do you mind if I keep the variable to
avoid churn?

https://codereview.chromium.org/23503003/diff/18001/cc/scheduler/scheduler_st...
File cc/scheduler/scheduler_state_machine.h (right):

https://codereview.chromium.org/23503003/diff/18001/cc/scheduler/scheduler_st...
cc/scheduler/scheduler_state_machine.h:66: READBACK_STATE_FORCED_COMMIT_PENDING,
On 2013/08/30 15:56:41, danakj wrote:
> This state enums are all different from each other in their verbage, and I am
> finding it tricky to read and understand.
> 
> CommitState has FRAME_IN_PROGRESS... Forced redraw has WAITING_FOR_COMMIT...
> This one has COMMIT_PENDING (and COMMIT_REQUESTED).
> 
> WDYT of..
> 
> READBACK_STATE_IDLE
> READBACK_STATE_NEEDS_BEGIN_FRAME
> READBACK_STATE_WAITING_FOR_COMMIT
> READBACK_STATE_WAITING_FOR_ACTIVATION
> READBACK_STATE_WAITING_FOR_DRAW_AND_READBACK
> READBACK_STATE_WAITING_FOR_REPLACEMENT_COMMIT
> READBACK_STATE_WAITING_FOR_REPLACEMENT_ACTIVATION
> 
> 
> FORCED_REDRAW_STATE_IDLE
> FORCED_REDRAW_STATE_WAITING_FOR_COMMIT
> FORCED_REDRAW_STATE_WAITING_FOR_ACTIVATION
> FORCED_REDRAW_STATE_WAITING_FOR_DRAW
> 
> 
> 
> All of this said, I'm not sure I like moving the "needs_foo" state into this
> enum. That's not a pattern we do elsewhere. needs states have been bools, with
> enums acting as "we're waiting for this" not "we need to do this". Maybe you
> could convince me why that's a good idea?

I like your names, it is clearer what each state is waiting for.

I am actually tempted to move more of the needs_foo bools into existing enums. 
It directly encodes the reason why we need something, making the scheduler state
easier to decode at a glance without having to look at the code.

https://codereview.chromium.org/23503003/diff/18001/cc/trees/thread_proxy.cc
File cc/trees/thread_proxy.cc (right):

https://codereview.chromium.org/23503003/diff/18001/cc/trees/thread_proxy.cc#...
cc/trees/thread_proxy.cc:145: // main thread, which  will be called after this
CompositeAndReadback
On 2013/08/30 15:56:41, danakj wrote:
> nit: double space

Done.

https://codereview.chromium.org/23503003/diff/18001/cc/trees/thread_proxy.cc#...
cc/trees/thread_proxy.cc:983: result.did_swap = false;
On 2013/08/30 15:56:41, danakj wrote:
> should we set did_readback false? or should we remove these two and rely on
the
> constructor?

I will set did_readback to false so it is explicit.

https://codereview.chromium.org/23503003/diff/18001/cc/trees/thread_proxy.cc#...
cc/trees/thread_proxy.cc:1066: } else if (draw_frame && swap_requested) {
On 2013/08/30 15:56:41, danakj wrote:
> Are you saying you expect this function to be called with swap_requested =
> false, and drawing_for_readback = false?

My intention was to make sure that swap_requested is true if we're not drawing
for readback. A DCHECK would be better than modifying the if statement as I did.

[danakj, 2013-09-03 22:45:01]

https://codereview.chromium.org/23503003/diff/18001/cc/scheduler/scheduler_st...
File cc/scheduler/scheduler_state_machine.cc (right):

https://codereview.chromium.org/23503003/diff/18001/cc/scheduler/scheduler_st...
cc/scheduler/scheduler_state_machine.cc:571: if (readback_state_ ==
READBACK_STATE_IDLE) {
On 2013/09/03 22:41:12, Brian Anderson (OOO 7-29) wrote:
> On 2013/08/30 15:56:41, danakj wrote:
> > What if the readback state is READBACK_STATE_REPLACEMENT_COMMIT_ACTIVATING
> then?
> > The comment seems to call out explicitly that we should advance then?
> 
> Good catch again. I should include
READBACK_STATE_REPLACEMENT_COMMIT_ACTIVATING
> in this if statement as well.

Can you add a test for this and any other mistakes we uncover that weren't
covered by a test?

[brianderson, 2013-09-04 19:31:03]

I am working on new tests now. Any more feedback on the latest patch?

[brianderson, 2013-09-04 21:34:38]

Working on more tests, but wanted feedback from folks familiar with the
delegated renderer regarding why the test I just added might not work with it.

https://codereview.chromium.org/23503003/diff/43001/cc/trees/layer_tree_host_...
File cc/trees/layer_tree_host_unittest.cc (right):

https://codereview.chromium.org/23503003/diff/43001/cc/trees/layer_tree_host_...
cc/trees/layer_tree_host_unittest.cc:547: EXPECT_EQ(prepare_to_draw_count_, 3);
This expectation fails with the delegated renderer. Does returning false from
PrepareToDraw behave differently with the delegated renderer?

[brianderson, 2013-09-04 21:48:59]

https://codereview.chromium.org/23503003/diff/43001/cc/trees/layer_tree_host_...
File cc/trees/layer_tree_host_unittest.cc (right):

https://codereview.chromium.org/23503003/diff/43001/cc/trees/layer_tree_host_...
cc/trees/layer_tree_host_unittest.cc:547: EXPECT_EQ(prepare_to_draw_count_, 3);
On 2013/09/04 21:34:39, Brian Anderson wrote:
> This expectation fails with the delegated renderer. Does returning false from
> PrepareToDraw behave differently with the delegated renderer?

I looks like the delegating renderer cannot do a readback and does not bother
calling prepare to draw for the readback. I will adjust the test expectation
accordingly.

[enne (OOO), 2013-09-04 23:24:32]

I am really not sure I follow what the need for a "replacement commit" after a
readback is for.  It seems like the forced commit that you do as a part of
readback is a "real" commit.  The only extra thing it seems to do on the
compositor thread is to union in some additional damage along with the damage
that would have happened with the frame.  It seems like if you treated that like
a real commit and swapped after the readback, then everything should be ok. 
What am I missing that the replacement commit is "fixing up"?

danakj: Maybe this is just magical thinking on my part, but is there a reason we
can't use your asynchronous CopyOutputRequest to implement CompositeAndReadback?
If we blocked until the callback, then the main thread would block through
commit / activation / draw as this patch is trying to do.  If so, that would
greatly simplify the scheduler.

https://codereview.chromium.org/23503003/diff/43001/cc/scheduler/scheduler_se...
File cc/scheduler/scheduler_settings.cc (right):

https://codereview.chromium.org/23503003/diff/43001/cc/scheduler/scheduler_se...
cc/scheduler/scheduler_settings.cc:12:
maximum_number_of_failed_draws_before_draw_is_forced_(3),
I like that this is bumped up to be a setting.

https://codereview.chromium.org/23503003/diff/43001/cc/scheduler/scheduler_st...
File cc/scheduler/scheduler_state_machine.cc (right):

https://codereview.chromium.org/23503003/diff/43001/cc/scheduler/scheduler_st...
cc/scheduler/scheduler_state_machine.cc:819: readback_state_ =
READBACK_STATE_IDLE;
I am a little bit unsure of this bit.  If the main thread is in a commit and
readback state, then it won't abort the commit.  Should this maybe just be a
DCHECK?

[brianderson, 2013-09-05 00:04:51]

Using the async readback path would be nice, since it would greatly simplify the
scheduler.

If we can't do that, it would be great if we could get rid of the replacement
commit requirement when we know the readback commit is valid.

One feature I do like about this patch is that it doesn't swap the readback
commit to screen, preventing CompositeAndReadback from increasing our latency if
the readback commit is redundant.  The readback commit is also sampled outside
our normal BeginFrame interval, which could introduce jitter if it were swapped
to the screen because the animation time is off or because we didn't wait for
the next batch of user input.

To completely address the latency and jitter issues however, the replacement
commit would have to be tied to the normal BeginFrame interval, which this patch
does not do yet.

[danakj, 2013-09-05 20:07:28]

On Wed, Sep 4, 2013 at 7:24 PM, <enne@chromium.org> wrote:

> I am really not sure I follow what the need for a "replacement commit"
> after a
> readback is for.  It seems like the forced commit that you do as a part of
> readback is a "real" commit.  The only extra thing it seems to do on the
> compositor thread is to union in some additional damage along with the
> damage
> that would have happened with the frame.  It seems like if you treated
> that like
> a real commit and swapped after the readback, then everything should be ok.
> What am I missing that the replacement commit is "fixing up"?
>
> danakj: Maybe this is just magical thinking on my part, but is there a
> reason we
> can't use your asynchronous CopyOutputRequest to implement
> CompositeAndReadback?
> If we blocked until the callback, then the main thread would block through
> commit / activation / draw as this patch is trying to do.  If so, that
> would
> greatly simplify the scheduler.
>

The bug tree exists detailing all the current uses of CompositeAndReadback
with thoughts about how to switch them over:
https://code.google.com/p/chromium/issues/detail?id=251936

Only one of them seems at all tricky IIRC.


>
>
> https://codereview.chromium.**org/23503003/diff/43001/cc/**
>
scheduler/scheduler_settings.**cc<https://codereview.chromium.org/23503003/diff/43001/cc/scheduler/scheduler_settings.cc>
> File cc/scheduler/scheduler_**settings.cc (right):
>
> https://codereview.chromium.**org/23503003/diff/43001/cc/**
>
scheduler/scheduler_settings.**cc#newcode12<https://codereview.chromium.org/23503003/diff/43001/cc/scheduler/scheduler_settings.cc#newcode12>
> cc/scheduler/scheduler_**settings.cc:12:
> maximum_number_of_failed_**draws_before_draw_is_forced_(**3),
> I like that this is bumped up to be a setting.
>
> https://codereview.chromium.**org/23503003/diff/43001/cc/**
>
scheduler/scheduler_state_**machine.cc<https://codereview.chromium.org/23503003/diff/43001/cc/scheduler/scheduler_state_machine.cc>
> File cc/scheduler/scheduler_state_**machine.cc (right):
>
> https://codereview.chromium.**org/23503003/diff/43001/cc/**
>
scheduler/scheduler_state_**machine.cc#newcode819<https://codereview.chromium.org/23503003/diff/43001/cc/scheduler/scheduler_state_machine.cc#newcode819>
> cc/scheduler/scheduler_state_**machine.cc:819: readback_state_ =
> READBACK_STATE_IDLE;
> I am a little bit unsure of this bit.  If the main thread is in a commit
> and readback state, then it won't abort the commit.  Should this maybe
> just be a DCHECK?
>
>
https://codereview.chromium.**org/23503003/<https://codereview.chromium.org/2...
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[danakj, 2013-09-05 20:09:20]

On Wed, Sep 4, 2013 at 8:04 PM, <brianderson@chromium.org> wrote:

> Using the async readback path would be nice, since it would greatly
> simplify the
> scheduler.
>
> If we can't do that, it would be great if we could get rid of the
> replacement
> commit requirement when we know the readback commit is valid.
>
> One feature I do like about this patch is that it doesn't swap the readback
> commit to screen, preventing CompositeAndReadback from increasing our
> latency if
>

We've never swapped the readback commit though, have we? We only swap if
there's no readback request present in thread proxy.


> the readback commit is redundant.  The readback commit is also sampled
> outside
> our normal BeginFrame interval, which could introduce jitter if it were
> swapped
> to the screen because the animation time is off or because we didn't wait
> for
> the next batch of user input.
>
> To completely address the latency and jitter issues however, the
> replacement
> commit would have to be tied to the normal BeginFrame interval, which this
> patch
> does not do yet.
>

I think you should keep the C&R codepath as simple to maintain and
understand as possible and not worry about correctness wrt animation jitter
or perf at all if that's going to complicate anything at all.


>
>
https://codereview.chromium.**org/23503003/<https://codereview.chromium.org/2...
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[brianderson, 2013-09-05 23:17:07]

After some thought, I would like to land this patch as-is to make C&R correct
and unblock pending scheduler changes.  Afterwards, I will work on removing C&R
from the scheduler. Simplifying C&R in the Scheduler, while maintaining
correctness, is not going to be trivial. Details below.

I combed through the issues blocked on
https://code.google.com/p/chromium/issues/detail?id=251936 and it looks like
RenderWidget::OnPaintAtSize is the only code path that changes the size of the
readback commit, necessitating the replacement commit.

I see a few ways forward here:
1) Land this patch and keep OnPaintAtSize. Everything is correct, but the
scheduler gets complicated.
2) Implement C&R as a wrapper around the async path and keep OnPaintAtSize. The
scheduler would still need to request a replacement commit and avoid drawing
until it has activated.
3) Implement C&R as a wrapper around the async path and get rid of
OnPaintAtSize. The scheduler does not need to request a replacement commit but
must still block C&R from committing the readback commit too early whether or
not a normal commit is pending. We might be able to use completion events to
simplify the scheduler even more though.
4) Get rid of OnPaintAtSize and fix C&R. This will allow us to get rid of the
replacement commit requirement, although we must still track if there is a
commit pending behind the readback and restore state properly after readback
completes.
5) Get rid of OnPaintAtSize and keep C&R broken. If we keep it broken, we will
have to relax test expectations for upcoming scheduler patches to land.

I am not a fan of 5.

In order to unblock pending scheduler changes any further, I would like to go
with 1 for now and then transition to 3 after the scheduler changes have landed
to simplify the state machine.

[enne (OOO), 2013-09-06 15:40:13]

Thanks for all the investigation and research.  It's helpful to know what the
actual source of needing the replacement commit is for.  I think I was trying to
suggest something like #3, but if #1 helps you move forward and you're planning
on cleaning it up later, then that sounds fine to me too.

I'll try to take a last look at this today but if danakj is ok with this, don't
feel blocked on waiting for me.

[brianderson, 2013-09-06 21:43:15]

https://codereview.chromium.org/23503003/diff/62001/cc/output/direct_renderer.cc
File cc/output/direct_renderer.cc (right):

https://codereview.chromium.org/23503003/diff/62001/cc/output/direct_renderer...
cc/output/direct_renderer.cc:139: return !output_surface_->IsLost();
This hopefully fixes the mac_rel failure when losing the context in the middle
of a readback by just failing the readback.

[danakj, 2013-09-06 21:45:56]

https://codereview.chromium.org/23503003/diff/62001/cc/output/direct_renderer.cc
File cc/output/direct_renderer.cc (right):

https://codereview.chromium.org/23503003/diff/62001/cc/output/direct_renderer...
cc/output/direct_renderer.cc:139: return !output_surface_->IsLost();
On 2013/09/06 21:43:15, Brian Anderson wrote:
> This hopefully fixes the mac_rel failure when losing the context in the middle
> of a readback by just failing the readback.

This is kinda abuse of this method. It's really meant like "is this renderer
capable of reading pixels", not checking on states. Can this be done outside the
renderer?

[brianderson, 2013-09-06 22:04:28]

https://codereview.chromium.org/23503003/diff/83001/cc/trees/thread_proxy.cc
File cc/trees/thread_proxy.cc (right):

https://codereview.chromium.org/23503003/diff/83001/cc/trees/thread_proxy.cc#...
cc/trees/thread_proxy.cc:1078: if (draw_frame &&
!layer_tree_host_impl_->IsContextLost()) {
It was easier to check for context lost here than I thought!

[enne (OOO), 2013-09-06 22:24:29]

After staring at this a bunch, this lgtm.  I do think this adds a whole lot of
complexity, but I think you've also added a lot of good tests to make sure the
interactions of all these states does the right thing.

I wish C&R would go away, but I think moving these scheduler changes are a lot
more important than shaving that yak.

https://codereview.chromium.org/23503003/diff/21001/cc/scheduler/scheduler_st...
File cc/scheduler/scheduler_state_machine.cc (right):

https://codereview.chromium.org/23503003/diff/21001/cc/scheduler/scheduler_st...
cc/scheduler/scheduler_state_machine.cc:527: // The following DCHECKs make sure
we are in the proper quiecent state.
typo: quiescent

https://codereview.chromium.org/23503003/diff/21001/cc/scheduler/scheduler_st...
cc/scheduler/scheduler_state_machine.cc:776: // We need to force a draw, but it
doesn't make sense to do this until
This patch doesn't change the status quo, but this is poor behavior for
impl-side painting.  Having to force a commit makes sense for non-impl-side
painting where you won't get new textures until a commit.  

For impl-side painting, it'd probably be better to just try drawing and set
needs commit in general (just to give the main thread a chance to send more
content if it wants).  There's really no need for a force, and no need to force
redraw afterwards.

https://codereview.chromium.org/23503003/diff/21001/cc/scheduler/scheduler_st...
cc/scheduler/scheduler_state_machine.cc:819: readback_state_ =
READBACK_STATE_IDLE;
I still wonder if this should just be a DCHECK.  I'm pretty sure this can't be
aborted.

[brianderson, 2013-09-06 22:45:00]

The patch that abuses CanReadback fixed the test that I added that loses the
context during the readback, but causes a couple of pixel tests to fail.

That would imply that they are also losing the context during a readback. It's
weird that this is only happening on mac_rel though. It's also weird that these
tests could pass before if the readback was performed while the context was
lost.

Hopefully the second version (that doesn't abuse CanReadback) doesn't have the
same issue, but I'm not very hopeful.

Investigation ongoing.

https://codereview.chromium.org/23503003/diff/21001/cc/scheduler/scheduler_st...
File cc/scheduler/scheduler_state_machine.cc (right):

https://codereview.chromium.org/23503003/diff/21001/cc/scheduler/scheduler_st...
cc/scheduler/scheduler_state_machine.cc:527: // The following DCHECKs make sure
we are in the proper quiecent state.
On 2013/09/06 22:24:30, enne wrote:
> typo: quiescent

Done.

https://codereview.chromium.org/23503003/diff/21001/cc/scheduler/scheduler_st...
cc/scheduler/scheduler_state_machine.cc:776: // We need to force a draw, but it
doesn't make sense to do this until
On 2013/09/06 22:24:30, enne wrote:
> This patch doesn't change the status quo, but this is poor behavior for
> impl-side painting.  Having to force a commit makes sense for non-impl-side
> painting where you won't get new textures until a commit.  
> 
> For impl-side painting, it'd probably be better to just try drawing and set
> needs commit in general (just to give the main thread a chance to send more
> content if it wants).  There's really no need for a force, and no need to
force
> redraw afterwards.

I've opened a separate bug to fix this here:
https://code.google.com/p/chromium/issues/detail?id=286486

https://codereview.chromium.org/23503003/diff/21001/cc/scheduler/scheduler_st...
cc/scheduler/scheduler_state_machine.cc:819: readback_state_ =
READBACK_STATE_IDLE;
On 2013/09/06 22:24:30, enne wrote:
> I still wonder if this should just be a DCHECK.  I'm pretty sure this can't be
> aborted.

Sorry I forgot about the comment you made previously. I agree about the DCHECK
and will change it in the next patch.

[brianderson, 2013-09-06 23:40:48]

Not sure what happened, but it looks like the more straight-forward way of
detecting a lost context doesn't cause the pixel tests to fail.

I talked with Dana and although she doesn't have time to do a full review, she
is ok with me going ahead with enne's lgtm and addressing any further concerns
with followup patches.

[commit-bot: I haz the power, 2013-09-06 23:41:58]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/brianderson@chromium.org/23503003/96001

[commit-bot: I haz the power, 2013-09-07 01:47:31]

Change committed as 221854

[brianderson, 2013-10-04 17:17:43]

https://chromiumcodereview.appspot.com/23503003/diff/96001/cc/scheduler/sched...
File cc/scheduler/scheduler_state_machine.cc (left):

https://chromiumcodereview.appspot.com/23503003/diff/96001/cc/scheduler/sched...
cc/scheduler/scheduler_state_machine.cc:553: commit_state_ = COMMIT_STATE_IDLE;
If the active tree was drawn twice (because of an animation or otherwise) ToT
prior to this patch would start the next commit before activation because it
would transition straight to idle here.  ToT doesn't (and didn't) have the logic
to enforce proper back pressure if a commit was started while there was still a
pending tree.

Scary. It seems to me that the pending tree could have just been overwritten
when the next commit came in?  This was also artificially inflating our perf
benchmarks. See https://code.google.com/p/chromium/issues/detail?id=287756 for
background.

It's also a good indication though that we should try to get
https://codereview.chromium.org/23907006 in, which does implement the proper
back pressure and starts the next commit without needing an additional draw to
kick it off.