search

RustAudio / audrey

A crate to simplify reading, writing and converting between a variety of audio formats.
Other
132 stars 16 forks source link

Allow for construction of `Reader` from any type `R: Read + Seek` #3

Closed mitchmindtree closed 7 years ago

mitchmindtree commented 7 years ago

This is a follow up to @est31's suggestion in #1 and is a follow on from an earlier attempt at something similar I tried when first starting the project.

The method attempts to detect the correct variant by attempting to construct each of the format-specific readers and return the first one that does not produce an error upon construction.

This feels a little hackish in that we depend on the upstream readers' to first "confirm" the format and early return an Err (rather than panic!ing or returning Ok anyway) if incorrect. Perhaps this is a reasonable expectation though?

Also, this method requires that the successfully detected inner format reader is constructed twice:

  1. By passing the reader argument via &mut to test for the format. We must use &mut to avoid moving the reader argument into the format reader's constructor, otherwise we would not be able to re-use the reader argument when testing the following formats.
  2. Passing the reader argument via move for constructing the Reader that will be returned after successfully detecting the format.

Perhaps a better/"proper" solution would be to dig a little deeper into each of the upstream crates in order to find functionality specifically suited to reading/confirming the header of each format? Or maybe it is safe to assume that this would be the first step of the format reader constructors anyway?

@est31 would be great to get your thoughts on all this!

mitchmindtree commented 7 years ago

We might be able to use this method to make the open function a little more robust. E.g. rather than depending on the file path extension to determine the format as is the case currently, we could use the extension as an indicator for which format to try first, and then continue to try for other formats if it fails?

est31 commented 7 years ago

Optimally it would check for a dedicated error by the upstream crates for when the magic fails. Lewton for example returns one of the two errors NoCapturePatternFound and NotVorbisHeader (one if its not an ogg stream, and one if its not an ogg/vorbis stream). Those errors are only returned if its a wrongly formatted file, not when e.g. the file claims to be ogg/vorbis but the data inside are inconsistent with the spec. I don't know about claxon/hound. @ruuda any ideas? maybe you could add such an error too?

ruuda commented 7 years ago

I think trying each format until one succeeds is a reasonable approach. Possibly a better way would be to read four bytes and match on those.

Both Hound and Claxon will immediately return a {hound, claxon}::Error::FormatError if the magic does not match.

mitchmindtree commented 7 years ago

Ok I think this might be good to go!

@est31 I took your advice and changed the new Reader::new function to check specifically for format-detection / header-related errors. I was unable to check specifically for the NoCaptureFoundPattern as it seems the OggReadError is not re-exported through lewton, so I settled for the VorbisError::OggError in that case.

I also added a commit that removes the old file extension parsing from Reader::open in favour of using the new Reader::new function internally.

r?