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: handle 0x00E1 / 0x00E0 segments from Pixel phones #84

Merged
merged 3 commits into from Jan 11, 2021

Conversation

AT41
Copy link
Contributor

@AT41 AT41 commented Jan 8, 2021

Original Issue: #82

I believe that the 0x00e1 bytes are not malformed, because in the 2nd picture of #82 (comment) the .jpg files already have an 0xffe1 segment. It seems to be a custom but unused segment, and it can be safely skipped over since the next 2 bytes indicate the size of this unused segment.

It might not be entirely related, but I took photos using a Pixel 4 XL emulator using Android Studios, and a similar "Unknown JPEG marker e0" exception error occurred when I tried to decode them.

This PR will skip over 0xe1 and 0xe0 segments while decoding.

@patrickhulce patrickhulce changed the title Fix to Issue #82 fix: handle 0x00E1 / 0x00E0 segments from Pixel phones Jan 9, 2021
Copy link
Collaborator

@patrickhulce patrickhulce left a comment

Choose a reason for hiding this comment

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

Thanks very much for the contribution @AT41 ! 🎉

because in the 2nd picture of #82 (comment) the .jpg files already have an 0xffe1 segment

This shouldn't be an indication of marker validity. Most images can and will have several APP1 segments of various types of metadata.

it can be safely skipped over since the next 2 bytes indicate the size of this unused segment.

For these particular pixel images they can be, but not generically. I'd prefer if the handler for these 0-padded markers would set a flag that referenced this recovery pattern.

I believe that the 0x00e1 bytes are not malformed

I'm not sure what to say here other than this file simply isn't a true valid JFIF :) (in several other ways as well, not just this one, there is no APP0 segment after SOI and no JFIF declared version either). Given the number of deviations from JFIF I might agree with your assessment that they're not trying to be valid and just shaving bytes wherever possible in a way that is still parseable by most decoders. The malformed segments in these example JPEGs are clearly supposed to be APP1 XML segments which are 0xFFE1 in the JFIF spec and the "unused bytes" label is the result of the way other JPEG decoders decide to handle the malformed segment, which is simply ignore it completely.

lib/decoder.js Outdated
@@ -813,7 +813,10 @@ var JpegImage = (function jpegImage() {
offset--;
}
break;

case 0xE0:
case 0xE1: // Unused bytes
Copy link
Collaborator

Choose a reason for hiding this comment

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

what about moving this into default block and something like...

// Recover from malformed APP1 markers popular in some phone models.
// See https://github.com/eugeneware/jpeg-js/issues/82
if (data[offset - 2] === 0x00 && data[offset - 1] === 0xE1) {
  const nextOffset = readUint16();
  if (data[nextOffset] === 0xFF) {
    offset = nextOffset;
    break;
  }
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Totally agree with you. In hindsight I should have seen that some of the files contained more than 1 0xFFE1 segment header and concluded that they aren't unique. Headers should also always start with 0xFF according to the JFIF standards, so you make a good point that 0x00E1 is definitely an error.

I've implemented the changes and added a new error that specifies the locations of 2 malformed headers like 0x00E1. Let me know if there's anything else that can be improved!

Copy link
Collaborator

@patrickhulce patrickhulce left a comment

Choose a reason for hiding this comment

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

LGTM, thanks again for the contribution and the flexibility in response to review! :)

@patrickhulce patrickhulce merged commit a2d7ed9 into jpeg-js:master Jan 11, 2021
zed-0xff pushed a commit to zed-0xff/jpeg-js that referenced this pull request Feb 24, 2023
* add unit test to reproduce jpeg-js#82

* Added new case to deal with e0 and e1 unused byte markers

* Popular invalid filemarkers now treated as exceptions

Co-authored-by: Hisham Al-Shurafa <halshura@gmail.com>
Co-authored-by: Anthony Tang <a.t.1592653@gmail.com>
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.

None yet

3 participants