fix(preview): close the PreviewImage tag gap across 5 ExifTool mechanisms - #430
Merged
Conversation
Scopes 5 tasks across the 3 verified ExifTool mechanisms for this tag (generic offset-pair, Samsung direct dump, vendor MakerNotes-inline), plus 2 tasks that need further tag-ID verification before coding.
… not omit Task 1's implementer traced real ExifTool source (ExtractBinary's pre-seek shortcut, ExifTool.pm ~9836) and a real out-of-bounds corpus file (LeicaCL.jpg) proving the original plan's 'omit entirely when unreadable' claim was a testing artifact: it came from explicitly requesting -PreviewImage, which disables the shortcut. In the actual default full-dump mode the tag-comparison harness and oxidex -j -e use, the placeholder shows regardless of bounds -- same as ThumbnailImage's existing read_or_placeholder. Corrects Global Constraints, Task 1, and flags the same question for Tasks 4 and 5.
… PreviewImage Exif.pm:707-768 excludes APP1's IFD2 (Leica JPEG preview) from the generic StripOffsets/StripByteCounts naming; the pair is PreviewImageStart/PreviewImageLength there instead, and DataTag PreviewImage reads the bytes they point at. Uses read_or_placeholder (same as ThumbnailImage), showing the declared-length placeholder even out of bounds, per PreviewImage's ExtractImage/ExtractBinary semantics -- verified against LeicaCL.jpg, whose IFD2:PreviewImageStart (7064224) is past its 50,939-byte file end yet ExifTool still reports the placeholder in default-dump mode. Also wires an IFD1->IFD2 walk into jpeg_helpers.rs's embedded-EXIF pipeline (parse_ifd2_preview_image, mirroring parse_ifd1_thumbnail): JPEG's own EXIF parsing never shared tiff_helpers.rs's parse_ifd_chain in the first place, so the tiff_helpers.rs change alone extracted IFD2:PreviewImage for standalone TIFF/DNG files but never executed for the JPEG-embedded case this task targets. Without this second half, LeicaCL.jpg (a JPEG) produced no IFD2 tags at all.
…ports Perl ExifTool's family-0 view (-G) groups IFD2 under "EXIF" the same way it already does for IFD0/IFD1/ExifIFD/GPS/InteropIFD, but this harness's family-alias table was missing "IFD2". After tiff_helpers.rs started emitting IFD2:PreviewImage (previous commit), the unmapped group made the tag-comparison binary double-count it: once as a false "missing" (ExifTool's EXIF:PreviewImage had no oxidex match) and once as a false "extra" (oxidex's IFD2:PreviewImage matched nothing). Verified against the JPEG corpus: matched-tag count rises from 3053 to 3054 with EXIF:PreviewImage now in Matched Tags and no residual IFD2:PreviewImage entry in Extra Tags.
process_tiff_ifd_tags/parse_ifd_chain only serve the standalone
TIFF/DNG/CR2/NEF path (operations.rs) -- never an APP1/JPEG file. But
Exif.pm:707-768's rename of 0x111/0x117 to PreviewImageStart/
PreviewImageLength only applies inside APP1's IFD2 ("APP1 IFD2 is for
Leica JPEG preview"); a standalone TIFF-family file's IFD2 keeps the
generic StripOffsets/StripByteCounts naming. Adding the rename to
process_tiff_ifd_tags therefore did the rename backwards from
ExifTool for every standalone-TIFF caller: any TIFF/DNG/CR2/NEF file
with a real chained IFD2 carrying genuine StripOffsets/StripByteCounts
would have been silently mis-renamed to PreviewImageStart/
PreviewImageLength/PreviewImage. No corpus file exercises this, so the
suite stayed green, but it was a latent correctness bug caught in
review.
Revert process_tiff_ifd_tags/parse_ifd_chain to their original
3-tuple/StripOffsets-unconditionally behavior. parse_ifd2_preview_image
(added in 87c5d0a, called only from jpeg_helpers.rs's JPEG-specific
handler) already correctly implicitly gates on APP1/JPEG and needs no
change -- it's now the sole code path performing this rename. Moves
both unit tests to call parse_ifd2_preview_image directly instead of
parse_ifd_chain, since that's the only path that should apply it; this
also gives the function its own direct test/mutation coverage, which
it previously lacked.
…4 PreviewImage dump
ExifTool.pm:7929-8127 finds this preview by byte pattern inside the
APP2/APP3(/APP4) segment payload itself (no offset/length pair), and
FoundTag with no IFD context defaults the group to File.
APP2 accumulates across consecutive segments (ExifTool's `elsif
($preview) { $preview .= $segData }` fallback) -- a single-segment
implementation truncated multi-segment previews (e.g. GoProHERO.jpg's
340142-byte preview) to the first segment's 65533 bytes. Verified
byte-for-byte against the full corpus of matching Samsung/GoPro/DJI/
Panasonic/Nikon files, including the 6-segment GoProHERO.jpg case.
Note: SamsungDigimaxA40.jpg, cited in the task brief as a worked
example, does not actually exercise this mechanism -- its [File]
PreviewImage comes from a different code path (MakerNotes.pm's
unknown-maker-note PreviewImage detection at tag 0x927C), which is
out of scope for this task.
…n tests Review found the APP4 branch of extract_direct_preview_image emitted unconditionally on every segment with a preview in progress, never checking whether the next segment was APP5 (ExifTool.pm:8116-8151, BenQ DC E1050's APP3->APP4->APP5 preview chain). That silently truncated any such preview at APP4 under the correct File:PreviewImage tag name -- a plausible-but-wrong value, worse than an absent tag. Added an APP5_MARKER arm: APP4 now only emits when the next segment isn't APP5, and APP5 (only relevant as a continuation) appends and always emits immediately, matching ExifTool's own lack of a further continuation check past APP5. Also added the multi-segment regression coverage this logic lacked: - app2_preview_dump_accumulates_across_consecutive_segments and app2_preview_dump_does_not_absorb_mpf_segment lock in the earlier fix for the brief's reference pseudocode, which only ever kept the first matching APP2 segment (confirmed against GoProHERO.jpg's real 340142-byte preview, previously truncated to 65533 bytes). - app3_preview_dump_continues_into_app4 and app3_preview_dump_continues_through_app4_into_app5 cover the new APP4/APP5 gating; the latter is a direct regression test for this fix (verified it fails against the pre-fix code via mutation testing). No file in the combined-samples corpus exercises a real APP3->APP4-> APP5 chain (checked by scanning marker sequences across ~5000 JPEGs), so that path is covered by synthetic segment construction only. The 25 real corpus files verified in the previous commit (Samsung/GoPro/ DJI/Panasonic/Nikon) still match ExifTool's PreviewImage byte counts exactly after this change.
…SOI-fixup Sony.pm:906-939's RawConv strips a 32-byte proprietary header then requires a D8 FF DB/E1 JPEG-SOI-minus-leading-FF pattern, reconstructing the FF. Omits the tag when the pattern doesn't match, matching ExifTool's PreviewError/return-undef behavior. 0x2001 was deliberately absent from Sony's string-map MAIN_TABLE because its value routinely lives outside the MakerNote payload that table's SonyIfd::value resolves against (the preview is stored elsewhere in the file). parse_sony_preview_image_tag walks the IFD the same way but resolves the entry against the whole TIFF block, mirroring how sigma::parse_sigma_makernote already handles Sigma's PreviewImage. Wired in from core::tiff_helpers::parse_makernote as an addition alongside the normal Sony dispatch, not a replacement for it.
…not omit Same class of error as the Task 1 correction: verified with a full-dump command (not explicit -PreviewImage) that ExifTool's ExtractBinary pre-seek shortcut also gates this MakerNotes tag, so a failed header-strip/SOI-fixup pattern still shows the placeholder with the raw untransformed byte count in default view, never an omission. Confirmed on SonyDSLR-A700.jpg. Also generalizes the Global Constraints guidance: placeholder-on-failure is now the confirmed norm across two mechanisms, not the exception -- omission needs its own positive verification, not an assumption.
…PreviewImage Corrects Task 3's original omit-on-failure contract, the same class of mistake Task 1 hit earlier in this plan: verified with a full-dump command (exiftool -G1 -s -a -u, not an explicit -PreviewImage/-b request) against SonyDSLR-A700.jpg and four other real corpus files that ExifTool's ExtractBinary shortcut reports the raw-declared-length placeholder in default-dump mode even when the header-strip/SOI-fixup RawConv never actually runs (truncated file) or doesn't match. parse_sony_preview_image now mirrors Task 1's read_or_placeholder two-way split: real transformed bytes when the SOI pattern matches, otherwise a "(Binary data N bytes, use -b option to extract)" placeholder built from the raw/untransformed length -- never an omission when raw bytes are non-empty. parse_sony_preview_image_tag does the same for the case where the declared offset/length can't even be read from ctx.tiff() at all (every sampled Sony DSLR/mirrorless file in this corpus), using the entry's declared byte count for the placeholder since no bytes were read. Verified byte-for-byte against exiftool -G1 -s -a -u on SonyDSLR-A700 (696508), SonyDSLR-A350 (658755), SonyDSLR-A200 (695912), SonyILCE-6000 (490354) and SonyNEX-7 (649702).
…ge/JpgFromRaw extract_dng_subifd_preview emitted PreviewImageStart/Length (and JpgFromRawStart/Length) but silently dropped the PreviewImage/JpgFromRaw key itself whenever the declared offset+length range fell outside the file. ExifTool's ExtractBinary (ExifTool.pm:9832) returns the declared-length "(Binary data N bytes, use -b option to extract)" placeholder without ever seeking in default (non -b) dump mode, so the tag is never omitted -- the same ExtractImage/ExtractBinary mechanism already fixed for JPEG's IFD2 PreviewImage (core::tiff_helpers::read_or_placeholder). Adds a byte-slice counterpart, dng_binary_or_placeholder, so the image key is always present: real bytes in bounds, placeholder otherwise. CR2 IFD0's PreviewImage already built the placeholder unconditionally and had no equivalent gap; confirmed against real ExifTool -G1 -s -a -u output on CanonRaw.cr2 and DNG.dng in the sample corpus (both PreviewImageStart/Length pairs are IFD0/SubIFD1-only, SubIFD2 keeps JpgFromRaw* per Exif.pm's DIR_NAME ne "SubIFD2" condition). A pre-existing test asserted the old (wrong) omission behavior; renamed and rewrote it to assert the placeholder, and added a companion in-bounds test. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Casio Type2 (0x2000 direct inline value), Olympus (CameraSettings 0x0100/0x0101/0x0102, falling back to Main's own 0x1035/0x1036/0x1037 when no CameraSettings sub-directory exists), and Minolta (0x0081 direct value, falling back to the 0x0088/0x0089 offset pair) all carry ExifTool's generic PreviewImage/previewImageTagInfo mechanism, none of which the string-map MakerNoteParser trait can carry (needs MetadataMap/TagValue::Binary). Wired as additive carve-out hooks in tiff_helpers.rs::parse_makernote, following the established Sigma/Sony pattern. Out-of-bounds declared ranges show ExifTool's declared-length placeholder rather than being omitted, matching the default-dump contract Tasks 1/3/4 established. Verified byte-for-byte against real ExifTool output across the full Casio/Olympus/Minolta corpus (322 files, 5 known pre-existing-gap mismatches on "OM SYSTEM\0"-header Olympus bodies, documented as a separate, unrelated OlympusParser gap).
Cleanup pass on top of the already-landed PreviewImage composite plan (Tasks 1-5), fixing 5 issues only the whole-branch review surfaced: - docs: correct the Task 2 SamsungDigimaxA40.jpg citation (that file has no APP2/APP3/APP4/APP5 segment at all; its [File] PreviewImage comes from an unrelated MakerNotes.pm path) and point at GoProHERO.jpg as the real verified ground truth for the direct-dump mechanism. - docs: correct the false "accounts for all 118 measured files" claim with the real corpus-sweep numbers (96/185 Samsung matched, 89 missing via MakerNoteSamsung1b/STMN and MakerNotePentax) and name SonyDSLR-A100.jpg plus 5 Olympus OM-System bodies as known follow-up. - sony.rs: document (comment-only) the known ~31-byte default-view divergence on the untested matches_soi success path, where oxidex stores real transformed bytes but ExifTool's ExtractBinary shortcut would print the raw untransformed count. - olympus.rs: fix camera_settings_wins_over_main_level_when_both_present, whose fixture never actually wrote a Main-level fallback trio, so it could not tell a priority-respecting implementation from a broken one. Extending the fixture also surfaced and fixed a latent CS_START buffer-corruption bug in the fixture builder itself. - minolta.rs: fix offset_pair_out_of_bounds_shows_placeholder_not_omission, which used LeicaCL.jpg's declared length (895146) under a comment claiming it mirrored Minolta.jpg's real length (26); now uses 26. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
swackhamer
force-pushed
the
claude/funny-williamson-010fa2
branch
from
August 2, 2026 15:49
d64dc2d to
dc53f56
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
PreviewImagewas declared in the Composite table but never fired anywhere — a corpus-wide gap of 118 files (Samsung 50, Sony 48, IFD2 7, Casio 3, Minolta 3, Canon 3, others). Investigation found ExifTool actually implements this one tag name via at least 5 independent mechanisms, each requiring separate verification against real ExifTool 13.59 source and corpus files:Exif.pmComposite path (PreviewImageStart/PreviewImageLength→PreviewImage) for JPEG-embedded IFD2 (Leica-style preview) —src/core/tiff_helpers.rssrc/core/jpeg_helpers.rs0x2001, with a header-strip + JPEG-SOI-marker-reconstruction value transform —src/parsers/tiff/makernotes/sony.rssrc/parsers/raw/metadata.rssrc/parsers/tiff/makernotes/{casio,olympus,minolta}.rssrc/composite/tables.rsandsrc/composite/compute.rsare untouched throughout — the core architectural finding is thatcompute()is string-only with no file access, so this tag can never be a pure composite; it belongs at the parser layer that already has file bytes.A significant correction recurred three times during implementation: the initial verification of ExifTool's on-failure behavior was wrong (based on testing with an explicit
exiftool -PreviewImage <file>, which disables ExifTool's placeholder shortcut). The corrected, now-consistently-applied behavior: show the placeholder(Binary data N bytes, use -b option to extract)with the declared/raw length even when the range is unreadable — never omit — matching the pre-existingread_or_placeholderpattern already used forThumbnailImage.Verification
cargo test --workspace: all green, 3381+ passed in the core suite, 0 failed.cargo clippy/cargo fmt --allclean throughout.Known residual gaps (measured, out of scope for this PR)
PreviewImage(two further, unenumerated mechanisms:MakerNoteSamsung1b/STMN andMakerNotePentax).SonyDSLR-A100.jpgand 5 Olympus OM-System-era bodies ("OM SYSTEM\0"header unrecognized by the whole Olympus parser, not preview-specific).src/parsers/tiff/makernotes/sigma.rs's pre-existingPreviewImageextraction still omits on OOB rather than showing the placeholder — untouched by this PR, never re-verified against the corrected understanding established here.src/parsers/tiff/makernote_dispatcher.rs(Casio exact-match string has a stray trailing period, never matches the realMakestring) kills every non-PreviewImageCasio MakerNotes tag on real files — found during this work, worked around locally forPreviewImage, but deserves its own follow-up given its much wider blast radius.Test plan
cargo test --workspace— all greencargo clippy/cargo fmt --all— cleandocs/plans/2026-08-02-preview-image-composite-plan.mdfor full commands/output)🤖 Generated with Claude Code