Skip to content

Speed up uncompressed DDS reader #9943

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.

Sign up for GitHub

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

Open
akx wants to merge 3 commits into
base: main
Choose a base branch
from
+43 −19
Open

Speed up uncompressed DDS reader #9943

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
5150c59
Benchmarks: split PATHS to LOAD_PATHS/SAVE_PATHS; benchmark uncompres…
akx Sep 2, 2026
dadbf4f
Speed up uncompressed DDS reading
akx Sep 2, 2026
dc4eeba
DDS: Directly use BGR rawmode when possible
radarhere Sep 15, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 8 additions & 4 deletions Tests/benchmarks.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,9 +45,13 @@

# For benchmarks that act on test fixture files, these are the paths loaded.
IMAGES_PATH = pathlib.Path(__file__).parent / "images"
PATHS = [
SAVE_PATHS = [
IMAGES_PATH / "flower2.jpg",
]
LOAD_PATHS = [
*SAVE_PATHS,
IMAGES_PATH / "uncompressed_rgb.dds",
]

@radarhere radarhere Sep 11, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

An alternative to splitting up PATHS would be to change test_save_jpeg to test_save, and forgo the quality argument.

Is there any reason not to do that?

@akx akx Sep 15, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Um... test_save_jpeg always saves a JPEG (into memory).

That would make it run the same thing for more than one image - is there a reason to do that?

@radarhere radarhere Sep 15, 2026 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, I meant saving both JPEG and DDS images - akx#31

@akx akx Sep 15, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then we could be instead testing saving with the default benchmark image, into different formats?

@radarhere radarhere Sep 15, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At this stage, I'm just wondering why it isn't helpful to test saving DDS images, since it would seem simpler to do so.

Let me ask a different question though - why use flower2.jpg, rather than hopper.jpg?

@akx akx Sep 15, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Of course it can be helpful to also benchmark writing DDS images, it just didn't feel in scope to add that in a PR that's speeding up DDS reading.
I can whip up another PR that does that more comprehensively (e.g. benchmark all common codecs, both reading and writing)?

As for why flower2.jpg, I can't recall I had a particular reason for choosing it over hopper.jpg in #9654.

@akx akx Sep 15, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There: #10001

@radarhere radarhere Sep 15, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you'd like to go forth with #10001, then sure, but don't do it for my sake. I was only trying to understand why there was a distinction being made between formats.


# These are derived from the other configuration, above.
RGB_MODES = [mode for mode in MODES if mode.startswith("RGB")]
Expand Down Expand Up @@ -567,7 +571,7 @@ def test_draw_lines_blend(


@pytest.mark.benchmark(group="load")
@pytest.mark.parametrize("path", PATHS, ids=_format_path)
@pytest.mark.parametrize("path", LOAD_PATHS, ids=_format_path)
def test_load(bench: BenchmarkFixture, path: pathlib.Path) -> None:
def run() -> None:
with Image.open(path) as im:
Expand All @@ -577,7 +581,7 @@ def run() -> None:


@pytest.mark.benchmark(group="save")
@pytest.mark.parametrize("path", PATHS, ids=_format_path)
@pytest.mark.parametrize("path", SAVE_PATHS, ids=_format_path)
def test_save_jpeg(bench: BenchmarkFixture, path: pathlib.Path) -> None:
with Image.open(path) as im:
im.load()
Expand Down Expand Up @@ -853,7 +857,7 @@ def test_quantize_grayscale_to_palette(
"source_type",
[
"synthetic",
*(pytest.param(image, id=f"{image.stem}") for image in PATHS),
*(pytest.param(image, id=f"{image.stem}") for image in LOAD_PATHS),
],
)
@pytest.mark.parametrize("palette_type", ["exact", "grayscale", "web"])
Expand Down
50 changes: 35 additions & 15 deletions src/PIL/DdsImagePlugin.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,6 @@

from . import Image, ImageFile, ImagePalette
from ._binary import i32le as i32
from ._binary import o8
from ._binary import o32le as o32

TYPE_CHECKING = False
Expand Down Expand Up @@ -371,8 +370,11 @@ def _open(self) -> None:
mask_count = 3

masks = struct.unpack(f"<{mask_count}I", header[84 : 84 + mask_count * 4])
self.tile = [ImageFile._Tile("dds_rgb", extents, 0, (bitcount, masks))]
return
if masks == (0xFF0000, 0x00FF00, 0x0000FF):
rawmode = "BGR"
else:
self.tile = [ImageFile._Tile("dds_rgb", extents, 0, (bitcount, masks))]
return
elif pfflags & DDPF.LUMINANCE:
if bitcount == 8:
self._mode = "L"
Expand Down Expand Up @@ -516,20 +518,38 @@ def decode(self, buffer: Image.DecoderInput) -> tuple[int, int]:
mask_totals.append(mask >> offset)

assert self.fd is not None
dest_length = self.state.xsize * self.state.ysize * len(masks)
while len(data) < dest_length:
bytes_read = self.fd.read(bytecount)
if len(bytes_read) < bytecount:
pixel_count = self.state.xsize * self.state.ysize

src = bytearray()
needed = pixel_count * bytecount
while len(src) < needed:
chunk = self.fd.read(min(needed - len(src), ImageFile.SAFEBLOCK))
if not chunk:
break
value = int.from_bytes(bytes_read, "little")
for i, mask in enumerate(masks):
masked_value = value & mask
src += chunk
pixel_count = len(src) // bytecount
src_length = pixel_count * bytecount
del src[src_length:]

nmasks = len(masks)
data = bytearray(pixel_count * nmasks)
for i, (mask, offset, total) in enumerate(
zip(masks, mask_offsets, mask_totals)
):
if not total: # Nothing to do here
continue
if total == 0xFF and offset % 8 == 0: # Whole byte, fast path
data[i::nmasks] = src[offset // 8 :: bytecount]
continue
values = (
int.from_bytes(src[p : p + bytecount], "little")
for p in range(0, src_length, bytecount)
)
data[i::nmasks] = bytes(
# Remove the zero padding, and scale it to 8 bits
data += o8(
int(((masked_value >> mask_offsets[i]) / mask_totals[i]) * 255)
if mask_totals[i]
else 0
)
int((((value & mask) >> offset) / total) * 255)
for value in values
)
self.set_as_raw(data)
return -1, 0

Expand Down