Fix auto fallback for --alsa-mixer-device and --alsa-mixer-index#910
Fix auto fallback for --alsa-mixer-device and --alsa-mixer-index#910roderickvd merged 1 commit intolibrespot-org:devfrom JasonLG1979:fix-alsa-mixer-device
Conversation
|
Would be nice if @coderootme and yourself could report back on various combinations, also in the long form of |
|
Yes, I can test it no problem! |
It works for me. Where my And: Everything works as intended. Note that |
|
After this quick fix is merged I will go though and de-panic the mixer(s). |
|
As a future improvement
|
|
But that's just my opinion. |
|
What do the other backends call it? Particularly |
|
PulseAudio and Gstreamer both call outputs Sinks. I don't know about rodio? |
|
In Gstreamer an element has an input called a PulseAudio works about the same way. |
|
https://docs.rs/rodio/latest/rodio/struct.Sink.html So basically everyone calls it a sink except ALSA. Which I can honestly understand since they all represent audio signal flow more abstractly while ALSA is lower level. Where all of the different "sink" abstractions will eventually hit the "real" ALSA "card,device" (on Linux ofc) |
|
So I guess |
|
@roderickvd that last commit fixes the auto fallback for |
|
Looking for testing from you @coderootme and a review and possibly merge from you @roderickvd. |
|
On related side note I looked into unpanicking the mixer and there are 2 things I see as temporary blockers for me:
|
|
Well hold up on the merge. auto fallback doesn't work with things like |
|
The last commit should fix fallbacks for devices that don't start with |
Just compiling 042f5f9 on a Raspberry Pi 4. Will report back in a few minutes! |
|
OK, so here is my test. I compiled with: My test on a DAC which has a hardware mixer – the one that previously failed to work without current librespot 0.3.1 7160dc1:
current librespot 0.3.1 7160dc1:
JasonLG1979's fix-alsa-mixer-device, commit 042f5f9:
JasonLG1979's fix-alsa-mixer-device, commit 042f5f9:
Is this a good enough test for you guys, or could I test something else, or report some more information? |
Awesome, I also fixed/handled some other situations.
|
I think that about covers it, thank you. @roderickvd balls in your court. |
|
@coderootme I guess I forgot: Should also all work in your case can you test those also? Sorry. |
|
|
I think it’s a good idea to have |
Have at it. It's all yours,lol!!! I think I've done all I can here without changing the mixer(s). |
|
Another question why |
Just tested these. All 3 work properly. |
Great!!! Thank you very much. @roderickvd what say you? |
JasonLG1979
changed the title
|
A little force push to get past a code 101. I like the little green check mark 😃 |
roderickvd
left a comment
roderickvd
left a comment
There was a problem hiding this comment.
Nice enhancement. As usual a few questions about idiom.
| .chars() | ||
| .last() | ||
| .unwrap_or_default() | ||
| .to_digit(10) |
There was a problem hiding this comment.
Converting to base-10 this way is rather explicit 😆 can't we just do .parse() or any of the usual idioms?
There was a problem hiding this comment.
This way anything but 0-9 is 0. It's shorter than parse u32 unwrap_or_default().
There was a problem hiding this comment.
Hold up. I'm still thinking about this one?
There was a problem hiding this comment.
Will this work for card numbers >= 10? That might not be so common but while we're going for robustness here...
There was a problem hiding this comment.
Will this work for card numbers >= 10? That might not be so common but while we're going for robustness here...
That's part of what I'm ciphering.
There was a problem hiding this comment.
It's pretty smart which I mean both in a good and a cautious way. Meaning: this catches a lot of variants in a way that is not entirely obvious. I think it would be wise to document possible values with a little code documentation for future readers.
There was a problem hiding this comment.
It's pretty smart which I mean both in a good and a cautious way. Meaning: this catches a lot of variants in a way that is not entirely obvious. I think it would be wise to document possible values with a little code documentation for future readers.
Done. I've put comments in the code that explain what it does.
So it now reads:
.unwrap_or_else(|| match device {
// Look for the dev index portion of --device.
// Specifically <dev index> when --device is <something>:CARD=<card name>,DEV=<dev index>
// or <something>:<card index>,<dev index>.
// If --device does not contain a ',' it does not contain a dev index.
// In the case that the dev index is omitted it is assumed to be 0 (mixer_default_config.index).
// Malformed --device values will also fallback to mixer_default_config.index.
Some(ref device_name) if device_name.contains(',') => {
// Turn <something>:CARD=<card name>,DEV=<dev index> or <something>:<card index>,<dev index>
// into DEV=<dev index> or <dev index>.
let dev = &device_name[device_name.find(',').unwrap_or_default()..]
.trim_start_matches(',');
// Turn DEV=<dev index> into <dev index> (noop if it's already <dev index>)
// and then parse <dev index>.
// Malformed --device values will fail the parse and fallback to mixer_default_config.index.
dev[dev.find('=').unwrap_or_default()..]
.trim_start_matches('=')
.parse::<u32>()
.unwrap_or(mixer_default_config.index)
}
_ => mixer_default_config.index,
})And:
Some(ref device_name) => {
// Look for the card name or card index portion of --device.
// Specifically <card name> when --device is <something>:CARD=<card name>,DEV=<dev index>
// or card index when --device is <something>:<card index>,<dev index>.
// --device values like `pulse`, `default`, `jack` may be valid but there is no way to
// infer automatically what the mixer should be so they fail auto fallback
// so --alsa-mixer-device must be manually specified in those situations.
let start_index = device_name.find(':').unwrap_or_default();
let end_index = match device_name.find(',') {
Some(index) if index > start_index => index,
_ => device_name.len(),
};
let card = &device_name[start_index..end_index];
if card.starts_with(':') {
// mixers are assumed to be hw:CARD=<card name> or hw:<card index>.
"hw".to_owned() + card
} else {
error!(
"Could not find an alsa mixer for \"{}\", it must be specified with `--{}` / `-{}`",
device_name,
ALSA_MIXER_DEVICE,
ALSA_MIXER_DEVICE_SHORT
);
exit(1);
}
}There was a problem hiding this comment.
The assumption during parsing either is that --device is not malformed. If --device is malformed it would fail when the alsa backend tried to open it anyway.
There was a problem hiding this comment.
@roderickvd is that more to your liking?
There was a problem hiding this comment.
A lot of it is just taking advantage of the fact that you can use find to walk though a string especially if you know what to expect and/or what you want to find for example:
random_string.find("random chars").unwrap_or_default()The above will either return the left most index of "random chars" (the "r") or 0 if the string does not include "random chars".
That's why I said " |
No what I mean is for new to replace open because open creates a new thing in those cases so it's strange to me that the functions are called open. |
Ah I see. I agree that |
|
For the backends it should be new, start, stop. Open in that case is an implementation detail IMHO. A backend might open on new or start depending. For the mixers it should just be new, as there's no close or even stop. Again whatever alsa calls messing with the mixer is an implementation detail. |
I'm not even really a fan of create as really how is it different from new? Although create is better than open, new is a very basic, pretty universal programming idiom. A function named new very clearly I think would be for creating a new thing. |
|
If there are different or alternative ways to make a new thing I prefer |
|
So in my example:
|
|
@roderickvd this is failing on nightly with: It appears to be caused by this (in code quotes so it doesn't add noise to that issue): Other than that I think this is ready to merge. I'm pretty sure I've addressed all of your comments? |
…device Fix auto fallback for --alsa-mixer-device and --alsa-mixer-index
2 participants
As mentioned in #898 (comment)
--alsa-mixer-deviceand--alsa-mixer-indexnow fallback to the card and index specified in--device.