DEV: Remove gifsicle dependency (PR #10357)

GitHub

This pull request has been mentioned on Discourse Meta. There might be relevant details there:

https://meta.discourse.org/t/uploaded-animated-gifs-of-4mb-or-larger-appear-very-small/147107/25

We should add a PSA to the announcements when we merge this.

This is ready now. FastImage v2.2.0 contains my additions to the gem.

Should we catch any exceptions here?

I think you meant file here?

            file = Discourse.store.download(user.uploaded_avatar)

Should we catch any exceptions here?

I do not think so. What exceptions do you have in mind?

FastImage should not throw (there is an option for that, disabled by default) and if AR throws then there is something more serious going on and we should see that in logs. If a onceoff job throws, it seems that it is repeated until it succeeds, which is desired behavior.

Yes, you are right. :woman_facepalming:t2:

Same as above.

LGTM :+1:

Should definitely come with a PSA on meta as well :wink:

I simplified the logic for rebaking animated GIF images and moved it to a scheduled job (it was a once-off job before). The reason for this change is to split the load of downloading and parsing every GIF images over a longer time period. Also, it no longer recreates uploads as a simple recreation of the optimized images is enough (these are generally used).

Should we use .where("extension = 'gif' || original_filename like '%.gif' ") … gifs can sneak in with the wrong filename we rely on actual content I think.

It can be null though in some cases

I recommend just instantiating the schedule here. no need to duplicate this code.

hmmm if we are not using “animated” anywhere why bother backfilling now? I think once we actually use this column it makes sense to have a back filler … but, is there any point doing this early?

That sounds like a good idea. :+1:

I removed the backfilling Rake task completely. We do not need one now to force the process and we already have the scheduled job anyway…

Shouldn’t that be each instead of find?