feat(moq-video): add the Android MediaCodec encoder and decoder - #3354
Conversation
…dev#3354) Squashed from `pr/android-mediacodec` at f0b85fd, open as moq-dev#3354.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Team Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (13)
Included review availability: Your plan provides up to 4 included reviews per hour; 0 remain after this review. WalkthroughThe change adds Android MediaCodec hardware encoding and decoding for H.264, H.265, and AV1 decoding. It adds Android Merge Risk: ⚪ Minimal · up to No actionable merge-blocking risk is established in the current change. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 72.13% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 61 functions across 8 files. (5 skipped: 5 unsupported.)
✨ Finishing Touches✨ Simplify code
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f0b85fdf30
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Android is the one platform moq-video had no hardware encoder for, so a
phone fell back to openh264 and spent its battery on a codec the SoC
already implements. This adds MediaCodec in synchronous ByteBuffer mode,
gated on `cfg(target_os = "android")` like the objc2 and windows backend
families, and selectable as `encode::Kind::Named("mediacodec")`.
MediaCodec is a queued device: it encodes frame N while frame N+k goes
in. So each access unit is stamped with the frame it belongs to, found
through the sample time the codec echoes back in its `BufferInfo`, rather
than with whatever frame happens to be going in at the time. That is the
case moq-dev#2503 carried the timestamp through encode for.
Two things the NDK only exposes from API 28, above the API 26 this crate
builds against, are stated in the module header rather than left to be
discovered. The input buffer geometry is unavailable, so NV12 is written
tightly packed and a device whose encoder pads its input rows would shear.
The name of the opened codec is unavailable, so a device with no hardware
encoder gets the AOSP software one under the `mediacodec` name instead of
falling through to openh264, which also means `Kind::Hardware` cannot be
enforced here.
`just rs android` compiles it. Android cross-compiles from the Linux dev
shell in about half a minute, so unlike the `windows` and `macos` recipes
this one needs no special host, only an NDK.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The decode counterpart to the MediaCodec encoder, and the surface vocabulary it needs. A phone was decoding H.264 in openh264 on the CPU while its SoC had a decoder sitting idle. The decoder is configured against an `AImageReader` surface, so a decoded picture never touches system memory: it stays in the graphic buffer the codec wrote it to, and comes out as the new `Surface::HardwareBuffer` variant. An application draws it by importing the buffer as an EGLImage in its own GL context, which is what the Android media stack expects. `Surface::into_i420` stays total, so a consumer that only wants pixels reads it back and everything that matches on `Surface` keeps working. Timestamps take two hops. Output buffers carry their own `presentation_time_us`, so any reordering the codec does is the codec's problem. The reader hop is FIFO and carries no timestamp of its own, so a queue of released timestamps pairs the two up, pushed when a picture is released for rendering and popped when its image is acquired. Frame size comes from the image's crop rectangle rather than the configured format, so a 1080-line picture in a 1088-line buffer comes out at 1080. The `Surface::HardwareBuffer` variant lands here rather than on its own, because a `#[non_exhaustive]` variant with no producer is public API nothing can reach, and its private constructors would read as dead code. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Use Android's actual sync-frame key and retain the request until a frame is accepted. Preserve decoder timestamps and crop origins, drain EOS through the ImageReader, and keep the API 26 backend behind a default feature so API 24 binding builds remain valid.
f0b85fd to
4959ca2
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4959ca2644
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if self.pending_images > 0 { | ||
| tracing::warn!( | ||
| decoder = NAME, | ||
| dropped = self.pending_images, | ||
| "rendered decoder outputs never arrived at the ImageReader" | ||
| ); | ||
| self.pending_images = 0; |
There was a problem hiding this comment.
When EOS releases more images than can be acquired at once, such as when the caller still owns the reader's slots or the decoder has a tail exceeding QUEUE_DEPTH, collect keeps returning MaxImagesAcquired and this timeout path merely resets the counter. It does not remove the old images from the persistent ImageReader, so after codec.flush() the next stream's initial drain can acquire and return pictures from the previous stream with their old timestamps. Keep the flush pending or recreate/drain the reader before accepting another stream instead of forgetting these images. (Written by GPT-5.6 Sol)
Useful? React with 👍 / 👎.
| let decoder = media_codec::MediaCodec::from_decoder_type(mime) | ||
| .ok_or_else(|| Error::Codec(anyhow::anyhow!("no MediaCodec decoder for {mime}")))?; |
There was a problem hiding this comment.
On an Android device or emulator that has only a software implementation for the requested MIME type, from_decoder_type can still return that codec. Because this opener is registered in the HARDWARE candidate list, decode::Kind::Hardware then succeeds with a software decoder instead of returning NoDecoder, which is particularly plausible for AV1 and defeats callers relying on the hardware-only CPU and latency guarantee. Query or select a genuinely hardware-accelerated codec before accepting this candidate. (Written by GPT-5.6 Sol)
Useful? React with 👍 / 👎.
* docs(quest): settle scope narrowing in place, and mark pre-media sidecar placement (moq-dev#3427) Co-authored-by: Claude Fable 5.1 <noreply@anthropic.com> * fix(moq-video): pick the V4L2 mode nearest the requested resolution (moq-dev#3355) Co-authored-by: Luke Curley <kixelated@gmail.com> Co-authored-by: Codex <codex@openai.com> * feat(moq-video): add the Android MediaCodec encoder and decoder (moq-dev#3354) Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> Co-authored-by: Luke Curley <kixelated@gmail.com> * docs(quest): import the post-grooming issues as quests (moq-dev#3431) Co-authored-by: Claude Fable 5.1 <noreply@anthropic.com> * docs(quest): apply the Codex findings on the issue import (moq-dev#3432) Co-authored-by: Claude Fable 5.1 <noreply@anthropic.com> * fix(claude): adopt a quest branch at the remote tip that was inspected (moq-dev#3421) Co-authored-by: Claude Opus 5 <noreply@anthropic.com> * docs(quest): record four findings from the m1 quest wave (moq-dev#3424) Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> * docs: reorganize the site around what a reader can do (moq-dev#3426) Co-authored-by: Claude Fable 5.1 <noreply@anthropic.com> * chore: ignore Claude Code's scratch directories (moq-dev#3428) Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> * chore(moq-audio,moq-cli): assert publish_capture stays Send off macOS (moq-dev#3433) Co-authored-by: Claude Opus 5 <noreply@anthropic.com> * docs: correct claims found during merge review (moq-dev#3435) Co-authored-by: GPT-5 <noreply@openai.com> * docs(quest): import the open issues that had no quest, and gate the dev merge (moq-dev#3434) Co-authored-by: Claude Fable 5.1 <noreply@anthropic.com> * docs(moq-audio): scope the local-task guidance to macOS (moq-dev#3436) Co-authored-by: Claude Opus 5 <noreply@anthropic.com> * docs: track deferred review findings (moq-dev#3438) Co-authored-by: GPT-5 <noreply@openai.com> * chore: remove redundant packaging work and plan relay ownership fixes (moq-dev#3440) Co-authored-by: GPT-6 <noreply@openai.com> * perf(net): avoid redundant chunk copies and plan performance investigations (moq-dev#3443) Co-authored-by: GPT-6 <noreply@openai.com> * fix(transcode): follow a source resolution change with the ladder (moq-dev#3381) Co-authored-by: Claude Opus 5 <noreply@anthropic.com> Co-authored-by: GPT-5 <noreply@openai.com> * feat(watch): share one AudioContext across audio decoders Spatial playback needs every remote in the same Web Audio graph. Injected contexts are never closed. Co-Authored-By: Cursor Grok 4.6 <noreply@cursor.com> Co-authored-by: Cursor <cursoragent@cursor.com> --------- Co-authored-by: Luke Curley <kixelated@gmail.com> Co-authored-by: Claude Fable 5.1 <noreply@anthropic.com> Co-authored-by: Franz Heinzmann <frando@unbiskant.org> Co-authored-by: Codex <codex@openai.com> Co-authored-by: GPT-5 <noreply@openai.com> Co-authored-by: Cursor Grok 4.6 <noreply@cursor.com> Co-authored-by: Cursor <cursoragent@cursor.com>
2 participants
Adds Android MediaCodec hardware encode and decode to
moq-video, behind the default-onmediacodecfeature on Android. H.264 and H.265 encoding use synchronous ByteBuffer input. H.264, H.265, and device-supported AV1 decoding render into anImageReader, returningAHardwareBuffersurfaces without a CPU round trip.This PR is part of the work to update iroh-live to the latest moq; see n0-computer/iroh-live#45.
Summary
moq-videoAndroid builds.Surface::HardwareBufferrepresentation, including visible crop geometry and CPU I420 read-back.just rs androidso the Android-only source and tests can be cross-compiled locally.Review repairs
request-syncparameter key and keeps an IDR request pending until a frame is accepted. The previous key was unknown to MediaCodec and could leave a new group starting with dependent frames.AImageinstead of pairing images through a FIFO. Surface frame drops could otherwise shift every later timestamp.main.AHardwareBufferreference.mediacodecfeature. Directmoq-videoAndroid builds enable it by default, while the language bindings continue to build without default features at Android API 24.Public API changes
mediacodec,Surfacegains the non-exhaustiveHardwareBuffervariant.frame::android::HardwareBufferexposes its owned NDK buffer and visibleleft,top,width, andheightgeometry.ndkcrate version is re-exported on that target and feature so consumers do not guess an ABI type version.The language-binding surface is unchanged. Its codec-only build does not enable
mediacodecand retains the Android API 24 floor.Validation
nix develop --command just fixnix develop --command just checknix develop --command just testcargo package --locked -p moq-video --allow-dirty --no-verifyjust rs androidwith cargo-ndk 4.1.2 and NDK 29, targeting arm64-v8a at API 26cargo ndk -t arm64-v8a --platform 24 check --locked -p moq-video --no-default-features --all-targetsThe original branch was also exercised on an x86_64 API 35 emulator through an Android demo application. I did not rerun that emulator test after the review repairs. Physical-device hardware encode, AHardwareBuffer import, and live
set_bitrateremain unverified.(Written by GPT-5)