Skip to content

[video_player_android] Fix incorrect dimensions swap #9199

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

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

marvin-kolja
Copy link

@marvin-kolja marvin-kolja commented May 4, 2025

Fixes flutter/flutter#166097

List which issues are fixed by this PR. You must list at least one issue.

Pre-Review Checklist

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

Footnotes

  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. 2 3

…270 rotation

Since flutter#6535 `VideoSize` is used to get width and height instead of getting them from the video `Format`. My testing concluded that `VideoSize` dimension values are post rotation and do not need to be swapped when there is a 90 or 270 degree rotation present.
@marvin-kolja
Copy link
Author

marvin-kolja commented May 4, 2025

Tested changes on an emulator using Android 15 (API 35):

Texture View before changes

before_texture_view

Texture View after changes

after_texture_view

Video Used
RedApple.MOV

Still need to test on physical devices:

  • Android API <= 21
  • Android API >= 29

@camsim99
Copy link
Contributor

camsim99 commented May 5, 2025

Just an FYI that I'm working on #9107, which will change the API >= 29 check (that gates the usage of Formats) to be more accurate. I'm not sure if it impacts this change, but it's just something to be aware of.

@camsim99
Copy link
Contributor

camsim99 commented May 5, 2025

Just tested out a Pixel Tablet running API 34 (handles crop and rotation automatically) and it appears to work on the main branch and in this PR (with the videos provided in the example app--not sure if this makes a difference). Will do more testing on physical devices.

return RotatedBox(
quarterTurns: rotation ~/ 90,
child: child,
);
Copy link
Contributor

Choose a reason for hiding this comment

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

Why this change?

Copy link
Author

Choose a reason for hiding this comment

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

This is just to mirror the changes that were done in #8685 to the _VideoPlayerWithRotation of the main video_player package.

I can revert that change if it doesn't make sense in this PR...But the example is broken otherwise.

@marvin-kolja
Copy link
Author

Just an FYI that I'm working on #9107, which will change the API >= 29 check (that gates the usage of Formats) to be more accurate. I'm not sure if it impacts this change, but it's just something to be aware of.

Thanks for bringing that up. I also thought about that, but I don't see any reason for conflict.

@marvin-kolja
Copy link
Author

Just tested out a Pixel Tablet running API 34 (handles crop and rotation automatically) and it appears to work on the main branch and in this PR (with the videos provided in the example app--not sure if this makes a difference). Will do more testing on physical devices.

Appreciate it! They will definitely reflect if this PR messed anything up with videos that do not need a rotation, but I don't think any of the example videos require rotation to begin with. Maybe we can add a new video that would meet that requirement?

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.

[video_player_android] Expose correct AspectRatio with rotationCorrection into consideration
2 participants