Re: [PR] TIKA-4838: Expose the MP4 audio and video track codecs (audio:format, video:format) [tika]

["Copilot (via GitHub)" <gi...@apache.org>]

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: dev-unsubscribe@tika.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org