Skip to content

Conversation

@camsim99
Copy link
Contributor

@camsim99 camsim99 commented Jul 18, 2025

Cherry-picks c7362b4 to 3.32 stable without the changes to the pre-existing SurfaceProducer.getSurface method, only the addition of SurfaceProducer.getForcedNewSurface.

Note that this change is already present in the 3.35 beta.

Issue Link:

What is the link to the issue this cherry-pick is addressing?
#155294, #169506

Changelog Description:

Adds a new method for requesting a new surface from the embedder than any previously returned.

Impact Description:

Developers using SurfaceProducer.getForcedNewSurface will receive a Surface that is different than previous calls to SurfaceProducer.getForcedNewSurface or pre-existing method SurfaceProducer.getSurface.

In other words, there is no impact to current users. This is a new method, so there will be a gap between 3.32.0 and 3.32.8, but I think it's worth landing because this would unblock fixing resuming/pausing the camera preview as described by the linked issues.

Workaround:

Working around the need for the new getForcedSurface method (like the camera issues linked above require) can be done by creating new SurfaceProducers; however, this might have other side-effect unbeknownst to me.

Risk:

  • Low
  • Medium
  • High

Test Coverage:

Are you confident that your fix is well-tested by automated tests?

  • Yes
  • No

Validation Steps:

You can test with flutter/packages#9360 (which I accidentally landed because I forgot this wasn't in stable yet...oops).

Pre-launch Checklist

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

…and avoid `SurfaceProducer` returning invalid `Surface` (flutter#169899)

> [!NOTE]
> For anyone reviewing this PR, see
flutter/packages#9360 for how I'd use the
`getSurface(boolean forceNewSurface)` method.

> [!NOTE]
> For anyone coming across this PR post-landing, in the code review
process, I renamed `getSurface(boolean forceNewSurface)` to
`getForcedNewSurface()`.

(1) Adds method `getSurface(boolean forceNewSurface)` to
`SurfaceProducer` that will force the creation of a new `Surface` when
`SurfaceProducer.getSurface()` is called.
(2) Fixes `SurfaceProducer` to avoid returning invalid `Surface`s when
`SurfaceProducer.getSurface()`/`getSurface(boolean forceNewSurface)` is
called.

My motivation for adding this is directly tied to
flutter#155294. The
`camera_android_camerax` plugin supports a camera preview use case that
requires providing a `Surface` to in order to render the preview. It
does so via `SurfaceProducer`; we provide a `Surface` retrieved from
`SurfaceProducer.getSurface()` to the CameraX library to render a camera
preview, and when the camera preview is done, a callback that we provide
is called, reporting that `Surface` as used and that it now should be
released/invalidated (see
[`SurfaceRequest`](https://developer.android.com/reference/androidx/camera/core/SurfaceRequest),
[`SurfaceRequest.Result`](https://developer.android.com/reference/androidx/camera/core/SurfaceRequest#provideSurface(android.view.Surface,java.util.concurrent.Executor,androidx.core.util.Consumer%3Candroidx.camera.core.SurfaceRequest.Result%3E))).
However, the CameraX library [makes no
guarantees](https://developer.android.com/reference/androidx/camera/core/Preview.SurfaceProvider#onSurfaceRequested(androidx.camera.core.SurfaceRequest):~:text=The%20camera%20may%20repeatedly%20request%20surfaces%20throughout%20usage%20of%20a%20Preview%20use%20case%2C%20but%20only%20a%20single%20request%20will%20be%20active%20at%20a%20time.)
about when requests for new `Surface`s are made, so the following race
condition can happen:

1. CameraX requests a `Surface`
2. We provide `Surface` A from `SurfaceProducer.getSurface()`
3. The camera preview was paused; CameraX requests a new `Surface`
4. The camera preview is resumed
5. We provide `Surface` A from `SurfaceProducer.getSurface()` because it
is still technically valid
6. CameraX calls our callback and now `Surface` A is released/invalid

I believe `SurfaceProducer` currently has no way to handle the level of
complexity of `Surface` handling required for use cases like the
`camera_android_camerax` camera preview, where a `Surface` may be
invalidated between calls to `SurfaceProducer.getSurface()`. So,
`getSurface(boolean forceNewSurface)` can support these use cases.*

`getSurface(boolean forceNewSurface)` simply will force
`SurfaceProducer` to return a new `Surface` when
`SurfaceProducer.getSurface` is called versus potentially returning the
same `Surface` as the previous invocation.

For `camera_android_camerax`, see
flutter/packages#9360 for an example of how it
would be used.

*I'd like to note that I also played around with creating new
`SurfaceProducer`s to solve the camera problem, which _was_ successful
and probably could be generally for that use case, but I think it's
cleaner to support this functionality from within the same
`SurfaceProducer` since we can 🤷‍♀️ It also helps avoid the user from
needing to create/manage multiple Flutter `Texture`s, from my
understanding.

forceNewSurface)` to never return an invalid `Surface`s

I noticed that `SurfaceProducer.getSurface()`/`getSurface(boolean
forceNewSurface)` does not check for `Surface` validity before returning
them while I was working on (1). Honestly, this just feels right? We
should ensure that a `Surface` is valid and can be used before returning
it and potentially confusing the user.

- [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].

<!-- Links -->
[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
@flutter-dashboard
Copy link

This pull request was opened from and to a release candidate branch. This should only be done as part of the official Flutter release process. If you are attempting to make a regular contribution to the Flutter project, please close this PR and follow the instructions at Tree Hygiene for detailed instructions on contributing to Flutter.

Reviewers: Use caution before merging pull requests to release branches. Ensure the proper procedure has been followed.

@github-actions github-actions bot added platform-android Android applications specifically engine flutter/engine related. See also e: labels. labels Jul 18, 2025
@camsim99 camsim99 marked this pull request as ready for review July 21, 2025 15:34
@camsim99 camsim99 added cp: review Cherry-picks in the review queue and removed cp: review Cherry-picks in the review queue labels Jul 21, 2025
@camsim99 camsim99 added the cp: review Cherry-picks in the review queue label Jul 21, 2025
@camsim99 camsim99 requested a review from reidbaker July 21, 2025 17:15
@camsim99 camsim99 changed the title [Android] Add a way to request new Surfaces from SurfaceProducer [CP][Android] Add a way to request new Surfaces from SurfaceProducer Jul 21, 2025
Copy link
Contributor

@matanlurey matanlurey left a comment

Choose a reason for hiding this comment

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

Just to clarify (leaving as blocking to force ack before submitting):

With this PR, users of stable 3.32.0 to 3.32.7, who consume plugins that use this (newly added) API, will get (semi)-cryptic errors that getForcedNewSurface() does not exist, and will either have to downgrade to an older plugin version (that does not use the API), or upgrade to the latest hot-fix.

If you are OK with that - i.e. this fixes a critical problem with no workarounds, and the above possibility (^) is better than the status quo, please go ahead and disregard this review and submit the PR.

If you want to chat further, grab me! I am still supportive, I just don't quite understand why we're changing public APIs in a hot fix (we've almost never done that, at least while I've been at Flutter).

/cc @stuartmorgan-g's FYI only ^

@camsim99
Copy link
Contributor Author

users...will get (semi)-cryptic errors that getForcedNewSurface() does not exist

Hmm this could be scary for some folks. Would love to get this fixed sooner than a month from now because a camera feature simply isn't working, but no one's throwing a riot over it not being fixed so if @stuartmorgan-g and @reidbaker think this is blocking, I'm ok to wait.

@matanlurey
Copy link
Contributor

Ack. I am dismissing my comment because ultimately you all own the experience. Thanks!

@matanlurey matanlurey dismissed their stale review July 21, 2025 19:29

Self-ack.

@reidbaker
Copy link
Contributor

Well the only plugin that we expect to use this API is camera and if you are using this API then you need it for functionality.

Can we set the minimum flutter version in our plugin to a patch version?

@camsim99
Copy link
Contributor Author

That would definitely avoid the error being shown! Though I admit, that would be a sorta big jump from the current min 3.29.0: https://github.com/flutter/packages/blob/9c85e5e72e7b015720e6f5934fca1c9f73cd1028/packages/camera/camera_android_camerax/pubspec.yaml#L9. Though, in a month, folks would have to update to 3.35 when it's promoted anyway.

@reidbaker
Copy link
Contributor

I think version constraints that reflect our api usages are ok and that in the packages repo we are ok bumping the minimum flutter version to any published stable. Lets land this.

@stuartmorgan-g
Copy link
Contributor

With this PR, users of stable 3.32.0 to 3.32.7, who consume plugins that use this (newly added) API, will get (semi)-cryptic errors that getForcedNewSurface() does not exist

As noted in discussion above, this would only be true if plugin authors add use of a new API without saying they require the Flutter SDK version that introduces that API. Doing that will always break people; it's not specific to cherry-picks.

We don't have a mechanism to catch it in CI if we don't do this for native API usage, so we should just be very careful with any flutter/packages PRs that add usage of this.

@camsim99
Copy link
Contributor Author

Bumping the minimum Flutter version for camera_android_camerax sounds good to me! Will be careful if used for other plugins, but there are no plans for that as of now.

@camsim99 camsim99 added the autosubmit Merge PR when tree becomes green via auto submit App label Jul 22, 2025
@auto-submit auto-submit bot merged commit 77b8e5d into flutter:flutter-3.32-candidate.0 Jul 22, 2025
164 checks passed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jul 25, 2025
auto-submit bot pushed a commit to flutter/packages that referenced this pull request Aug 6, 2025
…equest" (#9760)

> [!IMPORTANT]  
> **(DONE ✅ in flutter/flutter#172384
> flutter/flutter#169899 must be rolled into packages AND be in Flutter stable for this to land. I will try to cherry-pick this change into 3.32 stable once the 3.35 beta is cut.

> [!NOTE]
> Several files are not changed but simply formatted as a result of the bumped Flutter/Dart versions for `camera_android_camerax` that include flutter/flutter#171703. I will comment on these files for clarity.

Re-lands #9360.

Since timing for `Surface` requests cannot be guaranteed, request a new `Surface` from `SurfaceProducer.getSurface` each time a `Surface` is requested for rendering the camera preview to.

Fixes flutter/flutter#155294.
Fixes flutter/flutter#169506.

## Pre-Review Checklist

**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.

[^1]: Regular contributors who have demonstrated familiarity with the repository guidelines only need to comment if the PR is not auto-exempted by repo tooling.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

autosubmit Merge PR when tree becomes green via auto submit App cp: review Cherry-picks in the review queue engine flutter/engine related. See also e: labels. platform-android Android applications specifically

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants