TIKA-4869: Emit the video of a motion photo as an embedded document - #3115
Conversation
…uess an extension from detection
…cument, no warning
…er keys, no interning lookup Three agent reviews of the branch, and what they turned up: - the directory walk summed the lengths of every item from the video on, which the format never has anything after: two crafted Item:Length values wrapped the sum negative, put the offset past the end of the file, and an EOFException out of MotionPhoto.extract failed a JPEG that was otherwise fine. The video's own Item:Length is the distance to the end of the file, so that is what is read now, and a length the file cannot hold is settled from the stream's own length, before the image is spilled to disk for it. - getPath() throws where a parser read the image from a stream it cannot go back to (BPGParser does not rewind), which failed the image over a video that was merely out of reach. - the raw XMP keys carried the document's own prefix, so a motion photo that had been through exiftool listed its parts under GContainer:Directory and lost its video without a word. The flattener now keys the two Google container namespaces by URI, as it already does for Camera:. - MimeTypes.forName interns whatever it does not recognize, and Item:Mime comes from the file (TIKA-4826): EmbeddedDocumentUtil looks the extension up without registering anything. - in the OCR branch an escaping extract() skipped the OCR parse and dropped a pending metadata failure; it now runs after it. Also: the name is marked as carrying an inferred extension, and the class no longer claims AVIF, which HeifParser does not accept.
|
This is really nice. My bot has maybe one or two things worth changing. The hygiene thing with MediaType.parse should be fixed now in main(?). Let me know what you think, and thank you! |
|
On 1., I'm frankly not certain that we need to require video. If someone sticks a PDF after an image, we should extract it. I do want to avoid the Stackoverflow, though. |
From tballison's review on apache#3115: - the gate accepted any recognized type, and an image appended to an image was emitted and searched for a trailer of its own, so a file that nests itself chained as deep as it liked: on the plain parse path, where nothing caps the embedded depth, that reached a StackOverflowError. What is appended is still emitted whatever it turns out to be, a PDF behind an image included, but the branch below an emitted trailer is not searched again. - the name followed the declaration even where the bytes disagreed with it. It still follows the declaration, which names the format the file was written with and which detection cannot always tell apart (an MP4 with the isom brand types as quicktime, TIKA-3646), but where the two disagree about the kind of file it is, the bytes win and the extension is marked as inferred. - Files.size, the detection and the region stream threw out of the image parser, which the class says it does not do: locating the trailer is one guarded step now, and so is opening it. - the declared Item:Mime stays a string rather than going through MediaType.parse, which caches what it is given; it is only ever looked up for an extension. - the embedded parse writes its own div, as EMFParser and RawTiffParser do. Tests: a trailer that is recognized but is not a video, and a motion photo nested in a motion photo, which yields two documents rather than a chain. The declared length is read from the fixture instead of spelled out.
|
Thank you, and all of it is in, pushed just now. On 1., I agree, and I did not add the video requirement. The recursion is closed at its source instead: the branch below an emitted trailer is not searched for a trailer of its own. A PDF behind an image is still emitted, an image behind an image too, but it cannot chain. The new test builds the nesting your bot measured and asserts two documents where it got three. Worth its own ticket, maybe: what made this reachable at all is that On 2., the reason for preferring the declaration is that detection cannot tell these apart: an MP4 with the On 3., On the hygiene list: the declared |
There was a problem hiding this comment.
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Pull request overview
This PR adds support for extracting the trailing video from Google/Android Motion Photos (and older MicroVideo) and emitting it as an embedded ATTACHMENT, while also stabilizing XMP raw keys by canonicalizing certain namespace prefixes.
Changes:
- Add
MotionPhototrailer extraction and wire it into image parsing to emit the appended video as an embedded document. - Canonicalize Google container XMP namespace prefixes in
XmpSaxFlattenerso downstream consumers can rely on stableContainer:/Item:paths. - Add unit + integration tests covering extraction, naming, detection behavior, and “shared without video” cases.
Reviewed changes
Copilot reviewed 8 out of 12 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
tika-parser-xmp-commons/.../XmpSaxFlattenerTest.java |
Adds a test ensuring canonical Container:/Item: paths even when the source uses rewritten prefixes (e.g., ExifTool). |
tika-parser-xmp-commons/.../XmpSaxFlattener.java |
Introduces canonical prefix mapping for specific namespaces and applies it to element/attribute path construction. |
tika-parser-image-module/.../MotionPhotoXmpTest.java |
Updates expected XMP values in tests to match new fixture/trailer sizes. |
tika-parser-image-module/.../MotionPhotoVideoTest.java |
Adds focused unit tests for motion photo trailer extraction and edge cases. |
tika-parser-image-module/.../MotionPhoto.java |
Implements trailer location, detection, naming, and embedded-document emission logic. |
tika-parser-image-module/.../AbstractImageParser.java |
Calls MotionPhoto.extract(...) so trailing motion-photo video is emitted during image parsing. |
tika-parsers-standard-integration-tests/.../MotionPhotoVideoIntegrationTest.java |
Adds an integration test asserting MP4 typing + some parsed metadata in a full parser set. |
CHANGES.txt |
Documents the new Motion Photo video embedded-document behavior. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
The declaration is exact, the trailer runs from where it starts to the end of the file, so Content-Length can be set the way ZipParser sets it from the entry, AbstractPOIFSExtractor from the OLE2 directory and the PDF parser from the embedded file specification. A client should not have to read an embedded document to learn how big it is.
… length TikaInputStream takes the length from Content-Length when it is given the metadata, so the parse that follows knows how long the trailer is without spooling it to measure, and an opener lets it go back to the start of the region rather than copying it to do so.
A motion photo carries a short video after the image, and Tika already reads the XMP that says where it is, but not the video itself.
Two formats, one arithmetic: a Motion Photo lists its parts in
Container:Directory, the primary image first and the video last, with nothing behind it; the older MicroVideo givesCamera:MicroVideoOffset, the number of bytes from the end of the file to the start of the video. Either way the video ends at the end of the file, so its start follows from its own declared length. The same computation covers HEIC motion photos, whose video sits in a trailingmpvdbox whose 8 byte header is the primary item's padding; that path is reachable through the same extractor, but I have no sample file to prove it with.The image parsers emit it as an ATTACHMENT embedded document named
motion-photo.<ext>, parsed like any other embedded document. ATTACHMENT rather than INLINE because the video is not another rendering of the picture: it is literally appended behind the image, the JPEG ends at its EOI marker, and a reader that ignores the trailer still sees a complete image. It also carries what the still cannot: sound, motion, its own timeline and metadata.What is at the computed offset is typed by content, without the declared
Item:Mimeas a hint: a hint would let a wrong length pass as a video, since the mime magic returns the hint when it recognizes nothing. When the declared length does not fit the file, or the bytes there are not a video, nothing is emitted and nothing is recorded either. That is not a corrupt file worth an exception: sharing a motion photo out of the Android gallery keeps the XMP and takes the video, and the XMP that promised it is in the metadata for a client to see. The tests cover it with a file shaped the way sharing leaves one.The name follows the file's own declaration:
motion-photo.mp4whereItem:Mimesays so, and plainmotion-photofor a MicroVideo, which declares nothing. The parse that follows knows the format for certain, but by then the name is fixed: the recursive wrapper takes the embedded metadata when the embedded parse ends, so naming it afterwards has no effect (measured). Guessing an extension from detection seemed worse than carrying none.The XMP keys are keyed by namespace URI rather than by the prefix the document happens to use, the way
Camera:already was. An XMP prefix is the writer's choice, and exiftool rewrites these two namespaces toGContainer:, which would have silently cost the video on any file it has touched.The two test files declared a video in their XMP without carrying one; they now hold a small real MP4, and a third one covers the shared-without-video case.
Verified on a server built from this branch: both files answer with the video as an embedded document,
/unpackreturns its bytes unchanged (identical SHA-256 to the appended MP4), a plain JPEG is untouched, and a file whose video has been stripped yields no embedded document and no warning.https://issues.apache.org/jira/browse/TIKA-4869