Uploaded animated GIFs of 4MB or larger appear very small

Support
1

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

2020-04-07 10.18.45

6 Likes
2

What is the size of the GIF you are uploading? The one below is 245x250 and uploads fine.

babies12

4 Likes
3

This looks like an error in speficically what you are choosing to upload? Can you describe it exactly, step by step, with examples?

4

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.

featured_topic

6 Likes
5

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? :thinking:

6 Likes
6

Possibly a regression, does this happen with non animated gifs as well?

3 Likes
7

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
8

Here is a 2.06 mb GIF

giphy
giphy720Ɨ404 2.06 MB

Here is a 3.47 mb GIF

89952-master-strokes-beard-gif-Kill-bdei

Here is a 9.06 mb GIF

Ton-Sur-Ton-directed-by-REGAN-CAMERON-1080p

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
9

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
11

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.

five
five360Ɨ640 1.05 MB

8 Likes
12

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
13

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.

7 Likes
14

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
15

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
16

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 it not an animated gif?
      • let it through with normal pipeline

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
17

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
18

Just to clarify.

You would like us

  1. To nuke gifsicle out of our docker image
  2. When you rebake old posts that had resized animated gifs, they should show up broken image glyph in the post
  3. Fix our pull hotlinked images task, to bypass pulling gifs that are animated that required a resize, displaying a link instead
  4. Reject animated gifs that are either too big (dimension or size) and are animated
  5. Remove support for animated avatars from Discourse.
  6. 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
19

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
25

I removed the dependency of gifsicle in:

https://github.com/discourse/discourse/pull/10357

5 Likes