-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Add support for various BMP headers and color depths #1435
Conversation
Nice work. Looks good with a brief read, will try to review tomorrow. |
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.
Looks great!
src/bmp/BMPParser.cc
Outdated
} | ||
|
||
// The rest 48 bytes are ignored for LCS_WINDOWS_COLOR_SPACE and sRGB | ||
skip(48); |
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.
This also needs to be conditioned on the header size, right? (it could be 108 or 124 bytes at this point)
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.
I'm not sure what you mean. The only difference between V4 and V5 is that V5 has color management profiles, but they are located after the palette (and generally ignored), we already skip them in the line ptr = data + imgdOffset
, so neither color palette nor image data would be the problem. Or am I missing something?
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.
I could be mistaken, but what I meant here and on the prior comment is that I thought the V2 header is 4B smaller than the V3 header, so there should be no skip(4)
for V2 after parsing the mask for blue; and that V4 is 16B smaller than V5, so this skip is 32 for V4. Am I confused? You know the format better than I do now.
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.
That would affect only V1 and V2 images with color palette. Since we know that palette immediately follows DIB header, I simplified the code (now skip(4)
wouldn't affect parsing, but I removed it anyway).
so this skip is 32 for V4
It's not relevant anymore, but I think we would want to skip 48 for V4 and 64 for V5 (tested empirically with some images).
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.
There's a suite of BMPs here that includes one (q/rgb32h52.bmp) with a V2INFOHEADER: http://entropymine.com/jason/bmpsuite/ and http://entropymine.com/jason/bmpsuite/bmpsuite/html/bmpsuite.html and your parser works on that file, congrats 🙂! Firefox doesn't. Most images seem to work with just a few exceptions (g/rgb32.bmp for example, and a few I created with "advance settings" in Photoshop that cause "inconsistent image data size" messages).
I think I get what you're saying now: the skip()
doesn't matter because you seek to an absolute position with the ptr = data + imgdOffset
line right after. Previously it might have skipped into the palette but that didn't actually affect parsing. 👍
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.
Nice work!
Fixes #1433
Fixes #1436
Fixes #1437
Fixes #1438
All reported images are now parsed correctly: