Change timing of onSurfaceDestroyed to match onSurfaceCleanup
#161252
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
github-actions
bot
added
platform-android
reidbaker
approved these changes
Jan 7, 2025
| * </pre> | ||
| */ | ||
| default void onSurfaceCleanup() {} | ||
| default void onSurfaceCleanup() { |
Contributor
There was a problem hiding this comment.
Should we be in control of the call to onSurfaceDestroyed?
Seems confusing that if I override onSurfaceCleanup that this other life cycle method stops firing.
On the other hand, it's nice that if you use onSurfaceCleanup you don't have to worry about what to do with onSurfaceDestroyed because it won't ever be called.
Contributor
There was a problem hiding this comment.
We should at least document this
Contributor
Author
There was a problem hiding this comment.
Done, and applied to created/available as well for consistency,
|
This looks good to me. I believe it should solve the video player issue, though I'm not familiar with camerax. |
matanlurey
added
the
autosubmit
flutter-dashboard
bot
removed
the
autosubmit
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/packages
that referenced
this pull request
Jan 7, 2025
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/packages
that referenced
this pull request
Jan 8, 2025
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/packages
that referenced
this pull request
Jan 8, 2025
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/packages
that referenced
this pull request
Jan 8, 2025
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/packages
that referenced
this pull request
Jan 8, 2025
srujzs
pushed a commit
to srujzs/flutter
that referenced
this pull request
Jan 12, 2025
…utter#161252) Follow-up to flutter#160937 (flutter#160933). This more or less mirrors what we did already for `onSurfaceCreated` to match `onSurfaceAvailable`. With this approach, the master branch should immediately start seeing better behavior in terms of being able to use the `onSurfaceDestroyed` callback effectively (before the surface has been released). For example, for a plugin that already uses `onSurfaceDestroyed`, [i.e `camerax`](https://github.com/flutter/packages/blob/97ce56a68eea650dc784617b4eed7814cccedeb8/packages/camera/camera_android_camerax/android/src/main/java/io/flutter/plugins/camerax/PreviewHostApiImpl.java): ```java @OverRide public void onSurfaceDestroyed() { // Invalidate the SurfaceRequest so that CameraX knows to to make a new request // for a surface. request.invalidate(); } ``` ... the request is now invalidated _before_ the surface has been destroyed, which is what (I believe) we wanted? --- Folks that want to publish package updates that _definitely_ use the correct timing will have to wait for the next stable. /cc @hasali19 would be great to get your input here.
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/packages
that referenced
this pull request
Feb 12, 2025
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/packages
that referenced
this pull request
Feb 13, 2025
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/packages
that referenced
this pull request
Feb 13, 2025
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/packages
that referenced
this pull request
Mar 6, 2025
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/packages
that referenced
this pull request
Mar 7, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Follow-up to #160937 (#160933).
This more or less mirrors what we did already for
onSurfaceCreatedto matchonSurfaceAvailable.With this approach, the master branch should immediately start seeing better behavior in terms of being able to use the
onSurfaceDestroyedcallback effectively (before the surface has been released). For example, for a plugin that already usesonSurfaceDestroyed, i.ecamerax:... the request is now invalidated before the surface has been destroyed, which is what (I believe) we wanted?
Folks that want to publish package updates that definitely use the correct timing will have to wait for the next stable.
/cc @hasali19 would be great to get your input here.