Add unthrottled_interval to BeginFrameArgs [chromium/src : main]
Maggie Chen (Gerrit)
Maggie Chen added 2 comments![]( "Open in Gerrit")
min_refresh_interval_));Maggie Chen@mag...@chromium.org as you mentioned in a separate comment this value can become stale - but likely good to fix that in a separate CL to keep this one focused π
Let me merge `nominal_refresh_period_` into `min_refresh_interval_`.
update_vsync_params_callback_.Run(frame_time, nominal_refresh_period_);Maggie ChenBeginFrameSource will call UpdateVSyncParametersCallback() for any frame interval change. Can we update the unthrottled interval based on this function call instead of adding it to ui::VSyncParamsMac? It doesn't seem we need to know the same unthrottled interval for every single frame.
Sky DebreuilI meant not to add the unthrottled interval to either ui::VSyncParamsMac orBeginFrameArgs.
Maggie ChenNice - I chatted with Jonathan & he mentioned that it would likely be useful to have the `unthrottled_interval` on `BeginFrameArgs`.
I'm doing a follow up CL which uses the unthrottled value in `CompositorFrameSinkSupport::OnBeginFrame` so that would make it easy to acces - and he also mentioned there are use cases for other teams to have access to it there as well.
I will see if I can simplify I am doing here. I think maybe I can just use the existing `min_refresh_interval_` value on `ExternalBeginFrameSourceMac` but will see.
Thanks π
Sky Debreuil>> he mentioned that it would likely be useful to have the `unthrottled_interval` on `BeginFrameArgs`.
Is it because it's difficult or inconvenient to use the existing callback for `unthrottled_interval` change?
>> I think maybe I can just use the existing min_refresh_interval_ value on ExternalBeginFrameSourceMac
I will need to clean up the code because we currently have `nominal_refresh_period_` and `min_refresh_interval_`. `min_refresh_interval_` is could be stale when we read it. Since we probably won't support refresh rate range, we can keep just one of the two.
Jonathan RossAs I understand it, the rationale for adding `unthrottled_interval` to `BeginFrameArgs` is that itβs a useful value for many consumers.
I see the argument for passing the value via `UpdateVSyncParametersCallback` - however, the challenge becomes piping it to the places that need it.
For example, we want to use this new value in `CompositorFrameSinkSupport::OnBeginFrame`, but the only value passed in there is `BeginFrameArgs`. While there are ways to pipe it in separately, that could add complexity both now and for other use cases.
@jon...@chromium.org Could be good to get your input here - any thoughts? Thanks π
Maggie ChenTwo part goal for `unthrottled_interval`
- Viz throttling of `FrameSinks` would want to limit how much throttling is done. Especially as there can be multiple source for throttling. `Desktop Power Saving mode` and a new `non-interactive mode`.
- Clients may wish to perform different deadline vs scheduling decisions. For example a Renderer will throttle `BeginMainFrame` to 60fps on an 120fps display. We'd ideally like to have the Main-thread update for the deadline that matches 60fps interval. However we wouldn't want idle-time and v8 cleanup to be scheduled during frame production at 120fps.
That sounds reasonable to me.
Just want to remind a potential issue that the supported frame intervals from last GetSupportedFrameIntervals() might be stale if throttling happens at the first min_refresh_interval change in `OnBeginFrame()`, line #311, since this `update_vsync_params_callback_.Run()` is called after `OnBeginFrame()`. I assume we can only throttle it to a supported frame rate.
Related details
- Etienne Pierre-Doray
- Jonathan Ross
- Sky Debreuil
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Sky Debreuil (Gerrit)
Sky Debreuil voted and added 3 comments![]( "Open in Gerrit")
Votes added by Sky Debreuil
| Auto-Submit | +1 |
3 comments
this->interval * kUnthrottledIntervalJitterMultiplier);Sky Debreuil@jon...@chromium.org Perhaps it would be good for
Sorry was there anything needed for this one?
min_refresh_interval_));Maggie Chen@mag...@chromium.org as you mentioned in a separate comment this value can become stale - but likely good to fix that in a separate CL to keep this one focused π
Let me merge `nominal_refresh_period_` into `min_refresh_interval_`.
Sounds good! I'm using `min_refresh_interval_` interval here already - so no changes needed here?
Thanks π
mojo_base.mojom.TimeDelta unthrottled_interval;Sky DebreuilCan this be optional, rather than using `-1` as a null value?
For sure. I've updated the Mojom definition to use `mojo_base.mojom.TimeDelta?` and adjusted the traits to use std::optional.
I've also added additional unit tests to ensure everything is working. Please let me know if there are any issues. Thanks π
Related details
- Etienne Pierre-Doray
- Jonathan Ross
- Ken Buchanan
- Maggie Chen
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Maggie Chen (Gerrit)
Maggie Chen added 1 comment![]( "Open in Gerrit")
min_refresh_interval_));Maggie Chen@mag...@chromium.org as you mentioned in a separate comment this value can become stale - but likely good to fix that in a separate CL to keep this one focused π
Sky DebreuilLet me merge `nominal_refresh_period_` into `min_refresh_interval_`.
Sounds good! I'm using `min_refresh_interval_` interval here already - so no changes needed here?
Thanks π
- Etienne Pierre-Doray
- Jonathan Ross
- Ken Buchanan
- Sky Debreuil
Sky Debreuil (Gerrit)
Sky Debreuil voted and added 1 comment![]( "Open in Gerrit")
Votes added by Sky Debreuil
| Auto-Submit | +1 |
| Commit-Queue | +1 |
1 comment
That makes sense. I've updated the relevant bug for that with your comment (https://crbug.com/477242770)
Ken Buchanan (Gerrit)
Ken Buchanan voted and added 1 comment![]( "Open in Gerrit")
Votes added by Ken Buchanan
| Code-Review | +1 |
1 comment
Related details
- Etienne Pierre-Doray
- Jonathan Ross
- Sky Debreuil
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Sky Debreuil (Gerrit)
Sky Debreuil voted and added 1 comment![]( "Open in Gerrit")
Votes added by Sky Debreuil
| Auto-Submit | +1 |
1 comment
min_refresh_interval_));Maggie Chen@mag...@chromium.org as you mentioned in a separate comment this value can become stale - but likely good to fix that in a separate CL to keep this one focused π
Sky DebreuilLet me merge `nominal_refresh_period_` into `min_refresh_interval_`.
Maggie ChenSounds good! I'm using `min_refresh_interval_` interval here already - so no changes needed here?
Thanks π
You will have to wait for my CL being landed. That's it.
I spoke with Jonathan and possibly it makes sense to land mine first. I'm actually going on leave very soon (my third kid is due tomorrow) so perhaps that would work better.
This CL here does not have any functional changes - and the specific DCHECK for that `unthrotted_interval` being correct is disabled on Mac at the moment, so I don't think the merge will be an issue.
Please let me know if there is anything. Thanks! π
Related details
- Etienne Pierre-Doray
- Jonathan Ross
- Maggie Chen
Jonathan Ross (Gerrit)
Jonathan Ross voted![]( "Open in Gerrit")
- Etienne Pierre-Doray
- Maggie Chen
- Sky Debreuil
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Sky Debreuil (Gerrit)
Sky Debreuil added 3 comments![]( "Open in Gerrit")
this->interval * kUnthrottledIntervalJitterMultiplier);Sky Debreuil@jon...@chromium.org Perhaps it would be good for
Sorry was there anything needed for this one?
Done
min_refresh_interval_));Maggie Chen@mag...@chromium.org as you mentioned in a separate comment this value can become stale - but likely good to fix that in a separate CL to keep this one focused π
Sky DebreuilLet me merge `nominal_refresh_period_` into `min_refresh_interval_`.
Maggie ChenSounds good! I'm using `min_refresh_interval_` interval here already - so no changes needed here?
Thanks π
Sky DebreuilYou will have to wait for my CL being landed. That's it.
I spoke with Jonathan and possibly it makes sense to land mine first. I'm actually going on leave very soon (my third kid is due tomorrow) so perhaps that would work better.
This CL here does not have any functional changes - and the specific DCHECK for that `unthrotted_interval` being correct is disabled on Mac at the moment, so I don't think the merge will be an issue.
Please let me know if there is anything. Thanks! π
Done
update_vsync_params_callback_.Run(frame_time, nominal_refresh_period_);- Etienne Pierre-Doray
- Jonathan Ross
- Maggie Chen
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Sky Debreuil (Gerrit)
Sky Debreuil voted Commit-Queue+2![]( "Open in Gerrit")
| Commit-Queue | +2 |
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change![]( "Open in Gerrit")
Change information
Add unthrottled_interval to BeginFrameArgs
Currently, `BeginFrameArgs::interval` represents the target frame
interval. When throttling is active (e.g. for variable refresh rate),
this value increases, hiding the underlying hardware refresh rate.
This change adds `unthrottled_interval` to `BeginFrameArgs`. This field
stores the inverse of the maximum possible hardware refresh rate. This
allows clients to distinguish between the effective frame rate and the
native display capability. If no specific unthrottled value is provided,
it defaults to `interval`.
This CL only adds this new value and has no functional changes.
- M base/tracing/protos/chrome_track_event.proto
- M components/viz/common/frame_sinks/begin_frame_args.cc
- M components/viz/common/frame_sinks/begin_frame_args.h
- M components/viz/common/frame_sinks/begin_frame_args_unittest.cc
- M components/viz/common/frame_sinks/begin_frame_source.cc
- M components/viz/common/frame_sinks/begin_frame_source.h
- M components/viz/common/frame_sinks/begin_frame_source_unittest.cc
- M components/viz/common/frame_sinks/external_begin_frame_source_ios.mm
- M components/viz/service/frame_sinks/external_begin_frame_source_mac.cc
- M components/viz/service/frame_sinks/external_begin_frame_source_win.cc
- M components/viz/test/begin_frame_args_test.cc
- M components/viz/test/begin_frame_args_test.h
- M components/viz/test/begin_frame_source_test.h
- M components/viz/test/fake_external_begin_frame_source.cc
- M components/viz/test/fake_external_begin_frame_source.h
- M services/viz/public/cpp/compositing/begin_frame_args_mojom_traits.cc
- M services/viz/public/cpp/compositing/begin_frame_args_mojom_traits.h
- M services/viz/public/cpp/compositing/mojom_traits_unittest.cc
- M services/viz/public/mojom/compositing/begin_frame_args.mojom
Code-Review: +1 by Jonathan Ross, +1 by Ken Buchanan
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |