-
Notifications
You must be signed in to change notification settings - Fork 89
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
Introduces functionality for optionally returning incomplete pixel data #252
base: master
Are you sure you want to change the base?
Conversation
Currently, this library is rather rigid, with respect to the specification. If an error occurs while decoding the stream, the error is returned and recovery of the partially decoded image is impossible. However, there are some occasions where it may be possible, to return the potentially incomplete image data. In this case, the `try_recover` function may be used on the result of the `decode` function. If the data supports recovery, the incomplete pixels are returned, but if not, it simply returns the error. Added the `TryRecover` trait, the `Error::Recoverable` condition and implemented a simple example usage -- if an image does not have an EOI segment, it yields a recoverable error. Usage of the functionality is straightforward: ``` let mut decoder = Decoder::new(File::open("missing-eoi.jpg")?); // Bails with an IO error about UnexpectedEof // let pixels = decoder.decode()?; // Returns the incomplete pixel data of the image let pixels = decoder.decode().try_recover()?; ```
impl TryRecover for Result<Vec<u8>> { | ||
fn try_recover(self) -> Self { | ||
self.or_else(|err| match err { | ||
Error::Recoverable { pixels, .. } => { | ||
Ok(pixels) | ||
}, | ||
_ => Err(err), | ||
}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would rather provide this as an inherent method of Error
:
impl Error {
pub fn partial_recover(self) -> Result<Vec<u8>, Error>;
}
Each usage site can easily emulate the above trait by calling Result::{map_,}or_else
themselves which is even slightly more powerful by allowing other Ok
-types than Vec<u8>
.
@@ -291,7 +292,13 @@ impl<R: Read> Decoder<R> { | |||
loop { | |||
let marker = match pending_marker.take() { | |||
Some(m) => m, | |||
None => self.read_marker()?, | |||
None => match self.read_marker() { | |||
Err(e) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we really want to ingore all IO errors? see io::Error::kind()
and ErrorKind::UnexpectedEof
, I would start with a small set.
Recoverable { | ||
/// Error occurred while decoding the image | ||
err: Box<Self>, | ||
/// Incomplete pixel data of the image | ||
pixels: Vec<u8> | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the variant stored a struct RecoverableError
that contains the fields instead we could later add different error information and state in a SemVer compatible upgrade. And provide methods specifically for recoverable errors, such as listing of errors or a summary of their kinds / specificity. Nothing specific comes to mind but it's been quite useful to keep the option available.
Compare roughly design of image::ImageError
for some inspiration, maybe, but I don't want to cause rash complexification either. Simply these fields in a struct seems fine to me for the moment. That would answer the question of multiple errors by not having to answer it, yet 🙂
I've added my thoughts on idiomatic code style above but in terms of functionality, I do believe this is already decently covers a few cases. We clearly signal when a result is not standard compliant (except for implementation bugs) but have an interface to return the main decoding result as far as possible nevertheless. Finding a non-license encumbered test is always a hassle. However, as a guidance you can add a subfolder |
Thought I might start the conversation around a manner in which one could recover partially rendered image pixel data from a corrupt or partially invalid jpeg file.
Still to do / discuss:
Just wanted to throw out a minimally invasive rough draft, perhaps to start some thinking about things.
======================
Currently, this library is rather rigid, with respect to the specification.
If an error occurs while decoding the stream, the error is returned and
recovery of the partially decoded image is impossible.
However, there are some occasions where it may be possible, to return the
potentially incomplete image data. In this case, the
try_recover
functionmay be used on the result of the
decode
function. If the data supportsrecovery, the incomplete pixels are returned, but if not, it simply returns
the error.
Added the
TryRecover
trait, theError::Recoverable
condition andimplemented a simple example usage -- if an image does not have an EOI
segment, it yields a recoverable error.
Usage of the functionality is straightforward: