[vsync_waiter] add AwaitVSyncForSecondaryCallback() #25787
Conversation
|
It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat. If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix? Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. |
|
@farchond Based on our last conversation, I expected that were going to back out the secondary callback mechanism in lieu of task source grades that @iskakaushik recently worked on adding. |
|
Hey Chinmay! I did look into whether or not TaskSources would be a way to replace the SecondaryCallback mechanism, and came to the conclusion that this change is still necessary, since this PR solves a different problem. I talked with Nathan to understand how SecondaryCallbacks were being used, as implemented in his PR, #24526. I quickly learned that my initial approach to simply have ScheduleSecondaryVsyncCallback() not call AwaitVsync() would be wrong: this approach would break our perf benchmarks due to the incorrect handling of input trace events. I also realized that since we cannot eliminate SecondaryCallbacks, we need to capture the intent posed by them, and pass it to the derived class. Even if we replace the mechanism to something using TaskSources, the intent still needs to be passed down. The intent is essentially "wake me up at the next vsync, but not for frame scheduling." This differs from the intent of the primary callbacks, which may lead VsyncWaiter implementations to handle them differently. I think having the base class call a function leads to simpler code than creating a new TaskSource that just asks for wakeups. I could be misunderstanding the true use for TaskSources, but I don't see them as necessary for this use case. One example of the simplicity of the two functions solution is that many implementations of VsyncWaiter do not wish to differentiate between the two intents, which this PR handles transparently without the need for any code changes or additional complexity unless the derived class wants it. It's possible I'm just not understanding TaskSources well enough to see how they could be best used here, but if that's the case I definitely want to chat and learn more! |
|
Just spoke with Felipe and the use case for Fuchsia does make sense. Though perhaps we should also have safeguards in place for cases where touch down events that don't kick off frames don't await vsync or render additional frames. |
farchond
force-pushed
the
add_virtual_method
branch
from
b7be27f to
33f3b1e
Compare
chinmaygarde
added
the
waiting for tree to go green
|
This pull request is not suitable for automatic merging in its current state.
|
fluttergithubbot
removed
the
waiting for tree to go green
farchond
force-pushed
the
add_virtual_method
branch
from
33f3b1e to
a8e9959
Compare
* ccaae8d Roll Skia from 537293bf155f to adadb95a9f1e (1 revision) (flutter/engine#25905) * 2e3a7f8 Roll buildroot to pick change to cppwinrt invocation (flutter/engine#25993) * 1c22286 [Linux] revise dark theme detection (flutter/engine#25535) * 7424400 Moved PlatformMessage's to unique_ptrs (flutter/engine#25860) * 406c4da Ensure that AutoIsolateShutdown drops its reference to the DartIsolate on the intended task runner (flutter/engine#25899) * 72c2fda [web] Fix incorrect physical size due to visualviewport api on iOS (flutter/engine#25895) * a712ffe Roll Dart SDK from b8f4018535fa to 86c749398b3a (16 revisions) (flutter/engine#25999) * edbbb12 Roll Fuchsia Mac SDK from uQgs5ZmFq... to aCsEHpnS0... (flutter/engine#26001) * afbbeac Implement smooth resizing for Linux (flutter/engine#25884) * 3ffb8ef Roll Dart SDK from 86c749398b3a to b4210cc43086 (2 revisions) (flutter/engine#26006) * 039dcd9 pull googletest from github instead of fuchsia.googlesource (flutter/engine#25907) * 9c793f1 Roll Skia from adadb95a9f1e to 1dc2d0fe0fa0 (98 revisions) (flutter/engine#26009) * 82efb9a Roll Skia from 1dc2d0fe0fa0 to 115645ee9b1b (2 revisions) (flutter/engine#26012) * 91a4c72 Delete unused method from engine_layer.h (flutter/engine#25924) * 619f82f fuchsia: Fix multi-views fallout (flutter/engine#25984) * 0b4bf7e Fixes BUILD.gn if is_fuchsia (legacy embedder) and is_debug (flutter/engine#25858) * 2e9de09 Streamline frame timings recording (flutter/engine#25892) * 1077da8 [vsync_waiter] add AwaitVSyncForSecondaryCallback() (flutter/engine#25787) * 0053bef [build_fuchsia_artifacts] Move license copying into BuildBucket(). (flutter/engine#25815) * e8b80e7 Roll Skia from 115645ee9b1b to c411429239e9 (7 revisions) (flutter/engine#26018) * d3353b2 Move more parts to libraries. (flutter/engine#25863) * d1a1182 Roll Dart SDK from b4210cc43086 to 04e55dad908d (2 revisions) (flutter/engine#26020) * f631f5b Exclude third_party/dart/third_party/devtools from the license script (flutter/engine#25918) * 304539b Roll Fuchsia Mac SDK from aCsEHpnS0... to OyXxehV6e... (flutter/engine#26022) * c3e28ae Roll Fuchsia Linux SDK from 4numS0K6T... to -FIIsjZj2... (flutter/engine#26023) * 6cacb53 Roll Skia from c411429239e9 to 72de83df3a03 (1 revision) (flutter/engine#26024) * bfccba5 Add dependency on Windows SDK CIPD package (flutter/engine#26003) * 06cbf1e Roll Dart SDK from 04e55dad908d to 094c9024373c (1 revision) (flutter/engine#26027) * 73aaeea Roll Skia from 72de83df3a03 to dabb2891c4a1 (4 revisions) (flutter/engine#26025) * e229649 Extract Windows string_conversion target (flutter/engine#26029) * 3d73e06 Extract FML command_line target (flutter/engine#26028) * 4aace54 Make SceneBuilder.push* not return nullable objects (flutter/engine#25991) * 0d62a56 Roll Dart SDK from 094c9024373c to 2ea89ef8f6de (1 revision) (flutter/engine#26030) * 5a26c1f Use string_view inputs for conversion functions (flutter/engine#26031) * 4f28de4 Support windows registry access (flutter/engine#26032) * 234fae1 Roll Fuchsia Linux SDK from -FIIsjZj2... to KZCe5FqMb... (flutter/engine#26034) * 7a47d3b Roll Fuchsia Mac SDK from OyXxehV6e... to DSk0IzBHv... (flutter/engine#26035) * 01f9bd8 Roll Skia from dabb2891c4a1 to 686dd910dd6c (1 revision) (flutter/engine#26036) * 1825bef Revert Dart SDK to b8f4018535fa792891e2add3a475f35e3ec156ab (flutter/engine#26037)
This PR is a pre-requisite for #25551.
One issue I came across while implementing a better VsyncWaiter for Fuchsia is that the derived VsyncWaiter class was receiving two flavors of AwaitVsync() - one was for frame scheduling, and another was for trace bookkeeping. In order to implement a testable, robust VsyncWaiter with regards to proper frame scheduling, it became necessary to differentiate between the two intents behind AwaitVsync(), which is what the PR allows for by creating a separate function.
For other implementations of VsyncWaiter that do not wish to worry about this difference in intention, no change is necessary, as the default implementation of AwaitVsyncForSecondaryCallback() just calls AwaitVsync().