-
Notifications
You must be signed in to change notification settings - Fork 29.7k
[CP][Android] Add a way to request new Surfaces from SurfaceProducer …
#172384
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
[CP][Android] Add a way to request new Surfaces from SurfaceProducer …
#172384
Conversation
…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
|
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. |
Surfaces from SurfaceProducer …Surfaces from SurfaceProducer …
matanlurey
left a comment
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.
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 ^
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. |
|
Ack. I am dismissing my comment because ultimately you all own the experience. Thanks! |
|
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? |
|
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. |
|
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. |
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. |
|
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. |
77b8e5d
into
flutter:flutter-3.32-candidate.0
…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.
Cherry-picks c7362b4 to 3.32 stable without the changes to the pre-existing
SurfaceProducer.getSurfacemethod, only the addition ofSurfaceProducer.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.getForcedNewSurfacewill receive aSurfacethat is different than previous calls toSurfaceProducer.getForcedNewSurfaceor pre-existing methodSurfaceProducer.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
getForcedSurfacemethod (like thecameraissues linked above require) can be done by creating newSurfaceProducers; however, this might have other side-effect unbeknownst to me.Risk:
Test Coverage:
Are you confident that your fix is well-tested by automated tests?
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.