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

Lay groundwork for new Spotify API client #805

Merged
merged 67 commits into from
Jun 28, 2021

Conversation

roderickvd
Copy link
Member

The scope of this PR is the "client" part of the new Spotify API: getting registered as a Connect device, so we can start receiving messages from the dealer (which will be a subsequent PR).

Johannesd3 and others added 30 commits May 19, 2021 21:05
This is a squashed commit featuring the following:

Connect:
- Synchronize player volume with mixer volume on playback
- Fix step size on volume up/down events
- Remove no-op mixer started/stopped logic

Playback:
- Move from `connect` to `playback` crate
- Make cubic volume control available to all mixers with `--volume-ctrl cubic`
- Normalize volumes to `[0.0..1.0]` instead of `[0..65535]` for greater precision and performance (breaking)
- Add `--volume-range` option to set dB range and control `log` and `cubic` volume control curves
- Fix `log` and `cubic` volume controls to be mute at zero volume

Alsa mixer:
- Complete rewrite (breaking)
- Query card dB range for the `log` volume control unless specified otherwise
- Query dB range from Alsa softvol (previously only from hardware)
- Use `--device` name for `--mixer-card` unless specified otherwise
- Fix consistency for `cubic` between cards that report minimum volume as mute, and cards that report some dB value
- Fix `--volume-ctrl {linear|log}` to work as expected
- Removed `--mixer-linear-volume` option; use `--volume-ctrl linear` instead
Also remove some other getopts and string changes to a separate PR.
…optimisations

Optimize volume control logic
Dithering lowers digital-to-analog conversion ("requantization") error, linearizing output, lowering distortion and replacing it with a constant, fixed noise level, which is more pleasant to the ear than the distortion.

Guidance:

- On S24, S24_3 and S24, the default is to use triangular dithering. Depending on personal preference you may use Gaussian dithering instead; it's not as good objectively, but it may be preferred subjectively if you are looking for a more "analog" sound akin to tape hiss.

- Advanced users who know that they have a DAC without noise shaping have a third option: high-passed dithering, which is like triangular dithering except that it moves dithering noise up in frequency where it is less audible. Note: 99% of DACs are of delta-sigma design with noise shaping, so unless you have a multibit / R2R DAC, or otherwise know what you are doing, this is not for you.

- Don't dither or shape noise on S32 or F32. On F32 it's not supported anyway (there are no integer conversions and so no rounding errors) and on S32 the noise level is so far down that it is simply inaudible even after volume normalisation and control.

New command line option:

--dither DITHER Specify the dither algorithm to use - [none, gpdf,
                tpdf, tpdf_hp]. Defaults to 'tpdf' for formats S16
                S24, S24_3 and 'none' for other formats.

Notes:

This PR also features some opportunistic improvements. Worthy of mention are:
- matching reference Vorbis sample conversion techniques for lower noise
- a cleanup of the convert API
…ion-pregain

Print normalisation pregain in verbose mode
…overs

Fix leftovers from merging diverging branches
@roderickvd
Copy link
Member Author

roderickvd commented Jun 21, 2021

1562884 introduces a caching ApResolver component. By "caching" I mean that Spotify returns 9 access points, 3 dealers and 3 spclients in random order. ApResolver stores these, and each time it is asked for one of those access points, it pops one from the list until empty. When empty, it transparently resolves again to refill the list. This should help with reconnection.

I made it a component until further notice, because it reuses http_client in Session (currently for this only, but soon also for SpClient and the CDN) as well as config to get the proxy configuration. Vice versa Mercury, the dealer and future SpClient can easily access this through self.session().apresolver() for their own connection logic.

Edit: also I shuffled around a bit in Session::connect and removed Session::create to get this to work. That is by no means final.

@roderickvd
Copy link
Member Author

In e589348, does anyone know why these clippy lints were disabled? Doing write() instead of write_all() seems only useful when you think that the write might not complete fully. Since these are all writes to Vec I don't see why or how that could ever happen.

Cursory testing with this change seems to work just fine.

@roderickvd
Copy link
Member Author

Next step is taking a bit more time. To implement SpClient, we need an X-Spotify-Connection-Id which is pushed over the dealer. So, I have to hook up the dealer web socket and do away with spirc.

I'm working on it. Up to this point the builds have been working, but the next commit won't be playing audio anymore. It will focus on getting registered as a Connect device under the new API, so we are visible in the player. Handling and dispatching messages will come after, then the CDN.

Afterwards there will be plenty other bells, whistles and todo's to cover, but this should be the general direction.

@roderickvd
Copy link
Member Author

Having thought more about the new code organisation, and fiddling with disconnecting spirc, I concluded it's necessary to merge the current dev in, which I'll be doing next.

Reason is that in the new API, the way spirc is set up now is out of place. It receives and dispatches most of the events, where session et al in core handle them over the network. But in the new API, that's all handed over to the dealer and spclient. There really isn't much reason to have a separate connect crate, it's all core.

@Johannesd3 foresaw this in #673, splitting up discovery which is basically just zeroconf. But new-api was branched early, and it's time to get up to speed.

@roderickvd roderickvd changed the title WIP: Implement new Spotify API client Lay groundwork for new Spotify API client Jun 25, 2021
@roderickvd roderickvd marked this pull request as ready for review June 25, 2021 22:34
@roderickvd
Copy link
Member Author

That messed up the commit log so let's get this in and open a new WIP PR for the next steps, if you'll all agree.

@roderickvd roderickvd merged commit 39bf40b into librespot-org:new-api Jun 28, 2021
@roderickvd roderickvd deleted the new-api-client branch June 28, 2021 18:59
roderickvd added a commit that referenced this pull request Jun 28, 2021
@roderickvd roderickvd restored the new-api-client branch June 28, 2021 19:41
@roderickvd roderickvd deleted the new-api-client branch June 28, 2021 20:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants