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

Update to librespot 0.3.0 #445

Merged
merged 2 commits into from Oct 5, 2021

Conversation

roderickvd
Copy link
Contributor

We plan to release librespot 0.3.0 shortly. This PR adds support for the various new and changed command line options. Please refer to the extended commit log for more information.

I'll drop a note here once 0.3.0 is out of the door!

 * Allow initial volume to be empty, and make that the default. Doing so will
   have librespot query the current volume from hardware if so supported, or
   fall back to 50% otherwise. If an initial volume is entered, then that is
   treated as an override value.

 * Enable normalization by default, like in the official Spotify client.

 * Update the default pregain to 0 dB and the treshold to -2 dBFS, like in the
   latest official Spotify client.

 * Add an "auto" normalization gain type, and make that the default. Doing so
   will have librespot use album values when playing an album, and track values
   otherwise, like in the official Spotify client.

 * Add dithering options, defaulting to automatic depending on the format. This
   is what happens when no dithering option is selected. Set dithering to "none"
   to override.

 * Add a cubic volume control.

 * Add a configurable range for the volume controls, which is empty by default.
   Doing so will have librespot query the hardware device capabilities if so
   supported, or fall back to 60 dB otherwise. If a volume range is entered,
   then that is treated as an override value.

 * Add support for a F64 format.

 * Note that librespot-org is the Librespot Organization, which includes but is
   not limited to the great Sasha Hilton.

 * Fix various typos.
@moodeaudio
Copy link
Contributor

Great, thanks :-)

I'll review over the coming days.

@roderickvd
Copy link
Contributor Author

roderickvd commented Oct 5, 2021

Come to think of it: for librespot to query the volume range and current volume from Alsa, it needs to use the Alsa mixer. moOde doesn't do that and uses the softvol mixer instead. So, the hints in this PR may be misleading because there are no hardware queries. We could do as follows:

  1. update the hints to match;
  2. default volume range to 60 dB (no empty values);
  3. do allow initial volume to be empty, so that the previously cached volume is picked up.

What do you think?

Note that I do recommend softvol over the Alsa mixer in 99% of cases, unless there is some specific requirement, such as latching onto mute/unmute switches to physically power on/off or switch some relays. Softvol works in 64-bit floating point which will be better even than 32-bit integer mixing in hardware. For users with those specific requirements, I think they are advanced enough to configure the Alsa mixer themselves, without introducing this complexity in moOde.

@moodeaudio
Copy link
Contributor

Correct, we use software volume for audio renderers to keep it simple for users and so far no complaints. Also many users have audio devices that don't support hardware mixer.

Couple things:

  1. Since we use softvol exclusively is the volume_range option still needed?
  2. Initial volume is a number field and filtered in the HTML for 0-100 range. To also offer an option to "Use previously cached volume" i.e., initial_volume = "" the input field will need to be changed. It could be changed to dropdown with "Use cached volume" at the top followed by the items 0 to 100. Another option is to keep it numeric and have range from -1 - 100 where in the help its explained that -1 means "Use previously cached volume"
  3. Should volume_normalization default to "Yes"? Maybe best if leave it at "No" and let user decide whether to turn it on.

@roderickvd
Copy link
Contributor Author

  1. Since we use softvol exclusively is the volume_range option still needed?

If you leave it out, it will default to 60 dB. This is a sane value and was hardcoded before. If you leave it as a configurable setting, then it has the advantage that users can control the steepness of the volume curve, in effect influencing the usable volume range of the slider.

  1. Initial volume is a number field and filtered in the HTML for 0-100 range. To also offer an option to "Use previously cached volume" i.e., initial_volume = "" the input field will need to be changed. It could be changed to dropdown with "Use cached volume" at the top followed by the items 0 to 100. Another option is to keep it numeric and have range from -1 - 100 where in the help its explained that -1 means "Use previously cached volume"

My PR already does that, if we're talking about the same thing:

$initial_volume = empty($cfg_spotify['initial_volume']) ? '' : ' --initial-volume ' . $cfg_spotify['initial_volume'];

If the field is left empty, then the cached value is used.

  1. Should volume_normalization default to "Yes"? Maybe best if leave it at "No" and let user decide whether to turn it on.

I have no particular preference either way. Normalization is enabled by default in the official Spotify client, but purists may look the other way. In Librespot it's not standard either but we haven't debated it.

@moodeaudio
Copy link
Contributor

Ok so makes sense to have volume_range option and we can leave normalization initially "No" which would be consistent with the other renderers and MPD where DSP is initially off.

The issue with input fields of type="number" is that only numeric values are allowed. User could never set the field to "".

@roderickvd
Copy link
Contributor Author

Ok so makes sense to have volume_range option and we can leave normalization initially "No" which would be consistent with the other renderers and MPD where DSP is initially off.

OK I'll update the PR to do that and change the hints as well.

The issue with input fields of type="number" is that only numeric values are allowed. User could never set the field to "".

I understand what you're saying, but this is what I tried in Safari yesterday and worked just fine. Haven't tried other browsers yet, but in Safari I could just clear the value.

@moodeaudio
Copy link
Contributor

Being able to set numeric field to blank is because the attribute "required" is missing in the <input... statement. I did quick global search of the codebase and most number fields have the "required" attribute which is really the correct way to validate number fields.

Prolly the easiest would be to have -1 be the value that designates the conditions below. The help could say "Set to -1 to ..."

initial_volume = "Use previously cached volume"
volume-range = "Query from hardware or fall back to 60 dB otherwise"

When the field is set to -1 the startSpotify() function would test for this value instead of blank.

@roderickvd
Copy link
Contributor Author

OK I'll work on it tonight.

@roderickvd
Copy link
Contributor Author

03010b8 should about do it.

@moodeaudio moodeaudio merged commit f3ca4e6 into moode-player:develop Oct 5, 2021
@roderickvd
Copy link
Contributor Author

Thanks ❤️

We haven't released 0.3.0 yet as there was a last minute fix we want to vet first, but it shouldn't be long. In the meantime if you want to run librespot with these new options, feel free to compile the current dev -- it's in feature freeze and will become 0.3.0.

@moodeaudio
Copy link
Contributor

Will do.

Question: Is the -1 setting and default behavior for initial_volume independent of whether we launch librespot with --mixer sovtvol vs --mixer alsa (+ the other mixer params)?

For example let's say we have two audio devices. Device A has a hardware volume mixer and device B does not. With the -1 setting the volume slider on a client connecting to device A will be set to the current alsa volume on that device. If the client connects to device B the client volume slider will be set to 50%. Is that correct?

@roderickvd roderickvd deleted the librespot-0.3.0 branch October 6, 2021 12:25
@roderickvd
Copy link
Contributor Author

For example let's say we have two audio devices. Device A has a hardware volume mixer and device B does not. With the -1 setting the volume slider on a client connecting to device A will be set to the current alsa volume on that device. If the client connects to device B the client volume slider will be set to 50%. Is that correct?

Correct for device A.

Correct for device B if it is the first launch or the cache has been wiped from disk, otherwise it will use the previously set volume.

@moodeaudio
Copy link
Contributor

Perfect, thanks :-)

@roderickvd
Copy link
Contributor Author

We just released 0.3.0. One PR slipped through and we might release a 0.3.1 soon, but at least you can now get these new librespot options to work.

@moodeaudio
Copy link
Contributor

Compiled and working fine :-)

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