LayerTreeHostImpl::OnDraw is really late to realize there's nothing to draw. We should know there's no actual damage way before that in the paint process when the display list comes back the same.

I think damage can come from more than just display list update. it could also just be from layer geometry changing (animations), visible tiles finished rastered, and can also come from OnDraw parameters (supplied by android compositor) changing.

fwiw I'm still brainstorming what's the best way forward here. this has more to do with cc and android compositor integration than anything else. nothing great or concrete yet..

esprehn: In the Blink and ui integrations with cc, if nothing is changed during an update then cc aborts the commit.  If the pipeline gets to drawing and damage calculations, then some code has made some kind of update that might make it to the screen.  This bug sounds like it's about aborting at that part of the pipeline properly when possible.

enne@ Yeah, the bug came from folks using WebView to build browsers saying that we issue invalidations to the Android framework even when there's nothing to draw. At the point at which we finally abort it's too late for Android to abort so they do a big redraw of the same content which uses a lot of power.

At what point do we send the invalidation up to the Android framework? The no damage optimization discussed here is very late in the system for many types of changes which is what I was commenting on. For main thread invalidations that are actually off screen perhaps we can abort in a different way and avoid reporting the invalidations. Also how precise are we with the invalidation rects sent to Android and does it matter?

> At what point do we send the invalidation up to the Android framework?

ScheduledActionInvalidateCompositorFrameSink: https://cs.chromium.org/chromium/src/cc/trees/proxy_impl.cc?rcl=bbf5af70f469a5f7807ec4573a65c3710cfbb29a&l=544

It usually happens inside BeginImplFrame.

If folks want to understand the webview constraints here, maybe we can find a meeting room with a whiteboard.. I don't think I can do it easily over text.

Also I'd minimize webview-only code paths in cc. Those historically don't end well, because folks tend to forget webview exists.

Brain dump / discussion. Typing this out helps with thought process a bit :p

Very high level summary of constraints. CC cannot decide whether a frame has damage until all the inputs to the frame has stopped changing. In the android compositor, input to frame may still change *after* the last point to decide whether to start a frame. So something has to give here.

There are two source of input after "decision":
1) computeScroll, aka ticking root layer fling animation. This is easy to deal with, we know if there is a fling animation upfront, so can use it to decide.
2) app directly setting root layer scroll offset (or other things). There is no way around this one, but losing a frame of latency of this is unavoidable in this case anyway in other cases, so hopefully it will not be bad, or break apps. Otherwise this is a show stopper.

Beyond that, I think the tradeoff is between latency and throughput.

Idea one: Check with cc if there is damage so far to decide
* latency: no issues afaict
* throughputs: Potentially bad. This check is additional work per frame per drawing webview, on the critical path, ie UI thread has to wait for this with a synchronous IPC. So it all depends on how expensive that check is. If this is almost as expensive as OnDraw itself (not counting unrelated things like prepare tiles), then this could be a lot slower than today.

Idea two: Use what's available about that frame on the UI thread to decide
* throughput: better than current state, since UI don't need to wait on renderer. And this is the only reason why I'm even entertaining this idea
* latency: things from renderer side gains one frame of latency
Blink never wins the race with UI in webview; effectively blink's frame deadline is essentially 0. So is always in high latency mode, and I don't this change make blink worse. Although the dream of maybe having a non-0 deadline in webview is definitely out.
cc animations will be worse, but.. that's ok since animations are timed to the clock, and it will only miss the first frame?
Notably, input should *not* be worse, because UI knows whether there was input that frame, although it doesn't know whether input will cause damage
Anything else..?

Hmm, thinking a bit more, Idea one is likely a prerequisite for Idea two.., in case checking damage is too expensive

Some thoughts about this, if cc has an independent damage checking procedure and damage checking can use last cached states, its cost is acceptable (mostly <1ms), and do not affect scheduler's main/impl frame scheduling states.

1, New pending frame activation: check damage after activation, if no damage then no invalidate;
2, cc Animation, post vsync first after last onDraw, check damage during vsync callback, if no damage then no invalidate;  

Some situation we can skip independent damage checking and go back to normal procedure to improve the performance.

1, Has fling scrolling;
2, Last pending frame activation has damage (go checking in 2nd frame);
3, Last cc Animation frame has damage (go checking in 2nd frame);

2, cc Animation, post vsync first after last onDraw, check damage during vsync callback, if no damage then no invalidate, instead of, post another vsync callback to do damage checking again; 

1, Has user scroll, fling scroll, zoom in/out;
2, Last pending frame activation has damage (go checking in 2nd no damage frame);
3, Last cc Animation frame has damage (go checking in 2nd no damage frame);

happened to glance at this bug again, response to #7-11:

Hmm, yeah, I guess that is the logical conclusion to querying damage outside of draw in cc. And it looks absolutely terrifying to me: it's super brittle, hard to maintain, and hard to reason about.

Good thing is, if we check damage in BeginFrame, then almost all the things you listed do not matter. Only exception, as I said earlier, is fling scroll.

I think maybe the first step here should be checking if we can safely break the android view contract of when fling gets ticked, and see what apps (other than google search) breaks.

I don't have time in the next little while to work on this though..

enne/danakj/sunnyps: jamwalla is working on this right now

As a reminder, the has_no_damage optimization in the renderer layer compositor does not have much of an effect in webview, because by the time OnDraw happens, enough things already happened that android will eventually swap no matter what happens in OnDraw.

We will need something like LayerTreeFrameSinkClient::HasDamage(). It should return whether a frame will be produced by OnDraw right now. But hopefully it will be significantly cheaper than OnDraw. Note it doesn't need to be perfect. False positive is somewhat ok, since it's just the status quo. False negative would be bad though, since that could actually break things.

Any suggestions on how to go about doing that? I imagine need to update draw properties, then go through the layer tree and ask each layer if it has damage in the current viewport? Doesn't sound super cheap, but might be best possible here? Other ideas?


Currently, in a local experiment, jamwalla tried moving OnDraw to much earlier in the frame, at the cost of significantly worse scheduling (and loads of bugs), so that webview can actually decide whether to invalidate depending on if frame is produced. Seem to mostly work as expected.

> We will need something like LayerTreeFrameSinkClient::HasDamage(). 

This would be called after the BeginImplFrame right, but before the deadline occured (ie in the first half of the impl frame)? That puts it after input has been delivered?

The goal of https://bugs.chromium.org/p/chromium/issues/detail?id=517253 and https://bugs.chromium.org/p/chromium/issues/detail?id=394562 is to do CalculateDrawProperties and such at the front of BeginImplFrame. If that happened (with some way to deal with webview sending "more inputs" ie fling animation later.. and redo all that work) then after BeginImplFrame happened you could just ask if there is damage.

> Any suggestions on how to go about doing that? I imagine need to update draw properties, then go through the layer tree and ask each layer if it has damage in the current viewport? Doesn't sound super cheap, but might be best possible here? Other ideas?

To tell if there is damage you'd need to aggregate damage up to the root as we do in CalculateRenderPasses: https://cs.chromium.org/chromium/src/cc/trees/layer_tree_host_impl.cc?rcl=cbbdc4c297983084da57c445bf442296f5379b5e&l=825

This isn't valid unless you've computed draw properties, neither of which is cheap, so you don't wanna do them multiple times per frame if you can help it.

When there's a main frame, if it commits+activates inside the impl frame then we have to throw away the work done at the front of the frame. So we'd wanna predict (low/high latency mode) and do work at the front of the frame appropriately. But if we defer it for low latency mode it would not be available for you, so it sounds like high latency mode being sticky is good, other than forcing 2 trips through all this work for every main frame.. poor batteries. Since webview has a 0 deadline, maybe it's not possible for a commit+activation to occur inside the impl frame, as we go immediately to the deadline and making quads? This would make non-0 deadline and non-high-latency mode conflict, unless we could delay this feedback to WebView until the start of the deadline still.

Does any of this help? FWIW https://bugs.chromium.org/p/chromium/issues/detail?id=625689 is done, so input should be vsync aligned now, but we haven't tried to move any work in the compositor. But "Note that the feature was disabled in WebView due to compatibility issues." It'd be worth to confirm that there are not cases where non-vsync-aligned would make it to the compositor still, just to double check.

> This would be called after the BeginImplFrame right, but before the deadline occured (ie in the first half of the impl frame)? That puts it after input has been delivered?

Yes. Going to need to call it immediately after (webview's equivalent of?) BeginImplFrame, as that's the only synchronous call from the UI thread.

> The goal of https://bugs.chromium.org/p/chromium/issues/detail?id=517253 and https://bugs.chromium.org/p/chromium/issues/detail?id=394562 is to do CalculateDrawProperties and such at the front of BeginImplFrame. If that happened (with some way to deal with webview sending "more inputs" ie fling animation later.. and redo all that work) then after BeginImplFrame happened you could just ask if there is damage.

Ohh, exciting.

Fling is actually easy to deal with. cc already knows whether it needs fling ticks, so can just check that inside BeginImplFrame. The thing that's actually a problem is app directly changing scroll after BeginImplFrame. Those are rare though.

There are other things such as all the parameters passed to LTHI::OnDraw, but those are only there to support webview's weird software draw requirements. Those should never change after BeginImplFrame for "normal" hardware frames.

But I think overall, yes, if one day cc moves CalculateDrawProperties to BeginImplFrame, then there probably needs a bunch of conditions webview where that work should be skipped in BeginImplFrame in and wait until OnDraw

> Since webview has a 0 deadline, maybe it's not possible for a commit+activation to occur inside the impl frame, as we go immediately to the deadline and making quads?

Webview's deadline is a real fuzzy concept. It was immediate making quads back when compositor thread and UI thread were merged. But now that compositor thread is spun off, there actually is a tiny chance for a commit+activate to go through. Timing depends on if app is doing any expensive layouts in its java views; most apps don't do complex things, so in practice, deadline is very tiny.

> Since webview has a 0 deadline, maybe it's not possible for a commit+activation to occur inside the impl frame, as we go immediately to the deadline and making quads?

Webview still requires BeginImplFrame and making quads to be separate steps. It's still possible for BeginImplFrame to happen, but OnDraw to be skipped.

> This would make non-0 deadline and non-high-latency mode conflict, unless we could delay this feedback to WebView until the start of the deadline still.

Maybe webview needs an "always in high latency mode" in cc?

> But "Note that the feature was disabled in WebView due to compatibility issues." It'd be worth to confirm that there are not cases where non-vsync-aligned would make it to the compositor still, just to double check.

Real touch events are aligned by android already, so for most cases, this isn't a huge deal. That webview-only "tick root fling" thing will never be aligned, unless we break that api altogether.


So there is a real alignment here. Yay. Webview will always have to deal with changing inputs, and thus redoing CalculateRenderPasses. But I believe it should not be necessary for apps that don't mess around with webview frames. I'm more worried about moving a chunk of work from OnDraw to BeginImplFrame. BeginImplFrame is a synchronous call from UI thread whereas OnDraw (most of the time) is not. But not gonna know until we try I guess.

Is there any specific timeline for this work on desktop? Definitely don't want to do conflicting work. I think for now, factoring out UpdateDrawProperties/CalculateRenderPasses into its own independent call would be helpful for both, right? Maybe jamwalla@ can start on that right now.

> Is there any specific timeline for this work on desktop? Definitely don't want to do conflicting work.

There is no one actively working on it right now, so no timeline.

> I think for now, factoring out UpdateDrawProperties/CalculateRenderPasses into its own independent call would be helpful for both, right?

Making it easier to move the call into BeginImplFrame sounds good. CalculateRenderPasses shouldn't move, that is the step at the end of the deadline. But UpdateDrawProperties and DamageTracker::UpdateDamageTracking could happen earlier, these are both inputs to CalcRenderPasses.

> Maybe jamwalla@ can start on that right now.

Sure :)

> I'm more worried about moving a chunk of work from OnDraw to BeginImplFrame. BeginImplFrame is a synchronous call from UI thread whereas OnDraw (most of the time) is not. But not gonna know until we try I guess.

Had another idea to help with that. We can use whether the previous frame had damage to as a hint whether to check in BeginImplFrame.

It's always correct to just assume the frame has damage in BeginImplFrame and pay the cost of the unnecessary swap if assumption turned out to be wrong (status quo right now). So we should pay the cost of double checking in BeginImplFrame only if we have reason to believe that it will actually return no damage. And a pretty good hint is whether the previous frame had no damage.

This only works for a continuous chain of no damage frames, and I think that covers all the cases where this applies: damage that's slightly offscreen like animation or video, or input that doesn't generate damage like trying to scroll past scroll bounds.

I'm working on pulling a damage check out of CalculateRenderPasses (currently, the damage check at CalculateRenderPasses::EmptyDamageRect). We want to use the result of performing this damage check at the beginning of the frame. One case is that the result of this damage check is no longer valid if needs_update_draw_properties_ is true. Are there other cases where has_no_damage is no longer valid?

Also - in PrepareToDraw, what does ReconcileElasticOverscrollAndRootScroll do?

> Are there other cases where has_no_damage is no longer valid?

That will be invalid if any input is changed to the computation. To find that we'd have to look thru all the inputs and see where they come from. Damage is based on the draw properties computed by UpdateDrawProperties. num_contributors for example, if looking in code search, is updated from UpdateDrawProperties too. The frame sink capabilities are const, so they only change when the frame sink does, which causes draw properties to be invalidated also in LTHI::InitializeRenderer(). I think there are a few more inputs there, that you could look at and see what you find?

> Also - in PrepareToDraw, what does ReconcileElasticOverscrollAndRootScroll do?

Unfortunately there's no comment: https://cs.chromium.org/chromium/src/cc/input/input_handler.h?rcl=22a9493a44ed64d34caab9e1a579c83c06d4a914&l=61

git blame from codesearch shows it was added in https://chromium.googlesource.com/chromium/src/+/a8a42b25ed0742092fa94eae94c27280043f5cb5

lmk if that isn't enough breadcrumbs.

Thanks!

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/4d3dff9b366d0535b8c08eeb71636dd198ad30bd

commit 4d3dff9b366d0535b8c08eeb71636dd198ad30bd
Author: James Wallace-Lee <jamwalla@chromium.org>
Date: Thu Nov 02 18:12:54 2017

WebView: factor out HasDamage

Factor out the damage check performed in CalculateRenderPasses.

BUG= 687695 

Cq-Include-Trybots: master.tryserver.blink:linux_trusty_blink_rel
Change-Id: Iea91adc92fc4a30a9458de2f28d4d28f6d092ee6
Reviewed-on: https://chromium-review.googlesource.com/741001
Reviewed-by: danakj <danakj@chromium.org>
Reviewed-by: Bo <boliu@chromium.org>
Commit-Queue: James Wallace-Lee <jamwalla@chromium.org>
Cr-Commit-Position: refs/heads/master@{#513553}
[modify] https://crrev.com/4d3dff9b366d0535b8c08eeb71636dd198ad30bd/cc/trees/layer_tree_host_impl.cc
[modify] https://crrev.com/4d3dff9b366d0535b8c08eeb71636dd198ad30bd/cc/trees/layer_tree_host_impl.h
[modify] https://crrev.com/4d3dff9b366d0535b8c08eeb71636dd198ad30bd/cc/trees/layer_tree_impl.h

This doesn't feel like a P1, dropping priority.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/aefa586e7c8a20173fac5a4b8dfa301f8460e852

commit aefa586e7c8a20173fac5a4b8dfa301f8460e852
Author: James Wallace-Lee <jamwalla@chromium.org>
Date: Thu Dec 21 16:40:07 2017

cc: don't ProcessScheduledActions inside BeginImplFrame

Calls from Scheduler to SchedulerClient should not call Scheduler::ProcessScheduledActions or
otherwise reenter a Scheduler action. To prevent this, we add a boolean that we mark as true
to indicate that we are inside of a Scheduler action, before making a call to SchedulerClient.
We DCHECK that this boolean is false at the beginning of Scheduler actions, and exit
ProcessScheduledActions early if it is true.

The test demonstrates that WillBeginImplFrame doesn't end up calling any other SchedulerClient
method.

Bug:  687695 
Cq-Include-Trybots: master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.android:android_optional_gpu_tests_rel
Change-Id: I7e3ed5a1862ba91ab434abe151e3e10cbe5c1e8f
Reviewed-on: https://chromium-review.googlesource.com/806642
Commit-Queue: James Wallace-Lee <jamwalla@chromium.org>
Reviewed-by: danakj <danakj@chromium.org>
Cr-Commit-Position: refs/heads/master@{#525713}
[modify] https://crrev.com/aefa586e7c8a20173fac5a4b8dfa301f8460e852/cc/scheduler/scheduler.cc
[modify] https://crrev.com/aefa586e7c8a20173fac5a4b8dfa301f8460e852/cc/scheduler/scheduler.h
[modify] https://crrev.com/aefa586e7c8a20173fac5a4b8dfa301f8460e852/cc/scheduler/scheduler_unittest.cc

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/e71cf5898635a049b9cc9317f906207fe859c0c5

commit e71cf5898635a049b9cc9317f906207fe859c0c5
Author: James Wallace-Lee <jamwalla@chromium.org>
Date: Mon Jan 29 22:24:23 2018

WebView: Abort draw if no damage

This CL adds the --cc-check-damage-early flag. If the flag is enabled, an
additional call to HasDamage is made in LayerTreeHostImpl::WillBeginImplFrame.
If that check returns false, we can abort the frame early, which on WebView
prevents the compositor from needlessly drawing the frame.


Bug:  687695 
Cq-Include-Trybots: master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.android:android_optional_gpu_tests_rel
Change-Id: Iaae6749b40b09762b2ac581f784a7e6937d20229
Reviewed-on: https://chromium-review.googlesource.com/767851
Reviewed-by: Bo <boliu@chromium.org>
Reviewed-by: Steven Bennetts <stevenjb@chromium.org>
Reviewed-by: Pavel Feldman <pfeldman@chromium.org>
Reviewed-by: danakj <danakj@chromium.org>
Commit-Queue: James Wallace-Lee <jamwalla@chromium.org>
Cr-Commit-Position: refs/heads/master@{#532627}
[modify] https://crrev.com/e71cf5898635a049b9cc9317f906207fe859c0c5/cc/base/switches.cc
[modify] https://crrev.com/e71cf5898635a049b9cc9317f906207fe859c0c5/cc/base/switches.h
[modify] https://crrev.com/e71cf5898635a049b9cc9317f906207fe859c0c5/cc/scheduler/scheduler.cc
[modify] https://crrev.com/e71cf5898635a049b9cc9317f906207fe859c0c5/cc/scheduler/scheduler.h
[modify] https://crrev.com/e71cf5898635a049b9cc9317f906207fe859c0c5/cc/scheduler/scheduler_unittest.cc
[modify] https://crrev.com/e71cf5898635a049b9cc9317f906207fe859c0c5/cc/test/layer_tree_test.cc
[modify] https://crrev.com/e71cf5898635a049b9cc9317f906207fe859c0c5/cc/test/test_hooks.h
[modify] https://crrev.com/e71cf5898635a049b9cc9317f906207fe859c0c5/cc/trees/layer_tree_host_impl.cc
[modify] https://crrev.com/e71cf5898635a049b9cc9317f906207fe859c0c5/cc/trees/layer_tree_host_impl.h
[modify] https://crrev.com/e71cf5898635a049b9cc9317f906207fe859c0c5/cc/trees/layer_tree_host_unittest.cc
[modify] https://crrev.com/e71cf5898635a049b9cc9317f906207fe859c0c5/cc/trees/layer_tree_impl.cc
[modify] https://crrev.com/e71cf5898635a049b9cc9317f906207fe859c0c5/cc/trees/layer_tree_impl.h
[modify] https://crrev.com/e71cf5898635a049b9cc9317f906207fe859c0c5/cc/trees/layer_tree_settings.h
[modify] https://crrev.com/e71cf5898635a049b9cc9317f906207fe859c0c5/cc/trees/proxy_impl.cc
[modify] https://crrev.com/e71cf5898635a049b9cc9317f906207fe859c0c5/cc/trees/proxy_impl.h
[modify] https://crrev.com/e71cf5898635a049b9cc9317f906207fe859c0c5/cc/trees/single_thread_proxy.cc
[modify] https://crrev.com/e71cf5898635a049b9cc9317f906207fe859c0c5/cc/trees/single_thread_proxy.h
[modify] https://crrev.com/e71cf5898635a049b9cc9317f906207fe859c0c5/chrome/browser/chromeos/login/chrome_restart_request.cc
[modify] https://crrev.com/e71cf5898635a049b9cc9317f906207fe859c0c5/content/browser/renderer_host/render_process_host_impl.cc
[modify] https://crrev.com/e71cf5898635a049b9cc9317f906207fe859c0c5/content/renderer/gpu/render_widget_compositor.cc

📍 Pinpoint job started.
https://pinpoint-dot-chromeperf.appspot.com/job/15b133ea840000

📍 Pinpoint job started.
https://pinpoint-dot-chromeperf.appspot.com/job/11b3161a840000

to make this optimization more effective, we should pull prepare tiles out of invalidate/draw in sync compositor: https://cs.chromium.org/chromium/src/cc/scheduler/scheduler_state_machine.cc?rcl=39745234e3a9654e108105cd5512d407a2eeaa41&l=567

right now, an offscreen css animation don't generate any damage, but it generally requires updating tiles again, so webview ends up in a situation that is doing real egl swaps at 60fps just to do prepare tiles for things that are offscreen

so I think we should allow prepare tiles outside of draw if that frame has no damage, however, would like to have it not be inside the BeginFrame call exactly, since that's a blocking call from the UI thread.

sunnyps: that line bears a TODO to something like that under your name :) any thoughts here?

Our goal is I think to put it at WillBeginImplFrame, so that at the start of a frame we can prepare the tiles we will draw at the end of that frame.

The preparetiles call itself is supposed to be "cheap" tho it depends on UpdateDrawProperties - if anything changed - which is less cheap maybe.

If it's not in the BeginFrame then it wouldn't be happening after input arrived so then it doesn't represent the world.

I might not be understanding the proposal well here?

> If it's not in the BeginFrame then it wouldn't be happening after input arrived so then it doesn't represent the world.

We've got begin frame aligned input now right? Although this still applies to other things that can lead to prepare tiles.

Maybe a better way is this is to not block the UI thread PrepareTile work, ie send the result back after damage check.

> I might not be understanding the proposal well here?

No solid proposal, just an idea. This is just from me playing around with enabling that CL for a few minutes, and noticed webview still does worse than chrome in some situations.

> ie send the result back after damage check.

err, send the reply IPC to unblock browser after damage check, but before prepare tiles

That sounds good, keep the ui thread unblocked, and productive ordering in the renderer for the frame.

📍 Couldn't reproduce a difference.
https://pinpoint-dot-chromeperf.appspot.com/job/15b133ea840000

📍 Couldn't reproduce a difference.
https://pinpoint-dot-chromeperf.appspot.com/job/11b3161a840000

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/65606b095c8ad691ea846d81b5216ee6c7103471

commit 65606b095c8ad691ea846d81b5216ee6c7103471
Author: James Wallace-Lee <jamwalla@chromium.org>
Date: Fri Feb 16 23:06:33 2018

WebView: Enable early damage check if any of the last 3 frames had no damage

This logic happens behind the flag --cc-check-damage-early.

If any of the last 3 frames had no damage, we enable the damage check in
WillBeginImplFrame from crrev.com/c/767851. This is described further here:
https://docs.google.com/document/d/1Onv9teMJMyT63-8NywNQfaYY2PRTujvqI9Xa9jGcXvY

Bug:  687695 
Cq-Include-Trybots: master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.android:android_optional_gpu_tests_rel
Change-Id: Ie77abd0f587dc9815012bde87d5d0354a2f9cbbf
Reviewed-on: https://chromium-review.googlesource.com/854991
Reviewed-by: Avi Drissman <avi@chromium.org>
Reviewed-by: danakj <danakj@chromium.org>
Commit-Queue: James Wallace-Lee <jamwalla@chromium.org>
Cr-Commit-Position: refs/heads/master@{#537445}
[modify] https://crrev.com/65606b095c8ad691ea846d81b5216ee6c7103471/cc/trees/layer_tree_host_impl.cc
[modify] https://crrev.com/65606b095c8ad691ea846d81b5216ee6c7103471/cc/trees/layer_tree_host_impl.h
[modify] https://crrev.com/65606b095c8ad691ea846d81b5216ee6c7103471/cc/trees/layer_tree_host_unittest.cc
[modify] https://crrev.com/65606b095c8ad691ea846d81b5216ee6c7103471/cc/trees/layer_tree_settings.h
[modify] https://crrev.com/65606b095c8ad691ea846d81b5216ee6c7103471/content/renderer/gpu/render_widget_compositor.cc

Found a bug. See trace that's just spinning an empty begin frame. I added SchedulerStateMachine::OnBeginImplFrame and SchedulerStateMachine::WillDrawInternal events locally.

The problem is AbortDraw calls WillDrawInternal, which sets did_draw_in_last_frame_ to true, which makes ProactiveBeginFrameWanted true, which starts the cycle again.

Can't ship that to stable.

Deleted: trace_begin_frame_spin.json.gz 97.1 KB

trace_begin_frame_spin.json.gz 97.1 KB Download

AbortDraw setting did_draw_in_last_frame_ to seems totally backwards to me. Maybe the fix is to just don't do that?

AbortDraw doing WillDraw sounds wrongish, esp ProactiveBeginFrameWanted as a result.

Confirmed that setting did_draw_in_last_frame_ was the problem. WillDrawInternal and DidDraw internal are protected inside state_machine, tho; should I write a new state machine abort function for this use case?

probably ok to just make sure SchedulerStateMachine::AbortDraw doesn't set did_draw_in_last_frame_. Everything else (including setting did_draw_) probably makes sense because it's going through the motions of doing a draw without actually doing anything.

GPU triage: I believe 66 has branched or is about to branch. Should this be reverted on the branch, or is there an easy fix?

there is an easy fix: https://chromium-review.googlesource.com/c/chromium/src/+/933292

📍 Pinpoint job started.
https://pinpoint-dot-chromeperf.appspot.com/job/129f75ca440000

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/8ed5395dbf97d328142a5da56ce39ca6b1f78cc4

commit 8ed5395dbf97d328142a5da56ce39ca6b1f78cc4
Author: James Wallace-Lee <jamwalla@chromium.org>
Date: Fri Mar 09 21:19:38 2018

Don't set did_draw_in_last_frame_ in AbortDraw

In webview, frames with no damage sometimes call AbortDraw to prevent
draws. These aborts should not set did_draw_in_last_frame_ or the
scheduler will request new BeginFrames when it doesn't need to.

This CL stops setting did_draw_in_last_frame_ in AbortDraw while still
setting it in WillDraw.

Bug:  687695 
Cq-Include-Trybots: master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.android:android_optional_gpu_tests_rel
Change-Id: I84c39a5e06d578ea92558d81ce2fad5dc423cf75
Reviewed-on: https://chromium-review.googlesource.com/933292
Commit-Queue: James Wallace-Lee <jamwalla@chromium.org>
Reviewed-by: danakj <danakj@chromium.org>
Cr-Commit-Position: refs/heads/master@{#542229}
[modify] https://crrev.com/8ed5395dbf97d328142a5da56ce39ca6b1f78cc4/cc/scheduler/scheduler_state_machine.cc
[modify] https://crrev.com/8ed5395dbf97d328142a5da56ce39ca6b1f78cc4/cc/scheduler/scheduler_unittest.cc

#55 will need to be merged to m66, after some baking on trunk

Request merging https://chromium-review.googlesource.com/933292 to 66. That CL also fixes crbug.com/814678

Merge approved upon verification fo the fix in canary

I checked 67.0.3368.0 manually

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/ff61a83060d15b2ff9bb1136656498b2233c7ff4

commit ff61a83060d15b2ff9bb1136656498b2233c7ff4
Author: James Wallace-Lee <jamwalla@chromium.org>
Date: Tue Mar 13 00:32:20 2018

[Merge M66] Don't set did_draw_in_last_frame_ in AbortDraw

In webview, frames with no damage sometimes call AbortDraw to prevent
draws. These aborts should not set did_draw_in_last_frame_ or the
scheduler will request new BeginFrames when it doesn't need to.

This CL stops setting did_draw_in_last_frame_ in AbortDraw while still
setting it in WillDraw.

Bug:  687695 
Cq-Include-Trybots: master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.android:android_optional_gpu_tests_rel
Change-Id: I84c39a5e06d578ea92558d81ce2fad5dc423cf75
Reviewed-on: https://chromium-review.googlesource.com/933292
Commit-Queue: James Wallace-Lee <jamwalla@chromium.org>
Reviewed-by: danakj <danakj@chromium.org>
Cr-Commit-Position: refs/heads/master@{#542229}(cherry picked from commit 8ed5395dbf97d328142a5da56ce39ca6b1f78cc4)

Bug: 814678
Change-Id: I9ccc9bc6762d7c95958343319ed4f81efcfaeb66
Reviewed-on: https://chromium-review.googlesource.com/959575
Reviewed-by: Bo <boliu@chromium.org>
Cr-Commit-Position: refs/branch-heads/3359@{#184}
Cr-Branched-From: 66afc5e5d10127546cc4b98b9117aff588b5e66b-refs/heads/master@{#540276}
[modify] https://crrev.com/ff61a83060d15b2ff9bb1136656498b2233c7ff4/cc/scheduler/scheduler_state_machine.cc
[modify] https://crrev.com/ff61a83060d15b2ff9bb1136656498b2233c7ff4/cc/scheduler/scheduler_unittest.cc

😿 Pinpoint job stopped with an error.
https://pinpoint-dot-chromeperf.appspot.com/job/129f75ca440000

Previous job on this bug failed due to https://github.com/catapult-project/catapult/issues/4393, retrying now that it's fixed.

📍 Pinpoint job started.
https://chromeperf.appspot.com/job/148f34aac40000

Getting back to #36-39, ie making PrepareTiles no longer invalidate.

So the idea of "doing PrepareTiles in BeginFrame (when there is no damage) but unblocking the UI early" won't actually help performance.

Current webview IPCs are pretty optimized to avoid waiting on sync IPCs, and one of the optimizations is browser side will send asynchronous begin frames to each compositor, and then block on all of them together with a sync IPC per process. (In practice, webview only has one renderer process, which means UI only sends one sync IPC) This means can't really do the "send reply to UI early" trick that we do in draw.

Another way do the same IPC optimization is to use a futures pattern, ie send all the begin frames down at once, and then wait for them individually. But even this implementation isn't very amenable to "early return" when there are multiple webviews drawing. In that case, UI is still going to be blocked for the PrepareTile of all but the last compositor.

So I propose posting a real deadline task to do PrepareTiles in the synchronous compositor case as well. So.. invalidate if there is damage, schedule deadline task if there is no damage. Seems reasonable?

jamwalla tried to prototype using deadline and is running into difficulties
* scheduler doesn't really know what's a good time to schedule PrepareTiles
* also trying to get the deadline code in scheduler to work, but only under some conditions, is turning out to be a big mess

So next step in this proposal: pass up in invalidate whether draw is needed. And embedder can decide when to callback into "OnDraw". Then embedder can callback at a time that's before the next begin frame, but not on the critical path of the current frame.

And if we want to take yet another step: could separate out PrepareTiles (and anything else that's not producing a frame that current happens in OnDraw) into a completely separate call. ie apply the idea above for frames that do have damage as well. Upside is if there are multiple webviews drawing, then PrepareTiles for the first webview will no longer be on the critical path for the other webviews.


One step at a time though.

@danakj, @sunnyps, @enne, (others who know cc/scheduler?) -

I'm working on the approach above (#68) in crrev/c/1040663. In that CL the content/ layer (embedder) can call back into cc/trees/ if it needs PrepareTiles but not a draw, instead of the usual DemandDraw. I'd like to ask the scheduler to do a PrepareTiles if it's needed, something like DidModifyTilePriorities, but which does the usual steps in OnDraw to check the state machine and schedule what's needed.

So - what can I send to the scheduler, that will do everything OnDraw does *except* draw? Do you think that's best achieved through a new call to scheduler, or a new parameter to OnDraw, or something else?

Thank you!

+brianderson, skyostil: as I wrote in #69, I'm looking for what I think would be a new scheduler call that goes through the motions of a draw (esp. PrepareTiles) without actually drawing. I appreciate any suggestions!

boliu probably knows, but doesn't webview do a "fake" draw when using a 1x1 software SkCanvas? That should go through the motions of a draw, right?

The fallback tick wouldn't work here.

This current exercise here is to avoid invalidating the webview (the real one, not FrameSink::Invalidate) if compositor only has needs_prepare_tiles_, but not needs_redraw_. Scheduler doing this on its own is not ideal due to reasons in #68 above. So we tried to have scheduler tell webview in the FrameSink::Invalidate call whether it actually needs to draw; if not, webview can callback down at an appropriate time that's before the next frame, but not on the critical path of the current frame.

The call down can't really be the FrameSinkClient::OnDraw call since webview is not expecting a frame.

Fallback tick might appear to work, but actually not. It's doing a resourceless software draw, and the act of doing such a draw causes damage for the whole viewport, which kind of defeats the purpose of this whole exercise here.

So the behavior for the new call is essentially the draw deadline, except don't do the draw action, even if needs_redraw_ is true.

Can we do PrepareTiles in BeginFrame, and Invalidate only if needs_redraw
is true? BeginFrame is on critical path, but PrepareTiles itself shouldn't
be too expensive. Also, note that PrepareTiles without UpdateDrawProperties
doesn't make a lot of sense because tile priorities won't take changes like
scrolling into account otherwise.

I'm trying to think of scenarios where needs_prepare_tiles would be true
and needs_redraw will be false except for waiting for a pending tree to
activate. Are there others?

> Can we do PrepareTiles in BeginFrame, and Invalidate only if needs_redraw
> is true? BeginFrame is on critical path, but PrepareTiles itself shouldn't
> be too expensive.

Was trying to check how expensive PrepareTiles tend to be. Google search result page and poser circle are about the same, maybe 0.5ms CPU time (on nexus 5). Definitely not trivial, but cheaper than UpdateDrawProperties (about 1ms on), which is already on the critical path if we want to avoid invalidate.

So try to minimize the cost of damage check (ie UpdateDrawProperties) with some heuristic, only check in BeginFrame if previous N frames didn't have damage. Those heuristics will need to expand to incorporate prepare tiles. The heuristic becomes *less* effective as well, we can have an expensive BeginFrame that doesn't on the critical path even webview itself is not invalidating. Kinda hard to judge.. it does seem a lot simpler though.

> Also, note that PrepareTiles without UpdateDrawProperties
doesn't make a lot of sense because tile priorities won't take changes like
scrolling into account otherwise.

We could just call UpdateDrawProperties from PrepareTiles now? If no input changed, then it gets skipped anyway, and if input has changed, well, aren't things kind of wrong today already?

> I'm trying to think of scenarios where needs_prepare_tiles would be true
and needs_redraw will be false except for waiting for a pending tree to
activate. Are there others?

Compositor animations, which I don't know count as activating pending trees as well? The case I found is zooming into the very top of the page of poster circles page, so all the circles flying around are outside of the viewport.

📍 Pinpoint job started.
https://pinpoint-dot-chromeperf.appspot.com/job/149f3952240000

📍 Pinpoint job started.
https://pinpoint-dot-chromeperf.appspot.com/job/14a37442240000

📍 Pinpoint job started.
https://pinpoint-dot-chromeperf.appspot.com/job/14a657f2240000

📍 Pinpoint job started.
https://pinpoint-dot-chromeperf.appspot.com/job/13c84f92240000

📍 Pinpoint job started.
https://pinpoint-dot-chromeperf.appspot.com/job/12d42f12240000

📍 Pinpoint job started.
https://pinpoint-dot-chromeperf.appspot.com/job/15b1f2a2240000

📍 Pinpoint job started.
https://pinpoint-dot-chromeperf.appspot.com/job/14db8552240000

📍 Pinpoint job started.
https://pinpoint-dot-chromeperf.appspot.com/job/15e30802240000

📍 Pinpoint job started.
https://pinpoint-dot-chromeperf.appspot.com/job/16ca35a2240000

📍 Job complete. See results below.
https://pinpoint-dot-chromeperf.appspot.com/job/14a37442240000

📍 Job complete. See results below.
https://pinpoint-dot-chromeperf.appspot.com/job/149f3952240000

😿 Pinpoint job stopped with an error.
https://pinpoint-dot-chromeperf.appspot.com/job/15b1f2a2240000

All of the attempts failed. See the individual attempts for details on each error.

📍 Job complete. See results below.
https://pinpoint-dot-chromeperf.appspot.com/job/14db8552240000

📍 Job complete. See results below.
https://pinpoint-dot-chromeperf.appspot.com/job/15e30802240000

📍 Job complete. See results below.
https://pinpoint-dot-chromeperf.appspot.com/job/14a657f2240000

📍 Job complete. See results below.
https://pinpoint-dot-chromeperf.appspot.com/job/16ca35a2240000

📍 Job complete. See results below.
https://pinpoint-dot-chromeperf.appspot.com/job/13c84f92240000

📍 Job complete. See results below.
https://pinpoint-dot-chromeperf.appspot.com/job/12d42f12240000

Took me a while to page in all details.

Summarizing what's been discussed so far:
1. Original objective: 0 damage frames should not cause webview draws
2. Damage calculation is very late (draw) so we need to move it to the front (begin frame)
3. Damage calculation is too expensive so only do it if last frame (or 3) have no damage
4. The above doesn't solve the original problem because PrepareTiles by itself causes draws
5. Proposed solution of PrepareTiles inside begin frame doesn't work because of how webview interacts with multiple compositors (waits for all?)
6. Next idea: Create another frame sink API that allows webview to request a PrepareTiles if there was no damage (and hence no draw)

My earlier comment #73 was misleading because we only do the non-draw PrepareTiles after we've performed damage calculation in begin frame.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/0352b5909c4e2f75bd591145edd84bf98dae3ad4

commit 0352b5909c4e2f75bd591145edd84bf98dae3ad4
Author: James Wallace-Lee <jamwalla@chromium.org>
Date: Thu Jun 07 18:56:09 2018

Merge SynchronousCompositorProxyMojo into SynchronousCompositorProxy

SynchronousCompositorProxy only uses mojo and no longer uses the old
IPC. Mojo is now the default implementation rather than a subclass of
SynchronousCompositorProxyMojo.

Bug:  687695 
Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel;master.tryserver.blink:linux_trusty_blink_rel
Change-Id: Ieddf50f1441648ba1a88197b690b321ff6f83ba1
Reviewed-on: https://chromium-review.googlesource.com/1062956
Reviewed-by: Avi Drissman <avi@chromium.org>
Reviewed-by: Bo <boliu@chromium.org>
Commit-Queue: James Wallace-Lee <jamwalla@chromium.org>
Cr-Commit-Position: refs/heads/master@{#565364}
[modify] https://crrev.com/0352b5909c4e2f75bd591145edd84bf98dae3ad4/content/renderer/BUILD.gn
[modify] https://crrev.com/0352b5909c4e2f75bd591145edd84bf98dae3ad4/content/renderer/android/synchronous_compositor_proxy.cc
[modify] https://crrev.com/0352b5909c4e2f75bd591145edd84bf98dae3ad4/content/renderer/android/synchronous_compositor_proxy.h
[delete] https://crrev.com/3cfa170758437e45186413312814ac9744152f18/content/renderer/android/synchronous_compositor_proxy_mojo.cc
[delete] https://crrev.com/3cfa170758437e45186413312814ac9744152f18/content/renderer/android/synchronous_compositor_proxy_mojo.h
[modify] https://crrev.com/0352b5909c4e2f75bd591145edd84bf98dae3ad4/content/renderer/input/widget_input_handler_manager.cc

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/fbb0e336a2d6fe1237aecbb9cdfaa59e252857fa

commit fbb0e336a2d6fe1237aecbb9cdfaa59e252857fa
Author: James Wallace-Lee <jamwalla@chromium.org>
Date: Mon Jun 18 23:57:47 2018

AW: Don't draw if PrepareTiles is needed and no damage

In Android WebView, LayerTreeHost::WillBeginImplFrame can check if a
frame will have visible damage. If it doesn't, but tile priorities need
to be updated, webview will call View#Invalidate, because PrepareTiles
happens in OnDraw. WebView will draw, even though there is no damage.

This CL passes up to /content/browser whether or not each invalidation
needs a draw. If no draw is needed, don't post a real invalidate, and
call the new WillSkipDraw to trigger PrepareTiles.

TBR=reveman@chromium.org,yfriedman@chromium.org

Bug:  687695 
Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel;master.tryserver.blink:linux_trusty_blink_rel
Change-Id: Id5c22431af57262e2ef72e57a5a18e5bf677b5da
Reviewed-on: https://chromium-review.googlesource.com/1040663
Commit-Queue: James Wallace-Lee <jamwalla@chromium.org>
Reviewed-by: Dave Tapuska <dtapuska@chromium.org>
Reviewed-by: Bo <boliu@chromium.org>
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Reviewed-by: Sunny Sachanandani <sunnyps@chromium.org>
Cr-Commit-Position: refs/heads/master@{#568240}
[modify] https://crrev.com/fbb0e336a2d6fe1237aecbb9cdfaa59e252857fa/ash/components/fast_ink/fast_ink_view.cc
[modify] https://crrev.com/fbb0e336a2d6fe1237aecbb9cdfaa59e252857fa/cc/scheduler/scheduler.cc
[modify] https://crrev.com/fbb0e336a2d6fe1237aecbb9cdfaa59e252857fa/cc/scheduler/scheduler.h
[modify] https://crrev.com/fbb0e336a2d6fe1237aecbb9cdfaa59e252857fa/cc/scheduler/scheduler_state_machine.cc
[modify] https://crrev.com/fbb0e336a2d6fe1237aecbb9cdfaa59e252857fa/cc/scheduler/scheduler_state_machine.h
[modify] https://crrev.com/fbb0e336a2d6fe1237aecbb9cdfaa59e252857fa/cc/scheduler/scheduler_unittest.cc
[modify] https://crrev.com/fbb0e336a2d6fe1237aecbb9cdfaa59e252857fa/cc/test/fake_layer_tree_frame_sink_client.h
[modify] https://crrev.com/fbb0e336a2d6fe1237aecbb9cdfaa59e252857fa/cc/test/fake_layer_tree_host_impl_client.h
[modify] https://crrev.com/fbb0e336a2d6fe1237aecbb9cdfaa59e252857fa/cc/test/layer_tree_test.cc
[modify] https://crrev.com/fbb0e336a2d6fe1237aecbb9cdfaa59e252857fa/cc/trees/layer_tree_frame_sink.h
[modify] https://crrev.com/fbb0e336a2d6fe1237aecbb9cdfaa59e252857fa/cc/trees/layer_tree_frame_sink_client.h
[modify] https://crrev.com/fbb0e336a2d6fe1237aecbb9cdfaa59e252857fa/cc/trees/layer_tree_host_impl.cc
[modify] https://crrev.com/fbb0e336a2d6fe1237aecbb9cdfaa59e252857fa/cc/trees/layer_tree_host_impl.h
[modify] https://crrev.com/fbb0e336a2d6fe1237aecbb9cdfaa59e252857fa/cc/trees/layer_tree_host_impl_unittest.cc
[modify] https://crrev.com/fbb0e336a2d6fe1237aecbb9cdfaa59e252857fa/cc/trees/layer_tree_host_unittest.cc
[modify] https://crrev.com/fbb0e336a2d6fe1237aecbb9cdfaa59e252857fa/cc/trees/proxy_impl.cc
[modify] https://crrev.com/fbb0e336a2d6fe1237aecbb9cdfaa59e252857fa/cc/trees/proxy_impl.h
[modify] https://crrev.com/fbb0e336a2d6fe1237aecbb9cdfaa59e252857fa/cc/trees/single_thread_proxy.cc
[modify] https://crrev.com/fbb0e336a2d6fe1237aecbb9cdfaa59e252857fa/cc/trees/single_thread_proxy.h
[modify] https://crrev.com/fbb0e336a2d6fe1237aecbb9cdfaa59e252857fa/components/exo/layer_tree_frame_sink_holder.h
[modify] https://crrev.com/fbb0e336a2d6fe1237aecbb9cdfaa59e252857fa/content/browser/android/synchronous_compositor_host.cc
[modify] https://crrev.com/fbb0e336a2d6fe1237aecbb9cdfaa59e252857fa/content/browser/android/synchronous_compositor_host.h
[modify] https://crrev.com/fbb0e336a2d6fe1237aecbb9cdfaa59e252857fa/content/common/input/sync_compositor_messages.h
[modify] https://crrev.com/fbb0e336a2d6fe1237aecbb9cdfaa59e252857fa/content/common/input/synchronous_compositor.mojom
[modify] https://crrev.com/fbb0e336a2d6fe1237aecbb9cdfaa59e252857fa/content/renderer/android/synchronous_compositor_proxy.cc
[modify] https://crrev.com/fbb0e336a2d6fe1237aecbb9cdfaa59e252857fa/content/renderer/android/synchronous_compositor_proxy.h
[modify] https://crrev.com/fbb0e336a2d6fe1237aecbb9cdfaa59e252857fa/content/renderer/android/synchronous_layer_tree_frame_sink.cc
[modify] https://crrev.com/fbb0e336a2d6fe1237aecbb9cdfaa59e252857fa/content/renderer/android/synchronous_layer_tree_frame_sink.h

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/bee41268fc28e181eb165cd84122a1acc19cbae8

commit bee41268fc28e181eb165cd84122a1acc19cbae8
Author: Bo Liu <boliu@chromium.org>
Date: Wed Jun 27 21:04:33 2018

cc: Skip updating viewport/transform in skip draw

If a draw is skipped, these values do not change, and should just be
ignored.

Bug: 854520
Bug:  687695 
Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel;master.tryserver.blink:linux_trusty_blink_rel
Change-Id: I1e9582043116f0a5acb7f9217a73a8379b7ac46f
Reviewed-on: https://chromium-review.googlesource.com/1115762
Reviewed-by: Sunny Sachanandani <sunnyps@chromium.org>
Commit-Queue: Bo <boliu@chromium.org>
Cr-Commit-Position: refs/heads/master@{#570898}
[modify] https://crrev.com/bee41268fc28e181eb165cd84122a1acc19cbae8/cc/trees/layer_tree_host_impl.cc
[modify] https://crrev.com/bee41268fc28e181eb165cd84122a1acc19cbae8/cc/trees/layer_tree_host_impl_unittest.cc

Gonna call this fixed. We've done all we intended for has_no_damage optimization. Scheduling can still be improved, but that has no impact about the optimization.