cc: Fix infinite BeginFrame that Scheduler causes.

cc: Fix infinite BeginFrame that Scheduler causes.

REGRESSION(r206955): Remove the chance to disable SetNeedsBeginFrame.

r206955 made SetNeedsBeginFrame be called proactively, but the proactive condition mostly was true.

This issue makes proactive_begin_frame_wanted condition more restrict.
There are two changes.
1. Do not be proactive when invisible.
2. Do not be proactive when throttling frame production.

This patch is mostly based on what Brian Anderson consults.

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

[dshwang, 2013-06-24 16:07:47]

http://youtu.be/tg5r_WF2hCQ
I recorded the video to clarify the urgent bug.
It consumes CPU badly.

[danakj, 2013-06-24 16:56:27]

+nduca

[nduca, 2013-06-24 17:14:31]

brian can review this.

[brianderson, 2013-06-24 18:04:48]

Thanks for the patch @dshwang. We'll need to figure out a fix that doesn't
disable SetNeedsBeginFrame immediately though.

The bug we need to figure out is why we are not disabling SetNeedsBeginFrame on
the next BeginFrame.

I'll let you know if I come up with anything. Let me know if you figure it out
too!

https://codereview.chromium.org/17587014/diff/1/cc/scheduler/scheduler.cc
File cc/scheduler/scheduler.cc (right):

https://codereview.chromium.org/17587014/diff/1/cc/scheduler/scheduler.cc#new...
cc/scheduler/scheduler.cc:149: state_machine_.inside_begin_frame() ||
The intent of state_machine_.inside_begin_frame(), in this if statement was to
avoid disabling SetNeedsBeginFrame until the next BeginFrame, in case it was
re-enabled before the next BeginFrame.

Otherwise, there could be a disable glitch that gets picked up by the BeginFrame
provider causing us to lose a BeginFrame.

Apparently, it is not being disabled on the next BeginFrame though. Do you
happen to know why? I'm still trying to figure it out myself.

https://codereview.chromium.org/17587014/diff/1/cc/scheduler/scheduler.cc#new...
cc/scheduler/scheduler.cc:166:
client_->SetNeedsBeginFrameOnImplThread(needs_begin_frame_to_draw);
We should avoid disabling SetNeedsBeginFrame until the next BeginFrame, so we
can't put this here.

[dshwang, 2013-06-24 18:32:20]

On 2013/06/24 18:04:48, Brian Anderson wrote:
Thank you for good explanation!

> I'll let you know if I come up with anything. Let me know if you figure it out
> too!
Of course, I do.

> https://codereview.chromium.org/17587014/diff/1/cc/scheduler/scheduler.cc
> File cc/scheduler/scheduler.cc (right):
> 
>
https://codereview.chromium.org/17587014/diff/1/cc/scheduler/scheduler.cc#new...
> cc/scheduler/scheduler.cc:149: state_machine_.inside_begin_frame() ||
> The intent of state_machine_.inside_begin_frame(), in this if statement was to
> avoid disabling SetNeedsBeginFrame until the next BeginFrame, in case it was
> re-enabled before the next BeginFrame.
> 
> Otherwise, there could be a disable glitch that gets picked up by the
BeginFrame
> provider causing us to lose a BeginFrame.

As I understand, when disable is needed, we should get a chance to BeginFrame
onetime and then disable.

> Apparently, it is not being disabled on the next BeginFrame though. Do you
> happen to know why? I'm still trying to figure it out myself.

I can not find yet which code disable on the next BeginFrame. Can you explain?

[dshwang, 2013-06-24 18:53:20]

https://codereview.chromium.org/17587014/diff/8001/cc/scheduler/scheduler.cc
File cc/scheduler/scheduler.cc (right):

https://codereview.chromium.org/17587014/diff/8001/cc/scheduler/scheduler.cc#...
cc/scheduler/scheduler.cc:138: needs_begin_frame_to_draw ||
"state_machine_.inside_begin_frame() is true" means that COMMIT is performed
now. So we don't need to be proactive.

Moreover, being proactive removes the chance to disable "reserved BeginFrame".

[brianderson, 2013-06-24 20:12:30]

From your description, it does sound like the proactive request is the culprit.

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

https://codereview.chromium.org/17587014/diff/11001/cc/scheduler/scheduler_st...
cc/scheduler/scheduler_state_machine.cc:372: !inside_begin_frame_;
Can you see if this works instead? I would test it myself, but my tree is broken
right now.

if (!visible_ || output_surface_state_ != OUTPUT_SURFACE_ACTIVE)
  return false;

if (needs_commit_ || needs_forced_commit_ ||
    commit_state_ != COMMIT_STATE_IDLE)
  return true;

[dshwang, 2013-06-25 05:05:57]

On 2013/06/24 20:12:30, Brian Anderson wrote:
> From your description, it does sound like the proactive request is the
culprit.
> 
>
https://codereview.chromium.org/17587014/diff/11001/cc/scheduler/scheduler_st...
> File cc/scheduler/scheduler_state_machine.cc (right):
> 
>
https://codereview.chromium.org/17587014/diff/11001/cc/scheduler/scheduler_st...
> cc/scheduler/scheduler_state_machine.cc:372: !inside_begin_frame_;
> Can you see if this works instead? I would test it myself, but my tree is
broken
> right now.
> 
> if (!visible_ || output_surface_state_ != OUTPUT_SURFACE_ACTIVE)
>   return false;
> 
> if (needs_commit_ || needs_forced_commit_ ||
>     commit_state_ != COMMIT_STATE_IDLE)
>   return true;

Your condition relieve much the problem. but it does not completely remove
useless OnTimerTick call.
When I open 7 blank tabs and run tracing, all tracing buffer is consumed in 30
sec.

I agree that the visibility check is necessary.

How about merging all condition? It works well.

  if (!visible_ || output_surface_state_ != OUTPUT_SURFACE_ACTIVE)
    return false;

  return (needs_commit_ || needs_forced_commit_ ||
      commit_state_ != COMMIT_STATE_IDLE) &&
      !inside_begin_frame_;

[brianderson, 2013-06-25 17:58:00]

On 2013/06/25 05:05:57, dshwang wrote:
> On 2013/06/24 20:12:30, Brian Anderson wrote:
> > From your description, it does sound like the proactive request is the
> culprit.
> > 
> >
>
https://codereview.chromium.org/17587014/diff/11001/cc/scheduler/scheduler_st...
> > File cc/scheduler/scheduler_state_machine.cc (right):
> > 
> >
>
https://codereview.chromium.org/17587014/diff/11001/cc/scheduler/scheduler_st...
> > cc/scheduler/scheduler_state_machine.cc:372: !inside_begin_frame_;
> > Can you see if this works instead? I would test it myself, but my tree is
> broken
> > right now.
> > 
> > if (!visible_ || output_surface_state_ != OUTPUT_SURFACE_ACTIVE)
> >   return false;
> > 
> > if (needs_commit_ || needs_forced_commit_ ||
> >     commit_state_ != COMMIT_STATE_IDLE)
> >   return true;
> 
> Your condition relieve much the problem. but it does not completely remove
> useless OnTimerTick call.
> When I open 7 blank tabs and run tracing, all tracing buffer is consumed in 30
> sec.

Are all 7 tabs visible?

> 
> I agree that the visibility check is necessary.
> 
> How about merging all condition? It works well.
> 
>   if (!visible_ || output_surface_state_ != OUTPUT_SURFACE_ACTIVE)
>     return false;
> 
>   return (needs_commit_ || needs_forced_commit_ ||
>       commit_state_ != COMMIT_STATE_IDLE) &&
>       !inside_begin_frame_;

We still want the proactive request even if we are inside_begin_frame. I think
there is something else we are still missing here: why does a visible blank page
keep thinking it needs commits?

[dshwang, 2013-06-25 18:10:08]

On 2013/06/25 17:58:00, Brian Anderson wrote:
Hi, we both update coincidentally :)


> > Your condition relieve much the problem. but it does not completely remove
> > useless OnTimerTick call.
> > When I open 7 blank tabs and run tracing, all tracing buffer is consumed in
30
> > sec.
> 
> Are all 7 tabs visible?

I reported slightly wrong yesterday.

There are two issues.
 One big issue : what you think. Invisible tab proactively requests beginFrame.
 One small issue : ProactiveBeginFrameWantedByImplThread() is so optimistic.

 
> >   if (!visible_ || output_surface_state_ != OUTPUT_SURFACE_ACTIVE)
> >     return false;

This make other tabs not request beginFrame. So most issue is resolved.


> >   return (needs_commit_ || needs_forced_commit_ ||
> >       commit_state_ != COMMIT_STATE_IDLE);

However, I think this condition is so optimistic. It is very often for this
condition to be true. So OnTimerTick called hundreds time in few ms, and then
tracing buffer drains quickly.

Do you think "commit_state_ != COMMIT_STATE_IDLE" is necessary?


Anyway, what you suggest works well, so if above question is hard I think we can
land the patch including only what you said.
if (!visible_ || output_surface_state_ != OUTPUT_SURFACE_ACTIVE)
   return false;

[brianderson, 2013-06-25 18:36:15]

When OutputSurface::InitializeBeginFrameEmulation is called, what is the value
of throttle_frame_production on your machine?

throttle_frame_production being true is the only reason I can see that would
cause your trace buffer to get flooded.

If you force it to false, does that improve things for you?

If forcing it to false helps, can you add and hook up the
throttle_frame_production to SchedulerSettings. Then have
ProactiveBeginFrameWantedByImplThread also return false when
throttle_frame_production is true?

[brianderson, 2013-06-25 18:39:01]

Note: Please invert true/false in my previous comments regarding
throttle_frame_production.

[dshwang, 2013-06-25 18:52:24]

On 2013/06/25 18:36:15, Brian Anderson wrote:
> When OutputSurface::InitializeBeginFrameEmulation is called, what is the value
> of throttle_frame_production on your machine?
> 
> throttle_frame_production being true is the only reason I can see that would
> cause your trace buffer to get flooded.
> 
> If you force it to false, does that improve things for you?
> 
> If forcing it to false helps, can you add and hook up the
> throttle_frame_production to SchedulerSettings. Then have
> ProactiveBeginFrameWantedByImplThread also return false when
> throttle_frame_production is true?

In my machine, throttle_frame_production is always false.

It's very helpful, because OnTimerTick is called with 16ms interval now.

Ok, I'll submit the patch soon.

[brianderson, 2013-06-25 19:38:39]

lgtm with one nit.

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

https://codereview.chromium.org/17587014/diff/23001/cc/scheduler/scheduler_st...
cc/scheduler/scheduler_state_machine.cc:367: // Be not proactive when invisible.
nit: // Do not be proactive when invisible.

[commit-bot: I haz the power, 2013-06-26 05:58:36]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dongseong.hwang@intel.com/17587014/29001

[dshwang, 2013-06-26 05:58:41]

On 2013/06/25 19:38:39, Brian Anderson wrote:
> lgtm with one nit.

thx for lgtm :)

[commit-bot: I haz the power, 2013-06-26 08:12:38]

Change committed as 208662