Add a Dart FFI method to synchronously render a frame #50492
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 "@test-exemption-reviewer" in the #hackers channel in Chat (don't just cc them here, they won't see it! Use Discord!). 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. |
lib/ui/platform_dispatcher.dart
Outdated
| /// | ||
| /// This method should not be used in usual cases. It is designed for | ||
| /// situations that require a frame is rendered as soon as possible, even at | ||
| /// the cost of rendering quality. |
There was a problem hiding this comment.
Can you give the warmup frame as an explicit example here? Some of the verbiage from https://main-api.flutter.dev/flutter/scheduler/SchedulerBinding/scheduleWarmUpFrame.html would fit here as well to motivate that use case.
There was a problem hiding this comment.
Done, can you take a look?
lib/ui/platform_dispatcher.dart
Outdated
| // the restriction of the performance test. Eventually, we should either land | ||
| // the full changes, or remove this method. | ||
| // https://github.com/flutter/flutter/issues/142851 | ||
| void imposeSyncFrame() => _imposeSyncFrame(); |
There was a problem hiding this comment.
In terms of naming, I favor forceSyncFrame. I do like the Sync in the name, though.
There was a problem hiding this comment.
Why not just call it warmUpFrame? or something like that.
There was a problem hiding this comment.
Why not just call it warmUpFrame? or something like that.
I think "warm up frame" is a more application-level concept and belongs to the scheduler binding, and for PlatformDispatcher it's better to name it with what the developer makes the engine do.
There was a problem hiding this comment.
but its only used for a warm up frame, and probably isn't usefulfor anything besides a warm up frame.
There was a problem hiding this comment.
We're expecting some developers calling the dart:ui package directly instead of going through the framework, who will have their own scheduling scheme and terms. I think it's better to leave room for them on how they use this method. (I've added scheduleWarmUpFrame to the doc as an example, do you think it helps easing your concern?)
There was a problem hiding this comment.
But if you don't know what the usages are then its unlikely that you'll be able to design it in such a way as to accomidate them.
There was a problem hiding this comment.
I've added "used in scheduleWarmUpFrame" to document. Does it feel better?
(Although it's not in the framework yet, this is the PR to add it.)
| // the performance test. Eventually, we should either land the full changes, | ||
| // or remove this method. | ||
| // https://github.com/flutter/flutter/issues/142851 | ||
| FML_UNREACHABLE(); |
There was a problem hiding this comment.
This is in fact quite reachable. You could error log if you want to discourage calling this method but leaving a process exit accessible from dart:ui does not seem reasonable.
There was a problem hiding this comment.
This PR has been merged to #49950 because I realized that this PR does not improve the situation alone. Now this method is implemented.
|
Closing this PR because I realized that this PR alone does not improve our situation. Framework needs the transitional warm up rendering ( |
This PR adds
PlatformDispatcher.imposeSyncFrame.This function is not used by anyone, nor is implemented by the engine. The usage and implementation should be added by a later PR. The full PR must be verified by a performance test, but the performance test currently has a problem of not being able to recognize local dart:ui changes.
Pre-launch Checklist
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.