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

[mitchmindtree]

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]

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]

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]

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]

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?

[est31]

There can be a normal IO error too, which should be propagated upwards.

[mitchmindtree]

Remember we attempt to construct the WavReader again immediately after this line during which any IO errors that might have occurred in this previous call would likely occur again and be propagated upwards via the try! macro. Do you feel this is sufficient? Or would you prefer I add another branch here for returning the IO error as soon as possible?

[est31]

Oh right, we construct them twice.... Maybe it would be a good idea to re-use it? E.g. if its Ok(s) we return Reader::Wav(s) and be happy, and otherwise we either propagate the error (e.g. for io errors), or we continue (if the error was about the wrong format).

Just an anecdote, I had some errors in my HDD, and every time I wanted to read a specific file, it first hanged for some seconds and then errored. If we ignore IO errors and retry multiple times with different formats this means a much longer period to wait.

[mitchmindtree]

Oh right, we construct them twice.... Maybe it would be a good idea to re-use it?

The reason we have to construct it twice is due to an issue with ownership - I mention this in the parent comment:

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.

If we ignore IO errors and retry multiple times with different formats this means a much longer period to wait

True, this is a fair point - I'll add an extra branch that ensures we return immediately if we get any errors other than format-related errors.

[mitchmindtree]

@est31 I added a commit that changes the behaviour to return as soon as possible on non-format related errors.

[est31]

The reason we have to construct it twice is due to an issue with ownership - I mention this in the parent comment:

Oh I see. I guess we can merge it then and try to fix it later on.

[mitchmindtree]

Yeah I certainly agree it's not particularly nice, but as a temporary solution it should generally work fine while we can think about coming up with something better in the meantime - perhaps matching on the FourCC like @ruuda suggests.