TIKA-4850: Emit audio cover art as a THUMBNAIL embedded document

[dschmidt]

The audio parsers emitted cover art as INLINE, while every document container parser marks its preview image THUMBNAIL. A client that wants the representative image of a file can now take the first THUMBNAIL for audio too. The front cover (ID3 APIC / FLAC / Vorbis picture type 3) is the thumbnail; without one, the first picture of type "Other" or unknown (where taggers put unclassified main art) beats e.g. a back cover that happens to come first, and the very first picture is the last resort. Further pictures stay INLINE. MP4 covr carries no picture type, so its first image is the thumbnail. The choice is shared in CoverArt; the FLAC/Vorbis picture blocks are parsed into PictureBlock first so the choice sees all of them. Clients that looked for cover art as INLINE need to accept THUMBNAIL as well (noted in CHANGES).

https://issues.apache.org/jira/browse/TIKA-4850

[Copilot]

Pull request overview

This PR updates Tika’s audio parsers to emit the representative cover image as a THUMBNAIL embedded document (instead of INLINE), aligning audio behavior with container/document preview-image handling and enabling clients to consistently retrieve a “primary image” via the first THUMBNAIL.

Changes:

• Introduces shared thumbnail-selection logic in CoverArt (front cover if present, else first image).
• Updates MP3/OGG(FLAC+Vorbis)/MP4 parsers to mark the selected cover as THUMBNAIL and keep remaining images INLINE.
• Updates/extends tests and documents the behavioral change in CHANGES.txt.

Reviewed changes

Copilot reviewed 11 out of 11 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
tika-parsers/.../src/test/java/org/apache/tika/parser/ogg/VorbisParserTest.java	Updates assertions so the primary cover is THUMBNAIL and secondary covers remain INLINE.
tika-parsers/.../src/test/java/org/apache/tika/parser/ogg/OggAudioParserTest.java	Adds coverage for thumbnail selection rules (single image vs. front-cover preference).
tika-parsers/.../src/test/java/org/apache/tika/parser/ogg/FlacParserTest.java	Updates FLAC cover-art tests to assert THUMBNAIL for the representative image.
tika-parsers/.../src/test/java/org/apache/tika/parser/mp4/MP4ParserTest.java	Updates MP4 covr cover-art tests to treat the first image as THUMBNAIL.
tika-parsers/.../src/test/java/org/apache/tika/parser/mp3/Mp3ParserTest.java	Updates MP3 cover-art tests to assert the front cover is emitted as THUMBNAIL.
tika-parsers/.../src/main/java/org/apache/tika/parser/ogg/OggAudioParser.java	Refactors Vorbis/FLAC picture-block handling to select and emit a single THUMBNAIL.
tika-parsers/.../src/main/java/org/apache/tika/parser/ogg/FlacParser.java	Adjusts native-FLAC picture extraction to select a THUMBNAIL after collecting picture blocks.
tika-parsers/.../src/main/java/org/apache/tika/parser/mp4/boxes/TikaUserDataBox.java	Marks first MP4 covr image as THUMBNAIL, remaining as INLINE.
tika-parsers/.../src/main/java/org/apache/tika/parser/mp3/Mp3Parser.java	Emits ID3 pictures with a selected THUMBNAIL (front cover preferred).
tika-parsers/.../src/main/java/org/apache/tika/parser/audio/CoverArt.java	Adds shared logic for choosing thumbnail index and mapping to embedded resource type.
CHANGES.txt	Documents the behavior change for clients that previously relied on INLINE only.

Suppressed comments (1)

tika-parsers/tika-parsers-standard/tika-parsers-standard-modules/tika-parser-audiovideo-module/src/main/java/org/apache/tika/parser/ogg/FlacParser.java:30

• This import block contains a duplicate import java.util.List; and the java.* imports are out of order (e.g., java.nio.file.* interleaved with java.util.*). This commonly fails checkstyle rules around import grouping/order.

import java.nio.file.Path;
import java.util.Arrays;
import java.util.HashSet;
import java.util.List;
import java.util.Set;

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Copilot]

Pull request overview

Copilot reviewed 12 out of 12 changed files in this pull request and generated no new comments.

Suppressed comments (4)

Previously missed (3) — in code that hasn't changed since the last review.

tika-parsers/tika-parsers-standard/tika-parsers-standard-modules/tika-parser-audiovideo-module/src/test/java/org/apache/tika/parser/mp4/MP4ParserTest.java:204

• getResourceAsStream(...) can return null; the current try-with-resources will throw a NullPointerException with an unhelpful stack trace if the test resource is missing or mispackaged. Add an explicit null check with a clear assertion message before reading bytes.

        byte[] file;
        try (InputStream is = getResourceAsStream("/test-documents/testMP4_coverArt.m4a")) {
            file = is.readAllBytes();
        }

tika-parsers/tika-parsers-standard/tika-parsers-standard-modules/tika-parser-audiovideo-module/src/test/java/org/apache/tika/parser/mp4/MP4ParserTest.java:207

• indexOf(...) returns -1 when the needle is not found; subtracting 4 and then wrapping a ByteBuffer at a negative offset will throw with a confusing exception. Add an assertion that the udta marker was found and that the computed box size is sane before slicing.

        //append a copy of the file's udta box (with its covr) at the top level
        int udta = indexOf(file, "udta".getBytes(StandardCharsets.ISO_8859_1)) - 4;
        int size = ByteBuffer.wrap(file, udta, 4).getInt();

tika-parsers/tika-parsers-standard/tika-parsers-standard-modules/tika-parser-audiovideo-module/src/test/java/org/apache/tika/parser/mp4/MP4ParserTest.java:230

• indexOf(...) currently allocates a new byte[] on every iteration via Arrays.copyOfRange, which is avoidable and can make this test unnecessarily slow/GC-heavy on larger inputs. A simple nested-loop compare avoids per-iteration allocations.

    private static int indexOf(byte[] haystack, byte[] needle) {
        for (int i = 0; i <= haystack.length - needle.length; i++) {
            if (Arrays.equals(Arrays.copyOfRange(haystack, i, i + needle.length), needle)) {
                return i;
            }

tika-parsers/tika-parsers-standard/tika-parsers-standard-modules/tika-parser-audiovideo-module/src/main/java/org/apache/tika/parser/ogg/OggAudioParser.java:231

• This implementation builds a List where each PictureBlock holds a byte[] copy of the image payload. That means all embedded image bytes are retained until after thumbnail selection, increasing peak heap usage to roughly the sum of all picture sizes (also affecting FLAC, which now collects PictureBlocks before calling extractPictures). For files with many/large embedded images, this can materially increase memory pressure and OOM risk.

        List<PictureBlock> pictures = new ArrayList<>();
        for (String block : comments.getComments(METADATA_BLOCK_PICTURE)) {
            byte[] decoded;
            try {
                decoded = Base64.getMimeDecoder().decode(block);

[tballison]

From my agent:

  1. MEDIUM — native FLAC can emit two THUMBNAILs (reached by 2 reviewers, confirmed by me)
  FlacParser.java:104 runs OggAudioParser.extractComments (which emits metadata_block_picture comment
  pictures with its own thumbnailIndex), then :110 runs extractNativePictures with a second independent pick.
  A .flac with one PICTURE block plus one metadata_block_picture comment → two THUMBNAILs, breaking the PR's
  own one-per-file invariant. Fix: have the comment path return List<PictureBlock>, concatenate with native
  blocks in FlacParser, call extractPictures(List) once. Needs a test.

  2. LOW — the "unknown type" tier is effectively dead; docs/tests promise it (3 reviewers)
  CoverArt.java:68 uses <= OTHER, contract "negative = unknown". No caller normalizes: ID3 types are & 0xFF
  (ID3v2Frame.java:399,439) so never negative; FLAC only goes negative at ≥ 2^31. [back(4), type 200] → back
  cover wins, while CHANGES/javadoc/CoverArtTest say the unknown one should. Cheap fix: callers pass -1 when
  type >= ID3Tags.PICTURE_TYPES.length (they already compute that predicate at Mp3Parser.java:341 /
  OggAudioParser.java:275), add (4, 200) → 1 to CoverArtTest.

  3. LOW — stale javadoc on two of three sites — Mp3Parser.java:312-313, OggAudioParser.java:246-247 still
  say "front cover (or the first picture, if there is none)"; predates the "Other" tier (aa2476486e). Point
  at {@link CoverArt#thumbnailIndex} instead of restating.

  4. LOW — no ID3 parser-level test pins pass-through/order. All three MP3 fixtures are front-first, so a
  reversed or all-zero type list still passes. Mp3ParserTest already builds synthetic ID3v2 tags in-test
  (:473, :504); a back-then-front APIC pair asserting INLINE/THUMBNAIL costs ~15 lines, no binary.

  5. LOW — FLAC return→break is load-bearing and untested. A future revert to return would silently drop
  already-collected pictures (regression vs 4.0.0). One truncated-second-block test on a copy of
  testFLAC_twoCovers.flac would catch it — but confirm vorbis-java's FlacNativeFile tolerates the bad length
  first.

  6. LOW — duplicated emit loop. Mp3Parser.java:317-357 and OggAudioParser.java:251-285 are the same 25 lines
  and already diverge (null-vs-empty checks on mime/description). A CoverArt.Picture record + one
  CoverArt.extractPictures(List<Picture>, …) removes the boxed List<Integer> dance too; MP4 stays separate.
  resourceType(coverCount.getAndIncrement(), 0) reads as magic; inline the ternary.

Let me know what you think. Thank you for your iterations on this.

[dschmidt]

My pleasure, I like to solve problems for good instead of hacking around them :)

[dschmidt]

Thanks, that was a useful list. All six are in:

1 was real: extractComments now returns the pictures and FlacParser merges them with the native PICTURE blocks before one emission; testFLAC_commentAndNativePicture.flac (built from the existing fixture) pins one THUMBNAIL for a file carrying both.
2: types beyond the ID3 table count as unknown now, normalized in one place.
3: the javadocs point at CoverArt#thumbnailIndex instead of restating it.
4: a synthetic back-then-front APIC pair in Mp3ParserTest asserts INLINE/THUMBNAIL.
5: readNativePictures got a direct test: a PICTURE block declaring more data than the file has left ends the walk and keeps the earlier pictures. That avoids the vorbis-java tolerance question entirely.
6: one emission lives in CoverArt.extractPictures over a small Picture record; MP3, Ogg and FLAC share it, MP4 spells out its ternary.

[Copilot]

Pull request overview

Copilot reviewed 17 out of 18 changed files in this pull request and generated 2 comments.

[dschmidt]

Guarded: the test fails with a clear message if the fixture loses its udta box.

[dschmidt]

Removed.

[THausherr]

Also this:

[ERROR] Forbidden method invocation: java.io.DataOutput#writeBytes(java.lang.String) [Uses default charset]
[ERROR] in org.apache.tika.parser.ogg.FlacParserTest (FlacParserTest.java:146)
[ERROR] Forbidden method invocation: java.io.DataOutput#writeBytes(java.lang.String) [Uses default charset]
[ERROR] in org.apache.tika.parser.ogg.FlacParserTest (FlacParserTest.java:148)

The log didn't show it, but "explain error" shows it.

[dschmidt]

Fixed, explicit charsets now (ISO-8859-1 for the mime, UTF-8 for the description, matching PictureBlock.parse). My local loop had skipped the forbiddenapis check; noted for the next rounds.

[dschmidt]

🎉