mirror of
https://github.com/librespot-org/librespot.git
synced 2026-04-27 08:15:50 +03:00
[GH-ISSUE #898] Command line arguments are incorrectly echoed in TRACE. #447
Labels
No labels
A-Alsa
SpotifyAPI
Tokio 1.0
audio
bug
can't reproduce
compilation
dependencies
duplicate
enhancement
good first issue
help wanted
high priority
imported
imported
invalid
new api
pull-request
question
reverse engineering
wiki
wontfix
No milestone
No project
No assignees
1 participant
Notifications
Due date
No due date set.
Dependencies
No dependencies set.
Reference
starred/librespot#447
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "%!s()"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Originally created by @ghost on GitHub (Dec 5, 2021).
Original GitHub issue: https://github.com/librespot-org/librespot/issues/898
See this output:
The part that says
-3belongs to the argument before that.The argument is parsed correctly, because the other part of the log is correct:
So it seems only the echo is incorrect. Minor error, but could be fixed whatsoever I believe.
@ghost commented on GitHub (Dec 5, 2021):
Seems the problem is around this line:
github.com/librespot-org/librespot@e66cc5508c/src/main.rs (L669)The code expects a minus sign always denotes a new item, while a minus sign in this case denotes a negative value instead.
@JasonLG1979 commented on GitHub (Dec 5, 2021):
I got it that's my bad. I'll just make sure anything followed by a - is only 1 char and a letter that should do it.
@ghost commented on GitHub (Dec 5, 2021):
That check would probably be enough, unless
(-)[a-z]would be a valid username and/or password, i.e.--username -aor--password -awhich I am fairly certain are not valid.@ghost commented on GitHub (Dec 5, 2021):
Now that I think of it, device name should probably be valid as
--name "-a", which would break the rule, but should work.Anyone correct me please?
@JasonLG1979 commented on GitHub (Dec 6, 2021):
With the PR:
Outputs:
"--name" is kinda weird but still a valid device name.
"totally invalid because --verbose is a flag" and "totally invalid because -N is a flag" are ignored because they are never actually evaluated by
librespotbecause--verboseand-Nare flags so we only check if they exist."-a" is technically valid I guess maybe? Although I'd bet it would fail as soon as you tried to use a device named "-a".
-uis the short version of--usernameso "stuff" is redacted."fart" is not evaluated because it's not a valid flag or option or a value of a valid option. It's ignored completely by
librespot.@JasonLG1979 commented on GitHub (Dec 6, 2021):
Also @coderootme a couple notes about your args:
--backend alsais not necessary unless you have also compiled other backends intolibrespot.Unless you have some special reason to use
--mixer alsa(like you're using some alsa feature that requires that you use alsa volume control) software volume control inlibrespotis objectively superior to alsa hardware volume control, and much less potentially jankey because it actually works 100% of the time as apposed to alsa volume controls.--normalisation-method dynamicis redundant.dynamicis already the default.--alsa-mixer-device hw:1is also redundant as it defaults to whatever you set--deviceto.--normalisation-gain-type trackUnless you really prefer track mode all the time you might just leave that out so it defaults toauto. Which changes fromtracktoalbummode automatically depending on what you're listening to.@ghost commented on GitHub (Dec 6, 2021):
The advantage of using
alsa's mixer, for me at least, with DACs that support it, is that the volume change is instant, while the software volume control delays the volume change (because of buffer, I think). For audiophile setup, I do not use any mixer at all and change the volume with the volume knob on my amplifier only, which is not really that often when the whole gear is of high quality and all tracks are outputted at the same LUFS.Yes, I know, I will probably erase it. I've been using it for a long time now and I like to configure all parameters like these, in case the defaults might change in the future (I manage multiple librespot-powered devices and I wouldn't like some settings changing by itself just because the project development decides to change the default when no value is specified.
How probable is this situation with FOSS?
I thought so too, but it doesn't work without that parameter. Here is the output log when I completely remove
--alsa-mixer-device hw:1from the command:Yes, I currently really prefer track mode. I usually do not listen to whole albums and when I do, I do not mind that the relative volume of songs is changed. For very good masters, like Dire Straits/Mark Knopfler for example, I find the track mode relatively okay-ish, and for anything with bad mastering, I don't mind changing the relative volume of songs. The convenience of listening without touching the volume knob too often is more important to me than listening to an album with crappy mastering as the author intended, when the intention is shit in the first place. I'm not sure I'm making any sense here. But I will happily change my opinion in the future, as I have changed many opinions in the last years.
@JasonLG1979 commented on GitHub (Dec 6, 2021):
I wouldn't think there should be any noticeable delay normally, the buffer should only be 0.5 secs?
That would be a bug for @roderickvd the
,0part ofhw:1,0should probably be stripped off in that case and in the case that--devicetakes the form of something likehw:CARD=Generic,DEV=0it should probably be justhw:CARD=Generic? Idk about that one? That would require some more testing?In the meantime you can use
hw:1for--devicesince it's the same ashw:1,0and it should work.0is the default device. Any place anywhere in any alsa config (not justlibrespot) if the device is0you can omit it.Fair enough.
@roderickvd commented on GitHub (Dec 6, 2021):
I don’t know for sure either, but it does look redundant (and so not functioning). You can file an issue or PR for it. Right now still hard at work at other librespot stuff so it’d on the backlog.
@JasonLG1979 commented on GitHub (Dec 6, 2021):
You should already be getting warning messages about
--normalisation-method dynamicin dev. But reallylibrespotis not stable and makes not promises about stability whatsoever. Args can and have changed and may in the future with little to no notice.@JasonLG1979 commented on GitHub (Dec 6, 2021):
I'll add it to my list which adds it to your list,lol!!!
@JasonLG1979 commented on GitHub (Dec 6, 2021):
@roderickvd along the way I'll see about making the mixer not explode on errors,lol!!
@roderickvd commented on GitHub (Dec 6, 2021):
While I feel that API and command line option stability is important, consider everything as tentative baselined until 1.0.0 and it’s up to FOSS to keep up. I’m a moOde user myself and make efforts to keep that up to date.
@ghost commented on GitHub (Dec 6, 2021):
Yes, I was wondering about that. Why is the librespot development team thinking of removing the basic normalisation method?
I know many audophiles do not like any dynamics compression whatsoever; myself included, except for some special cases, for example when listening on speakers at night as a background noise when doing something else, but definitely not when fully focused on music using good gear.
Removing
simplemethod leaves users who want to equalize tracks to the same LUFS to usedynamicwith very lowpreampvalue, I think-9is the safest value. Which is OK, as long as your DAC and AMP have very high SNR, which most don't.simplenormalisation method solves this at least a bit for poor setups, in my opinion.But the whole audio theory is so difficult and trying to satisfy all categories of users, casual listeners and audophiles, is sometimes impossible. But removing an already implemented function which simply works, is always so strange to me.
mpdhas this function too. It is calledreplaygain_limitand by default it is enabled, i.e. it works the same as librespot'ssimplevolume normalization. See:github.com/MusicPlayerDaemon/MPD@2240327286/doc/mpdconf.example (L401)I love how these issue threads always result in discussions where all users learn something new. This community is so amazing! I love being here and reading all your posts and ideas. Obviously many developers here are very knowledgeable about the whole project, which is awesome!
@JasonLG1979 commented on GitHub (Dec 6, 2021):
The idea being that dynamic is objectively better than basic. Basic is literally just the volume being turned up and down as you've figured out. And 99.99% of people would rather use dynamic.
Currently you really can't put "audiophile" in the same sentence as Spotify since it's lossy audio. When they roll out the HiFi tier and start streaming FLAC maybe.
It's mostly about trying to pair the code and options down. There's no point keeping options and code that most people don't use. Code is not a static thing it has to be maintained. As other parts of the program are updated and changed that mostly unused code also has to updated and changed putting undo burden on devs.
The consensus I think was that dynamic > basic to most users and that audiophiles generally would not use it at all for the most part.
@roderickvd commented on GitHub (Dec 6, 2021):
@coderootme point did make me think it over. It's fair enough not to remove existing functionality that's working well and used in the wild, even when I don't think by a lot of people. Honestly in this particular case it is so little lines of code that the burden of maintainability isn't really an issue.
On the other hand there's the argument of toning down on configuration options which is an agreed direction.
Where the logic seems flawed however is the point on catering to audiophiles (count me in as one so it's not at all that I object to that group!). Audiophiles with a poor setup, little headroom or poor SNR, is just an oxymoron.
The dynamic limiter (yeah, actually a compressor) preserves DNR for 99% of the time by only throwing away bits (potentially, depending on the sample format) when the track is over limit. The basic limiter throws away DNR for the entire track even if it's just a short passage that's over limit.
For most music and playlists it really won't matter that much. You'd be surprised how little the limiter engages at the default settings with music that is mastered sensibly. I'm not talking about hip hop and pop rock of course, there's little to save there even for an audiophile. But start throwing classical into the mix and at those fringes of dynamic range, it really shows that the basic limiter is a poor man's tool.
@JasonLG1979 is right: by these accounts the dynamic limiter is objectively better. I understand that there will always be exceptions to that rule, but I'm not buying into the audiophile argument here.
@JasonLG1979 commented on GitHub (Dec 6, 2021):
Even with pop, rock, hip-hop and metal (generally what I listen to) if you set the
--normalisation-pregainto-3.0and--normalisation-thresholdto-3.0you're basically adhering to the ReplayGain standard with 3dB of additional headroom for tracks that might not be mastered the greatest. With those settings I have never noticed the limiter kick in either audibly or by skimming the logs.@JasonLG1979 commented on GitHub (Dec 6, 2021):
You can get pretty deep in the weeds when you start talking audiophile jargon with some of if grounded in reality more other things but generally speaking I tend to be an objectivist when it comes to hardware and software design. We can't know what every system sounds like so we have to go with the math and science as the great equalizer.
@ghost commented on GitHub (Dec 7, 2021):
I agree. This is a good point.