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/RED/FEC handling #548

Closed
aboba opened this issue Mar 16, 2016 · 9 comments
Closed

RTX/RED/FEC handling #548

aboba opened this issue Mar 16, 2016 · 9 comments
Assignees

Comments

@aboba
Copy link
Contributor

aboba commented Mar 16, 2016

Currently, we have:

partial dictionary RTCRtpParameters {
sequence codecs;
};

dictionary RTCRtpCodecParameters {
unsigned short payloadType;
DOMString mimeType;
unsigned long clockRate;
unsigned short channels = 1;
DOMString sdpFmtpLine;
};

My assumption is that RTX (or RED or FEC, for that matter) are included in codecs[], and therefore that fmtp parameters such as "apt" and "rtxtime" will be included within sdpFmtpLine. It may be useful to make that statement explicit.

@aboba aboba changed the title RTCRtpRtxParameters: missing "apt"? RTX/RED/FEC handling Mar 30, 2016
@ibc
Copy link

ibc commented Apr 12, 2016

For a future re-design (and as already commented in several issues/threads):

  • RTX is per media codec so it should be a property of a codec (which means that RTX is not a codec).
  • FEC is per stream and can carry any other PT defined within the SSRC stream it's protecting to. Therefore it may make sense that FEC is a codec.
  • AFAIU, same for RED.

@robin-raymond
Copy link

To further explain the trouble:
RTX codecs have an "apt" value which cross links the source codec to the RTX codec. This forces a requirement that not only an RTX codec exist, and that a custom "parameter" property exist on the RTX codec to define the RTX "apt" linkage and rtxTime. The RTX's properties (like clock rate, and channels) match exactly in the ftmp line.

The simpler solution would be to have an RTX payload type as part of the original codec. Otherwise you get things like:

  • what happens if two RTX "apt" codecs point to the same media codec?
  • what happens if RTX clock rate/channel do not match the pointed apt codec?
  • what happens if RTX codec is missing but encodings says to use RTX?
  • capabilities return a list of codecs, but then require a RTX capability PER codec to define the PT for the RTX per codec with the correct fmtp

This adding a sub "rtx" dictionary in RTCRtpCodecParameters with:

partial dictionary RTCRtpCodecParameters {
   ...
   RTCRtpRtxCodecParameters rtx;
   ...
}

dictionary RTCRtpRtxCodecParameters {
   unsigned short payloadType;
   unsigned long rtxTime;
}

I also disagree with @ibc FEC / RED that FEC must be defined codecs any more than RTX must be a codec. The same philosophy can exist as RTX. The difference would be that the payloadType set in the the FEC codec could match other codecs because FEC can be shared, whereas with RTX they cannot.

In fact, I'd say FEC creates worst ambiguities as a codec than RTX. This is because RED+ULPFEC requires a set of cross referenced codecs to match, specifically with RED+ULPFEC.

RED on the receiving side is supposed to be able to decode anything the sender throws at it (but still requires the codecs be defined). But on the sender side it's required to list the redundancy list per RFC 2198 with RED. The trouble is that RED on the sender can package the raw video codec by itself, or the ULPFEC version, but not at the same time as RED normally expected per that RFC (which is a list of redundancy codecs, not "either" "or" possible payloadType values as its currently used).

Thus FEC left as as a codec it means:

  • ambiguity when multiple FEC codecs are listed, which FEC to use for which codec?
  • requires multiple FECs codecs, at minimal one per clock rate at minimal
  • on the encoder side, would you need an fmtp to define RED with the base video codec specified (thus required ONE FEC codec PER media type, see https://tools.ietf.org/html/rfc2198 ) and then include the ULPFEC codec as the 2nd redundancy encoding (even though it's sent mutually exclusively not like 2198 expects) and that would create multiple FECs codecs with the same codec payload type (and that is not legal); or leave the fmtp blank and assume a pseudo "*" matching logic on clock rate (which is not exactly proper either)?
  • would you need to define a separate FEC codec for the ULPFEC because of the fmtp, or again leave fmtp blank and then attempt to match on codec clock rate?
  • what ULPFEC to use if there are multiple (e.g. per clock rate)?

I would say those ambiguities can all be removed by including an FEC sub-dictionary to the codec.

partial dictionary RTCRtpCodecParameters {
   ...
   RTCRtpFecCodecParameters fec;
   ...
}

dictionary RTCRtpFecCodecParameters {
   DOMString mechanism;
   unsigned short payloadType;
   Dictionary parameters;
}

If flex-fec is used, then the mechanism could be defined as flex-fec and the payloadType would be specific the flex-fec codec's payload type with parameters defining the flex-fec's codec parameters, and multiple codecs could use the same FEC payload type. If red+ulpfec is the mechanism, then the "parameters" could include the definition for the ULPFEC codec payload type to used inside.

Thus the only ambiguities remaining is enforcing a few basic rules:

  1. For RTX, disallow codecs from sharing the same RTX payload type value;
  2. For FEC, disallow codecs from sharing the same FEC payload type value if of a different clock rate.
  3. For codecs that share the same FEC payload type value, the FEC parameters must be defined the same.

But the need to have multiple cross references RTX, FEC codecs is eliminated and all the fmtp complexities with RTX apt and RED 2198.

No matter what we do here, it's a design trade off scenario...

@ibc
Copy link

ibc commented Apr 14, 2016

  1. For RTX, disallow codecs from sharing the same RTX payload type value;
  2. For FEC, disallow codecs from sharing the same FEC payload type value if of a different clock rate.
  3. For codecs that share the same FEC payload type value, the FEC parameters must be defined the same.

AFAIU FEC protects a stream rather than a payload type, so what would happen if all but one codec carried over the same SSRC does not have fec parameter?

@robin-raymond
Copy link

FEC does protect the stream but it's not required that it be used in combination with every codec. In other words, you need not use a FEC in combination with a particular codec on the sender side (disallowed or not used). And on the receiver, it need not be expected to ever be seen.

@ibc
Copy link

ibc commented Apr 16, 2016

Clear. Thanks.

@robin-raymond
Copy link

Also, we should consider if CN and DTMF are other "codec features" that could operate in the same manner like RTX / FEC, or be left as "magic" cross referenced codec features.

@robin-raymond
Copy link

robin-raymond commented Apr 22, 2016

Related: w3c/ortc#497

While in theory there's no RTX/RED/ULPFEC specific to a kind, in practice, it's usage is very much not only specific to a kind but specific to a codec. This is another reason to have the "feature" codecs as defined within the context of a codec.

I'm also not sure if FEC might need a mechanism list in capabilities (i.e. unlike ortc, capabilities in webrtc do not announce anything beyond the codec mimetype), not just a single mechanism; A browser might might want to announce support RED, RED+ULPFEC, and FlexFEC at the same time, or "either"/"or" the mechanisms exclusively on a particular codec.

The RTCRtpCodecParameters should not have RED, RED+ULPFEC, and FlexFEC mechanisms listed together (unless a valid use case is a receiver might expect "either"/"or" of multiple FEC scenarios but not know which to choose/expect in advance).

Just some more food for thought on this issue....

@aboba
Copy link
Contributor Author

aboba commented Jun 1, 2016

@robin-raymond What about RTCRtpCodecCapabilities.codecs[]? Is "rtx" still included there?

@alvestrand
Copy link
Contributor

Fixed by #733

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

7 participants