[viz] Allow viz to ack upon activation when interactive [chromium/src : main]
Ian Vollick (Gerrit)
Ian Vollick added 4 comments![]( "Open in Gerrit")
"expiry_milestone": 135Ian VollickWe should update `draw-immediately-when-interactive` to `135` in a follow up. Currently it targets m129
Done and added you as an owner.
if (ShouldAlwaysAckOnSurfaceActivation() && !BeginFrameAcksEnabled()) {Ian VollickFor `ShouldAlwaysAckOnSurfaceActivation && BeginFrameAcksEnabled` are we enqueuing the Ack, and the resources. But not forwarding them? Or are we sending the Ack without the resources?
Jonathan RossI'm not sure if I'm looking at the right thing, but what appears to happen is the following
- First, we call SubmitCompositorFrame on line 502 above which does indeed cause DidRecieveCompositorFrameAck, but because ShouldMergeBeginFrameWithAcks is true, we don't attempt to return resources (which wouldn't have mattered anyhow, because surface_returned_resources_ is empty at this point -- i.e., we haven't returned the resources yet). The DidReceiveCompositorFrameAck call causes us to increment ack_queued_for_client_count_ which then causes us in UpdateNeedsBeginFrameInternal to set needs_begin_frame_ to true [here](https://source.chromium.org/chromium/chromium/src/+/main:components/viz/service/frame_sinks/compositor_frame_sink_support.cc;l=1279;drc=8d9115add84ccaae3505b2fcb86cc93a53a378ce)
- Next, we call UnrefResources, which indirectly results in CompositorFrameSinkSupport::ReturnResources. However, since we need a begin frame, we don't release the resource [here](https://source.chromium.org/chromium/chromium/src/+/main:components/viz/service/frame_sinks/compositor_frame_sink_support.cc;l=485;drc=8d9115add84ccaae3505b2fcb86cc93a53a378ce), but instead store it away in `surface_returned_resources_`.
- Then, on the next SubmitCompositorFrameWithResources call, we ActivateFrame (which causes us to return resources, but we return nothing).
- It's the MaybeTestOnBeginFrame that ultimately triggers CompositorFrameSinkSupport::OnBeginFrame that plumbs the resources back to FakeCompositorFrameSinkClient::OnBeginFrame
Ian VollickPer offline discussion. The above flow is what we were expecting. The `CheckReturnedResourcesMatchExpected` will catch any errors with caching `surface_returned_resources_`.
Can we add a note here regarding that `UnrefResources` will enqueue the resources in `surface_returned_resources_` for `BeginFrameAcksEnabled`. Regardless of `ShouldAlwaysAckOnSurfaceActivation`
I tried. Plmk reopen if my comment is wrong.
// There should also be an un-acked frame, which was just submitted.
if (ShouldAlwaysAckOnSurfaceActivation()) {Ian VollickAre we hitting `Surface::ActivateFrame` in the test due to a lack of surface dependencies? Aka we can activate it immediately upon submission?
Jonathan RossYeah, looks like we hit the `if (activation_dependencies_.empty()) {` case in `Surface::CommitFrame`.
Ian VollickMind adding a note about that?
Done
"chromeos_lacros",Ian VollickWe can remove this. 😞
Done
Related details
- Jonathan Ross
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ian Vollick (Gerrit)
Ian Vollick added 2 comments![]( "Open in Gerrit")
Sorry for the churn. Noticed something else to sort out before putting out for real.
(state_machine_.pending_submit_frames() >= 0));Moving back to WIP while I look at this.
Related details
- Jonathan Ross
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Jonathan Ross (Gerrit)
Jonathan Ross added 5 comments![]( "Open in Gerrit")
// If this number of frames (or greater) have elapsed since an interaction, we
// will stop our early ack on activation behavior. The number chosen here is an
// arbitrary small number that appears to work.
constexpr uint64_t kAckOnSurfaceActivationCooldown = 3;Can we FeatureParam this? See example kNumDidNotProduceFrameBeforeThrottle
bool SurfaceManager::ShouldAckInteractiveFrame(const BeginFrameAck& ack) const {
if (!features::ShouldAckOnSurfaceActivationWhenInteractive() ||This can be in a loop `CompositorFrameSinkSupport::CheckPendingSurfaces` & `Surface::RecomputeActiveReferencedSurfaces`.
That's one of the cases where it can be worth caching this flag in a member variable.
if (ack.frame_id < last_interactive_frame_.value()) {Could a client be running slower, and the Ack could help?
Eg: Scrolling Renderer at 60 fps. Other Renderer running at 30 fps, and taking 2 VSync intervals to submit.
{
"name": "EnabledV3",
"enable_features": [
"DrawImmediatelyWhenInteractive"
]
},
{
"name": "EnabledV2",
"enable_features": [
"DrawImmediatelyWhenInteractive"
]
},
{
"name": "Enabled",
"enable_features": [
"DrawImmediatelyWhenInteractive"
]
}Since we turned the early experiments off, and only the topmost variant is ran anyways. We can remove the pre-V4
Related details
- Ian Vollick
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ian Vollick (Gerrit)
Ian Vollick added 5 comments![]( "Open in Gerrit")
Moving back to WIP while I look at this.
Done
// If this number of frames (or greater) have elapsed since an interaction, we
// will stop our early ack on activation behavior. The number chosen here is an
// arbitrary small number that appears to work.
constexpr uint64_t kAckOnSurfaceActivationCooldown = 3;Can we FeatureParam this? See example kNumDidNotProduceFrameBeforeThrottle
Done. I've also added a test to surface_synchronization_unittest.cc that checks that we early-ack when frames either precede the interactive frame or are within the number of cooldown frames.
bool SurfaceManager::ShouldAckInteractiveFrame(const BeginFrameAck& ack) const {
if (!features::ShouldAckOnSurfaceActivationWhenInteractive() ||This can be in a loop `CompositorFrameSinkSupport::CheckPendingSurfaces` & `Surface::RecomputeActiveReferencedSurfaces`.
That's one of the cases where it can be worth caching this flag in a member variable.
Done, though I tried caching this on creation vs lazily-caching the feature value when we use it (to avoid storing things away in functions that seem semantically const). However, some of the tests create the surface manager before the feature-applying-subclass has a chance to update the feature flag. To fix this, I have the classes construct things in SetUp rather than in the test ctor. This results in a fair bit of churn since we now have pointers. Please check this out and let me know if there are issues with setup or teardown.
if (ack.frame_id < last_interactive_frame_.value()) {Could a client be running slower, and the Ack could help?
Eg: Scrolling Renderer at 60 fps. Other Renderer running at 30 fps, and taking 2 VSync intervals to submit.
Ah, good point. It makes sense to me that this would be true, then. I've made that change and added a comment. I've also added a test (see above). Wdyt?
"name": "EnabledV3",
"enable_features": [
"DrawImmediatelyWhenInteractive"
]
},
{
"name": "EnabledV2",
"enable_features": [
"DrawImmediatelyWhenInteractive"
]
},
{
"name": "Enabled",
"enable_features": [
"DrawImmediatelyWhenInteractive"
]
}Since we turned the early experiments off, and only the topmost variant is ran anyways. We can remove the pre-V4
- Jonathan Ross
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Jonathan Ross (Gerrit)
Jonathan Ross added 3 comments![]( "Open in Gerrit")
// Start by creating a frame with an interaction at frame 10 (interactive is
// the default).I thought we were adding `MakeDefaultInteractiveCompositorFrame` because the default for `CompositorFrameMetadata::is_handling_interaction` was false?
bool SurfaceManager::ShouldAckInteractiveFrame(const BeginFrameAck& ack) const {
if (!features::ShouldAckOnSurfaceActivationWhenInteractive() ||Ian VollickThis can be in a loop `CompositorFrameSinkSupport::CheckPendingSurfaces` & `Surface::RecomputeActiveReferencedSurfaces`.
That's one of the cases where it can be worth caching this flag in a member variable.
Done, though I tried caching this on creation vs lazily-caching the feature value when we use it (to avoid storing things away in functions that seem semantically const). However, some of the tests create the surface manager before the feature-applying-subclass has a chance to update the feature flag. To fix this, I have the classes construct things in SetUp rather than in the test ctor. This results in a fair bit of churn since we now have pointers. Please check this out and let me know if there are issues with setup or teardown.
Yeah the test changes work. Init in SetUp is more preferred now due to feature flags.
if (ack.frame_id < last_interactive_frame_.value()) {Ian VollickCould a client be running slower, and the Ack could help?
Eg: Scrolling Renderer at 60 fps. Other Renderer running at 30 fps, and taking 2 VSync intervals to submit.
Ah, good point. It makes sense to me that this would be true, then. I've made that change and added a comment. I've also added a test (see above). Wdyt?
- Ian Vollick
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ian Vollick (Gerrit)
Ian Vollick added 1 comment![]( "Open in Gerrit")
// Start by creating a frame with an interaction at frame 10 (interactive is
// the default).I thought we were adding `MakeDefaultInteractiveCompositorFrame` because the default for `CompositorFrameMetadata::is_handling_interaction` was false?
I've updated the comment -- the MakeCompositorFrame function that's being used here is defined at the top of the file and I've modified it to make is_handling_interaction true.
Related details
- Jonathan Ross
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Jonathan Ross (Gerrit)
Jonathan Ross voted Code-Review+1![]( "Open in Gerrit")
- Ian Vollick
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Jonathan Ross (Gerrit)
Jonathan Ross added 1 comment![]( "Open in Gerrit")
// Start by creating a frame with an interaction at frame 10 (interactive is
// the default).Ian VollickI thought we were adding `MakeDefaultInteractiveCompositorFrame` because the default for `CompositorFrameMetadata::is_handling_interaction` was false?
I've updated the comment -- the MakeCompositorFrame function that's being used here is defined at the top of the file and I've modified it to make is_handling_interaction true.
😮
that scares me, but is outside the scope of this review
Related details
- Ian Vollick
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ian Vollick (Gerrit)
Ian Vollick voted Commit-Queue+2![]( "Open in Gerrit")
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change![]( "Open in Gerrit")
Change information
[viz] Allow viz to ack upon activation when interactive
With this change, we introduce a mode in which we send acks to clients
when viz surfaces activate and we're "interactive" (that is, we're
within some number of frames of a frame where we had damage due to
scrolling or a touch interaction). We expect that this will cause some
extra / wasted work, but should help regularize the timing of the
client work since it effectively negates backpressure. That said, it
is still possible for timing to be irregular (i.e., it's not clear
how much this helps and this CL does not attempt to quantify this nor
check via integration test).
NOTE:
Earlier patch sets have the tests enabled by default to see which
tests would break and have attempted to address and parameterize
affected tests (see patchset 4).
credit: suggested by jonross@. Jon also noted the cause of the issue
with a previous attempt (crrev.com/c/5675451) -- that if a dependency
submitted early, it wouldn't early ack.
- M chrome/browser/about_flags.cc
- M chrome/browser/flag-metadata.json
- M chrome/browser/flag_descriptions.cc
- M chrome/browser/flag_descriptions.h
- M components/viz/common/features.cc
- M components/viz/common/features.h
- M components/viz/service/frame_sinks/compositor_frame_sink_support.h
- M components/viz/service/frame_sinks/compositor_frame_sink_support_unittest.cc
- M components/viz/service/frame_sinks/frame_sink_manager_impl.h
- M components/viz/service/frame_sinks/surface_synchronization_unittest.cc
- M components/viz/service/surfaces/surface_manager.cc
- M components/viz/service/surfaces/surface_manager.h
- M components/viz/test/compositor_frame_helpers.cc
- M components/viz/test/compositor_frame_helpers.h
- M testing/variations/fieldtrial_testing_config.json
- M tools/metrics/histograms/enums.xml
Code-Review: +1 by Jonathan Ross
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |