Got a working version, would you mind reviewing it? I’m still unsure about best practices in elixir so might be doing something blatantly wrong or not following better conventions.
rgill
OvermindDL1
I’ve no speakers or so to test, but I’ll look over the code right quick since I have a few moments now. 
-
speakers/lib/speakers.ex at master · sample-usr/speakers · GitHub
Mostly defdelegates, nothing special here, examples are short and clean. I’m not used to seeing docs on the defdelegates themselves, wonder how they render, have you pushed it to hex yet? -
speakers/lib/speakers/nif_audio.ex at master · sample-usr/speakers · GitHub
Good standard nif module, proper errors. Could use documentation though (do those work on NIF functions?) even if just for programmer use (doc comments instead of@doc’s). -
speakers/lib/speakers/player.ex at master · sample-usr/speakers · GitHub
Yay specs, love specs, wonder if those come through the defdelegate, if not they should be added there too. ^.^
Noticing some interestingPlayerValidatorcalls in here, I’ll look at that file shortly, but I’m wondering why the NIF isn’t validating these and returning appropriate values (so someone can’t bypass these checks for example, by calling the NIF straight).
A curiosity, does theadd_to_queuetaking a URL accept afile:///some/path/to/file.oggwork? This isn’t documented.
Otherwise most calls just call to the NIF (they could indeed be defdelegates too). ^.^ -
speakers/lib/speakers/player_validator.ex at master · sample-usr/speakers · GitHub
Hmm yeah these checks should really be checked on the NIF side so the user can’t call the NIF directly to bypass these, and it would save the time from, for example, decoding the URI twice needlessly. And why not accept an empty schema/host if it’s afile://path anyway so you can default to passing just a filesystem path like/blah/thing/here.mp3or so? -
speakers/native/speakers_nifaudio at master · sample-usr/speakers · GitHub
TheREADME.mdhere could use updating. ^.^ -
speakers/native/speakers_nifaudio/Cargo.toml at master · sample-usr/speakers · GitHub
Simple enough. Should add yourself as an author though. -
speakers/native/speakers_nifaudio/src/lib.rs at master · sample-usr/speakers · GitHub
- Hmm, it’s a very basic interface, simple for simple playing. Definitely could use a higher level interface for controlling of sinks and sources and mixing though, but that can be done later. Instead of lazy allocating the device sink I’m wondering if it would still be better to handle it as a BEAM resource and let the elixir side hold on to it, so it can deallocate it whenever it’s actually ready (either by closing it via a NIF call or by letting it be GC’d). That would pollute the simple interface though, but it might making a more controlled interface more annoying…
- I recommend at least make a way to deallocate the lazy allocated device sink via a NIF call (
close_global_sink()or something?), and just let it re-lazy-allocate if it is needed again after that. - There definitely needs to be a way to pass in a binary and some metadata about the binary (type, like raw data, a wave, ogg, whatever) and then instance a memory source from that ran through the decoder appropriate for the type.
- Instead of renaming rodio things like
playto beresume, even though more accurate, I wonder if it is better to follow rodio’s naming conventions instead, so it’s docs are more relevant… reqwest::get(audio_url)Hmm, I don’t actually know ifreqwestsupports file paths… File paths definitely need to be supported somehow though..expect("Failed to get audio stream");Eh no, actually test if it fails and report a proper{:error, "Failed to get audio stream"}tuple back to elixir instead, ditto with the otherexpectcalls.expectcalls should never ever appear in released/production Rust code! The rustler error result type is more for program faults, or badarg’s or so, not improper logic or state.
Overall it looks like a good start. I’m getting ideas of a full rodio exposure to elixir, mixing and all. ^.^
1
rgill
This is really nice, thanks a bunch for the great review.
I have been following Dave Thomas’ course on Elixir and he recommends an approach to only have public methods in the root file and the rest should essentially go to modules of their own (which at least for now I think is a good approach) and then you defdelegate to the module methods.
I haven’t published it to hex yet since I wanted to iron out some stuff and make it a bit more stable-ish (not that anybody is gonna use it right away hehe).
Haven’t tried it on NIF modules, but I would add some there as well.
Specs are awesome and yup, they do come through defdelegate.
I added the validation through PlayerValidator because to be honest I just didn’t know how to do that in rust, I’m only starting to explore rust for this library. If you can point me to some examples, I can rework the validation.
And no, at the moment add_to_queue doesn’t accept file:// type paths, though I am definitely going to add that since it’s natively supported by rodio.
That was my first approach as well, but then I kinda got blocked by the elixir → rustler → rust type system and gave up on that to at least have some working version first. If you have any more ideas, I would happily try it again.
Yes, that’s actually easy since rodio provides a function to do it already.
As far as I understood, this is something that rodio does already, I haven’t really tested if it’s based on the file extension or what but you can pass different types of audio URLs and they’ll be played fine. But yeah, I need to research this more and maybe create a struct as AudioURL or something.
Had no idea how to use expect, I would have read up on error handling in rust. Can you expand a little on why they should never be in production code though?
Yeah, hopefully, I’ll keep on exposing more rodio functions
Thanks again for taking the time and reviewing it
2
OvermindDL1
Should be a fairly simple mapping, what did you get stuck on in it?
A source can be user defined, rodio just comes with some default things. For a binary with a ‘type’ you’d instance the proper decoder for the given type (wave, ogg, etc…) and it will return a source for you. For raw data you can just feed it directly in, though it would need some other information like it’s rate and so forth.
expect hard panics, bringing down the entire process, this will include the BEAM itself, it should only be used when the system is in an entirely unrecoverable state at that time.
2
rgill
I was trying to send an elixir stream down to rust. For instance, on the elixir side I have a file stream that then needs to be sent to the rust code. But what I don’t get is what would be the type for the decoding here speakers/native/speakers_nifaudio/src/lib.rs at master · sample-usr/speakers · GitHub
I see, then I’ll add the type as param.
Thanks, I’m going to add better error handling and response on the rust side.
0
OvermindDL1
NobbZ
Well, streams created by File.stream are functions, I doubt you can simply convert them to a slice of bytes.
We already suggested in the slack to either do the Filehandling on the rust side in a dirty NIF or in a port, or to read chunks into a binary and call the NIF repeatedly with those chunks.
That’s a week or two ago…
0
OvermindDL1
Oh I’d never say pass an Elixir stream over, rather a stream messages to fill it in (or a complete message of the entire thing). Reading an elixir stream over and over and over to get the full contents would be so slow in comparison, they aren’t exactly no-cost like Rust streams. 
2
rgill
@NobbZ that’s why based on your suggestion in slack I wired up the remote streaming part in rust.
But @OvermindDL1 just to understand your comment better and wrap my head around it, why exactly can’t we pass the stream to rust?
I’m not sure I understood the filling it in using a stream messages part. If I have something like the following
IO.stream(:stdio, :line)
|> Speakers.add_to_queue
how would I pass that as a “streaming message” to rust? If I am completely off base here, please point me to some resources where I can understand this better.
0
NobbZ
Because what you call a stream can be anything, a function, a struct of an unspecified module, a pid… And nothing but the Stream module knows how to deal with them.
Streams have to be considered opaque.
3