Skip to content

Some fixes #2

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

Open
wants to merge 2 commits into
base: three_phase_render
Choose a base branch
from

Conversation

knopp
Copy link

@knopp knopp commented Jul 13, 2024

Some fixes I did to get wonderous working.

unused_layers = unused_layers]() mutable {
auto task = [this, platform_view_layers = std::move(platform_view_layers),
callbacks = std::move(callbacks),
unused_layers = std::move(unused_layers)]() mutable {
TRACE_EVENT0("flutter", "FlutterPlatformViewsController::SubmitFrame::CATransaction");
Copy link
Author

@knopp knopp Jul 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We're capturing this anyway and calling methods on it (CompositeWithParams, RemoveUnusedLayers) which modifies variables such as views_to_recomposite_ - holding on to previous value in capture causes crashes when removing views.

We may want to synchronize this (i.e. block raster thread until the block completes) - in case there is something holding the platform thread for longer than expected we can have race condition.

@@ -783,34 +784,26 @@ static bool ClipRRectContainsPlatformViewBoundingRect(const SkRRect& clip_rrect,
DisposeViews();

// Composite Platform Views.
for (auto view_id : views_to_recomposite) {
CompositeWithParams(view_id, current_composition_params[view_id]);
for (auto view_id : views_to_recomposite_) {
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

DisposeViews modifies views_to_recompose_ so we can't use captured value.


platform_task_runner_->PostTask(task);
latch.Wait();
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems necessary otherwise things quickly crash in release mode. I assume there is some state read on platform thread that is being touched in next preroll. On macOS we can get away with not blocking the raster thread because we have stricter separation of data. Possible could do it here too if the wait causes performance issues.

@knopp
Copy link
Author

knopp commented Jul 13, 2024

I tried this PR with Wonderous + FLTEnableMergedPlatformUIThread. With CAMetalLayer the performance is really bad, regardless of whether there is a platform view present. Seems pathological almost (~30FPS), maybe it would be worth trying to figure out what is going on. With FlutterMetalLayer I seem to be getting consistent 120 fps with and without platform views.

jonahwilliams pushed a commit that referenced this pull request Sep 10, 2024
Relands flutter#54917. The change is the same as before, except now the native resources for `SkwasmColorFilter` and `SkwasmImageFilter` classes are no longer GC'd. Instead, we use manually managed native handles (vended and scoped by `withRawColorFilter` and `withRawImageFilter`). The bug in the previous PR was that filter objects were disposed with the paint while the framework continued holding onto them. When GC kicked the finalization registry, it attempted to double-free the filters.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant