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

Algorithm in "RTP matching rules" ignores FEC/RTX/RED packets #368

Closed
ibc opened this issue Jan 26, 2016 · 25 comments
Closed

Algorithm in "RTP matching rules" ignores FEC/RTX/RED packets #368

ibc opened this issue Jan 26, 2016 · 25 comments

Comments

@ibc
Copy link
Contributor

ibc commented Jan 26, 2016

If the browser selects payload 101 with ssrc 1111111 for "opus" codec, and payload 152 with ssrc 22222222 for RTX, that would mean that the receiver must be ready to handle incoming RTP packets with payload 152 and ssrc 22222222 (RTX).

But section "8.3 RTP matching rules" fails to describe that, because the ptTable and ssrcTable tables of the RTCRtpListener are just filled with parameters.encodings[i].ssrc and parameters.codecs[j].payloadType. This means that the RTX packets would be ignored according to the algorithm defined in the same section.

@ibc ibc changed the title "RTP matching rules" does not clarify handling of FEC/RTX/RED packets Algorithm in "RTP matching rules" ignores FEC/RTX/RED packets Jan 26, 2016
@aboba aboba added the 1.1 label Feb 1, 2016
@aboba
Copy link
Contributor

aboba commented Feb 29, 2016

Right. Since RTX/FEC/RED is set up as a codec, the RED, RTX or FEC PT would be set in parameters.codecs[k].payloadType. However, the RTX SSRC (22222222) and PT would be set in parameters.encodings[i].rtx.ssrc and parameters.encodings[i].rtx.payloadType. In the case of FEC, (including "red+fec"), the SSRC would be set in parameters.encodings[i].fec.ssrc.

@ibc
Copy link
Contributor Author

ibc commented Feb 29, 2016

Thanks. So then, should SSRC 22222222 be included in the ssrcTable of the RtpListener?

@aboba
Copy link
Contributor

aboba commented Feb 29, 2016

Yes, SSRCs relating to RTX and FEC need to be added to the ssrcTable so that these streams will be routed to the correct receiver. Also, PT's related to RTX need to be added to the ptTable, so that they will be routed to the correct receiver. RED does not need special treatment, since the PT for RED will be set in the codec entry, and the SSRC for RED will be set via fec.ssrc.

@ibc
Copy link
Contributor Author

ibc commented Feb 29, 2016

Thanks.

@aboba
Copy link
Contributor

aboba commented Feb 29, 2016

@ibc @robin-raymond Can you review PR #394 ?

@ibc
Copy link
Contributor Author

ibc commented Feb 29, 2016

LGTM

@robin-raymond
Copy link
Contributor

I'm going to add a set of rules that I think make sense after having implemented it.

@robin-raymond
Copy link
Contributor

Here's my steps:

  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;

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 Mar 1, 2016

Great! Complex but easy to implement by following those exact steps.

Just wondering: why do we allow "payloadType" based identification? Since RtpParameters is a monolithic object I cannot figure out any case in which we know about remote payloadTypesbut we don't know about remote SSRC. Which kind of ORTC endpoint is supposed to signal its payloadType values but does not signal its SSRC values?

BTW it seems that implementors (MS EDGE) require SSRC mapping.

@robin-raymond
Copy link
Contributor

Yes, there's a lot of steps but I think it creates a fairly predictable behaviour.

@ibc: I cannot figure out any case in which we know about remote payloadTypes but we don't know about remote SSRC.

This can happen if you do a capabilities exchange. You can send to the remote party something like "If you send anything, send within this criteria". This means you can establish exactly which payload types an incoming stream is expected to use by telling the remote party what to use when sending without ever having signalled a single SSRC for them to use in advance.

BTW, our C++ implementation of that logic is here if my written explanation isn't clear enough: https://github.com/openpeer/ortc-lib/blob/master/ortc/cpp/ortc_RTPListener.cpp search for findMapping. It also uses some helper functions here https://github.com/openpeer/ortc-lib/blob/master/ortc/cpp/ortc_RTPTypes.cpp

... not that you should have to view that code to understand the specification but maybe it will help to explain what I was trying to say above...

I don't know about edge but I believe they implemented their first version before the rules were completely laid out so I suspect they will be improving this later? maybe? dunno... let's hope :)

Finally, in all steps above, if a match is found an entry is always added to an SSRC table to make faster routing for future packets based solely on the incoming SSRC (in case that wasn't clear enough).

@robin-raymond
Copy link
Contributor

@ibc want to take a stable at this in a PR? I can work with you on it if you need or review after.

@ibc
Copy link
Contributor Author

ibc commented Mar 11, 2016

I will work on this this weekend.

@ibc
Copy link
Contributor Author

ibc commented Mar 13, 2016

I'm not sure what more must be done regarding RTP matching rules. They look good to me as they are in master now. However I think we should work on examples/use-cases to be able to identify possible issues.

@aboba
Copy link
Contributor

aboba commented Mar 16, 2016

Matching rules as they currently exist have a problem with rtx.ssrc and fec.ssrc. In Section 8.3 it says:

"If ssrc_table[ssrc] is already set to a value other than receiver, then receiver.receive() will throw an InvalidParameters exception."

So once rtx.ssrc is set, then setting fec.ssrc to the same value will cause receiver.receive() to throw an exception (or vice versa, setting fec.ssrc first, then rtx.ssrc).

In reality there is no problem because RTX and FEC use different payload types.

@ibc
Copy link
Contributor Author

ibc commented Mar 16, 2016

I don't yet understand the problem. All the audio/video stream, RTX and FEC are part of the same RtpReceiver so it should not throw an exception. Obviously I miss something...

@aboba
Copy link
Contributor

aboba commented Mar 21, 2016

The reference to encodings[],rtx.payloadType has been removed from the matching rules. Re-reading it now, Section 8.3 doesn't throw an exception if fec.ssrc and rtx.ssrc are the same within the same RtpReceiver (only if the same SSRC is mapped to different RtpReceivers). So the worst problems seem to be fixed.

@aboba
Copy link
Contributor

aboba commented Mar 21, 2016

@robin-raymond Can you review the matching rules to see if we have fixed the problems and can close this Issue?

@aboba
Copy link
Contributor

aboba commented May 19, 2016

@robin-raymond I think your algorithm is better than the existing matching rules and we should update Section 8.3.

@robin-raymond
Copy link
Contributor

TODO: add notes to spec as to what ORTC Lib did, but not say this is the "best" or "only" way it can/should be done and allow this as implementation feedback as input to a final proposal.

@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 30, 2016

@ibc @robin-raymond Some discussion of the JSEP Section 7 routing rules:
https://lists.w3.org/Archives/Public/public-ortc/2016Sep/0029.html
https://lists.w3.org/Archives/Public/public-ortc/2016Sep/0030.html
https://lists.w3.org/Archives/Public/public-ortc/2016Sep/0031.html

JSEP Section 7 rules versus Robin's rules:

  1. Check for MuxID - if present, add to mapped SSRC entry table and forward packet to receiver

[BA] Similar, though in JSEP Section 7, mismatch of MuxID directly causes unhandledrtp (no further checks done).

  1. Check if a known previous SSRC mapping exists, and forward to mapped receiver
  2. Check for SSRC in encoding params (all encodings, RTX and FEC); if any match then create a mapped SSRC entry and forward to receiver;

[BA] Rules 2 and 3 seem equivalent to JSEP Section 7.

  1. 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.

[BA] JSEP Section 7 does not "latch" SSRCs based on a PT match. In practice, it will only check pt_table if there is no SSRC match, but pt_table entries are always created (though it is not defined what happens when multiple receivers use the same PT).

  1. 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;

[BA] JSEP Section 7 rules do not latch on PTs but do consult pt_table if there is no MID or SSRC match.

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 the proposed algorithm will address this issue as well since rtx.ssrc and fec.ssrc are now taken into account. The proposed algorithm seems the same as Robin's steps 1-3. Step 5 is also in the proposed algorithm. Only difference I can see is in Step 4 where JSEP Section 7 says to take the first RtpReceiver matching the PT, and Robin's algorithm says to take the RtpReceiver which has the PT but no SSRCs specified.

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 will create ssrc_table entries for encodings[].ssrc, encodings[].rtx.ssrc and encodings[].fec.ssrc. Also, we will have pt_table entries for RED, RTX and FEC methods such as "ulpfec" or "flexfec", as long as there is no PT 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 is now fixed in the latest draft. Please reopen if there is a problem remaining.

@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