Skip to content
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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 5 commits into from
Jan 12, 2017
Merged

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

Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
542e04e
Allow for construction of `Reader` from any type `R: Read + Seek`
mitchmindtree Jan 10, 2017
879b412
Add missing error casts in Reader::new function
mitchmindtree Jan 10, 2017
e8a09da
Check for dedicated header reading errors rather than all error types.
mitchmindtree Jan 11, 2017
b004c04
Refactor `Reader::open` to use `Reader::new` internally.
mitchmindtree Jan 11, 2017
c840fad
Return as soon as possible on non-format related errors
mitchmindtree Jan 12, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
49 changes: 22 additions & 27 deletions src/read.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -114,8 +114,7 @@ pub struct Description {
pub enum ReadError {
Io(std::io::Error),
Reader(FormatError),
/// Provides the error from each format which which the read was attempted.
UnsupportedFormat { reader_errors: Vec<Box<std::error::Error>> }
UnsupportedFormat,
}

/// Errors that might be returned from the `open` function.
Expand Down Expand Up @@ -226,48 +225,45 @@ impl<R> Reader<R>
/// The format is determined by attempting to construct each specific format reader until one
/// is successful.
pub fn new(mut reader: R) -> Result<Self, ReadError> {
let mut reader_errors = Vec::new();

#[cfg(feature="wav")]
{
let maybe_err = match hound::WavReader::new(&mut reader) {
Ok(_) => None,
Err(err) => Some(err),
let is_wav = match hound::WavReader::new(&mut reader) {
Err(hound::Error::FormatError(_)) => false,
_ => true,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

};
try!(reader.seek(std::io::SeekFrom::Start(0)));
match maybe_err {
Some(err) => reader_errors.push(Box::new(err) as Box<std::error::Error>),
None => return Ok(Reader::Wav(try!(hound::WavReader::new(reader)))),
if is_wav {
return Ok(Reader::Wav(try!(hound::WavReader::new(reader))));
}
}

#[cfg(feature="flac")]
{
let maybe_err = match claxon::FlacReader::new(&mut reader) {
Ok(_) => None,
Err(err) => Some(err),
let is_flac = match claxon::FlacReader::new(&mut reader) {
Err(claxon::Error::FormatError(_)) => false,
_ => true,
};
try!(reader.seek(std::io::SeekFrom::Start(0)));
match maybe_err {
Some(err) => reader_errors.push(Box::new(err) as Box<std::error::Error>),
None => return Ok(Reader::Flac(try!(claxon::FlacReader::new(reader)))),
if is_flac {
return Ok(Reader::Flac(try!(claxon::FlacReader::new(reader))));
}
}

#[cfg(feature="ogg_vorbis")]
{
let maybe_err = match lewton::inside_ogg::OggStreamReader::new(&mut reader) {
Ok(_) => None,
Err(err) => Some(err),
let is_ogg_vorbis = match lewton::inside_ogg::OggStreamReader::new(&mut reader) {
Err(lewton::VorbisError::OggError(_)) |
Err(lewton::VorbisError::BadHeader(lewton::header::HeaderReadError::NotVorbisHeader)) => false,
_ => true,
};
try!(reader.seek(std::io::SeekFrom::Start(0)));
match maybe_err {
Some(err) => reader_errors.push(Box::new(err) as Box<std::error::Error>),
None => return Ok(Reader::OggVorbis(try!(lewton::inside_ogg::OggStreamReader::new(reader)))),
if is_ogg_vorbis {
return Ok(Reader::OggVorbis(try!(lewton::inside_ogg::OggStreamReader::new(reader))));
}
}

Err(ReadError::UnsupportedFormat { reader_errors: reader_errors })
Err(ReadError::UnsupportedFormat)
}

/// The format from which the audio will be read.
Expand Down Expand Up @@ -571,14 +567,14 @@ impl std::error::Error for ReadError {
match *self {
ReadError::Io(ref err) => std::error::Error::description(err),
ReadError::Reader(ref err) => std::error::Error::description(err),
ReadError::UnsupportedFormat { .. } => "no supported format was detected",
ReadError::UnsupportedFormat => "no supported format was detected",
}
}
fn cause(&self) -> Option<&std::error::Error> {
match *self {
ReadError::Io(ref err) => Some(err),
ReadError::Reader(ref err) => Some(err),
ReadError::UnsupportedFormat { .. } => None,
ReadError::UnsupportedFormat => None,
}
}
}
Expand Down Expand Up @@ -619,9 +615,8 @@ impl std::fmt::Display for ReadError {
match *self {
ReadError::Io(ref err) => err.fmt(f),
ReadError::Reader(ref err) => err.fmt(f),
ReadError::UnsupportedFormat { ref reader_errors } =>
write!(f, "{}: format readers produced these errors {:?}",
std::error::Error::description(self), reader_errors),
ReadError::UnsupportedFormat =>
write!(f, "{}", std::error::Error::description(self)),
}
}
}
Expand Down