Repository navigation
Feat/preserve xmp metadata - #497
DichenZhang1 wants to merge 21 commits into
Conversation
- Expose uhdr_enc_set_xmp_data and uhdr_dec_get_xmp in public C API - Support passing custom XMP metadata into encodeJPEGR and appendGainMap - In ISO metadata mode, pass through user/base XMP packet directly - In XMP metadata mode, merge Ultra HDR container directory into user/base XMP RDF - Automatically extract and preserve base JPEG XMP in API-2/3/4 encoding workflows - Wire -X <xmp_file> and probe XMP display into ultrahdr_app CLI - Add unit tests verifying XMP encoding, decoding, and preservation across API-0 and API-2
- Support encoding and decoding Extended XMP metadata (> 64 KB) in JPEG - Compute 128-bit MD5 GUID and inject xmpNote:HasExtendedXMP into Standard XMP - Chunk serialized Extended XMP payload across multiple http://ns.adobe.com/xmp/extension/\0 APP1 segments - Reassemble Extended XMP chunks during JPEG header parsing and expose reconstructed buffer - Calculate dynamic encoder output buffer allowance based on total XMP size and chunk count - Add unit tests validating multi-segment Extended XMP encoding, decoding, and bit-for-bit roundtrip integrity
…ripping - Add public C API uhdr_strip_gain_map and JpegR::stripGainMap to losslessly strip secondary gain map bitstream, MPF APP2, ISO 21496-1 APP2, and Ultra HDR XMP tags - Preserve primary JPEG entropy-coded scan data bit-for-bit along with Exif, ICC, APP13, Extended XMP, and user XMP metadata - Support size-query mode when out_stream->data is nullptr and out_stream->capacity is 0 - Add namespace-aware XML parsing for primary XMP merging and gain-map tag stripping (MergePrimaryXmp and stripGainMapFromXmp) - Add unit tests covering uhdr_strip_gain_map and primary XMP merging across namespace prefixes and re-encoding
… fixture coverage (#494)
Keep namespace bindings on mixed descriptions when stripping gain-map properties so retained descendants do not acquire unbound prefixes. Continue removing descriptions containing only gain-map metadata. Add a regression and retain existing property-removal coverage. Focused tests pass in dual-metadata and ISO-only builds; independent XML parsing validates the reproduced output.
Clean linked extension packets while preserving user metadata, updating the digest and standard XMP link, and removing empty packets. Validate linked chunk coverage before writing output and cover preservation and malformed input with regression tests.
… -Wfree-nonheap-object
|
Is XMP support intentionally limited to JPEG for this PR? |
gregbenz
left a comment
gregbenz
left a comment
There was a problem hiding this comment.
The existing XMP and stripping tests pass locally, but focused probes reproduced the three issues noted inline.
| std::vector<JOCTET> extXmpBuffer; | ||
| jpeg_extract_extended_xmp(&cinfo, expected_guid, extXmpBuffer); | ||
| if (!extXmpBuffer.empty()) { | ||
| mXMPBuffer = std::move(extXmpBuffer); |
There was a problem hiding this comment.
Should we keep the user properties from the standard packet when recovering Extended XMP? With a title in standard XMP and a description in the linked extension, this assignment leaves uhdr_dec_get_xmp() with only the description. Re-encoding the compressed base through API-4 then drops the title from the primary JPEG. A round-trip test with properties split between the two packets would catch this.
| std::string std_xmp(reinterpret_cast<const char*>(mXMPBuffer.data()), mXMPBuffer.size()); | ||
| const std::string kTag = "HasExtendedXMP="; | ||
| size_t pos = std_xmp.find(kTag); | ||
| if (pos != std::string::npos && pos + kTag.size() + 33 <= std_xmp.size()) { |
There was a problem hiding this comment.
Should this use getExtendedXmpGuidFromXmp() instead of the string search? A valid element-form link (<xmpNote:HasExtendedXMP>…</xmpNote:HasExtendedXMP>) isn't found here, so the reader falls back to the first extension. With an unrelated extension before the correctly linked one, it returns the unrelated metadata. The namespace-aware helper already resolves the intended GUID for that case. A regression test with the unrelated packet first would catch this.
|
|
||
| if (total_length == 0) { | ||
| total_length = seg_total_len; | ||
| reassembled.resize(total_length, 0); |
There was a problem hiding this comment.
Should we validate the declared Extended XMP length and chunk coverage before resizing these buffers? A 4,816-byte crafted input makes uhdr_dec_probe() attempt a 1 GiB allocation, with std::bad_alloc escaping the API. Bounding the allocation to validated available chunk data would prevent this. A tiny-input/huge-declared-length regression would catch it.
| } | ||
| string extra_xmlns; | ||
| for (const XmpNamespaceDeclaration& decl : std_ancestor_ns) { | ||
| if (decl.prefix.empty() || decl.prefix == "rdf" || decl.prefix == "x") continue; |
There was a problem hiding this comment.
Should we keep the inherited namespaces and xml:lang when moving these descriptions? Testing mergeStandardAndExtendedXmp() directly on this revision reproduced three cases:
- Standard XMP uses
rdf, while the extension usesr: the merged XML has an unboundrdfprefix. - A property inherits the default namespace from standard XMP: it loses that namespace after the merge.
- A title inherits
xml:lang="en", while the extension hasxml:lang="fr": its language changes to French.
The helper returns success in all three cases. Should we preserve the original context on the moved descriptions and add regression tests?
…in mergeStandardAndExtendedXmp
2 participants
No description provided.