[usmanmateen]

Resolves #4680

ImageResourceLoader.create_image_ref passed images to ImageRef.from_pil without normalizing them, so a CMYK image raised an OSError that the except didn't catch, and one such image in an HTML or Markdown file failed the whole conversion. This normalizes the image with normalize_image_for_png, the same way the Word and PowerPoint backends do since #4598. It also adds OSError to the caught errors, so an image that still can't be decoded (for example a truncated file) gives the usual "Could not process an image" warning and no picture, instead of failing the document.

Checklist:

• Documentation has been updated, if necessary. (not needed)
• Examples have been added, if necessary. (not needed)
• Tests have been added, if necessary.

Review evidence:

• The reproduction from HTML and Markdown conversion fails on a CMYK or truncated image #4680 prints ConversionStatus.FAILURE ['cannot write mode CMYK as PNG'] on main and ConversionStatus.SUCCESS [] with this change. The CMYK picture is kept at 200×100, in both HTML and Markdown.
• An HTML page with a truncated JPEG: FAILURE (Truncated File Read) on main, SUCCESS with this change.
• Two tests in tests/test_image_resource_loader.py: test_create_image_ref_converts_cmyk and test_create_image_ref_skips_undecodable_image. Both fail on main (cannot write mode CMYK as PNG, and the truncated image raising instead of warning) and pass here.
• pytest tests/test_image_resource_loader.py tests/test_backend_html.py tests/test_backend_mhtml.py tests/test_backend_markdown.py: 179 passed, 1 skipped, 2 failed. The 2 failures (test_fetch_refuses_non_public_host[localhost] and test_fetch_refuses_redirect_to_non_public_host) fail the same way on main in my environment (WSL2) and are about localhost URL checks this PR doesn't touch.
• make validate: all hooks pass.

[github-actions]

✅ DCO Check Passed

Thanks @usmanmateen, all your commits are properly signed off. 🎉

[mergify]

Merge Protections

🟢 Merge protection satisfied — ready to merge.

Show 1 satisfied protection

🟢 Enforce conventional commit

Make sure that we follow https://www.conventionalcommits.org/en/v1.0.0/

• title ~= ^(fix|feat|docs|style|refactor|perf|test|build|ci|chore|revert)(?:\(.+\))?(!)?:

[github-actions]

AI triage (advisory)

Checked commit 8d595de78f. The analysis read the diff only. It did not run code from this pull request.

Normalize images for PNG output in ImageResourceLoader and catch OSError exceptions to skip unreadable images safely.

Topic labels added: bug, html, markdown, error-handling, tests.

CI safety: no concern found. A maintainer must still approve CI.

1 related pull request(s) in the same code

PR	State	Signal	Assessment
#4598	merged	referenced as #4598	related: PR 4598 introduced normalize_image_for_png for office backends, while this PR applies it to ImageResourceLoader.

_{The ai:* labels are advisory. They do not approve CI and they do not add tests:full. A new push runs this check again.}

[github-actions]

AI first review (advisory): no blocking issue found

This review covers the full diff at 8d595de78f. It read the code only. It did not run tests or code from this pull request.

This PR fixes two related problems in ImageResourceLoader.create_image_ref: (1) CMYK images passed to ImageRef.from_pil raised OSError because PNG cannot store CMYK, and (2) other OSError failures (e.g. truncated files) propagated instead of being caught. The fix applies normalize_image_for_png (already used by Word, PowerPoint, OpenDocument, and Excel backends) and adds OSError to the caught exception tuple. Two targeted unit tests are added. The change is minimal and consistent with the existing backend pattern.

_{A maintainer makes the merge decision. This review never approves the pull request.}

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

[ceberam]

@usmanmateen The PR #4683 , which addresses issue #4682 , has been created on this same topic.
Can you explain how different your PR is from #4683? In particular, does it resolve the issues described in #4682 and in #4680 ? Are there gaps that are not covered in your PR? Does it provide a better implementation in terms of code quality and pythonic patterns, test coverage and robustness, maintainability and complexity?

[usmanmateen]

cc @PeterStaar-IBM since you reviewed #4598 and the last loader change in #4420. Happy to adjust anything.

[usmanmateen]

@ceberam Thanks for flagging it. #4681 was opened about five minutes before #4683, so I hadn't seen it. I compared the two:

• The loader change is the same in both PRs (normalize before ImageRef.from_pil and catch OSError). I ran fix: keep pictures whose mode cannot be written as PNG #4683's tests against this branch: the HTML, Markdown and AsciiDoc cases pass.
• fix: keep pictures whose mode cannot be written as PNG #4683 also normalizes in the MHTML path of html_backend.py, which this PR doesn't touch. Its MHTML test fails on this branch: the picture is kept but has no image, with the warning Could not process an embedded MHTML image ... cannot write mode CMYK as PNG. That's a real gap here.
• The one thing this PR adds is a test for the OSError path: create_image_ref returns None with a warning for a truncated image. With that change, an HTML page with a truncated image converts and keeps its text instead of failing. fix: keep pictures whose mode cannot be written as PNG #4683 makes the same code change but doesn't test it. Both tests from this PR pass on fix: keep pictures whose mode cannot be written as PNG #4683's branch (82d6486).

So #4683 covers everything #4681 does, plus MHTML. I'm happy to close this one in favour of #4683. If it helps, test_create_image_ref_skips_undecodable_image could be carried over there.

[ceberam]

Thanks @usmanmateen for your honest review and your overall contribution to Docling!
Even though your issue and PR was opened first (with few minutes of difference), the completeness argument is here more relevant.
Closing as superseded by #4683