Skip to content

Bound native FMP4 interleaving memory and recover cleanly from budget errors - #352

Open
JxnLexn wants to merge 3 commits into
seydx:mainfrom
JxnLexn:fix/fmp4-timestamp-discontinuity
Open

JxnLexn wants to merge 3 commits into
seydx:mainfrom
JxnLexn:fix/fmp4-timestamp-discontinuity

Conversation

@JxnLexn

@JxnLexn JxnLexn commented Sep 15, 2026 •

Copy link
Copy Markdown
Contributor

Fixes #351.

FMP4 output can retain gigabytes of video packets when media timestamps advance too slowly for FFmpeg's time-based interleaving limit. The affected production queue held 298,704 packets (7.84 GiB) spanning only 3.32 seconds of media time.

maxInterleaveBytes bounds that native queue independently of timestamps. FMP4Stream enables a 64 MiB budget by default; generic Muxer and low-level FormatContext retain their existing defaults. Zero disables the budget. Packet backing buffers, side data and packet metadata are counted under the existing context lock. A conservative upper-bound counter is reconciled against the actual queue when needed, avoiding a full traversal on every write. Both synchronous and asynchronous writes consume a rejected packet and return ENOMEM; the high-level muxer propagates this even with exitOnError: false.

The limit covers the interleaving queue, not codec/container-private memory or total process RSS. Timestamp correction is unchanged in this PR.

FMP4 teardown keeps releasing input/codecs if Muxer.close() repeats an earlier worker error. onClose receives the original pipeline error, while an external stop() completes normally rather than rethrowing it.

Verification

  • 321 tests passed against the locally built native addon: muxer-memory, muxer, format-context, fmp4-stream, rtp-stream, demuxer and pipeline. The branch includes current main (bee585a).
  • A real FMP4 pipeline advertises H.264/AAC, withholds audio and delivers monotonically increasing one-tick video DTS. Several packets accumulate before exhausting an 8 KiB test budget; no individual packet exceeds the budget. The test verifies the budget error reaches onClose exactly once, the input closes, external stop() resolves, and a fresh session emits video successfully.
  • A separate test exercises the first external stop while muxer close repeats its worker error.
  • Async/sync native limits, queue draining and budget recounts, rejected-packet ownership, explicit flush, invalid limits and side-data accounting are covered. A 30-second backward DTS jump remains tolerated.
  • TypeScript checks for library/tests/examples, ESLint, Prettier and diff whitespace checks passed.

Native verification uses the previously rebuilt macOS-arm64 addon against Homebrew FFmpeg 63.1.101 and the repository's matching internal headers. The native patch has not changed in this revision. Linux/static release builds and a production long-run test remain outstanding; this has not been deployed to the affected container.

@seydx

seydx commented Sep 18, 2026

Copy link
Copy Markdown
Owner

@JxnLexn

Thanks for the PR

Rebuilt the addon against your branch on macOS arm64, muxer-memory 12/12 and the muxer/format-context/fmp4/rtp/demuxer/pipeline suites all green. The budget logic looks right to me.

maxInterleaveBytes is the part I want. Timestamp independent, bounds the thing that actually ate the container, gives the owner an error it can act on.

maxDtsCorrection I'd rather take separately, mainly because of the 1s default on FMP4Stream. For continuous formats, so RTSP without AVFMT_TS_DISCONT, dtsErrorThreshold only kicks in at 3600s, so a 2s or 30s backward jump from a reconnect or an NTP step walks straight into muxFixupTs. Today that gets clamped and the stream survives with a glitch, with this PR it tears the session down. On my side that's HKSV restarting on every camera clock wobble.

Can you split it? maxInterleaveBytes in one PR, maxDtsCorrection and the muxFixupTs block in a second.

The FMP4 cleanup fix in doStop has to ride along with the memory one though. Budget rejects, Muxer.close() rethrows the worker error, and the old bare await bails out of doStop before the decoders are closed, so they leak and onClose never fires. That's the path I rely on to recreate a session.

Only drop throw outputError at the end. onClose already has that error from attachCompletion, and the line makes stop() reject for every external caller.

One gap while you're at it: there's no end to end test for the byte path through FMP4Stream. The budget tests all go against the raw Muxer, the real pipeline test goes through the DTS path. Budget rejection to onClose to a clean restart is the route I depend on, would be good to have it covered.

@JxnLexn JxnLexn changed the title Bound native FMP4 buffering and reject large timestamp regressions Bound native FMP4 interleaving memory and recover cleanly from budget errors Sep 19, 2026
@JxnLexn

JxnLexn commented Sep 19, 2026

Copy link
Copy Markdown
Contributor Author

Thanks for rebuilding and checking the broader suites. I agree that the memory bound and the decision to reject a camera clock change should be separate policies.

Updated this PR in 148d0aa, then merged current main; the head is now 2cb6055:

  • Removed maxDtsCorrection and the new muxFixupTs block entirely. Existing timestamp correction is unchanged; a regression test confirms a 30-second backward jump remains accepted.
  • Kept the native byte budget and the FMP4 cleanup fix.
  • Removed the cleanup rethrow. A dedicated test now checks the first external stop() resolves even when muxer close repeats its worker error, with the input released.
  • Added the requested real FMP4 byte-budget path: both streams are advertised, audio packets are withheld, and video timestamps stay monotonically increasing by one tick. Multiple packets accumulate before an 8 KiB test budget is exhausted (no single packet exceeds it). The test observes the budget error through onClose exactly once, confirms input closure and successful stop(), then starts a fresh session that emits video normally. This path cannot be satisfied by DTS rejection, which is no longer in the branch.

All 321 tests in muxer-memory/muxer/format-context/fmp4-stream/rtp-stream/demuxer/pipeline passed against the locally built addon; TypeScript and focused lint/format checks also passed. The native patch is unchanged from the revision you rebuilt.

The separate timestamp proposal is #354. I made it opt-in, default 0 in both Muxer and FMP4Stream, so even that proposal preserves reconnect/NTP tolerance unless the owner explicitly selects a threshold. Its 91 muxer-timestamps/muxer/fmp4-stream tests passed. The description calls out that the cleanup in this memory PR should land first.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

FMP4 timestamp regression can retain gigabytes in the native interleaving queue

2 participants