Make page CRC verification toggleable by end-users #44
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
These changes make the Ogg page CRC verification check toggleable at runtime by end-users via a new `PageParsingOptions` struct that may be passed at reader/parser construction time. I have paid special attention to making the new feature both backwards and forwards compatible, which led me to taking the following design choices: - No signature of any existing public method was modified. People looking into changing parsing behavior via the new `PageParsingOptions` struct must use the new `*_with_parse_opts` methods. - Marking `PageParsingOptions` as `#[non_exhaustive]`, so that we may add new public fields to it in the future without a semver-breaking change. Users must always default-initialize this struct. It only derives `Clone` too, in case we ever need to make it hold non-`Copy` types. - Shared ownership of the `PageParsingOptions` between different reader structs is achieved through `Arc`s, which ensures that no struct stops being `Send` or `Sync` with a negligble performance impact. The `fuzzing` cfg flag is still honored after these changes by using it to set a default value for the new `verify_hash` page parsing option accordingly.
We required it anyway due to recent changes.
|
This PR also bumps the MSRV to 1.61 to fix CI checks, as it is required for the current code to work anyway (the screenshot below is from
|
AlexTMjugador
added a commit
to OptiVorbis/OptiVorbis
that referenced
this pull request
Oct 12, 2024
Resolves #101. An OptiVorbis release with this feature is blocked until RustAudio/ogg#44 gets merged and an `ogg` release with it is published, as patched dependencies can't be used on `crates.io`.
AlexTMjugador
added a commit
to OptiVorbis/OptiVorbis
that referenced
this pull request
Oct 12, 2024
Resolves #101. An OptiVorbis release with this feature is blocked until RustAudio/ogg#44 gets merged and an `ogg` release with it is published, as patched dependencies can't be used on `crates.io`.
AlexTMjugador
added a commit
to OptiVorbis/OptiVorbis
that referenced
this pull request
Oct 12, 2024
Resolves #101. An OptiVorbis release with this feature is blocked until RustAudio/ogg#44 gets merged and an `ogg` release with it is published, as patched dependencies can't be used on `crates.io`.
est31
approved these changes
Oct 13, 2024
|
Thanks for the quick review and merge! 🚀 I was wondering if there are any plans to publish a release with these changes soon? I'd love to try them out on a new OptiVorbis release too, but no worries if we have to wait a little longer 😉 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
These changes make the Ogg page CRC verification check toggleable at runtime by end-users via a new
PageParsingOptionsstruct that may be passed at reader/parser construction time. I have paid special attention to making the new feature both backwards and forwards compatible, which led me to taking the following design choices:PageParsingOptionsstruct must use the new*_with_parse_optsmethods.PageParsingOptionsas#[non_exhaustive], so that we may add new public fields to it in the future without a semver-breaking change. Users must always default-initialize this struct. It only derivesClonetoo, in case we ever need to make it hold non-Copytypes.PageParsingOptionsbetween different reader structs is achieved throughArcs, which ensures that no struct stops beingSendorSyncwith a negligble performance impact.The
fuzzingcfg flag is still honored after these changes by using it to set a default value for the newverify_checksumpage parsing option accordingly.The need for these changes has been brought forward in OptiVorbis/OptiVorbis#101.