fix: minimal build with no default features

[roderickvd]

This commit makes the decoder module and its exports conditional on the having at least one decoder format enabled. Also changes the CI to prevent regression.

Fixes #730

[roderickvd]

I thought about creating a transitive "decoder" feature instead of duplicating the "symphonia, wav, flac, etc." list. Did not do it because it may confuse users. Edit: see below.

[roderickvd]

Argh. Thought about a quick fix, but the rabbit hole goes deeper. Compiling with just --no-default-features --features playback also gives a whole slew of non-trivial errors. There are a lot of expectations that there is at least one decoder today.

I see two options:

1. Do create a transitive "decoder" feature, and gate everything behind it.
2. Accept that Rodio needs at least one decoder to compile correctly.

[dvdsk]

I want to draw your attention to this: #539

the tldr: compile features are a mess ATM and offer a lot of avenues for issues.

I say if we start changing (and breaking some code that way) lets fix things properly and overhaul the entire feature set.

[roderickvd]

Sure, I understand. What would you like to do for v0.21?

Remove this “decoder” feature for now, until we sort everything out, and just duplicate the #[cfg(any(…)] everywhere?

Or: accept that compiling without default features is broken? I’d rather not because it breaks projects that don’t use any part of the decoder.

[dvdsk]

There is no rush to release v0.21 right? I can try and implement the changes proposed in #539 and then combine that with the transitive decoder feature you proposed. Then the theme for v0.21 will be: 'removing footguns'. Which I kinda like.

[roderickvd]

Fine for me - I thought we were in a release train but totally fine. Just to check: so you’re taking over on that feature issue? Let me know if and what you’d like me to contribute.

[dvdsk]

Fine for me - I thought we were in a release train but totally fine.

Yes your right there. This could be an easy change however, and if we do it now we probably no longer need to make breaking changes to features in the future.

so you’re taking over on that feature issue? Let me know if and what you’d like me to contribute.

I can allocate some time to take on #539. It seems like its own issue to me, on the other hand maybe changing the features should be one PR. I have no idea how to address it (539) yet. If you have ideas please, feel free to take on #539 and merge that with this :).

[dvdsk]

Taken from 539:

1. Document how enabled features determines the decoder used by rodio. Using different decoders for different streams in the same container will only be possible when specifying enabled codec and containers separately. As an example, it will be possible to configure rodio such that it uses lewton for ogg containing a vorbis stream while decoding ogg containing flac with symphonia.

2. Make the current container features (for example symphonia-isomp4) enable all codecs. This removes the pitfall of Reverb example rans into error with symphonia vorbis decoder  #516.

3. Add features that only enable a container without enabling any codecs to facilitate 1. and allow speeding up compilation.

Looking at it now the above seems overkill to me. Maybe we should have a feature for each container and none for codecs (they are all enabled when a container that can use them is enabled).

I say we first propose what we want the features to be before implementing them.

[roderickvd]

Looking at it now the above seems overkill to me. Maybe we should have a feature for each container and none for codecs (they are all enabled when a container that can use them is enabled).

Enabling all codecs for a container leads to the question: the Symphonia ones or the others? When both are enabled ("they are all enabled") then the implementation in v0.20 as well as v0.21 will give "the others" (hound, lewton, claxon) precedence over Symphonia.

Currently, default decoders are lewton, claxon, hound, and finally Symphonia only for MP3. Here's an opinion, I don't know how popular: how about changing the defaults all to Symphonia instead?

Pros: more functionality, more consistent without hybrid Symphonia / other decoder mix
Cons: some users may not want to have MPL licensed decoders. To me not really an issue if we still give them choice to switch to "the others".

All these variants should compile without warning then:

# defaults: playback,flac,vorbis,wav,mp3
# all decoders Symphonia
# "vorbis" will also enable "ogg"
cargo build 

# defaults plus aac
# "aac" will also enable "mp4"
cargo build --features aac

# whoever just wants to have the DSP modules
cargo build --no-default-features

# as a playback library with own sources
cargo build --no-default-features --features playback

# like the defaults but with some different Symphonia decoders
# "aac" will also enable "mp4"
# "vorbis" will also enable "ogg"
cargo build --no-default-features --features playback,vorbis,aac,aiff

# like the defaults but with alternative decoders
cargo build --no-default-features --features playback,claxon,lewton,hound,minimp3

# advanced usage: lots of stuff could be in a container
cargo build --no-default-features --features flac,mp4

This will probably still require an internal "decoder" feature flag.

Feel free to improve.

[dvdsk]

how about changing the defaults all to Symphonia instead?

agreed! symphonia has proven great. And as you say if the license proves an issue (which it should not) users can fall back. The current situation is also a small footgun since symphonia implements more features (mainly seek). Having those missing while having enabled a feature for your format is confusing.

I would also like to separate codecs and containers. Most users do not know the difference and that is yet another footgun. You enable ogg but you miss vorbis etc (not the best example). We could have three categories of codec/container features:

• container: enables all the codecs supported by that container
• container_without_codec: enables only that container
• codec: enables only that codec

Examples would be:

• wav: enables symphonia/wav, symphonia/pcm and symphonia/adpcm
• wav_without_codecs: enables only symphonia/wav. Useless without pcm or adpcm
• pcm, adpcm: both are useless without wav_without_codecs

Alternative and most user friendly would be to only have the container features.

[PetrGlad]

Check for no-default features was added to accommodate a no-playback build option that some users requested. In particular, with that option the whole output stream's code could (and should) be excluded.
Because Cargo does not support a "negative" feature specification, the playback feature was marked as default only to avoid surprises in the usual case when users do want playback. This way only users who actually want no playback would have to deal with more complicated dependency requiring "no-default-features" flag.
I do not mind supporting even slimmer version of rodio as long as it is either required for some use-case or does not complicate things.

If you feel like this option is needed, see my other comments here.

[renski-dev]

Argh. Thought about a quick fix, but the rabbit hole goes deeper. Compiling with just --no-default-features --features playback also gives a whole slew of non-trivial errors. There are a lot of expectations that there is at least one decoder today.

I see two options:

1. Do create a transitive "decoder" feature, and gate everything behind it.
2. Accept that Rodio needs at least one decoder to compile correctly.

Probably really late to this, but there is a third solution:

#[allow(clippy::large_enum_variant)]
enum DecoderImpl<R: Read + Seek> {
    #[cfg(all(feature = "wav", not(feature = "symphonia-wav")))]
    Wav(wav::WavDecoder<R>),
    #[cfg(all(feature = "vorbis", not(feature = "symphonia-vorbis")))]
    Vorbis(vorbis::VorbisDecoder<R>),
    #[cfg(all(feature = "flac", not(feature = "symphonia-flac")))]
    Flac(flac::FlacDecoder<R>),
    #[cfg(all(feature = "minimp3", not(feature = "symphonia-mp3")))]
    Mp3(mp3::Mp3Decoder<R>),
    #[cfg(feature = "symphonia")]
    Symphonia(symphonia::SymphoniaDecoder, PhantomData<R>),
    #[allow(dead_code)]
    None(Unreachable, PhantomData<R>),
}

enum Unreachable {}

This requires adding an unreachable branch to all match statements, but gets rid of all compile errors, leaving only warnings about unused items.

[roderickvd]

Not late at all! So that removes any need for an internal "decoder" feature right? Both are kinda hackish but yours seems the lesser of evils.

[renski-dev]

It removes the need for the intermediate feature flag. I would not call either hackish:

1. The feature flag removes the entire module, which can be a good thing as long as nothing on the outside couples too tightly to it.
2. Specializing enums or "deleting" enum variants of generic APIs is standard practice as well, see Infallible.
   • The only thing I personally dislike about the snippet I posted is the fact you need the PhantomData which makes the compiler mistakenly assume that the enum variant can be instantiated.

Of the two, I see it as a choice between:

1. Enforcing strongly decoupled decoder functionality, but introducing more combinations of feature flags we need to test against.
2. Allowing stronger coupling between the decoders and other modules (which can be desirable or undesirable), but reducing the number of build configurations.

Anything that results in the least breaking changes for users is probably best in the short term. In the long run however, the notion of supporting many independent decoding backends with a single generic interface (mostly the Settings and DecoderBuilder types) is a bit too optimistic in my opinion. So here comes the third and definitely most API-breaking approach:

• Use a trait (at the moment, the only candidate is Source) instead of an enum and provide implementations for common backends separately.
• This means using distinct types, e.g. SymphoniaDecoder for symphonia, each being feature gated.
• When types are not statically known or a "DefaultDecoder" is required, internally it would have to be something holding a dyn Source. The existing decoder API is essentially this, just implemented with an enum.

[renski-dev]

I will also add that if we consider the dyn SomeTrait option inside a generic decoder type, the fetching samples one-by-one approach that is currently used by Source is going to result in a performance hit. How much of a hit is difficult to say without trying it out and profiling, but processing small slices of samples instead would solve this trivially, as well as allow for more efficient implementations of filters and effects.

[dvdsk]

the fetching samples one-by-one approach that is currently used by Source

the "hope" is that the compiler will inline those calls, so having a consumer of Source that buffers should lead to the compiler unrolling 'some' loops.

How much of a hit is difficult to say without trying it out and profiling

Would be nice to see, you should know though there is a wider discussion regarding the Iterator approach vs alternatives. We are not ready to announce anything yet but the discussion is public. If your interested shoot me a mail and I'll send you a link (you'll find my email on davidsk.dev).

[renski-dev]

@dvdsk Thanks, sent you an e-mail.

[renski-dev]

I implemented a minimal best-case dyn Source test on this fork. I couldn't get the compiler to inline anything, as I suspected might be the case with dynamic dispatch. Looking at the LLVM IR output, it emits the vtables as expected, and judging by the execution time, LLVM doesn't do anything clever about it either.

[roderickvd]

Sorry, was occupied with other things before I can jump on this.

But indeed I’d vote to prevent going the dyn route. We should go out of our way and enable the compiler to do smart things.

I’m not sure what you mean with being too optimistic with the builder. I agree that it’s improbable that we’ll ever see more backends, let alone “lots more”. Honestly that also wasn’t the intent of the builder. It was there to expose some of Symphonia’s features, without tight coupling, and still allowing the other decoders to have their place.

If it were me as a user, I would dispense with the other decoders and go full Symphonia. But me as a Rodio collaborator, I want to allow for other uses to use differently licensed decoders.

[renski-dev]

But indeed I’d vote to prevent going the dyn route. We should go out of our way and enable the compiler to do smart things.

I agree completely, I only tested the trivial dyn setup to see how bad it would be at the moment.

The trouble is, I'm not certain that avoiding some kind of dynamic dispatch when fetching samples is always going to be possible. Any kind of dynamically-constructed (possibly end user-constructed, if we consider a DAW) graph is going to result in, for a standard headset setup, 44k*2 calls per second to next() at the very least.

So far, using rodio for my game engine, I've worked around this by creating my own "giga-source" containing an internal effects router where I buffer and process the streams in a very OpenAL-inspired way. Originally I started doing it this way not for performance reasons, but because implementing a pitch shifter with the Iterator interface ended up being a huge pain. Maybe it's a skill issue on my end, don't know.

[roderickvd]

My gut tells me we should go the buffer route even if we don't need dyn. Doing so, we could still offer a legacy Iterator implementation by just having next() pop the queue.

[roderickvd]

Probably really late to this, but there is a third solution:

#[allow(clippy::large_enum_variant)]
enum DecoderImpl<R: Read + Seek> {
    #[cfg(all(feature = "wav", not(feature = "symphonia-wav")))]
    Wav(wav::WavDecoder<R>),
    #[cfg(all(feature = "vorbis", not(feature = "symphonia-vorbis")))]
    Vorbis(vorbis::VorbisDecoder<R>),
    #[cfg(all(feature = "flac", not(feature = "symphonia-flac")))]
    Flac(flac::FlacDecoder<R>),
    #[cfg(all(feature = "minimp3", not(feature = "symphonia-mp3")))]
    Mp3(mp3::Mp3Decoder<R>),
    #[cfg(feature = "symphonia")]
    Symphonia(symphonia::SymphoniaDecoder, PhantomData<R>),
    #[allow(dead_code)]
    None(Unreachable, PhantomData<R>),
}

enum Unreachable {}

This requires adding an unreachable branch to all match statements, but gets rid of all compile errors, leaving only warnings about unused items.

Anyone against this for a v0.21 target? I like it best of the various options.

[dvdsk]

Anyone against this for a v0.21 target? I like it best of the various options.

I'm on board 👍

[renski-dev]

If dyn is necessary then we can consider using buffers? I remember that was discussed in part elsewhere here. In that case the processing could even be vectorized (inside a filter), but the whole implementation will be more complex (and complicated).

I find the compiler is quite good at vectorizing when it can guarantee contiguous data. Depends a lot on the task, but something common like averaging N channels down to mono gets vectorized out-of-the-box: https://godbolt.org/z/K5dcrTYbY.

Granted, I haven't checked when it does/doesn't do this on the current rodio codebase, but it's going to be difficult to guarantee or check which combinations of filters or kinds of graphs allow for vectorization.

So I support designing a buffer-based API in the long run. Also I think the DecoderImpl::None solution is fine for now in order to ship 0.21, as @roderickvd said.

Replaced my changes with cleaner proposal of @renski-dev

[roderickvd]

Just force pushed, replacing my previous take with the better proposal of @renski-dev.

[roderickvd]

@PetrGlad do you have any further review comments? If not, I'd like to get this merged. It seems we're good to release v0.21 after that!

[PetrGlad]

@roderickvd I am sorry for not answering earlier.
I can build with --no-default-features but the test build fail (mostly in examples)

cargo test --no-default-features

Do you intent to clean that up here or is there another plan for these?

[roderickvd]

@roderickvd I am sorry for not answering earlier.

No worries, I know how life goes. Thanks for responding now.

I can build with --no-default-features but the test build fail (mostly in examples)

cargo test --no-default-features

Do you intent to clean that up here or is there another plan for these?

I agree that an absolutely minimal build should compile and test without error or warning. It turned out to be a bit more work than just in examples/*.rs but I made it so in 0498f72.

[dvdsk]

I figured out a way to get the examples to work without needing conditional compile statements 🥳

we need to put this in Cargo.toml

[[example]]
name = "basic"
required-features = ["playback"]

Then is you run:

cargo r --no-default-features --example basic

you get a nice error from cargo:

error: target `basic` in package `rodio` requires the features: `playback`
Consider enabling them by passing, e.g., `--features="playback"`

[roderickvd]

Awesome I was looking for that. I'll refactor tomorrow or so.

[roderickvd]

I figured out a way to get the examples to work without needing conditional compile statements 🥳

This should do it.

[dvdsk]

We could handle these the same way as with the examples. I think this is fine though. Examples should be as simple as possible so not having conditional comp in there is nice. These tests are not meant to help users so having them look a little more daunting is fine.

[dvdsk]

thanks for all the hard work and patience everyone!