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

Tracking issue: Remove libwebp dependency #1984

Closed
fintelia opened this issue Aug 20, 2023 · 22 comments
Closed

Tracking issue: Remove libwebp dependency #1984

fintelia opened this issue Aug 20, 2023 · 22 comments

Comments

@fintelia
Copy link
Contributor

fintelia commented Aug 20, 2023

Now that a pure-Rust encoder implementation is being added in #1978, we're planning on removing usage of libwebp which is written in C.

Our encoder is very fast, but lacks support for writing lossy files and the output files it produces generally aren't quite as small. If you are relying on one of those features, please comment on this issue to explain your use case!

Note: regardless of changes to the image crate, the webp crate will continue to provide Rust bindings to libwebp. Switching should only be a couple lines of code:

fn encode_lossy_webp(img: DynamicImage, quality: f32) -> Vec<u8> {
    let img = img.to_rgba8();
    let (width, height) = img.dimensions();
    webp::Encoder::new(&*img, webp::PixelLayout::Rgba, width, height)
        .encode(quality)
        .to_vec()
}
@djc
Copy link
Contributor

djc commented Jan 15, 2024

We (as the original contributors of WebP encoding support) would be disappointed to see a removal of lossy WebP encoding support. We're using lossy WebP support exactly because we feel it provides a good trade-off between image size and quality, whereas lossless WebP images do not seem to do as well on that particular trade-off. Maybe the C-based WebP encoding support could be retained for lossy encoding only?

@randomairborne
Copy link

I was also using lossy webp encoding, for the smaller encoding size compared to JPEG.

@fintelia
Copy link
Contributor Author

Just added a code sample to the original post showing how to use the do lossy encoding by calling into the webp crate. I'd also be curious to hear whether anyone has experimented with AVIF and/or mozjpeg? Sources online claim that those options achieve as good or better size and quality to WebP

@djc
Copy link
Contributor

djc commented Jan 15, 2024

We were looking at AVIF earlier, but it looks like browser support might not be quite where we'd want it to be.

https://caniuse.com/avif

@fintelia
Copy link
Contributor Author

Oh, thanks for sharing that! Looks like the last holdout is Edge, which is getting support in version 121 and should be released in about two weeks (then it's just however long it takes for people to upgrade from older versions)

@omid
Copy link

omid commented Jan 19, 2024

We also use lossy webp. Same reason as others said.

@bakhtiyarneyman
Copy link

We use lossy webp because it allows pages to load much faster.

@timleslie
Copy link

We use lossy webp as a carefully considered tradeoff between image size and image quality (in a context where this trade-off is of significant financial importance).

@schmitch
Copy link

we also use the lossy encoding since we create preview pictures from pdf files and we do not need high image quality and more like "good enough" quality with a small size

@fintelia
Copy link
Contributor Author

Thanks for the responses! It would be helpful to know whether the lossy encoding must be part of the image crate? If we go ahead with the planned removal, is there a reason the code snippet above would not be sufficient?

@randomairborne
Copy link

I think it would confuse people to have a partial implementation in image-rs. Not sure what others might think, though.

@Shnatsel
Copy link
Contributor

I see two downsides to having the C libwebp around:

  1. The risk of memory safety vulnerabilities due to inherently unsafe C code
  2. More complicated build process (incl. cross-compilation)

I am not particularly concerned about CVEs in the encoding step. Historically it has proven much easier to get right than decoding. The hassle of dealing with C dependencies, however, is very real, and bites you before you even touch WebP - it blocks access to installing the image crate in general.

I think hiding libwebp dependency behind a non-default feature flag is sufficient. I figure exposing the lossy encoding interface through image might make it a little more convenient, but then there's the question of how to expose two different lossless encoders in a way that isn't a footgun.

@randomairborne
Copy link

randomairborne commented Feb 19, 2024

Unfortunately, as long as rust libwebp is incomplete, keeping libwebp around is kind of unavoidable. It at the very least needs to be documented that the rust encode function is always lossless.

@neckaros
Copy link

neckaros commented Mar 2, 2024

Just added a code sample to the original post showing how to use the do lossy encoding by calling into the webp crate. I'd also be curious to hear whether anyone has experimented with AVIF and/or mozjpeg? Sources online claim that those options achieve as good or better size and quality to WebP

Doesn't AVIF also depends on C library libdav1d?

@fintelia
Copy link
Contributor Author

fintelia commented Mar 5, 2024

Doesn't AVIF also depends on C library libdav1d?

AVIF encoding uses the Rust-based ravif library. Decoding does however require libdav1d.

@fintelia
Copy link
Contributor Author

I'm going to go ahead and close this now that 0.25.0 is released.

There is a place in the Rust ecosystem for a crate that simply provides safe wrappers around the C/C++ reference implementations of all the image formats. However, that has never been the goal of this crate or the broader image-rs org.

From the discussion, it sounds like a pure-Rust lossy WebP implementation wouldn't actually be a suitable replacement in current use cases unless it were able to achieve compression ratios on-par or better than libwebp. But perfect feature parity isn't the criteria we use for other codecs, and I don't think it can be the standard we use to decide when to drop a native dependency either

@randomairborne
Copy link

I would argue that we aren't really looking for "perfect feature parity", just something that allows the format to work as advertised- which is with high compression ratios.

@fintelia
Copy link
Contributor Author

Also for the record, this sort of engagement is not OK:

[your crate is only a] partial implementation

rust libwebp is incomplete

[your crate crate doesn't] work as advertised

Technical disagreements are fine. But repeatedly putting down the work of org members to bully us into reverting a change crosses a line.

@schmitch
Copy link

@fintelia well to be fair:

[your crate is only a] partial implementation

is actually the correct term form the webp handling, even if its a little bit harsh.
Actually I know that removing code is always a cool thing, since it removes an enormous maintance burden.

But in case of the webp encoder at least provide one of the two things:

  • provide documentation how to read the webp via libweb and put it into an image-rs object:
    fn encode_lossy_webp(img: DynamicImage, quality: f32) -> Vec<u8> {
     let img = img.to_rgba8();
     let (width, height) = img.dimensions();
     webp::Encoder::new(&*img, webp::PixelLayout::Rgba, width, height)
         .encode(quality)
         .to_vec()
    }
    
    this snippet is fine and provide a way to put the vec into image-rs, but it will probably be lost in time if it is not inside the README.md or a file inside docs/ or put it inside a code comment, so that it will be inside doc-rs
  • another crate that could at least be put into another repo at: https://github.com/image-rs

btw. why you get so many complaints is because of that: your library is one of the best (if not the best) image libraries in the wild and is loved by many. you will probably more often find yourself in a tight spot, but we love this library regardless

@Shnatsel
Copy link
Contributor

provide documentation how to read the webp via libweb

The WebP decoder is feature-complete and the decoding result is bit-identical to libwebp. I don't see why we wouldn't have people just use it directly.

I agree the limitations of the encoder should be documented. The doc comments right now don't mention any of the limitations, and that can easily be improved. We could even link to the example code showing how to encode WebP with libwebp.

@Shnatsel
Copy link
Contributor

I've opened #2314 to document the trade-offs involved more clearly, and link to example code for encoding with libwebp.

@fintelia
Copy link
Contributor Author

is actually the correct term form the webp handling, even if its a little bit harsh.

To be clear, that's how this sort of bullying works. Start with "partial implementation" which is harsh but factual. Then escalate to "incomplete", which isn't really true (all planned functionality had been implemented), but isn't worth arguing about. Also get the library name wrong in the same comment. Then when neither of those provoked the desired response, escalate again with the point about just wanting the encoder to "work as advertised". This sort of thing is extremely common in open source, which makes it really easy to go unnoticed. But it doesn't have to be something that's tolerated

@image-rs image-rs locked as off-topic and limited conversation to collaborators Aug 26, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

9 participants