Repository navigation
Conversation
With `fetch_images=True`, an inline `data:image/svg+xml` image became a picture without an image: the loader handed the decoded SVG to `PIL.Image.open()`, which has no SVG decoder, caught `UnidentifiedImageError` and returned `None`, so the payload was lost. Keep the decoded payload of such a data URI as the image resource. The ref carries the declared media type, the size the SVG declares (its `width` and `height`, or its `viewBox`), and the original data URI, which the Markdown and HTML serializers emit as-is, so the SVG survives exports and round trips. Raster payloads are untouched: PNG and JPEG still go through Pillow, and any other payload that Pillow cannot identify still warns and leaves a placeholder. Rendering an inline SVG into pixels needs an SVG renderer, so `ImageRef.pil_image` stays unavailable for these refs. Signed-off-by: simpleqt <2015900818@qq.com>
There was a problem hiding this comment.
Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.
An organization owner can choose the repository organization as the billing source for reviews requested by members with a paid Copilot license. Learn about Copilot code review billing.
|
✅ DCO Check Passed Thanks @simpleqt, all your commits are properly signed off. 🎉 |
Merge Protections🟢 Merge protection satisfied — ready to merge. Show 1 satisfied protection🟢 Enforce conventional commitMake sure that we follow https://www.conventionalcommits.org/en/v1.0.0/
|
AI triage (advisory)Checked commit The PR keeps the decoded payload of an inline Topic labels added: CI safety: no concern found. A maintainer must still approve CI. 1 related pull request(s) in the same code
The |
There was a problem hiding this comment.
AI first review (advisory): blocker found
This review covers the full diff at 9d28881b19. It read the code and did not run it.
The SVG ImageRef has no pixels. ImageRef.pil_image raises UnidentifiedImageError for it, and get_image() raises with it. Before this PR the SVG picture had image=None and these paths skipped it. Check that exports in REFERENCED mode and picture enrichment still work for a Markdown file that holds an inline SVG.
A maintainer makes the merge decision. This review never approves the pull request.
| # of the picture. Any other payload keeps the warning. | ||
| if (svg_ref := _inline_svg_image_ref(src_url, img_data)) is None: | ||
| raise | ||
| return svg_ref |
There was a problem hiding this comment.
blocker: SVG ImageRef makes PictureItem.get_image() raise instead of returning None
Trigger: Markdown or HTML input with  and fetch_images=True. This is now the default path for such input.
Cause: The new ImageRef has uri=data:image/svg+xml;... and no _pil. FloatingItem.get_image() returns self.image.pil_image. For a data: URI, ImageRef.pil_image calls PILImage.open(BytesIO(decoded)). It has no try/except, so it raises UnidentifiedImageError. This happens in docling_core/types/doc/common/reference.py (the pil_image property) and in docling_core/types/doc/items/node.py (FloatingItem.get_image).
Wrong result: Before this PR the picture had image=None. get_image() then returned None for a picture with no provenance, and callers skipped it. Now two paths crash:
DoclingDocument._with_pictures_refscallsitem.get_image(doc=self)with no guard. It runs forexport_to_markdown,export_to_htmlandsave_as_*withimage_mode=ImageRefMode.REFERENCED. A document with one inline SVG can no longer be exported in this mode.BaseItemAndImageEnrichmentModel.prepare_elementindocling/models/base_model.py:197callselement.get_image(conv_res.document). The picture description, classification and similar enrichment stages will crash on the SVG picture.
The docstring says pil_image is "not available" for such a ref. In fact it raises, and the new code does not guard the callers.
Test gap: The tests use only ImageRefMode.EMBEDDED and export_to_dict. Neither calls pil_image. No test covers REFERENCED mode or get_image() on the SVG picture.
Fix: Do not hand out an ImageRef that pil_image cannot open. Options:
- Keep
image=Noneand store the SVG payload elsewhere, for example in the picture meta. - Rasterize the SVG so the ref has real pixels.
- Change
docling_coreso thatpil_imagereturnsNonefor non-raster mimetypes.
Add a regression test that exports with ImageRefMode.REFERENCED and calls get_image(doc) on the SVG pictu…
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
Withdrawing: this change trades silent data loss for a crashI am closing this PR. Independent checking found a regression I introduced, and the fix belongs in What is still true. The reported bug is real: with What is wrong with my fix. It handed back an
So the export path I worried about is fine, but Where the fix belongs. The contract violation is in On the AI reviewer's blocker. The advisory review was right on the substance and I should have tested AI assistance: this retraction was produced with AI assistance (an autonomous coding agent); the reproduction above was executed locally against both this branch and the base commit. Update: the root-cause fix is open as docling-project/docling-core#916. |
Keep the image of an inline SVG data URI
Fixes #4742.
With
fetch_images=True, an inlinedata:image/pngis decoded, but an inlinedata:image/svg+xmlbecomes a picture with no image.ImageResourceLoader.create_image_refhands the decoded bytes to
PIL.Image.open(), Pillow has no SVG decoder, theUnidentifiedImageErroris caught, the loader warns and returnsNone— so thepayload is dropped:
The loader now catches that error and, when the source is a data URI declaring
image/svg+xml, returns anImageRefthat keeps the decoded payload:mimetype="image/svg+xml",dpi=72,sizetaken from the SVG'swidth/height(falling back to its
viewBox), anduriset to the original data URI, which theMarkdown and HTML serializers already emit as-is. Both pictures then report
image=(120.0, 80.0), and no warning is raised.The size attributes are read with regular expressions rather than an XML parser,
so a malformed or hostile document (entity expansion included) cannot affect the
loader. Nothing else changes: PNG/JPEG take the byte-identical Pillow path, any
other undecodable payload still warns and leaves a placeholder
(
data:image/png;base64,aW52YWxpZA==behaves as before, which an existing testpins), and local/remote
.svgsources are still skipped.Testing
Two tests in
tests/test_backend_markdown_images.py:test_inline_svg_data_uri_keeps_the_image(parametrized overfetch_images) —checks no warning, that the PNG still yields real pixels through Pillow, that
the SVG keeps its payload, size and URI, and that the payload survives both an
ImageRefMode.EMBEDDEDexport and amodel_validate(export_to_dict())round triptest_inline_svg_data_uri_size_comes_from_the_view_boxOn the base commit the inline-SVG tests fail (the warning fires, and
assert None is not Nonefor the missing image); with the fix the module is28 passed, andtest_backend_markdown_images.py+test_image_resource_loader.py+test_backend_markdown.pyare97 passed.ruffat both the versionpyproject.tomlpins (0.15.12) and the pre-commit rev(0.16.10) is clean, including the import-sort form CI uses for a
--no-install-projectinstall.A trade-off worth your judgement
Turning an SVG into pixels needs an SVG renderer, which this change deliberately
does not add (that is a dependency-packaging decision). So for these refs
ImageRef.pil_image/PictureItem.get_image()raiseUnidentifiedImageErrorrather than returning
None— the difference matters to consumers that needpixels, such as picture-enrichment models. Everything else, including Markdown and
HTML export and JSON round trips, now preserves the SVG payload. If you would
rather the loader rasterize instead, this change is the wrong shape and I would
withdraw it; @iluv7's comment on the issue describes that alternative. The shared
loader means the HTML path benefits too:
<img src="data:image/svg+xml;base64,…">now yields
('image/svg+xml', (40.0, 20.0)).AI assistance: this change was produced with AI assistance (an autonomous coding
agent) from the reporter's issue; the analysis, the reproduction, the
before/after test runs and the lint runs were executed locally, and a human
reviewed the result before submission.