Repository navigation
Stream backpressure: degrade the frame rate, never storm the encoder (#597) - #613
Merged
Merged
Conversation
…ncoder #597: a 10 Mbps H.264 YouTube stream produced a ~20s operator stall. The logs show the stream ran healthily for 96s (ffmpeg speed=1x), then slipped under real time, hit the compressed-video queue bound, and the core rebuilt the hardware encoder eight times in twenty seconds, each rebuild pushing a fresh ~75KB keyframe into an already-backed-up pipe. perf.log proves the UI thread was never blocked: across the 21.01s gap the sample counter advanced normally and per-sample dispatch stayed 0.0ms, so the core's snapshot emission fell to ~26% while it did the rebuilds. Compressed frames cannot be dropped, so the throttle has to act before the encoder. Owner rulings 2026-09-23: drop frame rate first and keep quality; backpressure ships on its own slice. Design mirrors the proven MonitorShedPolicy shape: queue utilisation drives an input divisor (60/30/20/15 fps), enter fast, recover slowly, hysteresis band between. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Architecture only; OBS is GPL and none of its code is copied (EncoderPolicy.h standing rule). Four changes: - Corrects a wrong claim. The first draft said compressed frames cannot be dropped. OBS drops them by priority under congestion. Our own B-frames-off configuration leaves two tiers, so our safe discard is a GOP tail: drop the queued chunks before the next keyframe, which skips instead of corrupting. - Adds a second lever. Throttling inflow stops the queue growing but never clears the ~1s already in it; a stream could stabilise a second behind and stay there. Backlog discard recovers that latency, and engages only above the higher threshold because it costs a visible skip. - Changes the measure to buffered milliseconds, taken as the wall-clock age of the oldest queued chunk. Percent-of-bound shifted meaning with bitrate and resolution, and a frames-to-ms conversion would lie precisely while the frame rate is being changed underneath it. - States encoder/connection separation as a design goal: a network fault restarts the connection and muxer and asks the running encoder for a keyframe, and must never rebuild the encoder. Also records what does not transfer: OBS measures its own socket send queue, while we hand bytes to FFmpeg, so our latency measure understates the truth. That makes the deferred egress-based health signal more urgent, not less. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Task 1 is a measurement that gates the rest: if halving the encoder's input rate does not roughly halve egress, Lever A cannot work as designed and the alternative contradicts the owner's "keep quality" ruling, so it stops and asks rather than substituting. Then the pure policy, the queue's buffered-latency telemetry, the two levers, the snapshot node, the restart floor, a slow-sink acceptance gate, and docs. The gate asserts the #597 signature specifically: the core's snapshot cadence measured against the sample counter, which is what distinguishes a producer slowdown from a UI freeze. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…r A) Measured on the real MF hardware H.264 encoder at 1920x1080@60, 10000 kbps: 60 input frames/s emits 74.40 MB per 10 s, 30 input frames/s emits 37.26 MB - ratio 0.500/0.501 over two runs, bits per frame constant (~124 KB). Two findings recorded in the test header and the task report: submit() does not pace the encoder (the compositor's keyed mutex does, so the throttle must skip the program frame, not the submit call), and the configured bitrate does not bind today (10000 and 2000 kbps both emit ~59.4 Mbps). Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…ubmit Task 1 measured that skipping only submitFrameToGpuEncoder leaves the stream byte-identical (ratio 0.998) — the encoder advances on the keyed mutex when the compositor releases a frame. Halving the export rate halves egress (0.500). The plan's gate location was wrong; the divisor now travels sender -> MediaCore -> compositor. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…mit path Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…own half Review finding on Task 2. AnUnknownMeasurementIsIgnored runs entirely at divisor 1, where a negative measurement also falls below the throttle threshold and takes the divisor_ == 1 short-circuit — so deleting the bufferedMs < 0 guard left it green. Mutation-proved: with the guard removed the old test still passes and the new AnUnknownMeasurementBreaksNeitherStreak fails, which is the #481 rule (a test that cannot construct the regression cannot catch it). Also renamed SustainedBacklogStepsDownOneLevelAtATime to ...StepsUp..., which is what it asserts, and added RecoveryStepsDownOneLevelAtATime for the step-down half nothing covered. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Task 1 measured that gating the sender's submit() changes nothing (the stream
came out byte-identical, ratio 0.998): the hardware encoder's thread advances
on the KEYED MUTEX of the encoder shared texture, so it only moves when the
compositor releases a new frame. Halving the EXPORT rate halved egress (0.500).
The throttle therefore lives in the compositor, and the divisor travels
sender -> MediaCore -> compositor.
- Interfaces.h: OutputBackpressureState{divisor} + an optional field on
OutputSender (absent = no queue to observe, NOT "healthy");
ICompositor::setEncoderExportDivisor(int) as a default no-op so Metal and the
stub are unaffected; a test-only observation seam on IOutputSender.
- D3D11CompositorAdapter: gate exportEncoderSharedTexture on
frameNumber % divisor, relaxed atomic written from the output tick.
- RtmpOutputSenderAdapter: observe the bitstream queue once per sync via
StreamBackpressurePolicy and publish the divisor; one log line per transition.
- MediaCore: take the MAX divisor across active GPU-direct senders and call
setEncoderExportDivisor only when it CHANGES, at both sync() call sites.
MAX, not per destination: one encoder texture feeds every GPU-direct sender, so
a healthy sibling IS throttled by a struggling one. Known limitation of this
slice, documented at every site; Lever B (the queue discard) stays per
destination.
Tests pin the decision where it is applied, not in the leaf policy:
RtmpOutputSenderBackpressure.TheDivisorGatesTheEncoderTextureExport drives the
real D3D11 compositor for 200 renders and asserts an exact 100/100 split;
TheSendersDivisorReachesTheCompositor drives the real sender's sync() to a
published divisor > 1 and asserts MediaCore carries the max, once per change.
Both proved RED first, including a mutation run with the plumbing removed.
Full suite 1189 passed, 0 failed (baseline 1187).
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Lever A's gate moved into the compositor in Task 4, so frames are shed once for every GPU-direct destination rather than once per sender. A per-sender shedFrames would read as each destination having shed its own; the value the sender's policy can actually report is 0. The count moves to realtimeEvidence.encoderExport alongside the effective divisor, and the policy's now-uncallable noteShedFrame()/shedFrames() are deleted there. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Review of 6125ed2 found three on-air risks and it is right on all three. 1+2 (blocking) A shed frame left frame.encoderSharedTexture empty. resolveGpuEncodePath reads an absent handle as CpuFallback, gpuPathChanged fires in ensureFfmpegProcess, and FFmpeg plus the hardware encoder are torn down and relaunched - once per shed frame, tens per second, where #597 itself was eight restarts in twenty seconds. For H.265 the relaunch is admitted off the GPU path, StreamStartAdmission refuses it, startRefusedInadmissible_ latches, and the stream is dead for the rest of the show. exportEncoderSharedTexture now takes submitPixels and its two jobs are split: the submit (the keyed-mutex release, which is what paces the encoder and what Task 1 measured) is shed; the handle metadata is published on EVERY frame, because a shed frame has not changed whether a GPU encoder texture exists. encoderSharedTexture.frameNumber now carries the last number actually submitted, tracked on the render thread - NOT publishedFrameNumber(), which the exporter's own thread stores asynchronously and which therefore cannot answer "did this render submit?". 3 The divisor latched across a stream stop. The !wantsRtmp path now resets the policy and clears sender_.backpressure, and applyEncoderExportDivisor skips stopped/idle senders - so the compositor cannot stay throttled with nothing streaming, and the next stream does not open at 15 fps on an empty queue. warning/failed still count: such a destination may still be GPU-direct with a backed-up queue. 5 lastEncoderExportDivisor_ is std::atomic<int>: it is written from the video-output thread and the audio worker's fallback, mutually exclusive only by a runtime flag. 7 The observation site now states that the policy ticks at the RENDER rate, so kEnterAfterOverWaterTicks is ~0.5s at 60Hz whatever the declared stream fps. 4+6 Tests. New RtmpOutputSenderBackpressure.AShedFrameNeverRestartsTheSendersEncodePath drives a REAL RtmpOutputSender against a REAL D3D11 compositor at divisor 2 - the one combination nothing covered, and the whole of findings 1-3 - and counts encode-path flips over 40 frames, asserting zero. TheDivisorGates... now also asserts the handle is present on every frame and that a shed frame never publishes a number the encoder was not given. TheSendersDivisor... now covers the stop reset, an absent-backpressure sender and a stopped sender. All three went RED against 6125ed2 first, one failure per finding. Filtered run only (3/3 passed, three consecutive runs): the owner's app is live with the virtual camera on, so the full suite is deferred per CLAUDE.md. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Both are one-liners in files Task 6 already opens: reset the last-submitted encoder frame number when the exporter is recreated, and name the inert stop-path divisor residual at the call site. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
… keyframe Task 4 (Lever A) throttles the encoder's input so the outgoing bitstream queue stops growing, but it never clears what is already queued, so a stream can stabilise a full second behind and sit there. This adds RtmpOutputSender::discardBacklogToNextKeyframe(), driven from observeStreamBackpressure() (called from sync()) whenever the policy raises decision.discardBacklog: it drops every chunk strictly ahead of the next queued keyframe, never the keyframe itself, and republishes queue telemetry so bitstreamBufferedMs() stops reporting the age of a chunk that no longer exists. With no keyframe queued nothing is dropped, since HEVC/AV1 here run with B-frames disabled and every frame after a keyframe is a reference frame. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Task 5 leaves backpressureDiscardedChunks_ write-only. Publishing it is half the job; per the per-stream-run ruling it must reset on the stop path beside the policy, or a new run reports the previous show's discards as its own. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…sted Task 5's review found a stream-corruption risk it could not settle statically: the discard assumes every CleanPoint chunk is a self-contained IDR with in-band parameter sets, and nothing configures that. Blocking evidence step added to the live gate, with the mitigation being to make the IDR self-contained rather than to teach the discard about headers. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…cessor Review findings on the discard mechanics itself were clean (loop boundary conditions, queue accounting, concurrency); this round addresses task quality: - Extract the discard decision into core::discardableGopTailLength(), a pure header-only function in StreamBackpressurePolicy.h testable with a bare vector<bool> and no sender, no queue, no seam at all. discardBacklogToNextKeyframe() is now a four-line locked wrapper around it. IOutputSender's test-only surface shrinks from 5 new virtuals to 2 (enqueueBitstreamChunkForTest, bitstreamQueueSnapshotForTest). - Use size() as the "not found" sentinel so "no keyframe queued" and "keyframe already at the head" are distinct, falsifiable cases instead of collapsing onto the same tautological zero. - Add a call-site test that builds a real backlog on the real queue, drives the decision through observeStreamBackpressure()/sync(), and asserts the real queue actually shrank - closing the "delete the whole if-block and every test stays green" gap. - Reset backpressureDiscardedChunks_ on the per-stream-run stop path beside the policy reset it belongs next to. - Guard the observeStreamBackpressure() call site in sync() with #if defined(_WIN32), matching the function's own guard (pre-existing POSIX compile gap from Task 4, fixed here). Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Task 5 re-review residuals. The two remaining test-only virtuals are acceptable because no wrapper forwards them AND their zero defaults make a test's precondition assert fail loudly rather than pass with nothing under test — but that reasoning has to be written down, with the instruction to move them behind a narrow interface if a third is ever wanted. Also pins [P,P,P,K] -> 3, the maximum-drop case every other test misses, and includes <cstddef> for the std::size_t the seam now names. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…ferred Task 6: OutputSender.backpressure gains the full per-destination view (level/bufferedMs/queuedChunks/enteredCount/discardedChunks/discardEvents/ lastReason/lastTransitionBufferedMs), and realtimeEvidence.encoderExport publishes the ONE effective divisor and shed-frame count the compositor actually applies, unconditionally - a divisor of 1 and 0 shed frames is the healthy reading, not an absent node. Per the amendments: shedFrames left the per-sender struct (the compositor sheds once, globally, not per destination); noteShedFrame()/shedFrames()/ shedFrames_ are deleted from StreamBackpressurePolicy as uncallable by construction, and their test is renamed to EnteredCountIsCounted. lastSubmittedEncoderFrameNumber_ now resets when encoderExport_ is recreated, and the inert stop-path divisor residual is named in a comment, not guarded. Adds the regression test the brief calls out as the actual gap: the discard-counter reset (already landed in an earlier Task 6 amendment) was unobservable and therefore untested until this task published the field. ADiscardedChunkCounterResetsOnTheNextStreamRun pins it - verified RED (failing at the exact assertion) with the reset temporarily disabled, then GREEN with it restored. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Verified against the running core: sessionState() publishes outputSenderSession .senders[], and there is no outputSenders node. The plan and spec both named a path that does not exist, which Task 8's gate would have read as 'node missing' indistinguishably from the node genuinely being absent. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…g flag Fixes every TASK QUALITY finding from the review: - Finding 1 (HIGH): observeStreamBackpressure() now gates on activeUseGpuDirect_ (the path baked into the RUNNING FFmpeg args) instead of useGpuDirect_ (only the desired path) - a destination whose FFmpeg failed to start no longer publishes a pristine healthy node. - Finding 4/9: the applyEncoderExportDivisor stop-path residual comment is moved to the return it describes and its substance corrected (publishing the divisor made it visible, not inert); encoderExport gains `exporting` so a stale divisor between shows is distinguishable from a live throttle. - Finding 5/7: the reset test now pins all five per-run counters, not just discardedChunks. - Finding 6/11: OutputBackpressureState gains `runId`, minted on every policy reconstruction, so a reset is distinguishable from a counter going backwards across a missed poll; shedFrames' process-cumulative nature is now documented against its per-run siblings. - Finding 7/2: bufferedMs is now re-read after a same-tick discard so it and queuedChunks describe the same instant. - Finding 8/3: adds observedAtMs so a frozen node (sync()'s early-return paths) is distinguishable from a fresh one. - Finding 9/4,5,10: documents discardedChunks vs discardEvents, drops the per-tick lastReason string allocation (const char*), saturates backpressureDiscardedChunks_. - Finding 3: writes and (once the testing constraint lifted) RUNS the declared coverage gap - MediaCoreCommandTest.cpp gains direct assertions on realtimeEvidence.encoderExport and outputSenderSession.senders[].backpressure. The second test was genuinely RED on first run (runId/observedAtMs were missing from MediaCore.cpp's JSON emission) and is GREEN after the fix. Full unfiltered corevideo-native-tests.exe run: 1202 passed, 0 failed. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…ting, fix the divisor latch - Item 1: adds RtmpOutputSenderBackpressure.AGpuDirectRequestRefusedForCompatibilityPublishesNoBackpressure, reproducing the actual finding-1 defect through the ordinary sync() path with no injection, no reorder, no new seam - a GPU-direct request refused for H.265-without-Enhanced-RTMP leaves useGpuDirect_ true and activeUseGpuDirect_ false with no running FFmpeg. Verified RED (gate reverted to useGpuDirect_, reason=gpu-direct measured) then GREEN. - Item 2: corrects all three comment copies claiming divisor>1 + exporting means a genuine live throttle. fullProgramReadback is vcam-OR-output-OR- recording, so exporting reads true with only the virtual camera on and no stream at all; the comments now say what the flag actually answers. - Item 4: renderVideoOutputTick now pushes an explicitly empty OutputSenderSession into applyEncoderExportDivisor on the tick senderDestinations goes empty, instead of trusting outputSender->sync()'s possibly-cached pre-stop snapshot on the last call it will ever receive. MediaCore's own senderDestinations list is authoritative and synchronous, so the divisor is corrected back to 1 instead of latching indefinitely between shows. No second reset path on the sender/policy side. - Item 3: EncoderExportIsPublishedUnconditionallyOnAFreshStubCore's shedFrames assertion null-checks the key first (Json::getNumber's missing-key fallback is also 0, so EXPECT_EQ(...,0) alone couldn't catch a typo); audited every other new snapshot assertion for the same trap - none found. Full unfiltered corevideo-native-tests.exe run: 1203 passed, 0 failed. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Task 6 fix round 2 item 4 shipped the fix without a test, judged out of scope because it needs a fake sender modelling AsyncOutputSender's cache. The existing BackpressurePublishingSender already is one -- it reports its divisor forever, destinations or not -- so the test costs nothing. Mutation-proved: reverting renderVideoOutputTick to trust the sender's cached snapshot fails it on the line that says nothing was pushed when the last destination went away. Without this, a latched divisor plus an "exporting" flag that is true whenever the vcam is on makes an idle machine read as throttled. Full suite 1204 passed, 0 failed. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…rung (#597) Step 1 found the ladder was not being bypassed - it was not being consulted. Six of the eight encoder rebuilds in the incident window carry no [outputSupervisor] restarting line at all: they came from RtmpOutputSenderAdapter::ensureFfmpegProcess re-opening its own transport from the media tick, under an adapter-local backoff whose first rung was ONE second and whose streak was cleared by the FIRST accepted frame after a rebuild. Up, one frame, dead, 1s, round again - the measured 2.5-3.5s cadence. TransportRestartFloor (pure, in OutputDestinationSupervisorPolicy.h) is the house 5/10/20/40/60s ladder in a form a second restart authority can hold, with the budget returned only after kHealthyRunMs of a run accepting output. The adapter now asks it at its single re-open gate, keyed on the elapsedMs clock MediaCore already feeds it. recover() still clears the floor, so the output supervisor stays the senior authority. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…stic Found by re-running the full suite independently: this failed about one run in three (measured 2 of 3 in isolation), which is worse than no test. It inferred "this frame was shed" from encoderSharedTexture.frameNumber not advancing. That also happens when D3DDecoupledExport takes its own bounded 3-slot refusal, where no shed occurred -- so the test counted a refusal as a shed and failed against a counter that was correct. A Lever A shed is decided by the divisor and nothing else, so the expectation is now computed from it. The Task 5 review warned about exactly this conflation and the sibling test above already avoids it; this one reintroduced it. 8 of 8 consecutive runs green after the fix. Full suite 1208 passed, 0 failed. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Finding 1, and it is the 21:09:10.088 line round 0 listed as Unsure. OutputDestinationSupervisorPolicy::observe() zeroed nextAttemptAtMs_ on a healthy INSTANT of a run past kHealthyRunMs without clearing faultPending_, so the restart gate fired on the next 250ms tick against a 5000ms rung. `healthy` comes from acceptedUnits freshness, not status, so a destination that fails while its last accepted unit is under a second old is failed and healthy in the same observation - the ordinary shape of a mid-show failure. The healthy-run branch is now gated on !faultPending_. Second half of the same finding: the supervisor's automatic restart went through recover(), which cleared the adapter floor round 0 had just added, so that route had no backstop at either level. IOutputSender gains restartForSupervisor() - default forwards to recover(), forwarded explicitly by AsyncOutputSender / CompositeOutputSender / SupervisedOutputSender per the wrapper law - and the RTMP/SRT adapter keeps its floor for it while an operator recover() still clears it. Finding 2: the floor was exempt whenever the transport was running, which gpuPathChanged / sizeChanged / audioChanged reach without any operator. The exemption is now a settings apply, running or not. Finding 3: the whole-decision test drove no supervisor despite its name. Split into RtmpOutputSender.TransportRebuildsAreNeverCloserThanTheCurrentLadderRung (adapter authority) and a new OutputDestinationSupervisor.RestartsAreNeverCloserThanTheCurrentLadderRung over the deterministic fault host, plus the pure policy property. Pre-fix both fail at +250ms; post-fix the first restart lands on the 5000ms rung. Finding 4: deleted the monotonic assertion that passed pre-fix on a 1ms margin. Finding 6: named the outputElapsedMs epoch invariant at MediaCore.cpp:8229. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…the wrapper law (#597) Item 1. Round 1 narrowed the floor exemption to a settings apply, which made a new state reachable: the gate can now refuse a rebuild while FFmpeg is still running, and it returned before stopFfmpegProcess(). The destination then spent up to a full rung with a live child fed nothing while publishing failed - and on SRT the lingering child holds the single caller slot, so the reconnect at the end of the rung is refused and the stream never comes back. The refusal path now stops the child, stamps stoppedAtMs, and names the rung in its warning. The alternative (exempt a running transport from the floor) was rejected: that is exactly the rule round 1 removed, and gpuPathChanged flaps with the transport up by definition. Items 2 and 3. restartForSupervisor shipped with nothing that failed when it was removed - FakeDestinationChild inherited the default forward, so the supervisor's own restart and an operator recover() were indistinguishable. Now counted separately and asserted at the decision site; a real-sender test pins that an armed floor survives a supervisor restart and yields to an operator recover(); and the forwarding is pinned through AsyncOutputSender's queue and through the production Composite -> Supervised -> Async stack. All five mutations fail. Item 4: the invariant TransportRestartFloor::clear() rests on is declared at the site. Item 6: one send-proof line per backoff window, not ~3,600 at rung 5. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Round-2 re-review finding. The streak clamps at kMaxConsecutiveFailures, so the "attempt K of 5" form publishes "attempt 6 of 5" forever once the ladder tops out, and "of 5" implies the destination is counting down to being abandoned. This floor retries at the top rung indefinitely; give-up belongs to the supervisor and already has its own word. The warning now states the rung and the consecutive-failure count without inventing a countdown. Full suite 1213 passed, 0 failed. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
… the encoder #597 Task 8. The acceptance gate: the first time Tasks 2-7's backpressure meets a real congested destination rather than a unit test. `--slow-sink` narrows the link to `--sink-rate` of the configured bitrate with a bandwidth-limited TCP proxy in front of an RTMP sink, then asserts the whole property: the stream never fails, the encoder is never rebuilt, Lever A's divisor steps up and comes back down, buffered latency returns below kRecoverBelowBufferedMs rather than sitting at the bound, Program holds 60fps, and the #597 signature - snapshot cadence judged against the core's own advancing SAMPLE COUNTER (realtimeEvidence.render.completedSlots), never wall time alone, because that is what separates a producer slowdown from a consumer stall. Two slow-sink mechanisms were measured and REJECTED, and both rejections are why the third is trustworthy. SRT transtype=live DROPS what it cannot deliver instead of blocking (70s: 3800 frames sent, 1984 received, our queue never aged past 0ms, Lever A never engaged) - a raised latency did not change it. FFmpeg's `-readrate` throttles MEDIA TIME, not bandwidth, and Lever A reduces bitrate while media time advances at real time by construction, so the deficit accumulates no matter how far the divisor climbs - it produced a confident, wrong negative result. A bandwidth ceiling is what #597 actually was. The throttle is VERIFIED, not assumed: the run fails unless the outgoing queue really grew past the policy's own threshold. Step 3b, the blocking evidence item, is SETTLED: an env-gated, bounded chunk trace in MediaFoundationGpuVideoEncoder shows every CleanPoint sample is a self-contained IDR carrying its parameter sets in band - h264 AUD,SPS,PPS,IDR and hevc AUD,VPS,SPS,PPS,IDR_W_RADL, 200 chunks each, zero non-keyframe chunks carrying a parameter set. Lever B's discard is safe as written; no CODECAPI_AVEncVideoPrependSPSPPSToIDR needed. The gate found a real failure and is left failing on it: 1 of 3 full-length runs stormed. The outgoing queue crossed 22 -> 60 chunks (its hard cap) inside one second, the overflow path failed the sender by design and the encoder was rebuilt. Three properties combine to lose that race - Lever B is gated on divisor > 1 so it cannot fire for the ~0.5s Lever A needs to step; the 750ms discard threshold sits only ~15 chunks below the 60-chunk cap; and Lever B's yield is phase-dependent (measured 2 to 51 chunks freed) because the GOP is the same size as the whole queue. Full cause chain and recommendations in the task-8 report; the fix is a design decision, not this task's. Also found and reported, not fixed: stopFfmpegProcess() waits 500ms for the child and never terminates, so on a congested link the destination keeps being published to after Stop and only the job object ends it at core exit. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…t storm The slow-sink gate ran the congested case three times; one run stormed with eleven encoder starts. The chain it measured: the queue crossed its 60-chunk cap inside a second, and the overflow path fails the sender on purpose so the supervisor restarts it -- which rebuilds the encoder. That is precisely what the spec forbids, and it predates this plan. Ruled: on overflow, discard the GOP tail first and fail only when there is no keyframe to discard to. That is Lever B's job at the moment it matters most, and it removes the divisor>1 race by construction. The cap does not move; an unbounded queue is unbounded latency. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
) The acceptance gate (Task 8) measured backpressure's own last-resort bound calling the restart storm this sub-project exists to remove: the outgoing bitstream queue crossed 22 chunks to its 60-chunk cap inside one second, the overflow path failed the sender ON PURPOSE so the supervisor would restart it, and the supervisor restart rebuilt the encoder. A destination fault must never rebuild the encoder. On overflow, run the GOP-tail discard FIRST and accept the arriving chunk if it freed room. Fail only when the discard frees nothing - i.e. no keyframe is queued, the one case where dropping anything corrupts the stream until the next keyframe. This is Lever B doing the job it was built for at the moment it matters most, and it removes the divisor > 1 race by construction: the overflow path does not consult the divisor at all. The 60-chunk / 2 MiB cap is UNCHANGED - an unbounded queue is unbounded latency. discardBacklogToNextKeyframe() is split into a ...Locked mutation plus a locking wrapper: the overflow path already holds bitstreamQueueMutex_, and std::mutex is not recursive, so calling the locking form there would self-deadlock the encoder's event loop. Overflow discards are counted in their own atomic (written on the encoder thread, summed into the published discardedChunks on the sync thread) and logged at most once a second. Tests drive the REAL enqueueBitstream through a new offerBitstreamChunkForTest seam - the existing seam deliberately bypasses the bound: a full queue holding a keyframe must not fail the sender (it dropped exactly the GOP tail and accepted the chunk), and a full queue with no keyframe must still fail. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…ach it (#597) Fix round 1 of Task 8b, driven by the two things round 0 could not claim. ITEM A - the cut point is a PARAMETER on the pure policy, split by site. corevideo::core::GopCutPoint::Nearest stays the default, so Lever B's ordinary discard is byte-identical: its job is recovering latency as the smallest clean skip. The queue's OVERFLOW path takes ::Last - the most room a safe cut can free - because its only alternative is failing the sender, which restarts it and rebuilds the encoder. Both rest on the same safety argument the gate proved: every keyframe here is a self-contained IDR, so a decoder resumes at any of them. Nothing inspects a chunk for anything but isKeyframe. ...AND THE ARRIVING CHUNK IS A CUT POINT TOO, which the burst gate measured: 60 queued chunks averaging 9.8 KB - all reference frames, no keyframe anywhere, so cutting to a QUEUED keyframe freed nothing - while the chunk being refused was 22.6 KB, keyframe-sized. The queue could not be cut because its own cut point was at the door. The window is one GOP wide and Lever A stretches the GOP in wall time (it sheds input frames; the keyframe interval is counted in frames), so a stalled link fills the queue before the next keyframe arrives. discardableBacklogForArrival() lets the arrival be the newest keyframe. With no keyframe queued AND none arriving the sender still fails: the bound is not optional and the 60-chunk / 2 MiB cap is unchanged. ITEM B - the gate could not reach the state it asserts about. A sustained narrowing never reaches the cap (Levers A and B hold the queue at 38-47 for twelve minutes) and narrowing further is worse: below ~0.5x the link is too narrow for FFmpeg's own RTMP handshake, so the egress child dies before it streams and the supervisor restarts a dead destination - a different failure wearing the same clothes. --burst-sink keeps the gentle sustained narrowing and STALLS the link outright for 3s every 20s, reproducing the 22-to-60-chunk onset Task 8 measured, and a burst run now FAILS unless the core logged an overflow line: a gate that cannot enter its own branch reports success while testing nothing. The storm DID recur at the cap before the arrival fix: 3 of 3 burst runs and 1 of 3 SUSTAINED runs, 7-11 encoder builds, every one logging "queue overflow with nothing safe to drop". Round 0's three sustained greens were three draws from that same ~1-in-3 rate. After: six runs (3 burst, 3 sustained), all PASS, exactly one encoder build each, seven overflow-discard events and zero nothing-safe-to-drop. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…failure (#597) Fix round 2 of Task 8b. ITEM 0 - the gate could not run HEVC at all, and the bug was not where it looked. MediaCore::startProgramOutput reads streamOutputProfile.codec into streamVideoCodec_ and then OVERWRITES every rtmp/rtmps destination's videoCodec with it, so sending the codec only in destinationSettings left the default "h264" written over the h265 that was asked for. The SRT branch is not overwritten, which is why this never showed before --slow-sink moved the gate onto an RTMP sink. The gate caught its own mismatch ("expected GPU-direct codec=hevc but the core reported codec=h264") rather than silently passing an h264 run as an HEVC one. Fixed by sending streamOutputProfile with ONLY the codec: every other key there defaults to the core's current value, so passing targetBitrateMbps would have dropped the real stream from 8.2Mbps to 6 and made the congestion gate easier. ITEM 1 - the branch-entry assertion was a disjunction, so a nothing-safe-to-drop line satisfied it: a run where the discard freed nothing and the sender failed CLEARED the check. That is the third harness here able to report success while measuring nothing. Burst mode now requires at least one successful overflow-discard AND zero nothing-safe-to-drop, the latter as its own failure - it fails the sender, the supervisor restarts it and the encoder is rebuilt, which is #597 itself. ITEM 2 - the overflow failure log said "nothing safe to drop (no keyframe queued)" even when the cut DID drop chunks and only the byte budget was still over. It now names which case actually happened. ITEM 3 - discardedChunks counts both discard sites, discardEvents counts only the policy's decisions (bumping it from the encoder thread would corrupt the cooldown those counters feed). Documented at the publish site, including that discardedChunks > 0 with discardEvents == 0 is the hard-cap signature, not a counter bug. ITEM 4 - the two characterisation tests are labelled as such, with why no mutation can fail them and which test carries the killer line instead. ITEM 5 - the arrival cut point drops the whole backlog even when a narrower cut would fit: recorded at the site as the latency-first choice it is. HEVC now runs GPU-direct end to end (mft=hardware-hevc, ffprobe decodes hevc), but across three congested runs the queue never reached its cap (peaks 0/58/38 - the cap counts CHUNKS and Lever A limits the arrival rate, so at divisor 4 the queue reaches seconds of latency in ~23 chunks), so the overflow branch was never entered on HEVC and the discontinuity question has no observed discard to judge yet. All three failed the gate correctly; the h264 control run on the same build passed with 2 overflow-discards and 0 nothing-safe-to-drop. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Fix round 3. Round 2 tightened the gate to require zero "nothing safe to drop" lines and, in the same commit, reworded the message the gate greps for. The substring stopped existing, so the assertion became vacuous by construction and every reported zero was unmeasured rather than measured-zero. That is the fourth measuring instrument in this sub-project able to report success while measuring nothing, and the first introduced while fixing the previous one. The repair is not a better grep. Both branches of the failure message and the discard line now come from modules/BitstreamQueueOverflow.h and are pinned by BitstreamQueueOverflowTest.cpp, so a future rewording breaks a test instead of silently disarming a gate. The queue's two bounds are named once so the code that enforces them and the message that describes them cannot disagree. Also: the counter-split justification was wrong in detail and is corrected - discardEvents_ is a pure output that feeds no cooldown, and the real reason the two counters stay separate is the data race on the non-atomic policy object. The split is now explained on the wire, not only in a C++ comment, so a reader of the snapshot meets it. Full suite 1231 passed, 0 failed. Committed by the controller: the implementing agent completed this work and its own mutation run (reporting that both assertion halves fired, including the one that had been vacuous) but hit a session limit before committing. Mutation reverted and verified absent; build and suite re-run here. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…diagnostic technique CLAUDE.md gains a hazard-map subsection in the GPU-direct streaming section: the perf.log-gap-vs-sample-COUNTER technique, the buffered-age signal and why not a frame-count conversion, both levers with their thresholds, the measured (not reasoned) throttle location, the shed-frame-must-publish-the-handle rule, the two restart authorities, the overflow path that called the storm, the in-band parameter-set evidence, the gate command, the four instruments that could have reported success while measuring nothing, what is NOT proven, and issues #601-#607. The spec gains an Outcome section recording the Task 1 probe, the Task 7 investigation that disproved its own hypothesis, and the one re-tuned gate assertion with its reason. The BACKLOG #597 row records slice 1 shipped and names slice 2. Documentation only; no production code, test or gate changes. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…#597) The final whole-branch review's thirteen findings, all taken. Report: .superpowers/sdd/2026-09-23-stream-backpressure/final-fix-report.md THE RULING THAT REVERSES AN EARLIER ONE. The restart floor's refusal path published status="failed" for the whole rung, and this branch lengthened the rungs from 1-30s to 5-60s. The supervisor classed that Retryable, armed a fault per rung and gave up after five - so the floor that exists to protect the encoder became a new path from "congested link" to "stream off until the operator re-arms", with the exposure window grown two- to sixfold. #603 deferred it on the grounds that fixing it changes give-up semantics; that reasoning does not survive the fact that we made the defect materially worse rather than leaving it alone. isSelfManagedRetryResultCode draws the distinction, and it takes TWO halves: classify() returning None is not enough, because a destination serving a 60s rung accepts no units and the "stopped accepting output" branch arms a fault after 5s by a different door. #603 is CLOSED, not narrowed. Also: - BUILD: six GTEST_SKIP() calls in MediaCoreCommandTest.cpp's non-Windows #else branches, against a vendored shim that defines no such macro, replaced with the house [ SKIPPED ] + return idiom. Verified on the stub configuration (COREVIDEO_WITH_RTMP_OUTPUT=OFF, no system GTest), not only on Windows. - LOG REGRESSION (da21351, missed by its own review): the byte-budget overflow line read "[gpu-encode] bitstream the cut freed 60-chunk room but ..." - ungrammatical, not findable by grepping "queue overflow", missing dropped, and printing the chunk CAP as the cut's yield. Fixed and pinned on the FULL composed line; the old assertions passed against the broken sentence. - CROSS-TASK: only the operator-stop path reconstructed the backpressure policy. reopen() now does too, so a recovered destination no longer republishes its pre-failure divisor against an empty queue and spend ~30s climbing back. - OBSERVABILITY: the per-sender node publishes appliedDivisor beside divisor. Lever A is per-encoder, so the healthy sibling of a throttled destination was reporting divisor 1 while being fed at the maximum. - All three sender-failing overflow branches now carry one grepped marker, so validate-gpu-encode.mjs's attribution column counts what its comment claims. - Docs: spec 5 amended (what shipped is removal of the overflow trigger plus a rate bound, not "never rebuild the encoder"); spec 7 amended (the operator readout was never built - #610 - and shedFrames is deliberately global); the divisor ladder stated as programFps / divisor in all three places, since at a 30 fps program the floor is 7.5 fps and not 15; the HEVC discard evidence stated precisely as decodability, not A/V sync; IOutputSender's five test-only virtuals' rule corrected to match reality with a hard "no sixth" (#611); discardCounterNote de-allocated; lastReason's shared-between-levers caveat written down; one stray blank line. Windows dev suite 1236/0 (baseline 1231/0), Release core 2,349,568 bytes. Stub gate green. No live congested gate run in this wave. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Final re-review, still-open item 1. The operator stop and the supervisor restart both reset the backpressure policy; the adapter's own re-open at the end of a floor rung did not. A self-reopened transport therefore resumed at its pre-failure divisor against an empty queue and needed about thirty seconds of health to climb back. Quality only, never stream-off -- but a reopen is a new run by every other measure this adapter keeps, so the policy resets with them. Also: the CLAUDE.md note on #603 pointed "below" at a section that is above it. Full suite 1236 passed, 0 failed. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
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
2 participants
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.
Closes #597. Also closes #603, which this branch un-deferred after making it worse.
The incident
A live YouTube stream at 10 Mbps H.264 stalled the operator's app for about twenty seconds. Two things were wrong at once: the destination could not keep up, and the encoder was torn down and rebuilt eight times while that happened.
What this does
Measures how far behind a destination is, as the wall-clock age of the oldest queued chunk. Never a frame count converted to milliseconds, because that conversion is wrong precisely while the frame rate is being changed underneath it.
Two levers. One feeds the encoder fewer frames so the queue stops growing. The other discards the backlog to a keyframe, recovering latency already lost. Per-frame quality is untouched by both.
Publishes all of it at
outputSenderSession.senders[].backpressureandrealtimeEvidence.encoderExport, so a stream quietly running at a reduced rate is visible rather than inferred.Bounds the restarts. A destination cannot be rebuilt faster than its backoff rung, on either of the two restart authorities that exist.
Gates it.
node scripts/validate-gpu-encode.mjs --seconds 240 --slow-sink --burst-sinkruns a real congested destination and fails unless the stream degrades without the encoder being rebuilt.Four things the plan got wrong, found by measurement
The throttle was specified in the wrong place. Skipping only the sender's submit left the stream byte-identical; the encoder advances on the keyed mutex when the compositor releases a frame. Halving the export rate halved egress.
The root cause of the restart storm was not what the plan assumed. There were two restart authorities, and the ladder was never bypassed — it was never consulted, because the sender adapter re-opened its own transport under a separate one-second backoff that reset on the first accepted frame.
Backpressure's own last-resort bound called the storm: the queue's overflow path failed the sender on purpose so the supervisor would rebuild the encoder. It now discards the backlog instead.
The plan named a snapshot path that does not exist, which the gate would have read as a missing node.
Reviewer attention, in order
The three discard paths in
RtmpOutputSenderAdapterandStreamBackpressurePolicy. Every one must leave the decoder resuming at a self-contained keyframe.The stand-off that stops a backing-off destination being escalated to give-up. It needed two changes, not one — classifying the backoff correctly still leaves the staleness and start-grace doors open, which a review verified by reverting half the fix and watching a congested stream die in under three minutes.
The gate's own assertions. Four measuring instruments in this work could report success while measuring nothing, and one of those we introduced ourselves while fixing another.
Verification
Full native suite 1236 passed, 0 failed, on the merged tree with main. Stub configuration builds and its gate is green. Congestion gate passes with both levers firing, program holding 60 fps, and the incident's own signature clean.
Not proven, stated plainly
Six-plus clean gate runs are a finite soak, not a guarantee — the failure this guards against appeared roughly one run in three before the fix.
Restart-ladder rung 1 is pinned only by unit tests.
The HEVC overflow branch has never fired, because the queue's cap counts chunks while the throttle limits arrival rate (#607).
HEVC discards were observed not to corrupt the RTMP/FLV path — decodability, not A/V sync. The MPEG-TS/SRT case, where an unsignalled clock jump was the actual concern, remains unobserved because the congestion harness runs on an RTMP sink.
One thing to decide before shipping
This branch introduces a new silent degradation mode. A show can go out at a quarter frame rate, correctly and deliberately, with nothing in the operator console saying so. The data is published; no interface binds to it. #610 tracks that readout.
Filed, not fixed here
#601 (the encoder ignores its configured bitrate — measured about 59.5 Mbps at both 10000 and 2000 kbps, which may be this incident's real trigger and which backpressure now masks), #602, #604, #605, #606, #607, #610, #611, #612.
🤖 Generated with Claude Code