-
Notifications
You must be signed in to change notification settings - Fork 5.8k
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
[image-picker][ios] Fix Animated GIF support #20034
Conversation
Co-authored-by: Expo Bot <[email protected]>
Awesome work and thanks for picking this up so quickly. |
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.
Looking good ✅
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.
keep them untouched as they're now on iOS - IMO this is better, because compression sometimes acts weirdly and they're larger than uncompressed
I would go with that solution ;)
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.
Nice! Good to see you back 🤗
I'm also in favor of 2) 😉
Co-authored-by: Tomasz Sapeta <[email protected]>
# Why I was talking to @hirbod who discovered that animated gifs aren't working on iOS in contrast to [what docs say](https://docs.expo.dev/versions/latest/sdk/imagepicker/#imagepickerlaunchimagelibraryasyncoptions): > Animated GIFs support If the selected image is an animated GIF, the result image will be an animated GIF too if and only if quality is set to undefined and allowsEditing is set to false. Otherwise compression and/or cropper will pick the first frame of the GIF and return it as the result (on Android the result will be a PNG, on iOS — GIF). What's more, I tracked it down that they probably never worked before (what I mentioned in #18138 description), because the initial Objective-C implementation was wrong (SDK 44 and earlier). Further Swift PRs (#15977, #18135, #18138) only refactored the already-bad Obj-C implementation. # How Found a way to do it properly with the `CoreGraphics` framework: - For non-compressed images, it is enough to passthrough the raw data - For images with `quality` set or being cropped, we need to recreate the GIF: 1. Get source image metadata 2. Create a destination buffer 3. Copy image frames, crop / compress each frame if needed 4. Save destination image data Also updated the docs accordingly. # Test Plan NCL on iOS Simulator, animated gif, all combinations of: - Quality: `undefined`, `1.0`, `!=1.0` - `allowsEditing`: `true`, `false` Also output from my test app (_the built-in iOS crop preview is buggy 🤷_): https://user-images.githubusercontent.com/278340/201655161-c09a8f2e-e51e-4ba3-bb5b-b953050d3f2c.mov # Discussion topics ❓ Two things: - It was undocumented, that on Android (at least SDK 47), quality MUST be set to `1.0` for animated gifs to work. Docs were saying something opposite, that it must be `undefined` (docs were wrong). My PR fixes that description. Please review my wording there. - On iOS, until now it wasn't working anyway. Even though I made it work for all qualities, it behaves the same for quality set to `1.0` or `undefined`. This is in contrast to JPEG, where quality defaults to `0.2`. Should we: 1. also compress the GIFs by default - when quality is not set, as we do for JPEG 2. keep them untouched as they're now on iOS - IMO this is better, because compression sometimes acts weirdly and they're larger than uncompressed If 1) then I need to adjust this PR to use default quality when `nil`. If 2) then the Android part needs a fix and so the docs accordingly. Co-authored-by: Expo Bot <[email protected]> Co-authored-by: Tomasz Sapeta <[email protected]>
Why
I was talking to @hirbod who discovered that animated gifs aren't working on iOS in contrast to what docs say:
What's more, I tracked it down that they probably never worked before (what I mentioned in #18138 description), because the initial Objective-C implementation was wrong (SDK 44 and earlier). Further Swift PRs (#15977, #18135, #18138) only refactored the already-bad Obj-C implementation.
How
Found a way to do it properly with the
CoreGraphics
framework:quality
set or being cropped, we need to recreate the GIF:Also updated the docs accordingly.
Test Plan
NCL on iOS Simulator, animated gif, all combinations of:
undefined
,1.0
,!=1.0
allowsEditing
:true
,false
Also output from my test app (the built-in iOS crop preview is buggy 🤷):
Screen.Recording.2022-11-14.at.13.00.23.mov
Discussion topics ❓
Two things:
It was undocumented, that on Android (at least SDK 47), quality MUST be set to
1.0
for animated gifs to work. Docs were saying something opposite, that it must beundefined
(docs were wrong). My PR fixes that description. Please review my wording there.On iOS, until now it wasn't working anyway. Even though I made it work for all qualities, it behaves the same for quality set to
1.0
orundefined
. This is in contrast to JPEG, where quality defaults to0.2
. Should we:If 1) then I need to adjust this PR to use default quality when
nil
.If 2) then the Android part needs a fix and so the docs accordingly.