pcwalton / rust-media

A free, comprehensive, and portable video/audio streaming library for Rust
Apache License 2.0
787 stars 57 forks source link

Use lewton instead of libvorbis and some little improvements #30

Closed est31 closed 7 years ago

est31 commented 7 years ago

I didn't exchange libogg with the ogg crate yet because of API restrictions. I need to come up with a better concept for this upstream.

est31 commented 7 years ago

Out of similar reasons as to why I didn't use the ogg crate, I didn't do a PR for the gif crate yet.

pcwalton commented 7 years ago

This looks good to me, but I'd like sign-off from a Vorbis expert.

@metajack Do you have thoughts on lewton?

est31 commented 7 years ago

A few points about lewton:

metajack commented 7 years ago

This looks a lot simpler, and I'm a big fan of the removal of most unsafe code. We should make sure we are opting into the ieee754 feature when used.

Also, we should probably have some unit tests that decode simple files just as a sanity check.

I'd love to see benchmarks of this vs. libvorbis so we know what optimization work needs to be done down the road.

est31 commented 7 years ago

@metajack when I run the benchmark tool I described in the post above, I get (these files are automatically downloaded from fairly stable locations):

Comparing speed for bwv_1043_vivace.ogg : libvorbis=0.8301s we=1.0253s difference=1.24x
Comparing speed for bwv_543_fuge.ogg    : libvorbis=1.3267s we=1.6383s difference=1.23x
Comparing speed for maple_leaf_rag.ogg  : libvorbis=0.3890s we=0.4762s difference=1.22x
Comparing speed for hoelle_rache.ogg    : libvorbis=0.6277s we=0.7707s difference=1.23x
Comparing speed for thingy-floor0.ogg   : libvorbis=0.3503s we=0.2717s difference=0.78x

Overall time spent for decoding by libvorbis: 3.5238s
Overall time spent for decoding by us: 4.1823s
Overall ratio of difference: 1.19x
est31 commented 7 years ago

@metajack I've enabled the ieee754 feature as requested.

About the sanity test unit tests: Where do you want them, here or in the lewton repo, because there are already such test files inside the lewton repo (you cd into dev/cmp then run cargo test --release)?

pcwalton commented 7 years ago

I think here is a good place for the tests.

metajack commented 7 years ago

I'm totally fine with 20% perf regression in exchange for safety. We can optimize lewton if needed.

I would like some sanity tests here, but they can be cursory since lewton is already also testing. Mostly I want to protect against the error case where rust-media still builds but doesn't actual work; that situation is common on Servo for android where we don't have any smoke tests.

est31 commented 7 years ago

Added an unit test. I couldn't use the ogg container decoder from rust-media as it isn't exposed via the high level API yet.

pcwalton commented 7 years ago

Awesome, thanks!