Repository navigation
quest(m1): finish the test coverage audit - #6175
Merged
Merged
Conversation
… tests wait for startup cpal's PulseAudio host maps BufferSize::Default to the server's default buffer: a 2 s tlength, 3.96 s end to end, filled in bursts. A new sink's writes were dropped until the next burst read them. Ask PulseAudio for 20 ms periods instead; other hosts keep their defaults. The device tests also assumed playback starts within 100 ms of opening. PulseAudio's idle null sink renders up to two seconds ahead, so on the nightly ARM runner every test failed before the device first pulled. They now wait until the device's clock moves on its own before writing. the_device_buffers_periods_not_seconds fails without the buffer fix (1.99 s ahead of the speaker). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
`@moq/net/zod` is exported but no test loaded it. Pin that falsy JSON frames are values rather than the end of the group, and that a frame the schema refuses throws. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…t rs gpu` `vulkan_gpu_device` could not load a Vulkan loader inside the Nix shell on a non-NixOS host, so `just rs gpu` failed before testing anything. The AMD and Intel branches now use nixpkgs Mesa's RADV or ANV driver and the vulkan-loader, as `just rs render-sw` does with lavapipe. Each run loads only its vendor's ICD, so the vendor assertion still means something. NVIDIA keeps the host driver, which GPU CI owns. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
`import::Track` is what moq-ffi, moq-c, and moq-gst publish through, and `TrackStream` is the FFI's raw byte-stream producer. Coverage showed the Track dispatch for avc1, hev1, av01, and vp9 and every TrackStream decode and finish never ran. Land a keyframe through each, and split each raw stream in 3-byte chunks so `finish` must flush the last picture (fails if it does not). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…ucer `MoqVideoEncoder` backs `moq.video.Encoder` in every wrapper, and `MoqMediaTrackStreamProducer::write` is how they publish raw Annex-B; no test in any language reached either. Encode pictures and check keyframes land at the start and at a cut, and write a chunked H.264 stream through the stream producer. The stream producer's `finish` doc (and the Swift wrapper's copy) said a trailing access unit is dropped. It is published: `finish` flushes the splitter, which the new test shows. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The MP3 importer never ran. Publish frames through `Track::audio` and check the catalog comes from the header and a group spans frames until the caller cuts. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Delete the m1 run and add its successor at the end of m2, after the release cut, with this run's baselines, findings, and what to check first. Plan the two follow-ups it found: VA-API renderer tests with no caller, and PulseAudio capture likely sharing playback's 2 s buffer. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Contributor
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 3 minutes. View limit details
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 |
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. |
This was referenced Oct 21, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Finishes the m1 run of the recurring test coverage audit (
quest/m1/audit-test-coverage.md) and schedules the m2 run.Fixes
just rs audio-devicesfailed all five playback tests on the hosted ARM runner. Two causes, both reproduced on x86_64 under CPU pinning:BufferSize::Defaultto the server's default buffer attributes: a 2 stlength, 3.96 s end to end. Playback now asks the PulseAudio host (only) for 20 ms periods, clamped to the device's reported range (playback::device::buffer_size). Other hosts keepBufferSize::Default.aec::taps_a_real_output_devicewaits for energy by deadline instead of a fixed iteration count.the_device_buffers_periods_not_secondsasserts the reported latency is under 500 ms; it fails without the fix.just rs gpucould not load a Vulkan loader inside the Nix shell on a non-NixOS host. AMD and Intel now run through the flake's Mesa ICDs andvulkan-loader, asjust rs render-swalready does. NVIDIA keeps the host driver.MoqMediaTrackStreamProducer::finishdoc (Rust and Swift) claimed it drops the trailing access unit. It publishes it; the doc now says so and a test proves it.New tests
moq_mux::import::Trackdecodes every video format (Avc1, Hev1, Av01, VP9; Avc3/Hvc1/VP8 were already covered) and MP3, including MP3 groups spanning frames until a cut.moq_mux::import::TrackStream(the FFI raw-stream producer) splits chunked Avc3, Hev1, and Av01 byte streams.MoqVideoEncoder(moq.video.Encoderin every wrapper) encodes, flushes, honorsset_bitrateandcut, and closes afterfinish.@moq/net's Zod frame reader (zod.ts, the only JS file with runtime code that no test loaded).Quests
quest/m1/audit-test-coverage.mdand adds its successor at the end of m2'sRequired, with m1 baselines, findings, and what to check first.quest/m2/vaapi-render-tests.md: the VA-API renderer import tests have no caller (GPU CI covers only NVIDIA).quest/m2/pulseaudio-capture-period.md: capture likely shares the 2 s buffer default.Public API and wire impact
No API or wire changes. Behavior change: playback through cpal's PulseAudio host (moq-audio's opt-in
pulseaudiofeature) now opens with 20 ms periods instead of the server's 2 s default. Doc-only fix onMoqMediaTrackStreamProducer::finish(Rust and Swift).Decisions (recommendations taken, easy to revisit)
Default.Overlaps with open PRs
rs/moq-audio/src/playback/driver.rs,aec.rs.moq-audiofiles.rs/moq-mux/src/import/track.rs(this PR only appends to its test module).rs/moq-ffi/src/media.rsandvideo.rsdocs (this PR changes one doc line and appends a test).Checks
just check(branch scope): green. Rust 3600 + 516 + 223 tests, the Dart and JS suites for the touched packages, and the Dart bindings freshness check (this PR regenerates one UniFFI checksum for the doc change).just rs audio-deviceslocally on x86_64 undertaskset: 1, 2, and 4 CPUs, serial and parallel, three runs each where parallel; all green.the_device_buffers_periods_not_secondsfails without the fix.just rs gpulocally on an Intel iGPU plus AMD dGPU host: green.rs audio-devicesleg will confirm).Not fixed here (owned elsewhere)
rs featuresstops at a moq-auth exampleSystemTime::nowlint (moq-dev/moq#6155, moq-dev/moq#6135).benchmark_feed_fetch_wakeshits the kio poll limit (moq-dev/moq#6112).cargo clippy -p moq-mux --all-targetsfails on a pre-existing unusedPollinbenches/jitter.rs.(Written by Claude Opus 5.5)
🤖 Generated with Claude Code