feat: relayonly and pcrepair options mapped to SDK peerConfig/peerRepair (OPTI-4246) - #198
feat: relayonly and pcrepair options mapped to SDK peerConfig/peerRepair (OPTI-4246)#198devin-ai-integration[bot] wants to merge 8 commits into
Conversation
…peerRepair (OPTI-4246) Co-Authored-By: craig.johnston <cjohn@dolby.com>
Co-Authored-By: craig.johnston <cjohn@dolby.com>
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
Co-Authored-By: craig.johnston <cjohn@dolby.com>
craig-johnston
left a comment
There was a problem hiding this comment.
Tested this against the real SDK build from millicast-sdk#544 (post-review-fixes) through a live rp2 stream, using the actual web-viewer + vue-viewer-plugin stack (not a bare harness). Forcing a pcrepair repair against a real connection reproducibly freezes/blacks out the video in Chrome, Firefox, and Safari — this isn't browser-specific, it's a gap in this PR's wiring.
Root cause: the SDK's View#peerRepair event (started/completed/failed) is never listened for here. state.Controls.viewerMigratingEvent — the flag setStream() in this same file checks to decide whether to run the crossfade swap onto the second <video> element — is only ever set by addSignalingMigrateListener(), which listens for the signaling migrate broadcast event (a server-initiated migration). A client-initiated peerRepair repair never fires that signaling event, so when the SDK emits the replacement peer's track event, setStream() takes the !viewerMigratingEvent branch and silently ignores it because the stream id differs from what's currently on screen. ~1s later the SDK's finishRepair() closes the old peer connection per the make-before-break design, and since the video element is still bound to that now-closed stream, it goes black/freezes — exactly what I saw in all three browsers.
The PR description's claim that "the plugin already handles the second track event from migration, so no UI change is needed" is true only for the pre-existing server-initiated migrate broadcast event; it doesn't hold for this PR's client-initiated peerRepair path, which has no equivalent listener anywhere in this diff.
Suggest: listen for millicastView.on('peerRepair', ...) and set viewerMigratingEvent true on state: 'started' (mirroring what addSignalingMigrateListener()'s signaling migrate listener does today), so the existing crossfade path in setStream() picks up the repair's track event instead of dropping it.
| connectOptions.peerConfig = { iceTransportPolicy: 'relay' }; | ||
| } | ||
| if (state.Params.viewer.pcrepair) { | ||
| connectOptions.peerRepair = { enabled: true }; |
There was a problem hiding this comment.
Nothing in this PR listens for the SDK's new peerRepair event (millicastView.on('peerRepair', ...)) emitted by View.repairConnection()/finishRepair() in millicast-sdk#544. Right after enabling connectOptions.peerRepair = { enabled: true } here, we need a listener that flips state.Controls.viewerMigratingEvent on state: 'started' — the same flag addSignalingMigrateListener() sets for the pre-existing server-initiated migrate broadcast event further down in this file — so the resulting track-event swap goes through the crossfade path in setStream() instead of being silently dropped there (the !viewerMigratingEvent branch that ignores a new stream id). Verified live (real rp2 stream, real SDK build with #544's fixes): forcing a repair freezes/blacks the video in Chrome, Firefox, and Safari because of this gap.
There was a problem hiding this comment.
Fixed in 819e690. setTrackEvent() now registers millicastView.on('peerRepair', ...) when pcrepair is set: state: 'started' sets viewerMigratingEvent true (same as the signaling migrate listener), state: 'failed' clears it so a later real track event is not treated as a swap. The peerRepair event is emitted by the View instance, so it survives the signaling replacement and only needs to be registered once. setStream() then runs the existing crossfade path and resets the flag as before.
…ack is displayed (OPTI-4246) Co-Authored-By: craig.johnston <cjohn@dolby.com>
|
Thanks for the live test, this was a real gap. Fixed in 819e690 (see the inline thread): a |
…I-4246) The previous commit reset viewerMigratingEvent to false when peerRepair emits 'failed'. That races View.js's finishRepair() rollback path: when the replacement peer had already swapped in (a real track event delivered and the plugin crossfaded onto it) before ultimately failing, rollback() re-emits the original peer's track event right after 'failed' to restore it (its own comment: "the application already received the tracks of the replacement; give it the current ones again"). Since JS emits listeners for 'peerRepair' synchronously before the following 'track' emit runs, clearing the flag on 'failed' would make setStream() drop that restoration via the same !viewerMigratingEvent guard this feature already had to work around, leaving the video stuck on the now-closing replacement stream. Not clearing it here is safe: setStream() already clears the flag itself once any swap actually completes, which covers both the success path and the restore-after-failed-repair path. The only edge case left unclear is a failure before the replacement's track event ever arrived at all (no restoration to synchronize with) - the flag stays true until the next unrelated track event, which then simply takes the crossfade branch instead of the plain-replace branch. That is not a black-screen risk. Verified with a synthetic started -> track (swap) -> failed -> track (restore) sequence against the real plugin build: video keeps playing throughout with this fix.
|
Fixed in 819e690 (listen for Re-verified end-to-end against a real rp2 stream through the actual built plugin (not a bare SDK harness):
Local repro notes for whoever picks this up: this only reproduces through the real |
…4246)
The inactive player/player2 element used
`:class="{ 'display: none;': ... }"`, which toggles a class literally
named "display: none;" - not a real class, so it never applied and both
elements stayed visible+laid out during a migration/repair crossfade.
Since #main-source is display:flex, having both present split the
container width between them, which is what made a repair look like the
video shifting left and then snapping back once the inactive element was
removed from the DOM.
Switched to a :style binding that actually sets display:none on whichever
element isn't current. This is a pre-existing bug in the crossfade path
shared by the server-initiated migrate flow and the new peerRepair flow;
it just had never been visibly exercised via pcrepair until the previous
two fixes made that path actually run.
Verified live: sampling video element width/left every 300ms through a
forced repair now shows exactly one element visible (full width) at any
time, with a clean cut to the replacement instead of a momentary
side-by-side split.
|
Follow-up fix in 74ac743: found while cross-browser testing the previous two fixes - the repair's crossfade worked (no more freeze) but the video visibly shifted left then snapped back during the swap. Root cause: Switched to a |
…e (OPTI-4246) @millicast/sdk@0.8.2-pcrepair.0 is now published under the pcrepair dist-tag (does not affect latest, still 0.8.1). Pin to the exact version rather than a semver range so this plugin prerelease can only ever resolve the SDK build that actually has peerRepair/relayonly support, per the rollout plan in the PR description.
Summary
Jira: OPTI-4246. Companion to millicast/millicast-sdk#544.
Adds two opt-in viewer options, both
falseby default, so the public viewer can test relay-only connections and the SDK's experimental peer repair with real traffic without changing behavior for anyone else.relayonly(true/'true'):connectOptions.peerConfig = { iceTransportPolicy: 'relay' }. The browser gathers only TURN candidates, so a direct host path can never be selected.pcrepair(true/'true'):connectOptions.peerRepair = { enabled: true }. The SDK replaces a connected-but-unusable peer connection (high RTT / loss / no media) when a better ICE pair exists, using the existing migrate path.A client-initiated repair does not fire the signaling
migrateevent, sosetStream()would otherwise ignore the replacement peer'strackevent and the video would freeze when the old peer is closed. Whenpcrepairis set,setTrackEvent()registersmillicastView.on('peerRepair', ...):so the replacement track goes through the existing crossfade path in
setStream(), which resets the flag afterwards. The event is emitted by theViewinstance, so one listener registered at init is enough.Both options are declared in
defaultViewerOptionsandprocessViewerOptionsso they are no longer forwarded ascustomKeys. Any other unknown query key still goes throughcustomKeysas before.Release
Version is set to
1.8.3-pcrepair.0. Publish with thepnpm-build-publishworkflow anddist_tag=pcrepair. The@millicast/sdkdependency will be pinned to0.8.2-pcrepair.0in a follow-up commit once that prerelease is on npm (the lockfile cannot be updated before it exists).Checks
pnpm run lintandpnpm run buildpass. Base ismainbecausedevelopis ~50 commits behindmainin this repo.Link to Devin session: https://dolby.devinenterprise.com/sessions/5a13aa779c4c4f8b8f5fc938cee80dcc
Open in Devin Desktop: https://dolby.devinenterprise.com/desktop/session/5a13aa779c4c4f8b8f5fc938cee80dcc?variant=devin
Requested by: @craig-johnston