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 SDP for inactive rids #399

Merged
merged 3 commits into from
Nov 16, 2023
Merged

Conversation

logist322
Copy link
Contributor

This PR adds ability to receive offer with simulcast including some inactive rids (marked with "~" prefix) and to generate correct answer.

@algesten
Copy link
Owner

Hi! Welcome to str0m!

Looking good. I don't actually know the spec that underpins this change, but I will look into it.

@logist322 logist322 marked this pull request as ready for review November 14, 2023 12:06
@logist322
Copy link
Contributor Author

@algesten Hi! Seems like this PR is ready. Waiting for your review!

@xnorpx
Copy link
Collaborator

xnorpx commented Nov 14, 2023

~ is libwebrtc specific?

@lolgesten
Copy link

I'm curious too. Can @pthatcher shed some light?

The ~ prefix on the RID seems to indicate that the stream might be paused using "CCM TMMBR and TMMBN" RTCP messages. Following the links to https://datatracker.ietf.org/doc/html/rfc5104 – which largely seems to concern H271.

@logist322 could you maybe describe how you encountered these ~?

@logist322
Copy link
Contributor Author

@algesten Sure! I have faced it after adding a video transceiver with several RTCRtpEncodingParameters some of them have active: false. That's the way how Chrome and Safari works (@xnorpx in pure libWebRTC too).
You can reproduce it with this simple code in Chrome or Safari:

(async () => {
    let pc = new RTCPeerConnection();
    await pc.addTransceiver("video", {
        direction: "sendrecv",
        sendEncodings: [{
            rid: "h",
            active: true
        }, {
            rid: "m",
            active: false
        }, {
            rid: "l",
            active: false
        }]
    });
    let offer = await pc.createOffer();
    console.log(offer.sdp);
})();

You should see a=simulcast:send h;~m;~l line in the printed sdp.

@algesten
Copy link
Owner

@logist322 thanks!

It sounds like ~ means a RID starts paused, but that might change later without SDP negotiation (whether there are RTCP ways for us to discover when something becomes unpaused, I don't know). We already have a paused state on the receive streams here: https://github.com/algesten/str0m/blob/main/src/streams/receive.rs#L95-L99

Does it sound right that this ~ state should have an effect for a newly created StreamRx?

@pthatcher
Copy link
Collaborator

pthatcher commented Nov 15, 2023 via email

@logist322
Copy link
Contributor Author

logist322 commented Nov 16, 2023

It sounds like ~ means a RID starts paused, but that might change later without SDP negotiation (whether there are RTCP ways for us to discover when something becomes unpaused, I don't know). We already have a paused state on the receive streams here: https://github.com/algesten/str0m/blob/main/src/streams/receive.rs#L95-L99

@algesten You are absolutely right! We do use it for changing RTCRtpEncodingParameters dynamically without SDP negotiation.

Does it sound right that this ~ state should have an effect for a newly created StreamRx?

I don't think so. Especially, if newly created StreamRxs already have paused: true.

@algesten
Copy link
Owner

Alright. Let's merge it. At this point the ~ has no implication for str0m. This PR will fix so it doesn't fall over.

@algesten algesten merged commit 2256a5d into algesten:main Nov 16, 2023
22 checks passed
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.

5 participants