Skip to content
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

[expo-av][android] fix video width same as video height #15579

Closed
wants to merge 1 commit into from

Conversation

mtroskot
Copy link

Why

On Android for some videos the width is the same as the video height. This PR fixes that issue.

#12839

How

Using pixelWidthHeightRatio as described in google/ExoPlayer#3690

Test Plan

Tested on emulator with reproducible example https://snack.expo.dev/@trokiize/android-video-size-wrong.

@expo-bot
Copy link
Collaborator

Hi there! 👋 I'm a bot whose goal is to ensure your contributions meet our guidelines.

I've found some issues in your pull request that should be addressed (click on them for more details) 👇

⚠️ Suggestion: Missing changelog entries


Your changes should be noted in the changelog. Read Updating Changelogs guide and consider (it's optional) adding an appropriate entry to the following changelogs:


Generated by ExpoBot 🤖 against 9b48bca

@expo-bot expo-bot added the bot: suggestions ExpoBot has some suggestions label Dec 15, 2021
@jeremyspritelyco
Copy link

why does this PR still open? does the issue fixed?

@jeremyspritelyco
Copy link

thanks for this PR. I am using this changes with patch package and it fixed my issue, related to #12839

Copy link
Contributor

@bbarthec bbarthec left a comment

Choose a reason for hiding this comment

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

As long as this PR does the job I'm ok with that, but it looks fishy that origianl height is not reported correctly and it's needed to be calculated from the pixelWidthHeightRatio and width.
cc @lukmccall, @tsapeta
The PR itself lacks proper CHANGELOG entry.

@lukmccall
Copy link
Contributor

It doesn't seem right. Can you provide more context about your solution?

@lukmccall lukmccall self-requested a review September 8, 2022 12:52
@mtroskot
Copy link
Author

mtroskot commented Sep 8, 2022

Doesn't the linked ExoPlayer github issue and the reproducible demo give enough context ?

@lukmccall
Copy link
Contributor

Doesn't the linked ExoPlayer github issue and the reproducible demo give enough context?

Ok, you convinced me that is enough information. It seems very weird that we have to calculate the video width. I'll merge that, but please add a changelog entry to https://github.com/expo/expo/blob/main/packages/expo-av/CHANGELOG.md

@cgav
Copy link

cgav commented Nov 23, 2022

@mtroskot Any chance you could add the changelog entry? Would be nice to finally have that issue resolved 🙂

@lukmccall
Copy link
Contributor

That issue should be fixed by #19332. We bumped the ExoPlayer. The current version should return the correct video size.

@lukmccall lukmccall closed this Nov 23, 2022
@cgav
Copy link

cgav commented Nov 23, 2022

@lukmccall It still seems to be broken in Expo47. I have tweaked the snack from @mtroskot and upgraded the npm modules to the latest versions. As you can see it shows a popup with the same width and height.

Snack: https://snack.expo.dev/@cgavrilete/android-video-size-wrong

@lukmccall
Copy link
Contributor

According to the https://github.com/google/ExoPlayer/blob/release-v2/RELEASENOTES.md#2140-2021-05-13 changelog, the fix which was pointed out in the description should be applied to the ExoPlayer codebase. It may not be enough, but it also means that this PR won't fix that either. I'll take a closer look later to check if that's true.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bot: suggestions ExpoBot has some suggestions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants