Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make compatible with rtaudio API 5 and 6. #1284

Merged
merged 4 commits into from Jan 22, 2024

Conversation

hzeller
Copy link
Contributor

@hzeller hzeller commented Jan 21, 2024

Recent rtaudio changed the API to not throw exceptions anymore and also have DeviceIDs not queried by index but IDs that are provided separately ( https://github.com/thestk/rtaudio/releases ).

Adapt the code-base to be compatible with the old and the new API as we have to exepect that in a transition period both APIs are common on various build platforms.

@AlexandreRouma
Copy link
Owner

AlexandreRouma commented Jan 21, 2024

lines 34 -> 48: Omitting the triple line comment stating the obvious, having a lambda like that is just making the code more visually bloated. It must be moved to a private member function instead. Additially, looking at the error types, throwing just because something is not a warning sounds like a bad idea:
image
This will result in a complete crash of the entire application if any of these gets fired outside of the trycatch, which considering it's a callback is not clear whether it's a possibility or not. Just print an error instead.

lines 58 -> 67: If you're gonna need an if macro anyway, why waste lines, memory and compute power generating a new array when you can just change the for statement itself... Also the entire code is camelCase so the variable name wasn't conform.

@hzeller hzeller force-pushed the 20240121-adapt-rtaudio branch 2 times, most recently from 374d728 to 3eea016 Compare January 21, 2024 22:35
Recent rtaudio changed the API to not throw exceptions anymore and
also have DeviceIDs not queried by index but IDs that are provided
separately ( https://github.com/thestk/rtaudio/releases ).

Adapt the code-base to be compatible with the old and the new API
as we have to exepect that in a transition period both APIs are
common on various build platforms.
@hzeller
Copy link
Contributor Author

hzeller commented Jan 21, 2024

Lines 34-> 48 : Done.

  • Added static member function and passed that to the setErrorCallback()
  • Made the choice of things to warn about and things to throw about a switch/case

lines 58->67: Done. Made the choice of source directly an #ifdef around the loop header.

@AlexandreRouma
Copy link
Owner

Ok. Also don't remove the info.probed indiscriminately. I didn't just put them there to add more lines. On some system, not checking for that causes a crash because RtAudio does not guarantee all indices to be valid devices.

@AlexandreRouma
Copy link
Owner

AlexandreRouma commented Jan 21, 2024

Other things while I'm at it:

  • The : 4 characters away from each other are ugly, use flog::error("AudioSinkModule Error getting audio device ({}) info: {}", i, e.what()); (yes the numbers in the log escape are useless, they were still there from the previous logger)
  • flog::warn("AudioSink: {0} ({1})", errorText, (int)type); should be flog::warn("AudioSinkModule Warning: {0} ({1})", errorText, (int)type); for consistency

@AlexandreRouma
Copy link
Owner

And also rename reportErrorsAsException to something more sensible and descriptive like errorCallback

@hzeller
Copy link
Contributor Author

hzeller commented Jan 21, 2024

Ok, I can re-add the probed for the old API.

I have a question in the AudioSource:

if (info.probed && info.inputChannels < 2) { continue; }

The info.probed is used in a context where stereo channels are queried with the result that non-probed inputs that are non-stereo will pass through. This looks like it was meant to skip if this was not info.probed; so similar like in the AudioOutput if (!info.probed) { continue; } ?

@AlexandreRouma
Copy link
Owner

AlexandreRouma commented Jan 21, 2024

Looks like a typo, was meant to be !probed || instead of &&, you can switch it to that and it'll have the desired behavior

@hzeller
Copy link
Contributor Author

hzeller commented Jan 21, 2024

Addressed

  • info.probed is now used when the old API is active.
  • fix the warning and error log formatting as suggested.
  • changed reportErrorAsException() to errorCallback()

@AlexandreRouma AlexandreRouma merged commit 86dcec7 into AlexandreRouma:master Jan 22, 2024
14 checks passed
@hzeller hzeller mentioned this pull request Jan 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants