Skip to content

fix(mobile): video player restarting when device rotates #17362

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

Merged

Conversation

Sese-Schneider
Copy link
Contributor

@Sese-Schneider Sese-Schneider commented Apr 3, 2025

Description

Fixes the video player restarting when rotating the device.

Cause was a builder which would create a new GlobalKey every time the device was rotated (and therefore the WidgetTree rebuilt). This new GlobalKey would cause a complete reinitialization of the Widget, rendering the useState useless.

Changed the key to a ValueKey which should still work for the hero animation it was introduced for.
#17362 (comment)

Fixes #16294

How Has This Been Tested?

  • Test A
    • open a video in portrait mode
    • rotate the device to landscape
    • video should continue playing and not restart
  • Test B
    • open the video
    • video should not be re-initialized

Checklist:

  • I have performed a self-review of my own code
  • I have made corresponding changes to the documentation if applicable
  • I have no unrelated changes in the PR.
  • I have confirmed that any new dependencies are strictly necessary.
  • I have written tests for new code (if applicable)
  • I have followed naming conventions/patterns in the surrounding code
  • All code in src/services/ uses repositories implementations for database calls, filesystem operations, etc.
  • All code in src/repositories/ is pretty basic/simple and does not have any immich specific logic (that belongs in src/services/)

@bo0tzz bo0tzz requested a review from shenlong-tanwen April 3, 2025 13:34
@bo0tzz bo0tzz changed the title fix(mobile): Video player restarting when device rotates fix(mobile): video player restarting when device rotates Apr 3, 2025
Copy link
Contributor

@mertalev mertalev left a comment

Choose a reason for hiding this comment

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

The global key was essential here when I added this code because of how the hero animation internally renders the target page. I don't think it should be changed to a value key unless we can confirm the video is never reinitialized.

@Sese-Schneider
Copy link
Contributor Author

@mertalev can you tell me how I can test out the hero animation? I was unable to reproduce this.
Based on my current understanding of the code the ValueKey would have the exact same effect, as the builder is not called again.

Issue with rotation is that the whole tree is rebuilt, and we need a consistent key across rebuilds to avoid the video controller reinitializing

@mertalev
Copy link
Contributor

mertalev commented Apr 3, 2025

You can add debug logs to the video player loading methods, including the aspect ratio, video source, etc. They should be called exactly once for an asset.

@Sese-Schneider
Copy link
Contributor Author

Sese-Schneider commented Apr 3, 2025

@mertalev thanks for your help, I know understand why the GlobalKey was needed.

I have now introduced the GlobalKey to be created earlier, ensuring it is not thrown away on every rebuild, while staying global during hero animations.
It has been put into the state following Flutter best practices.

Everything has been verified using loggin here on initial load (hero) and by rotating the device.

@Sese-Schneider Sese-Schneider requested a review from mertalev April 3, 2025 14:39
Copy link
Member

@shenlong-tanwen shenlong-tanwen left a comment

Choose a reason for hiding this comment

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

Thank you!

@alextran1502 alextran1502 merged commit 1e4b9ae into immich-app:main Apr 7, 2025
39 checks passed
midzelis pushed a commit that referenced this pull request Apr 10, 2025
* fix(mobile): Video player restarting when device rotates

* use global key in state

* Implement suggestions from code review
savely-krasovsky pushed a commit to savely-krasovsky/immich that referenced this pull request Jun 8, 2025
…17362)

* fix(mobile): Video player restarting when device rotates

* use global key in state

* Implement suggestions from code review
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Rotating phone during video playback restarts video from beginning
5 participants