Added reading DDS BGR;15 images #7574

radarhere · 2023-11-27T08:27:15Z

Resolves #7517

The issue found a DDS image with masks that mean the image is BGR;15.

I've created a test image using a modified Pillow.

akx · 2023-11-29T07:24:17Z

src/PIL/DdsImagePlugin.py

                self._mode = "RGB"
-            rawmode += masks[0xFF0000] + masks[0xFF00] + masks[0xFF]
+            if masks[:3] == (31744, 992, 31) and self.mode == "RGB":


The magic numbers here could use some explanation if you ask me.

I know what you're said sounds like a perfectly straightforward request, but I can't think of a short way to explain it that isn't just explaining what masks are, or what BGR;15 is.

There are 4 bit masks - https://learn.microsoft.com/en-us/windows/win32/direct3ddds/dds-pixelformat - R, G, B and A.

Each mask filters the pixel values to transform them into the value for each channel.

These masks are filtering 5 bits for R, 5 bits for G and 5 bits for B - BGR;15.

31744 = 0b0111110000000000 992 = 0b0000001111100000 31 = 0b0000000000011111

That looks like great material for a code comment right there :)

You could also replace the decimal constants with those binary constants – then it's immediately more clear for a reader.

Clearer, yes, it just feels verbose... I've pushed a commit.

akx · 2023-11-29T07:25:26Z

src/PIL/DdsImagePlugin.py

+            if masks[:3] == (31744, 992, 31) and self.mode == "RGB":
+                rawmode = "BGR;15"
+            else:
+                masks = {mask: ["R", "G", "B", "A"][i] for i, mask in enumerate(masks)}


Suggested change

masks = {mask: ["R", "G", "B", "A"][i] for i, mask in enumerate(masks)}

masks = {mask: "RGBA"[i] for i, mask in enumerate(masks)}

would be more efficient, since the (possibly-mutable-but-the-interpreter-can't-tell) list doesn't need to regenerated for every call of this function, but it could use the same "RGBA" string constant used just below.

I've added a version of this.

akx · 2023-11-29T09:02:00Z

src/PIL/DdsImagePlugin.py

+            # Each channel is derived using a mask of the pixel values
+            masks = {
+                mask: "RGB"[i]
+                for i, mask in enumerate(struct.unpack("<3I", header.read(12)))


The previous version always read 16 bytes for masks – this will now read either 12 or 16 bytes. That seems odd?

It isn't always necessary to read the last 4 bytes, and therefore seems more efficient to not do so where possible.

If you are concerned about the file pointer position, that's not relevant, as it is just reading from

Pillow/src/PIL/DdsImagePlugin.py

Line 127 in 697c24b

header = BytesIO(header_bytes)

I think it would actually be more efficient and clearer to not do another read-and-struct-unpack? (Would also mean that the entire DDS_PIXELFORMAT is always consumed.)

akx · 2023-11-29T09:03:15Z

src/PIL/DdsImagePlugin.py

+                rawmode = (
+                    masks[0b0000000000011111]
+                    + masks[0b0000001111100000]
+                    + masks[0b0111110000000000]
+                    + ";15"
+                )


Will this now support esoteric formats like GRB;15 (and if so, will everything be handled correctly down the line)? ~~If not, then I think just "RGB;15" would be better?~~

EDIT: right, the whole point here was BGR;15 – my bad, too little coffee.

This will support RGB;15 and BGR;15

Pillow/src/libImaging/Unpack.c

Lines 1576 to 1577 in 697c24b

{"RGB", "RGB;15", 16, ImagingUnpackRGB15},

{"RGB", "BGR;15", 16, ImagingUnpackBGR15},

akx · 2023-11-29T09:05:04Z

src/PIL/DdsImagePlugin.py

+                for i, mask in enumerate(struct.unpack("<3I", header.read(12)))
+            }
+            if self.mode == "RGB" and all(
+                mask in (0b0000000000011111, 0b0000001111100000, 0b0111110000000000)


This did become less clear to read, you're right. Wonder if these and the 0xFF0000 masks from below could benefit from becoming constants á la (I didn't double check whether I have RGB/BGR right here):

R_MASK_555 = 0b0000000000011111 G_MASK_555 = 0b000001111100000 B_MASK_555 = 0b111110000000000 R_MASK_888 = 0x000000FF G_MASK_888 = 0x0000FF00 B_MASK_888 = 0x00FF0000 A_MASK_888 = 0xFF000000

akx · 2023-11-29T09:05:48Z

src/PIL/DdsImagePlugin.py

+                # Try 8 bits for each mask
+                if self.mode == "RGBA":
+                    # The fourth mask, alpha,
+                    # is only valid when the correct flags are present


What are the correct flags?

https://learn.microsoft.com/en-us/windows/win32/direct3ddds/dds-pixelformat seems inconsistent on this to me

DDPF_ALPHAPIXELS | Texture contains alpha data; dwRGBAlphaBitMask contains valid data.

Alpha mask for reading alpha data. dwFlags must include DDPF_ALPHAPIXELS or DDPF_ALPHA

The code is checking for DDPF_ALPHAPIXELS

Pillow/src/PIL/DdsImagePlugin.py

Line 150 in aa03c5a

if not pfflags & DDPF_ALPHAPIXELS:

akx · 2023-11-29T09:06:33Z

src/PIL/DdsImagePlugin.py

+                    + ";15"
+                )
+            else:
+                # Try 8 bits for each mask


What's the trying here? Rather, what happens if trying fails?

Given that the issue related to this issue has found 5 bit masks, it seems possible that there could be other mask values used in files, beyond 5 bit and 8 bits.

If it fails, an error is raised, and ultimately an UnidentifiedImageError.

akx

A couple comment suggestions based on the discussion, to make this clearer for future Pillow archaeologists without having to look at PRs :)

akx · 2023-11-29T10:16:36Z

src/PIL/DdsImagePlugin.py

+                    + ";15"
+                )
+            else:
+                # Try 8 bits for each mask


Suggested change

# Try 8 bits for each mask

# Assume 8 bits for each mask

akx · 2023-11-29T10:16:48Z

src/PIL/DdsImagePlugin.py

+                # Try 8 bits for each mask
+                if self.mode == "RGBA":
+                    # The fourth mask, alpha,
+                    # is only valid when the correct flags are present


Suggested change

# is only valid when the correct flags are present

# is only valid when `pfflags & DDPF_ALPHAPIXELS`

radarhere · 2023-11-30T10:37:07Z

My initial version of this was PR just added support for the specific scenario of the issue.

However, the second version with more explanation of how the code works makes it clear that this solution is incomplete, so I'm closing this in favour of a complete solution in #7589

Added reading BGR;15 images

4ae3ce2

radarhere force-pushed the dds_bgr15 branch from 677d549 to 4ae3ce2 Compare November 27, 2023 08:27

radarhere mentioned this pull request Nov 27, 2023

PIL.UnidentifiedImageError: cannot identify image file (DDS file) #7517

Closed

akx reviewed Nov 29, 2023

View reviewed changes

Allow reordering of 5-bit masks

aa03c5a

radarhere force-pushed the dds_bgr15 branch from 91a5b05 to aa03c5a Compare November 29, 2023 08:56

akx reviewed Nov 29, 2023

View reviewed changes

radarhere closed this Nov 30, 2023

radarhere deleted the dds_bgr15 branch November 30, 2023 10:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added reading DDS BGR;15 images #7574

Added reading DDS BGR;15 images #7574

radarhere commented Nov 27, 2023

akx Nov 29, 2023

radarhere Nov 29, 2023 •

edited

Loading

akx Nov 29, 2023

radarhere Nov 29, 2023

akx Nov 29, 2023

radarhere Nov 29, 2023

akx Nov 29, 2023

radarhere Nov 29, 2023

akx Nov 29, 2023

akx Nov 29, 2023 •

edited

Loading

radarhere Nov 29, 2023

akx Nov 29, 2023

akx Nov 29, 2023

radarhere Nov 29, 2023

akx Nov 29, 2023

radarhere Nov 29, 2023

akx left a comment

akx Nov 29, 2023

akx Nov 29, 2023

radarhere commented Nov 30, 2023

	masks = {mask: ["R", "G", "B", "A"][i] for i, mask in enumerate(masks)}
	masks = {mask: "RGBA"[i] for i, mask in enumerate(masks)}

	{"RGB", "RGB;15", 16, ImagingUnpackRGB15},
	{"RGB", "BGR;15", 16, ImagingUnpackBGR15},

	# is only valid when the correct flags are present
	# is only valid when `pfflags & DDPF_ALPHAPIXELS`

Added reading DDS BGR;15 images #7574

Added reading DDS BGR;15 images #7574

Conversation

radarhere commented Nov 27, 2023

Choose a reason for hiding this comment

radarhere Nov 29, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akx Nov 29, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akx left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

radarhere commented Nov 30, 2023

radarhere Nov 29, 2023 •

edited

Loading

akx Nov 29, 2023 •

edited

Loading