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

Remove with-vorbis and with-tremor features #750

Merged
merged 3 commits into from
May 26, 2021

Conversation

roderickvd
Copy link
Member

As discussed in #648, recently put forward by @sashahilton00 in #734 and helpful in #727.

This may ruffle some feathers. As far as I know, LibreELEC compiles with-vorbis [1] and Spotifyd with-tremor [2]. On both accounts I don't see why. For LibreELEC I asked @heitbaum over at #681:

I just checked and compiling with-vorbis only saves 167 kB. Likewise the slight boost in performance will be neglible. At the same time, the bindings are unmaintained and only pass integer samples instead of the floats they were originally in, causing quantization error and lower sound quality. Finally, there is no Rust-unsafe code in lewton. So there's a lot to say about moving to lewton and nothing of significance for keeping libvorbis as I can see.

As for lewton, the lowest-performance hardware that librespot targets has recently been set to be Raspberry Pi Zero. Which has hard float support and so has no need for tremor. In fact even the original RPi v1 had hard float support so I can't imagine which hardware is targeted here.

Please chime in if you think this would be a bad idea.

[1] https://github.com/LibreELEC/LibreELEC.tv/blob/master/packages/addons/service/librespot/package.mk
[2] https://github.com/Spotifyd/spotifyd/blob/master/Cargo.toml

@roderickvd roderickvd changed the title Remove vorbis and tremor features Remove with-vorbis and with-tremor features May 18, 2021
@Johannesd3
Copy link
Contributor

For reference: Spotifyd/spotifyd#37 (comment). But I don't think this applies nowadays.

@roderickvd
Copy link
Member Author

Yes if the sole reason was to workaround a bug in lewton back in 2017, then obviously that has been fixed since then.

@roderickvd
Copy link
Member Author

Failing on apresolve::test::test_apresolve_port_443 is a new one. Probably transitive? Will go ahead with merging if the others pass.

@Johannesd3
Copy link
Contributor

Seems like a dns error

@roderickvd roderickvd merged commit 11dfede into librespot-org:dev May 26, 2021
@roderickvd roderickvd deleted the remove-vorbis-and-tremor branch May 26, 2021 19:43
@plietar
Copy link
Contributor

plietar commented Jun 1, 2021

the lowest-performance hardware that librespot targets has recently been set to be Raspberry Pi Zero. Which has hard float support and so has no need for tremor. In fact even the original RPi v1 had hard float support so I can't imagine which hardware is targeted here.

I think I added tremor in order to support some MIPS devices that did not have hardware floating point. I'm not at all saying it should be kept, just wanted to add some context.

@heitbaum
Copy link

This may ruffle some feathers. As far as I know, LibreELEC compiles with-vorbis [1] and Spotifyd with-tremor [2]. On both accounts I don't see why. For LibreELEC I asked @heitbaum over at #681:

I have just raised the PR for LIbreELEC for 0.3.1 - just asked our forum for someone to test. All updated with your vorbis cleanups 👍 LibreELEC/LibreELEC.tv#5891 and LibreELEC/LibreELEC.tv#5892

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking includes a breaking change dependencies
Projects
None yet
4 participants