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

[Proposal] Add static RTCRtpParameters.factory() method #447

Closed
ibc opened this issue Apr 1, 2016 · 30 comments
Closed

[Proposal] Add static RTCRtpParameters.factory() method #447

ibc opened this issue Apr 1, 2016 · 30 comments

Comments

@ibc
Copy link
Contributor

ibc commented Apr 1, 2016

Let's assume I want to send video.

I get my video capabilities:

var myVideoCaps = RTCRtpSender.getCapabilities('video');

// I get this:
{
  codecs :
  [
    {
      name                 : 'VP8',
      kind                 : 'video',
      clockRate            : 90000,
      preferredPayloadType : 100,
      rtx :
      {
        preferredPayloadType : 96,
        rtxTime              : 1234  // No idea
      },
      rtcpFeedback :
      [
        { type: 'ccm', parameter: 'fir' },
        { type: 'nack' }
      ]
    }
  ]
}

So just VP8 with RTX and some RTCP stuff.

Assuming I exchange my capabilities with the remote peer and we both agree on VP8, I need to construct my RTCRtpParameters which would look as follows:

var myRtpParameters =
{
  muxId  : 'my-video',  // Yeah!
  codecs :
  [
    {
      name        : 'VP8',
      clockRate   : 90000,
      payloadType : 100,
      rtx :
      {
        payloadType : 96,
        rtxTime     : 1234
      },
      rtcpFeedback :
      [
        { type: 'ccm', parameter: 'fir' },
        { type: 'nack' }
      ]
    }
  ],
  encodings : [],  // Empty
  rtcp      :  // TODO
};

QUESTION 1:

How can I set the RTCRtcpParameters rtcp field?

QUESTION 2:

Simple scenario so, of course, I can leave the encodings field empty. But... it happens that I need to generate a SDP offer so I need the SSRCs.

This means that I need to create the encodings sequence by myself. Ok, we know that we'll just send VP8 and its RTX, so it is clear that just 2 streams are needed, so we can create this encodings:

myRtpParameters.encodings =
[
  {
    ssrc             : 11111111, // why not?
    codecPayloadType : 100
  },
  {
    ssrc             : 22222222, // why not?
    codecPayloadType : 96
  }
];

QUESTION 3:

This is a very simple example. Let's imagine multiple codecs, FEC, RED, etc. And I do need the SSRCs before sending media. Do I really need to create a complex encodings sequence with as many entries as media codecs + feature codecs?

Simpler API solution:

UPDATE: See comment below for a better proposal.

var myRtpParameters;  // The above RTCRtpParameters with no `encodings` nor `rtcp`

// Set my RTP parameters but don't send RTP yet
videoRtpSender.setParameters(myRtpParameters);

// Get the completed & updated parameters (includes `encodings` and `rtcp`)
var myUpdatedRtpParameters = videoRtpSender.getParameters();

// Let's create a SDP offer and send it, etc

// Send RTP
videoRtpSender.send();  // No arguments

QUESTION 4:

How am I supposed to set the remaining RTCRtpEncodingParameters fields? I mean:

unsigned long       maxBitrate;
double              minQuality = 0;
double              resolutionScale;
double              framerateScale;
boolean             active = true;
DOMString           encodingId;
sequence<DOMString> dependencyEncodingIds;

Who is supposed to fill them? how can I know which values are supported by my browser?

@ibc
Copy link
Contributor Author

ibc commented Apr 1, 2016

Better approach (compatible with current spec):

var myRtpParameters;  // The above RTCRtpParameters with no `encodings` nor `rtcp`

// Optionally generate full parameters via a static `RTCRtpParameters` method:
var myFullRtpParameters = RTCRtpParameters.factory(myRtpParameters);

// myFullRtpParameters includes `encodings`, `rtcp, etc (chosen by the browser)

// Let's create a SDP offer and send it, etc

// Send RTP
videoRtpSender.send(myFullRtpParameters);  // Same as now

I've just added a static RTCRtpParameters.factory() method. It breaks nothing and solves the need (in some cases) of having full parameters before sending media.

NOTE: I've updated the subject of the issue.

@ibc ibc changed the title [Proposal] RtpSender setParameters(), getParameters() and send() [Proposal] Add static RTCRtpParameters.factory() method Apr 1, 2016
@ibc
Copy link
Contributor Author

ibc commented Apr 1, 2016

NOTE: Whether RTCRtpEncodingParameters keep their current syntax or not is 100% irrelevant for this API proposal.

@robin-raymond
Copy link
Contributor

Add static RTCRtpParameters.factory() method

This is something you can write entirely in JS, no? Why do you think it needs to be part of the API?

@ibc
Copy link
Contributor Author

ibc commented Apr 1, 2016

This is something you can write entirely in JS, no? Why do you think it needs to be part of the API?

By letting the browser choose the SSRCs (while still letting us know them before sending media) we avoid the "sending SSRC conflict issue".

Also, each browser may want to create different streams model based on its capabilities and internal implementation. It is easier to ask the browser rather than assuming that every browser will understand a so complex encodings sequence.

Even more: if we want to send a SVC offer we may want the browser to provide us with a full encodings sequence that includes the different SSRCs for such a SVC codec and their corresponding PT (I don't know whether a SVC codec uses different PT in each layer or not).

Also, consider questions 1 and 4.

@robin-raymond
Copy link
Contributor

By letting the browser choose the SSRCs (while still letting us know them before sending media) we avoid the "sending SSRC conflict issue".

The SSRC for a sender can be chosen by anyone as long as they don't reuse the same SSRC twice in a row. So I could argue that merely having a method to allocate next ssrc is enough to satisfy this concern, although even that could be done in JS but just not with as much certainty as a browser.

Also, each browser may want to create different streams model based on its capabilities and internal implementation. It is easier to ask the browser rather than assuming that every browser will understand a so complex encodings sequence.

Aw, but I would say it doesn't remove the complex encodings. We still need to allow flexibility for the "do as I say" part of the API (and that includes fully loaded gun pointing to a foot). The parameters can also be generated by receiving capabilities of a remote party so a factory would complicate the parameters more because then the parameters from the factory would need to be merged into a routine that generated them from capabilities (making this more complex and not less).

So... I would say if the parameters are too complex to implement then we need to make sure they are defined enough to be implementable and fix any areas of concern rather than add yet another method that makes (IMHO) things even more problematic and needs another definition.

Also, each browser may want to create different streams model based on its capabilities and internal implementation

Yes, except that you may base your generation not on local capabilities but on remote ones. That's how I've seen things done in samples thus far and it works well.

Even more: if we want to send a SVC offer we may want the browser to provide us with a full encodings sequence that includes the different SSRCs for such a SVC codec and their corresponding PT (I don't know whether a SVC codec uses different PT in each layer or not).

I would want to see a proposal / explanation of pain areas on that before passing judgement... and why it's needed for SVC's case. It's a bit too vague right now to understand why it's needed (although it might be).

Q1: How can I set the RTCRtcpParameters rtcp field?

Not sure I understand the question...

Q4: How am I supposed to set the remaining RTCRtpEncodingParameters fields? I mean:
Who is supposed to fill them? how can I know which values are supported by my browser?

Capabilities may be lacking (or not needed); this does need review. A developer would definitely fill them as appropriate. Maybe I'm not clear on the question(s) though?

@ibc
Copy link
Contributor Author

ibc commented Apr 1, 2016

So... I would say if the parameters are too complex to implement then we need to make sure they are defined enough to be implementable and fix any areas of concern rather than add yet another method that makes (IMHO) things even more problematic and needs another definition.

That sounds good to me, but then we need a complete refactor of the current RtpEncodingParameters (the current sequence of separate but inter-related objects is not a choice IMHO) so we avoid ambiguities and different ways of expressing the same. Let's be strict.

To be clear: a SRTP stream is identified by a SSRC value and carries payloads on top of it, so why should we need a sequence of separate encodings with same SSRC value? It's clear to me that in the future encodings model each entry should mean a SRTP stream (so unique SSRC value) and should include the list of PTs it carrries (along with stream parameters).

Regarding the SVC codec and its representation within codecs and encodings, I would also like to see an example since I'm not an expert in this topic.

Not sure I understand the question...

Regarding RTCRtcpParameters, why should I be allowed to set cname or reducedSize (taking into account that I can't know whether my browser supports reducedSize or not?

Capabilities may be lacking (or not needed); this does need review.

OK.

A developer would definitely fill them as appropriate.

It's hard to manually fill them when we have no way yo know what the underlying browser supports.

Maybe I'm not clear on the question(s) though?

Now yes :)

@ibc
Copy link
Contributor Author

ibc commented Apr 1, 2016

To be clear: a SRTP stream is identified by a SSRC value and carries payloads on top of it, so why should we need a sequence of separate encodings with same SSRC value? It's clear to me that in the future encodings model each entry should mean a SRTP stream (so unique SSRC value) and should include the list of PTs it carrries (along with stream parameters).

Just want to be sure I'm right. Is the following encodings valid or not?

[
    {
        ssrc: 1111,
        codecPayloadType: 101,  // VP8
        active: true,
        priority: 1,
        maxBitrate: 1000
    },
    {
        ssrc: 1111,
        codecPayloadType: 102,  // VP9
        active: false,
        priority: 2,
        maxBitrate: 500
    }
]

@robin-raymond
Copy link
Contributor

@ibc As a sender, that's not valid. Two ssrcs cannot be used to send outgoing. You need to pick one. For the receiver, that's fine.

@ibc
Copy link
Contributor Author

ibc commented Apr 1, 2016

I don't understand. There is just a single SSRC (1111) so a single SRTP stream ready to carry PT 101 and PT 102 (or add here CN, DTMF, etc). How is this not valid?

I assume I did not properly explain and I assume that a encodings sequence (in a sender) allows multiple entries with same SSRC but different PT (wchi must be unique as per spec).

My question was intended to clarify whether two encodings entries with same SSRC and different PT can hold different values for certain attributes such as active, priority, etc.

@robin-raymond
Copy link
Contributor

@ibc you have a list of 2 encodings, both with ssrc 1111. If you put that into a sender the implication is that you are sending 2 streams, one with VP8 and the other with VP9 both using ssrc 1111. That's not legal.

As a receiver it's fine because the idea is that's a simulcast, where only one is active at a time (thus legal).

@ibc
Copy link
Contributor Author

ibc commented Apr 1, 2016

Ok....

[
    {
        ssrc: 1111,
        codecPayloadType: 101,  // alaw
        active: true,
        priority: 1,
        maxBitrate: 1000
    },
    {
        ssrc: 1111,
        codecPayloadType: 102,  // CN
        active: false,
        priority: 2,
        maxBitrate: 500
    }
]

@robin-raymond
Copy link
Contributor

@ibc CN is a 'feature' codec, similar to RTX, RED and FEC in many ways. Those are just described codecs in the codec list and can be used as appropriate by the engine if supported. There's no need (or purpose) to define them as a separate encoded stream. That would not be legal because it's not a proper media codec on its own, nor a layer.

@ibc
Copy link
Contributor Author

ibc commented Apr 1, 2016

Then, should we reconsider #439?

@robin-raymond
Copy link
Contributor

@ibc no, it's legal to send VP8 and VP9 if you use different SSRCs in the sender (sender simulcasting); as well as legal in the receiver. Just that "feature" codecs are not encodings, they are codec descriptions and are only needed in the codec list because they require separated PT values from the main encoding but they can be understood without an encoding.

@ibc
Copy link
Contributor Author

ibc commented Apr 1, 2016

Note that in SDP land two browser may signal support for VP8 and VP9 while just indicating a single SSRC for media codecs transmission. And both must be ready to receive both codecs at any time. Or that's the theory. Or may be they can just select any of the peer supported media codecs but cannot change it in runtime (without renegotiation).

However, this arises another concern: If CN and DTMF are not real codecs, then they should be properties of real codecs (same as RTX and FEC).

And, in the other side, it is becoming more and more clear to me that the "future" encodings field should not be a sequence of separate RtpEncodingParameters, but a more strict object with properties such as:

  • ssrc (stream for media codecs)
  • rtxSsrc (if needed)
  • fecSsrc (if needed)

Even more, assuming this (so RTX/FEC are no longer codecs in the list) how should I fill encodings if I want to manually set the SSRC for RTX/FEC?

@robin-raymond
Copy link
Contributor

@ibc Yup, something like that, hence the bug filing #444 and codec "features".

@ibc
Copy link
Contributor Author

ibc commented Apr 1, 2016

So assuming #444 we no longer would need a "sequence of encodings" in the sender, right?
I mean, the encodings field of the RtpParameterscould just be a single dictionary as follows:

{
    unsigned long       ssrc;
    unsigned long       rtxSsrc;
    unsigned long       fecSsrc;
    RTCPriorityType     priority;
    unsigned long       maxBitrate;
    double              minQuality = 0;
    double              resolutionScale;
    double              framerateScale;
    unsigned long       maxFramerate;
    boolean             active = true;
    DOMString           encodingId;
    sequence<DOMString> dependencyEncodingIds;
}

Am I right?

@robin-raymond
Copy link
Contributor

@ibc no, sequence is still needed for simulcasting and/or SVC from the sender; otherwise it's a sequence of 1 for simple scenario.

@ibc
Copy link
Contributor Author

ibc commented Apr 1, 2016

Fine. And just in those cases (simulcast or SVC) the codecPayloadType member would make sense.

@robin-raymond
Copy link
Contributor

@ibc yes

@ibc
Copy link
Contributor Author

ibc commented Apr 1, 2016

Great. I think we'll need to come to this when we are able to describe simulcast and SVC examples, but I like the way it's going on.

@ibc
Copy link
Contributor Author

ibc commented Apr 1, 2016

Mmmm, so in a simple use case of VP8, send() would just hold a single media codec (VP8) and (optionally) a single encoding (no need for codecPayloadType).

And when in simulcast or SVC, for each media codec in codecs there could be just a single (and optional) encoding and it MUST have a codecPayloadType. Which also leads to the fact that just one encoding without codecPayloadType should be allowed.

@ibc
Copy link
Contributor Author

ibc commented Apr 1, 2016

(RED stuff missing)

@ibc
Copy link
Contributor Author

ibc commented Apr 4, 2016

Note for future discussions: It makes no sense at all that ssrc is a property of encoding. "Encoding" means codec payload processing.

@ibc
Copy link
Contributor Author

ibc commented Apr 4, 2016

NOTE 2: Fec is not per codec but per stream, so a single PT handles all the media codec PTs within a media stream. Therefore fec must not be a property of a codec. In contrast, rtx requires a PT per protected media codec.

@aboba
Copy link
Contributor

aboba commented Apr 6, 2016

See related Issue: #238

RTCRtpParameters function myCapsToSendParams(RTCRtpCapabilities sendCaps, RTCRtpCapabilities remoteRecvCaps)

produces an RTCRtpParameters dictionary that can be used when calling send(parameters), given the sender and remote receiver capabilities. Similarly,

myCapsToSendParams(remoteSendCaps, recvCaps)

produces an RTCRtpParameters dictionary that can be used when calling receive(parameters).

@ibc
Copy link
Contributor Author

ibc commented Apr 7, 2016

@aboba: My above proposal (which can be closed since there is a much better one in progress) was not related to codecs negotiation. Its aim was to tell the browser to properly generate a full RTCRtpParameters object (which includes ssrc and so on).

The current API suggests "here you can put SSRCs but please never put SSRCs because by using PT is enough for receiver given the '8.3 RTP Matching Rules' section". Even more, the current API (when it comes to RTP parameters) is not an API, but a JSONized version of a SDP m= line, which makes things even worse given that now I need to be an expert in SDP (to understand all these parameters) and then in ORTC (to understand how to map all those SDPish parameters into the RTCRtpPArameters blob).

A new real API proposal is being cooked.

@robin-raymond
Copy link
Contributor

The current API suggests "here you can put SSRCs but please never put SSRCs because by using PT is enough for receiver given the ...

I'm not sure why you say this - routing by PT is one option but not the only options. We support routing by PT, SSRC, Mux ID, and RID (at the encoding level), and, any combination of those together.

If SDP was just about marshalling a few codecs and a few encodings I think it would be far less problematic, but we know it's a lot more than that which is why the API reduces it to the actually encoding information needed: codecs + encodings.

@robin-raymond
Copy link
Contributor

@ibc this bug has been tossed around a bit, feel it's lost a bit of focus; Is there something here to remain proposed at this point? I kind of feel that if there is it might need a fresh issue proposal if so;

@ibc
Copy link
Contributor Author

ibc commented Apr 16, 2016

@robin-raymond I agree that this subject (and others) should be placed into a WG meeting.

Just to summarize my opinion, the current RtpParameters API is not an API but the "description" of what a real API should internally produce and, optionally, a read-only Object retrievable via a getParameters() API method or a parameters getter.

Please ignore it. Let's wait for the WG meeting and proposals will show up.

@ibc ibc closed this as completed Apr 16, 2016
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