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

Switch to slicing-by-8 CRC32 algorithm #37

Merged
merged 1 commit into from May 21, 2018

Conversation

Projects
None yet
3 participants
@enzo1982
Contributor

enzo1982 commented Apr 30, 2018

Hi all,

This PR replaces the single look-up-table CRC32 implementation with the slicing-by-8 algorithm. The new algorithm is approximately 5 times faster than single table lookup CRC.

This improves Ogg FLAC encoding speed by about 5% and decoding by 10%. Other codecs benefit too. Opus decoding is roughly 1%, Vorbis decoding about 2% faster. Pure muxing of already encoded material will probably benefit the most, but I did not test this.

Switching to a slicing algorithm was proposed before by Rodney Brown, but the discussion came to nothing after raising questions about the patent situation. I did a patent search and found the last of the relevant patents on this algorithm having expired in December 2017.

See here for my original article on this and here for the discussion on HydrogenAudio.

It would be great to see this added to the official Ogg source.

Robert

@tdaede

This comment has been minimized.

tdaede commented May 7, 2018

LGTM, thanks! I think we can live with the extra 7.75KiB table space, so no need to make it configurable. I'll take a bit to review the docs you linked before merging.

@enzo1982

This comment has been minimized.

Contributor

enzo1982 commented May 8, 2018

Great! Thanks for having a look at it!

The _ogg_crc_init in the #if 0 block is not meant as an option to configure dynamic initialization. It's just to illustrate how the table is generated, just like _ogg_crc_entry before. I think it would be good to keep it to take some magic away from the numbers.

@enzo1982

This comment has been minimized.

Contributor

enzo1982 commented May 21, 2018

@tdaede Any news on this one?

@xiph-mirror xiph-mirror merged commit bc82844 into xiph:master May 21, 2018

2 of 3 checks passed

continuous-integration/travis-ci/push The Travis CI build is in progress
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment