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

RTX APT capability? #444

Closed
robin-raymond opened this issue Mar 29, 2016 · 46 comments
Closed

RTX APT capability? #444

robin-raymond opened this issue Mar 29, 2016 · 46 comments

Comments

@robin-raymond
Copy link
Contributor

Since we don't have a capability for RTX APT, a single RTX codec gets listed allocating a single preferredPayloadType. The trouble is that payload type is not relevant because an RTX codec is needed per media codec that supports RTX. Likewise, there's no guarantee the engine supports RTX for each media codec.

Currently a developer would need to disregard the RTX codec, and allocate their own payload per RTX codec per media codec they wish to use with RTX. This is certainly doable but problematic. There is currently no way for the developer to know which media codecs support RTX or not.

I suspect this issue may exist for RED + ULPFEC, FlexFec too possibly. Although there's an advantage that each of those only need to allocate a single codec per clock rate (as the payload contains the original media codec) whereas that's not true of RTX payloads.

@ibc
Copy link
Contributor

ibc commented Mar 30, 2016

getCapabilities() may retrieve as many RTX named codecs as media types supporting RTX. Each of these RTX codec should include a options.apt or similar field pointing to the preferredPayloadType of the corresponding media codec.

But again: These things happen because we are considering RTX a codec rather than a "real-codec feature".

@robin-raymond
Copy link
Contributor Author

@ibc we could reverse this and make each codec announce it's corresponding RTX payload type instead of having apt inside the RTX codec and announcing RTX as a codec

So in CodecCapability / CodecParameters would have an rtxPayloadType value (optionally) and remove the RTX codec entirely.

@ibc
Copy link
Contributor

ibc commented Mar 30, 2016

That's exactly what I meant with "real-codec feature" :)

And we can do the same with FEC.

@murillo128
Copy link

Not sure if I understand the problem. RTX is codec-unaware, so either it is supported on RTPSender/RTPReceive or not. So in terms on capabilities, if RTX is supported, it is supported for all codecs.

Another thing is if you want to use it for some specific codecs or not (associating an ssrc).

@ibc
Copy link
Contributor

ibc commented Mar 30, 2016

So in terms on capabilities, if RTX is supported, it is supported for all codecs.

Yep. The point is that, in case the browser supports RTX, every real codec retrieved via getCapabilities() would include a rtxPayloadType numeric property.

@murillo128
Copy link

Yep. The point is that, in case the browser supports RTX, every real codec retrieved via getCapabilities() would include a rtxPayloadType numeric property.

Indeed

@robin-raymond
Copy link
Contributor Author

In theory RTX is universal for all codecs, but in practice, it's not always offered for all codecs. So there needs to be a way to make it optional property if we do it this way.

@ibc
Copy link
Contributor

ibc commented Mar 30, 2016

there needs to be a way to make it optional property

Just don't set the corresponding rtpCodecParameters.rtxPayloadType property when calling rtpSender.send().

@robin-raymond
Copy link
Contributor Author

@ibc that's all true, but I'm being specific about that because if we are going to issue any "fixes" we need to be 100% clear about everything to the dotted i and crossed t.

@robin-raymond
Copy link
Contributor Author

@pthatcher do you have an opinion on this issue?

I think this is applicable to FEC too.

@aboba
Copy link
Contributor

aboba commented Mar 30, 2016

@pthatcher @robin-raymond

Related WebRTC 1.0 Issue: w3c/webrtc-pc#548

@robin-raymond
Copy link
Contributor Author

Just to be clear, I'm in favour of this change to make RTX and FEC as options under the "main" real codecs but I think we need to get buy in for such a change. I find implicit cross reference meanings between codecs and their RTX/FEC counterparts in the codec list is very problematic and cumbersome (.e.g. making sure clock rate matches, settings match and ambiguity where multiple matches are possible, RED listed with or without payload types, multiple REDs with ambiguous choices, etc). I further think this is applicable to both RTX and FEC.

Plus, it's not just an RTX preferred payload type. RTX also has rtxTime. So if we put this under the CodecCapability / or CodecParameter we'd have to include these values as appropriate.

For example:

partial dictionary RTCRtpCodecCapability {
   RTCRtxCodecCapability           rtx;
   sequence<RTCFecCodecCapability> fec;
};

dictionary RTCRtpCodecCapability {
   payloadtype      preferredPayloadType;
   unsigned long    rtxTime;    // recommended rtx time value
}

dictionary RTCFecCodecCapability {
   DOMString        mechanism;
   payloadtype      preferredPayloadType; // if applicable, e.g. RED+ULPFEC
   Dictionary       parameters;    // optional fec specific capabilities (which do exist for some codecs)
}
partial dictionary RTCRtpCodecParameters {
   RTCRtxCodecParameters           rtx;
   sequence<RTCFecCodecParameters> fec;  // maybe need a sequence to support multiple mechanisms simultaneously
};

dictionary RTCRtxCodecParameters {
   payloadtype      payloadType;
   unsigned long    rtxTime;
}

dictionary RTCFecCodecParameters {
   DOMString        mechanism;
   payloadtype      payloadType;   // if applicable, e.g. RED + ULPFEC
   Dictionary       parameters;    // optional fec specific parameters
}

@ibc
Copy link
Contributor

ibc commented Mar 31, 2016

@robin-raymond IMHO that's exactly the way to go. Then we can also refactor current encodings which IMHO should be simplified A LOT. Something like:

dictionary RTCRtpParameters {
    DOMString                                 muxId = "";
    sequence<RTCRtpCodecParameters>           codecs;
    sequence<RTCRtpHeaderExtensionParameters> headerExtensions;
    RTCRtpEncodingParameters                  encodings;  // Not a sequence anymore
    RTCRtcpParameters                         rtcp;
    RTCDegradationPreference                  degradationPreference = "balanced";
};

partial dictionary RTCRtpEncodingParameters {
    unsigned long       mediaSsrc;
    unsigned long       rtxSsrc;
    unsigned long       fecSsrc;
    [...]
};

@murillo128
Copy link

You need too support simulcast.. :)

@murillo128
Copy link

what I don't understand is why don't we set the codec inside the encoding:

dictionary RTCRtpParameters {
    DOMString                                 muxId = "";
    sequence<RTCRtpHeaderExtensionParameters> headerExtensions;
    sequence<RRTCRtpEncodingParameters>                 encodings; 
    RTCRtcpParameters                         rtcp;
    RTCDegradationPreference                  degradationPreference = "balanced";
};

partial dictionary RTCRtpEncodingParameters {
    unsigned long       mediaSsrc;
    unsigned long       rtxSsrc;
    unsigned long       fecSsrc;
    sequence<RTCRtpCodecParameters>           codecs;
    [...]
};

Then we don't need to reference the payolad type anymore as it would be inside the codec parameters, right?

@ibc
Copy link
Contributor

ibc commented Mar 31, 2016

@murillo128 true. And that would lead to a better layered structure ("encodings" is the transport of "medias"). No more cross references.

@ibc
Copy link
Contributor

ibc commented Mar 31, 2016

5 PayloadType (media)
4 SSRC (encoding)
3 SRTP
2 DTLS
1 ICE

@robin-raymond
Copy link
Contributor Author

@murillo128 @ibc if you put the codecs inside the encoding you lose a feature. Right now you can define the codec list and have a "latch all" where a stream can match by payload type alone. This also allows change of codec to occur in simulcast without problem if you switch from one codec to another.

@murillo128
Copy link

None of them seems a feature to me, but a bad side effect.

"Catch all" means that you need to find the codec, find the payload type
and set the payload type in the encoding. If you don't check that there is
a codec for that payload before setting it into encoding then you can have
inconsistencies. So, not only we simplify the sou, but remove a point of
failure.

Also, if I am not mistaken, changing codec in simulcast requires changing
the payload in each encoding anyway, so the flow is the same.
El 31/3/2016 19:16, "Robin Raymond" notifications@github.com escribió:

@murillo128 https://github.com/murillo128 @ibc https://github.com/ibc
if you put the codecs inside the encoding you lose a feature. Right now you
can define the codec list and have a "latch all" where a stream can match
by payload type alone. This also allows change of codec to occur in
simulcast without problem if you switch from one codec to another.


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#444 (comment)

@robin-raymond
Copy link
Contributor Author

@murillo128 no, the encoding list for a catch all is empty. This allows a receiver to just name the codecs and start receiving without specifying anything about the encoding and any stream coming in will automatically latch by payload type. Plus a changed codec in a simulcast stream will be handled by the same mechanism. It's basically a way to auto-fill new encodings and it's not a side effect or point of failure as it was a design consideration and one of the reasons why this choice was made originally.

@ibc
Copy link
Contributor

ibc commented Mar 31, 2016

@robin-raymond I'm afraid you are usually making the syntax of RTCRtpParameters to be dependent on the receiver "RTP matching rules". That is not (and MUST NOT be) the only factor, not at all.

May we please talk about the syntax of RTCRtpParameters without mentioning the "RTP matching rules"? When it comes to map ORTC to SDP such an argument is 100% irrelevant. When it comes to develop a SFU it is also irrelevant.

We need an API that let the developer check the browser's capabilities and another API that provides the developer with a full RTCRtpParameters object (which includes SSRC values) before the developer calls send() on the corresponding RtpSenders. Being able to properly signal the complete RTP parameters (including SSRCs) to the remote peer before sending RTP is a MUST HAVE feature.

@murillo128
Copy link

I was referring to their usage in rtpsender (and I think @ibc also)

That is what happens when having the same objects for encoding and decoding
:)

@ibc
Copy link
Contributor

ibc commented Mar 31, 2016

I was referring to their usage in rtpsender (and I think @ibc also)

Yes. I always mean the stuff that the developer must pass to the RtpSender.

@ibc
Copy link
Contributor

ibc commented Mar 31, 2016

For example: if ORTC supported H264 SVC (which AFAIK means multi-stream with different SSRCs and probably different PTs), how is the user supposed to provide RtpSender.send() with the corresponding parameters? And how is the user supposed to "retrieve" those complex RtpParameters (before calling send()) in case he needs to signal them to the remote peer during the negotiation?

Just letting the browser "fill SSRC values here and there" at the time RTP is sent is not the solution.

@murillo128
Copy link

Aren't we trying to address too many different topics in a single issue? ;)

@ibc
Copy link
Contributor

ibc commented Mar 31, 2016

Yep... one thing leads to the other..
Sorry, let's focus.

@robin-raymond
Copy link
Contributor Author

This thread isn't just about the sender... The receiver is also involved. Routing via PayloadType alone was a desired feature in the design.

@ibc
Copy link
Contributor

ibc commented Mar 31, 2016

Routing via PayloadType alone was a desired feature in the design.

I'm not worried about how the receiver ORTC browser will route incoming RTP (that is its business). I'm worrier regarding how the sender browser, the sender user, or a SFU will generate those RTP parameters.

BTW: Routing just via PT is not valid if the browser receives multiple streams from multiple peers over the same transport (SFU scenario).

@robin-raymond
Copy link
Contributor Author

@ibc the encoding params in combination with the codec work just fine in that scenario. I'm not saying the cross referencing is "the best way possible", but it does allow to describe things accurately and it does work. The original bug filed was regarding the cross referencing between "media codecs" and "feature" codecs and "feature" codecs and encodings; not "encodings" cross reference standard "media codecs" which I view as a different concern.

As for routings via PT and SFU, if you expect packets to come in on the same port and you are multiplexing multiple ICE usernameFrag/passwords then you have to have special demux rules to handle multiple parties. ORTC does not expect multiple streams by different parties on the same physical socket address. If you are already demuxing multiple parties prior to coming into any receiver object (which you'd have to do due to DTLS anyway), then PT is a valid routing rule even for an SFU.

@murillo128
Copy link

Shouldn't we move the encoding parameter change to a different issue? The
original apt issue was kind of agreed

@robin-raymond
Copy link
Contributor Author

ORTC must support that given that it is supposed to be WebRTC 1.0 compatible. In WebRTC we have already that (we had it in Chrome using PlanB, which means many SSRCs into the same m=line and transport, and now with Plan Unified in Firefox Nightly by having multiple bundled m=lines from many participants).

It already supports this... I'm referring to your statement:

BTW: Routing just via PT is not valid if the browser receives multiple streams from multiple peers over the same transport (SFU scenario).

That statement implies something that is not correct given the context. Defining just the list of codecs with payload types is perfectly legal and doable for a SFU as well as a client. In SDP the codecs are defined by the m line so all streams within the same m line have the same meaning for their payload type. Thus routing by payload type is perfectly legally and the meaning of each stream can be understood by the payload type. Obviously the SSRC is used to demux multiple streams. My point is that simply filling in a list of codecs and having the streams separated is entirely doable in both client a SFU because of the shared meaning for the payload type.

If you do something funky where there is multiple meanings for the same payload type on the same transport then things go funny and my point is that should not be possible anyway the way things are defined in the API.

So if you have a receiver with rtp parameters with just a list of codecs, the receiver is perfectly capable of creating dynamic encodings on the fly based upon the codec payload type. Obviously the SSRC plays a role in demuxing here between streams.

@robin-raymond
Copy link
Contributor Author

And to be further clear, it's possible for multiple receivers to be attached to the same transport with different PT meanings. But you have to have routing definitions in those cases to remove ambiguity or you could cause mismatches. If you had VP8 PT as 101 and H264 as 101 on the same transport, with nothing other than the payload type defined for the routing then things go a bit funky. If you have an SSRC or a muxid or something else that clears up the usage then the routing is fine.

The context here is a single receiver having a list of codecs and the meaning of the codecs is thus shared across all encodings, or possible no encodings listed and thus auto-interpretation based on payload type alone. Again, I think the focus of this bug needs to be able cross referencing between "feature" codecs and media codecs and not the cross referencing between codec list and encoding parameters. If this doesn't clear things up then please file a more appropriate bug about that topic.

@ibc
Copy link
Contributor

ibc commented Mar 31, 2016

Thanks for the clarification, I was a bit afraid :)

So if you have a receiver with rtp parameters with just a list of codecs, the receiver

(Let's assume the "receiver" is the SFU)

is perfectly capable of creating dynamic encodings on the fly based upon the codec payload type. Obviously the SSRC plays a role in demuxing here between streams.

Imagine Alice is a ORTC browser that does not signal SSRC, and Bob a WebRTC 1.0 browser that does not support muxId. Once the SFU receives the "offer" (sorry) from Alice it must generate a "SDP reoffer" for Bob in which it MUST signal the SSRCs belonging to Alice streams. So the SFU must know in advance those SSRCs (even if it would not need them in case it is a final endpoint).

A "workaround" would be: The SFU receives the list of codecs of Alice and, based on them, it assumes the exact number of SSRCs Alice will send. So the SFU can create a SDP offer for Bob with those SSRCs and should rewrite the SSRC field of the RTP packets received from Alice. That would work... it is just that I don't think that it is possible to know the exact number of SSRCs we are going to receive by just having the list of remote codecs.

@ibc
Copy link
Contributor

ibc commented Mar 31, 2016

If you have an SSRC or a muxid or something else that clears up the usage then the routing is fine.

Sure. The problem is that, clearly, ORTC invites to not signal the sending SSRC.

The context here is a single receiver having a list of codecs and the meaning of the codecs is thus shared across all encodings, or possible no encodings listed and thus auto-interpretation based on payload type alone. Again, I think the focus of this bug needs to be able cross referencing between "feature" codecs and media codecs and not the cross referencing between codec list and encoding parameters. If this doesn't clear things up then please file a more appropriate bug about that topic.

Agreed.

aboba added a commit that referenced this issue Apr 1, 2016
aboba added a commit that referenced this issue Apr 1, 2016
change log updates for Issues #444 and #450
@murillo128
Copy link

In order to close the original issue, i would like to propose to add a new dictionary for the rtx parameters that would be added to each RTCRtpCodecParameters that supports rtx:

//New dictionary
dictionary RTCRtpCodecRTXParameters {
             payloadtype               payloadType;
             unsigned long             rtxtime;
};

dictionary RTCRtpCodecParameters {
             DOMString                 name;
             payloadtype               payloadType;  
             unsigned long             clockRate;
             unsigned long             maxptime;
             unsigned long             ptime;
             unsigned long             numChannels;
             sequence<RTCRtcpFeedback> rtcpFeedback;
             Dictionary                parameters;
             RTCRtpCodecRTXParameters  rtx;                       // NEW: rtx.payloadType
};

@pthatcherg
Copy link

Sorry for being late to the conversation. I have a few thoughts, and I'm not sure how redundant or different they are, since it's such a long conversation and it's hard to tell what the current state is.

  1. Capabilities shouldn't say anything about RTX. Just assume RTX can be sent for any codec. I think we are all in agreement about that.
  2. Parameters mostly just needs to specify an RTX PT one way or another. I think it would be nice if we had codec.rtxPayloadType instead of rtxCodec.parameters["apt"]. However, that will probably put it out of sync with WebRTC.
  3. You also have to pick a PT for FEC, but it's not dumb like RTX. You just need one, not one per other PT.

@ibc
Copy link
Contributor

ibc commented Apr 7, 2016

1 - Capabilities shouldn't say anything about RTX. Just assume RTX can be sent for any codec. I think we are all in agreement about that.

Right. The problem is that, somehow, we need a proper way to tell the browser whether we want to send RTX or not and, if so, we need a proper way to retrieve the browser chosen parameters regarding RTX (basically the RTX PT associated to each sending media codec), and also a proper API to override them.

2 - Parameters mostly just needs to specify an RTX PT one way or another. I think it would be nice if we had codec.rtxPayloadType instead of rtxCodec.parameters["apt"]. However, that will probably put it out of sync with WebRTC.

I disagree. We should not design a SDP based API. If so, I don't understand the purpose of ORTC.

3 - You also have to pick a PT for FEC, but it's not dumb like RTX. You just need one, not one per other PT.

Same as above: the user should be able to retrieve the browser chosen PT for FEC, and should be able to override it if desired (this is optional as long as he can retrieve it before media is sent).

@robin-raymond
Copy link
Contributor Author

@pthatcher - Thanks for the feedback on this one.

  1. Capabilities shouldn't say anything about RTX. Just assume RTX can be sent for any codec. I think we are all in agreement about that.

Yup.

  1. Parameters mostly just needs to specify an RTX PT one way or another. I think it would be nice if we had codec.rtxPayloadType instead of rtxCodec.parameters["apt"]. However, that will probably put it out of sync with WebRTC.

I think we are in agreement, and yes, being out of sync needlessly is bad. Perhaps we should instead take this to 1.0 group and describe that it's an annoying cross referencing "feature" codecs (and actually is not needed and causes issues when combined with clock rates, channels, etc, as described at the top of this bug report). If they agree and fix then we can all agree and fix.

  1. You also have to pick a PT for FEC, but it's not dumb like RTX. You just need one, not one per other PT.`

LOL. "not dumb like RTX".

True, but in the context of the codec, the value could be a PT within the context of the codec just like RTX, except that the same PT value could be set the same across multiple codecs. Just a nice little clean-up to avoid cross referencing codecs needlessly... I've already done the cross referencing logic in my implementation but I'd happily rip it out :)

@murillo128
Copy link

Take into account that RTX not only has a payload but a rtxtime, also if we remove it as a codec, we need a way to set the rtx ssrc.

@ibc
Copy link
Contributor

ibc commented Apr 8, 2016

dictionary RTCRtpCodecParameters {
        DOMString                 name;
        payloadtype               payloadType;
        unsigned long             clockRate;
        unsigned long             maxptime;
        unsigned long             ptime;
        unsigned long             numChannels;
        sequence<RTCRtcpFeedback> rtcpFeedback;
        Dictionary                parameters;
        RTCRtpCodecRTXParameters  rtx;
};

dictionary RTCRtpCodecRTXParameters {
        payloadtype               payloadType;
        unsigned long             rtxtime;
};

@robin-raymond
Copy link
Contributor Author

Take into account that RTX not only has a payload but a rtxtime, also if we remove it as a codec, we need a way to set the rtx ssrc.

Long thread above, so I'm including where this was described above how it could be done, and @ibc redefined a bit more explicitly:
#444 (comment)

@robin-raymond
Copy link
Contributor Author

To summarize:

RTX (and maybe FEC too) cause ambiguities as codecs and maybe better served to be aspects of existing codecs rather than full separate and independent codecs.

Since this would break 1.0 conceptually, I'm going to label it as such. I think an appropriate explanation of the issue should be brought to the 1.0 group and see if they have interest in addressing the problem then we can have a combined effort to resolve in both groups.

@aboba
Copy link
Contributor

aboba commented Apr 14, 2016

This issue requires a change to the WebRTC 1.0 object model. An Issue has been filed against WebRTC 1.0: w3c/webrtc-pc#548

@elagerway
Copy link
Contributor

Closing, this needs to be taken up in the WebRTC Working Group as per my recent message to mail list:
https://lists.w3.org/Archives/Public/public-ortc/2016Apr/0165.html

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

6 participants