tballison commented on PR #2936:
URL: https://github.com/apache/tika/pull/2936#issuecomment-4934951662
Feedback from claude:
```
The feature is worth having and the byte-slicing is genuinely bounds-safe (I
traced every offset; no OOB, no infinite loops, entry counts can't run away).
But three things
stand out: about half the diff re-implements traversal that
metadata-extractor already does, there are two real bugs in the edit-list
logic, and the metadata key shape
deserves a conversation before it becomes API.
1. Most of the plumbing is redundant (verified, not guessed)
Mp4MetaHandler extends Mp4MediaHandler, whose shouldAcceptContainer
already accepts minf and stbl, and whose shouldAcceptBox already accepts stsd.
Mp4MetaHandler.processSampleDescription(SequentialReader) is an empty
override waiting to be filled in.
I confirmed this by running the stock handler plus a one-line hdlr
injection against the PR's own fixture:
>>> injecting TracingMetaHandler at hdlr/meta
MetaHandler.shouldAcceptContainer(minf) = true
MetaHandler.shouldAcceptContainer(stbl) = true
MetaHandler.shouldAcceptBox(stsd) = true
*** Mp4MetaHandler.processSampleDescription CALLED ***
first 16 bytes of stsd payload: 0000000000000001000000506d656278
entry type at +12: 'mebx'
So instead of the hdlr hijack that returns this and suppresses
Mp4MetaHandler entirely, plus the global minf/stbl/stsd acceptance in
TikaMp4BoxHandler, the PR could return
a TikaMp4MetaHandler extends Mp4MetaHandler from the hdlr branch (passing
in emptyEditDuration and the movie timescale) and override
processSampleDescription. That would:
- keep Mp4MetaDirectory alive rather than silently deleting it from the
extracted directories,
- stop TikaMp4BoxHandler from descending into minf/stbl and slurping the
stsd payload of every track whose handler type isn't soun/vide/hint/text/meta
(sbtl, clcp, tmcd,
…), which it now does for no benefit,
- delete shouldAcceptContainer's minf/stbl cases and shouldAcceptBox's
stsd case.
Only edts (container) and elst (box) genuinely need adding to
TikaMp4BoxHandler — I confirmed the stock handler returns false for both.
Separately, processMovieTimescale is dead weight: the base
processMovieHeader already stores the movie timescale, and
directory.getLongObject(Mp4Directory.TAG_TIME_SCALE)
returns 600 on this exact fixture. Ten lines and a field can go.
To be fair about the Mp4MetaDirectory deletion: I checked the tag IDs, and
Mp4MetaDirectory only ever holds TAG_CREATION_TIME=101,
TAG_MODIFICATION_TIME=102,
TAG_LANGUAGE_CODE=104, none of which collide with the 0x01xx tags
processActualMp4Directory reads. So there's no visible regression today. It's a
latent one.
2. Two correctness bugs in processEditList
The javadoc says "leading empty edit," but the loop scans every entry and
takes the first mediaTime == -1 wherever it appears:
- Edit list [(dur=100, mt=0), (dur=740, mt=-1), (dur=1, mt=0)] — the track
presents media starting at movie time 0, so the start is 0. The code reports
1233333.
- Two consecutive leading empty edits [(740, -1), (300, -1), (1, 0)] — the
true start is 1040 ticks. The code reports 740.
The fix is to only honor entry 0, and to sum consecutive leading -1
entries. Neither shape appears in Apple's own files, but the code doesn't say
that and nothing stops a
fuzzer or a third-party muxer.
Related, and cheaper to fix than to argue about: emptyEditDuration *
1_000_000L overflows for a version-1 elst with a large segment_duration. A
emptyEditDuration >
Long.MAX_VALUE / 1_000_000 guard costs one line.
Also, resetting emptyEditDuration on tkhd assumes tkhd precedes edts. It's
mandated, but resetting on the trak container instead (processContainer routes
through
processBox("trak", null, …)) is free and order-independent.
3. The design question I'd want settled before merge
<key>.track-start-us attaches a track-level fact to every key name the
track declares. The PR body confirms this in practice: on the real iPhone Live
Photo, the
still-image-time track also declares transform keys, and all of them get a
.track-start-us sibling.
For still-image-time the statement is meaningful only because that track
has exactly one one-tick sample, so track-start is the sample's presentation
time. The code never
checks the sample count — it filters on "is delayed," which correlates
with "single sample" in Apple's output but isn't the invariant. Note the JIRA
description explicitly
lists stts among the boxes to correlate; the PR doesn't read it. A delayed
multi-sample mebx track would get <key>.track-start-us on keys whose values are
timestamped all
over the track.
Two smaller shape questions:
Namespace. com.apple.quicktime.still-image-time.track-start-us is a
Tika-invented suffix grafted into Apple's namespace. It reads like a key Apple
defined. It doesn't
collide with reserved Tika keys (the suffix makes that impossible), so
it's not a trust-boundary problem in the TIKA-4769 sense — but it's the kind of
thing that's hard to
rename later.
Absence is ambiguous. Suppressing undelayed tracks means "no
still-image-time.track-start-us" can't distinguish "still is the first frame"
from "not a Live Photo."
Emitting 0 for a declared-but-undelayed still-image-time track would be
unambiguous, at the cost of noise on the other keys — which loops back to the
per-key question.
4. Test gaps
The fixture is a hand-built .mov whose minf contains only stbl and whose
stbl contains only stsd. Real QuickTime metadata tracks have gmhd, a
data-reference hdlr
(dhlr/alis), and dinf inside minf, plus stts/stsc/stsz/stco inside stbl.
The test therefore never exercises the traversal as it will actually run — in
particular that
inner hdlr, which under the PR's approach now reaches super.processBox and
gets handler-type-dispatched. It works (I checked: alis falls through), but the
test wouldn't
catch it if it stopped working.
Nothing covers: an undelayed meta track producing no key (asserted in the
PR body, untested), a version-1 elst, a non-leading empty edit, or a video
track with a leading
empty edit followed by a meta track without one (the state-leak path).
Nits
- The shouldAcceptContainer comment says timed metadata tracks have
handler type mdta. They have meta — mdta is the moov/meta/hdlr metadata
handler. The code's own hdlr
check disagrees with the comment.
- readInt64 is defined between boxType and addLocation, far from its only
caller.
- tikaMetadata.set(...) here vs tikaMetadata.add(...) on the ilst path —
two delayed tracks declaring the same key silently overwrite.
- The fixture change also adds mvhd and a trak to a file that previously
had neither, so testQuickTimeMetadataKeys now additionally produces
dcterms:created,
xmpDM:duration, and xmpDM:audioSampleRate=600 (the last being a
pre-existing MP4Parser quirk — it maps movie timescale to audio sample rate —
not this PR's fault).
Existing assertions still pass; just noting the fixture is no longer inert.
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
