search

rust3ds / ctru-rs

Rust wrapper for libctru
https://rust3ds.github.io/ctru-rs/
Other
116 stars 17 forks source link

Make ndsp::wave::Wave Send + Sync #177

Closed 0x00002a closed 2 months ago

0x00002a commented 3 months ago

Currently the Wave wrapper struct is not Send + Sync, this is an issue for actually using it as data really needs to be fed to the ndsp service using threads or else it will cause audio clipping (libctru example uses threads and my own experience with a cpal backend supports this).

I need this to properly implement a cpal backend without resorting to a wrapper.

In terms of soundness, as far as I can tell from reading the code and having the cpal backend seem to work fine, this should be okay. The libctru example also backs this up since it uses it in a multithreaded manner but I don't wanna rely on that being 100% correct :p.

Note I've added a dependency on static_assertions so that this part of the API is tested but I'm not married to it if its an issue

Meziu commented 3 months ago

I perfectly get where this PR is coming from. As standard (and as myself even stated in my many experiences using ndsp), using threads to implement audio is pretty much a necessity if your objective is to have anything that sounds better than hot garbage.

However, and I don’t like saying this, I don’t trust libctru’s implementation of ndsp to make the required checks to implement the Send + Sync traits. In the past I’ve already had to tip-toe around many issues inherent in libctru’s implementation (which mainly stem by the dubious way a linked list is embedded in every ndspWave object). One of these cases is the weird behaviour around the Drop implementation of Wave, which obligated me to clear the queue in case the wave was being used here.

In the context of single-thread applications (or apps that implement the whole audio logic on a single thread), the drop order issue is something that can be reasonably worked with (and in general, it’s not much of an issue to drop data since any piece of code will be able to check whether data is valid or not before acting).

In multi-threaded apps, these are assumptions that simply cannot be made (not at our crate’s level at least, your app may put more restrictions in place). Even if somehow I did good enough of a job in my implementation to avoid use-after-free, I’m sure a lot of “randomly clearing the queue and breaking the audio code” shenanigans would still happen, and I’m not ready to work around fixing those.

This is the reason why I suggested the idea of making a complete reimplementation in Rust for ndsp here.

Regardless of my doubts about this, I want this PR to start more discussion about handling this problem, since it’s the first time anyone has showed interest in improving the ndsp wrapper 😛.