-
Notifications
You must be signed in to change notification settings - Fork 125
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
feat(decoder): add FFDC marker support #79
Conversation
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.
thanks very much for the fix and a test case 🎉 😃
just one question on if numberOfLines
is actually worth exposing
lib/decoder.js
Outdated
@@ -782,6 +782,11 @@ var JpegImage = (function jpegImage() { | |||
resetInterval = readUint16(); | |||
break; | |||
|
|||
case 0xFFDC: // Number of Lines marker | |||
readUint16() // skip data length | |||
this.numberOfLines = readUint16() |
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.
is this number ever different from the height of the image?
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.
actually it is a pretty worthless information, but I felt bad checking a data and ignoring it, I guess we can ignore it since this marker is deprecated
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.
Switched to just ignoring the marker, thank you for your review!
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.
LGTM after revert of the extra test changes :)
test/index.js
Outdated
@@ -1,141 +1,143 @@ | |||
var fs = require('fs'), |
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.
revert all these changes?
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.
Oh right i'm sorry, format on save option... reverted now
many thanks @Chupsy! 🎉 |
Hi,
First of all, thank you for all you hard work on this project, easy to use and very clear to understand.
This PR aims to fix an issue where files using the FFDC marker were refused with the error "Unknown JPEG marker ffdc". Even though it is a rare marker, which simply gives another info of the height of the file, I added the handling and tests for this marker.
Thank you for your time reviewing this PR