RFC: start of audio system

[tjkirch]

~Starts to address #32.~ Now with sample (sound effect) and file (music) playback, I think this fixes #32. APIs for generating sound aren't included, but I think that can be a separate issue.

~I have this working prototype for an audio interface and I was hoping for feedback before I go further. I'm not experienced with Rust/C interfaces, so there could be mistakes there. Does this fit crankstart? Would it expand to the rest of the sound API OK?~ Ready for review!

I mainly patterned this after the graphics module, with some tweaks. I attempted to follow crankstart and Playdate standards, veering away only where it seemed clear. The most important change is probably the sound modules/subsystems - the Playdate audio APIs are the largest set of APIs, so I wanted a design that could split out the main subsystems. sound is the top-level, and sampleplayer is the specific subsystem I implemented here, for sound effects.

A few specific decisions worth consideration:

• Storing the subsystem pointer (e.g. *const crankstart_sys::playdate_sound_sampleplayer) in Sound as well as the specific structs SamplePlayer and AudioSample for convenience.
• Handling errors that I can't really test and aren't documented, e.g. when play returns non-1.
• ~The use of &mut on the AudioSample argument to set_sample - I think I originally did this because of the use of *mut for arguments all over the place in the bindings, but in the user-facing API, I don't think it matters..? I should probably make it &.~ Made & with internal Rc; see comments below.

~If this design is basically OK, I'd want to implement fileplayer as well before merging, to make sure the modular approach works cleanly, and because fileplayer is for music, probably the next most important subsystem after sound effects. (I don't think I'll need to generate sounds on-device, so most of the remaining APIs aren't as urgent for me.)~ Added.

I tested all of the functions in the simulator, and confirmed playback worked on device. Docstrings detail any non-obvious behavior I found.

~The only calls I found confusing were get_offset/set_offset, which were clearly getting through to the Playdate APIs OK (it did offset, and wouldn't go beyond my sample length) but weren't being set exactly to the values I passed through, even with rate 1.0. I suspect my WAV files may be odd; I'm not very experienced with sound files or APIs.~ I misunderstood the purpose of offset and clarified the docstring.

[rtsuk]

Overall this seems good.

Storing the subsystem pointers seems fine, they are essentially static.

For handling errors, I think it is a bit of a mixed bag already, so whatever approach you use is fine. An interesting unrelated project would be to figure out what makes sense for error handling overall, and make sure that's at least possible for game authors.

I think there might be a soundness problem with AudioSample. What happens if you start playing it, then drop it? It appears the Rust code will free it while the audio system is still using it. This is an aspect the Playdate folks never document. I asked about it for bitmap tables.

If the samples can be immutable, I think we could use Rc. That way the Rust player Struct could keep the sample alive until it is done with it, regardless of what the caller does.

With Sprites I ended up using Rc<RefCell> to solve a similar issue, but hopefully that's not needed here.

[tjkirch]

I think there might be a soundness problem with AudioSample. What happens if you start playing it, then drop it? It appears the Rust code will free it while the audio system is still using it. This is an aspect the Playdate folks never document. I asked about it for bitmap tables.

@rtsuk I tried to test this. I made a SamplePlayer in my game struct (so, long-lived) and an AudioSample in my update loop (so, short-lived), and added logging at the new/free points. I used a long WAV file to be sure it was freed before it finished playing. This is what happened:

• player created
• I take action to play the sound
• sample is loaded
• sample starts playing
• sample is freed
• sound continues to play for its remaining ~9 seconds OK
• I take action again, twice in a row, so two samples are created and freed quickly, and I hear (a single copy of) the sample play all the way through OK
• I set the crank spinner in the simulator to its max speed, playing the sound on each revolution, so it played a few hundred times quickly, and nothing bad happened, it kept playing

So it seems to work, but from what you said, I'm worried that perhaps it relies on undocumented behavior, or maybe it's a use-after-free because it's relying on nothing reusing the memory where the sample was loaded. What do you think? Would a Rc<RefCell> be the right approach for safety?

If the samples can be immutable

When the sample is passed as an argument, the bindings expect a mutable pointer, but I'm not sure what that means under the hood. For our purposes, it's effectively immutable, because we're just using it to store a pointer, and we don't change that pointer. But I can't say whether it's safe to treat it as immutable - maybe it's changing something in the underlying struct that makes it unsafe to load into two players, or something? I'm not sure how to tell or test.

[rtsuk]

I think we can treat the sample as immutable from Rust's point of view, so we can get away with just Rc, but there's no harm to Rc.

I think it is best to assume the most dangerous possible behavior from the PlayDate firmware, so we should not free anything we've handed to them until they aren't using it anymore. If you look at the way Sprite works, that should provide maximal safety. Since the Rc is inside a cloneable struct it's not too hard on the game developer.

[tjkirch]

^ The force push improves a few comments in the original commit based on what I learned building the next module (fileplayer) and adds #[derive(Debug)].

[tjkirch]

The second push adds two commits: one for using Rc for AudioSample, to address the concern above, and one that adds FilePlayer for music playback.

[tjkirch]

I tested the audio sample Rc change with short-lived samples in the update function, which played fine and were not freed until I changed the sample later, and with long-lived samples in the game struct, which worked as before.