Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix RIDs bug on updating non-simulcast media #612

Merged
merged 1 commit into from
Jan 16, 2025

Conversation

OxleyS
Copy link
Contributor

@OxleyS OxleyS commented Jan 16, 2025

Fixes #611

425a851 added a From impl that converted from a slice of RIDs into a Rids instance, and unconditionally used it when updating media via SDP. However, this impl did not take into account empty slices, which are naturally produced when simulcast is not in play. This caused non-simulcast tracks to start reporting Rids::Specific(vec![]) once they were updated by e.g. a direction change.

As the correct Rids value on an empty slice depends on whether you're sending or receiving, this PR removes the From impl entirely and replaces it with manual conversion while updating media via SDP.

On updating media via SDP (e.g. direction switch), the claimed RX/TX RIDs
was being unintentionally changed from Rids::Any/Rids::None to
Rids::Specific with an empty RIDs array. This fix prevents that.
@algesten
Copy link
Owner

Let's land it.

There's a bigger question here about the semantic meaning of Rids Specific, Any and None. Before None the meaning was

  • Any - used by direct api since we don't have SDP to specify which specific RIDs to expect as incoming. That means the streams are allocated without needing to check they were declared up front
  • Specific - used in SDP api to know what has been negotiated

I think I introduced None to be able to separate the case where in an SDP negotiation one side says I want x, ,y, z and the other responds with empty vs there being no rids in question at all.

@algesten algesten merged commit 25125c1 into algesten:main Jan 16, 2025
25 checks passed
@OxleyS OxleyS deleted the non-simulcast-update-media branch January 16, 2025 12:11
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.

rids_rx() and rids_tx() return the wrong value when updating an existing non-simulcast track
2 participants