Fix bmpcodec 16bpp decoding #276
Conversation
src/bmpcodec.c
Outdated
| return OutOfMemory; | ||
| bmi->bV5BlueMask = (data_read[3]<<24 | data_read[2]<<16 | data_read[1]<<8 | data_read[0]); | ||
|
|
||
| return Ok; | ||
| } |
There was a problem hiding this comment.
This doesn't sound right. I don't think the masks are even present there with BITMAPINFOHEADER when biCompression is BI_RGB. In that case it should use the default 16 bits per pixel values:
bmi->red_mask = 0x7C00;
bmi->green_mask = 0x3E0;
bmi->blue_mask = 0x1F;
There was a problem hiding this comment.
The cause of this issue was #285 which needs to be fixed (I'll do so separately)
Addressed this feedback though
tests/testtext
Outdated
| @@ -0,0 +1,210 @@ | |||
| #! /bin/sh | |||
There was a problem hiding this comment.
This is now .gitignored right? Shouldn't be part of the commit. :-)
src/bmpcodec.c
Outdated
| @@ -738,7 +750,7 @@ gdip_read_BITMAPINFOHEADER (void *pointer, ImageSource source, BITMAPV5HEADER *b | |||
| bmi->bV5ClrUsed = 0; | |||
| bmi->bV5ClrImportant = 0; | |||
|
|
|||
| return Ok; | |||
| goto fill_rgb_masks; | |||
There was a problem hiding this comment.
This looks kind of useless. There will never be masks since there's no compression and hence no BI_BITFIELDS (bV5Compression should be set to BI_RGB for clarity). And it will never be 16-bit since the allowed values for bcBitCount are only 1, 4, 8, and 24 (https://msdn.microsoft.com/en-us/library/windows/desktop/dd183372(v=vs.85).aspx).
src/bmpcodec.c
Outdated
| bmi->bV5GreenMask = 0x3E0; | ||
| bmi->bV5BlueMask = 0x1F; | ||
| return Ok; | ||
| } |
There was a problem hiding this comment.
These two blocks could probably be moved up to the "goto fill_rgb_masks" point and then the mask reading code could be shared instead of being duplicated. In fact, I think the if (bmi->bV5BitCount != 16) block is unnecessary.
src/bmpcodec.c
Outdated
| return OutOfMemory; | ||
| bmi->bV5BlueMask = (data_read[3]<<24 | data_read[2]<<16 | data_read[1]<<8 | data_read[0]); | ||
| } | ||
|
|
There was a problem hiding this comment.
Any reason why you don't fall through to the code below? Also, now the implicit BI_RGB masks from previous change are missing...
I meant something like
if (bmi->bV5Compression == BI_RGB) {
... // set the 16-bit 555 masks
return Ok;
}
and later on after bmi->bV5BlueMask = (data_read[3]<<24 | data_read[2]<<16 | data_read[1]<<8 | data_read[0]); (line 807 in original, 840 now) add something like
if (bmi->bV5Size == sizeof (BITMAPINFOHEADER))
return Ok;
There was a problem hiding this comment.
For the record I don't mind the code duplication if it makes the code more readable. It's matter of personal opinion and I find your version little more readable even though it's a bit verbose... but the BI_RGB part has to be fixed in either case.
|
Yeah these are the only 16bpp formats. I don't quite understand what happens yet with unknown masks because it seems a bit confusing: #292 |
Fixes #189
Fixes #157