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

Added support for skipping GIF compression. #738

Closed
wants to merge 1 commit into from

Conversation

Mikunj
Copy link
Contributor

@Mikunj Mikunj commented Jun 27, 2018

When users select GIFs they get compressed to static Jpeg images. This PR allows you to skip that compression and thus keep the original GIF image.

There's also some minor fixes with compressImageMaxWidth and compressImageMaxHeight on iOS. Before it would scale the image to compressImageMaxWidth or compressImageMaxHeight even if the image was smaller than those values.

Fixed minor issues with compressImageMaxWidth and
compressImageMaxHeight.
@fubar
Copy link
Contributor

fubar commented Jun 27, 2018

I think this has too many side effects. First, there are multiple file types that you would want to be able to keep in its original format and without modification, not just gifs. We recently discussed this here #689.

Then a user should be able to indicate whether they want images to be converted and compressed or not. For compression, this behaviour already exists on the iOS side (see 64d09ba) - simply leave out the compressImageQuality and compressImageMaxWidth/Height options and it uses the original file.

Based on this, I have opened a PR just a couple days ago that will always convert unknown file types (#735). Adding to the set of "known" file types that don't get converted should be well reasoned for and documented.

So in summary, you already have the ability to skip image compression in iOS, but it would be great to have the same behaviour extended to Android as well!

Are there any other motivations for this change? Happy to hear your thoughts!

@Mikunj
Copy link
Contributor Author

Mikunj commented Jun 27, 2018

The main motivation for the change was that the change in 64d09ba only worked on iOS. So if we left out compressImageQuality then it would all work fine on iOS, however on Android it would still convert the GIF into a JPEG and furthermore the file sizes stayed the same since compression wasn't applied.

I could go ahead and modify the code so that users can pass in an array of mimes that they don't want to compress, which would give more flexibility to this use-case.

@fubar
Copy link
Contributor

fubar commented Jun 28, 2018

I think that would create an awkward user/developer experience.

I think the first step is to create parity on the Android side with what we have for iOS today, ie. use original images if they are smaller than the specified max dimensions and no compression is desired. This should already cover most cases in which you want to use the original GIFs. In the cases where the dimensions of a GIF are larger than the given maximums, you still need to respect the user's/developer's choice and make it smaller (and into a JPG). This should always have precedence over any other settings - if I say no image should be larger than a certain size, I don't want this library to give me larger ones.

This would then leave the question what to do with GIFs and PNGs when no dimension limit is set but compression is desired. This is a tough one to answer because no behaviour will cover all use cases. Since this is primarily an image picker library though, I see a case for leaving GIFs and PNGs unmodified as they are already compressed formats, and because their compression is very different from JPGs, they are generally not used for photos. So maybe a good line to draw here is that we should convert and compress any non-standard formats (eg. HEIC), known but uncompressed formats (eg. TIFF, raw), as well as JPGs, but not convert any other ones (namely GIF and PNG). If no compression is desired either, this would largely stay the same, except JPGs would remain unmodified as well.

Thinking this through a bit further, I could also envision a use case in which a user/developer would want all images to stay like they are, in their original format, and with no modification or compression applied. For that use case I can see how a "don't convert anything" setting could be useful. This is a rather specific use case though and I'm not sure this fits into the purpose of this library.

@Mikunj
Copy link
Contributor Author

Mikunj commented Jun 28, 2018

Alright i can leave this specific use case on my fork but i can go and make the equivalent changes made in 64d09ba on Android.

@fubar
Copy link
Contributor

fubar commented Jun 28, 2018

That would be great, thanks! I'd also love if @ivpusic could chime in here since it's his library. And note that my PR to convert unknown file types (#735) has not been merged yet.

@Mikunj
Copy link
Contributor Author

Mikunj commented Jul 1, 2018

I'll get the other change done by this week. I'm going to close this PR.

@Mikunj Mikunj closed this Jul 1, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants