Skip to content

Keep dynacast-paused layers paused across publisher renegotiations - #1148

Merged
pblazej merged 3 commits into
mainfrom
lukas/refresh-dynacast-layers-after-answer
Oct 8, 2026
Merged

pblazej merged 3 commits into
mainfrom
lukas/refresh-dynacast-layers-after-answer

Conversation

@lukasIO

@lukasIO lukasIO commented Oct 7, 2026

Copy link
Copy Markdown
Contributor

Applying a publisher answer re-enables every RTP encoding, and the server
only ever reports subscribed qualities that changed, so layers dynacast
had paused were never paused again after a renegotiation. Accumulate the
subscribed qualities on the track and re-apply them once the answer is
set. Senders are fed the accumulated list on every path, so a re-apply
cannot disagree with the live update that preceded it.

Applying a publisher answer re-enables every RTP encoding, and the server
only ever reports subscribed qualities that changed, so layers dynacast
had paused were never paused again after a renegotiation. Accumulate the
subscribed qualities on the track and re-apply them once the answer is
set. Senders are fed the accumulated list on every path, so a re-apply
cannot disagree with the live update that preceded it.
@github-actions

github-actions Bot commented Oct 7, 2026

Copy link
Copy Markdown

⚠️ This PR does not contain any files in the .changes directory.

@devin-ai-integration devin-ai-integration Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Devin Review found 1 potential issue.

1 flag not posted on this PR by your GitHub settings — view it in Devin Review. (Configure)

Devin Review

Comment on lines +284 to +285
if !qualities.isEmpty, let sender = track._state.rtpSender {
sender.raw._set(subscribedQualities: qualities)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 Stale generic qualities override primary codec

When updates switch from generic to codec-specific, refreshSubscribedQualities reapplies stale generic qualities after current primary-codec qualities. Both caches target the same sender, so renegotiation can reopen a paused layer or suppress an enabled one.

Learn more

The signal callback applies either a generic quality list or a codec-specific list, depending on which field the server includes (didUpdateSubscribedCodecs). Each path now retains its own cache. A primary codec update does not clear or reconcile the generic cache. On a publisher answer, the refresh applies each codec cache and then writes the generic cache to the primary sender, even if that cache is older. This leaves the sender with the older subscription state until another signal update changes it.

Example: The generic list caches high: true. Later the primary VP8 codec receives high: false. After a new answer, the codec refresh disables high, then the generic refresh re-enables it; the publisher transmits a layer the server paused.

Recommended fix: Track which cache most recently controls the primary sender, or reconcile generic updates into its codec-specific cache. In refreshSubscribedQualities, apply only the authoritative state for each sender, including when signaling changes between generic and codec-specific updates.

Devin Review


Was this helpful? React with 👍 or 👎 to provide feedback.

@pblazej pblazej Oct 8, 2026 •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The overwrite would need a track to receive a generic-only update followed by a codec-specific one. Checked the server: SubscribedQualityUpdate has had a single builder since v1.1.0 (May 2022) that always populates both fields (SubscribedQualities = SubscribedCodecs[0].Qualities, kept "for compatible with old client"), and the format never changes for a track within a session. The SDK takes the codec branch whenever subscribed_codecs is non-empty, so the generic cache is never populated against any supported server and this isn't reachable. Theoretical hazard only; removing the deprecated generic path would eliminate it entirely.

Comment thread .github/workflows/ci.yaml

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unrelated to this change, just triggered here.

@pblazej
pblazej merged commit fbcd8b0 into main Oct 8, 2026
56 of 57 checks passed
@pblazej
pblazej deleted the lukas/refresh-dynacast-layers-after-answer branch October 8, 2026 09:36
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.

2 participants