Rebuild the FFmpeg AAR with libvorbis, and make Ogg Vorbis reachable (#254)
`FFmpegCommandBuilder` has emitted `-c:a libvorbis` since the day it was written, and libvorbis was not in the AAR this app ships: the configure line omitted `--enable-libvorbis`, and `strings` on both ABIs' `libavcodec.so` named every other external encoder and not that one. The arm was unreachable from both ends, so nobody ever hit it -- but the first user to pick Ogg Vorbis would have got `Unknown encoder 'libvorbis'`. That is #238's shape again: two individually-correct facts, a builder arm and a configure line, that no test put together, and that no coverage number can see. So the binary is rebuilt rather than the arm rewritten. FFmpeg's in-tree `vorbis` encoder was already in there and was tried first; it is experimental, stereo-only, and its quality knob spans 2x its floor against libvorbis's 6x. Shipping it would have meant `-strict experimental`, a forced `-ac 2` that silently upmixes every mono source, and a slider with nowhere to go. What ships instead is the arm as originally written, `-c:a libvorbis -q:a 5`, with `OGG_VORBIS` added to the presets, `VORBIS` added to `ENCODABLE_AUDIO`, and Ogg's per-codec extension fixed so a Vorbis file is not named `.opus`. The flag is `--enable-libvorbis`, read out of ffmpeg-kit's `get_library_name()` rather than guessed: the `--enable-lame` / `--enable-opus` rule predicts `--enable-vorbis`, and that is not it. An unrecognised `--enable-*` is ignored silently, so the artifact was checked before `bin/README.md` was touched -- `libvorbis` present in both ABIs, the configure line otherwise identical, FFmpeg still n8.1.2, 10 shared libraries per ABI, every LOAD still `0x4000`. Both mutations were run on API 34 rather than predicted. Pointing the arm at `libopus` reddens the e2e test with `expected:<[audio/vorbis]> but was:<[audio/opus]>` while its `OggS` assertion still passes, which is why the track MIME is asserted and the container magic is not enough. Adding `-ac 2` back reddens it with `expected:<[1]> but was:<[2]>`: this class's own fixture is mono, so mono staying mono is an assertion rather than a claim. The unit test's load-bearing assertion inverts with this change and is rewritten to say so. It used to assert that `libvorbis` was *absent*; it now asserts the encoder name plus the two flags that must not be there. Nothing on the JVM can tell a real encoder name from a fictional one -- which is exactly how this survived four coverage waves -- so the e2e test is the only thing that proves the positive. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
Claude Opus 5
@@ -76,7 +76,7 @@ days. Read it as the current answer, and see the git history if you need the old
|
||||
`angle_indirect` and `swangle_indirect` all boot, while `auto`, `off`, `guest` and
|
||||
`swiftshader_indirect` do not. `docs/local-emulator.md` has the evidence and the per-API renderer
|
||||
table.
|
||||
- **CI runs API 37, and it gates.** The matrix is 33/34/35/36/37. **Seven** of the 71 instrumented
|
||||
- **CI runs API 37, and it gates.** The matrix is 33/34/35/36/37. **Seven** of the 72 instrumented
|
||||
tests cannot be *run* on that image, for three measured reasons and two inherited: three Media3
|
||||
tests fail inside the emulator's own `c2.goldfish.h264.decoder`, one SAF test takes the framework
|
||||
down when it rotates the display, and its sibling — the SAF picker round trip — aborts
|
||||
@@ -88,9 +88,11 @@ days. Read it as the current answer, and see the git history if you need the old
|
||||
Media3 tests plus the rotation — runs 34041156680, 34041593697, 34042397320, 34043502322 and
|
||||
34045105857. **No picker test has ever reported on the advisory leg**, which is a correction to
|
||||
what the marker's own KDoc used to say. All seven carry `@FailsOnEmulatorApi37` and run in a
|
||||
separate `continue-on-error` job; the gating leg runs the other 64 — **the same 64 for the third
|
||||
time running**, which is exactly how this paragraph goes stale unnoticed: 69−5, 70−6 and 71−7
|
||||
are all 64.
|
||||
separate `continue-on-error` job; the gating leg runs the other **65**. That figure had been 64
|
||||
three times running — 69−5, 70−6 and 71−7 are all 64 — which is exactly how this paragraph went
|
||||
stale unnoticed, because the one number a reader checks against a run had not moved while the
|
||||
suite grew twice underneath it. #254 is the first change since to move it, by adding a test and
|
||||
no marker.
|
||||
|
||||
**These two numbers move with the suite and are derived, not remembered.** `grep -cE
|
||||
'^\s*@Test' ` over `app/src/androidTest` is the first; the second is that minus the marker
|
||||
@@ -431,6 +433,14 @@ install for code that can never run — and on API 37 the full APK does not fit
|
||||
variable**, and `--no-verify` needs the repo owner's say-so each time rather than being reached
|
||||
for when the gate is inconvenient.
|
||||
|
||||
**What that keying cannot see is `bin/`.** The classifier matches `app/src/main/*` and
|
||||
`app/src/{test,androidTest}/*` and nothing else, so a commit that replaces only the committed
|
||||
FFmpeg AAR — a *different native binary* under every instrumented test — invalidates no cache and
|
||||
sweeps nothing, while the JVM gate that does run cannot execute FFmpeg at all. #254 is where that
|
||||
was noticed, and it did not hit it: the AAR and the `app/src` change that needs it are one commit,
|
||||
so the sweep ran. An AAR rebuilt on its own would not be, and should be committed alongside
|
||||
something under `app/src` or swept by hand.
|
||||
|
||||
Why it is worth tens of minutes a commit: the alternative was measured on 2026-09-06, when one PR
|
||||
spent several gating legs learning one leg at a time what a sweep answers in one pass — and the
|
||||
failing leg **moved** between runs (API 35 red then green, API 34 green then red). One leg at a
|
||||
|
||||
@@ -48,6 +48,8 @@ Everything Media3 structurally cannot do:
|
||||
|
||||
- Containers outside MP4/WebM/Ogg/WAV/AAC — MKV, MOV, AVI, FLV, MPEG-TS, WMV/ASF
|
||||
- **MP3 output** — Android has no MP3 encoder at any version; this is a platform gap
|
||||
- **Ogg Vorbis output** — the same gap: Android has no Vorbis encoder either. Encoded with
|
||||
`libvorbis`, which the bundled build carries since #254
|
||||
- GIF and image sequences
|
||||
- Input codecs with no platform decoder on the device
|
||||
- CRF and 2-pass rate control, for the quality tier
|
||||
|
||||
@@ -86,6 +86,25 @@ class FFmpegEngineTest {
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* Channels per track, or 0 for a track that does not declare any.
|
||||
*
|
||||
* Read out of the container rather than assumed from the request, because the thing worth
|
||||
* catching is an encoder that quietly changed the channel count on the way through — which is
|
||||
* exactly what a stereo-only encoder does to this class's mono fixture.
|
||||
*/
|
||||
private fun channelCounts(file: File): List<Int> {
|
||||
val extractor = MediaExtractor()
|
||||
return try {
|
||||
extractor.setDataSource(file.absolutePath)
|
||||
(0 until extractor.trackCount).map {
|
||||
extractor.getTrackFormat(it).getInteger(MediaFormat.KEY_CHANNEL_COUNT, 0)
|
||||
}
|
||||
} finally {
|
||||
extractor.release()
|
||||
}
|
||||
}
|
||||
|
||||
// --- the formats that justify bundling FFmpeg at all -------------------
|
||||
|
||||
@Test
|
||||
@@ -149,6 +168,60 @@ class FFmpegEngineTest {
|
||||
assertEquals("OggS", magic)
|
||||
}
|
||||
|
||||
/**
|
||||
* The first execution, ever, of the Vorbis encode arm — and the reason it needed one.
|
||||
*
|
||||
* `FFmpegCommandBuilder` carried `-c:a libvorbis` from the day it was written and nothing
|
||||
* could ask for it: no preset produced `AudioCodec.VORBIS` and `ContainerCapabilities` left it
|
||||
* out of the encodable set, so the arm was unreachable from both ends (#254). It was also
|
||||
* **wrong**: `--enable-libvorbis` was in neither `bin/README.md`'s configure line nor
|
||||
* `tools/ffmpeg/build-ffmpeg.sh`, and `libvorbis` was not among the encoder names in the
|
||||
* shipped `libavcodec.so`. The first user to pick Ogg Vorbis would have got "Unknown encoder
|
||||
* 'libvorbis'". #254 rebuilt the AAR with `--enable-libvorbis`; **this test is the only thing
|
||||
* in the repo that can tell whether that rebuild actually included it**, because a wrong
|
||||
* ffmpeg-kit `--enable-*` name is ignored silently and the JVM cannot tell a real encoder name
|
||||
* from a fictional one.
|
||||
*
|
||||
* ## Why the container magic is not enough here
|
||||
*
|
||||
* `encodesOpus` above stops at `OggS`, and for that test it is sufficient. Here it would be
|
||||
* **vacuous**: Vorbis and Opus are both Ogg streams, so this ticket's acceptance mutation —
|
||||
* pointing the arm at `libopus` — produces a file with byte-identical first four bytes.
|
||||
* Measured, not assumed: `-c:a libopus -b:a 128k -f ogg` on this class's own fixture writes
|
||||
* `OggS` too. So the assertion has to reach the track, and `MediaExtractor` reporting
|
||||
* `audio/vorbis` against `audio/opus` is what separates them.
|
||||
*
|
||||
* Asserted as the whole track list rather than as "contains Vorbis", which also pins that the
|
||||
* `-vn` from the audio-only path really dropped the video: a stray video track would fail here
|
||||
* rather than pass an `any { ... }` check.
|
||||
*
|
||||
* ## The channel count is the second claim, and it is not decoration
|
||||
*
|
||||
* `sample_h264.mp4` is **mono** — one AAC channel — and that is what makes this assertion
|
||||
* bite. FFmpeg's in-tree `vorbis` encoder is stereo-only, so building on it forces `-ac 2` and
|
||||
* silently upmixes every mono source, a compromise this app makes in no other arm. That
|
||||
* compromise is the reason #254 rebuilt the binary rather than shipping the in-tree encoder,
|
||||
* so re-adding `-ac 2` has to redden something: it reddens this.
|
||||
*/
|
||||
@Test
|
||||
fun encodesOggVorbisThroughAnEncoderTheBundledBinaryActuallyHas() {
|
||||
val out = convert(OutputFormat.OGG_VORBIS)
|
||||
assertTrue("no Ogg produced", out.exists() && out.length() > 0)
|
||||
|
||||
val magic = out.inputStream().use { String(it.readNBytes(4), Charsets.US_ASCII) }
|
||||
assertEquals("OggS", magic)
|
||||
assertEquals(
|
||||
"expected a lone Vorbis track -- an Opus one would carry the same OggS magic",
|
||||
listOf(MediaFormat.MIMETYPE_AUDIO_VORBIS),
|
||||
trackMimes(out),
|
||||
)
|
||||
assertEquals(
|
||||
"the fixture is mono and libvorbis takes any channel count, so nothing may upmix it",
|
||||
listOf(1),
|
||||
channelCounts(out),
|
||||
)
|
||||
}
|
||||
|
||||
/**
|
||||
* The percentage itself, which every other test in this class computes and none of them reads.
|
||||
*
|
||||
|
||||
@@ -230,6 +230,12 @@ class Media3Engine(private val context: Context) : HardwareTranscoder {
|
||||
* unreachable code buys nothing — but it is an entry waiting on a routing change rather
|
||||
* than a live one. `Media3EngineMimeTypesTest` routes all six encodable codecs and asserts
|
||||
* which three arrive, so if that set moves, the disagreement fails rather than surprises.
|
||||
*
|
||||
* **Unreachable here is not the same as unreachable.** Since #254 a Vorbis encode is a
|
||||
* thing a user can ask for — `OutputFormat.OGG_VORBIS` — and it is served by
|
||||
* `FFmpegCommandBuilder`, which is the whole point of the router rule above sending it
|
||||
* there. What stays dead is this arm specifically, because `MEDIA3_AUDIO` still excludes
|
||||
* Vorbis: Android has no Vorbis encoder at any API level, exactly as with MP3.
|
||||
*/
|
||||
internal fun audioMimeTypeFor(codec: AudioCodec): String? = when (codec) {
|
||||
AudioCodec.AAC -> MimeTypes.AUDIO_AAC
|
||||
|
||||
@@ -53,6 +53,44 @@ object FFmpegCommandBuilder {
|
||||
/** Containers in the ISO base-media family, where HEVC needs the hvc1 brand. */
|
||||
private val MP4_FAMILY = setOf(Container.MP4, Container.MOV)
|
||||
|
||||
/**
|
||||
* Ogg Vorbis, through libvorbis.
|
||||
*
|
||||
* ## This named an encoder the binary did not have, for as long as it existed
|
||||
*
|
||||
* These are the exact flags the arm carried before #254, and the arm had never run: `VORBIS`
|
||||
* was absent from `ContainerCapabilities.ENCODABLE_AUDIO` and no `OutputFormat` offered it.
|
||||
* It could not have run either. `--enable-libvorbis` was not in the AAR's configure line, and
|
||||
* `strings` on the shipped `libavcodec.so` named `libx264`, `libx265`, `libvpx`, `libmp3lame`,
|
||||
* `libopus`, `libdav1d`, `libsvtav1` and `libjxl` — no `libvorbis`. The first user to pick Ogg
|
||||
* Vorbis would have got "Unknown encoder 'libvorbis'". #254 rebuilt the AAR with
|
||||
* `--enable-libvorbis` (`bin/README.md` carries the new configure line and checksum) and made
|
||||
* the arm reachable. The flags did not have to change; the binary under them did.
|
||||
*
|
||||
* **Nothing on the JVM can tell a real encoder name from a fictional one**, which is exactly
|
||||
* how that survived four coverage waves. `FFmpegCommandBuilderTest` can only pin that this is
|
||||
* what the builder emits. That `libvorbis` is really in there is proved by `FFmpegEngineTest`'s
|
||||
* `encodesOggVorbisThroughAnEncoderTheBundledBinaryActuallyHas`, on a device, and by nothing
|
||||
* else in this repo.
|
||||
*
|
||||
* Two flags are deliberately *absent*, and both would be forced by FFmpeg's in-tree `vorbis`
|
||||
* encoder — the one the binary already had, and the one a first pass at #254 used:
|
||||
*
|
||||
* - **no `-strict experimental`**. The in-tree encoder carries `AV_CODEC_CAP_EXPERIMENTAL`
|
||||
* and libavcodec refuses it without the flag. libvorbis is not experimental.
|
||||
* - **no `-ac 2`**. The in-tree encoder is stereo-only — *"Current FFmpeg Vorbis encoder only
|
||||
* supports 2 channels."* — so it would silently upmix a mono source and downmix a surround
|
||||
* one, a compromise this app makes nowhere else. libvorbis takes any channel count, so mono
|
||||
* stays mono — the e2e test's fixture is mono and it asserts the output still is.
|
||||
*
|
||||
* `-q:a 5` is libvorbis's classic ~160 kbps setting, and the scale behind it is the third
|
||||
* reason for the rebuild. Over one 3 s clip libvorbis spans 10931..64166 bytes across q0..q10
|
||||
* where the in-tree encoder spans 7549..14645 — so libvorbis at this setting (16429 bytes)
|
||||
* already writes more than the in-tree encoder can at q10, and the knob has somewhere to go
|
||||
* if this app ever exposes it.
|
||||
*/
|
||||
private val VORBIS_ARGS = listOf("-c:a", "libvorbis", "-q:a", "5")
|
||||
|
||||
fun build(request: ConversionRequest, inputPath: String, outputPath: String): List<String> {
|
||||
val plan = CopyPlanner.plan(request.spec, request.probe)
|
||||
return buildList {
|
||||
@@ -185,7 +223,7 @@ object FFmpegCommandBuilder {
|
||||
AudioCodec.FLAC -> listOf("-c:a", "flac")
|
||||
AudioCodec.PCM -> listOf("-c:a", "pcm_s16le")
|
||||
AudioCodec.OPUS -> listOf("-c:a", "libopus", "-b:a", "128k")
|
||||
AudioCodec.VORBIS -> listOf("-c:a", "libvorbis", "-q:a", "5")
|
||||
AudioCodec.VORBIS -> VORBIS_ARGS
|
||||
else -> listOf("-c:a", "aac", "-b:a", "192k")
|
||||
}
|
||||
}
|
||||
|
||||
@@ -7,11 +7,15 @@ package org.libremediaconverter.model
|
||||
*
|
||||
* "Can MP4 carry AV1?" and "can this app make AV1?" have different answers, and remux is exactly
|
||||
* where the difference shows. MP4 carries AV1 and ALAC happily; neither engine here encodes them.
|
||||
* Matroska carries Vorbis; nothing in [org.libremediaconverter.ffmpeg.FFmpegCommandBuilder] emits a
|
||||
* Vorbis encoder. A single `isValid` boolean would answer one of those questions and give the wrong
|
||||
* Matroska carries VP8; nothing in [org.libremediaconverter.ffmpeg.FFmpegCommandBuilder] emits a
|
||||
* VP8 encoder. A single `isValid` boolean would answer one of those questions and give the wrong
|
||||
* error for the other — telling a user "MP4 cannot hold AV1" when the truth is "your AV1 file can be
|
||||
* copied into MP4, just not re-encoded to it".
|
||||
*
|
||||
* The example used to be Vorbis, and #254 is what stopped it being true — by rebuilding the
|
||||
* bundled FFmpeg, because the Vorbis arm named `libvorbis` and the binary did not carry it. The
|
||||
* gap is a video-only one now.
|
||||
*
|
||||
* So the matrix is indexed by mode: [CodecMode.COPY] asks only what the muxer accepts,
|
||||
* [CodecMode.ENCODE] additionally asks what this app can encode.
|
||||
*
|
||||
@@ -81,10 +85,28 @@ object ContainerCapabilities {
|
||||
*/
|
||||
private val ENCODABLE_VIDEO = setOf(VideoCodec.H264, VideoCodec.H265, VideoCodec.VP9)
|
||||
|
||||
/** Vorbis is absent for the same reason: nothing here emits a Vorbis encoder. */
|
||||
/**
|
||||
* Audio codecs this app can encode. Every codec any container here carries, as of #254.
|
||||
*
|
||||
* The comment this replaces said "Vorbis is absent for the same reason: nothing here emits a
|
||||
* Vorbis encoder", and it was false as written — `FFmpegCommandBuilder.audioArgs` has had a
|
||||
* Vorbis arm since the builder existed. Its absence from this set was what made that arm
|
||||
* unreachable, and nothing recorded the decision either way. It also hid a second fault: the
|
||||
* arm named `libvorbis`, which was not compiled into the bundled binary, so the format the app
|
||||
* declined to offer was one it could not actually have produced. #254 rebuilt the AAR with
|
||||
* `--enable-libvorbis` and added the codec here in the same change.
|
||||
*
|
||||
* That makes this set equal to the union of [CARRIES_AUDIO], which `ContainerCapabilitiesTest`
|
||||
* now asserts rather than leaving to be noticed. The consequence is that [validateAudio]'s
|
||||
* "this app cannot encode X audio" arm has no reachable input. It stays: the video half of the
|
||||
* same rule is live (VP8 and AV1), and this is where an ALAC or an AC-3 entry would land the
|
||||
* day the matrix carries one. It is F4-shaped — a second line of defence that cannot currently
|
||||
* be provoked — and the set-equality assertion is what turns that from a hope into a check.
|
||||
*/
|
||||
private val ENCODABLE_AUDIO = setOf(
|
||||
AudioCodec.AAC,
|
||||
AudioCodec.OPUS,
|
||||
AudioCodec.VORBIS,
|
||||
AudioCodec.MP3,
|
||||
AudioCodec.FLAC,
|
||||
AudioCodec.PCM,
|
||||
|
||||
@@ -12,8 +12,19 @@ package org.libremediaconverter.model
|
||||
* and without it `.mka`, MP4 is `.mp4` or `.m4a`. That distinction is why they are functions rather
|
||||
* than properties.
|
||||
*
|
||||
* The extension turned out to depend on a second thing, which is what [audioCodecExtensions] is
|
||||
* for — see its parameter note.
|
||||
*
|
||||
* @param ffmpegFormat the `-f` value. Named explicitly rather than left to extension inference,
|
||||
* which is unreliable for MPEG-TS and ASF.
|
||||
* @param audioCodecExtensions per-codec overrides of [audioExtension]. Ogg is the only container
|
||||
* that needs one, and it is the reason this parameter exists: one Ogg stream can hold Vorbis,
|
||||
* Opus or FLAC, and RFC 7845 §9 asks for `.opus` on an Ogg that carries Opus alone while
|
||||
* everything else in an Ogg is a plain `.ogg`. A single container-wide extension cannot say
|
||||
* both — and it said `opus` for *every* Ogg until [OutputFormat.OGG_VORBIS] existed, which
|
||||
* would have named a Vorbis file `.opus`. That is the same defect as the `FLAC` preset that
|
||||
* once declared Matroska with a `.flac` extension, which is what moved these fields onto the
|
||||
* container in the first place.
|
||||
*/
|
||||
enum class Container(
|
||||
val label: String,
|
||||
@@ -22,6 +33,7 @@ enum class Container(
|
||||
private val audioExtension: String,
|
||||
private val videoMime: String?,
|
||||
private val audioMime: String,
|
||||
private val audioCodecExtensions: Map<AudioCodec, String> = emptyMap(),
|
||||
) {
|
||||
MP4("MP4", "mp4", "mp4", "m4a", "video/mp4", "audio/mp4"),
|
||||
MOV("MOV", "mov", "mov", "m4a", "video/quicktime", "audio/mp4"),
|
||||
@@ -32,7 +44,7 @@ enum class Container(
|
||||
FLV("FLV", "flv", "flv", "flv", "video/x-flv", "video/x-flv"),
|
||||
ASF("WMV/ASF", "asf", "wmv", "wma", "video/x-ms-wmv", "audio/x-ms-wma"),
|
||||
|
||||
OGG("Ogg", "ogg", null, "opus", null, "audio/ogg"),
|
||||
OGG("Ogg", "ogg", null, "ogg", null, "audio/ogg", mapOf(AudioCodec.OPUS to "opus")),
|
||||
WAV("WAV", "wav", null, "wav", null, "audio/wav"),
|
||||
AAC_ADTS("AAC", "adts", null, "aac", null, "audio/aac"),
|
||||
MP3("MP3", "mp3", null, "mp3", null, "audio/mpeg"),
|
||||
@@ -45,7 +57,17 @@ enum class Container(
|
||||
/** Whether this container can hold a video track at all. */
|
||||
val canHoldVideo: Boolean get() = videoExtension != null
|
||||
|
||||
fun extensionFor(hasVideo: Boolean): String = if (hasVideo) videoExtension ?: audioExtension else audioExtension
|
||||
/**
|
||||
* The filename extension for an output in this container.
|
||||
*
|
||||
* [audioCodec] takes no default on purpose. A default would let a caller get `.ogg` for an
|
||||
* Opus output by saying nothing, which is exactly the silent-wrong-answer shape the audio
|
||||
* codec argument was added to close.
|
||||
*/
|
||||
fun extensionFor(hasVideo: Boolean, audioCodec: AudioCodec): String = when {
|
||||
hasVideo -> videoExtension ?: audioExtension
|
||||
else -> audioCodecExtensions[audioCodec] ?: audioExtension
|
||||
}
|
||||
|
||||
fun mimeTypeFor(hasVideo: Boolean): String = if (hasVideo) videoMime ?: audioMime else audioMime
|
||||
}
|
||||
@@ -102,7 +124,7 @@ data class OutputSpec(val container: Container, val videoCodec: VideoCodec, val
|
||||
audioCodec.isCopyOrAbsent() &&
|
||||
(videoCodec == VideoCodec.COPY || audioCodec == AudioCodec.COPY)
|
||||
|
||||
val extension: String get() = container.extensionFor(hasVideo)
|
||||
val extension: String get() = container.extensionFor(hasVideo, audioCodec)
|
||||
val mimeType: String get() = container.mimeTypeFor(hasVideo)
|
||||
|
||||
private fun VideoCodec.isCopyOrAbsent() = this == VideoCodec.COPY || this == VideoCodec.NONE
|
||||
@@ -129,6 +151,19 @@ enum class OutputFormat(val label: String, val spec: OutputSpec) {
|
||||
MP3("MP3", OutputSpec(Container.MP3, VideoCodec.NONE, AudioCodec.MP3)),
|
||||
M4A_AAC("M4A (AAC)", OutputSpec(Container.MP4, VideoCodec.NONE, AudioCodec.AAC)),
|
||||
OPUS("Opus", OutputSpec(Container.OGG, VideoCodec.NONE, AudioCodec.OPUS)),
|
||||
|
||||
/**
|
||||
* The other codec Ogg carries, and the only preset added to make an existing arm reachable.
|
||||
*
|
||||
* `FFmpegCommandBuilder` has emitted a Vorbis encoder since the builder was written, and
|
||||
* nothing could ask for it: no preset produced [AudioCodec.VORBIS] and `ContainerCapabilities`
|
||||
* refused it on the Advanced picker, so the arm was dead in both directions (#254). It was also
|
||||
* naming `libvorbis`, which the bundled FFmpeg did not carry until that same ticket rebuilt it,
|
||||
* so making it reachable meant rebuilding the binary under it. Named for
|
||||
* the container as well as the codec because [OPUS] shares that container and the two produce
|
||||
* differently-named files — `.opus` against `.ogg`.
|
||||
*/
|
||||
OGG_VORBIS("Ogg Vorbis", OutputSpec(Container.OGG, VideoCodec.NONE, AudioCodec.VORBIS)),
|
||||
FLAC("FLAC", OutputSpec(Container.FLAC, VideoCodec.NONE, AudioCodec.FLAC)),
|
||||
WAV("WAV", OutputSpec(Container.WAV, VideoCodec.NONE, AudioCodec.PCM)),
|
||||
|
||||
|
||||
@@ -106,7 +106,10 @@ class Media3MuxersTest {
|
||||
* has changed its mind and somebody should say so on purpose.
|
||||
*
|
||||
* - Media3's MP4 muxer accepts Vorbis; [ContainerCapabilities] declines to offer it, because
|
||||
* Vorbis-in-MP4 is poorly supported by players.
|
||||
* Vorbis-in-MP4 is poorly supported by players. That refusal is about **this container**,
|
||||
* not about the codec: since #254 the app encodes Vorbis for Ogg, Matroska and WebM, and
|
||||
* the assertion below is what keeps MP4 out of that list on purpose rather than by
|
||||
* omission — it is `CARRIES_AUDIO[MP4]`, so widening the encodable set cannot reach it.
|
||||
* - The matrix offers MP3 and FLAC in MP4, which is legal and which FFmpeg writes happily, but
|
||||
* Media3's MP4 muxer carries neither — so those jobs route to FFmpeg rather than failing.
|
||||
*/
|
||||
|
||||
@@ -166,6 +166,47 @@ class FFmpegCommandBuilderTest {
|
||||
assertPair(cmd(OutputFormat.OPUS), "-c:a", "libopus")
|
||||
}
|
||||
|
||||
/**
|
||||
* The arm that named an encoder the shipped binary did not contain.
|
||||
*
|
||||
* This read `-c:a libvorbis` from the day the builder was written and had never been run: no
|
||||
* preset produced [AudioCodec.VORBIS] and `ContainerCapabilities` refused it. It could not
|
||||
* have worked either — `--enable-libvorbis` was in neither `bin/README.md`'s configure line
|
||||
* nor `tools/ffmpeg/build-ffmpeg.sh`, and the string `libvorbis` was not in the shipped
|
||||
* `libavcodec.so` while `libopus`, `libmp3lame`, `libx264` and five others were. #254 rebuilt
|
||||
* the AAR with it.
|
||||
*
|
||||
* **What this test cannot do is tell you that.** `-c:a libvorbis` and `-c:a libvorbisss` are
|
||||
* the same string to a JVM assertion, which is precisely how the defect survived four coverage
|
||||
* waves and a review that asked whether every test asserted something. The positive claim —
|
||||
* that this encoder exists in the binary and produces a Vorbis track — is proved by
|
||||
* `FFmpegEngineTest.encodesOggVorbisThroughAnEncoderTheBundledBinaryActuallyHas` on a device,
|
||||
* and by nothing else in this repo.
|
||||
*
|
||||
* The two negatives are the assertions that carry real weight here, because each pins a
|
||||
* decision rather than a name. `-strict experimental` and `-ac 2` are what FFmpeg's in-tree
|
||||
* `vorbis` encoder forces, and taking the in-tree encoder would silently upmix mono; the arm's
|
||||
* KDoc has the measurements. `-f ogg` is asserted because encoder and muxer together are what
|
||||
* make the file — an encoder without its muxer is how a Vorbis stream ends up in a container
|
||||
* that will not open.
|
||||
*/
|
||||
@Test
|
||||
fun `ogg vorbis names libvorbis, with no experimental gate and no forced stereo`() {
|
||||
val args = cmd(OutputFormat.OGG_VORBIS)
|
||||
|
||||
assertPair(args, "-c:a", "libvorbis")
|
||||
assertPair(args, "-q:a", "5")
|
||||
assertPair(args, "-f", "ogg")
|
||||
assertFalse(
|
||||
"libvorbis is not experimental; -strict belongs to FFmpeg's in-tree encoder: $args",
|
||||
args.contains("-strict"),
|
||||
)
|
||||
assertFalse(
|
||||
"libvorbis takes any channel count, so mono must not be upmixed: $args",
|
||||
args.contains("-ac"),
|
||||
)
|
||||
}
|
||||
|
||||
/**
|
||||
* The arm most conversions actually take, and the only one in `audioArgs` with no test.
|
||||
*
|
||||
@@ -216,7 +257,13 @@ class FFmpegCommandBuilderTest {
|
||||
|
||||
@Test
|
||||
fun `audio only formats never carry a video encoder`() {
|
||||
listOf(OutputFormat.MP3, OutputFormat.FLAC, OutputFormat.WAV, OutputFormat.OPUS)
|
||||
listOf(
|
||||
OutputFormat.MP3,
|
||||
OutputFormat.FLAC,
|
||||
OutputFormat.WAV,
|
||||
OutputFormat.OPUS,
|
||||
OutputFormat.OGG_VORBIS,
|
||||
)
|
||||
.forEach { format ->
|
||||
val args = cmd(format)
|
||||
assertFalse("$format should not set -c:v", args.contains("-c:v"))
|
||||
|
||||
@@ -19,6 +19,16 @@ import org.junit.Test
|
||||
*/
|
||||
class ContainerCapabilitiesTest {
|
||||
|
||||
/**
|
||||
* The codecs the matrix can actually be asked about.
|
||||
*
|
||||
* `COPY` and `NONE` are excluded because [ContainerCapabilities.accepts] refuses the first
|
||||
* outright — `resolving COPY before asking the matrix is required` covers that — and answers
|
||||
* the second `true` for every container without consulting any table.
|
||||
*/
|
||||
private val realAudioCodecs = AudioCodec.entries - AudioCodec.COPY - AudioCodec.NONE
|
||||
private val realVideoCodecs = VideoCodec.entries - VideoCodec.COPY - VideoCodec.NONE
|
||||
|
||||
private val h264Source = InputProbe(
|
||||
videoCodec = "h264",
|
||||
audioCodec = "aac",
|
||||
@@ -79,10 +89,50 @@ class ContainerCapabilitiesTest {
|
||||
}
|
||||
|
||||
@Test
|
||||
fun `Matroska carries Vorbis on copy but nothing here encodes it`() {
|
||||
assertTrue(ContainerCapabilities.accepts(Container.MKV, AudioCodec.VORBIS, CodecMode.COPY))
|
||||
fun `Matroska carries VP8 on copy but nothing here encodes it`() {
|
||||
assertTrue(ContainerCapabilities.accepts(Container.MKV, VideoCodec.VP8, CodecMode.COPY))
|
||||
assertFalse(
|
||||
ContainerCapabilities.accepts(Container.MKV, AudioCodec.VORBIS, CodecMode.ENCODE),
|
||||
ContainerCapabilities.accepts(Container.MKV, VideoCodec.VP8, CodecMode.ENCODE),
|
||||
)
|
||||
}
|
||||
|
||||
/**
|
||||
* Where the copy/encode gap actually is, asserted as a set rather than as examples.
|
||||
*
|
||||
* This used to have an audio twin — Matroska carries Vorbis, and nothing was thought to encode
|
||||
* it. That was never true of the code: `FFmpegCommandBuilder` has emitted a Vorbis encoder
|
||||
* since it was written, and only `ENCODABLE_AUDIO`'s omission made the arm unreachable (#254).
|
||||
* With Vorbis in the set, **the audio gap is empty** and the mode axis earns its place on the
|
||||
* video side alone.
|
||||
*
|
||||
* Two consequences worth having pinned rather than rediscovered:
|
||||
*
|
||||
* - `validateAudio`'s "this app cannot encode X audio" arm now has no reachable input, which
|
||||
* is why no test drives it. It stays in production as the landing spot for the first ALAC
|
||||
* or AC-3 entry, and this test is what will fail the day one is carried without an encoder
|
||||
* — where before, an omission like Vorbis's could sit unnoticed for the life of the file.
|
||||
* - The video list is the real one, and asserting it as a set is what makes an accidental
|
||||
* addition visible: an encoder added for VP8 without a matching `ENCODABLE_VIDEO` entry
|
||||
* would leave this passing, but a *carried* codec quietly dropped from the encodable set
|
||||
* would not.
|
||||
*/
|
||||
@Test
|
||||
fun `the copy-only gap is video-only, and VP8 and AV1 are all of it`() {
|
||||
fun <T> gap(codecs: List<T>, accepts: (Container, T, CodecMode) -> Boolean): Set<T> =
|
||||
Container.entries.flatMap { container ->
|
||||
codecs
|
||||
.filter { accepts(container, it, CodecMode.COPY) }
|
||||
.filterNot { accepts(container, it, CodecMode.ENCODE) }
|
||||
}.toSet()
|
||||
|
||||
assertEquals(
|
||||
"no container may carry an audio codec this app cannot also encode",
|
||||
emptySet<AudioCodec>(),
|
||||
gap(realAudioCodecs, ContainerCapabilities::accepts),
|
||||
)
|
||||
assertEquals(
|
||||
setOf(VideoCodec.VP8, VideoCodec.AV1),
|
||||
gap(realVideoCodecs, ContainerCapabilities::accepts),
|
||||
)
|
||||
}
|
||||
|
||||
@@ -405,21 +455,30 @@ class ContainerCapabilitiesTest {
|
||||
assertEverySuggestionValid(invalid, mp3Source)
|
||||
}
|
||||
|
||||
/**
|
||||
* The spec that used to be this class's example of an unencodable audio codec, now valid.
|
||||
*
|
||||
* It asserted `"This app cannot encode Vorbis audio. It can still be copied from a Vorbis
|
||||
* source."` for exactly this spec, and the message was wrong about the app: the encoder
|
||||
* existed, unreachable (#254). Asserting the positive is what stops the omission coming back —
|
||||
* a revert of `ENCODABLE_AUDIO` fails here rather than merely restoring an old refusal that
|
||||
* reads plausible.
|
||||
*
|
||||
* The audio arm it used to cover no longer has a reachable input; `the copy-only gap is
|
||||
* video-only` above is where that is now recorded, and `copying is offered as the fix when the
|
||||
* codec is right but unencodable` still covers the live video half of the same rule.
|
||||
*/
|
||||
@Test
|
||||
fun `an audio codec this app cannot encode is refused, and copying is offered instead`() {
|
||||
// Matroska carries Vorbis; nothing here encodes it. The refusal has to say so *and* say
|
||||
// what would work, which is the audio twin of `copying is offered as the fix when the codec
|
||||
// is right but unencodable`.
|
||||
fun `Vorbis into Matroska is a re-encode this app will do`() {
|
||||
val spec = OutputSpec(Container.MKV, VideoCodec.H264, AudioCodec.VORBIS)
|
||||
|
||||
val invalid = ContainerCapabilities.validate(spec, h264Source) as? Validation.Invalid
|
||||
?: throw AssertionError("encoding Vorbis must be refused")
|
||||
|
||||
assertEquals(
|
||||
"This app cannot encode Vorbis audio. It can still be copied from a Vorbis source.",
|
||||
invalid.message,
|
||||
assertTrue(
|
||||
"Vorbis is encodable, so this spec must validate: ${ContainerCapabilities.validate(spec, h264Source)}",
|
||||
ContainerCapabilities.validate(spec, h264Source).isValid,
|
||||
)
|
||||
assertEverySuggestionValid(invalid, h264Source)
|
||||
// The plan has to reach the encoder, not merely be permitted: an AAC source into Matroska
|
||||
// cannot be upgraded to a copy, so this is an Encode carrying the codec that was asked for.
|
||||
assertEquals(AudioPlan.Encode(AudioCodec.VORBIS), CopyPlanner.plan(spec, h264Source).audio)
|
||||
}
|
||||
|
||||
@Test
|
||||
|
||||
@@ -330,6 +330,28 @@ class ConversionRouterTest {
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* Ogg Vorbis leaves the hardware path one rule earlier than its Ogg sibling, and the reason
|
||||
* shown to the user is the difference.
|
||||
*
|
||||
* Two rules would each send it to FFmpeg — Media3 cannot encode Vorbis, and it cannot write
|
||||
* Ogg at all — and the order decides which explanation appears. The audio-encoder check runs
|
||||
* first deliberately: `NO_PLATFORM_ENCODER` ("Android has no encoder for this format") is true
|
||||
* of Vorbis on every Android version and tells the user something about their choice, where
|
||||
* `CONTAINER_UNSUPPORTED` would name an internal boundary they cannot act on. That ordering is
|
||||
* documented in the router and this is what holds it — asserting only the engine would pass
|
||||
* with the two rules swapped.
|
||||
*/
|
||||
@Test
|
||||
fun `ogg vorbis routes to ffmpeg because Android has no Vorbis encoder`() {
|
||||
val d = route(OutputFormat.OGG_VORBIS)
|
||||
assertEquals(Engine.FFMPEG, d.engine)
|
||||
assertEquals(Reason.NO_PLATFORM_ENCODER, d.reason)
|
||||
// The sibling in the same container stops at the container rule instead, because Media3
|
||||
// *can* encode Opus. One container, two reasons, and only the codec differs.
|
||||
assertEquals(Reason.CONTAINER_UNSUPPORTED, route(OutputFormat.OPUS).reason)
|
||||
}
|
||||
|
||||
/** M4A is the audio format that does stay on hardware, because its container is MP4. */
|
||||
@Test
|
||||
fun `m4a stays on hardware because MP4 is a container Media3 can write`() {
|
||||
|
||||
@@ -75,9 +75,14 @@ class OutputFormatTest {
|
||||
fun `every container names an extension, a mime type and an ffmpeg muxer`() {
|
||||
Container.entries.forEach { container ->
|
||||
listOf(true, false).forEach { hasVideo ->
|
||||
val ext = container.extensionFor(hasVideo)
|
||||
assertTrue("$container has no extension", ext.isNotBlank())
|
||||
assertFalse("$container extension has a dot", ext.startsWith("."))
|
||||
// Every audio codec, because the extension now varies by one — see the Ogg pair
|
||||
// below. A container that answered blank for a codec it carries would be a
|
||||
// filename with no extension at all.
|
||||
AudioCodec.entries.forEach { audioCodec ->
|
||||
val ext = container.extensionFor(hasVideo, audioCodec)
|
||||
assertTrue("$container/$audioCodec has no extension", ext.isNotBlank())
|
||||
assertFalse("$container/$audioCodec extension has a dot", ext.startsWith("."))
|
||||
}
|
||||
assertTrue(
|
||||
"$container has no mime type",
|
||||
container.mimeTypeFor(hasVideo).contains('/'),
|
||||
@@ -89,10 +94,34 @@ class OutputFormatTest {
|
||||
|
||||
@Test
|
||||
fun `audio-only variants of a container get their own extension`() {
|
||||
assertEquals("mp4", Container.MP4.extensionFor(hasVideo = true))
|
||||
assertEquals("m4a", Container.MP4.extensionFor(hasVideo = false))
|
||||
assertEquals("mkv", Container.MKV.extensionFor(hasVideo = true))
|
||||
assertEquals("mka", Container.MKV.extensionFor(hasVideo = false))
|
||||
assertEquals("mp4", Container.MP4.extensionFor(hasVideo = true, audioCodec = AudioCodec.AAC))
|
||||
assertEquals("m4a", Container.MP4.extensionFor(hasVideo = false, audioCodec = AudioCodec.AAC))
|
||||
assertEquals("mkv", Container.MKV.extensionFor(hasVideo = true, audioCodec = AudioCodec.AAC))
|
||||
assertEquals("mka", Container.MKV.extensionFor(hasVideo = false, audioCodec = AudioCodec.AAC))
|
||||
}
|
||||
|
||||
/**
|
||||
* The second thing the extension depends on, and the reason [Container.extensionFor] takes a
|
||||
* codec at all.
|
||||
*
|
||||
* One Ogg stream holds Vorbis or Opus, and the two are named differently: RFC 7845 §9 asks for
|
||||
* `.opus` on an Ogg carrying Opus alone, while a Vorbis one is a plain `.ogg`. The container
|
||||
* declared `opus` for every Ogg until [OutputFormat.OGG_VORBIS] existed, which would have
|
||||
* shipped a Vorbis file called `.opus` — the same shape as the `FLAC` preset that once
|
||||
* declared Matroska with a `.flac` extension, which is the regression guarded above.
|
||||
*
|
||||
* Both halves are asserted. Pinning only the Vorbis one would pass just as well if the
|
||||
* override map were deleted and every Ogg went back to a single extension, which is the
|
||||
* mutation that has to fail.
|
||||
*/
|
||||
@Test
|
||||
fun `Ogg names its file after the codec in it, not after the container`() {
|
||||
assertEquals("opus", OutputFormat.OPUS.extension)
|
||||
assertEquals("ogg", OutputFormat.OGG_VORBIS.extension)
|
||||
// The MIME type does not split the same way: audio/ogg is correct for both, so the SAF
|
||||
// create-document contract sees one type for the two formats.
|
||||
assertEquals("audio/ogg", OutputFormat.OPUS.mimeType)
|
||||
assertEquals("audio/ogg", OutputFormat.OGG_VORBIS.mimeType)
|
||||
}
|
||||
|
||||
/** Regression guard: FLAC used to be declared as Matroska with a `.flac` extension. */
|
||||
|
||||
+23
-9
@@ -22,19 +22,33 @@ It also removes roughly forty minutes from every cold CI run.
|
||||
| API level | 33, matching the app's minSdk |
|
||||
| ABIs | arm64-v8a, x86_64 |
|
||||
| Shared libraries | 20 (10 per ABI) |
|
||||
| SHA-256 | `ae188c9aec3c89a1c87a169589253c85438d57cfdcc3ce8b40fb3e87de368ff2` |
|
||||
| SHA-256 | `c8f4491d2c626566cbf18d5035513c1a5d8049e6696531342ea030c5427df507` |
|
||||
| Rebuilt | 2026-09-06, to add libvorbis (#254). Previous archive: `ae188c9a…`, same tag and FFmpeg version, one library fewer |
|
||||
|
||||
Configure line, read back out of the shipped `libavutil.so`:
|
||||
|
||||
```
|
||||
--enable-asm --enable-cross-compile --enable-gpl --enable-iconv
|
||||
--enable-inline-asm --enable-jni --enable-libass --enable-libdav1d
|
||||
--enable-libfontconfig --enable-libfreetype --enable-libfribidi
|
||||
--enable-libharfbuzz --enable-libjxl --enable-libmp3lame --enable-libopus
|
||||
--enable-libsvtav1 --enable-libvpx --enable-libx264 --enable-libx265
|
||||
--enable-lto --enable-mediacodec --enable-neon --enable-optimizations
|
||||
--enable-pic --enable-pthreads --enable-shared --enable-small
|
||||
--enable-swscale --enable-v4l2-m2m --enable-version3 --enable-zlib
|
||||
--enable-asm --enable-cross-compile --enable-gpl --enable-iconv
|
||||
--enable-inline-asm --enable-jni --enable-libass --enable-libdav1d
|
||||
--enable-libfontconfig --enable-libfreetype --enable-libfribidi
|
||||
--enable-libharfbuzz --enable-libjxl --enable-libmp3lame --enable-libopus
|
||||
--enable-libsvtav1 --enable-libvorbis --enable-libvpx --enable-libx264
|
||||
--enable-libx265 --enable-lto --enable-mediacodec --enable-neon
|
||||
--enable-optimizations --enable-pic --enable-pthreads --enable-shared
|
||||
--enable-small --enable-swscale --enable-v4l2-m2m --enable-version3
|
||||
--enable-zlib
|
||||
```
|
||||
|
||||
`--enable-libvorbis` is the one that arrived late, in #254, and the two ways to get it wrong are
|
||||
worth having written down. ffmpeg-kit's `--enable-*` names are its own — `--enable-lame` for
|
||||
libmp3lame, `--enable-opus` for libopus — so `--enable-vorbis` is the plausible guess and it is not
|
||||
the flag; `get_library_name()` in the upstream `scripts/function.sh` calls library 9 `libvorbis`.
|
||||
And an unrecognised `--enable-*` is **ignored silently**, so a build that dropped it looks exactly
|
||||
like one that worked. What tells them apart is the binary:
|
||||
|
||||
```sh
|
||||
unzip -p bin/ffmpeg-kit-next-8.1.1.aar 'jni/x86_64/libavcodec.so' > /tmp/libavcodec.so
|
||||
strings /tmp/libavcodec.so | grep -x libvorbis # and the same for arm64-v8a
|
||||
```
|
||||
|
||||
Every `.so` reports `LOAD align 0x4000`, so the archive satisfies the 16 KB page-size
|
||||
|
||||
Binary file not shown.
@@ -1,9 +1,11 @@
|
||||
# Coverage-read findings
|
||||
|
||||
**Status:** ten findings, none fixed, none urgent. F1-F4 came from the 2026-08-26 read; F5 was added
|
||||
on 2026-08-27 while decomposing #132; **F6-F10 were added on 2026-09-02 from the wave-4 read**. Every
|
||||
entry here is a *code* observation — something a test would document rather than repair. The test
|
||||
gaps found in the same reads are tickets, not entries here; see [Not covered here](#not-covered-here).
|
||||
**Status:** ten findings; **F1 is closed — by #254 on 2026-09-06, which found it was a defect rather
|
||||
than the dead arm it was filed as** — and the other nine stand, none urgent. F1-F4 came from the
|
||||
2026-08-26 read; F5 was added on 2026-08-27 while decomposing #132; **F6-F10 were added on
|
||||
2026-09-02 from the wave-4 read**. Every entry here is a *code* observation — something a test
|
||||
would document rather than repair. The test gaps found in the same reads are tickets, not entries
|
||||
here; see [Not covered here](#not-covered-here).
|
||||
**Scope:** what a JaCoCo read turned up that writing a test would not fix. This is a survey, not a
|
||||
work order. Acting on any entry is a separate decision and would be its own commit.
|
||||
**Last verified:** `main` at `54ca2dd`, 2026-09-02. Coverage measured that day with
|
||||
@@ -40,7 +42,9 @@ Same vocabulary as `defect-audit.md`, deliberately, so the two read alike:
|
||||
- **No action** — recorded because it looks like a finding and is not.
|
||||
|
||||
Nothing below was observed on a device, and nothing below needs to be: every entry is a claim about
|
||||
what the code says, checkable by reading it.
|
||||
what the code says, checkable by reading it. **F1's resolution is the exception, and it had to be**:
|
||||
what that entry turned on — whether the encoder it named exists in the shipped binary — is not
|
||||
readable from the source at all.
|
||||
|
||||
---
|
||||
|
||||
@@ -110,6 +114,56 @@ files agree and to say so in one place.
|
||||
2. Correct the `ContainerCapabilities.kt:84` comment, which is false as written, and give the
|
||||
`FFmpegCommandBuilder` arm the treatment `Media3Engine.kt:221-233` already models.
|
||||
|
||||
### Resolved 2026-09-06 (#254) — and the arm was not merely unreached, it was unrunnable
|
||||
|
||||
Vorbis is now in `ENCODABLE_AUDIO`, `OutputFormat.OGG_VORBIS` is a one-tap preset beside `OPUS`,
|
||||
and `FFmpegEngineTest.encodesOggVorbisThroughAnEncoderTheBundledBinaryActuallyHas` asserts the
|
||||
produced track's MIME and its channel count. The false comment is gone.
|
||||
|
||||
**The finding this entry did not have is that `-c:a libvorbis` could never have worked.** Three
|
||||
independent sources agree and none of them is the coverage report:
|
||||
|
||||
| source | says |
|
||||
|---|---|
|
||||
| `bin/README.md`'s configure line, read back out of the shipped `libavutil.so` | `--enable-libopus`, `--enable-libmp3lame`, `--enable-libvpx`, `--enable-libx264/5`, `--enable-libdav1d`, `--enable-libsvtav1`, `--enable-libjxl` — **no `--enable-libvorbis`** |
|
||||
| `tools/ffmpeg/build-ffmpeg.sh` | neither `COMMON_LIBS` nor `EXTRA_LIBS` names it |
|
||||
| `strings` on `jni/x86_64/libavcodec.so` | the `lib*` encoder names present are `libdav1d libjxl libmp3lame libopus libsvtav1 libvpx libx264 libx265`. `libvorbis` is absent; `libavcodec/vorbisenc.c` is present |
|
||||
|
||||
So the first user to pick Ogg Vorbis would have got `Unknown encoder 'libvorbis'`. The arm was
|
||||
*wrong*, not just dead — and **nothing short of building the command and running it could have
|
||||
found that**, which is why the e2e half of this ticket is the load-bearing half. It is #238's shape
|
||||
again: two covered facts (a builder arm, a configure line) that no test put together.
|
||||
|
||||
**The AAR was rebuilt rather than the arm rewritten, and the measurements are why.** A first pass
|
||||
at this ticket implemented Vorbis on FFmpeg's in-tree `vorbisenc.c`, which the binary already had.
|
||||
It works, and it is not good enough to sit in a picker beside MP3, FLAC and Opus:
|
||||
|
||||
| | `libvorbis` | in-tree `vorbis` |
|
||||
|---|---|---|
|
||||
| experimental gate | none | **needs `-strict experimental`** |
|
||||
| channels | mono, stereo, surround | **stereo only** |
|
||||
| `-q:a 0..10`, one 3 s clip | 10931 -> 64166 bytes | 7549 -> 14645 bytes |
|
||||
|
||||
`AV_CODEC_CAP_EXPERIMENTAL` is upstream FFmpeg saying *do not ship this by accident*. The
|
||||
stereo limit forces `-ac 2`, so a mono source is silently upmixed — and **this repo's own fixture,
|
||||
`sample_h264.mp4`, is mono**, so the compromise was not hypothetical. And a quality knob spanning
|
||||
2x its floor against libvorbis's 6x has nowhere to go: libvorbis at `-q:a 5` writes 16429 bytes of
|
||||
that clip, more than the in-tree encoder produces at q10.
|
||||
|
||||
So #254 added `--enable-libvorbis` to `tools/ffmpeg/build-ffmpeg.sh` and rebuilt: a new ~35 MB blob
|
||||
in git history permanently, a new configure line and SHA-256 in `bin/README.md`. What that bought
|
||||
is the arm as originally written — `-c:a libvorbis -q:a 5`, no experimental gate, no forced
|
||||
channel count — and mono that stays mono, which the e2e test asserts alongside the track MIME.
|
||||
|
||||
Two things about the flag are worth keeping, because both are ways to get this wrong quietly.
|
||||
ffmpeg-kit's `--enable-*` names come from its own `get_library_name()` and are not FFmpeg's — it is
|
||||
`--enable-lame` for libmp3lame and `--enable-opus` for libopus — so `--enable-vorbis` is the
|
||||
plausible guess and it is **wrong**; id 9 is literally `libvorbis`, so `--enable-libvorbis` is
|
||||
right, and it pulls libogg in with it. And ffmpeg-kit does **not** error on an unrecognised
|
||||
`--enable-*`, so a rebuild that quietly omitted the library looks exactly like one that worked.
|
||||
`strings jni/*/libavcodec.so | grep -x libvorbis` and the e2e test are the only two things that
|
||||
tell those apart.
|
||||
|
||||
---
|
||||
|
||||
## F2 — `ConversionRequest.hardwareEncodeAvailable` is written, read by nothing, and its KDoc describes behaviour that was removed
|
||||
@@ -444,7 +498,7 @@ the cheaper order.
|
||||
|
||||
| ID | Finding | Severity | Evidence | Action |
|
||||
|---|---|---|---|---|
|
||||
| F1 | `FFmpegCommandBuilder` emits a Vorbis encoder `ContainerCapabilities` says does not exist | low | confirmed by inspection; unreachability traced through four call sites | **decide**: feature or dead arm — the comment is false either way |
|
||||
| F1 | `FFmpegCommandBuilder` emits a Vorbis encoder `ContainerCapabilities` says does not exist | low → **the severity was wrong** | confirmed by inspection; unreachability traced through four call sites | **closed #254 as a feature** — and the encoder it named is not in the shipped binary, so the arm could never have run |
|
||||
| F2 | `hardwareEncodeAvailable` written, never read; KDoc describes removed behaviour | low | confirmed by inspection; `FFmpegCommandBuilderTest:132` corroborates | **decide**: delete or mark vestigial |
|
||||
| F3 | `ConversionRequest.videoCodec` / `.audioCodec` have no callers | low | confirmed by inspection | delete, or keep for symmetry — **not** a test gap |
|
||||
| F4 | Two private guards reachable only by direct call | n/a | confirmed by inspection | **no action** — named exemption, per #88 |
|
||||
|
||||
@@ -500,7 +500,7 @@ Every one was read. **None of them is an e2e test gap**, which is the result:
|
||||
| 3 | `CopyPlanner:28`, `OutputFormat:222-223` | public members with no callers. **#253**, with F5 |
|
||||
| 3 | `MediaProbe:210-212` | `probeWithFFprobe`'s `catch` — **F7's sibling, and now measured**. See below |
|
||||
| 2 | `FFmpegCommandBuilder:167-168` | `COPY`/`NONE -> error(...)` — F4-shaped, deliberately exempt |
|
||||
| 1 | `FFmpegCommandBuilder:188` | the `VORBIS` encode arm. No `OutputFormat` produces it, but `ContainerCapabilities` lists it for WEBM and OGG. **#254** |
|
||||
| 1 | `FFmpegCommandBuilder:188` | the `VORBIS` encode arm. No `OutputFormat` produced it, but `ContainerCapabilities` listed it for WEBM and OGG. **#254 — closed, and it was the row that turned out to be a defect**: the arm named `libvorbis`, which was not compiled into the shipped AAR at all, so it could never have run. Closing it meant rebuilding the AAR with `--enable-libvorbis`, not editing the arm. See F1 in `coverage-read-findings.md` |
|
||||
| 1 | `ConversionWorker:231` | `?: error("Could not open the input file.")`. `UnopenableUriTest` fails the job *downstream* of it, so the elvis is unprovoked — F4-shaped, same as the two above |
|
||||
|
||||
**`MediaProbe:210-212` is the one that gained a measurement.** F7 ruled `probeWithExtractor`'s catch
|
||||
|
||||
+19
-4
@@ -60,11 +60,22 @@ Only `arm64-v8a` and `x86_64` are built, matching the app's `abiFilters`. Droppi
|
||||
|
||||
## Library selection
|
||||
|
||||
Flag names come from `get_library_name()` in the upstream `scripts/function.sh`. Two
|
||||
Flag names come from `get_library_name()` in the upstream `scripts/function.sh`. Three
|
||||
that are easy to get wrong:
|
||||
|
||||
- It is **`--enable-lame`**, not `--enable-libmp3lame`.
|
||||
- It is **`--enable-libsvtav1`** for SVT-AV1.
|
||||
- It *is* **`--enable-libvorbis`** — the rule above makes `--enable-vorbis` the natural
|
||||
guess and it is wrong. Read the function rather than extrapolating from the first two;
|
||||
library 9 is named `libvorbis` there. Enabling it also enables libogg, which ffmpeg-kit
|
||||
pulls in as its dependency without being asked.
|
||||
|
||||
**An unrecognised `--enable-*` is ignored silently.** ffmpeg-kit does not error on one, so a
|
||||
build that quietly dropped a library looks exactly like one that worked, and forty minutes
|
||||
later there is an AAR that is wrong in a way nothing in the log says. #254 is where that
|
||||
was learned, from the other end: the builder carried `-c:a libvorbis` for months against a
|
||||
binary with no libvorbis in it — unreachable, so no user ever hit it, and no build log ever
|
||||
mentioned it. Check the artifact, not the log — `strings jni/*/libavcodec.so | grep -x <name>`.
|
||||
|
||||
MP3 deserves a note: **Android has no MP3 encoder at any API level**. That is a platform
|
||||
gap, not a Media3 limitation, so `--enable-lame` is the only way the app can output MP3.
|
||||
@@ -100,11 +111,15 @@ and `x86_64`. Confirmed against the artifact rather than assumed:
|
||||
`--enable-gpl --enable-version3 --enable-libx264 --enable-libx265 --enable-libsvtav1
|
||||
--enable-libvpx --enable-libmp3lame --enable-libopus --enable-libdav1d --enable-libass
|
||||
--enable-libfontconfig --enable-libfreetype --enable-libfribidi --enable-libharfbuzz
|
||||
--enable-mediacodec --enable-jni --enable-shared --enable-small --enable-lto`
|
||||
--enable-mediacodec --enable-jni --enable-shared --enable-small --enable-lto`.
|
||||
**Since 2026-09-06 it also carries `--enable-libvorbis`** (#254), which is the only
|
||||
difference between that build and the one in `bin/` today — same tag, same FFmpeg
|
||||
version, same 10 shared libraries per ABI, all still `LOAD align 0x4000`.
|
||||
- Present and verified: `libx264` (with an x264 core banner, so genuinely linked),
|
||||
`libx265`, `libsvtav1`, `libmp3lame`, `h264_mediacodec`, `hevc_mediacodec`, `libopus`,
|
||||
`libdav1d`, the GIF encoder and muxer, libass internals (`ass_shaper_new`), and the
|
||||
`subtitles`, `scale`, `palettegen`, `paletteuse` and `concat` filters.
|
||||
`libdav1d`, `libvorbis` (from 2026-09-06), the GIF encoder and muxer, libass internals
|
||||
(`ass_shaper_new`), and the `subtitles`, `scale`, `palettegen`, `paletteuse` and
|
||||
`concat` filters.
|
||||
|
||||
Note `--enable-version3`: combined with `--enable-gpl` this makes the binary **GPL-3.0**,
|
||||
which is what `LICENSES/README.md` states.
|
||||
|
||||
@@ -28,7 +28,10 @@ OUT=/work/out
|
||||
# Library selection
|
||||
# ---------------------------------------------------------------------------
|
||||
# Flag names come from get_library_name() in scripts/function.sh — note it is
|
||||
# --enable-lame, NOT --enable-libmp3lame.
|
||||
# --enable-lame, NOT --enable-libmp3lame. Read that function before adding one: the
|
||||
# names are ffmpeg-kit's, not FFmpeg's, and they agree only sometimes. libvorbis is
|
||||
# one that does agree (id 9 is literally "libvorbis"), so --enable-libvorbis is right
|
||||
# and the --enable-vorbis this rule would predict is not.
|
||||
#
|
||||
# android-media-codec gives FFmpeg the h264_mediacodec / hevc_mediacodec wrappers.
|
||||
# Those are the fallback-within-the-fallback: hardware encode from the FFmpeg side
|
||||
@@ -41,6 +44,11 @@ COMMON_LIBS=(
|
||||
--enable-lame # MP3 encode. Android has NO MP3 encoder at any API level,
|
||||
# so this is the only way the app can output MP3 at all.
|
||||
--enable-opus
|
||||
--enable-libvorbis # Ogg Vorbis encode. Android has no Vorbis ENCODER at any API
|
||||
# level either, and FFmpeg's own in-tree vorbis encoder is
|
||||
# experimental, stereo-only and barely responds to -q:a, so
|
||||
# this is the only usable route. Pulls libogg in as its
|
||||
# dependency (ffmpeg-kit sets LIBRARY_LIBOGG with it).
|
||||
--enable-dav1d # fast AV1 decode
|
||||
)
|
||||
|
||||
|
||||
Reference in New Issue
Block a user