Building the avif feature without asm

[foresterre]

AVIF encoding support has been added to image, and can be enabled by using the feature flag avif. The encoder uses ravif under the hood which is a crate which provides AVIF image encoding, which is in turn based on rav1e, an AVIF video encoder written in Rust (and optionally assembly).

Recently, ravif enabled the rav1e asm feature by default (since ravif 0.6.3). This is great for performance improvements, but requires nasm to be installed. I've run into many issues with installing and running nasm on the CI. As a result I would like to disable the default features of ravif, so my build no longer depends on nasm.

Unfortunately, to the best of my knowledge, a crate can not enable features for a dependency deeper than one level. This would mean that if I want to use the latest version of ravif and subsequently rav1e, without the asm feature, the image crate would need to set the ravif to build without default features.

I can imagine that in some cases building with the asm feature is preferential, so I would like to discuss the possibility of adding a feature like avif_no_asm which depends on ravif without default features enabled.

What do you think? :smiley:

Draft

I think the following changes would need to be made to the manifest file:

ravif = { version = "0.6.0", optional = true, default-features = false }

# features

[features]
avif = ["ravif/asm", "rgb"]
avif_no_asm = ["ravif", "rgb"]

The complete manifest can be found at: https://github.com/foresterre/image/blob/ravif-no-asm/Cargo.toml#L34

[fintelia]

Unfortunately, to the best of my knowledge, a crate can not enable features for a dependency deeper than one level.

There is actually a workaround for this: since Cargo uses the union of all requested features from all places in the tree that depend on a crate, we could stop asking for the asm feature and you'd still be able to depend on ravif another time and request the asm feature there.

[est31]

rav1e is extremely slow without the asm feature. With that I mean extremely slow. I fear that if it were made the default, people would turn away from using avif entirely. At most, it should be controlled via a default on feature of the image-rs crate.

[foresterre]

rav1e is extremely slow without the asm feature. With that I mean extremely slow. I fear that if it were made the default, people would turn away from using avif entirely. At most, it should be controlled via a default on feature of the image-rs crate.

I also prefer to use the asm feature where possible, because it definitely provides a significant speed up.

Having it on by default however does mean that the clever work around mentioned by @fintelia would probably no longer work, since that would require being able to subtract features, which is not possible since features are additive.

In addition, this will require a modern version of nasm to be installed (~~for example the version in the Ubuntu 20.04 APT repo is too old~~ @barrbrain corrected me on this below). As long as AVIF is opt-in anyways I don't think that's a problem, but if it becomes a default feature (which may be the case for the next major release as discussed in #1398), it means that this crate will have build dependencies beyond just a Rust toolchain.

[barrbrain]

In addition, this will require a modern version of nasm to be installed (for example the version in the Ubuntu 20.04 APT repo is too old). As long as AVIF is opt-in anyways I don't think that's a problem, but if it becomes a default feature (which may be the case for the next major release as discussed in #1398), it means that this crate will have build dependencies beyond just a Rust toolchain.

I am writing to clarify the current state of the rav1e nasm dependency. Details of package nasm in focal shows that nasm 2.14.02 is available in Ubuntu 20.04. As at rav1e v0.4.0, nasm 2.14.02 or newer is required to build with the asm feature on x86_64.

[johannesvollmer]

To be honest, I think it's a huge regression in crate usability to suddenly require users to install some random dependency by hand. There shouldn't be any additional steps required other than adding the dependency name in the cargo.toml file. It's literally one of the selling points of Rust and Cargo. We go to great lengths in order to make a library pure Rust for this reason.

People already use this crate a lot, and when a major version suddenly requires installing another library by hand, and dealing with system environment variables or whatever, users will simply revert to the previous version. The error message is not even very helpful, it just says: nasm not found. Some users might not even realize that the ravif dependency clould simply be disabled to make their Project compile again.

The ravif library offers prebuilt binaries without any dependencies. Why is this only possible for prebuilt libraries, but not for the Rust crate? Can't the Rust crate specify prebuilt binaries?

[johannesvollmer]

According to the author, it won't be feasible to build NASM on compile time, as building it requires even more dependencies to be installed. Therefore, it can't be avoided to install NASM by hand if you enable the nasm feature in ravif.

This is perfectly fine for the ravif crate, but given that avif is only one of the many codecs in image-rs, I suggest we should not require this manual installation step by default for every user.

From my perspective, the only question left is: Do we want the users to observe suboptimal performance unless they change a flag, or do we want them to explicitly activate the whole avif feature before using it, never without nasm? Or is it possible to include the prebuilt NASM DLL/A files?

[SuperCuber]

The error message is not even very helpful

I'll add that the error message for when the feature isn't enabled isn't very helpful either: The image format Avif is not supported should probably be something like The feature "avif" is not enabled

Do we want the users to observe suboptimal performance unless they change a flag

I think this could be an acceptable solution if there's good enough documentation for it - maybe in this table in the "encoding" cell add a footnote mentioning to enable the feature for a performance boost

[FrankenApps]

I think it would also be fine if both options can be enabled and if none are enabled by default (+ the improved error message as mentioned by @SuperCuber). However in either case I am in favor of adding the avif_no_asm feature.

[ealmloff]

This appears to be fixed by https://github.com/image-rs/image/pull/1976 and published in image 0.25

[fintelia]

#2178 added a disabled-by-default feature flag to use nasm, without having to go through any hurdles to enable it directly on the rav1e crate