tballison commented on PR #3090:
URL: https://github.com/apache/tika/pull/3090#issuecomment-5480927659
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.
--
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]
