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

RTP matching rules: don't remove pt_table[packet.pt] #547

Closed
ibc opened this issue May 16, 2016 · 17 comments
Closed

RTP matching rules: don't remove pt_table[packet.pt] #547

ibc opened this issue May 16, 2016 · 17 comments

Comments

@ibc
Copy link
Contributor

ibc commented May 16, 2016

Else if pt_table[packet.pt] is set: set rtp_receiver to pt_table[packet.pt], set ssrc_table[packet.ssrc] to rtp_receiver, set pt_table[packet.pt] to null and route the packet to rtp_receiver.
Question: Do we remove all pt_table[packet.pt] entries set to rtp_receiver?

Don't remove them. If the sender changes the SSRC (without signaling it to us) the RTP packet won't match any rule.

@ibc
Copy link
Contributor Author

ibc commented May 16, 2016

...because remember: "SSRCs can collide at any time because they are just 32 bits long !!!" so every RFC and RTP related spec must waste at least 10 lines to prevent this "probable" issue.

@ibc
Copy link
Contributor Author

ibc commented May 16, 2016

Even more: this problem also happen when encoding.ssrc is given. In that case, "RTP matching rules" do not fill the pt_table which means that, if the sender changes the SSRC, packets won't match anything.

So, two things:

  • Fill pt_table always, even if ssrcs are given.
  • Don't remove entries from pt_table.

@aboba
Copy link
Contributor

aboba commented May 19, 2016

@robin-raymond This Issue appears to exist in your proposed matching algorithm if an SSRC were previously specified or was locked and is then changed (e.g. due to a conflict). So Step 4 below may not be correct:

  1. Check for MuxID - if present, add to mapped SSRC entry table and forward packet to receiver
  2. Check if a known previous SSRC mapping exists, and forward to mapped receiver
  3. Check for SSRC in encoding params (all encodings, RTX and FEC); if any match then create a mapped SSRC entry and forward to receiver;
  4. If encodings[0..n] exist, Check payload types in encoding params where an SSRC does NOT exist; find "best" match that works (where earliest non-conflicting match works) and internally set non-set SSRC entry (i.e. lock in SSRC value) and then create an SSRC mapping and forward to mapped receiver; "Best" is where payload type is a direct match to an entry (where no SSRC value has previous been set or previously "locked-in" to a value); if none exist then where a codec matches from codec list but payload type nor SSRC is not set in the encoding; finally, the order receiver's .receive() was called originally is chosen before other receiver's .receive() called later, thus making two equally matched receivers choose one over the other.
  5. If no list of encodings[0..n] were specified, then find codec payload match from codec list and create a new internal encoding to match with a filled in payload type and fill in SSRC based on the incoming packet;
  6. If no match can be made above then fire an unhandled event for the muxid, ssrc, payload type combination.

@ibc
Copy link
Contributor Author

ibc commented May 25, 2016

Guys, there is a problem if we always store the PT values in the ptTable regardless ssrc was also signaled:

Many rtpReceivers may use the same transport to receive streams from many senders (example: a SFU forwarding N streams over the same transport). In such a scenario we want that all those senders are able to use the same PT:

  • This is required in BUNDLE and those PTs must refer to the same codec+configuration.
  • In pure ORTC the same PT may refer to different codecs.

So, the RtpListener of the transport cannot place a single PT entry in ptTable for a single rtpReceiver because there could be many rtpReceivers using the same PT over the same transport.

@ibc
Copy link
Contributor Author

ibc commented May 30, 2016

Friendly ping.

@aboba
Copy link
Contributor

aboba commented May 31, 2016

@ibc You are correct. It is not an error for multiple RtpReceivers to utilize the same PT value, since they can receive the correct stream via the SSRC (or muxId). Note that in the original routing rules, a PT_table entry is only created if all of the parameters.encodings[i].ssrc is unset. So if multiple receivers were to specify the same PT, this would not be a problem as long as each specified an SSRC. However, if more than one receiver left all of the parameters.encodings[i].ssrc unset, then an error would result since pt_table[pt] is already set to a value other than receiver. This is in fact a problem because it means that two receivers had registered to receive packets with the same PT value, regardless of SSRC.

Overall, I think this points out the complexity of the routing rules - and the difficulty of verifying that proposed changes do not result in regressions. It makes me wonder if we need to develop test cases that we run through whenever we propose changes.

@ibc
Copy link
Contributor Author

ibc commented May 31, 2016

After implementing it, my conclusions are:

  • ptTable MUST NOT be filled if ssrcs are given for all the encodings and all the encoding.rtx.ssrc and encoding.fec.ssrc.
  • If ssrcs are not given, ptTable must be filled and, on RTP packet receipt, the ssrcTable must be filled with the packet SSRC.
    • However, the corresponding entries in ptTable MUST NOT be removed (this is because SSRCs may change in the future).

So this is a kind of "first-come first-served" or, in other words, just the first of many rtpReceivers over the same transport can just signal PT values without signaling also SSRCs.

Some working test units here: https://github.com/ibc/mediasoup/blob/master/test/test_RtpReceiver.js#L23

@aboba
Copy link
Contributor

aboba commented May 31, 2016

@ibc I have consolidated all the proposed matching rule changes into PR #555 so we can review all of them in one place.

One issue that is not yet addressed is handling of RTX/RED/FEC. As you say, ptTable should not be filled for RTX if encoding.rtx.ssrc is set and for FEC if encoding.fec.ssrc is set. For RED, ptTable should not be filled if encoding.ssrc is set for the RED codec. Need to revisit this if/when the object model changes.

aboba added a commit that referenced this issue May 31, 2016
Fix for Issues #547 and #546

Rebase of PR #555
@aboba
Copy link
Contributor

aboba commented Jul 14, 2016

Robin believes that his algorithm doesn't exhibit this issue either. So we have assigned it to him :)

@aboba
Copy link
Contributor

aboba commented Sep 13, 2016

Routing Rules PR in JSEP: rtcweb-wg/jsep#320

@aboba
Copy link
Contributor

aboba commented Sep 14, 2016

@aboba
Copy link
Contributor

aboba commented Sep 30, 2016

@ibc @robin-raymond In JSEP Section 7 rules, PT_Table entries are not removed, and there is no latching of SSRCs due to PT matches.

JSEP Section 7 rules do fill the PT table even if SSRCs are specified, though the PT table is only consulted if there is no MID or SSRC match. There is no statement about errors if PTs are not unique, so maybe there needs to be some arbitration mechanism (e.g. first PT_Table entry wins, last entry wins, etc.).

So the requests seem to be fulfilled:

Fill pt_table always, even if ssrcs are given.
Don't remove entries from pt_table.

Do you think that the JSEP Section 7 rules resolve this issue?

aboba added a commit that referenced this issue Oct 4, 2016
Routing Rules update to address compatibility with Section 7 of https://tools.ietf.org/html/draft-ietf-rtcweb-jsep

Addresses the following issues: #368, #546, #547
@aboba
Copy link
Contributor

aboba commented Oct 4, 2016

@ibc @robin-raymond - Can you check whether this issue is resolved by PR #602 (which is based on JSEP Section 7)?

@aboba
Copy link
Contributor

aboba commented Oct 4, 2016

It looks to me like this Issue is addressed by the JSEP Section 7 rules. The proposed algorithm does not remove pt_table entries, although it does latch on PT reception. If the SSRC changes, incoming packets will still be routed. Also when encoding.ssrc is given, the proposed algorithm still fills the pt_table entry.

The proposed algorithm also supports "first-come first-served" allowing the first of many rtpReceivers signaling a PT values to be routed packets with that PT, regardless of whether a muxID or SSRCs were provided.

@aboba
Copy link
Contributor

aboba commented Oct 31, 2016

JSEP rules have changed:
https://tools.ietf.org/html/draft-ietf-rtcweb-jsep-17#page-66

aboba added a commit that referenced this issue Jan 9, 2017
Another attempt to fix Issues #368, #546, #547
@aboba
Copy link
Contributor

aboba commented Jan 18, 2017

@ibc @robin-raymond @pthatcherg The JSEP Appendix B RTP matching rules appear to address this issue, correct?
https://tools.ietf.org/html/draft-ietf-rtcweb-jsep-18#appendix-B

The proposed rules do not remove pt_table entries except if PT overlap is created by a call to receive().
SSRC latching is included which should handle the SSRC change case.
pt_table entries are filled, regardless of whether encodings[].ssrc, encodings[].rtx.ssrc or encodings[].fec.ssrc are given, as long as PT does not overlap.

aboba added a commit that referenced this issue Apr 18, 2017
Yet another attempt to fix Issues #368, #546, #547
@aboba
Copy link
Contributor

aboba commented Sep 22, 2017

I believe that this issue has ben resolved in the current draft. Please reopen if it is not fixed.

@aboba aboba closed this as completed Sep 22, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants