Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2413063002:
content/blimp: Set up hooks for enabling LTHRemote in the renderer. (Closed)
|
Created:
4 years, 2 months ago by Khushal Modified:
4 years, 1 month ago CC:
chromium-reviews, mlamouri+watch-content_chromium.org, anandc+watch-blimp_chromium.org, maniscalco+watch-blimp_chromium.org, sriramsr+watch-blimp_chromium.org, bgoldman+watch-blimp_chromium.org, jam, steimel+watch-blimp_chromium.org, gcasto+watch-blimp_chromium.org, shaktisahu+watch-blimp_chromium.org, nyquist+watch-blimp_chromium.org, perumaal+watch-blimp_chromium.org, marcinjb+watch-blimp_chromium.org, jessicag+watch-blimp_chromium.org, kmarshall+watch-blimp_chromium.org, lethalantidote+watch-blimp_chromium.org, piman+watch_chromium.org, darin-cc_chromium.org, cc-bugs_chromium.org, dtrainor+watch-blimp_chromium.org, scf+watch-blimp_chromium.org, khushalsagar+watch-blimp_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Descriptioncontent/blimp: Set up hooks for enabling LTHRemote in the renderer.
Sets up hooks for requesting a RemoteCompositorBridge from the
ContentRendererClient, which is necessary to create a remote
LayerTreeHost. Also adds an implementation of the Bridge in blimp.
BUG=648442
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
Committed: https://crrev.com/d3cbb4d90d7367030d637cbb7b68b2293f6ebe06
Cr-Commit-Position: refs/heads/master@{#427569}
Patch Set 1 #Patch Set 2 : Rebase #
Total comments: 1
Patch Set 3 : missed a few things #Patch Set 4 : gn check #
Total comments: 37
Patch Set 5 : Addressed comments #
Total comments: 2
Patch Set 6 : proto comment #
Total comments: 2
Patch Set 7 : Addressed comments #
Total comments: 7
Patch Set 8 : Rebase #
Total comments: 19
Patch Set 9 : Addressed comments #Patch Set 10 : Rebase #
Total comments: 14
Patch Set 11 : added comments #
Total comments: 4
Patch Set 12 : Addressed comments #Patch Set 13 : Rebase. #Messages
Total messages: 48 (24 generated)
Description was changed from ========== content/blimp: Set up hooks for enabling LTHRemote in the renderer. Sets up hooks for requesting a RemoteCompositorBridge from the ContentRendererClient, which is necessary to create a remote LayerTreeHost. Also adds an implementation of the Bridge in blimp. BUG=648442 ========== to ========== content/blimp: Set up hooks for enabling LTHRemote in the renderer. Sets up hooks for requesting a RemoteCompositorBridge from the ContentRendererClient, which is necessary to create a remote LayerTreeHost. Also adds an implementation of the Bridge in blimp. BUG=648442 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel ==========
khushalsagar@chromium.org changed reviewers: + piman@chromium.org, wez@chromium.org
+piman for content and minor cc change. +wez for blimp. https://codereview.chromium.org/2413063002/diff/20001/blimp/engine/renderer/s... File blimp/engine/renderer/scheduler.cc (right): https://codereview.chromium.org/2413063002/diff/20001/blimp/engine/renderer/s... blimp/engine/renderer/scheduler.cc:90: next_frame_time_ = base::TimeTicks::Now() + frame_delay_; I added this mostly to space out frames where the page schedules one, but it doesn't result in an update to the client so we would end up processing frames back-to-back. Let me know if you think its worth it for now. Since going forward there will be better throttling here anyway.
lgtm
khushalsagar@chromium.org changed reviewers: + danakj@chromium.org
I realized I missed few things when tested it with the corresponding client side patch. +danakj in case you have some comments about the additional cc changes. They're trivial and blimp specific though.
The CQ bit was checked by khushalsagar@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: Try jobs failed on following builders: linux_precise_blink_rel on master.tryserver.blink (JOB_FAILED, http://build.chromium.org/p/tryserver.blink/builders/linux_precise_blink_rel/...) linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by khushalsagar@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.
khushalsagar@chromium.org changed reviewers: + dtrainor@chromium.org - wez@chromium.org
+dtrainor to look at the blimp bits instead. Since looks like wez is busy for the day.
wez@chromium.org changed reviewers: + wez@chromium.org
https://codereview.chromium.org/2413063002/diff/60001/blimp/engine/renderer/s... File blimp/engine/renderer/scheduler.cc (right): https://codereview.chromium.org/2413063002/diff/60001/blimp/engine/renderer/s... blimp/engine/renderer/scheduler.cc:17: const base::TimeDelta kDefaultFrameDelay = Is TimeDelta POD? I thought it had a ctor/dtor, so won't this result in ini/fini? https://codereview.chromium.org/2413063002/diff/60001/blimp/engine/renderer/s... blimp/engine/renderer/scheduler.cc:73: base::Bind(&Scheduler::StartMainFrame, weak_ptr_factory_.GetWeakPtr())); Take a WeakPtr at construction time, rather than calling GetWeakPtr on each Bind. However, why do you need a WeakPtr at all? Doesn't this class own the OneShotTimer? So when this is deleted, so is the timer? What is the threading property for this class? https://codereview.chromium.org/2413063002/diff/60001/blimp/engine/renderer/s... File blimp/engine/renderer/scheduler.h (right): https://codereview.chromium.org/2413063002/diff/60001/blimp/engine/renderer/s... blimp/engine/renderer/scheduler.h:17: // Responsible for scheduling frame updates sent to the client. Can you elaborate on *how* it schedules them? All I see here are some void() functions and a next_frame_time() API. How are they used? What is the |client| for? https://codereview.chromium.org/2413063002/diff/60001/blimp/engine/renderer/s... blimp/engine/renderer/scheduler.h:21: SchedulerClient* client); Who owns this? What is its lifetime? https://codereview.chromium.org/2413063002/diff/60001/blimp/engine/renderer/s... blimp/engine/renderer/scheduler.h:24: void SetNeedsMainFrame(); What is a "main frame" in the context of this scheduler? https://codereview.chromium.org/2413063002/diff/60001/blimp/engine/renderer/s... blimp/engine/renderer/scheduler.h:26: void DidReceiveFrameUpdateAck(); SetNeedsMainFrame(), at least, needs explaining. https://codereview.chromium.org/2413063002/diff/60001/blimp/engine/renderer/s... blimp/engine/renderer/scheduler.h:38: bool CanProduceMainFrames() const; Under what circumstance can we _not_ produce "main frames"? https://codereview.chromium.org/2413063002/diff/60001/blimp/engine/renderer/s... blimp/engine/renderer/scheduler.h:56: SchedulerClient* client_; nit: Initialize to nullptr here, just in case? https://codereview.chromium.org/2413063002/diff/60001/blimp/engine/renderer/s... File blimp/engine/renderer/scheduler_client.h (right): https://codereview.chromium.org/2413063002/diff/60001/blimp/engine/renderer/s... blimp/engine/renderer/scheduler_client.h:17: virtual void StartMainFrameUpdate() = 0; Please document what this call means, and what the API is for! :) Does this really need its own header, vs appearing in the Scheduler.h? https://codereview.chromium.org/2413063002/diff/60001/cc/layers/picture_layer.cc File cc/layers/picture_layer.cc (right): https://codereview.chromium.org/2413063002/diff/60001/cc/layers/picture_layer... cc/layers/picture_layer.cc:212: DCHECK(GetLayerTree()->engine_picture_cache()); nit: Is it worth DCHECK()ing these, given that you're going to de-reference them, and thereby crash, later in this function? https://codereview.chromium.org/2413063002/diff/60001/cc/proto/compositor_mes... File cc/proto/compositor_message.proto (right): https://codereview.chromium.org/2413063002/diff/60001/cc/proto/compositor_mes... cc/proto/compositor_message.proto:26: optional bool frame_ack = 4; Can you clarify what values this holds? What does a true vs false ACK mean? https://codereview.chromium.org/2413063002/diff/60001/content/public/renderer... File content/public/renderer/content_renderer_client.h (right): https://codereview.chromium.org/2413063002/diff/60001/content/public/renderer... content/public/renderer/content_renderer_client.h:285: cc::RemoteProtoChannel* remote_proto_channel, What is the lifetime requirement on this parameter?
https://codereview.chromium.org/2413063002/diff/60001/blimp/engine/renderer/s... File blimp/engine/renderer/scheduler.cc (right): https://codereview.chromium.org/2413063002/diff/60001/blimp/engine/renderer/s... blimp/engine/renderer/scheduler.cc:17: const base::TimeDelta kDefaultFrameDelay = On 2016/10/13 16:53:52, Wez wrote: > Is TimeDelta POD? I thought it had a ctor/dtor, so won't this result in > ini/fini? Its POD, doesn't have a dtor. Found an example of similar use as well, https://cs.chromium.org/chromium/src/cc/scheduler/compositor_timing_history.c... https://codereview.chromium.org/2413063002/diff/60001/blimp/engine/renderer/s... blimp/engine/renderer/scheduler.cc:73: base::Bind(&Scheduler::StartMainFrame, weak_ptr_factory_.GetWeakPtr())); On 2016/10/13 16:53:52, Wez wrote: > Take a WeakPtr at construction time, rather than calling GetWeakPtr on each > Bind. However, why do you need a WeakPtr at all? Doesn't this class own the > OneShotTimer? So when this is deleted, so is the timer? > > What is the threading property for this class? The class is created/destroyed and lives on the same thread. You're right, I don't here. Removed. https://codereview.chromium.org/2413063002/diff/60001/blimp/engine/renderer/s... File blimp/engine/renderer/scheduler.h (right): https://codereview.chromium.org/2413063002/diff/60001/blimp/engine/renderer/s... blimp/engine/renderer/scheduler.h:17: // Responsible for scheduling frame updates sent to the client. On 2016/10/13 16:53:52, Wez wrote: > Can you elaborate on *how* it schedules them? All I see here are some void() > functions and a next_frame_time() API. How are they used? What is the |client| > for? Added a comment. I don't think we need to get into the details about how exactly the class works internally, so I mentioned the signals that it needs to make these decisions. What the |client| is for is document on that interface. https://codereview.chromium.org/2413063002/diff/60001/blimp/engine/renderer/s... blimp/engine/renderer/scheduler.h:21: SchedulerClient* client); On 2016/10/13 16:53:52, Wez wrote: > Who owns this? What is its lifetime? I don't know if there is an established pattern for this, but from what I've seen in code, if a class takes a Delegate/Client interface in its ctor, it is assumed that it will outlive the class. We document if a pointer can be null, otherwise it is implicitly assumed that it can't be. And it doesn't seem of much benefit to me to have this be stated explicitly on every class. Happy to add a comment if you feel otherwise though. https://codereview.chromium.org/2413063002/diff/60001/blimp/engine/renderer/s... blimp/engine/renderer/scheduler.h:24: void SetNeedsMainFrame(); On 2016/10/13 16:53:52, Wez wrote: > What is a "main frame" in the context of this scheduler? Added a comment. I kept the word main frame mostly to stay consistent with the nomenclature we use to refer to frames produced by blink, so it would be obvious to anyone looking at the code as to what frame we're talking about. https://codereview.chromium.org/2413063002/diff/60001/blimp/engine/renderer/s... blimp/engine/renderer/scheduler.h:26: void DidReceiveFrameUpdateAck(); On 2016/10/13 16:53:52, Wez wrote: > SetNeedsMainFrame(), at least, needs explaining. Done. https://codereview.chromium.org/2413063002/diff/60001/blimp/engine/renderer/s... blimp/engine/renderer/scheduler.h:38: bool CanProduceMainFrames() const; On 2016/10/13 16:53:52, Wez wrote: > Under what circumstance can we _not_ produce "main frames"? Done. https://codereview.chromium.org/2413063002/diff/60001/blimp/engine/renderer/s... blimp/engine/renderer/scheduler.h:56: SchedulerClient* client_; On 2016/10/13 16:53:52, Wez wrote: > nit: Initialize to nullptr here, just in case? The ctor takes it as an argument, so is it necessary? https://codereview.chromium.org/2413063002/diff/60001/blimp/engine/renderer/s... File blimp/engine/renderer/scheduler_client.h (right): https://codereview.chromium.org/2413063002/diff/60001/blimp/engine/renderer/s... blimp/engine/renderer/scheduler_client.h:17: virtual void StartMainFrameUpdate() = 0; On 2016/10/13 16:53:52, Wez wrote: > Please document what this call means, and what the API is for! :) Done. > > Does this really need its own header, vs appearing in the Scheduler.h? Not really. Moved. https://codereview.chromium.org/2413063002/diff/60001/cc/layers/picture_layer.cc File cc/layers/picture_layer.cc (right): https://codereview.chromium.org/2413063002/diff/60001/cc/layers/picture_layer... cc/layers/picture_layer.cc:212: DCHECK(GetLayerTree()->engine_picture_cache()); On 2016/10/13 16:53:52, Wez wrote: > nit: Is it worth DCHECK()ing these, given that you're going to de-reference > them, and thereby crash, later in this function? I found an unhelpful seg-fault instead of a pretty stack trace when I was debugging this, so that's why decided to add it here. https://codereview.chromium.org/2413063002/diff/60001/cc/proto/compositor_mes... File cc/proto/compositor_message.proto (right): https://codereview.chromium.org/2413063002/diff/60001/cc/proto/compositor_mes... cc/proto/compositor_message.proto:26: optional bool frame_ack = 4; On 2016/10/13 16:53:52, Wez wrote: > Can you clarify what values this holds? What does a true vs false ACK mean? The value can never be false. I suppose it should be an enum for the message type. But I'm going to kill these protos in cc as soon as we remove the RemoteProtoChannel, so its going to be much clearer and well-structured when these move into blimp. https://codereview.chromium.org/2413063002/diff/60001/content/public/renderer... File content/public/renderer/content_renderer_client.h (right): https://codereview.chromium.org/2413063002/diff/60001/content/public/renderer... content/public/renderer/content_renderer_client.h:285: cc::RemoteProtoChannel* remote_proto_channel, On 2016/10/13 16:53:53, Wez wrote: > What is the lifetime requirement on this parameter? Done.
cc LGTM https://codereview.chromium.org/2413063002/diff/80001/cc/proto/compositor_mes... File cc/proto/compositor_message.proto (right): https://codereview.chromium.org/2413063002/diff/80001/cc/proto/compositor_mes... cc/proto/compositor_message.proto:25: // Frame Ack. The comment is literally the same text as the variable name, can you say what it is instead of its name?
https://codereview.chromium.org/2413063002/diff/80001/cc/proto/compositor_mes... File cc/proto/compositor_message.proto (right): https://codereview.chromium.org/2413063002/diff/80001/cc/proto/compositor_mes... cc/proto/compositor_message.proto:25: // Frame Ack. On 2016/10/13 19:23:23, danakj wrote: > The comment is literally the same text as the variable name, can you say what it > is instead of its name? Done.
ping to wez. :)
https://codereview.chromium.org/2413063002/diff/60001/blimp/engine/renderer/s... File blimp/engine/renderer/scheduler.cc (right): https://codereview.chromium.org/2413063002/diff/60001/blimp/engine/renderer/s... blimp/engine/renderer/scheduler.cc:17: const base::TimeDelta kDefaultFrameDelay = On 2016/10/13 18:47:27, Khushal wrote: > On 2016/10/13 16:53:52, Wez wrote: > > Is TimeDelta POD? I thought it had a ctor/dtor, so won't this result in > > ini/fini? > > Its POD, doesn't have a dtor. > > Found an example of similar use as well, > https://cs.chromium.org/chromium/src/cc/scheduler/compositor_timing_history.c... Oh, OK; I'm surprised that assigning it a value using FromMilliseconds doesn't trigger generation of _init code, though. https://codereview.chromium.org/2413063002/diff/60001/blimp/engine/renderer/s... File blimp/engine/renderer/scheduler.h (right): https://codereview.chromium.org/2413063002/diff/60001/blimp/engine/renderer/s... blimp/engine/renderer/scheduler.h:17: // Responsible for scheduling frame updates sent to the client. On 2016/10/13 18:47:27, Khushal wrote: > On 2016/10/13 16:53:52, Wez wrote: > > Can you elaborate on *how* it schedules them? All I see here are some void() > > functions and a next_frame_time() API. How are they used? What is the > |client| > > for? > > Added a comment. I don't think we need to get into the details about how exactly > the class works internally, so I mentioned the signals that it needs to make > these decisions. > What the |client| is for is document on that interface. I'm not asking for details of how we decide when to schedule, but of how this class is used. :) https://codereview.chromium.org/2413063002/diff/60001/blimp/engine/renderer/s... blimp/engine/renderer/scheduler.h:21: SchedulerClient* client); On 2016/10/13 18:47:27, Khushal wrote: > On 2016/10/13 16:53:52, Wez wrote: > > Who owns this? What is its lifetime? > > I don't know if there is an established pattern for this, but from what I've > seen in code, if a class takes a Delegate/Client interface in its ctor, it is > assumed that it will outlive the class. We document if a pointer can be null, > otherwise it is implicitly assumed that it can't be. And it doesn't seem of much > benefit to me to have this be stated explicitly on every class. > Happy to add a comment if you feel otherwise though. If it is made explicit that |client| is a caller-supplied delegate then that's fine, but I don't think that is presently the case. https://codereview.chromium.org/2413063002/diff/60001/blimp/engine/renderer/s... blimp/engine/renderer/scheduler.h:24: void SetNeedsMainFrame(); On 2016/10/13 18:47:27, Khushal wrote: > On 2016/10/13 16:53:52, Wez wrote: > > What is a "main frame" in the context of this scheduler? > > Added a comment. I kept the word main frame mostly to stay consistent with the > nomenclature we use to refer to frames produced by blink, so it would be obvious > to anyone looking at the code as to what frame we're talking about. Understood. However, I think it would be clearer if we had less Blink-specific naming here, e.g. if we just had StartFrameUpdate() and SetNeedsFrame() or ForceFrameUpdate(), for example (SetNeedsMainFrame() strictly describes what the call does, but in spirit is not a very helpful name ;) https://codereview.chromium.org/2413063002/diff/60001/blimp/engine/renderer/s... blimp/engine/renderer/scheduler.h:56: SchedulerClient* client_; On 2016/10/13 18:47:27, Khushal wrote: > On 2016/10/13 16:53:52, Wez wrote: > > nit: Initialize to nullptr here, just in case? > > The ctor takes it as an argument, so is it necessary? No, just suggested for consistency w/ the other members. https://codereview.chromium.org/2413063002/diff/100001/blimp/engine/renderer/... File blimp/engine/renderer/scheduler.h (right): https://codereview.chromium.org/2413063002/diff/100001/blimp/engine/renderer/... blimp/engine/renderer/scheduler.h:27: // when a frame should be produced on the engine. And how does it communicate that to the rest of the Engine? Is that via StartMainFrameUpdate, for example?
https://codereview.chromium.org/2413063002/diff/60001/blimp/engine/renderer/s... File blimp/engine/renderer/scheduler.h (right): https://codereview.chromium.org/2413063002/diff/60001/blimp/engine/renderer/s... blimp/engine/renderer/scheduler.h:17: // Responsible for scheduling frame updates sent to the client. On 2016/10/18 01:49:42, Wez wrote: > On 2016/10/13 18:47:27, Khushal wrote: > > On 2016/10/13 16:53:52, Wez wrote: > > > Can you elaborate on *how* it schedules them? All I see here are some > void() > > > functions and a next_frame_time() API. How are they used? What is the > > |client| > > > for? > > > > Added a comment. I don't think we need to get into the details about how > exactly > > the class works internally, so I mentioned the signals that it needs to make > > these decisions. > > What the |client| is for is document on that interface. > > I'm not asking for details of how we decide when to schedule, but of how this > class is used. :) Ah, added a comment about using the client interface. Better? https://codereview.chromium.org/2413063002/diff/60001/blimp/engine/renderer/s... blimp/engine/renderer/scheduler.h:21: SchedulerClient* client); On 2016/10/18 01:49:42, Wez wrote: > On 2016/10/13 18:47:27, Khushal wrote: > > On 2016/10/13 16:53:52, Wez wrote: > > > Who owns this? What is its lifetime? > > > > I don't know if there is an established pattern for this, but from what I've > > seen in code, if a class takes a Delegate/Client interface in its ctor, it is > > assumed that it will outlive the class. We document if a pointer can be null, > > otherwise it is implicitly assumed that it can't be. And it doesn't seem of > much > > benefit to me to have this be stated explicitly on every class. > > Happy to add a comment if you feel otherwise though. > > If it is made explicit that |client| is a caller-supplied delegate then that's > fine, but I don't think that is presently the case. The class takes the client in its ctor, I thought that was explicit enough. Or I guess the pattern that I'm used to seeing/using, https://cs.chromium.org/chromium/src/blimp/engine/session/page_load_tracker.h for instance. Added a comment nonetheless. :) https://codereview.chromium.org/2413063002/diff/60001/blimp/engine/renderer/s... blimp/engine/renderer/scheduler.h:24: void SetNeedsMainFrame(); On 2016/10/18 01:49:42, Wez wrote: > On 2016/10/13 18:47:27, Khushal wrote: > > On 2016/10/13 16:53:52, Wez wrote: > > > What is a "main frame" in the context of this scheduler? > > > > Added a comment. I kept the word main frame mostly to stay consistent with the > > nomenclature we use to refer to frames produced by blink, so it would be > obvious > > to anyone looking at the code as to what frame we're talking about. > > Understood. However, I think it would be clearer if we had less Blink-specific > naming here, e.g. if we just had StartFrameUpdate() and SetNeedsFrame() or > ForceFrameUpdate(), for example (SetNeedsMainFrame() strictly describes what the > call does, but in spirit is not a very helpful name ;) Sure, I don't really have a strong preference on this, either way is good. May be I've gotten to used to cc/blink naming. :P https://codereview.chromium.org/2413063002/diff/100001/blimp/engine/renderer/... File blimp/engine/renderer/scheduler.h (right): https://codereview.chromium.org/2413063002/diff/100001/blimp/engine/renderer/... blimp/engine/renderer/scheduler.h:27: // when a frame should be produced on the engine. On 2016/10/18 01:49:42, Wez wrote: > And how does it communicate that to the rest of the Engine? Is that via > StartMainFrameUpdate, for example? Done.
The CQ bit was checked by khushalsagar@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/2413063002/diff/60001/blimp/engine/renderer/s... File blimp/engine/renderer/scheduler.cc (right): https://codereview.chromium.org/2413063002/diff/60001/blimp/engine/renderer/s... blimp/engine/renderer/scheduler.cc:17: const base::TimeDelta kDefaultFrameDelay = On 2016/10/18 01:49:42, Wez wrote: > On 2016/10/13 18:47:27, Khushal wrote: > > On 2016/10/13 16:53:52, Wez wrote: > > > Is TimeDelta POD? I thought it had a ctor/dtor, so won't this result in > > > ini/fini? > > > > Its POD, doesn't have a dtor. > > > > Found an example of similar use as well, > > > https://cs.chromium.org/chromium/src/cc/scheduler/compositor_timing_history.c... > > Oh, OK; I'm surprised that assigning it a value using FromMilliseconds doesn't > trigger generation of _init code, though. Aha, looks like From*() are all constexpr! https://codereview.chromium.org/2413063002/diff/60001/blimp/engine/renderer/s... File blimp/engine/renderer/scheduler.h (right): https://codereview.chromium.org/2413063002/diff/60001/blimp/engine/renderer/s... blimp/engine/renderer/scheduler.h:21: SchedulerClient* client); On 2016/10/18 05:10:03, Khushal wrote: > On 2016/10/18 01:49:42, Wez wrote: > > On 2016/10/13 18:47:27, Khushal wrote: > > > On 2016/10/13 16:53:52, Wez wrote: > > > > Who owns this? What is its lifetime? > > > > > > I don't know if there is an established pattern for this, but from what I've > > > seen in code, if a class takes a Delegate/Client interface in its ctor, it > is > > > assumed that it will outlive the class. We document if a pointer can be > null, > > > otherwise it is implicitly assumed that it can't be. And it doesn't seem of > > much > > > benefit to me to have this be stated explicitly on every class. > > > Happy to add a comment if you feel otherwise though. > > > > If it is made explicit that |client| is a caller-supplied delegate then that's > > fine, but I don't think that is presently the case. > > The class takes the client in its ctor, I thought that was explicit enough. Or I > guess the pattern that I'm used to seeing/using, > https://cs.chromium.org/chromium/src/blimp/engine/session/page_load_tracker.h > for instance. > Added a comment nonetheless. :) If you pass a bare pointer to a function, ctor, whatever, then it isn't clear whether it's (1) used by that function, (2) retained and used for the duration of the receiving objects' lifetime, (3) owned by the recipient or (4) ref-counted, (though thankfully we have ways to express #3 & #4 these days :) Hence we need to at least clarify #1 vs #2 :) https://codereview.chromium.org/2413063002/diff/60001/blimp/engine/renderer/s... blimp/engine/renderer/scheduler.h:24: void SetNeedsMainFrame(); On 2016/10/18 05:10:03, Khushal wrote: > On 2016/10/18 01:49:42, Wez wrote: > > On 2016/10/13 18:47:27, Khushal wrote: > > > On 2016/10/13 16:53:52, Wez wrote: > > > > What is a "main frame" in the context of this scheduler? > > > > > > Added a comment. I kept the word main frame mostly to stay consistent with > the > > > nomenclature we use to refer to frames produced by blink, so it would be > > obvious > > > to anyone looking at the code as to what frame we're talking about. > > > > Understood. However, I think it would be clearer if we had less Blink-specific > > naming here, e.g. if we just had StartFrameUpdate() and SetNeedsFrame() or > > ForceFrameUpdate(), for example (SetNeedsMainFrame() strictly describes what > the > > call does, but in spirit is not a very helpful name ;) > > Sure, I don't really have a strong preference on this, either way is good. > May be I've gotten to used to cc/blink naming. :P Perhaps... :P Can we rename this to indicate what it means, though, rather than how it implements it? This call MUST be made whenever there is a change to the content that warrants generating a new frame. So something like ScheduleFrameUpdate() would seem more appropriate. Similarly, I'd reword the comment to make it explicit that this API must be called every time there is new content to render - i.e. that the scheduler won't keep scheduling frames unless you keep calling this. https://codereview.chromium.org/2413063002/diff/120001/blimp/engine/renderer/... File blimp/engine/renderer/scheduler.h (right): https://codereview.chromium.org/2413063002/diff/120001/blimp/engine/renderer/... blimp/engine/renderer/scheduler.h:20: // frame. Sorry, meant to comment last time; it's not clear what it means to "synchronously start" something - does it start preparing the next frame *asynchronously*? Also, since no frame-id is specified, it's not clear what is meant by "the requested frame". Suggest something like: "Called by the Scheduler to start preparation of the next frame update." https://codereview.chromium.org/2413063002/diff/120001/blimp/engine/renderer/... blimp/engine/renderer/scheduler.h:28: // The user provides the SchedulerClient to be notified of when a frame update nit: s/user/caller s/the/a https://codereview.chromium.org/2413063002/diff/120001/blimp/engine/renderer/... blimp/engine/renderer/scheduler.h:30: class Scheduler { nit: Scheduler seems awfully generic for something specific to frame scheduling; how about FrameScheduler? Have you considered re-naming SchedulerClient to FrameScheduler::Delegate? https://codereview.chromium.org/2413063002/diff/140001/blimp/engine/renderer/... File blimp/engine/renderer/scheduler.cc (right): https://codereview.chromium.org/2413063002/diff/140001/blimp/engine/renderer/... blimp/engine/renderer/scheduler.cc:45: DCHECK(!frame_ack_pending_) << "We can have only frame update in flight"; nit: "...only one frame..." That said, I'd recommend not bothering with additional log text unless there's actually interested data to log - if this DCHECK fires then the stack identifies the condition pretty clearly, doesn't it? https://codereview.chromium.org/2413063002/diff/140001/blimp/engine/renderer/... blimp/engine/renderer/scheduler.cc:75: return needs_frame_update_ && !frame_ack_pending_; This logic doesn't seem to match the function name; the function name is *Can*ProduceMainFrames, whereas the logic is "did the caller explicitly indicate that they want a frame, AND did the last frame we produced get ACK'd" - i.e. not whether we *can* produce a frame, but whether we *should*, in some sense. Renaming the function to ReadyForNextFrameUpdate() might help clarify the intent here. https://codereview.chromium.org/2413063002/diff/140001/blimp/engine/renderer/... File blimp/engine/renderer/scheduler.h (right): https://codereview.chromium.org/2413063002/diff/140001/blimp/engine/renderer/... blimp/engine/renderer/scheduler.h:54: void ScheduleMainFrameIfNecessary(); nit: Rename this and the other two internal methods not to refer to "MainFrame", just Frame?
https://codereview.chromium.org/2413063002/diff/140001/blimp/engine/renderer/... File blimp/engine/renderer/blimp_remote_compositor_bridge.cc (right): https://codereview.chromium.org/2413063002/diff/140001/blimp/engine/renderer/... blimp/engine/renderer/blimp_remote_compositor_bridge.cc:21: DCHECK(remote_proto_channel_); nit: No need to DCHECK if you're about to de-ref. https://codereview.chromium.org/2413063002/diff/140001/blimp/engine/renderer/... File blimp/engine/renderer/blimp_remote_compositor_bridge.h (right): https://codereview.chromium.org/2413063002/diff/140001/blimp/engine/renderer/... blimp/engine/renderer/blimp_remote_compositor_bridge.h:25: cc::RemoteProtoChannel* remote_proto_channel, nit: Clarify ownership/lifetime of this? https://codereview.chromium.org/2413063002/diff/140001/blimp/engine/renderer/... File blimp/engine/renderer/scheduler.cc (right): https://codereview.chromium.org/2413063002/diff/140001/blimp/engine/renderer/... blimp/engine/renderer/scheduler.cc:88: next_frame_time_ = base::TimeTicks::Now() + frame_delay_; nit: Is this the desired next-frame-time, bearing in mind that StartMainFrame may be triggered in response to SetNeedsFrameUpdate OR DidReceiveFrameUpdateAck()? i.e. if we have a continuously updating page, and a 30ms frame-delay, and a 15ms round-trip delay then we'll actually get frames generated every 45ms, not every 30ms. https://codereview.chromium.org/2413063002/diff/140001/cc/blimp/layer_tree_ho... File cc/blimp/layer_tree_host_remote.cc (right): https://codereview.chromium.org/2413063002/diff/140001/cc/blimp/layer_tree_ho... cc/blimp/layer_tree_host_remote.cc:240: // on initialization, even if the settings end up being a no-op. This comment is rather confusing. Are you simply saying that LTHR ignores |debug_state| since there's no actual compositing happening at the sending end - only the remote end needs to have debug state set? https://codereview.chromium.org/2413063002/diff/140001/cc/proto/compositor_me... File cc/proto/compositor_message.proto (right): https://codereview.chromium.org/2413063002/diff/140001/cc/proto/compositor_me... cc/proto/compositor_message.proto:26: // successfully on the client. nit: This wording implies that there is a possibility of _failure_ to process a frame update on the Client; is that the case..? What does the value of the bool indicate? Is the presence of the field all that matters? Would it be clearer to use an empty message rather than a bool? https://codereview.chromium.org/2413063002/diff/140001/content/renderer/gpu/r... File content/renderer/gpu/render_widget_compositor.cc (right): https://codereview.chromium.org/2413063002/diff/140001/content/renderer/gpu/r... content/renderer/gpu/render_widget_compositor.cc:243: DCHECK(!threaded_); nit: Command-line switches come from outside the process; what will happen if we somehow get mismatched switches in a Release build? What is the failure mode then?
https://codereview.chromium.org/2413063002/diff/60001/blimp/engine/renderer/s... File blimp/engine/renderer/scheduler.h (right): https://codereview.chromium.org/2413063002/diff/60001/blimp/engine/renderer/s... blimp/engine/renderer/scheduler.h:21: SchedulerClient* client); On 2016/10/19 22:01:34, Wez wrote: > On 2016/10/18 05:10:03, Khushal wrote: > > On 2016/10/18 01:49:42, Wez wrote: > > > On 2016/10/13 18:47:27, Khushal wrote: > > > > On 2016/10/13 16:53:52, Wez wrote: > > > > > Who owns this? What is its lifetime? > > > > > > > > I don't know if there is an established pattern for this, but from what > I've > > > > seen in code, if a class takes a Delegate/Client interface in its ctor, it > > is > > > > assumed that it will outlive the class. We document if a pointer can be > > null, > > > > otherwise it is implicitly assumed that it can't be. And it doesn't seem > of > > > much > > > > benefit to me to have this be stated explicitly on every class. > > > > Happy to add a comment if you feel otherwise though. > > > > > > If it is made explicit that |client| is a caller-supplied delegate then > that's > > > fine, but I don't think that is presently the case. > > > > The class takes the client in its ctor, I thought that was explicit enough. Or > I > > guess the pattern that I'm used to seeing/using, > > https://cs.chromium.org/chromium/src/blimp/engine/session/page_load_tracker.h > > for instance. > > Added a comment nonetheless. :) > > If you pass a bare pointer to a function, ctor, whatever, then it isn't clear > whether it's (1) used by that function, (2) retained and used for the duration > of the receiving objects' lifetime, (3) owned by the recipient or (4) > ref-counted, (though thankfully we have ways to express #3 & #4 these days :) > > Hence we need to at least clarify #1 vs #2 :) I understand that in general when you take a bare pointer, you need to clarify what the its lifetime expectations are, i.e., for how much duration are you expecting the caller to keep that object alive. What I meant to say was that its a very common pattern in the code base to inject a FooClient* or FooDelegate* when constructing Foo, and I have rarely seen comments on Foo specifying the lifetime of these interfaces in this case. It is assumed that the delegate or client will outlive Foo and comments generally specify when there is a divergence from this expectation. https://codereview.chromium.org/2413063002/diff/60001/blimp/engine/renderer/s... blimp/engine/renderer/scheduler.h:24: void SetNeedsMainFrame(); On 2016/10/19 22:01:34, Wez wrote: > On 2016/10/18 05:10:03, Khushal wrote: > > On 2016/10/18 01:49:42, Wez wrote: > > > On 2016/10/13 18:47:27, Khushal wrote: > > > > On 2016/10/13 16:53:52, Wez wrote: > > > > > What is a "main frame" in the context of this scheduler? > > > > > > > > Added a comment. I kept the word main frame mostly to stay consistent with > > the > > > > nomenclature we use to refer to frames produced by blink, so it would be > > > obvious > > > > to anyone looking at the code as to what frame we're talking about. > > > > > > Understood. However, I think it would be clearer if we had less > Blink-specific > > > naming here, e.g. if we just had StartFrameUpdate() and SetNeedsFrame() or > > > ForceFrameUpdate(), for example (SetNeedsMainFrame() strictly describes what > > the > > > call does, but in spirit is not a very helpful name ;) > > > > Sure, I don't really have a strong preference on this, either way is good. > > May be I've gotten to used to cc/blink naming. :P > > Perhaps... :P > > Can we rename this to indicate what it means, though, rather than how it > implements it? This call MUST be made whenever there is a change to the content > that warrants generating a new frame. So something like ScheduleFrameUpdate() > would seem more appropriate. Similarly, I'd reword the comment to make it > explicit that this API must be called every time there is new content to render > - i.e. that the scheduler won't keep scheduling frames unless you keep calling > this. Actually that comment will not be accurate. Since calling this does not necessarily mean that there is any new content to render. It will also be called for requestAnimationFrame for instance, where the page just wants to do something on the next vsync but the page content might not have changed at all. The other thing is I don't think this class should have to care about the reasons why a caller needs to start a frame. You call it when you want the scheduler to tell you when its a good time to start a frame update, why you need that frame is up to you. I've renamed the method to ScheduleFrameUpdate if that is more descriptive. https://codereview.chromium.org/2413063002/diff/120001/blimp/engine/renderer/... File blimp/engine/renderer/scheduler.h (right): https://codereview.chromium.org/2413063002/diff/120001/blimp/engine/renderer/... blimp/engine/renderer/scheduler.h:20: // frame. On 2016/10/19 22:01:34, Wez wrote: > Sorry, meant to comment last time; it's not clear what it means to > "synchronously start" something - does it start preparing the next frame > *asynchronously*? > > Also, since no frame-id is specified, it's not clear what is meant by "the > requested frame". > > Suggest something like: "Called by the Scheduler to start preparation of the > next frame update." I wanted to write synchronously because the assumption is that once this method is called, the frame request has been responded to. So if you call ScheduleFrameUpdate from this callback, then that will be considered as a request to schedule another frame. May be synchronously doesn't help in stating that so dropped. Updated the comment to clarify that instead. https://codereview.chromium.org/2413063002/diff/120001/blimp/engine/renderer/... blimp/engine/renderer/scheduler.h:28: // The user provides the SchedulerClient to be notified of when a frame update On 2016/10/19 22:01:34, Wez wrote: > nit: s/user/caller s/the/a Done. https://codereview.chromium.org/2413063002/diff/120001/blimp/engine/renderer/... blimp/engine/renderer/scheduler.h:28: // The user provides the SchedulerClient to be notified of when a frame update On 2016/10/19 22:01:34, Wez wrote: > nit: s/user/caller s/the/a Done. https://codereview.chromium.org/2413063002/diff/120001/blimp/engine/renderer/... blimp/engine/renderer/scheduler.h:30: class Scheduler { On 2016/10/19 22:01:34, Wez wrote: > nit: Scheduler seems awfully generic for something specific to frame scheduling; > how about FrameScheduler? > > Have you considered re-naming SchedulerClient to FrameScheduler::Delegate? I renamed it to FrameScheduler. I think FrameSchedulerClient is fine for the callback interface. https://codereview.chromium.org/2413063002/diff/140001/blimp/engine/renderer/... File blimp/engine/renderer/blimp_remote_compositor_bridge.cc (right): https://codereview.chromium.org/2413063002/diff/140001/blimp/engine/renderer/... blimp/engine/renderer/blimp_remote_compositor_bridge.cc:21: DCHECK(remote_proto_channel_); On 2016/10/19 22:15:27, Wez wrote: > nit: No need to DCHECK if you're about to de-ref. Done. https://codereview.chromium.org/2413063002/diff/140001/blimp/engine/renderer/... File blimp/engine/renderer/blimp_remote_compositor_bridge.h (right): https://codereview.chromium.org/2413063002/diff/140001/blimp/engine/renderer/... blimp/engine/renderer/blimp_remote_compositor_bridge.h:25: cc::RemoteProtoChannel* remote_proto_channel, On 2016/10/19 22:15:27, Wez wrote: > nit: Clarify ownership/lifetime of this? Done. https://codereview.chromium.org/2413063002/diff/140001/blimp/engine/renderer/... File blimp/engine/renderer/scheduler.cc (right): https://codereview.chromium.org/2413063002/diff/140001/blimp/engine/renderer/... blimp/engine/renderer/scheduler.cc:45: DCHECK(!frame_ack_pending_) << "We can have only frame update in flight"; On 2016/10/19 22:01:34, Wez wrote: > nit: "...only one frame..." > > That said, I'd recommend not bothering with additional log text unless there's > actually interested data to log - if this DCHECK fires then the stack identifies > the condition pretty clearly, doesn't it? The reason why this is an error is because the Scheduler is implementing the protocol policy of having only one frame with a pending ack at a time. It nice to have a helpful log that makes the error that we have here explicit. https://codereview.chromium.org/2413063002/diff/140001/blimp/engine/renderer/... blimp/engine/renderer/scheduler.cc:75: return needs_frame_update_ && !frame_ack_pending_; On 2016/10/19 22:01:34, Wez wrote: > This logic doesn't seem to match the function name; the function name is > *Can*ProduceMainFrames, whereas the logic is "did the caller explicitly indicate > that they want a frame, AND did the last frame we produced get ACK'd" - i.e. not > whether we *can* produce a frame, but whether we *should*, in some sense. > > Renaming the function to ReadyForNextFrameUpdate() might help clarify the intent > here. I just renamed it to ShouldProduceMainFrames, that looks clear enough. https://codereview.chromium.org/2413063002/diff/140001/blimp/engine/renderer/... blimp/engine/renderer/scheduler.cc:88: next_frame_time_ = base::TimeTicks::Now() + frame_delay_; On 2016/10/19 22:15:27, Wez wrote: > nit: Is this the desired next-frame-time, bearing in mind that StartMainFrame > may be triggered in response to SetNeedsFrameUpdate OR > DidReceiveFrameUpdateAck()? i.e. if we have a continuously updating page, and a > 30ms frame-delay, and a 15ms round-trip delay then we'll actually get frames > generated every 45ms, not every 30ms. Yeah, but if it is triggered in response to DidReceiveFrameUpdateAck, the delay used will still be based on the time when the last frame was produced. The |next_frame_time_| is set each time we start a frame update. If we sent an update at 0ms, the next frame time will be 30 ms. Now for a round trip of 15 ms, we get an ack at 15ms. The delay used for scheduling the task for starting the frame will be |next_frame_time_| - base::TimeTicks::Now(), i.e., 30 ms - 15ms = 15ms. So the frame will still kick-off at 30 ms. Even if the round trip is longer than the frame delay itself. For instance, for a round trip of 45 ms, the timer will run StartMainFrame at 30ms, early out because we'll have the frame ack pending and not update the |next_frame_time_|. Now when we get the ack at 45 ms, the delay for starting the frame will be, 30ms - 45ms = -15ms, which will schedule the task with no delay. So this should make sure that there is at least a 30 ms delay between any 2 frames, in the case where the network round trip is lower than that, or if running main frames is not resulting in any updates to the client at all. https://codereview.chromium.org/2413063002/diff/140001/blimp/engine/renderer/... File blimp/engine/renderer/scheduler.h (right): https://codereview.chromium.org/2413063002/diff/140001/blimp/engine/renderer/... blimp/engine/renderer/scheduler.h:54: void ScheduleMainFrameIfNecessary(); On 2016/10/19 22:01:34, Wez wrote: > nit: Rename this and the other two internal methods not to refer to "MainFrame", > just Frame? Done. https://codereview.chromium.org/2413063002/diff/140001/cc/blimp/layer_tree_ho... File cc/blimp/layer_tree_host_remote.cc (right): https://codereview.chromium.org/2413063002/diff/140001/cc/blimp/layer_tree_ho... cc/blimp/layer_tree_host_remote.cc:240: // on initialization, even if the settings end up being a no-op. On 2016/10/19 22:15:28, Wez wrote: > This comment is rather confusing. Are you simply saying that LTHR ignores > |debug_state| since there's no actual compositing happening at the sending end - > only the remote end needs to have debug state set? Yes, only the remoting end should need to have this set and I'm not sure if there is a use-case for having this plumbed to the client from the engine through compositor messages, rather than something setting it on the client-side directly. I think dev-tools uses this, but I wanted to defer adding it till we know how that is going to be setup. Wdyt? https://codereview.chromium.org/2413063002/diff/140001/cc/proto/compositor_me... File cc/proto/compositor_message.proto (right): https://codereview.chromium.org/2413063002/diff/140001/cc/proto/compositor_me... cc/proto/compositor_message.proto:26: // successfully on the client. On 2016/10/19 22:15:28, Wez wrote: > nit: This wording implies that there is a possibility of _failure_ to process a > frame update on the Client; is that the case..? Oh I wanted to specify that this will be in response to the feature receiving the update and applying it to the compositor state. Applying of the update can never fail. > > What does the value of the bool indicate? Is the presence of the field all that > matters? Would it be clearer to use an empty message rather than a bool? The value of the bool is irrelevant. Its just the presence of the field. I think this should be an enum with the message type. I'd say please ignore the structure of protos here, I'm not paying much attention to cleaning up the protos for control messages right now. These are going to be nuked shortly. Its a bit of a mess from the old path which depends on these and I want to migrate to the new path quickly so we can remove all the old code that depends on these messages, and re-define them clearly in blimp/. I think the bool is fine for now. The next message type to come here is going to be scroll and scale updates from the client so having an empty message is going to be more confusing then. https://codereview.chromium.org/2413063002/diff/140001/content/renderer/gpu/r... File content/renderer/gpu/render_widget_compositor.cc (right): https://codereview.chromium.org/2413063002/diff/140001/content/renderer/gpu/r... content/renderer/gpu/render_widget_compositor.cc:243: DCHECK(!threaded_); On 2016/10/19 22:15:28, Wez wrote: > nit: Command-line switches come from outside the process; what will happen if we > somehow get mismatched switches in a Release build? What is the failure mode > then? I'm not sure what you mean by mis-matched switches. The only thing this DCHECK is verifying is that we did not create the compositor thread since that is also done based on the kUseRemoteCompositing switch here, https://cs.chromium.org/chromium/src/content/renderer/render_thread_impl.cc?s...
The CQ bit was checked by khushalsagar@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/2413063002/diff/140001/blimp/engine/renderer/... File blimp/engine/renderer/scheduler.cc (right): https://codereview.chromium.org/2413063002/diff/140001/blimp/engine/renderer/... blimp/engine/renderer/scheduler.cc:88: next_frame_time_ = base::TimeTicks::Now() + frame_delay_; On 2016/10/20 01:29:33, Khushal wrote: > On 2016/10/19 22:15:27, Wez wrote: > > nit: Is this the desired next-frame-time, bearing in mind that StartMainFrame > > may be triggered in response to SetNeedsFrameUpdate OR > > DidReceiveFrameUpdateAck()? i.e. if we have a continuously updating page, and > a > > 30ms frame-delay, and a 15ms round-trip delay then we'll actually get frames > > generated every 45ms, not every 30ms. > > Yeah, but if it is triggered in response to DidReceiveFrameUpdateAck, the delay > used will still be based on the time when the last frame was produced. > > The |next_frame_time_| is set each time we start a frame update. If we sent an > update at 0ms, the next frame time will be 30 ms. Now for a round trip of 15 ms, > we get an ack at 15ms. The delay used for scheduling the task for starting the > frame will be |next_frame_time_| - base::TimeTicks::Now(), i.e., 30 ms - 15ms = > 15ms. So the frame will still kick-off at 30 ms. > > Even if the round trip is longer than the frame delay itself. For instance, for > a round trip of 45 ms, the timer will run StartMainFrame at 30ms, early out > because we'll have the frame ack pending and not update the |next_frame_time_|. > Now when we get the ack at 45 ms, the delay for starting the frame will be, 30ms > - 45ms = -15ms, which will schedule the task with no delay. So this should make > sure that there is at least a 30 ms delay between any 2 frames, in the case > where the network round trip is lower than that, or if running main frames is > not resulting in any updates to the client at all. Acknowledged. https://codereview.chromium.org/2413063002/diff/180001/blimp/engine/renderer/... File blimp/engine/renderer/frame_scheduler.cc (right): https://codereview.chromium.org/2413063002/diff/180001/blimp/engine/renderer/... blimp/engine/renderer/frame_scheduler.cc:84: // the client. It's really not clear from this comment under which conditions it is even possible for this to occur - IIUC it's basically because of the separation between StartFrameUpdate and DidSendFrameUpdateToClient? Is there a reason why we can't set |frame_ack_pending_| to true here, and eliminate DidSendFrameUpdateToClient(), to simplify this state-machine? https://codereview.chromium.org/2413063002/diff/180001/blimp/engine/renderer/... File blimp/engine/renderer/frame_scheduler.h (right): https://codereview.chromium.org/2413063002/diff/180001/blimp/engine/renderer/... blimp/engine/renderer/frame_scheduler.h:19: // Used to notify the SchedulerClient to start the requested frame update. nit: SchedulerClient naming, here and elsewhere. https://codereview.chromium.org/2413063002/diff/180001/blimp/engine/renderer/... blimp/engine/renderer/frame_scheduler.h:47: base::TimeTicks next_frame_time() const { return next_frame_time_; } Can this be removed? I can't see anything using it?
https://codereview.chromium.org/2413063002/diff/180001/blimp/engine/renderer/... File blimp/engine/renderer/frame_scheduler.cc (right): https://codereview.chromium.org/2413063002/diff/180001/blimp/engine/renderer/... blimp/engine/renderer/frame_scheduler.cc:84: // the client. On 2016/10/20 23:53:27, Wez wrote: > It's really not clear from this comment under which conditions it is even > possible for this to occur - IIUC it's basically because of the separation > between StartFrameUpdate and DidSendFrameUpdateToClient? Is there a reason why > we can't set |frame_ack_pending_| to true here, and eliminate > DidSendFrameUpdateToClient(), to simplify this state-machine? Hmm, so StartFrameUpdate won't necessarily result in DidSendFrameUpdateToClient. For instance, if it does not result in any content changes. Basically if we reach here, https://cs.chromium.org/chromium/src/cc/blimp/layer_tree_host_remote.cc?sq=pa... So we can have the state where we run StartFrameUpdate, running this causes ScheduleFrameUpdate to be called, because there can be a request to schedule the next frame while we are updating for the current one, which will queue a task with the |frame_tick_timer_|, and we then have a DidSendFrameUpdateToClient. Though what we can probably do is, if we get a DidSendFrameUpdateToClient, we can reset the frame_tick_timer_ and remove this early out here instead. Does that sound cleaner? https://codereview.chromium.org/2413063002/diff/180001/blimp/engine/renderer/... File blimp/engine/renderer/frame_scheduler.h (right): https://codereview.chromium.org/2413063002/diff/180001/blimp/engine/renderer/... blimp/engine/renderer/frame_scheduler.h:47: base::TimeTicks next_frame_time() const { return next_frame_time_; } On 2016/10/20 23:53:27, Wez wrote: > Can this be removed? I can't see anything using it? Oh, I think some test was. Will remove.
https://codereview.chromium.org/2413063002/diff/180001/blimp/engine/renderer/... File blimp/engine/renderer/frame_scheduler.cc (right): https://codereview.chromium.org/2413063002/diff/180001/blimp/engine/renderer/... blimp/engine/renderer/frame_scheduler.cc:49: ScheduleFrameUpdateIfNecessary(); Wait... this call will never have any effect, surely? :P https://codereview.chromium.org/2413063002/diff/180001/blimp/engine/renderer/... blimp/engine/renderer/frame_scheduler.cc:84: // the client. On 2016/10/21 00:03:11, Khushal wrote: > On 2016/10/20 23:53:27, Wez wrote: > > It's really not clear from this comment under which conditions it is even > > possible for this to occur - IIUC it's basically because of the separation > > between StartFrameUpdate and DidSendFrameUpdateToClient? Is there a reason > why > > we can't set |frame_ack_pending_| to true here, and eliminate > > DidSendFrameUpdateToClient(), to simplify this state-machine? > > Hmm, so StartFrameUpdate won't necessarily result in DidSendFrameUpdateToClient. > For instance, if it does not result in any content changes. Basically if we > reach here, > https://cs.chromium.org/chromium/src/cc/blimp/layer_tree_host_remote.cc?sq=pa... > > So we can have the state where we run StartFrameUpdate, running this causes > ScheduleFrameUpdate to be called, because there can be a request to schedule the > next frame while we are updating for the current one, which will queue a task > with the |frame_tick_timer_|, and we then have a DidSendFrameUpdateToClient. > > Though what we can probably do is, if we get a DidSendFrameUpdateToClient, we > can reset the frame_tick_timer_ and remove this early out here instead. Does > that sound cleaner? Hmmm. Resetting the timer in DidSendFrameUpdateToClient will lead to clock-churn in some cases; we'll stop the timer (leaving a no-op delayed task in the queue) only to likely create an identical timer task when the ACK arrives. But it'll only be one wasted timer task, I suppose, so probably not worth worrying about... However, the lack of a 1:1 correspondence between StartFrameUpdate and DidSendFrameUpdateToClient is really what creates the confusion. With the current impl, if creating the frame update is super slow then we could conceivably call StartFrameUpdate multiple times before DidSendFrameUpdateToClient is even called (Though obviously not if it's synchronous). Is that actually possible, though? Really the required semantics of StartFrameUpdate and the Did* APIs needs to be spelled-out in the header (e.g. that StartFrameUpdate MAY result in DidSend*, that DidSend* MUST be followed by DidReceive*Ack eventually, etc).
https://codereview.chromium.org/2413063002/diff/180001/blimp/engine/renderer/... File blimp/engine/renderer/frame_scheduler.cc (right): https://codereview.chromium.org/2413063002/diff/180001/blimp/engine/renderer/... blimp/engine/renderer/frame_scheduler.cc:49: ScheduleFrameUpdateIfNecessary(); On 2016/10/21 01:40:35, Wez wrote: > Wait... this call will never have any effect, surely? :P Woops. :P Will remove. https://codereview.chromium.org/2413063002/diff/180001/blimp/engine/renderer/... blimp/engine/renderer/frame_scheduler.cc:84: // the client. On 2016/10/21 01:40:35, Wez wrote: > On 2016/10/21 00:03:11, Khushal wrote: > > On 2016/10/20 23:53:27, Wez wrote: > > > It's really not clear from this comment under which conditions it is even > > > possible for this to occur - IIUC it's basically because of the separation > > > between StartFrameUpdate and DidSendFrameUpdateToClient? Is there a reason > > why > > > we can't set |frame_ack_pending_| to true here, and eliminate > > > DidSendFrameUpdateToClient(), to simplify this state-machine? > > > > Hmm, so StartFrameUpdate won't necessarily result in > DidSendFrameUpdateToClient. > > For instance, if it does not result in any content changes. Basically if we > > reach here, > > > https://cs.chromium.org/chromium/src/cc/blimp/layer_tree_host_remote.cc?sq=pa... > > > > So we can have the state where we run StartFrameUpdate, running this causes > > ScheduleFrameUpdate to be called, because there can be a request to schedule > the > > next frame while we are updating for the current one, which will queue a task > > with the |frame_tick_timer_|, and we then have a DidSendFrameUpdateToClient. > > > > Though what we can probably do is, if we get a DidSendFrameUpdateToClient, we > > can reset the frame_tick_timer_ and remove this early out here instead. Does > > that sound cleaner? > > Hmmm. Resetting the timer in DidSendFrameUpdateToClient will lead to clock-churn > in some cases; we'll stop the timer (leaving a no-op delayed task in the queue) > only to likely create an identical timer task when the ACK arrives. But it'll > only be one wasted timer task, I suppose, so probably not worth worrying > about... > > However, the lack of a 1:1 correspondence between StartFrameUpdate and > DidSendFrameUpdateToClient is really what creates the confusion. With the > current impl, if creating the frame update is super slow then we could > conceivably call StartFrameUpdate multiple times before > DidSendFrameUpdateToClient is even called (Though obviously not if it's > synchronous). Is that actually possible, though? Nope, that's not possible. DidSendFrameUpdateToClient should be called synchronously when we are within a frame update. I'll DCHECK that here to make it obvious too. > > Really the required semantics of StartFrameUpdate and the Did* APIs needs to be > spelled-out in the header (e.g. that StartFrameUpdate MAY result in DidSend*, > that DidSend* MUST be followed by DidReceive*Ack eventually, etc). Sure, let me write some more detailed comments here to clarify the state expectations.
https://codereview.chromium.org/2413063002/diff/180001/blimp/engine/renderer/... File blimp/engine/renderer/frame_scheduler.cc (right): https://codereview.chromium.org/2413063002/diff/180001/blimp/engine/renderer/... blimp/engine/renderer/frame_scheduler.cc:49: ScheduleFrameUpdateIfNecessary(); On 2016/10/21 03:13:56, Khushal wrote: > On 2016/10/21 01:40:35, Wez wrote: > > Wait... this call will never have any effect, surely? :P > > Woops. :P Will remove. Done. https://codereview.chromium.org/2413063002/diff/180001/blimp/engine/renderer/... blimp/engine/renderer/frame_scheduler.cc:84: // the client. On 2016/10/21 03:13:56, Khushal wrote: > On 2016/10/21 01:40:35, Wez wrote: > > On 2016/10/21 00:03:11, Khushal wrote: > > > On 2016/10/20 23:53:27, Wez wrote: > > > > It's really not clear from this comment under which conditions it is even > > > > possible for this to occur - IIUC it's basically because of the separation > > > > between StartFrameUpdate and DidSendFrameUpdateToClient? Is there a > reason > > > why > > > > we can't set |frame_ack_pending_| to true here, and eliminate > > > > DidSendFrameUpdateToClient(), to simplify this state-machine? > > > > > > Hmm, so StartFrameUpdate won't necessarily result in > > DidSendFrameUpdateToClient. > > > For instance, if it does not result in any content changes. Basically if we > > > reach here, > > > > > > https://cs.chromium.org/chromium/src/cc/blimp/layer_tree_host_remote.cc?sq=pa... > > > > > > So we can have the state where we run StartFrameUpdate, running this causes > > > ScheduleFrameUpdate to be called, because there can be a request to schedule > > the > > > next frame while we are updating for the current one, which will queue a > task > > > with the |frame_tick_timer_|, and we then have a DidSendFrameUpdateToClient. > > > > > > Though what we can probably do is, if we get a DidSendFrameUpdateToClient, > we > > > can reset the frame_tick_timer_ and remove this early out here instead. Does > > > that sound cleaner? > > > > Hmmm. Resetting the timer in DidSendFrameUpdateToClient will lead to > clock-churn > > in some cases; we'll stop the timer (leaving a no-op delayed task in the > queue) > > only to likely create an identical timer task when the ACK arrives. But it'll > > only be one wasted timer task, I suppose, so probably not worth worrying > > about... > > > > However, the lack of a 1:1 correspondence between StartFrameUpdate and > > DidSendFrameUpdateToClient is really what creates the confusion. With the > > current impl, if creating the frame update is super slow then we could > > conceivably call StartFrameUpdate multiple times before > > DidSendFrameUpdateToClient is even called (Though obviously not if it's > > synchronous). Is that actually possible, though? > > Nope, that's not possible. DidSendFrameUpdateToClient should be called > synchronously when we are within a frame update. I'll DCHECK that here to make > it obvious too. > > > > > Really the required semantics of StartFrameUpdate and the Did* APIs needs to > be > > spelled-out in the header (e.g. that StartFrameUpdate MAY result in DidSend*, > > that DidSend* MUST be followed by DidReceive*Ack eventually, etc). > > Sure, let me write some more detailed comments here to clarify the state > expectations. Done. https://codereview.chromium.org/2413063002/diff/180001/blimp/engine/renderer/... File blimp/engine/renderer/frame_scheduler.h (right): https://codereview.chromium.org/2413063002/diff/180001/blimp/engine/renderer/... blimp/engine/renderer/frame_scheduler.h:47: base::TimeTicks next_frame_time() const { return next_frame_time_; } On 2016/10/21 00:03:11, Khushal wrote: > On 2016/10/20 23:53:27, Wez wrote: > > Can this be removed? I can't see anything using it? > > Oh, I think some test was. Will remove. Done.
LGTM w/ nits https://codereview.chromium.org/2413063002/diff/180001/blimp/engine/renderer/... File blimp/engine/renderer/frame_scheduler.h (right): https://codereview.chromium.org/2413063002/diff/180001/blimp/engine/renderer/... blimp/engine/renderer/frame_scheduler.h:19: // Used to notify the SchedulerClient to start the requested frame update. On 2016/10/20 23:53:27, Wez wrote: > nit: SchedulerClient naming, here and elsewhere. Pingy https://codereview.chromium.org/2413063002/diff/200001/blimp/engine/renderer/... File blimp/engine/renderer/frame_scheduler.h (right): https://codereview.chromium.org/2413063002/diff/200001/blimp/engine/renderer/... blimp/engine/renderer/frame_scheduler.h:42: // |client| will be asynchronously informed of starting a frame update using nit: "|client->StartFrameUpdate() will be called[-back?] asynchronously when the scheduler is ready for the next frame to be started."? https://codereview.chromium.org/2413063002/diff/200001/blimp/engine/renderer/... blimp/engine/renderer/frame_scheduler.h:48: // client. Make explicit that this MUST be called from within StartFrameUpdate(), before it returns, if that is the requirement. Or, better still, get rid of this and have StartFrameUpdate() return a bool indicating whether or not it actually sent anything.
https://codereview.chromium.org/2413063002/diff/180001/blimp/engine/renderer/... File blimp/engine/renderer/frame_scheduler.h (right): https://codereview.chromium.org/2413063002/diff/180001/blimp/engine/renderer/... blimp/engine/renderer/frame_scheduler.h:19: // Used to notify the SchedulerClient to start the requested frame update. On 2016/10/25 21:50:09, Wez wrote: > On 2016/10/20 23:53:27, Wez wrote: > > nit: SchedulerClient naming, here and elsewhere. > > Pingy Done. https://codereview.chromium.org/2413063002/diff/200001/blimp/engine/renderer/... File blimp/engine/renderer/frame_scheduler.h (right): https://codereview.chromium.org/2413063002/diff/200001/blimp/engine/renderer/... blimp/engine/renderer/frame_scheduler.h:42: // |client| will be asynchronously informed of starting a frame update using On 2016/10/25 21:50:09, Wez wrote: > nit: "|client->StartFrameUpdate() will be called[-back?] asynchronously when the > scheduler is ready for the next frame to be started."? Done. https://codereview.chromium.org/2413063002/diff/200001/blimp/engine/renderer/... blimp/engine/renderer/frame_scheduler.h:48: // client. On 2016/10/25 21:50:09, Wez wrote: > Make explicit that this MUST be called from within StartFrameUpdate(), before it > returns, if that is the requirement. Or, better still, get rid of this and have > StartFrameUpdate() return a bool indicating whether or not it actually sent > anything. I added a DCHECK to make sure it is only called from StartFrameUpdate. Otherwise to return the bool from that function I would be doing the same thing in BlimpRemoteCompositorBridge.
The CQ bit was checked by khushalsagar@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from piman@chromium.org, danakj@chromium.org, wez@chromium.org Link to the patchset: https://codereview.chromium.org/2413063002/#ps240001 (title: "Rebase.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #13 (id:240001)
Message was sent while issue was closed.
Description was changed from ========== content/blimp: Set up hooks for enabling LTHRemote in the renderer. Sets up hooks for requesting a RemoteCompositorBridge from the ContentRendererClient, which is necessary to create a remote LayerTreeHost. Also adds an implementation of the Bridge in blimp. BUG=648442 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel ========== to ========== content/blimp: Set up hooks for enabling LTHRemote in the renderer. Sets up hooks for requesting a RemoteCompositorBridge from the ContentRendererClient, which is necessary to create a remote LayerTreeHost. Also adds an implementation of the Bridge in blimp. BUG=648442 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel Committed: https://crrev.com/d3cbb4d90d7367030d637cbb7b68b2293f6ebe06 Cr-Commit-Position: refs/heads/master@{#427569} ==========
Message was sent while issue was closed.
Patchset 13 (id:??) landed as https://crrev.com/d3cbb4d90d7367030d637cbb7b68b2293f6ebe06 Cr-Commit-Position: refs/heads/master@{#427569} |
