fix(png): We were not correctly suppressing hint metadata #4983
Conversation
Each file writer that is capable of writing arbitrary metadata is supposed to tace care to suppress hints meant for other writers, or for OIIO in general, not interpret them as literal metadata to write to the file. PNG was not doing so, and ended up writing hints as literal metadata. This brings it in line with our behavior when writing OpenEXR and other formats. Signed-off-by: Larry Gritz <lg@larrygritz.com>
There was a problem hiding this comment.
Pull request overview
This PR fixes a bug where the PNG writer was incorrectly writing metadata hints (intended for other file formats or OIIO itself) as literal metadata in PNG files. The fix adds logic to filter out metadata with namespace prefixes like "oiio:" or other format names (e.g., "exr:", "tiff:") before writing to PNG files, bringing PNG's behavior in line with other format writers like OpenEXR.
Key changes:
- Added metadata filtering logic to suppress format-specific hints and OIIO-internal hints
- Updated test reference output to reflect corrected chunk types in error messages
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/png.imageio/png_pvt.h | Added metadata filtering to prevent writing format hints as literal PNG metadata |
| testsuite/png-damaged/ref/out.txt | Updated test expectations to reflect correct PNG chunk types (oFFs vs tEXt) |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
src/png.imageio/png_pvt.h
Outdated
| auto parts = Strutil::splitsv(name, ":", 2); | ||
| OIIO_DASSERT(parts.size() == 2); |
There was a problem hiding this comment.
The variable 'parts' is assigned but never used. This computation is unnecessary since the assertion only verifies the result. Consider removing this line and relying on the existing colonpos check.
| auto parts = Strutil::splitsv(name, ":", 2); | |
| OIIO_DASSERT(parts.size() == 2); |
There was a problem hiding this comment.
Right you are, Copilot! Those two lines were left over from a previous stab at implementing this, then I changed my mind but didn't clean up all the extraneous stuff.
Signed-off-by: Larry Gritz <lg@larrygritz.com>
lgritz
merged commit 0fe78f7
into
AcademySoftwareFoundation:main
…twareFoundation#4983) Each file writer that is capable of writing arbitrary metadata is supposed to tace care to suppress hints meant for other writers, or for OIIO in general, not interpret them as literal metadata to write to the file. PNG was not doing so, and ended up writing hints as literal metadata. This brings it in line with our behavior when writing OpenEXR and other formats. --------- Signed-off-by: Larry Gritz <lg@larrygritz.com>
2 participants
Each file writer that is capable of writing arbitrary metadata is supposed to tace care to suppress hints meant for other writers, or for OIIO in general, not interpret them as literal metadata to write to the file. PNG was not doing so, and ended up writing hints as literal metadata. This brings it in line with our behavior when writing OpenEXR and other formats.