JeffFessler / Sound.jl

Provide `sound` & `record` functions for playing & recording audio in Julia. Also Julia version of phase vocoder.
MIT License
25 stars 1 forks source link

Support SignalBase #5

Closed mchitre closed 2 years ago

mchitre commented 2 years ago

May I recommend that you support https://github.com/haberdashPI/SignalBase.jl (almost the same API as SampledSignals.jl -- and someday SampledSignal.jl is meant to migrate to it -- see https://github.com/JuliaAudio/SampledSignals.jl/issues/56)? That way you'll also end up adding support for other implementations of SignalBase (e.g. https://github.com/haberdashPI/SignalOperators.jl, https://github.com/org-arl/SignalAnalysis.jl, etc).

mchitre commented 2 years ago

Also you might want to consider using https://github.com/JuliaPackaging/Requires.jl to add support for SampledSignal.jl, without bringing it in and all its dependencies as dependencies.

JeffFessler commented 2 years ago

Thanks for the suggestion of using Requires. I had heard of it, but I had not taken the time to understand its purpose before and now I get it. I added that to PR #6.

JeffFessler commented 2 years ago

I don't understand how to use SignalBase. I don't see any type there, just methods for Array types. But sound(x::AbstractMatrix) is already a method that uses the default sampling rate so I don't see how to dispatch specifically with something from SignalBase, whereas SampleBuf is a specific type so I can dispatch on it. Please clarify or feel free to make a PR. Thanks!

mchitre commented 2 years ago

Fair enough. Slightly tricky.

The way SignalBase works is by defining a framerate(x) on any vector-or-matrix-like type to give the sampling rate. Any type that defines this provides a sampling rate (e.g. SampledSignal in SignalAnalysis.jl) and so all you need is to define a sound(x::AbstractVector, S::Real = framerate(x) method to work with it.

The only tricky bit is that SignalBase currently doesn't provide a fallback method. Instead SignalOperators does (returns missing), and so does SignalAnalysis (returns 1). The simplest solution would be to move the fallbacks to SignalBase, so that you can provide a default if you see S is missing.

@haberdashPI, @ssfrr any thoughts?

haberdashPI commented 2 years ago

Fallback methods have to be opinionated, and it is not clear what the fallback should be, in general. The fact that SignalAnalysis and SignalOperators use different defaults seems indicative of this problem to me (sometimes 1 makes sense, sometimes missing makes sense).

More generally, SignalBase is intended as a generic, traits based approach to signal definitions. At some point it might make sense to include a Signal trait, but I had been experimentng with several approaches to this in SignalOperators, and hadn't settled on one yet, so I didn't include it in SignalBase (which is meant to be more stable).

To use something that supported Signal Base you would have to use duck typing. I.e. just try the methods on some generic Any value, and hope they work. If you want to constrain it to some subset of types, you'd need use trait based dispatch (e.g. http://ucidatascienceinitiative.github.io/IntroToJulia/Html/DispatchDesigns)

JeffFessler commented 2 years ago

Perhaps a fallback could go here then, because 8192 Hz is specific to Matlab's history.

framerate(x::Any) = 8192 # fallback
framerate(x::SampleBuf) = x.samplerate # might be unnecessary eventually?

sound(x::AbstractArray, S::Real = framerate(x)) = ...

Thoughts?

haberdashPI commented 2 years ago

Just my two cents, and I don't have a horse in this race but I personally feel, given the use here, that a request for a samplerate from a signal without one should be an error of some sort.

framerate(x::Any) = error("The signal you are trying to play as a sound does not have a known samplerate. ",
                          "Consider wrapping in a `SampleBuf` or defining `framerate(x::MyType)`").
JeffFessler commented 2 years ago

This discussion has compelled me to jettison the obsolete default of 8192 Hz and instead defer to framerate. Thus will force the caller to either specify the rate as a positional argument or pass a type that has a rate associated with it. So we lose a tiny bit of compatibility with Matlab's outdated default, while gaining generality and safer practices.

Personally I like samplerate better than framerate for audio, but it doesn't really matter to me because my students will always be passing in the rate argument as a Real. It seems that I don't even need any dependence on SignalBase here.

Thanks for the great input and feedback. I think that #7 should close this issue but I'll leave it open for a bit in case anyone sees any issues with that PR.

mchitre commented 2 years ago

Agree with defaulting to framerate rather than 8192 Hz. The only reason you may still need the dependency on SignalBase is that framerate needs to be the exported symbol from that module for it to be compatible with it.

haberdashPI commented 2 years ago

Yeah, one of the original motivations for SignalsBase was the name clash of (originally) samplerate. It's nice if all the sound packages can be imported and you don't have a different method for every package just to find the framerate, which is really the same concept everywhere. I went with framerate because in principle you can have multiple samples per frame, even for audio.

On Mon, Nov 29, 2021, 10:47 PM Mandar Chitre @.***> wrote:

Agree with defaulting to framerate rather than 8192 Hz. The only reason you may still need the dependency on SignalBase is that framerate needs to be the exported symbol from that module for it to be compatible with it.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/JeffFessler/Sound.jl/issues/5#issuecomment-982335104, or unsubscribe https://github.com/notifications/unsubscribe-auth/AB37KWFSP5BJ5HWDQQ5SQ3TUORXPFANCNFSM5I6ILT3Q .

JeffFessler commented 2 years ago

Ok I updated #7 to import SignalBase so that framerate hopefully is used properly.

Since you both obviously work a lot with audio signals in Julia, whereas I am new to this aspect, I wonder if you have any insights about how to make the CI run more smoothly. My runtests are passing fine on macOS, but are failing on windows and linux. My guess is that it is because the mac servers in github's cloud have audio devices in them but the linux and windows servers do not (or something like that) so the CI runs fail out somewhere deep inside PortAudio calls because there is no sound car there. Is there a work around you have found for CI of audio in a cross-platform way?

mchitre commented 2 years ago

Great!

I don't have much experience doing CI on a sound card code. The way I'd probably approach it would be to stub the call to the sound card (in this case to PortAudio), and check that the correct values are passed in to the call.

JeffFessler commented 2 years ago

Thanks for the suggestion. FYI, I found an answer in the PortAudio test suite: check devices() first to make sure there is actually an audio device in the system. Incorporated in #10.