Skip to content

Fix ICO save producing an empty file for images smaller than 16x16 - #9745

Closed
binggao1230 wants to merge 1 commit into
python-pillow:mainfrom
binggao1230:fix-ico-save-small-image-empty
Closed

binggao1230 wants to merge 1 commit into
python-pillow:mainfrom
binggao1230:fix-ico-save-small-image-empty

Conversation

@binggao1230

Copy link
Copy Markdown
Contributor

Saving an image smaller than the smallest default ICO size (16×16) as ICO, without an explicit sizes argument, silently produces an empty, unreadable file:

import io
from PIL import Image

buf = io.BytesIO()
Image.new("RGBA", (8, 8), (10, 20, 30, 255)).save(buf, "ICO")
print(len(buf.getvalue()))          # 6  -> just the ICONDIR header, idCount=0
buf.seek(0)
Image.open(buf).load()              # UnidentifiedImageError: cannot identify image file

The 8×8 image is representable as an ICO (save(buf, "ICO", sizes=[(8, 8)]) works fine), but the default-sizes path drops it entirely with no error or warning.

Cause

The default sizes start at (16, 16), and _save skips every candidate size larger than the source image. For an image smaller than 16×16 every default candidate is skipped, so frames stays empty and o16(len(frames)) writes idCount=0 — a 6-byte header-only file.

Fix

Filter the requested sizes up front, and if none fit, fall back to the image's own size (capped at the 256×256 ICO maximum) so a valid, readable icon is always written. This also fixes the related case where every explicitly requested size is larger than the image (previously also an empty file) — consistent with the documented behaviour that oversized requested sizes are ignored.

Tests

test_save_smaller_than_default_sizes parametrizes (1, 1), (8, 8), (15, 15) and the non-square (8, 12), asserting each round-trips to a valid ICO of the right size with info["sizes"] and a corner pixel preserved. test_save_all_sizes_larger_than_image pins the explicit-sizes fallback. All fail on main with UnidentifiedImageError and pass with the fix; the full Tests/test_file_ico.py suite stays green (27 passed); ruff format, ruff check and mypy are clean.

Saving an image smaller than the smallest default size (16x16) as ICO,
without an explicit sizes argument, wrote a header-only 6-byte file with
zero icon entries. Reopening it raised UnidentifiedImageError and the image
was lost silently.

The default sizes start at 16x16 and the save loop skips every candidate
larger than the source image, so for a sub-16 image all candidates are
skipped and no frame is written. Filter the requested sizes up front and, if
none fit, fall back to the image's own size (capped at the 256x256 ICO
maximum) so a valid, readable icon is always written. This also covers the
case where every explicitly requested size is larger than the image.
@codspeed

codspeed Bot commented Jun 30, 2026

Copy link
Copy Markdown

Merging this PR will not alter performance

✅ 331 untouched benchmarks


Comparing gaoflow:fix-ico-save-small-image-empty (5bb1b35) with main (6590b1b)

Open in CodSpeed

@radarhere

Copy link
Copy Markdown
Member

This also fixes the related case where every explicitly requested size is larger than the image

I'm not convinced on this point. If the user requests a specific sets of sizes, and we can't provide them, I think an error should be raised, rather than silently giving the user back something other than what they requested. I think the expectation in #2266 agrees with me.

I've created #9766 as an alternative.

@binggao1230

Copy link
Copy Markdown
Contributor Author

You're right about the explicit case — if the caller names sizes we can't provide, raising beats silently handing back something else. #9766 is the better shape.

One edge case in it though: the default path raises too, when one dimension is under 16 and the other is over 256.

Image.new("RGBA", (300, 8)).save("out.ico")   # ValueError: All sizes too large for image

sizes becomes [im.size] = [(300, 8)], which the size[0] > min(256, im.width) filter then drops, leaving frames empty. Same for (8, 300) and (260, 10). Not a regression — those write the 6-byte empty file on main today — but a plain save() with no sizes argument probably shouldn't raise.

Happy to close this in favour of #9766.

@radarhere

Copy link
Copy Markdown
Member

So you think 300x8 should save as 256x7? In that scenario, I wonder if it should also save as 128x3, 64x2 and 32x1.

@radarhere radarhere closed this Jul 27, 2026
@radarhere

Copy link
Copy Markdown
Member

#9766 has been merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

🤖-assisted AI-assisted

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants