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

Matching rule update #557

Closed
wants to merge 1 commit into from
Closed

Matching rule update #557

wants to merge 1 commit into from

Conversation

aboba
Copy link
Contributor

@aboba aboba commented May 31, 2016

Fix for Issues #547 and #546

Rebase of PR #555

Fix for Issues #547 and #546

Rebase of PR #555
@ibc
Copy link
Contributor

ibc commented May 31, 2016

LGTM

@aboba
Copy link
Contributor Author

aboba commented May 31, 2016

@ibc The matching rules still do not seem quite right with respect to RTX/RED/FEC. For example, if rtx.ssrc is set in all the "real" codec encodings, there should not be a pt_table entry for the RTX codec. I think the same is true for fec.ssrc - if this is set for all encodings for a given fec.mechanism, then there should not be a pt_table entry for that FEC mechanism (including RED + ulpfec).

@ibc
Copy link
Contributor

ibc commented May 31, 2016

if rtx.ssrc is set in all the "real" codec encodings, there should not be a pt_table entry for the RTX codec

Why is that so bad? (regardless I'm not against that)

@ibc
Copy link
Contributor

ibc commented May 31, 2016

However, the current spec already matches what you suggest:

payload type table: If parameters.muxId is unset and parameters.encodings[i].ssrc is unset for all values of i from 0 to the number of encodings, then add entries to pt_table by setting pt_table[parameters.codecs[j].payloadType] to receiver, for values of j from 0 to the number of codecs.
If pt_table[pt] is already set to a value other than receiver then receiver.receive() will throw an InvalidParameters exception.

This is, if any encoding.ssrc is set, no entry will be added into the ptTable, so your concern above is already satisfied:

if rtx.ssrc is set in all the "real" codec encodings, there should not be a pt_table entry for the RTX codec

@aboba
Copy link
Contributor Author

aboba commented Jun 1, 2016

@ibc If parameters.encodings[i].ssrc is set for some value of i, then pt_table entries are not added. As you noted, in a situation where parameters.encodings[i].rtx.ssrc is not set, RTX packets will be dropped. Where parameters.encodings[i].rtx.ssrc is not set for any value of i, a pt_table entry for "rtx" should be added. Where parameters.encodings[i].fec.ssrc is not set for any value of i, a pt_table entry corresponding to parameters.encodings[i].fec.mechanism should be added.

@ibc
Copy link
Contributor

ibc commented Jun 1, 2016

Where parameters.encodings[i].rtx.ssrc is not set for any value of i, a pt_table entry for "rtx" should be added.

There may be multiple RTX codecs in codecs. That would require that the browser should take the corresponding encoding.codecPayloadType, iterate all the codecs, find a RTX one whose apt points to encoding.codecPayloadType and set such a codec.payloadType into ptTable. And ever more difficult for FEC (there could be multiple FEC codecs and we have no way which one is supposed to be used in which encoding...).

This is VERY hard.

@ibc
Copy link
Contributor

ibc commented Jun 1, 2016

To be absolutely clear. The following RtpParameters given to receive():

{
    codecs :
    [
        {
            name        : 'audio/opus',
            payloadType : 100
        },
        {
            name        : 'audio/ulaw',
            payloadType : 0
        }
    ],
    encodings :
    [
        {
            codecPayloadType : 100,
            ssrc             : 1111
        },
        {
            codecPayloadType : 0,
        }
    ]
}

produce the following routing tables (tested in my implementation):

{
    "muxIdTable": {},
    "ptTable": {},
    "ssrcTable": {
        "1111": "55333413"
    }
}

So, the ULAW stream (which will have a non announced SSRC value) will just be dropped.

@ibc
Copy link
Contributor

ibc commented Jun 1, 2016

To summarize:

  • The RTCRtpListener (which is attached to a single RTCDtlsTransport) does not fill its pt_table (if muxId or ssrcs are given) because filling it would cause conflicts if N RtpReceivers use the same transport (because most probably they will share PT values). This is intended and we need it.
  • In the other side, the spec allows not giving muxId nor ssrcs for very "simple" use cases (a single RtpReceiver per transport). But given my example above, it's clear that such a "feature" is really error prune.

So IMHO there are two possible solutions or workarounds:

  1. muxId MUST be given or, if not, every encoding, and every encoding.rtx, and every encoding.fec MUST have ssrc (so pt_table is just removed).
  2. Change the pt_table filling rule as follows:

If parameters.muxId is unset and parameters.encodings[i].ssrc is unset for some value of i from 0 to the number of encodings, or parameters.encodings[i].rtx is set but parameters.encodings[i].rtx.ssrc is unset for some value of i from 0 to the number of encodings, or parameters.encodings[i].fec is set but parameters.encodings[i].fec.ssrc is unset for some value of i from 0 to the number of encodings, then add entries to pt_table by setting pt_table[parameters.codecs[j].payloadType] to receiver, for values of j from 0 to the number of codecs. If pt_table[pt] is already set to a value other than receiver, or parameters.codecs[j].payloadType is unset for any value of j from 0 to the number of codecs, then receiver.receive() will throw an InvalidParameters exception.

This is: if any ssrc value is not given, then fill the pt_table.

@ibc
Copy link
Contributor

ibc commented Jun 1, 2016

I've implemented proposal 2 above, and the result of the routing tables are the following:

{
    "muxIdTable": {},
    "ptTable": {
        "0": "78347415",
        "100": "78347415"
    },
    "ssrcTable": {
        "1111": "78347415"
    }
}

So, it works.

ibc added a commit to versatica/mediasoup that referenced this pull request Jun 1, 2016
@ibc
Copy link
Contributor

ibc commented Jun 1, 2016

Here my working test units after implementing the solution 2 above:

https://github.com/ibc/mediasoup/blob/master/test/test_RtpReceiver.js?ts=2#L600

@ibc
Copy link
Contributor

ibc commented Jun 1, 2016

@robin-raymond
Copy link
Contributor

I think we should add a PR that describes differing views of what this could be but let's let the WG argue this out since this requires compatibility. I think this also requires a set of test cases to evaluate what rules must work and how.

@aboba
Copy link
Contributor Author

aboba commented Oct 4, 2016

I am going to revise this PR based on the matching rules in JSEP. Stay tuned.

@aboba aboba closed this Oct 4, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants