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.

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

Fix another "signed byte" bug introduced in #234 #243

Merged
merged 1 commit into from
Feb 15, 2017

Conversation

Nadahar
Copy link
Contributor

@Nadahar Nadahar commented Feb 14, 2017

This fixes the bug reported in #242. I've tested it against the submitted image and it now parses correctly. I've also tested it with no diffs against the images repository.

I'm really sorry for this, I've looked through it and I can't see any more of these now. It's extra bad that a release took place with this bug in it.

@Nadahar Nadahar changed the title Fix "signed byte" bug introduced in #234 Fix another "signed byte" bug introduced in #234 Feb 14, 2017
@drewnoakes drewnoakes merged commit bb05e9e into drewnoakes:master Feb 15, 2017
@drewnoakes
Copy link
Owner

Thanks very much for the fix. Don't worry about the bug -- it happens. Probably should have given it more time to mature, but it's fine. I'll get a minor release out with the fix. I'll add the sample image to the repo too.

@Nadahar
Copy link
Contributor Author

Nadahar commented Feb 15, 2017

@drewnoakes You should add this as well, it has a DNL segment. The DNL segment won't be parsed now because the reader stops too early, but it will be parsed if the reader reads it.

beach

@drewnoakes
Copy link
Owner

Thanks. Does it matter that the image fails to decode for that attachment?

@Nadahar
Copy link
Contributor Author

Nadahar commented Feb 15, 2017

@drewnoakes DNL doesn't seem to be very well supported, Firefox for sure doesn't decode it and even Photoshop won't (it just says DNL not supported). I haven't searched for a viewer that can display it so I've never actually seen the image, but it really doesn't matter. I've inspected it with JPEGsnoop and it seems to be valid according to the specs. Metadata-extractor will also parse it just fine as it is now, except that the height is reported to be 0 since the DNL segment is never read. When the reader reads all the file, the DNL segment is processed and the correct height is set.

DNL is part of the JPEG spec, and it was easy to implement, so why not..?

@drewnoakes
Copy link
Owner

Sounds reasonable. Hope the image isn't indecent :) I'll try to decode it somehow and add it to the library. Seems like a great candidate for testing of the improved JPEG processing in the pipeline.

@Nadahar
Copy link
Contributor Author

Nadahar commented Feb 16, 2017

@drewnoakes It's not easy to find a DNL-capable viewever since they don't inform on such details. You'd simply have to try, so I figured a hex editor was quicker. Here's the image with the length from the DNL segment edited into the frame header:

beach

I think it should be within the bounds of decency 😉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants