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

lewton_decoder as default, libvorbis_decoder as optional #124

Merged
merged 3 commits into from
Feb 8, 2018

Conversation

awiouy
Copy link
Collaborator

@awiouy awiouy commented Feb 6, 2018

Switch lewton_decoder as default and libvorbis_decoder as optional after #123
Works fine here

@awiouy awiouy force-pushed the lewton_as_default branch from 86946d0 to 5d5e081 Compare February 7, 2018 06:30
@plietar
Copy link
Contributor

plietar commented Feb 7, 2018

cc @est31

@sashahilton00
Copy link
Member

Are there any upsides/downsides to doing this? Presumably libvorbis is more mature?

@est31
Copy link

est31 commented Feb 7, 2018

Upsides:

  • safety. There have been multiple CVEs against libvorbis. lewton on the other hand doesn't use a single line of unsafe code.
  • no need for a C compiler

Downsides:

  • lewton needs to be compiled in release mode in order to have comparable speed to libvorbis. lewton still is faster than realtime in debug mode, but be prepared for startled users :).
  • yes libvorbis has been around longer and has less bugs. If you find any bug in lewton, please report it!

@sashahilton00
Copy link
Member

Thanks for clarifying. Sounds like we could make the switch to lewton as default without too much impact then. We may also want to consider switching the default build mode to release if we do make lewton the default, depending on the speed, but given that librespot is eventually to be the behind-the-scenes library for the daemon, it's probably unnecessary. Thoughts?

@plietar
Copy link
Contributor

plietar commented Feb 7, 2018

We may also want to consider switching the default build mode to release

AFAIK there isn't a way of changing what cargo build does. The best we can do is use --release in the README (which is already the case).

There's a couple of other places in librespot which are pretty slow in debug mode (generating DH keys during handshake for example takes a handful of seconds on x86_64, and likely a lot more on low end devices) so this isn't really a concern to me.

@awiouy awiouy force-pushed the lewton_as_default branch 2 times, most recently from a3fc178 to ade0797 Compare February 7, 2018 20:01
@awiouy
Copy link
Collaborator Author

awiouy commented Feb 7, 2018

I have rebased this against current #123

It now builds as follows:

  • neither with-tremor nor with-vorbis: lewton
  • with-tremor: tremor
  • with-vorbis: vorbis
  • both with-tremor and with-vorbis: tremor

In summary:

  • tremor is implemented using fixed point arithmetic, which is suitable for devices without hardfloat support
  • lewton and vorbis are alternatives for other devices

Chosing between lewton and vorbis:
Many pros and cons have been presented above. For me, the main advantage of lewton over vorbis, is that lewton requires far less dependencies, especially system dependencies such as gcc, ogg..sys and vorbis..sys. Moreover, lewton has a very reactive support, see above and #123.

@awiouy awiouy force-pushed the lewton_as_default branch from ade0797 to f400a89 Compare February 7, 2018 23:25
@sashahilton00
Copy link
Member

If no one objects, I'll merge this in a couple of hours and update the docs.

@sashahilton00 sashahilton00 merged commit bd59ded into librespot-org:master Feb 8, 2018
@awiouy awiouy deleted the lewton_as_default branch June 13, 2018 15:42
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.

4 participants