Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2854163003:
[cc] Plumb BeginFrameAcks through SurfaceManager to DisplayScheduler. (Closed)
|
Created:
3 years, 7 months ago by Eric Seckler Modified:
3 years, 7 months ago CC:
chromium-reviews, qsr+mojo_chromium.org, danakj+watch_chromium.org, viettrungluu+watch_chromium.org, nona+watch_chromium.org, shuchen+watch_chromium.org, jam, yzshen+watch_chromium.org, abarth-chromium, jbauman+watch_chromium.org, Aaron Boodman, darin-cc_chromium.org, piman+watch_chromium.org, kalyank, cc-bugs_chromium.org, James Su, darin (slow to review), yusukes+watch_chromium.org, brianderson, enne (OOO), Sami Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
Description[cc] Plumb BeginFrameAcks through SurfaceManager to DisplayScheduler.
Plumbs the BeginFrameAcks from DidNotProduceFrame and
SubmitCompositorFrame through new SurfaceObserver methods from
CompositorFrameSinks to the DisplayScheduler.
Also replaces the expected surface damage heuristics previously used
by DisplayScheduler with tracking BeginFrame(Ack)s and surface damage.
This is work towards unified BeginFrame acknowledgments, see:
Tracking bug: https://crbug.com/697086
Design doc: http://bit.ly/beginframeacks
BUG=697086, 646774, 678119, 703079, 707513
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_site_isolation
Review-Url: https://codereview.chromium.org/2854163003
Cr-Commit-Position: refs/heads/master@{#474989}
Committed: https://chromium.googlesource.com/chromium/src/+/e356c64f253923983248656d304d8aa471aad5f0
Patch Set 1 #Patch Set 2 : CFSS notifies FrameSinkManager about its frame sink's state. #Patch Set 3 : Track state per surface. #
Total comments: 5
Patch Set 4 : track state in DisplayScheduler rather than Surface #
Total comments: 8
Patch Set 5 : address comments, wait for all surfaces with correct last_args #Patch Set 6 : fix tests, additional tests still missing. #Patch Set 7 : Pass ack via SurfaceDamaged, add back tests. #
Total comments: 13
Patch Set 8 : address nits #Patch Set 9 : fix clang compile error #Depends on Patchset: Messages
Total messages: 53 (24 generated)
Description was changed from ========== [cc] Plumb BeginFrameAcks through SurfaceManager to DisplayScheduler. BUG=697086, 646774, 678119, 703079, 707513 ========== to ========== [cc] Plumb BeginFrameAcks through SurfaceManager to DisplayScheduler. BUG=697086, 646774, 678119, 703079, 707513 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
The CQ bit was checked by eseckler@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
eseckler@chromium.org changed reviewers: + sunnyps@chromium.org
A very unpolished first attempt without tests and any of the necessary cleanup elsewhere (to remove plumbing of acks to the BeginFrameSources and LCSN etc). Sunny, does this approach make sense on a high level, or were you thinking of something different?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
On 2017/05/03 16:23:07, Eric Seckler wrote: > A very unpolished first attempt without tests and any of the necessary cleanup > elsewhere (to remove plumbing of acks to the BeginFrameSources and LCSN etc). > > Sunny, does this approach make sense on a high level, or were you thinking of > something different? I was thinking of something like this: Frame sinks are in one of two states: waiting and ready. 1. ready -> waiting: in CFSSupport::OnBeginFrame and in DidReceiveCompositorFrameAck if last_begin_frame_args.sequence_num != last_begin_frame_ack.sequence_num (the latter handles the deadline at begin frame case) 2. waiting -> ready: on CFSSupport::SubmitCompositorFrame and in BeginFrameDidNotSwap (maybe only if ack.seq_num = args.seq_num) (It might make sense to have a third state, idle, so that ready is the state in between submit frame and draw.) The frame sink state is calculated by CFSSupport which informs FrameSinkManager. DisplayScheduler can look up state and frame sink hierarchy from FrameSinkManager. DisplayScheduler waits until there are no waiting frame sinks in the display's hierarchy.
On 2017/05/05 03:07:54, sunnyps wrote: > On 2017/05/03 16:23:07, Eric Seckler wrote: > > A very unpolished first attempt without tests and any of the necessary cleanup > > elsewhere (to remove plumbing of acks to the BeginFrameSources and LCSN etc). > > > > Sunny, does this approach make sense on a high level, or were you thinking of > > something different? > > I was thinking of something like this: > > Frame sinks are in one of two states: waiting and ready. > 1. ready -> waiting: in CFSSupport::OnBeginFrame and in > DidReceiveCompositorFrameAck if last_begin_frame_args.sequence_num != > last_begin_frame_ack.sequence_num (the latter handles the deadline at begin > frame case) > 2. waiting -> ready: on CFSSupport::SubmitCompositorFrame and in > BeginFrameDidNotSwap (maybe only if ack.seq_num = args.seq_num) > > (It might make sense to have a third state, idle, so that ready is the state in > between submit frame and draw.) > > The frame sink state is calculated by CFSSupport which informs FrameSinkManager. > DisplayScheduler can look up state and frame sink hierarchy from > FrameSinkManager. DisplayScheduler waits until there are no waiting frame sinks > in the display's hierarchy.
Thanks Sunny, this sounds good. I see how it would work for the "normal" DisplayScheduler logic, but how would we integrate the flush_pipeline mode with this? In flush_pipeline mode, we'd need to flush CompositorFrames with the wrong corresponding BeginFrameAck (and run their draw callbacks) and give the CFSS's client a chance to handle the current BeginFrame (frame sinks should only become ready when corresponding CompositorFrame/BeginFrameDidNotSwap was received). Would we implement this flushing in CFSS, if that is who manages/sets the frame sink state? On 2017/05/05 03:07:54, sunnyps wrote: > On 2017/05/03 16:23:07, Eric Seckler wrote: > > A very unpolished first attempt without tests and any of the necessary cleanup > > elsewhere (to remove plumbing of acks to the BeginFrameSources and LCSN etc). > > > > Sunny, does this approach make sense on a high level, or were you thinking of > > something different? > > I was thinking of something like this: > > Frame sinks are in one of two states: waiting and ready. > 1. ready -> waiting: in CFSSupport::OnBeginFrame and in > DidReceiveCompositorFrameAck if last_begin_frame_args.sequence_num != > last_begin_frame_ack.sequence_num (the latter handles the deadline at begin > frame case) I believe an additional condition for the second case is that the CFSS is currently observing BeginFrames and hasn't yet did-not-swap-acked the last BeginFrame, right? > 2. waiting -> ready: on CFSSupport::SubmitCompositorFrame and in > BeginFrameDidNotSwap (maybe only if ack.seq_num = args.seq_num) > > (It might make sense to have a third state, idle, so that ready is the state in > between submit frame and draw.) > > The frame sink state is calculated by CFSSupport which informs FrameSinkManager. > DisplayScheduler can look up state and frame sink hierarchy from > FrameSinkManager. DisplayScheduler waits until there are no waiting frame sinks > in the display's hierarchy. Would SurfaceManager/FrameSinkManager notify DisplayScheduler about FrameSink status changes via the SurfaceObserver path or in a different way?
PTAL :)
sunnyps@chromium.org changed reviewers: + fsamuel@chromium.org
+fsamuel@
The state transitions make sense to me but I'm not sure if this state should be
per frame sink or per surface.
If per surface, we need this state per surface:
{
BeginFrameArgs last_begin_frame_args_
BeginFrameAck last_begin_frame_ack_
ReadyToDrawState = IDLE, PENDING or READY as discussed above
}
Why per surface instead of per frame sink? Because we only care about surfaces
referenced by the root surface. (Resize is a special case where the scheduler
waits until a new root surface id is created but child producers might create
new surface ids that we don't know about yet. This has to be solved by surface
synchronization and is probably out of scope for us.)
One way of approaching this is to extend the SurfaceObserver interface with
these methods:
1. SurfaceDiscarded (so that we don't wait for any destroyed surfaces)
2. SurfaceActivated (equivalent to SubmitCompositorFrame when surface
synchronization is disabled - this is where we do the PENDING -> READY
transition if BeginFrameAck is current)
3. SurfaceDidNotSwap (update BeginFrameAck and do the check)
4. Some way of getting the BeginFrameArgs? is observing begin frame source? Note
that we want to know what begin frame args were delivered to that surface last.
E.g. if a frame sink stops observing begin frames we want to know the last begin
frame args and ack which is different from the BFS's current args.
Then display scheduler can have a map of surface id to the above state and
decide when to draw.
On 2017/05/10 06:27:40, sunnyps wrote:
> +fsamuel@
>
> The state transitions make sense to me but I'm not sure if this state should
be
> per frame sink or per surface.
>
> If per surface, we need this state per surface:
>
> {
> BeginFrameArgs last_begin_frame_args_
> BeginFrameAck last_begin_frame_ack_
> ReadyToDrawState = IDLE, PENDING or READY as discussed above
> }
>
> Why per surface instead of per frame sink? Because we only care about surfaces
> referenced by the root surface. (Resize is a special case where the scheduler
> waits until a new root surface id is created but child producers might create
> new surface ids that we don't know about yet. This has to be solved by surface
> synchronization and is probably out of scope for us.)
>
> One way of approaching this is to extend the SurfaceObserver interface with
> these methods:
> 1. SurfaceDiscarded (so that we don't wait for any destroyed surfaces)
> 2. SurfaceActivated (equivalent to SubmitCompositorFrame when surface
> synchronization is disabled - this is where we do the PENDING -> READY
> transition if BeginFrameAck is current)
> 3. SurfaceDidNotSwap (update BeginFrameAck and do the check)
> 4. Some way of getting the BeginFrameArgs? is observing begin frame source?
Note
> that we want to know what begin frame args were delivered to that surface
last.
> E.g. if a frame sink stops observing begin frames we want to know the last
begin
> frame args and ack which is different from the BFS's current args.
>
> Then display scheduler can have a map of surface id to the above state and
> decide when to draw.
One issue with this is dealing with figuring out the "future" surface hierarchy
before drawing. Currently, this happens only during draw during aggregation, so
DisplayScheduler would have to figure this hierarchy out on the fly while
dealing with new surface / root surface replacements.
Tracking frame sink state is closer to tracking BeginFrameObserver state, but
also works before surfaces are created / CompositorFrames are submitted.
On 2017/05/10 06:27:40, sunnyps wrote:
> +fsamuel@
>
> The state transitions make sense to me but I'm not sure if this state should
be
> per frame sink or per surface.
>
> If per surface, we need this state per surface:
>
> {
> BeginFrameArgs last_begin_frame_args_
> BeginFrameAck last_begin_frame_ack_
> ReadyToDrawState = IDLE, PENDING or READY as discussed above
> }
>
> Why per surface instead of per frame sink? Because we only care about surfaces
> referenced by the root surface. (Resize is a special case where the scheduler
> waits until a new root surface id is created but child producers might create
> new surface ids that we don't know about yet. This has to be solved by surface
> synchronization and is probably out of scope for us.)
>
> One way of approaching this is to extend the SurfaceObserver interface with
> these methods:
> 1. SurfaceDiscarded (so that we don't wait for any destroyed surfaces)
> 2. SurfaceActivated (equivalent to SubmitCompositorFrame when surface
> synchronization is disabled - this is where we do the PENDING -> READY
> transition if BeginFrameAck is current)
> 3. SurfaceDidNotSwap (update BeginFrameAck and do the check)
> 4. Some way of getting the BeginFrameArgs? is observing begin frame source?
Note
> that we want to know what begin frame args were delivered to that surface
last.
> E.g. if a frame sink stops observing begin frames we want to know the last
begin
> frame args and ack which is different from the BFS's current args.
>
> Then display scheduler can have a map of surface id to the above state and
> decide when to draw.
Surfaces can outlive their clients. For example, a child client can crash but
the parent can still refer to its surface. We probably want to track that too so
we know not to expect any response.
BeginFrames are tied to clients not surfaces though, so maybe leaving this on
FrameSinks makes sense? FrameSinkManager knows about reachability from the root.
I'm not sure...
On 2017/05/10 06:40:30, Eric Seckler wrote:
> On 2017/05/10 06:27:40, sunnyps wrote:
> > +fsamuel@
> >
> > The state transitions make sense to me but I'm not sure if this state should
> be
> > per frame sink or per surface.
> >
> > If per surface, we need this state per surface:
> >
> > {
> > BeginFrameArgs last_begin_frame_args_
> > BeginFrameAck last_begin_frame_ack_
> > ReadyToDrawState = IDLE, PENDING or READY as discussed above
> > }
> >
> > Why per surface instead of per frame sink? Because we only care about
surfaces
> > referenced by the root surface. (Resize is a special case where the
scheduler
> > waits until a new root surface id is created but child producers might
create
> > new surface ids that we don't know about yet. This has to be solved by
surface
> > synchronization and is probably out of scope for us.)
> >
> > One way of approaching this is to extend the SurfaceObserver interface with
> > these methods:
> > 1. SurfaceDiscarded (so that we don't wait for any destroyed surfaces)
> > 2. SurfaceActivated (equivalent to SubmitCompositorFrame when surface
> > synchronization is disabled - this is where we do the PENDING -> READY
> > transition if BeginFrameAck is current)
> > 3. SurfaceDidNotSwap (update BeginFrameAck and do the check)
> > 4. Some way of getting the BeginFrameArgs? is observing begin frame source?
> Note
> > that we want to know what begin frame args were delivered to that surface
> last.
> > E.g. if a frame sink stops observing begin frames we want to know the last
> begin
> > frame args and ack which is different from the BFS's current args.
> >
> > Then display scheduler can have a map of surface id to the above state and
> > decide when to draw.
>
> One issue with this is dealing with figuring out the "future" surface
hierarchy
> before drawing. Currently, this happens only during draw during aggregation,
so
> DisplayScheduler would have to figure this hierarchy out on the fly while
> dealing with new surface / root surface replacements.
The future surface hierarchy doesn't matter because only surface ids that the
root surface knows about (transitively) will be drawn. What is important that
the correct frame (synced to the last begin frame) for each surface be drawn.
>
> Tracking frame sink state is closer to tracking BeginFrameObserver state, but
> also works before surfaces are created / CompositorFrames are submitted.
Again, this doesn't really apply because we will always wait for the root
surface first and then we can only draw surfaces that the root surface
references.
On 2017/05/11 01:06:58, sunnyps wrote:
> On 2017/05/10 06:40:30, Eric Seckler wrote:
> > On 2017/05/10 06:27:40, sunnyps wrote:
> > > +fsamuel@
> > >
> > > The state transitions make sense to me but I'm not sure if this state
should
> > be
> > > per frame sink or per surface.
> > >
> > > If per surface, we need this state per surface:
> > >
> > > {
> > > BeginFrameArgs last_begin_frame_args_
> > > BeginFrameAck last_begin_frame_ack_
> > > ReadyToDrawState = IDLE, PENDING or READY as discussed above
> > > }
> > >
> > > Why per surface instead of per frame sink? Because we only care about
> surfaces
> > > referenced by the root surface. (Resize is a special case where the
> scheduler
> > > waits until a new root surface id is created but child producers might
> create
> > > new surface ids that we don't know about yet. This has to be solved by
> surface
> > > synchronization and is probably out of scope for us.)
> > >
> > > One way of approaching this is to extend the SurfaceObserver interface
with
> > > these methods:
> > > 1. SurfaceDiscarded (so that we don't wait for any destroyed surfaces)
> > > 2. SurfaceActivated (equivalent to SubmitCompositorFrame when surface
> > > synchronization is disabled - this is where we do the PENDING -> READY
> > > transition if BeginFrameAck is current)
> > > 3. SurfaceDidNotSwap (update BeginFrameAck and do the check)
> > > 4. Some way of getting the BeginFrameArgs? is observing begin frame
source?
> > Note
> > > that we want to know what begin frame args were delivered to that surface
> > last.
> > > E.g. if a frame sink stops observing begin frames we want to know the last
> > begin
> > > frame args and ack which is different from the BFS's current args.
> > >
> > > Then display scheduler can have a map of surface id to the above state and
> > > decide when to draw.
> >
> > One issue with this is dealing with figuring out the "future" surface
> hierarchy
> > before drawing. Currently, this happens only during draw during aggregation,
> so
> > DisplayScheduler would have to figure this hierarchy out on the fly while
> > dealing with new surface / root surface replacements.
>
> The future surface hierarchy doesn't matter because only surface ids that the
> root surface knows about (transitively) will be drawn. What is important that
> the correct frame (synced to the last begin frame) for each surface be drawn.
True, but we need to consider the root surface's "new" hierarchy before
aggregation / DrawAndSwap, right? For example, at the moment, DisplayScheduler
is only told about Surface damages for surfaces in the last aggregation's
hierarchy (aggregator_->previous_contained_surfaces()), AFAICT.
So we'll probably need some additional logic in DisplayScheduler to deal with
changing hierarchy during the current BeginFrame (because of a new root surface
or because a new compositorframe for a child surface changes its
referenced_surfaces), as this changes the set of surfaces we want to wait for (I
believe?).
> > Tracking frame sink state is closer to tracking BeginFrameObserver state,
but
> > also works before surfaces are created / CompositorFrames are submitted.
>
> Again, this doesn't really apply because we will always wait for the root
> surface first and then we can only draw surfaces that the root surface
> references.
Sounds reasonable. Do we need to consider not-yet-existing referenced surfaces
though (e.g. by considering them "pending")?
PTAL - now tracking a "ProducerState" per Surface. (I'm doing this rather than
setting last_args/last_ack per Surface since it seemed a bit more explicit -
WDYT?)
Thanks! :)
On 2017/05/11 07:30:14, Eric Seckler wrote:
> On 2017/05/11 01:06:58, sunnyps wrote:
> > On 2017/05/10 06:40:30, Eric Seckler wrote:
> > > On 2017/05/10 06:27:40, sunnyps wrote:
> > > > +fsamuel@
> > > >
> > > > The state transitions make sense to me but I'm not sure if this state
> should
> > > be
> > > > per frame sink or per surface.
> > > >
> > > > If per surface, we need this state per surface:
> > > >
> > > > {
> > > > BeginFrameArgs last_begin_frame_args_
> > > > BeginFrameAck last_begin_frame_ack_
> > > > ReadyToDrawState = IDLE, PENDING or READY as discussed above
> > > > }
> > > >
> > > > Why per surface instead of per frame sink? Because we only care about
> > surfaces
> > > > referenced by the root surface. (Resize is a special case where the
> > scheduler
> > > > waits until a new root surface id is created but child producers might
> > create
> > > > new surface ids that we don't know about yet. This has to be solved by
> > surface
> > > > synchronization and is probably out of scope for us.)
> > > >
> > > > One way of approaching this is to extend the SurfaceObserver interface
> with
> > > > these methods:
> > > > 1. SurfaceDiscarded (so that we don't wait for any destroyed surfaces)
> > > > 2. SurfaceActivated (equivalent to SubmitCompositorFrame when surface
> > > > synchronization is disabled - this is where we do the PENDING -> READY
> > > > transition if BeginFrameAck is current)
> > > > 3. SurfaceDidNotSwap (update BeginFrameAck and do the check)
> > > > 4. Some way of getting the BeginFrameArgs? is observing begin frame
> source?
> > > Note
> > > > that we want to know what begin frame args were delivered to that
surface
> > > last.
> > > > E.g. if a frame sink stops observing begin frames we want to know the
last
> > > begin
> > > > frame args and ack which is different from the BFS's current args.
> > > >
> > > > Then display scheduler can have a map of surface id to the above state
and
> > > > decide when to draw.
> > >
> > > One issue with this is dealing with figuring out the "future" surface
> > hierarchy
> > > before drawing. Currently, this happens only during draw during
aggregation,
> > so
> > > DisplayScheduler would have to figure this hierarchy out on the fly while
> > > dealing with new surface / root surface replacements.
> >
> > The future surface hierarchy doesn't matter because only surface ids that
the
> > root surface knows about (transitively) will be drawn. What is important
that
> > the correct frame (synced to the last begin frame) for each surface be
drawn.
> True, but we need to consider the root surface's "new" hierarchy before
> aggregation / DrawAndSwap, right? For example, at the moment, DisplayScheduler
> is only told about Surface damages for surfaces in the last aggregation's
> hierarchy (aggregator_->previous_contained_surfaces()), AFAICT.
>
> So we'll probably need some additional logic in DisplayScheduler to deal with
> changing hierarchy during the current BeginFrame (because of a new root
surface
> or because a new compositorframe for a child surface changes its
> referenced_surfaces), as this changes the set of surfaces we want to wait for
(I
> believe?).
I think we might actually get away without considering new hierarchies, because
new surfaces in the hierarchy should be "ready" to draw when they are added into
the hierarchy.
> > > Tracking frame sink state is closer to tracking BeginFrameObserver state,
> but
> > > also works before surfaces are created / CompositorFrames are submitted.
> >
> > Again, this doesn't really apply because we will always wait for the root
> > surface first and then we can only draw surfaces that the root surface
> > references.
>
> Sounds reasonable. Do we need to consider not-yet-existing referenced surfaces
> though (e.g. by considering them "pending")?
After chatting to Fady, I think the only scenario when a not-yet-existing
surface can be referenced is when a CompositorFrame is activated because of a
SurfaceDependencyTracker deadline (otherwise, all referenced_surfaces should
exist). In that case, I'm not sure if we can get away without waiting for this
not-yet-existing surface - given that the SurfaceDependencyTracker basically
decided that it's unlikely that it is going to receive a CompositorFrame?
https://codereview.chromium.org/2854163003/diff/40001/cc/surfaces/compositor_... File cc/surfaces/compositor_frame_sink_support.cc (right): https://codereview.chromium.org/2854163003/diff/40001/cc/surfaces/compositor_... cc/surfaces/compositor_frame_sink_support.cc:102: if (producer_state_ == Surface::PENDING && I was thinking more along the lines of plumbing this to SurfaceObserver::OnSurfaceDamaged(BeginFrameAck) but only if Surface is activated. Something like this: if (current_surface_ && !current_surface_->HasPendingFrame()) { surface_manager_->SurfaceModified(surface->surface_id(), ack); } DisplayScheduler can do the surface state tracking internally. I think that's better because this is really specific to DisplayScheduler and we should encapsulate it there as much as possible. We can make more changes to it later. https://codereview.chromium.org/2854163003/diff/40001/cc/surfaces/compositor_... cc/surfaces/compositor_frame_sink_support.cc:161: SetProducerState(Surface::READY); Similarly here we already plumb surface activation to SurfaceObserver::OnSurfaceModified. We can tack on the BeginFrameAck to that and do the state tracking in DisplayScheduler. https://codereview.chromium.org/2854163003/diff/40001/cc/surfaces/compositor_... cc/surfaces/compositor_frame_sink_support.cc:238: See my later comments in DisplayScheduler first. If the begin frame ack is compared to the scheduler's begin frame args we don't need this because the scheduler only updates its begin frame args after finishing the previous frame. https://codereview.chromium.org/2854163003/diff/40001/cc/surfaces/display_sch... File cc/surfaces/display_scheduler.h (right): https://codereview.chromium.org/2854163003/diff/40001/cc/surfaces/display_sch... cc/surfaces/display_scheduler.h:92: bool expecting_root_surface_damage_because_of_resize_; DisplayScheduler needs a map<SurfaceId, BeginFrameAck>. OnSurfaceCreated/Destroyed will add/remove entries to the map. OnSurfaceDamaged will update the BeginFrameAck. A surface is ready if its begin frame ack matches the display scheduler's current begin frame args. For v1 we can draw when all surfaces are ready. For regular chrome, this will be an improvement over our current heuristic for common cases even if not completely accurate (waiting for more surfaces than needed). For headless, the display scheduler and cc schedulers will use an indefinite deadline and it should be ok to wait until all surfaces have ack'd. Note that OnSurfaceDamaged is called when the surface is activated (normally or via SurfaceDependencyTracker deadline).
Thank you, Sunny! PTAL :) I think there are two issues with the approach you're suggesting: a) Based on results from our discussion last week, we want to consider Surfaces with pending frames "ready", too (for one, b/c the producer is also CompositorFrameAck-throttled in this case, but fsamuel@ may have more reasons). So tracking through OnSurfaceDamaged() alone doesn't suffice, as it wouldn't be called if a new CompositorFrame is submitted but not activated. b) Comparing a Surface's last_ack to the DisplayScheduler's last BeginFrameArgs alone doesn't suffice, as we wouldn't know whether the Surface's producer has received this BeginFrame (b/c the CFSS may not be observing BeginFrames at the moment). So we need to track last_args per surface to figure out who is/was active. Because of a) and because I'm uncomfortable with "abusing" OnSurfaceDamaged for no-damage acks (this would make DisplayScheduler start observing BeginFrames even if there's no need to, for example), I added SurfaceObserver::OnSurfaceFinishedBeginFrame. Because of b), we also need a way to let the DisplayScheduler know when a Surface's last_args have changed. Thus, I added SurfaceObserver::OnSurfaceReceivedBeginFrame. https://codereview.chromium.org/2854163003/diff/40001/cc/surfaces/display_sch... File cc/surfaces/display_scheduler.h (right): https://codereview.chromium.org/2854163003/diff/40001/cc/surfaces/display_sch... cc/surfaces/display_scheduler.h:92: bool expecting_root_surface_damage_because_of_resize_; On 2017/05/19 03:46:34, sunnyps wrote: > DisplayScheduler needs a map<SurfaceId, BeginFrameAck>. > OnSurfaceCreated/Destroyed will add/remove entries to the map. OnSurfaceDamaged > will update the BeginFrameAck. > > A surface is ready if its begin frame ack matches the display scheduler's > current begin frame args. For v1 we can draw when all surfaces are ready. For > regular chrome, this will be an improvement over our current heuristic for > common cases even if not completely accurate (waiting for more surfaces than > needed). For headless, the display scheduler and cc schedulers will use an > indefinite deadline and it should be ok to wait until all surfaces have ack'd. > > Note that OnSurfaceDamaged is called when the surface is activated (normally or > via SurfaceDependencyTracker deadline). I'm not sure whether you were suggesting this, but I'm now tracking all surfaces in this map (not only those in "our" hierarchy), but am only waiting for those in previous_contained_surfaces.
On 2017/05/19 14:11:42, Eric Seckler wrote: > Thank you, Sunny! PTAL :) > > I think there are two issues with the approach you're suggesting: > > a) Based on results from our discussion last week, we want to consider Surfaces > with pending frames "ready", too (for one, b/c the producer is also > CompositorFrameAck-throttled in this case, but fsamuel@ may have more reasons). > So tracking through OnSurfaceDamaged() alone doesn't suffice, as it wouldn't be > called if a new CompositorFrame is submitted but not activated. Drawing a surface doesn't run the draw callbacks for the pending frame which would unthrottle the producer. Also, the scheduler won't indefinitely for such surfaces, only until the next begin frame. If that's the case can't we pass both begin frame args and ack via OnSurfaceDamaged? I'm not opposed to being explicit by adding new SurfaceObserver methods either so whatever's easier is fine. > b) Comparing a Surface's last_ack to the DisplayScheduler's last BeginFrameArgs > alone doesn't suffice, as we wouldn't know whether the Surface's producer has > received this BeginFrame (b/c the CFSS may not be observing BeginFrames at the > moment). So we need to track last_args per surface to figure out who is/was > active. > > Because of a) and because I'm uncomfortable with "abusing" OnSurfaceDamaged for > no-damage acks (this would make DisplayScheduler start observing BeginFrames > even if there's no need to, for example), I added > SurfaceObserver::OnSurfaceFinishedBeginFrame. > > Because of b), we also need a way to let the DisplayScheduler know when a > Surface's last_args have changed. Thus, I added > SurfaceObserver::OnSurfaceReceivedBeginFrame. > > https://codereview.chromium.org/2854163003/diff/40001/cc/surfaces/display_sch... > File cc/surfaces/display_scheduler.h (right): > > https://codereview.chromium.org/2854163003/diff/40001/cc/surfaces/display_sch... > cc/surfaces/display_scheduler.h:92: bool > expecting_root_surface_damage_because_of_resize_; > On 2017/05/19 03:46:34, sunnyps wrote: > > DisplayScheduler needs a map<SurfaceId, BeginFrameAck>. > > OnSurfaceCreated/Destroyed will add/remove entries to the map. > OnSurfaceDamaged > > will update the BeginFrameAck. > > > > A surface is ready if its begin frame ack matches the display scheduler's > > current begin frame args. For v1 we can draw when all surfaces are ready. For > > regular chrome, this will be an improvement over our current heuristic for > > common cases even if not completely accurate (waiting for more surfaces than > > needed). For headless, the display scheduler and cc schedulers will use an > > indefinite deadline and it should be ok to wait until all surfaces have ack'd. > > > > Note that OnSurfaceDamaged is called when the surface is activated (normally > or > > via SurfaceDependencyTracker deadline). > > I'm not sure whether you were suggesting this, but I'm now tracking all surfaces > in this map (not only those in "our" hierarchy), but am only waiting for those > in previous_contained_surfaces. I think it's better to start with something simple like waiting for all surfaces.
https://codereview.chromium.org/2854163003/diff/60001/cc/surfaces/surface_obs... File cc/surfaces/surface_observer.h (right): https://codereview.chromium.org/2854163003/diff/60001/cc/surfaces/surface_obs... cc/surfaces/surface_observer.h:19: virtual void OnSurfaceCreated(const SurfaceInfo& surface_info) {} This should be a pure abstract class otherwise we might end up violating the style guide (no multiple inheritance except interfaces) for subclasses. https://codereview.chromium.org/2854163003/diff/60001/cc/surfaces/surface_obs... cc/surfaces/surface_observer.h:19: virtual void OnSurfaceCreated(const SurfaceInfo& surface_info) {} Should we add the begin frame args and ack to SurfaceInfo to cover the begin frame when the surface is created? https://codereview.chromium.org/2854163003/diff/60001/cc/surfaces/surface_obs... cc/surfaces/surface_observer.h:29: virtual void OnSurfaceReceivedBeginFrame(const SurfaceId& surface_id, bikeshed nit: OnSurfaceBeginFrame https://codereview.chromium.org/2854163003/diff/60001/cc/surfaces/surface_obs... cc/surfaces/surface_observer.h:35: virtual void OnSurfaceFinishedBeginFrame(const SurfaceId& surface_id, bikeshed nit: OnSurfaceBeginFrameAck
On 2017/05/22 06:46:22, sunnyps wrote: > Drawing a surface doesn't run the draw callbacks for the pending frame which > would unthrottle the producer. Maybe think of it this way: If a producer A's Surface has a pending frame, it is blocked on a frame submission of producer B to some not-yet-existing Surface. Only then can the pending frame activate and unblock A. If B is ready (e.g. because it has submitted a frame to an older surface), we shouldn't continue to wait for A. > Also, the scheduler won't indefinitely for such > surfaces, only until the next begin frame. But there's no reason why we shouldn't trigger an early deadline in the case above, right? > If that's the case can't we pass both > begin frame args and ack via OnSurfaceDamaged? I'm not opposed to being explicit > by adding new SurfaceObserver methods either so whatever's easier is fine. I don't think we can pass the BeginFrameArgs via OnSurfaceDamaged, because we need to find out about the args as soon as they are received (otherwise, we don't know whether the CFSS is observing the BeginFrame and thus whether we should wait for its ack/damage). I'd also like to keep the acks explicit, since that means that the DisplayScheduler doesn't need to start observing a BeginFrame if someone responds with DidNotProduceFrame. > > I'm not sure whether you were suggesting this, but I'm now tracking all > surfaces > > in this map (not only those in "our" hierarchy), but am only waiting for those > > in previous_contained_surfaces. > > I think it's better to start with something simple like waiting for all > surfaces. I'm not sure if that would suffice. Since all Displays share a single SurfaceManager, we would be trying to wait for surfaces on a possibly different BeginFrameSource hierarchy. (And with the headless use case, these hierarchies should operate independently.) Maybe we could only check for all Surfaces where last_begin_frame_args.source_id is equal to the source_id of the DisplayScheduler's BeginFrameSource (see new patch set)? I think that should work since we use separate BeginFrameSources for different Displays AFAICT. https://codereview.chromium.org/2854163003/diff/60001/cc/surfaces/surface_obs... File cc/surfaces/surface_observer.h (right): https://codereview.chromium.org/2854163003/diff/60001/cc/surfaces/surface_obs... cc/surfaces/surface_observer.h:19: virtual void OnSurfaceCreated(const SurfaceInfo& surface_info) {} On 2017/05/22 06:46:38, sunnyps wrote: > Should we add the begin frame args and ack to SurfaceInfo to cover the begin > frame when the surface is created? We could, but it's not really relevant in that situation, as SurfaceCreated is only called when a CompositorFrame was submitted (and thus, a newly created surface is always ready to draw). Let's add it later if we end up needing it? https://codereview.chromium.org/2854163003/diff/60001/cc/surfaces/surface_obs... cc/surfaces/surface_observer.h:19: virtual void OnSurfaceCreated(const SurfaceInfo& surface_info) {} On 2017/05/22 06:46:38, sunnyps wrote: > This should be a pure abstract class otherwise we might end up violating the > style guide (no multiple inheritance except interfaces) for subclasses. Done. https://codereview.chromium.org/2854163003/diff/60001/cc/surfaces/surface_obs... cc/surfaces/surface_observer.h:29: virtual void OnSurfaceReceivedBeginFrame(const SurfaceId& surface_id, On 2017/05/22 06:46:38, sunnyps wrote: > bikeshed nit: OnSurfaceBeginFrame Done. https://codereview.chromium.org/2854163003/diff/60001/cc/surfaces/surface_obs... cc/surfaces/surface_observer.h:35: virtual void OnSurfaceFinishedBeginFrame(const SurfaceId& surface_id, On 2017/05/22 06:46:38, sunnyps wrote: > bikeshed nit: OnSurfaceBeginFrameAck Done.
This design looks good but there are one or two details I'm not sure about (comments inline). On 2017/05/22 15:32:15, Eric Seckler wrote: > On 2017/05/22 06:46:22, sunnyps wrote: > > Drawing a surface doesn't run the draw callbacks for the pending frame which > > would unthrottle the producer. > Maybe think of it this way: If a producer A's Surface has a pending frame, it is > blocked on a frame submission of producer B to some not-yet-existing Surface. > Only then can the pending frame activate and unblock A. If B is ready (e.g. > because it has submitted a frame to an older surface), we shouldn't continue to > wait for A. > > > Also, the scheduler won't indefinitely for such > > surfaces, only until the next begin frame. > But there's no reason why we shouldn't trigger an early deadline in the case > above, right? I mean to say that drawing a surface with a pending frame has no effect on whether the producer is throttled or not. Drawing the surface will just draw the active frame so it seems strictly better to wait for the surface to activate by extending the deadline not waiting indefinitely. I still think that we should put BeginFrameAck in OnSurfaceDamaged and therefore wait until activation but I'll defer to fsamuel@ on this. You're right about why we want BeginFrameArgs to be in a different callback. Begin frame for a surface basically means that we're expecting damage from the surface so maybe we should name it something like SurfaceDamageExpected or something like that. > > > If that's the case can't we pass both > > begin frame args and ack via OnSurfaceDamaged? I'm not opposed to being > explicit > > by adding new SurfaceObserver methods either so whatever's easier is fine. > I don't think we can pass the BeginFrameArgs via OnSurfaceDamaged, because we > need to find out about the args as soon as they are received (otherwise, we > don't know whether the CFSS is observing the BeginFrame and thus whether we > should wait for its ack/damage). > > I'd also like to keep the acks explicit, since that means that the > DisplayScheduler doesn't need to start observing a BeginFrame if someone > responds with DidNotProduceFrame. The scheduler should use the "has_damage" property of the ack to determine if it should keep asking for begin frames. > > > > I'm not sure whether you were suggesting this, but I'm now tracking all > > surfaces > > > in this map (not only those in "our" hierarchy), but am only waiting for > those > > > in previous_contained_surfaces. > > > > I think it's better to start with something simple like waiting for all > > surfaces. > > I'm not sure if that would suffice. Since all Displays share a single > SurfaceManager, we would be trying to wait for surfaces on a possibly different > BeginFrameSource hierarchy. (And with the headless use case, these hierarchies > should operate independently.) Maybe we could only check for all Surfaces where > last_begin_frame_args.source_id is equal to the source_id of the > DisplayScheduler's BeginFrameSource (see new patch set)? I think that should > work since we use separate BeginFrameSources for different Displays AFAICT. Thanks for the explanation. Using source_id to restrict surfaces sgtm but so does using previous contained surfaces. Either way I'd like to avoid walking the surface hierarchy. > > https://codereview.chromium.org/2854163003/diff/60001/cc/surfaces/surface_obs... > File cc/surfaces/surface_observer.h (right): > > https://codereview.chromium.org/2854163003/diff/60001/cc/surfaces/surface_obs... > cc/surfaces/surface_observer.h:19: virtual void OnSurfaceCreated(const > SurfaceInfo& surface_info) {} > On 2017/05/22 06:46:38, sunnyps wrote: > > Should we add the begin frame args and ack to SurfaceInfo to cover the begin > > frame when the surface is created? > > We could, but it's not really relevant in that situation, as SurfaceCreated is > only called when a CompositorFrame was submitted (and thus, a newly created > surface is always ready to draw). Let's add it later if we end up needing it? > > https://codereview.chromium.org/2854163003/diff/60001/cc/surfaces/surface_obs... > cc/surfaces/surface_observer.h:19: virtual void OnSurfaceCreated(const > SurfaceInfo& surface_info) {} > On 2017/05/22 06:46:38, sunnyps wrote: > > This should be a pure abstract class otherwise we might end up violating the > > style guide (no multiple inheritance except interfaces) for subclasses. > > Done. > > https://codereview.chromium.org/2854163003/diff/60001/cc/surfaces/surface_obs... > cc/surfaces/surface_observer.h:29: virtual void > OnSurfaceReceivedBeginFrame(const SurfaceId& surface_id, > On 2017/05/22 06:46:38, sunnyps wrote: > > bikeshed nit: OnSurfaceBeginFrame > > Done. > > https://codereview.chromium.org/2854163003/diff/60001/cc/surfaces/surface_obs... > cc/surfaces/surface_observer.h:35: virtual void > OnSurfaceFinishedBeginFrame(const SurfaceId& surface_id, > On 2017/05/22 06:46:38, sunnyps wrote: > > bikeshed nit: OnSurfaceBeginFrameAck > > Done.
Thanks for the moonlight reviews, Sunny! Fady, PTAL at comments about surfaces with pending frames :) On 2017/05/23 06:38:18, sunnyps wrote: > This design looks good but there are one or two details I'm not sure about > (comments inline). > > On 2017/05/22 15:32:15, Eric Seckler wrote: > > On 2017/05/22 06:46:22, sunnyps wrote: > > > Drawing a surface doesn't run the draw callbacks for the pending frame which > > > would unthrottle the producer. > > Maybe think of it this way: If a producer A's Surface has a pending frame, it > is > > blocked on a frame submission of producer B to some not-yet-existing Surface. > > Only then can the pending frame activate and unblock A. If B is ready (e.g. > > because it has submitted a frame to an older surface), we shouldn't continue > to > > wait for A. > > > > > Also, the scheduler won't indefinitely for such > > > surfaces, only until the next begin frame. > > But there's no reason why we shouldn't trigger an early deadline in the case > > above, right? > > I mean to say that drawing a surface with a pending frame has no effect on > whether the producer is throttled or not. Drawing the surface will just draw the > active frame so it seems strictly better to wait for the surface to activate by > extending the deadline not waiting indefinitely. Yeah I was thinking this at first, too. But consider that a surface A with pending frame can only activate if another surface B submits (and activates) a new CompositorFrame. We won't consider surface B (or rather, the prior surface B' of the producer for surface B) as ready if it is trying to produce a CompositorFrame right now - so if there's a chance for surface A to activate because B is working on a CompositorFrame, we would wait because B' isn't ready yet. If B isn't currently trying to produce a CompositorFrame, it doesn't make sense to wait for surface A, because its pending frame won't activate. Essentially, we wouldn't wait for surfaces with pending frames, but for their blockers instead. But I agree, this is something Fady should decide. > I still think that we should put BeginFrameAck in OnSurfaceDamaged and therefore > wait until activation but I'll defer to fsamuel@ on this. You're right about why > we want BeginFrameArgs to be in a different callback. Begin frame for a surface > basically means that we're expecting damage from the surface so maybe we should > name it something like SurfaceDamageExpected or something like that. SG. Happy to rename to SurfaceDamageExpected :) > The scheduler should use the "has_damage" property of the ack to determine if it > should keep asking for begin frames. Ah, right :) Will do that if we decide to wait for surfaces with pending frames.
piman@chromium.org changed reviewers: + piman@chromium.org
sg, lgtm
On 2017/05/23 18:04:23, piman wrote: > sg, lgtm oops, wrong CL. LGTM anyway
On 2017/05/23 09:17:39, Eric Seckler wrote: > Thanks for the moonlight reviews, Sunny! > > Fady, PTAL at comments about surfaces with pending frames :) > > > On 2017/05/23 06:38:18, sunnyps wrote: > > This design looks good but there are one or two details I'm not sure about > > (comments inline). > > > > On 2017/05/22 15:32:15, Eric Seckler wrote: > > > On 2017/05/22 06:46:22, sunnyps wrote: > > > > Drawing a surface doesn't run the draw callbacks for the pending frame > which > > > > would unthrottle the producer. > > > Maybe think of it this way: If a producer A's Surface has a pending frame, > it > > is > > > blocked on a frame submission of producer B to some not-yet-existing > Surface. > > > Only then can the pending frame activate and unblock A. If B is ready (e.g. > > > because it has submitted a frame to an older surface), we shouldn't continue > > to > > > wait for A. > > > > > > > Also, the scheduler won't indefinitely for such > > > > surfaces, only until the next begin frame. > > > But there's no reason why we shouldn't trigger an early deadline in the case > > > above, right? > > > > I mean to say that drawing a surface with a pending frame has no effect on > > whether the producer is throttled or not. Drawing the surface will just draw > the > > active frame so it seems strictly better to wait for the surface to activate > by > > extending the deadline not waiting indefinitely. > Yeah I was thinking this at first, too. But consider that a surface A with > pending frame can only activate if another surface B submits (and activates) a > new CompositorFrame. We won't consider surface B (or rather, the prior surface > B' of the producer for surface B) as ready if it is trying to produce a > CompositorFrame right now - so if there's a chance for surface A to activate > because B is working on a CompositorFrame, we would wait because B' isn't ready > yet. If B isn't currently trying to produce a CompositorFrame, it doesn't make > sense to wait for surface A, because its pending frame won't activate. > > Essentially, we wouldn't wait for surfaces with pending frames, but for their > blockers instead. > > But I agree, this is something Fady should decide. > > > I still think that we should put BeginFrameAck in OnSurfaceDamaged and > therefore > > wait until activation but I'll defer to fsamuel@ on this. You're right about > why > > we want BeginFrameArgs to be in a different callback. Begin frame for a > surface > > basically means that we're expecting damage from the surface so maybe we > should > > name it something like SurfaceDamageExpected or something like that. > SG. Happy to rename to SurfaceDamageExpected :) > > > The scheduler should use the "has_damage" property of the ack to determine if > it > > should keep asking for begin frames. > Ah, right :) Will do that if we decide to wait for surfaces with pending frames. To be honest, there is no obviously right answer here. Blockers for pending frames may take several BeginFrames to arrive and so we'd be delaying aggregation for CompositorFrames that might not arrive soon. Then again, doing LESS work overall allows the system as a whole to catch up. Let's go with Sunny's approach and maybe in the future I'll collect data and change it if it turns out faster.
On 2017/05/24 01:39:25, Fady Samuel wrote: > On 2017/05/23 09:17:39, Eric Seckler wrote: > > Thanks for the moonlight reviews, Sunny! > > > > Fady, PTAL at comments about surfaces with pending frames :) > > > > > > On 2017/05/23 06:38:18, sunnyps wrote: > > > This design looks good but there are one or two details I'm not sure about > > > (comments inline). > > > > > > On 2017/05/22 15:32:15, Eric Seckler wrote: > > > > On 2017/05/22 06:46:22, sunnyps wrote: > > > > > Drawing a surface doesn't run the draw callbacks for the pending frame > > which > > > > > would unthrottle the producer. > > > > Maybe think of it this way: If a producer A's Surface has a pending frame, > > it > > > is > > > > blocked on a frame submission of producer B to some not-yet-existing > > Surface. > > > > Only then can the pending frame activate and unblock A. If B is ready > (e.g. > > > > because it has submitted a frame to an older surface), we shouldn't > continue > > > to > > > > wait for A. > > > > > > > > > Also, the scheduler won't indefinitely for such > > > > > surfaces, only until the next begin frame. > > > > But there's no reason why we shouldn't trigger an early deadline in the > case > > > > above, right? > > > > > > I mean to say that drawing a surface with a pending frame has no effect on > > > whether the producer is throttled or not. Drawing the surface will just draw > > the > > > active frame so it seems strictly better to wait for the surface to activate > > by > > > extending the deadline not waiting indefinitely. > > Yeah I was thinking this at first, too. But consider that a surface A with > > pending frame can only activate if another surface B submits (and activates) a > > new CompositorFrame. We won't consider surface B (or rather, the prior surface > > B' of the producer for surface B) as ready if it is trying to produce a > > CompositorFrame right now - so if there's a chance for surface A to activate > > because B is working on a CompositorFrame, we would wait because B' isn't > ready > > yet. If B isn't currently trying to produce a CompositorFrame, it doesn't make > > sense to wait for surface A, because its pending frame won't activate. > > > > Essentially, we wouldn't wait for surfaces with pending frames, but for their > > blockers instead. > > > > But I agree, this is something Fady should decide. > > > > > I still think that we should put BeginFrameAck in OnSurfaceDamaged and > > therefore > > > wait until activation but I'll defer to fsamuel@ on this. You're right about > > why > > > we want BeginFrameArgs to be in a different callback. Begin frame for a > > surface > > > basically means that we're expecting damage from the surface so maybe we > > should > > > name it something like SurfaceDamageExpected or something like that. > > SG. Happy to rename to SurfaceDamageExpected :) > > > > > The scheduler should use the "has_damage" property of the ack to determine > if > > it > > > should keep asking for begin frames. > > Ah, right :) Will do that if we decide to wait for surfaces with pending > frames. > > To be honest, there is no obviously right answer here. Blockers for pending > frames may take several BeginFrames to arrive and so we'd be delaying > aggregation for CompositorFrames that might not arrive soon. Then again, doing > LESS work overall allows the system as a whole to catch up. Let's go with > Sunny's approach and maybe in the future I'll collect data and change it if it > turns out faster. Note that we won't be waiting for multiple frames for aggregation (part of draw and swap). The choice is between an early deadline i.e. draw as soon as all producers have submitted a frame even if not activated or late deadline i.e. draw when all producers have activated or next vsync.
On 2017/05/24 02:41:32, sunnyps wrote: > On 2017/05/24 01:39:25, Fady Samuel wrote: > > On 2017/05/23 09:17:39, Eric Seckler wrote: > > > Thanks for the moonlight reviews, Sunny! > > > > > > Fady, PTAL at comments about surfaces with pending frames :) > > > > > > > > > On 2017/05/23 06:38:18, sunnyps wrote: > > > > This design looks good but there are one or two details I'm not sure about > > > > (comments inline). > > > > > > > > On 2017/05/22 15:32:15, Eric Seckler wrote: > > > > > On 2017/05/22 06:46:22, sunnyps wrote: > > > > > > Drawing a surface doesn't run the draw callbacks for the pending frame > > > which > > > > > > would unthrottle the producer. > > > > > Maybe think of it this way: If a producer A's Surface has a pending > frame, > > > it > > > > is > > > > > blocked on a frame submission of producer B to some not-yet-existing > > > Surface. > > > > > Only then can the pending frame activate and unblock A. If B is ready > > (e.g. > > > > > because it has submitted a frame to an older surface), we shouldn't > > continue > > > > to > > > > > wait for A. > > > > > > > > > > > Also, the scheduler won't indefinitely for such > > > > > > surfaces, only until the next begin frame. > > > > > But there's no reason why we shouldn't trigger an early deadline in the > > case > > > > > above, right? > > > > > > > > I mean to say that drawing a surface with a pending frame has no effect on > > > > whether the producer is throttled or not. Drawing the surface will just > draw > > > the > > > > active frame so it seems strictly better to wait for the surface to > activate > > > by > > > > extending the deadline not waiting indefinitely. > > > Yeah I was thinking this at first, too. But consider that a surface A with > > > pending frame can only activate if another surface B submits (and activates) > a > > > new CompositorFrame. We won't consider surface B (or rather, the prior > surface > > > B' of the producer for surface B) as ready if it is trying to produce a > > > CompositorFrame right now - so if there's a chance for surface A to activate > > > because B is working on a CompositorFrame, we would wait because B' isn't > > ready > > > yet. If B isn't currently trying to produce a CompositorFrame, it doesn't > make > > > sense to wait for surface A, because its pending frame won't activate. > > > > > > Essentially, we wouldn't wait for surfaces with pending frames, but for > their > > > blockers instead. > > > > > > But I agree, this is something Fady should decide. > > > > > > > I still think that we should put BeginFrameAck in OnSurfaceDamaged and > > > therefore > > > > wait until activation but I'll defer to fsamuel@ on this. You're right > about > > > why > > > > we want BeginFrameArgs to be in a different callback. Begin frame for a > > > surface > > > > basically means that we're expecting damage from the surface so maybe we > > > should > > > > name it something like SurfaceDamageExpected or something like that. > > > SG. Happy to rename to SurfaceDamageExpected :) > > > > > > > The scheduler should use the "has_damage" property of the ack to determine > > if > > > it > > > > should keep asking for begin frames. > > > Ah, right :) Will do that if we decide to wait for surfaces with pending > > frames. > > > > To be honest, there is no obviously right answer here. Blockers for pending > > frames may take several BeginFrames to arrive and so we'd be delaying > > aggregation for CompositorFrames that might not arrive soon. Then again, doing > > LESS work overall allows the system as a whole to catch up. Let's go with > > Sunny's approach and maybe in the future I'll collect data and change it if it > > turns out faster. > > Note that we won't be waiting for multiple frames for aggregation (part of draw > and swap). The choice is between an early deadline i.e. draw as soon as all > producers have submitted a frame even if not activated or late deadline i.e. > draw when all producers have activated or next vsync. Yup, I understand.
Description was changed from ========== [cc] Plumb BeginFrameAcks through SurfaceManager to DisplayScheduler. BUG=697086, 646774, 678119, 703079, 707513 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== [cc] Plumb BeginFrameAcks through SurfaceManager to DisplayScheduler. BUG=697086, 646774, 678119, 703079, 707513 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_site_isolation ==========
Now passing the acks via SurfaceDamaged. Sunny, PTAL. Thank you :) https://codereview.chromium.org/2854163003/diff/120001/cc/surfaces/display_sc... File cc/surfaces/display_scheduler.h (right): https://codereview.chromium.org/2854163003/diff/120001/cc/surfaces/display_sc... cc/surfaces/display_scheduler.h:30: virtual bool SurfaceHasUndrawnFrame(const SurfaceId& surface_id) const = 0; FYI, I ended up still needing this. Tracking whether a surface has damage via OnSurfaceDamaged() is problematic because we don't know when the damage was drawn (because we don't know which surfaces were in the aggregation).
lgtm % nits https://codereview.chromium.org/2854163003/diff/120001/cc/surfaces/direct_com... File cc/surfaces/direct_compositor_frame_sink_unittest.cc (right): https://codereview.chromium.org/2854163003/diff/120001/cc/surfaces/direct_com... cc/surfaces/direct_compositor_frame_sink_unittest.cc:155: TEST_F(DirectCompositorFrameSinkTest, AcknowledgesBeginFramesWithDamage) { nit: I don't think we need this test here because we're already testing it for CFSSupport. https://codereview.chromium.org/2854163003/diff/120001/cc/surfaces/display.cc File cc/surfaces/display.cc (right): https://codereview.chromium.org/2854163003/diff/120001/cc/surfaces/display.cc... cc/surfaces/display.cc:401: if (!surface->HasActiveFrame() || nit: Can surface->HasActiveFrame() be false? If not, can we DCHECK(surface->HasActiveFrame())? https://codereview.chromium.org/2854163003/diff/120001/cc/surfaces/surface_ma... File cc/surfaces/surface_manager.cc (right): https://codereview.chromium.org/2854163003/diff/120001/cc/surfaces/surface_ma... cc/surfaces/surface_manager.cc:121: for (auto& observer : observer_list_) nit: Why OnSurfaceDestroyed in addition to OnSurfaceDiscarded? Is it because surfaces which are destroyed but not discarded may be drawn by other surfaces but acks will not be submitted any more? I don't know if this difference is important in practice. https://codereview.chromium.org/2854163003/diff/120001/cc/test/test_begin_fra... File cc/test/test_begin_frame_ack_tracker.h (right): https://codereview.chromium.org/2854163003/diff/120001/cc/test/test_begin_fra... cc/test/test_begin_frame_ack_tracker.h:13: class TestBeginFrameAckTracker : public SurfaceObserver { nit: Can we rename this to FakeSurfaceObserver and add the other functionality that's needed for some tests like tracking damaged surfaces? https://codereview.chromium.org/2854163003/diff/120001/content/browser/frame_... File content/browser/frame_host/render_widget_host_view_child_frame_unittest.cc (right): https://codereview.chromium.org/2854163003/diff/120001/content/browser/frame_... content/browser/frame_host/render_widget_host_view_child_frame_unittest.cc:248: // Tests that BeginFrameAcks are forwarded correctly from the nit: We don't need this test because we're already testing this for CFSSupport. https://codereview.chromium.org/2854163003/diff/120001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_aura_unittest.cc (right): https://codereview.chromium.org/2854163003/diff/120001/content/browser/render... content/browser/renderer_host/render_widget_host_view_aura_unittest.cc:3307: // Tests that BeginFrameAcks are forwarded correctly from the nit: The only extra behavior that's tested here is about latest confirmed sequence numbers. Maybe ok to keep for now but we'll remove it along with LCFS.
Patchset #8 (id:140001) has been deleted
The CQ bit was checked by eseckler@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2854163003/diff/120001/cc/surfaces/direct_com... File cc/surfaces/direct_compositor_frame_sink_unittest.cc (right): https://codereview.chromium.org/2854163003/diff/120001/cc/surfaces/direct_com... cc/surfaces/direct_compositor_frame_sink_unittest.cc:155: TEST_F(DirectCompositorFrameSinkTest, AcknowledgesBeginFramesWithDamage) { On 2017/05/25 20:49:13, sunnyps wrote: > nit: I don't think we need this test here because we're already testing it for > CFSSupport. Done. (You're right, the only additional behavior tested here is the forwarding of acks on CompositorFrame and DidNotProduceFrame messages in DirectCFS, for which we can also just rely on integration tests.) https://codereview.chromium.org/2854163003/diff/120001/cc/surfaces/display.cc File cc/surfaces/display.cc (right): https://codereview.chromium.org/2854163003/diff/120001/cc/surfaces/display.cc... cc/surfaces/display.cc:401: if (!surface->HasActiveFrame() || On 2017/05/25 20:49:13, sunnyps wrote: > nit: Can surface->HasActiveFrame() be false? If not, can we > DCHECK(surface->HasActiveFrame())? I think the only place where that may happen is when CFSS::RequestCopyOfSurface() is called while there's no active frame on its current_surface_. I'm adding a conditional there, since it doesn't make sense to call SurfaceDamaged() if there's no active frame. Done. https://codereview.chromium.org/2854163003/diff/120001/cc/surfaces/surface_ma... File cc/surfaces/surface_manager.cc (right): https://codereview.chromium.org/2854163003/diff/120001/cc/surfaces/surface_ma... cc/surfaces/surface_manager.cc:121: for (auto& observer : observer_list_) On 2017/05/25 20:49:13, sunnyps wrote: > nit: Why OnSurfaceDestroyed in addition to OnSurfaceDiscarded? Is it because > surfaces which are destroyed but not discarded may be drawn by other surfaces > but acks will not be submitted any more? I don't know if this difference is > important in practice. I want to avoid waiting for destroyed Surfaces (that haven't yet been gc-ed for some reason), since they won't receive a SurfaceModified (and with that, a BeginFrameAck) for any potentially pending BeginFrame. I'm not sure in which situations a destroyed Surface may actually remain alive, but making assumptions about this seems dangerous anyway (?). Keeping as-is for now. https://codereview.chromium.org/2854163003/diff/120001/cc/test/test_begin_fra... File cc/test/test_begin_frame_ack_tracker.h (right): https://codereview.chromium.org/2854163003/diff/120001/cc/test/test_begin_fra... cc/test/test_begin_frame_ack_tracker.h:13: class TestBeginFrameAckTracker : public SurfaceObserver { On 2017/05/25 20:49:13, sunnyps wrote: > nit: Can we rename this to FakeSurfaceObserver and add the other functionality > that's needed for some tests like tracking damaged surfaces? Done. https://codereview.chromium.org/2854163003/diff/120001/content/browser/frame_... File content/browser/frame_host/render_widget_host_view_child_frame_unittest.cc (right): https://codereview.chromium.org/2854163003/diff/120001/content/browser/frame_... content/browser/frame_host/render_widget_host_view_child_frame_unittest.cc:248: // Tests that BeginFrameAcks are forwarded correctly from the On 2017/05/25 20:49:13, sunnyps wrote: > nit: We don't need this test because we're already testing this for CFSSupport. Done. (see DirectCFS comment) https://codereview.chromium.org/2854163003/diff/120001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_aura_unittest.cc (right): https://codereview.chromium.org/2854163003/diff/120001/content/browser/render... content/browser/renderer_host/render_widget_host_view_aura_unittest.cc:3307: // Tests that BeginFrameAcks are forwarded correctly from the On 2017/05/25 20:49:14, sunnyps wrote: > nit: The only extra behavior that's tested here is about latest confirmed > sequence numbers. Maybe ok to keep for now but we'll remove it along with LCFS. It also tests that DidNotProduceFrame is called while DFH is locked. I'll keep it for now and we can simplify when LCSN goes away.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Description was changed from ========== [cc] Plumb BeginFrameAcks through SurfaceManager to DisplayScheduler. BUG=697086, 646774, 678119, 703079, 707513 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_site_isolation ========== to ========== [cc] Plumb BeginFrameAcks through SurfaceManager to DisplayScheduler. Plumbs the BeginFrameAcks from DidNotProduceFrame and SubmitCompositorFrame through new SurfaceObserver methods from CompositorFrameSinks to the DisplayScheduler. Also replaces the expected surface damage heuristics previously used by DisplayScheduler with tracking BeginFrame(Ack)s and surface damage. This is work towards unified BeginFrame acknowledgments, see: Tracking bug: https://crbug.com/697086 Design doc: http://bit.ly/beginframeacks BUG=697086, 646774, 678119, 703079, 707513 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_site_isolation ==========
The CQ bit was checked by eseckler@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
As it's not unlikely that this will break some perf bots, I'll try to land this today and collect some data over the weekend to determine if we need to (revert and) change anything.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by eseckler@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from piman@chromium.org, sunnyps@chromium.org Link to the patchset: https://codereview.chromium.org/2854163003/#ps180001 (title: "fix clang compile error")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 180001, "attempt_start_ts": 1495803224620800,
"parent_rev": "ff8ac992b2d0476ee7806728fcbf003600807efd", "commit_rev":
"706db9ce9ee866359ba0b3b8c58166247fa7e012"}
CQ is committing da patch.
Bot data: {"patchset_id": 180001, "attempt_start_ts": 1495803224620800,
"parent_rev": "d3046f4f42508426ff8415cd27db7b27787fd0c5", "commit_rev":
"e356c64f253923983248656d304d8aa471aad5f0"}
Message was sent while issue was closed.
Description was changed from ========== [cc] Plumb BeginFrameAcks through SurfaceManager to DisplayScheduler. Plumbs the BeginFrameAcks from DidNotProduceFrame and SubmitCompositorFrame through new SurfaceObserver methods from CompositorFrameSinks to the DisplayScheduler. Also replaces the expected surface damage heuristics previously used by DisplayScheduler with tracking BeginFrame(Ack)s and surface damage. This is work towards unified BeginFrame acknowledgments, see: Tracking bug: https://crbug.com/697086 Design doc: http://bit.ly/beginframeacks BUG=697086, 646774, 678119, 703079, 707513 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_site_isolation ========== to ========== [cc] Plumb BeginFrameAcks through SurfaceManager to DisplayScheduler. Plumbs the BeginFrameAcks from DidNotProduceFrame and SubmitCompositorFrame through new SurfaceObserver methods from CompositorFrameSinks to the DisplayScheduler. Also replaces the expected surface damage heuristics previously used by DisplayScheduler with tracking BeginFrame(Ack)s and surface damage. This is work towards unified BeginFrame acknowledgments, see: Tracking bug: https://crbug.com/697086 Design doc: http://bit.ly/beginframeacks BUG=697086, 646774, 678119, 703079, 707513 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_site_isolation Review-Url: https://codereview.chromium.org/2854163003 Cr-Commit-Position: refs/heads/master@{#474989} Committed: https://chromium.googlesource.com/chromium/src/+/e356c64f253923983248656d304d... ==========
Message was sent while issue was closed.
Committed patchset #9 (id:180001) as https://chromium.googlesource.com/chromium/src/+/e356c64f253923983248656d304d... |
