[HCPP] Clean up overlay layer when last frame had overlay content and current doesn't #173881

gmackall · 2025-08-15T21:39:13Z

My current understanding of the issue is that we are never pushing a new empty frame for this layer, so the texture gets stuck as shown in the bug. If we keep track of when we had content last frame, and don't in the current frame, we can push a single empty frame for that layer when needed.

See example in #173635 (comment)

Pre-launch Checklist

I read the Contributor Guide and followed the process outlined there for submitting PRs.
I read the Tree Hygiene wiki page, which explains my responsibilities.
I read and followed the Flutter Style Guide, including Features we expect every widget to implement.
I signed the CLA.
I listed at least one issue that this PR fixes in the description above.
I updated/added relevant documentation (doc comments with ///).
I added new tests to check the change I am making, or this PR is test-exempt.
I followed the breaking change policy and added Data Driven Fixes where supported.
All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

Note: The Flutter team is currently trialing the use of Gemini Code Assist for GitHub. Comments from the gemini-code-assist bot should not be taken as authoritative feedback from the Flutter team. If you find its comments useful you can update your code accordingly, but if you are unsure or disagree with the feedback, please feel free to wait for a Flutter team member's review for guidance on which automated comments should be addressed.

gemini-code-assist

Code Review

This pull request aims to fix an issue where overlay content remains visible after it's been removed. The approach of tracking whether the last frame had overlays and submitting an empty frame to clear it is correct. However, the implementation for submitting the empty frame is missing a crucial step: clearing the frame's canvas to transparent. Without this, stale data might be rendered, which could undermine the fix. I've added a suggestion to address this. Otherwise, the changes look good.

engine/src/flutter/shell/platform/android/external_view_embedder/external_view_embedder_2.cc

gmackall · 2025-08-15T22:21:49Z

Driver test with the change, texture and no_texture:

hybrid_composition_pp_platform_view texture

hybrid_composition_pp_platform_view no_texture

Without change:

reidbaker · 2025-08-18T14:56:46Z

@flar I added you as someone with expertise in the engine. If you are not the right person can you add the right person and remove yourself? For example, I don't know if there is a better way to clear the state without submitting a frame.

reidbaker

@gmackall Read back through the driver and app code and see if there are comments you could add that would help an author or review of future prs that handle framework migrations know if some future the change would break the value of this test.

For example the screenshots you added in the pr were super helpful.

reidbaker · 2025-08-18T14:51:57Z

engine/src/flutter/shell/platform/android/external_view_embedder/external_view_embedder_2.cc

+      overlay_frame = layer->surface->AcquireFrame(frame_size_);
+      overlay_frame->Canvas()->Clear(flutter::DlColor::kTransparent());
+      overlay_frame->set_submit_info({.frame_boundary = false});
+      overlay_frame->Submit();


Does this submit need to be here since there is a submit outside the if statment? Are there costs to a double submit frame?

That is a different frame that we submit below, so yes I believe we need to

It feels like we should experience the removal of an overlay (in this case does it really mean "the existence of a prior platform layer"?) more proactively, but I don't understand what happens when the overlays come and go.

Is this mechanism unique in having an internal structure that comes and goes when platforms appear and disappear?

Also, is there somewhere else that is responsible for whether or not the overlay layer is being displayed that we could "turn off"? That might be more efficient than just erasing it.

I agree that I did not expect to need to handle the removal of the overlay at this layer. For example, in the old PV code:

flutter/engine/src/flutter/shell/platform/android/external_view_embedder/external_view_embedder.cc

Line 118 in 2265d94

if (overlay == overlay_layers.end()) {

we don't need to submit an additional frame

I suspect this is because HCPP now (indirectly) uses a surface flinger which states in the documentation:
https://source.android.com/docs/core/graphics/surfaceflinger-windowmanager

When the display is between refreshes, the display sends the VSync signal to SurfaceFlinger. The VSync signal indicates that the display can be refreshed without tearing. When SurfaceFlinger receives the VSync signal, SurfaceFlinger walks through its list of layers looking for new buffers. If SurfaceFlinger finds a new buffer, SurfaceFlinger acquires the buffer; if not, SurfaceFlinger continues to use the previously acquired buffer. SurfaceFlinger must always display something, so it hangs on to one buffer. If no buffers have ever been submitted on a layer, the layer is ignored.

(emphasis mine)

Does that make sense?

It's vague and doesn't explain how buffers go away. Also, there are buffers and there are layers. It sounds like you can remove them if they have the text "so it hangs on to one buffer". Do we have 2 buffers when we have PVs? Can we unregister them and leave only the buffer for the base scene?

I believe we could remove them entirely, but we are already not doing that and instead managing the visibility of the overlay surface earlier (when there are no PVs visible), to avoid recreating and deleting it frequently:

https://github.com/flutter/flutter/blob/master/engine/src/flutter/shell/platform/android/external_view_embedder/external_view_embedder_2.cc#L73-L80

See the description of #162908. This managing of the surfaces visibility was something that wasn't required in the old PV code path https://github.com/flutter/flutter/blob/master/engine/src/flutter/shell/platform/android/external_view_embedder/external_view_embedder.cc#L74
And I suspect we just are properly handling removing it when there are no platform views to overlay, but not when there are platform views BUT there are no overlays. And we can either update it to be empty, or hide it.

But after looking at it more perhaps it makes more sense for us to be hiding it instead of showing it as empty? Instead of submitting a frame, I've also tested something like:

void AndroidExternalViewEmbedder2::MaybeHideOverlaySurface() { if (overlay_layer_is_shown_) { jni_facade_->hideOverlaySurface2(); overlay_layer_is_shown_ = false; } } void AndroidExternalViewEmbedder2::MaybeShowOverlaySurface() { if (!overlay_layer_is_shown_) { jni_facade_->showOverlaySurface2(); overlay_layer_is_shown_ = true; } }

And then we replace the lines here about submitting a frame with:

... if (overlay_frame != nullptr) { overlay_frame->set_submit_info({.frame_boundary = false}); overlay_frame->Submit(); MaybeShowOverlaySurface(); } else { MaybeHideOverlaySurface(); } ...

And this works as well

To try to answer your question more directly, I see that the overlay surface has its own dedicated SurfaceControl, which has docs that read:

Handle to an on-screen Surface managed by the system compositor. The SurfaceControl is a combination of a buffer source, and metadata about how to display the buffers.

Based on this I believe that the overlay and the Platform views both have their own independent buffers. But this is all my reading on the fly, am new to this area of the code.

I've changed to something like the above, mirroring the structure of how we hide the overlay layer when there are no platform views shown.

reidbaker · 2025-08-18T14:52:31Z

engine/src/flutter/shell/platform/android/external_view_embedder/external_view_embedder_2.cc

+      overlay_frame->Canvas()->Clear(flutter::DlColor::kTransparent());
+      overlay_frame->set_submit_info({.frame_boundary = false});
+      overlay_frame->Submit();
+      has_overlay_layers_ = false;


Does this need to happen after the submit? If so can you add a comment explaining why? if not why do work after submit?

I think it's fine for it to be anywhere in the if statement - it won't matter until the next frame, we just need to keep track of the state frame by frame.

Only thing is if the frame submission in the line above can fail in a way that would prevent us getting to this line, it's better to have it in this order. But I don't know really know that flow 🤷

flar

Some suggestions and nits...

flar · 2025-08-18T19:09:20Z

engine/src/flutter/shell/platform/android/external_view_embedder/external_view_embedder_2.cc

+      overlay_frame = layer->surface->AcquireFrame(frame_size_);
+      overlay_frame->Canvas()->Clear(flutter::DlColor::kTransparent());
+      overlay_frame->set_submit_info({.frame_boundary = false});
+      overlay_frame->Submit();


It feels like we should experience the removal of an overlay (in this case does it really mean "the existence of a prior platform layer"?) more proactively, but I don't understand what happens when the overlays come and go.

Is this mechanism unique in having an internal structure that comes and goes when platforms appear and disappear?

Also, is there somewhere else that is responsible for whether or not the overlay layer is being displayed that we could "turn off"? That might be more efficient than just erasing it.

engine/src/flutter/shell/platform/android/external_view_embedder/external_view_embedder_2.h

flar

Functionally looks fine - some suggestions for style/readability if you like...

flar · 2025-08-19T19:42:00Z

engine/src/flutter/shell/platform/android/external_view_embedder/external_view_embedder_2.cc

+    }
    overlay_frame->set_submit_info({.frame_boundary = false});
    overlay_frame->Submit();
+    prev_frame_overlay_layer_shown_ = true;


Looks good. I think it's cleaner if the boolean assignments could be moved inside of the ifs so they are associated with the action that changes the state itself.

Also, the name could be "is_overlay_showing_" (or shown) since "prev_frame" isn't a relevant part of the state - just whether it is showing or not showing.

Another idea - add private methods to do the on/off toggle of the overlay and consolidate the state management into dedicated methods near the state flag.

I added private methods and renamed to overlay_layer_has_content_, and moved the reassignment inside of the if statements.

I also guarded the later call to jni_facade_->showOverlaySurface2(); behind checking that the overlay layer has content to display
https://github.com/flutter/flutter/blob/master/engine/src/flutter/shell/platform/android/external_view_embedder/external_view_embedder_2.cc#L162

as otherwise I believe we could:
have a frame with

No PVs visible

Overlay content to display

followed by a frame with

PVs visible

No Overlay content to display

And we would get the same bug again, because the hiding we added in this pr would be overridden by the later call to show. Unlikely, but possible, so I made us only show if we also have overlay content to display. In that rare case we would make a double call to jni_facade_->showOverlaySurface2(); but that doesn't seem like the biggest deal to me.

Hopefully this all looks right, sorry for the many iterations.

…w on if there is pv content

…ay_layer' into hcpp_clean_up_overlay_layer

flar · 2025-08-20T08:48:18Z

engine/src/flutter/shell/platform/android/external_view_embedder/external_view_embedder_2.cc

        jni_facade->swapTransaction();

-        if (prev_frame_no_platform_views) {
+        if (prev_frame_no_platform_views && overlay_layer_has_content_) {


I don't understand this change, if the has_content flag is true, then shouldn't the showOverlay method have been called in the showIfNeeded call?

Doesn't this result in an extra JNI dispatch to "showOverlaySurface2"?

I don't understand this change, if the has_content flag is true, then shouldn't the showOverlay method have been called in the showIfNeeded call?

Yes, this was always the case. The reason I added it is that if it isn't true, before we would still have called dispatched to showOverlaySurface2 when we have the following two frames:

one with

NO PVs visible

YES Overlay content to display

followed by a frame with

YES PVs visible

NO Overlay content to display

But in this case we still don't want to display the overlay surface (it would result in the same bug that the PR is fixing in the first place). So this addition guards against that

Adding an and to the if statement doesn't result in an extra call, as it only makes the dispatch to showOverlaySurface2 happen less often.

But before this change, and still after, we will get a double dispatch if we get two frames in a row with the following properties:

NO PVs visible

NO overlay content to display

followed by

YES PVs visible

YES Overlay content to display

Let me know if any of this needs clarifying

(See DrawStatus and DrawSurfaceStatus in rasterizer.h)

It looks like the Submit calls simply call callbacks that were supplied when some implementation of Surface constructed a SurfaceFrame inside its AcquireFrame method. In some cases the callbacks just return true. In other cases they make a platform call with non-obvious threading and/or synchronicity implications.

It's not clear how the actions of those submit callbacks relate to either further code in the same thread, or in code posted as a task.

Ok I spent more time looking in to this and am somewhat more confused, but it seems we maybe don't have a threading problem here? It definitely looks to me like the call to hideOverlaySurface isn't being made on the platform thread, but the method that it calls:
https://github.com/flutter/flutter/blob/master/engine/src/flutter/shell/platform/android/io/flutter/embedding/engine/FlutterJNI.java#L1338
Is annotated as being callable only on the ui thread.

And even if I locally add a line to ensure this:

@SuppressLint("NewApi") @UiThread public void hideOverlaySurface2() { ensureRunningOnMainThread(); // <-- new line here if (platformViewsController2 == null) { throw new RuntimeException( "platformViewsController must be set before attempting to destroy an overlay surface"); } platformViewsController2.hideOverlaySurface(); }

I still don't get an error, which tells me this must be running on the platform (ui) thread. This is extra confusing, because if I log from the c++ side:

if (!FrameHasPlatformLayers()) { frame->Submit(); // If the previous frame had platform views, hide the overlay surface. HideOverlayLayerIfNeeded(); FML_LOG(ERROR) << "HI GRAY, hiding overlay layer because no platform views and current thread is " << (task_runners_.GetPlatformTaskRunner()->RunsTasksOnCurrentThread() ? "platform" : "not platform"); jni_facade_->applyTransaction(); return; }

I get logs:

HI GRAY, hiding overlay layer because no platform views and current thread is not platform

Which is extremely confusing! But my takeaway here is that we must eventually be passing this call to the platform thread, as otherwise we would be failing the call to ensureRunningOnMainThread();

So I think we don't have a race condition here (because it seems everything is being done on the platform thread), even if I don't understand the internals of why that is the case.

Perhaps we could even move both calls out of the posted task back into the main method?

Yeah, I had that thought as well but when I tried that for the task that we are posting below I DID get an error for making a call not on the platform thread:

E/flutter ( 7330): [ERROR:flutter/fml/platform/android/jni_util.cc(206)] java.lang.RuntimeException: Methods marked with @UiThread must be executed on the main thread. Current thread: 1.raster E/flutter ( 7330): at io.flutter.embedding.engine.FlutterJNI.ensureRunningOnMainThread(FlutterJNI.java:1607) E/flutter ( 7330): at io.flutter.embedding.engine.FlutterJNI.onDisplayPlatformView2(FlutterJNI.java:1373)

Again, very strange because it implies we are only passing the hideOverlaySurface calls but not the calls done in the task below. I really don't understand why that would be the case.

flar · 2025-08-21T21:41:29Z

engine/src/flutter/shell/platform/android/external_view_embedder/external_view_embedder_2.cc

        jni_facade->swapTransaction();

-        if (prev_frame_no_platform_views) {
+        if (prev_frame_no_platform_views && overlay_layer_has_content_) {


We might be able to also get rid of the prev_frame_no_pvs variable as well.

Up top, if we don't have anything to process, then call the internal hideIfNeeded method.

Then we show it if overlay_frame isn't null, or we show it in the callback, but only one of those should be needed, shouldn't it?

Also, is that PostTask run in a different thread? If it is, then we might want to think about whether the first case of noticing no more platform views shouldn't also be delegated to a Posted Task.

flar · 2025-08-21T21:42:10Z

engine/src/flutter/shell/platform/android/external_view_embedder/external_view_embedder_2.cc

        jni_facade->swapTransaction();

-        if (prev_frame_no_platform_views) {
+        if (prev_frame_no_platform_views && overlay_layer_has_content_) {


We need to either always call it in this method, or always call it in a posted task on the other thread. Having it called by either thread could lead to race conditions.

…ay_layer' into hcpp_clean_up_overlay_layer

flar

LGTM

…ontent and current doesn't (flutter/flutter#173881)

flutter/flutter@d2ac021...26bb33b 2025-08-22 [email protected] [HCPP] Clean up overlay layer when last frame had overlay content and current doesn't (flutter/flutter#173881) 2025-08-22 [email protected] Migrate more files to use WidgetStateProperty (flutter/flutter#174176) 2025-08-22 [email protected] Roll Skia from 75fef9fb3ed7 to cb15e1452399 (1 revision) (flutter/flutter#174255) 2025-08-22 [email protected] Roll Skia from 006241a7fbe1 to 75fef9fb3ed7 (1 revision) (flutter/flutter#174254) 2025-08-22 [email protected] Skip wasm build when dry run is disabled and --wasm is not specified. (flutter/flutter#174184) 2025-08-22 [email protected] Roll Dart SDK from c153c5259e62 to 4f9623f024ab (2 revisions) (flutter/flutter#174250) 2025-08-22 [email protected] Roll Skia from d70087007490 to 006241a7fbe1 (2 revisions) (flutter/flutter#174252) 2025-08-22 [email protected] Roll Skia from c09589f7ca69 to d70087007490 (22 revisions) (flutter/flutter#174245) 2025-08-21 [email protected] [ Widget Preview ] Add regression test for issue 173895 (flutter/flutter#174037) 2025-08-21 [email protected] Improve xcresult comment and naming (flutter/flutter#173129) 2025-08-21 [email protected] Update `.gemini/styleguide.md` to encourage `master`-only (flutter/flutter#174065) 2025-08-21 [email protected] [ Widget Preview ] Fix crash when attempting to provide non-const params to a `Preview` (flutter/flutter#174242) 2025-08-21 [email protected] Roll Skia from 721e68fe652a to c09589f7ca69 (12 revisions) (flutter/flutter#174162) 2025-08-21 [email protected] Roll Dart SDK from 0d0a0c394381 to c153c5259e62 (7 revisions) (flutter/flutter#174235) 2025-08-21 [email protected] [ Tool ] Throw `ToolExit` when asset entries use absolute paths (flutter/flutter#174230) 2025-08-21 [email protected] Roll Dart SDK from 0d0a0c394381 to c153c5259e62 (7 revisions) (flutter/flutter#174227) 2025-08-21 [email protected] [ Tool ] Cleanup widget preview and frontend server shutdown (flutter/flutter#173863) 2025-08-21 [email protected] Use an alternative to `git describe` for `master` version resolution (flutter/flutter#174088) 2025-08-21 [email protected] Report a correct display ID in the window metrics event on win32 (flutter/flutter#174156) 2025-08-21 [email protected] Revert "Update the AccessibilityPlugin::Announce method to account fo… (flutter/flutter#174223) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-packages Please CC [email protected],[email protected] on the revert to ensure that a human is aware of the problem. To file a bug in Packages: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md

… current doesn't (flutter#173881) Fixes flutter#173635 My current understanding of the issue is that we are never pushing a new empty frame for this layer, so the texture gets stuck as shown in the bug. If we keep track of when we had content last frame, and don't in the current frame, we can push a single empty frame for that layer when needed. See example in flutter#173635 (comment) ## Pre-launch Checklist - [x] I read the [Contributor Guide] and followed the process outlined there for submitting PRs. - [x] I read the [Tree Hygiene] wiki page, which explains my responsibilities. - [x] I read and followed the [Flutter Style Guide], including [Features we expect every widget to implement]. - [x] I signed the [CLA]. - [x] I listed at least one issue that this PR fixes in the description above. - [x] I updated/added relevant documentation (doc comments with `///`). - [x] I added new tests to check the change I am making, or this PR is [test-exempt]. - [x] I followed the [breaking change policy] and added [Data Driven Fixes] where supported. - [x] All existing and new tests are passing. If you need help, consider asking for advice on the #hackers-new channel on [Discord]. **Note**: The Flutter team is currently trialing the use of [Gemini Code Assist for GitHub](https://developers.google.com/gemini-code-assist/docs/review-github-code). Comments from the `gemini-code-assist` bot should not be taken as authoritative feedback from the Flutter team. If you find its comments useful you can update your code accordingly, but if you are unsure or disagree with the feedback, please feel free to wait for a Flutter team member's review for guidance on which automated comments should be addressed.  [Contributor Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#overview [Tree Hygiene]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md [test-exempt]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#tests [Flutter Style Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md [Features we expect every widget to implement]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md#features-we-expect-every-widget-to-implement [CLA]: https://cla.developers.google.com/ [flutter/tests]: https://github.com/flutter/tests [breaking change policy]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#handling-breaking-changes [Discord]: https://github.com/flutter/flutter/blob/main/docs/contributing/Chat.md [Data Driven Fixes]: https://github.com/flutter/flutter/blob/main/docs/contributing/Data-driven-Fixes.md --------- Co-authored-by: Gray Mackall <[email protected]>

Fixes #175882. This ended up being a very similar bug to #173881, and in my understanding comes from the same incorrect assumption. In old PV land these views would stop displaying if we stopped sending updates to them. But in HCPP the underlying implementation ends up using an Android SurfaceFlinger which continues displaying what it was last told to. So again we have a bug here were we aren't taking the new responsibility of clearing that we need to. See #173881 (comment) Other solutions would be to just always call hide on the set of (keys of view_params which are not in composition order) on every frame (which would avoid this mirroring of state), or to modify the jni here to pass over one big bundle with all the information required to do all the existing calls as well as determine which views need to be hidden. But we are already doing this same mirroring of visibility state for the overlay layer (see Hide/ShowOverlayLayerIfNeeded()), so I think it is reasonable to take the same approach here. This is sort of a follow up to #162908 as well, this PR properly removes the view when it is the last platform view, but does not fix the case that this PR attempts to more generally fix where we go from 2->1 (or n->n-1 > 0) pvs. ## Pre-launch Checklist - [x] I read the [Contributor Guide] and followed the process outlined there for submitting PRs. - [x] I read the [Tree Hygiene] wiki page, which explains my responsibilities. - [x] I read and followed the [Flutter Style Guide], including [Features we expect every widget to implement]. - [x] I signed the [CLA]. - [x] I listed at least one issue that this PR fixes in the description above. - [x] I updated/added relevant documentation (doc comments with `///`). - [x] I added new tests to check the change I am making, or this PR is [test-exempt]. - [x] I followed the [breaking change policy] and added [Data Driven Fixes] where supported. - [x] All existing and new tests are passing. If you need help, consider asking for advice on the #hackers-new channel on [Discord]. **Note**: The Flutter team is currently trialing the use of [Gemini Code Assist for GitHub](https://developers.google.com/gemini-code-assist/docs/review-github-code). Comments from the `gemini-code-assist` bot should not be taken as authoritative feedback from the Flutter team. If you find its comments useful you can update your code accordingly, but if you are unsure or disagree with the feedback, please feel free to wait for a Flutter team member's review for guidance on which automated comments should be addressed.  [Contributor Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#overview [Tree Hygiene]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md [test-exempt]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#tests [Flutter Style Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md [Features we expect every widget to implement]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md#features-we-expect-every-widget-to-implement [CLA]: https://cla.developers.google.com/ [flutter/tests]: https://github.com/flutter/tests [breaking change policy]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#handling-breaking-changes [Discord]: https://github.com/flutter/flutter/blob/main/docs/contributing/Chat.md [Data Driven Fixes]: https://github.com/flutter/flutter/blob/main/docs/contributing/Data-driven-Fixes.md --------- Co-authored-by: Gray Mackall <[email protected]>

…ontent and current doesn't (flutter/flutter#173881)

… current doesn't (flutter#173881) Fixes flutter#173635 My current understanding of the issue is that we are never pushing a new empty frame for this layer, so the texture gets stuck as shown in the bug. If we keep track of when we had content last frame, and don't in the current frame, we can push a single empty frame for that layer when needed. See example in flutter#173635 (comment) ## Pre-launch Checklist - [x] I read the [Contributor Guide] and followed the process outlined there for submitting PRs. - [x] I read the [Tree Hygiene] wiki page, which explains my responsibilities. - [x] I read and followed the [Flutter Style Guide], including [Features we expect every widget to implement]. - [x] I signed the [CLA]. - [x] I listed at least one issue that this PR fixes in the description above. - [x] I updated/added relevant documentation (doc comments with `///`). - [x] I added new tests to check the change I am making, or this PR is [test-exempt]. - [x] I followed the [breaking change policy] and added [Data Driven Fixes] where supported. - [x] All existing and new tests are passing. If you need help, consider asking for advice on the #hackers-new channel on [Discord]. **Note**: The Flutter team is currently trialing the use of [Gemini Code Assist for GitHub](https://developers.google.com/gemini-code-assist/docs/review-github-code). Comments from the `gemini-code-assist` bot should not be taken as authoritative feedback from the Flutter team. If you find its comments useful you can update your code accordingly, but if you are unsure or disagree with the feedback, please feel free to wait for a Flutter team member's review for guidance on which automated comments should be addressed.  [Contributor Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#overview [Tree Hygiene]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md [test-exempt]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#tests [Flutter Style Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md [Features we expect every widget to implement]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md#features-we-expect-every-widget-to-implement [CLA]: https://cla.developers.google.com/ [flutter/tests]: https://github.com/flutter/tests [breaking change policy]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#handling-breaking-changes [Discord]: https://github.com/flutter/flutter/blob/main/docs/contributing/Chat.md [Data Driven Fixes]: https://github.com/flutter/flutter/blob/main/docs/contributing/Data-driven-Fixes.md --------- Co-authored-by: Gray Mackall <[email protected]>

…#176742) Fixes flutter#175882. This ended up being a very similar bug to flutter#173881, and in my understanding comes from the same incorrect assumption. In old PV land these views would stop displaying if we stopped sending updates to them. But in HCPP the underlying implementation ends up using an Android SurfaceFlinger which continues displaying what it was last told to. So again we have a bug here were we aren't taking the new responsibility of clearing that we need to. See flutter#173881 (comment) Other solutions would be to just always call hide on the set of (keys of view_params which are not in composition order) on every frame (which would avoid this mirroring of state), or to modify the jni here to pass over one big bundle with all the information required to do all the existing calls as well as determine which views need to be hidden. But we are already doing this same mirroring of visibility state for the overlay layer (see Hide/ShowOverlayLayerIfNeeded()), so I think it is reasonable to take the same approach here. This is sort of a follow up to flutter#162908 as well, this PR properly removes the view when it is the last platform view, but does not fix the case that this PR attempts to more generally fix where we go from 2->1 (or n->n-1 > 0) pvs. ## Pre-launch Checklist - [x] I read the [Contributor Guide] and followed the process outlined there for submitting PRs. - [x] I read the [Tree Hygiene] wiki page, which explains my responsibilities. - [x] I read and followed the [Flutter Style Guide], including [Features we expect every widget to implement]. - [x] I signed the [CLA]. - [x] I listed at least one issue that this PR fixes in the description above. - [x] I updated/added relevant documentation (doc comments with `///`). - [x] I added new tests to check the change I am making, or this PR is [test-exempt]. - [x] I followed the [breaking change policy] and added [Data Driven Fixes] where supported. - [x] All existing and new tests are passing. If you need help, consider asking for advice on the #hackers-new channel on [Discord]. **Note**: The Flutter team is currently trialing the use of [Gemini Code Assist for GitHub](https://developers.google.com/gemini-code-assist/docs/review-github-code). Comments from the `gemini-code-assist` bot should not be taken as authoritative feedback from the Flutter team. If you find its comments useful you can update your code accordingly, but if you are unsure or disagree with the feedback, please feel free to wait for a Flutter team member's review for guidance on which automated comments should be addressed.  [Contributor Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#overview [Tree Hygiene]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md [test-exempt]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#tests [Flutter Style Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md [Features we expect every widget to implement]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md#features-we-expect-every-widget-to-implement [CLA]: https://cla.developers.google.com/ [flutter/tests]: https://github.com/flutter/tests [breaking change policy]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#handling-breaking-changes [Discord]: https://github.com/flutter/flutter/blob/main/docs/contributing/Chat.md [Data Driven Fixes]: https://github.com/flutter/flutter/blob/main/docs/contributing/Data-driven-Fixes.md --------- Co-authored-by: Gray Mackall <[email protected]>

Gray Mackall added 2 commits August 15, 2025 14:29

fix'

86cbb0e

format

d5af338

github-actions bot added platform-android Android applications specifically engine flutter/engine related. See also e: labels. team-android Owned by Android platform team labels Aug 15, 2025

gemini-code-assist bot reviewed Aug 15, 2025

View reviewed changes

engine/src/flutter/shell/platform/android/external_view_embedder/external_view_embedder_2.cc Outdated Show resolved Hide resolved

Gray Mackall added 2 commits August 15, 2025 14:42

clear it too, I guess

74ce0ca

driver test

1d1f1d8

gmackall marked this pull request as ready for review August 15, 2025 22:24

gmackall requested a review from a team as a code owner August 15, 2025 22:24

Gray Mackall added 2 commits August 15, 2025 15:39

clean up comments

ea06c9b

fix missing types

534621f

reidbaker requested a review from flar August 18, 2025 14:54

reidbaker approved these changes Aug 18, 2025

View reviewed changes

Gray Mackall and others added 2 commits August 18, 2025 10:24

missing license

8dc53fe

Merge branch 'master' into hcpp_clean_up_overlay_layer

25ad6c7

flar approved these changes Aug 18, 2025

View reviewed changes

Gray Mackall added 2 commits August 18, 2025 12:19

naming

ba02184

change to mirroring the format of prev_frame_no_platform_views

f6a9ef0

gmackall requested review from flar and reidbaker August 19, 2025 02:59

Gray Mackall and others added 3 commits August 18, 2025 20:14

comment

946315b

Merge branch 'master' into hcpp_clean_up_overlay_layer

cec6e60

Merge branch 'master' into hcpp_clean_up_overlay_layer

8401b6e

flar approved these changes Aug 19, 2025

View reviewed changes

Gray Mackall added 3 commits August 19, 2025 21:16

make the hiding and showing private methods, also guard the final sho…

4b684b9

…w on if there is pv content

format

aea07ca

Merge remote-tracking branch 'refs/remotes/origin/hcpp_clean_up_overl…

e00795c

…ay_layer' into hcpp_clean_up_overlay_layer

flar requested changes Aug 20, 2025

View reviewed changes

gmackall requested a review from flar August 20, 2025 17:50

Merge branch 'master' into hcpp_clean_up_overlay_layer

32d8233

flar requested changes Aug 21, 2025

View reviewed changes

Gray Mackall added 2 commits August 21, 2025 15:50

change to fully tracking the visibility

2de65f7

Merge remote-tracking branch 'refs/remotes/origin/hcpp_clean_up_overl…

fb1df89

…ay_layer' into hcpp_clean_up_overlay_layer

gmackall requested a review from flar August 21, 2025 22:54

move togglign to thread

c53477e

flar approved these changes Aug 22, 2025

View reviewed changes

reidbaker approved these changes Aug 22, 2025

View reviewed changes

gmackall added the autosubmit Merge PR when tree becomes green via auto submit App label Aug 22, 2025

auto-submit bot added this pull request to the merge queue Aug 22, 2025

Merged via the queue into flutter:master with commit 26bb33b Aug 22, 2025
186 checks passed

flutter-dashboard bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Aug 22, 2025

engine-flutter-autoroll mentioned this pull request Aug 22, 2025

Roll Flutter from d2ac0210ee05 to 26bb33b9572b (20 revisions) flutter/packages#9872

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 22, 2025

26bb33b95 [HCPP] Clean up overlay layer when last frame had overlay c…

c295410

…ontent and current doesn't (flutter/flutter#173881)

gmackall mentioned this pull request Oct 8, 2025

[HCPP] Properly remove hcpp views that are no longer visible #176742

Merged

9 tasks

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Nov 12, 2025

26bb33b95 [HCPP] Clean up overlay layer when last frame had overlay c…

b37c6c4

…ontent and current doesn't (flutter/flutter#173881)

[HCPP] Clean up overlay layer when last frame had overlay content and current doesn't #173881

[HCPP] Clean up overlay layer when last frame had overlay content and current doesn't #173881

Uh oh!

Conversation

gmackall commented Aug 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pre-launch Checklist

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

Uh oh!

gmackall commented Aug 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

reidbaker commented Aug 18, 2025

Uh oh!

reidbaker left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gmackall Aug 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gmackall Aug 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

flar left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

flar left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gmackall Aug 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gmackall Aug 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

gmackall commented Aug 15, 2025 •

edited

Loading

gmackall commented Aug 15, 2025 •

edited

Loading

reidbaker left a comment •

edited

Loading

gmackall Aug 18, 2025 •

edited

Loading

gmackall Aug 18, 2025 •

edited

Loading

gmackall Aug 20, 2025 •

edited

Loading

gmackall Aug 20, 2025 •

edited

Loading

gmackall Aug 26, 2025 •

edited

Loading