✨ Support AVIF image format

[smldub]

• Avif from Image package added to GenericImageProcessor
• Some tests added to image/generic.rs (however they are all failing)
• Docker image works! and is able to serve AVIF images (in zip and rar packages) somehow...

[aaronleopold]

Some tests added to image/generic.rs (however they are all failing)

If you would like me to take a look, just let me know! Otherwise, I'll assume this PR is good to peek at once the draft status is removed

[smldub]

Just want to give a progress update so you don't think I've disappeared. After doing some research, I have found that

1. Image uses ravif for its encoder which is what cavif is based on. Unfortunately, I believe ravif is only an encoder, so it cannot decode or convert from avif to other file formats. The maintainer of this project, avif-decode but this has dav1d as a dependency (hidden in its avif-parse dependency).
2. Image uses libdav1d for its decode which requires the feature "avif-native" to be enabled (as you pointed out).
3. I have found rav1d, which looks like a well supported rust dav1d library. But I don't see any higher-level interfaces using it yet. Image appears to be discussing it

In terms of some paths forwards I've come up with,
a. You could add dav1d as a dependency, and then the image package should just work with feature enabled (it does on my end)
b. I could look more at rav1d, and see if it is possible to incorporate it.
I'll start looking at b as an option, but let me know what you think.

[aaronleopold]

Hey no worries, life is busy!

I appreciate the time you've put into researching this, I think the best option would be the one with the least amount of friction. Based on what you've linked, it feels like just opting into that avif-native feature might be the best bet. It is easy enough to make sure libdav1d-dev is added to the base docker image, and if folks running bare metal (more accessible after the upcoming release) want AVIF it can be recorded somewhere that the library is potentially required

[smldub]

When implementing the avif type I had to strip alpha information from the image to make it into a jpeg. The way I did it always returns a DynamicImage::ImageRgb8, I don't know if JPEGs are ever any other type of image (luma or 16bit) but I could in theory write a more robust alpha stripping function if that is the case. Let me know what you think!

[aaronleopold]

When implementing the avif type I had to strip alpha information from the image to make it into a jpeg

Noted. I think this is likely okay, but I am not an image expert. I'll keep it in mind when I have a moment to review your changes. Thank you!

[aaronleopold]

Since dav1d is required at compile time, we probably need to update the workflows to ensure the runner has the proper installation process. The dav1d-sys crate docs point us to this: https://github.com/rust-av/dav1d-rs/blob/master/.github/workflows/dav1d.yml

[smldub]

Not really familiar at all with CI tools, but hopefully this is a step in the right direction!

[smldub]

Jpeg supports rgb8 and luma8. I don't know how important it is for all of the images types to interconvert, but right now the code will error out if someone attempts to convert a PNG with rgb16 to a Jpeg. It looks like the image Avif encoder handles this by converting all 16bit info into 8bit, we could do a similar thing, or (when its added) convert to jpg-xl.

[aaronleopold]

I think this looks great! I only have a few comments/questions

As a side note, and I don't think it has to be done in this PR, but dav1d probably needs to be added to the system-setup.sh script too at some point

[smldub]

As a side note, and I don't think it has to be done in this PR, but dav1d probably needs to be added to the system-setup.sh script too at some point

Would this addition be a check for having Dav1d installed like the cargo and node check?

[aaronleopold]

As a side note, and I don't think it has to be done in this PR, but dav1d probably needs to be added to the system-setup.sh script too at some point

Would this addition be a check for having Dav1d installed like the cargo and node check?

I think it should just be installed if possible, using brew for macOS in that script and just appending whatever the dep is for the linux distro.

For Windows, I'm assuming installation is more complicated. So perhaps just a check in that ps1 script I could have sworn there was a system-setup.ps1, but I don't see one lol. You can just omit it for Windows then

[smldub]

whatever the dep is for the linux distro.

For arch it should be simple to add, but ubuntu/debian, as far as I know, ships with a version of Dav1d that is too old to work with the dav1d-sys crate. So we would have to add an installation script.

[aaronleopold]

whatever the dep is for the linux distro.

For arch it should be simple to add, but ubuntu/debian, as far as I know, ships with a version of Dav1d that is too old to work with the dav1d-sys crate. So we would have to add an installation script.

Is installation more complicated than just adding it to the existing apt-get -y install command? I don't think there is an --no-upgrade arg last I checked. Not really an issue if it is more complex, you have the entire if block to work with as needed per platform

[aaronleopold]

All of these run clauses need a shell added:

• bash for linux
• powershell for windows

[smldub]

Is installation more complicated than just adding it to the existing apt-get -y install command?

I've had to compile it from source on debian systems to get it to work (to get an new enough version), which is what I do in the docker and github actions. Potentially one could add the sid debian repo and pull their dav1d package, but I didn't test if that one worked.

In the setup-script you assume they have node and rust preinstalled, because these are often installed outside of package managers, so because this requires compiling and installing maybe just leaving that up to the user as well.

entire if block to work with as needed per platform

but it shouldn't be that hard to add (it will just require installing ninja and meson for building)

[aaronleopold]

I've had to compile it from source on debian systems to get it to work (to get a new enough version), which is what I do in the docker and github actions. Potentially one could add the sid debian repo and pull their dav1d package, but I didn't test if that one worked.

In the setup-script you assume they have node and rust preinstalled, because these are often installed outside of package managers, so because this requires compiling and installing maybe just leaving that up to the user as well.

That makes sense. I think if it's a one-liner addition to install it (arch and macOS?) then we should add it, otherwise we can just do a check for its existence. You're probably correct in that if it's more complicated to install we can let folks decide how they prefer to do it

[codecov]

Codecov Report

Attention: Patch coverage is 98.25436% with 7 lines in your changes missing coverage. Please review.

Files	Patch %	Lines
core/src/filesystem/content_type.rs	16.66%	5 Missing ⚠️
core/src/filesystem/image/generic.rs	99.37%	2 Missing ⚠️

Files	Coverage Δ
core/src/filesystem/common.rs	52.56% <ø> (ø)
core/src/filesystem/image/mod.rs	100.00% <100.00%> (ø)
core/src/filesystem/image/process.rs	87.65% <100.00%> (+0.55%)	⬆️
core/src/filesystem/image/webp.rs	100.00% <100.00%> (ø)
core/src/filesystem/image/generic.rs	99.63% <99.37%> (-0.37%)	⬇️
core/src/filesystem/content_type.rs	37.70% <16.66%> (-0.72%)	⬇️

[aaronleopold]

CI passed 🎉

Your feature branch needs to be updated, but otherwise I think this is ready to go. Thanks for all your work to get this in!

[smldub]

Thanks for walking me though the process! Maybe as a note or I could add now, probably some documentation should be added to contributor docs.

something like

You need to install yarn, rust, dav1d and node.

[aaronleopold]

Maybe as a note or I could add now, probably some documentation should be added to contributor docs.

something like

You need to install yarn, rust, dav1d and node.

Yeah makes sense, you're welcome to go for it! I won't merge this PR until the current nightly build finishes anyways