Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2785103003:
[cc] CompositorFrameSinkSupport: Defer BeginFrameAck of pending frames. (Closed)
|
Created:
3 years, 8 months ago by Eric Seckler Modified:
3 years, 7 months ago CC:
chromium-reviews, cc-bugs_chromium.org, Sami, enne (OOO) Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
Description[cc] CompositorFrameSinkSupport: Defer BeginFrameAck of pending frames.
This defers forwarding the BeginFrameAck attached to a CompositorFrame
submitted to CompositorFrameSinkSupport until it is activated.
For this purpose, CFSS continues to observe BeginFrames while a frame
is pending to be activated, even if the client is not in need of
BeginFrames.
This is work towards unified BeginFrame acknowledgments, see:
Tracking bug: https://crbug.com/697086
Design doc: http://bit.ly/beginframeacks
DO_NOT_SUBMIT.
superseded by https://codereview.chromium.org/2854163003/.
BUG=703079, 697086, 646774
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
Patch Set 1 #
Total comments: 14
Patch Set 2 : add some comments, plumb SurfaceDiscarded. #
Total comments: 12
Patch Set 3 : address comments, some fixes, added tests. #
Total comments: 6
Patch Set 4 : add comment #
Messages
Total messages: 20 (12 generated)
Description was changed from ========== [cc] CompositorFrameSinkSupport: Defer BeginFrameAck of pending frames. This defers forwarding the BeginFrameAck attached to a CompositorFrame submitted to CompositorFrameSinkSupport until it is activated. For this purpose, CFSS continues to observe BeginFrames while a frame is pending to be activated, even if the client is not in need of BeginFrames. This is work towards unified BeginFrame acknowledgments, see: Tracking bug: https://crbug.com/697086 Design doc: http://bit.ly/beginframeacks BUG=703079, 697086, 646774 ========== to ========== [cc] CompositorFrameSinkSupport: Defer BeginFrameAck of pending frames. This defers forwarding the BeginFrameAck attached to a CompositorFrame submitted to CompositorFrameSinkSupport until it is activated. For this purpose, CFSS continues to observe BeginFrames while a frame is pending to be activated, even if the client is not in need of BeginFrames. This is work towards unified BeginFrame acknowledgments, see: Tracking bug: https://crbug.com/697086 Design doc: http://bit.ly/beginframeacks BUG=703079, 697086, 646774 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: + brianderson@chromium.org, fsamuel@chromium.org
Fady + Brian, could you have a look if this is sensible? :) Once we agree on the approach here, I'll try to add some tests...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
A bunch of questions. I'm still trying to wrap my head around this code. Thanks! https://codereview.chromium.org/2785103003/diff/1/cc/surfaces/compositor_fram... File cc/surfaces/compositor_frame_sink_support.cc (right): https://codereview.chromium.org/2785103003/diff/1/cc/surfaces/compositor_fram... cc/surfaces/compositor_frame_sink_support.cc:63: PendingFrameDiscarded(); There are other ways for frames to get discarded. For example: 1. Submit a CompositorFrame that is pending with a given LocalSurfaceId. 2. Submit another CompositorFrame with a different LocalSurfaceId. The first LocalSurfaceId will get discarded...I think you want to plumb through OnSurfaceDiscarded. https://codereview.chromium.org/2785103003/diff/1/cc/surfaces/compositor_fram... cc/surfaces/compositor_frame_sink_support.cc:93: if (has_pending_frame_) { How do we get here? https://codereview.chromium.org/2785103003/diff/1/cc/surfaces/compositor_fram... cc/surfaces/compositor_frame_sink_support.cc:121: // DisplayScheduler once it is activated. I'm confused. We send a AcknowledgeLastBeginFrameWithoutUpdates, don't we? https://codereview.chromium.org/2785103003/diff/1/cc/surfaces/compositor_fram... cc/surfaces/compositor_frame_sink_support.cc:197: PendingFrameDiscarded(); There are other ways for a pending frame to get discarded. See above. I think you need OnSurfaceDiscarded plumbed through. https://codereview.chromium.org/2785103003/diff/1/cc/surfaces/compositor_fram... cc/surfaces/compositor_frame_sink_support.cc:258: if (!has_pending_frame_) I'm confused about when this can happen? Should this be a DCHECK? https://codereview.chromium.org/2785103003/diff/1/cc/surfaces/compositor_fram... cc/surfaces/compositor_frame_sink_support.cc:279: UpdateNeedsBeginFramesInternal(); A comment about how we are no longer waiting for a frame to activate so we need to check whether we need to continue to observe BeginFrames. https://codereview.chromium.org/2785103003/diff/1/cc/surfaces/compositor_fram... cc/surfaces/compositor_frame_sink_support.cc:310: if (!last_begin_frame_args_.IsValid()) Why would this happen?
A bunch of questions. I'm still trying to wrap my head around this code. Thanks! https://codereview.chromium.org/2785103003/diff/1/cc/surfaces/compositor_fram... File cc/surfaces/compositor_frame_sink_support.cc (right): https://codereview.chromium.org/2785103003/diff/1/cc/surfaces/compositor_fram... cc/surfaces/compositor_frame_sink_support.cc:63: PendingFrameDiscarded(); There are other ways for frames to get discarded. For example: 1. Submit a CompositorFrame that is pending with a given LocalSurfaceId. 2. Submit another CompositorFrame with a different LocalSurfaceId. The first LocalSurfaceId will get discarded...I think you want to plumb through OnSurfaceDiscarded. https://codereview.chromium.org/2785103003/diff/1/cc/surfaces/compositor_fram... cc/surfaces/compositor_frame_sink_support.cc:93: if (has_pending_frame_) { How do we get here? https://codereview.chromium.org/2785103003/diff/1/cc/surfaces/compositor_fram... cc/surfaces/compositor_frame_sink_support.cc:121: // DisplayScheduler once it is activated. I'm confused. We send a AcknowledgeLastBeginFrameWithoutUpdates, don't we? https://codereview.chromium.org/2785103003/diff/1/cc/surfaces/compositor_fram... cc/surfaces/compositor_frame_sink_support.cc:197: PendingFrameDiscarded(); There are other ways for a pending frame to get discarded. See above. I think you need OnSurfaceDiscarded plumbed through. https://codereview.chromium.org/2785103003/diff/1/cc/surfaces/compositor_fram... cc/surfaces/compositor_frame_sink_support.cc:258: if (!has_pending_frame_) I'm confused about when this can happen? Should this be a DCHECK? https://codereview.chromium.org/2785103003/diff/1/cc/surfaces/compositor_fram... cc/surfaces/compositor_frame_sink_support.cc:279: UpdateNeedsBeginFramesInternal(); A comment about how we are no longer waiting for a frame to activate so we need to check whether we need to continue to observe BeginFrames. https://codereview.chromium.org/2785103003/diff/1/cc/surfaces/compositor_fram... cc/surfaces/compositor_frame_sink_support.cc:310: if (!last_begin_frame_args_.IsValid()) Why would this happen?
On 2017/03/30 12:41:56, Fady Samuel wrote: > A bunch of questions. I'm still trying to wrap my head around this code. Thanks! Not unexpected :) Added some more source comments around where you had questions. https://codereview.chromium.org/2785103003/diff/1/cc/surfaces/compositor_fram... File cc/surfaces/compositor_frame_sink_support.cc (right): https://codereview.chromium.org/2785103003/diff/1/cc/surfaces/compositor_fram... cc/surfaces/compositor_frame_sink_support.cc:63: PendingFrameDiscarded(); On 2017/03/30 12:41:56, Fady Samuel wrote: > There are other ways for frames to get discarded. > > For example: > > 1. Submit a CompositorFrame that is pending with a given LocalSurfaceId. > 2. Submit another CompositorFrame with a different LocalSurfaceId. > > The first LocalSurfaceId will get discarded...I think you want to plumb through > OnSurfaceDiscarded. In that particular case, I don't think we'd want to ack right away though, which is why I'm not handling it as PendingFrameDiscarded at the moment. Instead, if this happens, we just update the begin_frame_ack_for_pending_frame_ and wait for the new frame to be activated before acking. That said, there may easily be other cases for the surface to be discarded that we _should_ handle as PendingFrameDiscarded(). I'm now plumbing through OnSurfaceDiscarded - and tracking the case above using surface_id_for_pending_frame_ so that I don't prematurely call AcknowledgeLastBeginFrameWithoutUpdates. https://codereview.chromium.org/2785103003/diff/1/cc/surfaces/compositor_fram... cc/surfaces/compositor_frame_sink_support.cc:93: if (has_pending_frame_) { On 2017/03/30 12:41:56, Fady Samuel wrote: > How do we get here? A client may continue to receive further BeginFrames while a prior CompositorFrame remains pending. In such cases, it's likely that it doesn't submit another CompositorFrame (e.g. because it is CompositorFrameAck-throttled) and thus acks the BeginFrames through BFDidNotSwap. https://codereview.chromium.org/2785103003/diff/1/cc/surfaces/compositor_fram... cc/surfaces/compositor_frame_sink_support.cc:121: // DisplayScheduler once it is activated. On 2017/03/30 12:41:56, Fady Samuel wrote: > I'm confused. We send a AcknowledgeLastBeginFrameWithoutUpdates, don't we? Not right away. We only send the ack from AcknowledgeLastBeginFrameWithoutUpdates if the pending frame isn't activated before we receive the next BeginFrame(s). Otherwise we just postpone forwarding the CompositorFrame's ack until PendingFrameActivated() - which may happen before the next BeginFrame :) https://codereview.chromium.org/2785103003/diff/1/cc/surfaces/compositor_fram... cc/surfaces/compositor_frame_sink_support.cc:197: PendingFrameDiscarded(); On 2017/03/30 12:41:56, Fady Samuel wrote: > There are other ways for a pending frame to get discarded. See above. I think > you need OnSurfaceDiscarded plumbed through. Done. I'll have to keep this call to PendingFrameDiscarded separately though, right? https://codereview.chromium.org/2785103003/diff/1/cc/surfaces/compositor_fram... cc/surfaces/compositor_frame_sink_support.cc:258: if (!has_pending_frame_) On 2017/03/30 12:41:56, Fady Samuel wrote: > I'm confused about when this can happen? Should this be a DCHECK? Things like ClearSurface() submit CompositorFrames that I'm not tracking via has_pending_frame_. https://codereview.chromium.org/2785103003/diff/1/cc/surfaces/compositor_fram... cc/surfaces/compositor_frame_sink_support.cc:279: UpdateNeedsBeginFramesInternal(); On 2017/03/30 12:41:56, Fady Samuel wrote: > A comment about how we are no longer waiting for a frame to activate so we need > to check whether we need to continue to observe BeginFrames. Done. https://codereview.chromium.org/2785103003/diff/1/cc/surfaces/compositor_fram... cc/surfaces/compositor_frame_sink_support.cc:310: if (!last_begin_frame_args_.IsValid()) On 2017/03/30 12:41:55, Fady Samuel wrote: > Why would this happen? Well, not all clients wait for BeginFrames to submit CompositorFrames :P Some just submit a first CompositorFrame without prior BeginFrame, and some draw synchronously or don't care about BeginFrames at all (think android_webview).
https://codereview.chromium.org/2785103003/diff/20001/cc/surfaces/compositor_... File cc/surfaces/compositor_frame_sink_support.cc (right): https://codereview.chromium.org/2785103003/diff/20001/cc/surfaces/compositor_... cc/surfaces/compositor_frame_sink_support.cc:96: begin_frame_ack_for_pending_frame_.source_id = ack.source_id; I wonder if we can modify the pending CompositorFrame's BeginFrameAck directly instead of messing with a copy here? Would that be messier? https://codereview.chromium.org/2785103003/diff/20001/cc/surfaces/compositor_... cc/surfaces/compositor_frame_sink_support.cc:126: has_pending_frame_ = true; Surface::HasPendingFrame exists. Would it make sense to plumb that out from SurfaceFactory? bool HasPendingFrame() const { return surface_factory_->current_surface() ? surface_factorysurface_factory_->current_surface()->HasPendingFrame() : false; [rename current_surface_for_testing()] https://codereview.chromium.org/2785103003/diff/20001/cc/surfaces/compositor_... File cc/surfaces/compositor_frame_sink_support.h (right): https://codereview.chromium.org/2785103003/diff/20001/cc/surfaces/compositor_... cc/surfaces/compositor_frame_sink_support.h:91: void AcknowledgeBeginFrame(const BeginFrameAck& ack); nit: comment. https://codereview.chromium.org/2785103003/diff/20001/cc/surfaces/compositor_... cc/surfaces/compositor_frame_sink_support.h:92: void AcknowledgeLastBeginFrameWithoutUpdates(); nit: comment. https://codereview.chromium.org/2785103003/diff/20001/cc/surfaces/compositor_... cc/surfaces/compositor_frame_sink_support.h:107: bool has_pending_frame_ = false; Surface already knows this. https://codereview.chromium.org/2785103003/diff/20001/cc/surfaces/compositor_... cc/surfaces/compositor_frame_sink_support.h:108: LocalSurfaceId surface_id_for_pending_frame_; Do we need this? https://codereview.chromium.org/2785103003/diff/20001/cc/surfaces/compositor_... cc/surfaces/compositor_frame_sink_support.h:118: BeginFrameAck begin_frame_ack_for_pending_frame_; Maybe either make this base::Optional<BeginFrameAck> or simplify modify in place?
I added some tests now too. However, I realized that SurfaceDependencyTracker is also a BeginFrameObserver (which doesn't currently ack BeginFrames :O). Since the tracker already deals with pending surfaces & surface activations, it might be a more sensible place to track latest_confirmed_sequence_number of pending frames. CFSSupport could just directly acknowledge then. I'll prototype what that would look like next week, as an alternative to this patch. Maybe that turns out to be simpler.... :D https://codereview.chromium.org/2785103003/diff/20001/cc/surfaces/compositor_... File cc/surfaces/compositor_frame_sink_support.h (right): https://codereview.chromium.org/2785103003/diff/20001/cc/surfaces/compositor_... cc/surfaces/compositor_frame_sink_support.h:91: void AcknowledgeBeginFrame(const BeginFrameAck& ack); On 2017/03/31 00:09:02, Fady Samuel wrote: > nit: comment. Done. https://codereview.chromium.org/2785103003/diff/20001/cc/surfaces/compositor_... cc/surfaces/compositor_frame_sink_support.h:92: void AcknowledgeLastBeginFrameWithoutUpdates(); On 2017/03/31 00:09:02, Fady Samuel wrote: > nit: comment. Done. https://codereview.chromium.org/2785103003/diff/20001/cc/surfaces/compositor_... cc/surfaces/compositor_frame_sink_support.h:107: bool has_pending_frame_ = false; On 2017/03/31 00:09:02, Fady Samuel wrote: > Surface already knows this. not quite: CFSSupport only acknowledges when a CompositorFrame that it itself submitted is activated, not if some other CompositorFrame activates (e.g. one queued by ClearSurface). Therefore, has_pending_frame_ was only tracking CFSSupport-submitted pending frames. but no need for this field with base::Optional below. https://codereview.chromium.org/2785103003/diff/20001/cc/surfaces/compositor_... cc/surfaces/compositor_frame_sink_support.h:108: LocalSurfaceId surface_id_for_pending_frame_; On 2017/03/31 00:09:02, Fady Samuel wrote: > Do we need this? yeah, see comment in SurfaceDiscarded: If we submit a CompositorFrame to a new local_surface_id, the old surface will be destroyed after the frame becomes pending. https://codereview.chromium.org/2785103003/diff/20001/cc/surfaces/compositor_... cc/surfaces/compositor_frame_sink_support.h:118: BeginFrameAck begin_frame_ack_for_pending_frame_; On 2017/03/31 00:09:02, Fady Samuel wrote: > Maybe either make this base::Optional<BeginFrameAck> or simplify modify in > place? base::Optional it is! modify-in-place might be an option to explore, too. Not sure if it would be simpler though.
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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2785103003/diff/40001/cc/surfaces/compositor_... File cc/surfaces/compositor_frame_sink_support.cc (right): https://codereview.chromium.org/2785103003/diff/40001/cc/surfaces/compositor_... cc/surfaces/compositor_frame_sink_support.cc:96: begin_frame_ack_for_pending_frame_->source_id = ack.source_id; Is it possible for the source_id to change? If not, should this be a DCHECK? If yes, do we need to store a collection of acks? https://codereview.chromium.org/2785103003/diff/40001/cc/surfaces/compositor_... cc/surfaces/compositor_frame_sink_support.cc:268: if (last_begin_frame_args_.IsValid()) { Can you add a comment explaining why this block is needed? https://codereview.chromium.org/2785103003/diff/40001/cc/surfaces/compositor_... cc/surfaces/compositor_frame_sink_support.cc:327: void CompositorFrameSinkSupport::AcknowledgeLastBeginFrameWithoutUpdates() { It looks like this acknowledges the most recently sent begin frame even before the pending frame hasn't been acknowledged. When would you want to acknowledge out of order? Am I misunderstanding something?
Let's try out doing this in SurfaceDependencyTracker before getting hung up on this patch though :) https://codereview.chromium.org/2785103003/diff/40001/cc/surfaces/compositor_... File cc/surfaces/compositor_frame_sink_support.cc (right): https://codereview.chromium.org/2785103003/diff/40001/cc/surfaces/compositor_... cc/surfaces/compositor_frame_sink_support.cc:96: begin_frame_ack_for_pending_frame_->source_id = ack.source_id; On 2017/03/31 19:21:27, brianderson wrote: > Is it possible for the source_id to change? > > If not, should this be a DCHECK? > If yes, do we need to store a collection of acks? Considering headless use cases (dynamically replacing the BFS): yeah, it's possible :) We don't need to store multiple acks though, since we can always only ack the latest BeginFrame. If the source changes, we just update the latest_confirmed_sequence_number of the begin_frame_ack_for_pending_frame_ (that we will eventually send when the CompositorFrame activates). https://codereview.chromium.org/2785103003/diff/40001/cc/surfaces/compositor_... cc/surfaces/compositor_frame_sink_support.cc:268: if (last_begin_frame_args_.IsValid()) { On 2017/03/31 19:21:27, brianderson wrote: > Can you add a comment explaining why this block is needed? Done. https://codereview.chromium.org/2785103003/diff/40001/cc/surfaces/compositor_... cc/surfaces/compositor_frame_sink_support.cc:327: void CompositorFrameSinkSupport::AcknowledgeLastBeginFrameWithoutUpdates() { On 2017/03/31 19:21:27, brianderson wrote: > It looks like this acknowledges the most recently sent begin frame even before > the pending frame hasn't been acknowledged. Correct. In order to eventually acknowledge the activated CompositorFrame's updates (to latest_confirmed_sequence_number), we need to continue observing the BeginFrame source. We use this method to acknowledge BeginFrames that pass while the frame is still pending and to ack the current BeginFrame if the pending frame is discarded for some reason. > When would you want to acknowledge out of order? You're right, I guess we could decide not to send acks for past BeginFrames (during OnBeginFrame), since these acks are for not for the current BeginFrame and contain no updates to latest_confirmed_sequence_number.
Patchset #4 (id:60001) has been deleted
Description was changed from ========== [cc] CompositorFrameSinkSupport: Defer BeginFrameAck of pending frames. This defers forwarding the BeginFrameAck attached to a CompositorFrame submitted to CompositorFrameSinkSupport until it is activated. For this purpose, CFSS continues to observe BeginFrames while a frame is pending to be activated, even if the client is not in need of BeginFrames. This is work towards unified BeginFrame acknowledgments, see: Tracking bug: https://crbug.com/697086 Design doc: http://bit.ly/beginframeacks BUG=703079, 697086, 646774 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== [cc] CompositorFrameSinkSupport: Defer BeginFrameAck of pending frames. This defers forwarding the BeginFrameAck attached to a CompositorFrame submitted to CompositorFrameSinkSupport until it is activated. For this purpose, CFSS continues to observe BeginFrames while a frame is pending to be activated, even if the client is not in need of BeginFrames. This is work towards unified BeginFrame acknowledgments, see: Tracking bug: https://crbug.com/697086 Design doc: http://bit.ly/beginframeacks DO_NOT_SUBMIT. superseded by https://codereview.chromium.org/2854163003/. BUG=703079, 697086, 646774 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
