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

Fix: don’t fail if scan contains trailing bytes #66

Merged
merged 2 commits into from
Apr 21, 2020
Merged

Fix: don’t fail if scan contains trailing bytes #66

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3c1a08a
Add failing test for decoding an image with trailing scan bytes
rluba Mar 4, 2020
d550fa7
Fix: don’t fail if scan unit contains trailing bytes
rluba Mar 4, 2020
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
12 changes: 12 additions & 0 deletions lib/decoder.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -319,6 +319,18 @@ var JpegImage = (function jpegImage() {
}
}

if (mcu === mcuExpected) {
// 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? :)

break;
}
}
offset += 1;
} while (offset < data.length - 2);
}

// find marker
bitsCount = 0;
marker = (data[offset] << 8) | data[offset + 1];
Expand Down
Binary file added test/fixtures/redbox-with-trailing-bytes.jpg
View file
Open in desktop
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
9 changes: 9 additions & 0 deletions test/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,15 @@ it('should be able to decode a JPEG with RST intervals', function(t) {
t.end();
});

it('should be able to decode a JPEG with trailing bytes', function(t) {
var jpegData = fixture('redbox-with-trailing-bytes.jpg');
var rawImageData = jpeg.decode(jpegData);
var expected = fixture('redbox.jpg');
var rawExpectedImageData = jpeg.decode(expected);
t.deepEqual(rawImageData.data, rawExpectedImageData.data);
t.end();
});

it('should be able to decode a grayscale JPEG', function(t) {
var jpegData = fixture('apsara.jpg');
var rawImageData = jpeg.decode(jpegData);
Expand Down