-
Notifications
You must be signed in to change notification settings - Fork 5.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Allow render to be called multiple times for one BeginFrame #36438
Conversation
Not sure whether I should "@" some people here, maybe @dnfield @jonahwilliams @gaaclarke @flar engine experts? May I get a code review, thanks :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The documentation on FlutterView.render
specifies when it is safe/allowed to call render.
I'm not quite clear on how this will affect the pipeline - it seems like it will now be trivial for a dart:ui application to flood the pipeline if we remove guardrails around when you can call render
. Today the contract is that the application can expect that it's time to call render because it got a call to onBeginFrame
. In this world, the application calls render
whenever it thinks it has been working too long and might want to give an update. But the application doesn't know about vsync and it will be very hard to reason about why render is getting called if we make this change.
I don't think we should make this change. It too easily allows wasted work to happen.
@dnfield Hi, thanks for the quick reply :)
I will change that doc accordingly (probably after we come to a conclusion what should be done for this PR)
Firstly, IMHO, a normal flutter app calls Secondly, in my proposal ( That said, I do agree that, if the rasterizer thread takes too long (e.g. takes 50ms for one rasterize), it is a waste to submit a Scene per 16.6ms (but should submit per 50ms). If this is still a problem for you, can I change as follows: Add a flag to In addition, given it is a so low-level API that most people will never touch, it seems reasonable to provide some flexibility to it. |
@dnfield Another possibility for Dart code to understand the queue is full so it do not do anything more: Add this 4-line function: // return: whether it is prepared successfully. If return false, it means pipeline is full,
// and thus the user should not really compute the Scene to avoid unnecessary work.
bool Animator::PrepareExtraRender() {
if (!producer_continuation_) {
producer_continuation_ = layer_tree_pipeline_->Produce();
}
return static_cast<bool>(producer_continuation_);
} usage: realize_next_vsync_comes; // see the design for details https://docs.google.com/document/d/1FuNcBvAPghUyjeqQCOYxSt6lGDAQ1YxsNlOvrUx0Gko/edit
var prepared = window.prepareExtraRender();
if (prepared) {
ui.Scene scene = compute_the_scene();
window.render(scene);
} else {
// do not do anything since the rasterizer queue is already so full
// this mimic the behavior of Animator::BeginFrame, where we skip the current frame if it is full
} |
IMHO the pipeline seems not to be flooded - it has depth 2. In other words, even if we call For a dart:ui application, if needed, it can use the
Just as mentioned above, adding a flag like |
Oops sorry @chinmaygarde and @iskakaushik I just clicked the "Icons.refresh" on dnfield and do not know why github remove review requests to you... |
If we are worried that users may submit multiple window.render inside one vsync, another possibility: We may add some code in the C++ layer (or Dart layer), such that we check current vsync status, and only |
Fizzling like that would be expensive, and we'd be giving developers a button to push that actually makes things slower. We should avoid that. |
@dnfield If speed is a concern, maybe the original PR is ok: For a normal usage, it only adds |
There seems to be another way that is not very slower: Firstly, the ...
if (!onlyRenderOncePerBeginFrame && !producer_continuation_) {
producer_continuation_ = layer_tree_pipeline_->Produce();
}
...
void SetOnlyRenderOncePerBeginFrame(bool value) { this->onlyRenderOncePerBeginFrame = value; }
class Animator { ... bool onlyRenderOncePerBeginFrame; ... } (no need for locks, since all on UI thread.) Then there comes the concern that Secondly, seems we can use the |
@dnfield And for zero speed decrease if you like: The |
You're discounting the time it takes to actually make a native call from Dart. On top of that, we should not expose an API that might or might not do something and developers have no good way to reason about whether they're really supposed to call it or not. This proposal is fundamentally changing the invariants around Why, for example, shouldn't the framework just call render and schedule a new frame when its hit its potential limit? |
@dnfield Hi thanks for the reply.
Originally I thought that is small just like a normal function call... Ok now I learn it.
Indeed they have a way to reason: look at time or vsync info. They should not submit twice inside one vsync interval.
I am not sure, if I expose the vsync info and ensure only one call is made per vsync interval (16.67ms), does this satisfy your requirement?
Again, as mentioned above, dev should not call it multiple times per frame. A naive dev may use
Then maybe we should return
Because of the fundamental design of the preempt proposal (https://docs.google.com/document/d/1FuNcBvAPghUyjeqQCOYxSt6lGDAQ1YxsNlOvrUx0Gko/edit), mainly "The flow chart" section. Indeed, window.render is called per vsync interval (16.67ms). The main difference from classical code is that, it may be called multiple times per onBeginFrame (when onBeginFrame is super slow). |
Devices do not always have 60fps vsync - sometimes it's 90 or 120 or more or less (at one point we had a customer looking at 240hz devices, and it's likely there are customers out there looking at 30hz use cases). There is no way currently in dart:ui to know what the current refresh rate is, and on some platforms it's not even possible to implement because the vendors don't provide an API for it (e.g. some Android vendors), and it can change from frame to frame. In other words, a developer must not assume that 16.67ms is the right interval for a frame in all circumstances. And the query of |
Definitely! That's why I also propose to expose vsync-related information to the dev. Last week you asked me to describe it and it was at "Get last vsync time information" section of google doc.
Totally agree. Indeed in my (previous) implementation, I let the C++ side expose the |
P.S. I am also considering relaxing when to start a onBeginFrame which seems to reduce unnecessary idle time and improve performance. That may be related to the big picture you are concerned - how vsync and code are interacted. I will add it to design doc and reply here maybe in an hour. |
@dnfield Here it goes: "Relax onBeginFrame starting criterion" section in https://docs.google.com/document/d/1FuNcBvAPghUyjeqQCOYxSt6lGDAQ1YxsNlOvrUx0Gko/edit |
I think it would be easier to start with a patch that exposes more about vsync timings to the developer, because that will be critical to whether this one makes sense. |
Thanks, I will do that. |
Let's close this one out to get it off review queues until we have more consensus around the approach. |
All right... |
This PR is a part for implementing the 60fps smooth rendering (#101227).
List which issues are fixed by this PR. You must list at least one issue.
flutter/flutter#101227
If you had to change anything in the flutter/tests repo, include a link to the migration guide as per the breaking change policy.
The only change is an "if" as follows (all else are just tests)
Pre-launch Checklist
writing and running engine tests.
///
).If you need help, consider asking for advice on the #hackers-new channel on Discord.
By the way, the tests and code does work: If I comment out the code, the tests fail.