feat(twitch): Enhanced Broadcasting — two mixes in one graph, and a negotiation that falls back quietly (#326) - #333
Merged
Conversation
…inct tracks (#326) routing.Compile emitted one mix whose internal labels were fixed constants -- a_t0, a_mix, aout -- so concatenating two compiled graphs collided on every one of them. That is why nothing could set ffmpeg.DestSpec.SecondAudioOutLabel, which has been able to map and encode a second audio track since #331 and has had no way to be asked for one. Every label site now goes through a namespace. The empty namespace is byte-for-byte what the package emitted before, so a destination that gains a VOD track does not have its live mix rewritten, and a destination that never asks for one produces the argv it always did. CompilePair returns both mixes in one filter_complex plus the label to map for the second. A secondary that will not compile is a WARNING, not an error: an optional VOD track must never veto a working broadcast. A primary that will not compile is still an error -- there is no stream without it. TWO TAPS OF ONE INGEST TRACK NEED NO asplit. Both halves emit [0:a:N] for a track they share, and the obvious reading is that a filter pad feeds one input so a shared tap needs an explicit split. An input STREAM is not a filter pad and FFmpeg inserts the split itself -- measured on 6.0.1 (Alpine 3.18, the floor internal/ffmpeg/detect.go enforces) and 8.1.2. An asplit here would have been dead weight carried on a guess. TestAPairedGraphReachesFFmpegAsTwoDistinctMixes hands the real graph to the real binary and reads the tones back off both tracks, because the failure this ships with otherwise is TWO TRACKS THAT ARE THE SAME MIX -- which a track count sees as success. Mutating the VOD mix to carry the live content fails it at -18.1 dB on a tone that must be absent. It needs no skip: PCM in NUT with no video uses only codecs built into every FFmpeg, and the binary lookup goes through testenv.FFmpegBinary, whose skip lives inside internal/testenv and which fails rather than skips under POLYEMESIS_REQUIRE_FFMPEG. The skip census is unchanged at 94. All six tests mutation-verified; each mutation recorded in its test's doc comment with the observed failure. Claude-Session: https://claude.ai/code/session_01HeLrWaDmsNeeNSbHQfEofX
…326) routing.CompilePair landed with nothing able to ask for it. This is the path from a stored setting to the wire: two columns, the engine's compile step, and the one place a DestSpec is built. db.Destination gains Multitrack -- opt in to Twitch Enhanced Broadcasting -- and VODProfile, the second mix. Both default to off, and off produces byte for byte the filter graph and the argv the destination produced before the columns existed. VODProfile IS A POINTER because "no second mix" and "a second mix that happens to be the zero profile" are different things, and the zero profile fails Validate. The column stores '' rather than '{}' for the same reason and marshalVODProfile enforces the one spelling of absence at the write end: json.Marshal of a nil pointer produces `null`, which is not empty, takes the decode branch, and arrives as a nil profile by a route the reader cannot tell from a corrupt value. Had either end disagreed, every row written before this column existed would have come back carrying a second audio track that cannot compile. NOT ON THE PROVISIONAL PATH. A provisional compile already runs on a guessed layout and says so; a second guessed mix on top doubles what is approximate while the operator is being told the first one is unreliable. The VOD track returns on the first reconcile after a probe succeeds. The second label rides on routing.Result rather than only on Pair, so the engine's one description of an output carries a VOD track without a single signature changing type -- and the backup feed picks it up through the same struct, rather than silently publishing one track where the primary publishes two. THREE DRIFT GUARDS, all satisfied rather than worked around: - TestUITypesCanNameEveryDestinationField wanted a types.ts entry. - TestEveryStoredLeafIsClassified wanted all 22 new stored JSON leaves classified. Every one is sPublic: a routing profile is mix settings, not a credential. This is the #310/#324 guard and it fired correctly. - TestReadSafeViewsScrubEverySecretLeaf follows from that classification. NO CAPABILITY-MATRIX COLUMN, deliberately. Support is yes/manual/no/ unknown and Enhanced Broadcasting's honest answer is "depends whether your server has a supported GPU" -- `yes` would be false for the majority GPU-less VPS install, which is the normal case here, and the matrix answers a per-ACCOUNT question while this is a per-destination toggle. The UI toggle is gated to Twitch, worded so that not getting Enhanced Broadcasting reads as ordinary rather than as a fault. Both new db tests mutation-verified, each mutation and its observed failure recorded in the test's doc comment. A third mutation was discarded rather than counted: it failed to BUILD, which is not the named test failing. Claude-Session: https://claude.ai/code/session_01HeLrWaDmsNeeNSbHQfEofX
multitrack could describe a negotiation and could not make one. Negotiate is the decision: ask Twitch, read the verdict, resolve the endpoint, and answer where to publish. IT RETURNS NO ERROR, deliberately. Twitch refuses any client without a supported GPU, and polyemesis is built to be installed on the operator's own server -- a rented VPS has none. On most installs the fallback IS the path, every time, for ever. An error return would make the ordinary case look broken: logged at error level, counted as a fault, retried by somebody who assumed non-nil meant something had gone wrong. The caller publishes to Target or to the destination's own URL, and both are correct. A NO-GPU ASK DOES NOT MAKE THE CALL. Twitch was measured refusing, by name, a request with no GPU information, an Intel iGPU, an unrecognised vendor and an out-of-date driver -- so the answer is known, and spending a round trip at go-live to be told it would delay every broadcast on every GPU-less install to learn nothing. The test asserts the call is not made, not merely that the outcome is a fallback: the same answer arrived at slowly is the failure a Use==false assertion cannot see. A LEAK FOUND AND CLOSED. The package doc says status.html_en_us "is scrubbed before it is shown anywhere". It was not. Config.explain returns the field verbatim, and that field is Twitch QUOTING THE REQUEST BACK -- the request that carries the stream key. Only the errors Client.Fetch builds were ever scrubbed, so the transport and decode paths were safe and the verdict path was not. Every note now leaves Negotiate through one scrubbing closure. TestNoOutcomeEverCarriesTheStreamKeyInItsNote sweeps all four paths, including a server that echoes the key back the way the live endpoint does; it FAILED before this change, which is how the gap was found. Same shape as #310 and #324. The minted-key test is the important one and it is about a failure that WORKS: publishing with the operator's own key does not fail loudly, it CONNECTS and sends a ladder the ingest never agreed to. So the assertion is that the signed v1_ prefix specifically survives -- the minted key ENDS with the operator's own, so "contains the operator key" would pass on the wrong value. Its fixture is SYNTHETIC and says so. The captured fixture ships with `authentication` emptied because a real minted key is a live credential; committing one would put a working stream key in the repository, which is the thing the leak above is about. The shape is reproduced from the measurement in IngestEndpoint.Authentication. All five tests mutation-verified against the real code paths they name -- Resolve's minted-key branch, the short circuit, Verdict's StatusError case, the scrubber, and the VOD preference -- each recorded with its observed failure. VERIFIED AGAINST FIXTURES, NOT THE LIVE API. Both fixtures were captured from the live endpoint by earlier work; nothing here re-derives them and I hold no Twitch token. The success path has never been exercised against Twitch by this change. Claude-Session: https://claude.ai/code/session_01HeLrWaDmsNeeNSbHQfEofX
There was a problem hiding this comment.
Pull request overview
This PR wires Twitch Enhanced Broadcasting support through routing compilation (two mixes in one FFmpeg graph), destination storage/schema/API/UI plumbing, and a negotiation layer that deliberately falls back quietly rather than failing broadcasts.
Changes:
- Add label namespacing in
internal/routingand introduceCompilePairto safely concatenate two mixes into one-filter_complex. - Extend destinations with
multitrack(opt-in negotiation) andvodProfile(second mix) and carrySecondAudioOutLabelthrough to FFmpegDestSpec. - Add
internal/multitrack.Negotiateplus tests focused on fallback behavior and credential scrubbing.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| ui/src/lib/types.ts | Adds multitrack and vodProfile fields to the UI destination type. |
| ui/src/components/DestinationDialog.tsx | Adds a Twitch-only toggle to persist the multitrack boolean. |
| internal/routing/pair.go | Introduces paired compilation (CompilePair) and secondary label namespace prefixing. |
| internal/routing/pair_test.go | Adds structural + FFmpeg-measurement tests for paired graphs and compatibility guarantees. |
| internal/routing/filtergraph.go | Adds label namespacing support and threads it through compilation sites. |
| internal/multitrack/negotiate.go | Implements negotiation outcome model with quiet fallback + note scrubbing. |
| internal/multitrack/negotiate_test.go | Adds fixtures/tests covering minted-key publishing and key-scrubbing guarantees. |
| internal/engine/destinations.go | Uses CompilePair when a destination has a secondary mix and propagates SecondOutLabel to FFmpeg. |
| internal/db/multitrack_test.go | Adds DB round-trip tests for multitrack and vodProfile storage semantics. |
| internal/db/destinations.go | Adds multitrack + vod_profile columns, scan/marshal logic, and migration steps. |
| internal/api/redact_drift_test.go | Classifies new destination leaves (multitrack, vodProfile.*) as public/masked appropriately. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+68
to
+70
| b := make([]byte, r.ContentLength) | ||
| _, _ = r.Body.Read(b) | ||
| sent = b |
| // TestTheEmptyNamespaceIsByteIdenticalToTheSingleMixGraph. | ||
| type ns struct{ prefix string } | ||
|
|
||
| // of namespaces a fixed label. |
…nted key (#326) Two corrections to 007e915, one to a comment and one to a real gap the comment's wrong reasoning was hiding. THE COMMENT WAS WRONG ABOUT WHICH FIELD CARRIES THE KEY. 007e915 said status.html_en_us is "Twitch quoting the request back -- the request carrying the stream key", and called the scrub a fix for an observed leak. Measured against the live endpoint with a distinctive canary sent as `authentication`: html_en_us echoes client.name, not the key. A refusal for missing canvases named the broadcast software and did not contain the canary, and no refusal that could be produced quoted the key. The key that does come back is in ingest_endpoints[].authentication -- the 312-character minted key on the success path. The scrub stays, rejustified: html_en_us is attacker-influenced text from a third party that polyemesis renders to an operator, and it is built by quoting request fields back. Scrubbing text we do not control is cheap. That is defence, and it is now described as defence. A comment naming the wrong field is how the next person scrubs the wrong field. THE GAP THE WRONG REASONING HID. Chasing the right field surfaced one that matters more. destSecrets registered row.StreamKey -- the ORIGINAL -- and alerts.SecretSet.Scrub is a substring replace. The minted key ENDS WITH the original, so registering only the original masks its last segment and leaves v1_<64 hex signature>_<8 hex>_<hex manifest>_<MASK> standing in process.log, on the monitoring page's argv, and in every error the supervisor renders. A partially redacted live credential, which reads as protection to anyone glancing at the file. Same class as #310 and #324. destSecrets now takes the credentials that did not exist until go-live. Variadic rather than a row field: a minted key is a fact about one run of one process, and storing it would store a credential stale by the next broadcast. The test asserts the SIGNATURE PREFIX is gone, not that the original key is absent -- the latter passes on the broken version, which is exactly why the gap was easy to miss. A negative control pins that the gap is real, so the first test cannot pass because alerts.Redact's residual pass happened to catch it; if that control ever fails, the protection has moved and the comment needs rewriting rather than the test deleting. ALSO RECORDED: the live endpoint returned a SUCCESSFUL negotiation, with a full ladder and a minted key, for a plainly invalid stream key. Validation happens at publish, not at negotiation, so Outcome.Use is not evidence the credential works. Written on the field a caller reads. Three mutations, each with the named test confirmed running. The third asserts a PASS: dropping wireSpellings leaves the test green, confirming it does not lean on truncation expansion, which is a separate concern. Claude-Session: https://claude.ai/code/session_01HeLrWaDmsNeeNSbHQfEofX
|
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.
Wires Twitch Enhanced Broadcasting from the routing compiler to the negotiation, and stops at an honest boundary short of the go-live call site. Builds on #328 (the client) and #331 (two-track egress).
The blocker, solved
routing.Compileemitted one mix whose internal labels were fixed constants —a_t0,a_mix,aout— so concatenating two compiled graphs collided on every one of them. That is why nothing could setffmpeg.DestSpec.SecondAudioOutLabel, which has been able to map and encode a second audio track since #331 and has had no way to be asked for one.Every label site now goes through a namespace. The empty namespace is byte-for-byte what the package emitted before, asserted across profiles exercising every label site (per-track pans, amix, loudness, delay, resample, all six ducking labels) — so a destination that gains a VOD track does not have its live mix rewritten, and one that never asks for a second track produces the argv it always did.
An assumption killed rather than carried. I expected two graphs sharing
[0:a:0]to need an explicitasplit, the wayduckGraphsplits a trigger that must also reach the mix. They don't — an input stream is not a filter pad, and FFmpeg inserts the split itself. Measured in Docker on FFmpeg 6.0.1 (Alpine 3.18, the floorinternal/ffmpeg/detect.goenforces) and on 8.1.2, both building the two-tap graph and producing tone-distinct mixes. Anasplitwould have been dead weight carried on a guess.What is here
internal/routing— label namespacing +CompilePair. A secondary that will not compile is a warning, not an error: an optional VOD track must never veto a working broadcast. A primary that will not compile is still an error.db.Destination—Multitrack(opt in) andVODProfile(the second mix), both defaulting to off, plus the API/UI/types entries. Wired throughplanDestinationsinto the one place aDestSpecis built, so the backup feed carries the same two tracks rather than silently dropping one.multitrack.Negotiate— ask, read the verdict, resolve the endpoint, decide. Returns no error, because on most installs falling back is the path.Not on the provisional path: a provisional compile already runs on a guessed layout and says so, and a second guessed mix doubles what is approximate while the operator is being told the first is unreliable.
Secret handling: a correction, and a real gap closed
Correction. An earlier revision of this PR claimed
status.html_en_uscarries the stream key because it "quotes the request back". It does not. Measured against the live endpoint with a canary sent asauthentication:html_en_usechoesclient.name— a refusal for missing canvases named the broadcast software and did not contain the canary. The key that comes back is iningest_endpoints[].authentication, the 312-character minted key on the success path.The
html_en_usscrub stays, rejustified: it is attacker-influenced text from a third party that we render to an operator, built by quoting request fields back. Scrubbing text we do not control is cheap defence — it is just not a fix for an observed leak, and the comment now says so. A comment naming the wrong field is how the next person scrubs the wrong field.The gap that reasoning was hiding, which matters more.
destSecretsregisteredrow.StreamKey— the original — andalerts.SecretSet.Scrubis a substring replace. The minted key ends with the original, so registering only the original masks its last segment and leavesstanding in
process.log, on the monitoring page's argv, and in every error the supervisor renders — a partially redacted live credential that reads as protection. Same class as #310 and #324.destSecretsnow takes credentials that did not exist until go-live, variadic rather than a row field because a minted key is a fact about one run, not about the destination.TestTheMintedKeyIsMaskedWholeAndNotJustItsTailasserts the signature prefix is gone, not that the original key is absent — the latter passes on the broken version, which is exactly why the gap was easy to miss. A negative control pins that the gap is real, so the first test cannot pass becausealerts.Redact's residual pass happened to catch it.Also recorded: the live endpoint returns a successful negotiation for a plainly invalid stream key — validation happens at publish, not negotiation. So
Outcome.Useis not evidence the credential works, and that is written on the field a caller reads.Two tests worth reading
TestAPairedGraphReachesFFmpegAsTwoDistinctMixeshands the real graph to the real binary and reads tone content back off both tracks. The failure this ships with otherwise is two tracks that are the same mix, which a track count sees as success. Mutating the VOD profile to carry the live content fails it at −18.1 dB on a tone that must be absent. It needs no skip: PCM in NUT with no video uses only codecs built into every FFmpeg, and the binary lookup goes throughtestenv.FFmpegBinary.TestASuccessfulNegotiationPublishesWithTheMintedKeyis about a failure that works: publishing with the operator's own key does not fail loudly, it connects and sends a ladder the ingest never agreed to. The minted key ends with the operator's own, so "contains the operator key" would pass on the wrong value — the assertion is that the signedv1_prefix survives. Its fixture is synthetic and says so: the captured fixture ships withauthenticationemptied, because committing a real minted key would put a working stream key in the repository.Decisions taken, with reasons
internal/ffmpegalready selects its encoder from local hardware detection; what negotiation must report is the agreed codec, which is OBS's own.codecdeclaration on the same lines as.id. Mappingjim_nvenc→h264_nvencwould assert an equivalence we never exercise in order to then ignore it.Supportisyes/manual/no/unknown, and the honest answer is "depends whether your server has a supported GPU".yeswould be false for the majority GPU-less install. The matrix answers a per-account question; this is a per-destination toggle.-c:v copy— video is encoded once in a shared rendition upstream — so Twitch's returned ladder is somethingReconcilereports on, not something a destination can honour.Verified how
CompilePair, two distinct mixeshtml_en_usechoesclient.name; success for an invalid keyThree drift guards satisfied rather than worked around:
TestUITypesCanNameEveryDestinationField,TestEveryStoredLeafIsClassified(22 new stored leaves, allsPublic— a routing profile is mix settings, not a credential), andTestReadSafeViewsScrubEverySecretLeaf. Skip census unchanged at 94.Every test mutation-verified with the named test confirmed running —
-runwith a typo exits 0 printing[no tests to run], which reads as passing — and each mutation recorded in its test's doc comment with the observed failure. One candidate mutation was discarded rather than counted because it failed to build, which is not the named test failing.Gates green:
gofmt -l,go vet ./...,go build ./...,go test ./...,npx tsc --noEmit,npx vitest run(115 passing).The honest boundary — what is NOT here
Negotiateis not yet called at go-live.startDeststill publishes torow.Target()unconditionally. Two pieces remain:internal/configcarries a comment recording that exactly such a key was removed for "presenting a settable knob that did nothing".startDest, substitutingOutcome.TargetwhenUseis true and surfacingNoteonce.Everything those two need is in place and tested: the decision function, the second mix, the storage, and the egress. Scoped out rather than half-done, per "a solid foundation with an honest boundary beats a thin version of everything."
Closes part of #326.
https://claude.ai/code/session_01HeLrWaDmsNeeNSbHQfEofX