Repository navigation
fix(mux): finished importers release their input storage - #6543
Conversation
Quest-Claim: 871f2f0645a0c448
A binding keeps each finished importer until its broadcast ends, so the track and its catalog rendition stay published for late readers. That also kept every parser's input storage: splitter tails, container atom/tag/element buffers, TS packet scratch, and a retained SPS or sequence header sliced from a whole access unit. Release that storage at its owner on finish, and copy retained parameter sets so they no longer pin the access unit they came from. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EeyiEy5FWz3z44DYZVo7vU
kixelated
changed the title
|
Outcome: finished importers and moq-video producers now drop their parser input storage, and the parameter sets they keep own their own bytes. Without the fix, all 7 new allocation-counting tests fail, with 1-3 MB still held per retired source. With it they pass. (Written by Claude Opus 5.5) |
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. |
![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: 46fa878feb
ℹ️ 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".
|
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 17 minutes. View limit details
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
WalkthroughThe change copies AV1, H.264, and H.265 configuration bytes into owned storage. Import finish paths clear buffered input or reset selected splitters before finishing. New integration tests measure heap bytes released after finished importers and producers are dropped while published tracks remain held. The release-audit quest document and its Required-list entry are removed. Priority: ➖ Normal Merge Risk: ⚪ Minimal · up to
|
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage |  |
Docstring coverage is 73.53% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 34 functions across 11 files. | Write docstrings for the functions missing them to satisfy the coverage threshold. |
-
Check name Status Explanation Title check The title clearly and concisely describes the main change: finished importers release their retained input storage. Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request. Description check The description directly explains the importer memory-release changes, retained tracks and renditions, parameter-set ownership, tests, and related documentation cleanup.
✨ Finishing Touches
-
✨ Simplify code
-
- Commit to this branch
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
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 @coderabbitai help to get the list of available commands.
They are not Client/Server session tests, so they belong in inline test modules. The counting allocator counts only on a measuring thread, so it can serve each crate's unit-test binary without disturbing other tests. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…t/finished-import-scratch
|
Merge summary:
(Written by Claude Opus 5.5) |
Summary
Bindings keep every finished importer until its broadcast ends (
Finished::keepinrs/moq-ffi), so the track stays listed and its catalog rendition stays published for late readers. That also kept each parser's input storage. On a long-lived broadcast that retires sources, every retired source pinned its largest write or element.Measured first with a counting allocator, using real ffmpeg-made 720p, 4 Mb/s inputs (H.264/H.265 Annex-B, fMP4, CMAF, MKV, TS, FLV). This is what dropping a finished importer freed while its tracks were still held:
After the fix, what remains is the same at every write size and input length, and it is the same per source for 1 or 8 retired sources: bookkeeping only. A finished
moq_video::encode::Producerdropped from about 2 MB to about 0 for 1 MiB access units.Two kinds of allocation outlived EOF:
tail(H.264/H.265/AV1, inimport::Track,import::TrackStream, the TS H.264/H.265 streams, andmoq_video::encode::Producer), the fMP4/MKV/FLVbuffer, and the TS packetscratch. Each owner now drops it infinish. MKV also kept its whole last element, because it retains the bytes from the last emitted tag onward.last_sps(H.264/H.265) andlast_seq(AV1) were slices offind_sps's copy of the whole keyframe. Each one pinned a full access-unit copy, live and after finish. They now own just the parameter set.Kept as is: tracks, catalog renditions, clean media tails, broadcast finish, and late lookup and read. Only scratch the importer can no longer use is dropped. The FFI wrappers and the audio codec lifetime from moq-dev/moq#6421 are unchanged.
Tests
Inline tests in
rs/moq-mux/src/import/mod.rsandrs/moq-video/src/encode/producer.rsinstall a counting allocator for each crate's unit-test binary. It counts only on the measuring thread, likers/moq-audio/src/capture/buffer.rs, so other tests are unaffected. Each source buffers a 1 MiB element (an access unit, a free atom, a script tag, a block) or a 1 MiB write. Each test retires 1 and then 4 sources on one broadcast, holds every published track, and asserts that dropping the finished importers frees less than a quarter of 1 MiB per source.Public API and wire impact
finishonimport::{Track, TrackStream, Container}, the container importers, andencode::Producernow drops internal scratch. Decoding afterfinishwas already invalid, because the track is finished.moq-ffiis unchanged, so the bindings, docs, and drafts need no sync.Consumer: long-lived binding broadcasts (
rs/moq-ffivideo.rs/media.rs) that retire video or imported media sources.Follow-ups (not in this PR)
find_sps/find_sequence_headercopy the whole keyframe on every keyframe just to scan for one parameter set. Scanning the frame'sByteswould make this zero-copy.Split::sps/pps) still pins one earlier input allocation. The cost is bounded per live source.finishsilently discards a truncated trailing element. Under the fail-loud rule,finishshould probably refuse instead.Quest: deletes
quest/m1/release-audit/finished-import-scratch.mdand its epic link.🤖 Generated with Claude Code
https://claude.ai/code/session_01EeyiEy5FWz3z44DYZVo7vU
(Written by Claude Opus 5.5)