Repository navigation
[Android][Impeller]we need to be notified before destroying the surface #152839
Description
Use case
I've read the migration guide and the migrated source of video_player.
Solution for the image loss problem in video_player:
In onSurfaceDestroyed, save and destroy the state of ExoPlayer, and then restore it in onSurfaceCreated. However, I believe this is a rather expensive operation, especially since the restoration process may cause noticeable pauses for the user. More importantly, it requires the video to stop playing, which may not be acceptable for some apps.
Therefore, I think a more efficient method should be used to solve this problem.
Proposal
Introduce an onSurfaceDestroying event.
In this event, we can remove the destroying surface from ExoPlayer to prevent errors. Then just call SetVideoSurface() in onSurfaceCreated (or even onSurfaceDestroyed). That should be able to avoid the steps of saving, destroying and restoring the state of ExoPlayer.
Activity
[-][Android][Impeller]we need to be notified before the destroying surface[/-][+][Android][Impeller]we need to be notified before destroying the surface[/+]jonahwilliams commentedon Aug 4, 2024
It's not really clear what you are proposing? What is the difference between onSurfaceDestroying and the existing lifecycle?
xxoo commentedon Aug 4, 2024
@jonahwilliams Thanks for reply
The difference is that in
onSurfaceDestroying, you can do things that are not possible inonSurfaceDestroyed, such as saving the current frame. Additionally, since the surface has already been destroyed inonSurfaceDestroyed,ExoPlayermay encounter errors, meaning that the player instance also has to be destroyed. However, if you can receive a notification beforehand, you have the opportunity to avoid this issue.xxoo commentedon Aug 4, 2024
@jonahwilliams I have updated the proposal to make it more clear. Please take another look thanks
8 remaining items
xxoo commentedon Aug 14, 2024
@matanlurey Thanks for reply
The cost indeed won’t be greater than the first time it was opened, but it still falls short of providing a seamless playback experience. This is because the media file may need to be reopened and buffered. Moreover, this issue only exists in Android.
This is the error that occurs when I call
clearVideoSurfaceinonSurfaceDestroyed. In fact, the error occurs whether or notclearVideoSurfaceis called. So, I believe that removing theSurfacefrom ExoPlayer at this point might already be too late. To avoid the error, it might only be possible by saving, destroying, and restoring ExoPlayer’s state. I suspect this might be the reason why you did it this way.To migrate to
SurfaceProducer, users should avoid cachingSurfaceinstances and instead use thegetSurface()method each time. However, this may not always be practical in real-world scenarios. This is because aSurfaceinstance might need to retain user-set states, or libraries like ExoPlayer may inherently require caching theSurfaceinstance. To maintain the usability ofTexturewidget, I believe it is still necessary to introduce an event that ensures users have the opportunity to prevent the following errors from occurring.matanlurey commentedon Aug 14, 2024
Why are you calling this? AFAICT this is not in migrated PR (which also doesn't crash).
matanlurey commentedon Aug 14, 2024
(At this point I need a specific code sample, i.e. "in this PR X, if I do Y, I observe a crash".)
xxoo commentedon Aug 15, 2024
@matanlurey
Because I tried to avoid saving and restoring the state of ExoPlayer
Please use this repo to reproduce the error
And visit here to see what was changed
I made the necessary changes on video_player_android 2.6.0
steps:
matanlurey commentedon Aug 15, 2024
That is not possible, and is the point of the PR.
At this point I am going to close this bug as WAI - when the PR merges, if there are problems, we can address them.
xxoo commentedon Aug 15, 2024
In the current api set, it is impossible indeed. And this is the reason I opened this proposal.
github-actions commentedon Aug 29, 2024
This thread has been automatically locked since there has not been any recent activity after it was closed. If you are still experiencing a similar issue, please open a new bug, including the output of
flutter doctor -vand a minimal reproduction of the issue.