TIKA-3646: Type a plain MP4 by its tracks, not by its brand - #3114
TIKA-3646: Type a plain MP4 by its tracks, not by its brand#3114dschmidt wants to merge 2 commits into
Conversation
…rs put it behind the media data
|
Looks like we have to parse the mp4 in some cases where the right information isn't close enough to the front. For some parsers, we have a pattern where the detector detects family, and then the parser can make more specific, as with Adobe Illustrator and PDFs. There's another parser/detector where we do this too and we make the Detector optional, but it basically just calls the parser. What would you think of folding this logic into the parser to update the mime type during parse? |
|
Honestly, having sat with this for a bit: I am not sure any more that it is worth it. I built it for #3115, where the video trailing a motion photo has to be typed before it is emitted. It turns out not to buy anything there, because the embedded parse runs That is the general picture, too. So folding it into the parser would mostly mean writing what the parser already does. As I see it:
I lean towards 1, or 3 if the shared primitives are worth keeping on their own. Your call. |
There was a problem hiding this comment.
🟡 Changes recommended
There is at least one confirmed functional issue in the new detector (ArrayBoxes.read() short-read/null behavior) that should be fixed before merging.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR implements RFC 4337-style detection for “plain MP4” files by inspecting track handler types in the moov box (video/audio/other) rather than relying on the ftyp brand, addressing cases where isom previously caused fallback to video/quicktime (TIKA-3646 / TIKA-2935).
Changes:
- Adds
MP4TrackDetectorto classify MP4s asvideo/mp4,audio/mp4, orapplication/mp4by walking boxes and readingmoov/trak/mdia/hdlr. - Introduces
Mp4Boxesas a shared utility for ISO BMFF box traversal/reading (32-bit size, 64-bit largesize, and size==0). - Adds thorough unit tests for correct typing and for malformed/truncated inputs; updates existing MP4 sample-entry code to use the shared box utilities.
File summaries
| File | Description |
|---|---|
| tika-parsers/.../Mp4SampleEntriesTest.java | Updates FourCC test calls to use the new shared Mp4Boxes utility. |
| tika-parsers/.../MP4TrackDetectorTest.java | Adds coverage for track-based MP4 typing, streaming/moov placement scenarios, and malformed input safety. |
| tika-parsers/.../TikaMp4VideoHandler.java | Switches FourCC reads to Mp4Boxes.fourCC. |
| tika-parsers/.../TikaMp4SoundHandler.java | Switches FourCC reads to Mp4Boxes.fourCC. |
| tika-parsers/.../Mp4SampleEntries.java | Refactors internal box walking to reuse Mp4Boxes and adds a child-box cap. |
| tika-parsers/.../Mp4Boxes.java | New shared box primitives (end calculation, payload start, findBox, FourCC helpers). |
| tika-parsers/.../MP4TrackDetector.java | New detector that finds moov and types MP4s based on track handlers with bounds/caps. |
| CHANGES.txt | Documents the MP4 typing behavior change for the upcoming release. |
Review details
- Files reviewed: 8/8 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| @Override | ||
| public byte[] read(long offset, int size) { | ||
| if (offset < 0 || offset > length - Math.min(size, 8)) { | ||
| return null; | ||
| } | ||
| int available = (int) Math.min(size, length - offset); | ||
| return Arrays.copyOfRange(bytes, (int) offset, (int) offset + available); | ||
| } |
| //a movie box behind the media data, where recorders that do not write | ||
| //for streaming put it, is only reachable by seeking; the spooled file | ||
| //is the one the parse reads afterwards, so it is not read twice | ||
| try (SeekableByteChannel channel = Files.newByteChannel(tis.getPath())) { | ||
| return movieBox(new ChannelBoxes(channel)); |
Tika's own fixtures show the problem:
testMP4Video.mp4andtestMP4AudioOnly.mp4both carry theisombrand, and both are detected asvideo/quicktime, because the mime magic forvideo/mp4only matches the brandsmp41andmp42. Addingisomto that magic would type the audio-only file as video, so the brand cannot answer this; the tracks have to.MP4TrackDetectorwalks the top level boxes by their size fields, which steps over the media data rather than reading it, finds the movie box and reads the handler type of eachmoov/trak/mdia/hdlr: a video track makes itvideo/mp4, otherwise an audio track makes itaudio/mp4, otherwiseapplication/mp4, as RFC 4337 describes. Brands that name a format of their own (M4A, 3GP, HEIC, AVIF, CR3, ...) are left to their own magic, and so is a file whose movie box cannot be reached.The box primitives are shared rather than written again:
boxEnd,findBoxand the FourCC readers move fromMp4SampleEntries(TIKA-4838) intoMp4Boxes, which now also handles the 64 bit largesize and the size 0 form; the sample entry walker and the two sample handlers use it.Reaching the movie box: a file written for streaming has it right behind the file type box, and those are answered from a 256 KB prefix without touching the disk. A recorder that does not write for streaming puts it behind the media data instead, megabytes in, and there the stream is spooled and the boxes are seeked over, the way the container detectors do it; the spooled file is the one the parse reads next, so nothing is read twice.
Bounds, since a detector reads whatever arrives: sizes are checked as longs before any cast, the walk stops at the first box that does not fit what encloses it, and the number of boxes, the number of tracks and the size of the movie box read into memory are capped. Tests cover every truncation of a well formed file and random bytes behind a valid header.
Verified on a server built from this branch:
testMP4AudioOnly.mp4now detects as audio/mp4 andtestMP4Video.mp4as video/mp4, where both used to be video/quicktime, and so do the files attached to these two tickets, including the one whose movie box sits 2.9 MB in. HEIC, AVIF and M4A files are unchanged.Both files attached to these tickets now detect as video/mp4, so this fixes TIKA-3646 and the example in TIKA-2935; the audio/mp4 and application/mp4 cases RFC 4337 describes come from the same walk. What it does not touch are the formats with a brand of their own, which have their own magic.
https://issues.apache.org/jira/browse/TIKA-3646