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

[RtpParameters] Spec should state that codec.payloadType must be unique #405

Closed
ibc opened this issue Mar 4, 2016 · 12 comments
Closed

[RtpParameters] Spec should state that codec.payloadType must be unique #405

ibc opened this issue Mar 4, 2016 · 12 comments

Comments

@ibc
Copy link
Contributor

ibc commented Mar 4, 2016

If two codecs (let's say VP8 and VP9) within the same RtpParameters share the same payloadType: 100 then the world explodes (although it may be valid in the ORTC syntax).

IMHO the spec should state that codec.payloadType must be unique within the same RtpParameters.codecs.

@ibc
Copy link
Contributor Author

ibc commented Mar 4, 2016

Also, encoding.codecPayloadType should be unique (same reason).

And when it happens that each codec should have its specific encoding parameters, it comes to my mind that RTCEncodingParameters should be a property of each RTCCodecParameters (but that's another story).

@robin-raymond
Copy link
Contributor

Agreed on RtpParameters.codecs.payloadType but encoding.codecPayloadType points to the codec to use within codec list and must match a value specified within RtpParameters.codecs.payloadType codec list.

@ibc
Copy link
Contributor Author

ibc commented Mar 8, 2016

  • What does a encoding without codecPayloadType mean when there are also other encodings with codecPayloadtype? Note that according to the ORTC spec, encoding.codecPayloadType is optional.
  • What do two encodings with same codecPayloadType mean?

I think we should make this subject more strict, so there are no redundant/similar, but different, ways of expressing the same.

My 2 cents:

  • encoding.codecPayloadType should be mandatory and, of course, must point to a codec with same payloadType.
  • For each codec in codecs (and assuming that codec.payloadType MUST be unique), there should be a corresponding encoding with same codecPayloadType.

@aboba
Copy link
Contributor

aboba commented Mar 9, 2016

The spec says that if encodings[].codecPayloadType is unset, the browser will choose it. In practice I do not see how this can work - for a sender, the encoder will not know what codec to use to encode, and for a receiver it will not know how to decode packets routed to it.

@ibc
Copy link
Contributor Author

ibc commented Mar 9, 2016

OK, so then why aren't encoding defined within codecs? At the end we are agreeing in:

  • PT in codecs must be unique.
  • encoding must have a codecPayloadType existing in codecs.

So, it would be much easier as follows:

dictionary RTCRtpCodecParameters {
             DOMString                  name;
             payloadtype                payloadType;
             unsigned long              clockRate;
             unsigned long              maxptime;
             unsigned long              numChannels;
             RTCRtpEncodingParameters   encoding;     // <---- NOTE 
             sequence<RTCRtcpFeedback>  rtcpFeedback;
             Dictionary                 parameters;
};

(and remove codecPayloadType from RTCRtpEncodingParameters).

The new RTCRtpCodecParameters.encoding field may be optional, and we are done. This would allow the same as the current spec, but in a much more robust and less error prune fashion.

@robin-raymond
Copy link
Contributor

@aboba wrote:

So if it is unset or there is no match that seems like cause for an exception. Having the browser choose encodings[].codecPayloadType makes no sense to me.

The previously discussed behaviour was pick the first useful codec in the list and use it. That's how I've implemented mine. This is not a required parameter in my implementation.

@ibc I don't like encodings being inside the codec. Some codecs like ulp+fec, red, etc require multiple codecs in coordination to work. Plus, I don't like having to enumerate a list within a sub list to figure out what's going on. This would be a major change at this point.

The idea behind the codec list is to indicate the codecs available to the system. The encodings was to describe each simulcast or layer. They have separate meanings.

@ibc
Copy link
Contributor Author

ibc commented Mar 9, 2016

I agree as long as the spec properly states the constraints and requirements of both RtpCodecParameters and RtpEncodingParameters:

  • PT in codecs must be unique.
  • encoding must have a codecPayloadType existing in codecs.

@aboba
Copy link
Contributor

aboba commented Mar 9, 2016

A few questions:

  1. Robin suggests that encodings[],codecPayloadType can be unset, but that the browser should choose the "first useful codec". However, since there is no encodings[].kind attribute, how do we know whether to choose an audio or video codec from parameters.codecs[]?
  2. With respect to "PT in codecs must be unique" we have both RTCRtpCodecCapability.preferredPayloadType. Typically, RTCRtpCodecCapability.preferredPayloadType will be different for each value of RTCRtpCodecCapability.name (e.g. a differently named codec will have a different preferred payloadType). Should we be more explicit about this?
  3. With respect to RTCRtpCodecParameters.payloadType. In Issue Same payload for different codecs in same RtpSender/RtpReceiver #407 we talked about allowing different codecs to use the same payloadType. So shouldn't we avoid adding a rule on reusing a value of payloadType for a different codec, since the matching rules in Section 8.3 do not prohibit that?
  4. Two encodings within the same RtpSender/RtpReceiver can have the same value of encodings[].codecPayloadType (e.g. simulcast or scalable video coding). So this is not an issue, is it?

@aboba aboba added the 1.1 label Mar 9, 2016
@ibc
Copy link
Contributor Author

ibc commented Mar 10, 2016

Robin suggests that encodings[],codecPayloadType can be unset, but that the browser should choose the "first useful codec". However, since there is no encodings[].kind attribute, how do we know whether to choose an audio or video codec from parameters.codecs[]?

I assume that RtpParameters.codecs must be set in order for the browser to be able to choose a codec for a encoding without codecPayloadType.

With respect to "PT in codecs must be unique" we have both RTCRtpCodecCapability.preferredPayloadType. Typically, RTCRtpCodecCapability.preferredPayloadType will be different for each value of RTCRtpCodecCapability.name (e.g. a differently named codec will have a different preferred payloadType). Should we be more explicit about this?

I think that a mapping between PT and codec name is desirable.

With respect to RTCRtpCodecParameters.payloadType. In Issue #407 we talked about allowing different codecs to use the same payloadType. So shouldn't we avoid adding a rule on reusing a value of payloadType for a different codec, since the matching rules in Section 8.3 do not prohibit that?

I think we better prohibit duplicated PT for different codecs within same RtpParameters. I don't think that would provide any useful feature.

Two encodings within the same RtpSender/RtpReceiver can have the same value of encodings[].codecPayloadType (e.g. simulcast or scalable video coding). So this is not an issue, is it?

I don't know too much about simulcast/SVC. Does it work by having different streams (so different SSRCs) and sending the same PT over them with different configuration? If so, then of course we need to allow different encoding entries with same codecPayloadType.

@robin-raymond
Copy link
Contributor

Doc needs to add that capabilities preferredPayloadType needs to return a unique PT for each.

@robin-raymond
Copy link
Contributor

PT is codec list must already be unique for any given list of codecs within a RtpParameter list.

@robin-raymond
Copy link
Contributor

on receiver.receive() we need to disallow changing of media kind. This should not be legal.

aboba added a commit that referenced this issue Mar 12, 2016
@aboba aboba closed this as completed Mar 13, 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