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

[WIP] Only resize large GIFs when neccessary to avoid breaking alpha #11379

Closed

Conversation

moagstar
Copy link
Collaborator

Added some logic to the custom emoji resizing code to prevent resizing in certain circumstances. This is because the pillow gif handling code seems to be rather flaky with regards to handling gif color palletes, causing broken gifs after resizing. The workaround is to only resize when absolutely necessary, which is basically whenever not resizing wouldn't cause bandwidth issues (size, file size). Non square images are always resiezd.

#10351

Testing Plan:
I added some tests to EmojiTest and tested manually.

I discussed this approach on chat.zulip.org and there seems to be consensus that this is a worthwhile approach. Now I am looking for feedback on the actual values chosen.

@moagstar moagstar changed the title [WIP] Added some logic to the custom emoji resizing code to prevent resizin… [WIP] Only resize large GIFs to avoid breaking images Jan 28, 2019
@moagstar moagstar changed the title [WIP] Only resize large GIFs to avoid breaking images [WIP] Only resize large GIFs when neccessary to avoid breaking alpha Jan 28, 2019
@@ -35,6 +35,8 @@
DEFAULT_AVATAR_SIZE = 100
MEDIUM_AVATAR_SIZE = 500
DEFAULT_EMOJI_SIZE = 64
MAX_EMOJI_GIF_SIZE = 128
MAX_EMOJI_GIF_FILE_SIZE_BYTES = 16 * 1024 * 1024 # 16 kb
Copy link
Sponsor Member

@timabbott timabbott Jan 28, 2019

Choose a reason for hiding this comment

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

From looking https://slackmojis.com/ (a random website I found with lots of custom emoji), most custom emoji files folks might try to upload are probably bigger than 16KB. Based on that analysis, I think doing something like 128KB as the limit here would be reasonable. 128 seems right for max size.

@timabbott
Copy link
Sponsor Member

@moagstar this looks reasonable, thanks for doing this! Can you clean up the linter errors and adjust the constants as suggested above? After those changes, I'm happy to merge this.

@timabbott
Copy link
Sponsor Member

Oh, also, you should clean up the commit message to line-wrap properly and otherwise follow our conventions (compare in git log and you'll see what I mean). Check out the Zulip commit message guidelines for more details: https://zulip.readthedocs.io/en/latest/contributing/version-control.html#commit-messages

@moagstar moagstar force-pushed the 10351-custom-gif-emoji-alpha-color branch from 929a27f to db3ac35 Compare January 29, 2019 09:34
@moagstar moagstar force-pushed the 10351-custom-gif-emoji-alpha-color branch 2 times, most recently from d4dd893 to 1c8fd47 Compare January 29, 2019 09:49
@zulipbot zulipbot added size: L and removed size: M labels Jan 29, 2019
@moagstar moagstar force-pushed the 10351-custom-gif-emoji-alpha-color branch from 1c8fd47 to 45a5a94 Compare January 29, 2019 10:27
@zulipbot zulipbot added size: M and removed size: L labels Jan 29, 2019
…ain circumstances.

This additional logic to prevent resizing is certain circumstances (file size, dimensions) is necessary because the pillow gif handling code seems to be rather flaky with regards to handling gif color palletes, causing broken gifs after resizing. The workaround is to only resize when absolutely necessary reducing the chance that gifs get broken.
@moagstar moagstar force-pushed the 10351-custom-gif-emoji-alpha-color branch from 45a5a94 to cc2e58c Compare January 29, 2019 10:36
@moagstar
Copy link
Collaborator Author

@timabbott Thanks for your review and link re commit messages. I have fixed the linter errors and failing test now.

@timabbott
Copy link
Sponsor Member

Nice, merged as 1e65cdd, after rewriting the commit message to be more readable to folks who didn't work on this issue. Thanks for fixing this @moagstar!! I think a lot of users will appreciate this :)

@timabbott timabbott closed this Jan 29, 2019
@sjuxax
Copy link

sjuxax commented Mar 24, 2019

@moagstar Is there a particular reason that you're resizing any non-square gif? Consider https://slackmojis.com/categories/7-party-parrot-emojis, in which many of the gifs are non-square, but otherwise meet the constraints fine.

@moagstar
Copy link
Collaborator Author

@sjuxax this was one of the requirements as discussed on github and chat.zulip.org

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants