-
Notifications
You must be signed in to change notification settings - Fork 0
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
feat: Add support for allowed aspect ratios [CC-708] #40
Conversation
@@ -305,7 +302,50 @@ fileprivate extension YPAssetZoomableView { | |||
self?.centerAssetView() | |||
} | |||
} | |||
|
|||
|
|||
func findClosestAllowedAspectRatio(for aspectRatio: CGFloat) -> CGFloat { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would be nice to have a test for but we don't have that established in the image picker library nor do we have the CI infrastructure setup to run tests even if we did have them here 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah true. Maybe it would be worth it to just add a simple unit test file for this, even if it's not automated in the CI.
873a271
to
d2a0cc9
Compare
a3a8d78
to
ba6e260
Compare
@@ -234,6 +237,15 @@ open class LibraryMediaManager { | |||
} else { | |||
// transfer the transform so the video renders in the correct orientation | |||
videoCompositionTrack.preferredTransform = transform | |||
videoComposition?.renderSize = videoSize |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On line 236, can probably clean up that comment right? I think we determined that videoComposition?.renderSize = cropRect.size
is needed? (based on Slack convo)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh yeah sure, I'll clean it up.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated now.
ba6e260
to
aa915ae
Compare
This PR adds support for a bunch of configuration variables allowing setting lists of "allowed aspect ratios" for single-video and multi-asset scenarios. The videos (or media, in the case of multiple selection) will be auto-cropped to the nearest aspect ratio from the list. An "override" list is also provided for both scenarios (single video and multi-selection), allowing for a situation where we'll want to allow certain aspect ratios to be "always allowed" but never selected automatically.
Additionally, it adds a configuration value for "max video resolution", which allows setting a maximum value for the resulting processed video's pixel count (height * width). It also automatically rounds the resulting width and height values to the nearest even number, which is a requirement for many video post processing use cases.