feat!: Replace lilliput with govips

[KararTY]

Pull request checklist:

• CHANGELOG.md was updated, if applicable

Description

Old text

Preview: https://streamable.com/0lu2pe

There's a few pain-points with govips (If your distro has libvips below version 8.12 you will have to build from source) but it solves several issues that lilliput has:

• Advertises low memory usage, https://github.com/libvips/libvips/wiki/Speed-and-memory-use
• Animated WebP is supported. (See video)
• The output parameters actually work, which means one can reduce the output size considerably. (See video)

We can do custom outputs based on the input, which also means custom quality etc. (In video everything is exported as WebP with quality at 10%)

But, I'm just danking around. Consider this PR a proof-of-concept, maybe someone more can look into govips / vips and see how we could implement this.

For building libvips, I used the following: https://github.com/libvips/libvips/wiki/Build-for-Ubuntu

This PR brings support for animated image previews without lilliput by replacing it with a library (govips) that interfaces with libvips.

Why is this PR replacing lilliput? lilliput has several issues related to memory usage, which libvips resolves as well as issues with animated WebP that are yet to be supported even on lilliput's main contributor and user: Discord. libvips resolves both issues: See contents of "Old text" for more context.

Supported animated file formats with this PR:

• GIF
• WebP ( Fixes Explore if we can serve image thumbnail for WebP images with reasonable effort #259 )

Not (yet) supported animated file formats:

• Animated "image/avif" libvips/libvips/discussions/3048 - Couldn't get it to work on Windows or WSL/Ubuntu. See comment: feat!: Replace lilliput with govips #312 (comment)

All inputs are currently turned into WebP to potentially save on bandwidth. One can save more bandwidth by setting a quality limit on the WebP output, but for this PR I decided to leave this out: It can be its own PR with a new config entry.

Additionally, this PR introduces another breaking change by renaming enable-lilliput config entry to enable-animated-thumbnails.

Test links

Animated WebP https://7tv.app/emotes/604281c81ae70f000d47ffd9
Animated WebP https://mathiasbynens.be/demo/animated-webp-supported.webp
Static WebP https://7tv.app/emotes/617142c4ffc7244d797caa57
Static WebP https://www.gstatic.com/webp/gallery/5.webp
Animated GIF https://codelabs.developers.google.com/static/codelabs/avif/images/ice_qcif_15fps.gif
Animated GIF https://media.tenor.com/2hJTw8DEzqEAAAAM/forsen-forsen-e.gif

[codecov]

Codecov Report

Merging #312 (7be73e3) into master (688f0d3) will increase coverage by 0.40%.
The diff coverage is 9.75%.

@@            Coverage Diff             @@
##           master     #312      +/-   ##
==========================================
+ Coverage   45.35%   45.76%   +0.40%     
==========================================
  Files         101      100       -1     
  Lines        3684     3651      -33     
==========================================
  Hits         1671     1671              
+ Misses       1965     1932      -33     
  Partials       48       48              

Impacted Files	Coverage Δ
internal/resolvers/default/thumbnail_loader.go	34.42% <0.00%> (ø)
pkg/thumbnail/thumbnail.go	0.00% <0.00%> (ø)
internal/resolvers/default/link_resolver.go	42.92% <100.00%> (ø)
pkg/config/config.go	28.20% <100.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

[KararTY]

I've been unable to get animated avif to work, on Windows by using the suffix -all binary distribution or on WSL/Ubuntu by rebuilding libvips with libheif-dev.

Static avif files like this fox works:
https://raw.githubusercontent.com/link-u/avif-sample-images/master/fox.profile2.8bpc.yuv422.monochrome.odd-width.odd-height.avif

So does static 7TV emotes:
https://cdn.7tv.app/emote/60ae3e54259ac5a73e56a426/3x.avif

But not animated 7TV emotes. The error I get on Windows for animated .avif is [govips.info] failed to understand image format.

Now, 7TV does have .webp endpoints too: Their non-suffixed/non-file-format-ending version uses animated WebP which works with out of the box libvips on Ubuntu & Windows -static/-all distributions. And that preview is solved by this PR.

If anyone could test, or have resources on how to figure out a solution to support animated avif, it would be very helpful. Maybe in a separate PR.

Required changes for testing

diff --git a/pkg/thumbnail/thumbnail.go b/pkg/thumbnail/thumbnail.go
index d92504a..3126425 100644
--- a/pkg/thumbnail/thumbnail.go
+++ b/pkg/thumbnail/thumbnail.go
@@ -15,6 +15,7 @@ var (
 		"image/png",
 		"image/gif",
 		"image/webp",
+		"image/avif",
 		"application/pdf",
 	}
 
@@ -22,6 +23,7 @@ var (
 	animatedThumbnails = []string{
 		"image/gif",
 		"image/webp",
+		"image/avif",
 	}
 
 	cfg config.APIConfig
@@ -95,14 +97,14 @@ func BuildAnimatedThumbnail(inputBuf []byte, resp *http.Response) ([]byte, error
 	maxThumbnailSize := int(cfg.MaxThumbnailSize)
 	format := image.Format()
 
-	if image.Width() <= maxThumbnailSize && image.Height() <= maxThumbnailSize {
+	if image.Width() <= maxThumbnailSize && image.Height() <= maxThumbnailSize && format != vips.ImageTypeAVIF {
 		return inputBuf, nil
 	}
 
 	importParams := vips.NewImportParams()
 
 	// n=-1 is used for animated images to make sure to get all frames and not just the first one.
-	if format == vips.ImageTypeGIF || format == vips.ImageTypeWEBP {
+	if format == vips.ImageTypeGIF || format == vips.ImageTypeWEBP || format == vips.ImageTypeAVIF {
 		importParams.NumPages.Set(-1)
 	}
 

Edit: libvips/libvips#3048 (reply in thread) May be related? I'm unsure how to verify this limitation.

[leon-richardt]

Should we maybe continue supporting the enable-lilliput parameter as an alias for enable-animated-thumbnails and print a deprecation warning when it's used? That way, this needn't be a breaking change.

[KararTY]

Should we maybe continue supporting the enable-lilliput parameter as an alias for enable-animated-thumbnails and print a deprecation warning when it's used? That way, this needn't be a breaking change.

I was thinking about making that change a separate PR, but since the switch to libvips is a breaking change and that feature hasn't landed in a released version of chatterino/api I thought it wouldn't really matter how many breaking changes you shove in.
😂 👌 . o O ()

I like the idea of a deprecation notice, however.

Smol rant: Why are the configs named the way they are? It's called "enableLilliput" yet it's enabled by default. How does other software name their configs, especially when they're on by default? Is that even a distinction that they do? What's the deal with ...

[leon-richardt]

Should we maybe continue supporting the enable-lilliput parameter as an alias for enable-animated-thumbnails and print a deprecation warning when it's used? That way, this needn't be a breaking change.

I was thinking about making that change a separate PR, but since the switch to libvips is a breaking change and that feature hasn't landed in a released version of chatterino/api I thought it wouldn't really matter how many breaking changes you shove in. 😂 👌

Good point. I agree now that the enable-lilliput should not be an alias.

I like the idea of a deprecation notice, however.

I would even argue that we should error out if the enable-lilliput parameter is used in the config. For example, assume the following scenario:

1. User has enable-lilliput: false in his config to disable the generation of animated thumbnails.
2. User upgrades their version to the new release which uses enable-animated-thumbnails instead.
3. Since enable-animated-thumbnails is true by default, animated thumbnails will now be generated even though the user has not made any changes to his config.

By erroring out with a descriptive message when the old parameter is detected, we can make sure users are not running into unexpected behavior when upgrading.

[leon-richardt]

Other than changing the logging method to what's used elsewhere, I think this looks good 👍

[KararTY]

Other than changing the logging method to what's used elsewhere, I think this looks good 👍

Wouldn't this be a change for BuildStaticThumbnail() too? If so, maybe best if done in separate PR?

Also in a few more places https://github.com/Chatterino/api/search?q=fmt.Errorf & https://github.com/Chatterino/api/search?q=fmt.Println

[leon-richardt]

Other than changing the logging method to what's used elsewhere, I think this looks good +1

Wouldn't this be a change for BuildStaticThumbnail() too? If so, maybe best if done in separate PR?

Yes, I should have done that in BuildStaticThumbnail() as well. But I still think that this PR should include the change for BuildAnimatedthumbnail()