Add support for various BMP headers and color depths #1435
Conversation
Hakerh400
changed the title
|
Nice work. Looks good with a brief read, will try to review tomorrow. |
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.
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.
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.
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.
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.
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. 👍
Fixes #1433
Fixes #1436
Fixes #1437
Fixes #1438
All reported images are now parsed correctly: