Proper way to abstract a third-party provider

Hi all!

I have a library that analyzes songs. I currently have a Song object, like so (I simplified things), that uses ffmpeg (a third-party audio decoder) to decode the song; my library then uses this decoded song to analyze it:

pub struct Song {
   pub title: Option<String>,
   pub analysis: Option<Analysis>,
   ...
}

impl Song {
    #[cfg(feature = "ffmpeg")]
    pub fn from_path<P: AsRef<Path>>(path: P) -> Result<Self> {
        let raw_song = Song::decode(path.as_ref())?;

        Ok(Song {
            title: raw_song.title,
            analysis: Song::analyze(&raw_song.sample_array)?,
            ...
        })
    }

    #[cfg(feature = "ffmpeg")]
    pub(crate) fn decode(path: &Path) -> BlissResult<InternalSong> {
         // ffmpeg-specific code
         ...
    }

    pub fn analyze(sample_array: &[f32]) -> Result<Analysis> {
       // library-agnostic code
       ...
    }

99% of my users will want to use that "ffmpeg" feature, because they don't care about how a song is decoded, they just want to go from song path to analyzed song.
However, it would be nice if the remaining 1% could opt out of the ffmpeg dependency, and decode songs the way they see fit.
I toyed with making something like this:

pub trait Decode {
    fn decode(path: &Path) -> BlissResult<InternalSong>;

    fn from_path<P: AsRef<Path>>(path: P) -> BlissResult<Song> {
        let raw_song = Song::decode(path.as_ref())?;

        Ok(Song {
            title: raw_song.title,
            analysis: Song::analyze(&raw_song.sample_array)?,
            ...
        })
    }
}

#[cfg(feature = "ffmpeg")]
impl Decode for Song {
    fn decode(path: &Path) -> BlissResult<InternalSong> {
         // ffmpeg-specific code
    }
}

However, I see at least two issues with this:

  • I have to make the InternalSong struct public, which I'm afraid will confuse users (should I use Song? InternalSong?). I have documentation, but I'm afraid this'll create more friction than needed
  • And most importantly, it means it's a breaking change for current users, which will have to start adding use crate::song::Decode everywhere they want to use Song::from_path (which is everytime they wanna use my library)

So all in all, I'm afraid of creating a lot more unnecessary friction for 99% users just so that 1% can enjoy a nice interface to implement their own decoding mechanism.

Is there any other way where we could get the best of both worlds? I.e. letting some users implement their own decode and have everything integrated with Song::from_path (other parts of the library use it down the line, so it'd be extra nice to simply implement decode and then everything else works from there), while retaining compatibility for the rest?
I'm thinking that maybe I'm missing some other nice Rust abstraction :slight_smile:

Cheers!

You could have the ffmpeg feature enabled by default, and allow that 1% of users to opt-out by disabling it.

Wouldn't it make more sense to have a Decoder trait and implement that for a type Ffmpeg which you could hide behind a feature gate? What if people want to support multiple decoders? With your interface they couldn't do that. It would also decouple your Song from the decoding functionality, allowing you to better hide the grimy details of decoding in your public API.

As to your two points (I) having an InternalSong / another type the decoder decodes to, I wouldn't mind that design. Implementing a Decoder will be something only expert users of your crate will do, give them an example (or just link to your existing implementations) and they will figure it out. And (II) breaking your current interface: I don't know what version you are currently on, but if you have a 0.* version, I wouldn't mind breaking your interface a bit. That is the contract of 0.* versions, they aren't meant to have long-term stability guarantees (though some do).

2 Likes

Thanks for your idea! It sounds interesting, but I'm not sure I entirely get it :grimacing:. Would you have a doc link for that pattern / these concepts?

Any person using the crate would still have to use the Decoder trait in order to do a Song::from_path, right? The crate is a 0.* version, but adding an extra thing to do whenever you use the lib (adding a use statement) feels a tiny bit like a step backwards somehow.

I think something like csv::Reader is what I'm suggesting, sort of.

Here's what I was thinking:

pub struct Song {
    title: String,
}

pub mod decoder {
    use crate::Song;

    use std::path::Path;

    pub struct InternalSong {
        title: String,
    }

    pub trait Decoder {
        fn decode(path: &Path) -> InternalSong;

        fn from_path<P: AsRef<Path>>(path: P) -> Song {
            let raw_song = Self::decode(path.as_ref());

            Song {
                title: raw_song.title,
            }
        }
    }

    // #[cfg(feature = "ffmpeg")]
    mod ffmpeg {
        use super::{Decoder, InternalSong};

        use std::path::Path;

        pub struct FFmpeg;

        impl Decoder for FFmpeg {
            fn decode(path: &Path) -> InternalSong {
                InternalSong {
                    title: "test".to_owned(),
                }
            }
        }
    }

    // #[cfg(feature = "ffmpeg")]
    pub use ffmpeg::FFmpeg;
}

use decoder::{Decoder, FFmpeg};

fn main() {
    let song: Song = FFmpeg::from_path("path");
}

Seeing this though made me realize that the Decoder trait is probably redundant and it is enough to have a FFmpeg struct with the from_path and decode functions directly associated with it.

I really like the idea of decoupling decoding from Song itself. It's a common pattern, where you have Readers and Writers—like the csv crate I linked above, or polars::prelude::ParquetReader as another example—that produce a Song. flate2 is another great example of this pattern because it implements quite a few of these decoders for various compression formats.

2 Likes

If you're okay with Song::from_path always using ffmpeg even if there are other decoders, then your API could look something like

impl Song {
    #[cfg(feature = "ffmpeg")]
    pub fn from_path(path: impl AsRef<Path>) -> Result<Self> {
        Self::from_path_with_decoder(path, Ffmpeg)
    }

    pub fn from_path_with_decoder(path: impl AsRef<Path>, decoder: impl Decode) -> Result<Self> {
        let raw = decoder.decode(path)?;
        Ok(Self { raw })
    }
}

Rust doesn't have a specific feature for default arguments, so if you want consumers to be able to use from_path without specifying backend, it will have to be a separate function from the function that allows specifying backend. This is just a limitation of the language.

1 Like

After some thinking, it does make a lot of sense that people would bring a trait or something to specify which decoder they wanna use, there's no way (and there should be no other way) around that.

I was wondering though, do you think it would be possible to have something like Song::from_path(path) after the user has use'd a decoder trait, and have from_path "automagically" use that decoder to do the decoding?

What I'm thinking about it that the majority of use cases is to do

song = Song::from_path(path);
something_with_song_analysis(&song);

and not care about the decoder.

So people could easily switch decoders if they wanted to while still being able to leave most of the examples, etc, untouched (instead of having to for example search and replace all instances of Ffmpeg::from_path).
Or something that would make it easy for people to switch around decoders - not sure whether it makes sense or not :sweat_smile:

Presumably the point of the decoder is that it gets the music data into one particular format, PCM or whatever. So it's something like

+------+   Decoder    +------+    Song      +------+
| File | ---------->  | Data | -----------> | Song |
+------+  .decode()   +------+   .new()     +------+

I think you wanted to do this in one step, but why not just do it in two?

Have the user import whatever decoder they want and...

let song: Song = Decoder::decode(path).into();

or the like. They can change the decoder by changing one line if they want.

use song::Ffmpeg as Decoder;

Or you could perhaps set up a type alias for which decoder is the default in your own crate using features. This would be reasonable if there's no significant difference in the output of different decoders.

(If I depend on your crate with no default features, then later depend on another crate who depends on your crate with the ffmpeg feature, that could change which decoder I use. If that translates to getting different results, it could be an unpleasant surprise. But if it just means more input formats are available, that's fine IMO.)


I wrote and threw out a bunch more, but it basically boiled down to: Easiest-in-your-head but unidiomatic isn't necessarily the best. If you hacked up something automagical enough to do what you describe, I might find it surprising and thus annoying, not convenient.

If you really want that method to do it in one step, I think you already have the answer on how it's done idiomatically.

4 Likes

I'd say Rustaceans don't really like automagical stuff. Explicit typing and control flow is something I highly value about Rust and registering a decoder somewhere so it is automagically used by a different API from a different type where it is not evident from the source code that said decoder is used underneath the hood is something I'd highly discourage you from doing.[1] Or as a famous poet once said:

:slightly_smiling_face:


  1. The only place I ever use such a model is when I register a tracing_subscriber to capture my logs. ↩︎

5 Likes

That's a nice trick to make things a bit simpler, totally didn't think about that. I'll go that route then, you guys convinced me that automagic things might not be the best things :slight_smile:
I'll go and implement that then! I might post the code after implementation, to see if it makes sense to you, but I'll mark the post as a solution though :smiley: thanks a lot!

3 Likes

Hi again!

It took me some time, but I've implemented what (I think) looked like what you guys suggested, here Add a "disable-ffmpeg" feature by Polochon-street · Pull Request #65 · Polochon-street/bliss-rs · GitHub (this is the song.rs file where the magic happens, the rest are just changes left and right).
Does this look sane?

I was thinking of isolating the decoder module into its own file, and maybe renaming bliss_ffmpeg into something else, but does the rest look like something usable? Especially wrt. how the docstrings are written for instance :slight_smile: