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: don’t fail if scan contains trailing bytes #66

Merged
merged 2 commits into from Apr 21, 2020

Conversation

rluba
Copy link
Contributor

@rluba rluba commented Mar 4, 2020

Here's another PR for an issue I found: Some JPEGs contain trailing bytes at the end of a scan. All other jpeg programs and libraries I’ve tested (including the Indepdendend JPEG group’s decoder or stb_image) simply skip these bytes.

This PR changes the decoder to skip trailing bytes at the end of a scan instead of failing with a "maker was not found" error.

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 @rluba!

// Skip trailing bytes at the end of the scan - until we reach the next marker
do {
if (data[offset] === 0xFF) {
if (data[offset + 1] !== 0x00) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

mind walking me through this one? :)

if we're intending on being loose AFAICT we'd be fine just checking for data[offset] === 0xFF?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

According to the spec, any FF followed by 00 is not a marker. Since I don’t have enough examples to know what the heck encoders might use as fill bytes, I want to be sure that I’m not stopping before finding an actual marker.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But the code could be more concise. It’s a leftover from having more conditions there before arriving at this version.

Copy link
Collaborator

Choose a reason for hiding this comment

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

it's definitely not a marker, but I was under the impression it was filler data. it is handled appropriately in the rest of jpeg-js fwiw

but we basically cycle through it just like we would here, so it wouldn't really have a practical effect other than consistency with the code below

Copy link
Collaborator

Choose a reason for hiding this comment

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

maybe we make a note of this conversation in a comment or change it so we don't wonder in the future why FF00 is a valid marker on line 336 but not here? :)

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.

comment wasn't a big deal, LGTM thanks!

@patrickhulce patrickhulce merged commit cfeb1c7 into jpeg-js:master Apr 21, 2020
zed-0xff pushed a commit to zed-0xff/jpeg-js that referenced this pull request Feb 24, 2023
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

2 participants