Hi there guys, I just wondered why GIFs are so small when you upload them - even if you choose 100% as the size E.g
6 Likes
What is the size of the GIF you are uploading? The one below is 245x250 and uploads fine.
4 Likes
This looks like an error in speficically what you are choosing to upload? Can you describe it exactly, step by step, with examples?
Here is a gif that triggers the issue. When I look at its properties on my computer, its type is set to āimage/gifā. Its size is 10.4MB. It seems that Discourse is resizing its images. I am able to upload smaller gifs that I created in the same way without any display issues.
6 Likes
So this is about GIFs that are normally too large to fit the upload limit, they are being auto-resized somehow? That resulting image is tiny:
50 x 29, 32kb
Perhaps this is a regression @sam? 
6 Likes
Possibly a regression, does this happen with non animated gifs as well?
3 Likes
Thanks guys - Iām using http://giffox.com/ to make gifs.
Sometimes it works and sometimes notā¦ I think you might be right about the size. Was just doing some tests
Can make it work if itās very short.
1 Like
Here is a 2.06 mb GIF
Here is a 3.47 mb GIF
Here is a 9.06 mb GIF
Yeah looks like something very bad is happening at the 4mb limit to these @sam @eviltrout. I edited the title to reflect the issue. We got to fix this, weāve regressed.
6 Likes
Just wanted to confirm this is isolated to animated gifs.
I think we use Gifsicle: Command-Line Animated GIFs for resizing do my guess is that params changed
@vinothkannans can you have a quick look today
7 Likes
https://github.com/discourse/discourse/commit/b57d4586d713102ce939fd8d17f2ce200f290171
Itās happening since we changed the way of image downsizing. The above commit will fix this issue and the larger gifs will scale down properly.
8 Likes
Is this the correct fix? I donāt think we should be ādownsizingā what are effectively videos? That feels like a rather extreme (and possibly very server CPU intensive) manipulation, that could ruin video (gif) quality.
Iām not sure about this. I think we should outright reject animated GIFs which are too large rather than shoehorning ourselves into the video resizing (or even better, convert to mp4) business, that is a whole other problem to attackā¦
7 Likes
Yes, it looks like a straight forward fix. I will change the current functionality.
Anyway, it looks like weāre resizing the GIF images for the last 5 years using āgifsicleā and we didnāt receive any performance issue on it. @zogstrip may know more details.
https://github.com/discourse/discourse/commit/d456460d33859a3f6890062083ace6f64aa17266
7 Likes
I think in this case, rejecting very specific gifs is going to add more code to our repo.
At the moment we just assume all gifs are āanimatedā per:
If we are going to change it so some gifs are rejected we are going to have to do format detection for animated gifs and then drive some special rejection code. This is especially complicated for images we have to optimize (due to dimensions) cause we will no longer be able to optimize animated gifs.
On the upside we get to get rid of the gifsicle dependency. On the downside our upload pipeline becomes more complex.
Personally I think @vinothkannans fix is fine and low risk, it is code we have had for 5 years and there are no huge perf issues I know about.
That said @codinghorror I would not wrestle here if you want to nuke support for animated gif resizing but it is a fairly complex change, identification is easy enough, but driving the rejection with an error message may be a bit tricky.
Your animated gif is larger than 10mb so it can not be uploaded
Your animated gif is larger than 600x400 so it can not be uploaded
Anyway your call here. My call is just to leave the fix as is and move on but if you want to kill support let us know.
Note, this removal will also require we remove support for animated avatars (an optional feature, I am not super happy we have to be honest)
2 Likes
Hmm, so as long as the animated GIF fits in the total upload size bucket (??), it effectively gets its ā¦ āvideoā quality reduced to matchā¦ what?
Iām super unclear what is actually happening here.
Obviously resizing height and width is totally inappropriate for a GIFā¦ thatās what led to the original bug above?
2 Likes
The history was that we were ādestroyingā animated gifs when we were optimizing so if you uploaded images that was to be lightboxed we would butcher it.
Internals of our code had no ādetectionā logic to figure out if a GIF was animated or not. The lightboxing happens after the post is committed to the database.
The fix was to make our āresizeā code not butcher animated gifs and work properly with them.
The problem is that we need to redo chunks of our upload pipeline to give āspecial handlingā for animated gifs if we are stripping support.
- Gif is uploaded
- Is it an animated gif?
- Is the pre-processor going to need to resize it?
- reject animated gif
- is the post processor going to need to optimize it?
- reject animated gif
- Is the pre-processor going to need to resize it?
- Is it not an animated gif?
- let it through with normal pipeline
- Is it an animated gif?
There are also some side effects to consider with long term rebaking of markdown if params ever change and pulling hotlinked images.
The fix of simply fixing resizing meant that we did not need to worry about this stuff.
3 Likes
I donāt feel we should be doing āoptimizationā on animated gifs at all, so I think the code needs to detect this. Adding that path would open the door for eventual gif to mp4 video conversion which we want anywayā¦ eventually. So I feel this is necessary work.
2 Likes
Just to clarify.
You would like us
- To nuke
gifsicleout of our docker image - When you rebake old posts that had resized animated gifs, they should show up broken image glyph in the post
- Fix our pull hotlinked images task, to bypass pulling gifs that are animated that required a resize, displaying a link instead
- Reject animated gifs that are either too big (dimension or size) and are animated
- Remove support for animated avatars from Discourse.
- Keep track of animated vs not animated in the uploads table.
We are looking at about 1-2 weeks of fiddly work here, just confirming you want us to take it, especially since stuff is working right now.
2 Likes
Sure, less dependencies is better, isnāt it?
Probably OK
Simply check remote file size. If itās too big, donāt pull. I do not care otherwise.
Yes, mostly file size will be an issue Iād expect, long before height and width dimensions really matter. (In sing-song Twilight Zone āanother dimensionā¦ the dimension of tiiiiimeā) This is also why itās so frickinā weird to treat an image like a video. They are rather different animals!
Yes, nobody sane wants this.
We need this code path because we should eventually auto-convert GIFs to MP4s anyway. This gets us part of the way there and thatās a net positive to the world.
2 Likes