Copilot commented on code in PR #3062:
URL: https://github.com/apache/tika/pull/3062#discussion_r3845053050
##########
tika-parsers/tika-parsers-standard/tika-parsers-standard-modules/tika-parser-audiovideo-module/src/main/java/org/apache/tika/parser/mp4/TikaMp4VideoHandler.java:
##########
@@ -76,6 +76,16 @@ private void extractFromSampleDescriptions(byte[] b) {
break;
}
int end = pos + (int) size;
+ String format = fourCc(b, pos + 4);
+ //protected streams replace the codec fourcc with 'encv' (ISO
common
+ //encryption) or 'drmi' (FairPlay); the original codec is in the
nested frma box
+ if ("encv".equals(format) || "drmi".equals(format)) {
+ String original = findOriginalFormat(b, pos +
VISUAL_ENTRY_SIZE, end, 0);
+ if (original != null) {
+ format = original;
+ }
+ }
+ tikaMetadata.set(Video.FORMAT, format);
Review Comment:
New behavior is introduced for protected streams (`encv`/`drmi` → recover
original codec from nested `frma`), but the added tests only cover unprotected
samples (`avc1`/`mp4a`). Please add a test fixture with protected video and/or
audio (e.g., `encv` + `frma=avc1`, and `enca`/`drms` + `frma=mp4a`) and assert
that `video:format`/`audio:format` reflect the original codec, and that
`audio:has-drm` remains set for protected audio.
##########
tika-core/src/main/java/org/apache/tika/metadata/Video.java:
##########
@@ -37,4 +37,12 @@ public interface Video {
* reflects the last one.
*/
Property BITRATE = Property.internalInteger("video:bitrate");
+
+ /**
+ * The video track's four-character codec identifier from the MP4/QuickTime
+ * sample description (e.g. "avc1" for H.264, "hvc1"/"hev1" for HEVC). For
+ * protected streams this is the original codec from the 'frma' box.
Distinct
+ * from {@link XMPDM#VIDEO_COMPRESSOR}, the human-readable compressor name.
Review Comment:
`Video.FORMAT` is populated from iterating sample descriptions; because it
uses `Metadata#set(...)` with a SIMPLE cardinality property, later sample
entries will overwrite earlier ones. Please document the effective behavior
(e.g., 'if multiple sample entries exist, the value reflects the last
encountered entry') or adjust extraction to set only once (e.g., first
non-null) if that’s the intended contract.
##########
tika-parsers/tika-parsers-standard/tika-parsers-standard-modules/tika-parser-audiovideo-module/src/main/java/org/apache/tika/parser/mp4/TikaMp4VideoHandler.java:
##########
@@ -105,6 +115,38 @@ private static int findBtrtAverageBitRate(byte[] b, int
pos, int end) {
return 0;
}
+ private static final int MAX_BOX_DEPTH = 10;
+
+ /**
+ * Scans the child boxes of a protected ('encv'/'drmi') sample entry for
the
+ * 'frma' (original format) box nested in the 'sinf' protection scheme
+ * information box, and returns its codec fourcc, or null if there is none.
+ */
+ private static String findOriginalFormat(byte[] b, int pos, int end, int
depth) {
+ if (depth > MAX_BOX_DEPTH) {
+ return null;
+ }
+ while (pos + 8 <= end) {
+ long size = EndianUtils.getUIntBE(b, pos);
+ if (size < 8 || size > end - pos) {
+ return null;
+ }
+ int boxEnd = pos + (int) size;
+ String type = fourCc(b, pos + 4);
+ if ("frma".equals(type) && pos + 12 <= boxEnd) {
+ return fourCc(b, pos + 8);
+ }
+ if ("sinf".equals(type)) {
+ String original = findOriginalFormat(b, pos + 8, boxEnd, depth
+ 1);
+ if (original != null) {
+ return original;
+ }
+ }
+ pos = boxEnd;
+ }
+ return null;
+ }
Review Comment:
`findOriginalFormat` logic is duplicated in both `TikaMp4VideoHandler` and
`TikaMp4SoundHandler` with only docstring differences. Consider extracting this
into a shared helper (e.g., an MP4 box parsing utility used by both handlers)
so fixes (bounds/recursion behavior, box traversal rules, etc.) don't need to
be applied twice.
--
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]
