[
https://issues.apache.org/jira/browse/TIKA-4777?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18095262#comment-18095262
]
ASF GitHub Bot commented on TIKA-4777:
--------------------------------------
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.
```
> Extract QuickTime timed metadata (mebx) tracks
> ----------------------------------------------
>
> Key: TIKA-4777
> URL: https://issues.apache.org/jira/browse/TIKA-4777
> Project: Tika
> Issue Type: Improvement
> Reporter: Dominik Schmidt
> Priority: Major
>
> Apple Live Photo videos mark the moment the paired still image was
> captured in a timed metadata track: the sample description declares the
> format 'mebx' with a keys structure containing
> com.apple.quicktime.still-image-time. The item value itself is always -1;
> the actual time is the presentation time of that sample, derived from the
> track's sample tables.
> Everything needed lives in the moov box, no mdat access is required:
> - mdhd: track timescale
> - stsd: detect the mebx sample entry and its still-image-time key
> - stts and elst: an empty edit shifts the single one-tick sample to the
> still moment
> Example from a real Live Photo video (iPhone 15 Pro, iOS 18.5): the
> still-image-time track has timescale 600, an edit list with an empty edit
> of 740 units followed by the sample, giving a still time of 1.2333 s.
> Neither the current MP4 handler (which reads the static keys/ilst
> metadata, TIKA-2861) nor the underlying metadata-extractor exposes timed
> metadata tracks; ExifTool only surfaces the constant -1 marker value.
> Note that the Apple maker note tag 0x0017 in the paired image
> (historically documented as "LivePhotoVideoIndex") does not encode a
> usable time on current iOS versions, so the video track is the only
> reliable source for this timestamp.
> Proposal: extend TikaMp4BoxHandler to accept the relevant trak child
> boxes, correlate mdhd/stsd/stts/elst per track, and emit the still time
> as metadata (e.g. com.apple.quicktime.still-image-time, in
> microseconds).
> Related: TIKA-2861 (QuickTime item-list metadata), TIKA-4776 (image-side
> Apple maker note, LONG8), TIKA-852.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
