Re: [rtcweb] Review of draft-ietf-rtcweb-fec

["Mo Zanaty (mzanaty)" <[e-mail]>]

Agreed on all responses, except some further clarifications inline, see Mo:.

From: Justin Uberti <[e-mail]<mailto:[e-mail]>>
Date: Friday, April 14, 2017 at 6:45 PM
To: mzanaty <[e-mail]<mailto:[e-mail]>>
Cc: "[e-mail]<mailto:[e-mail]>" <[e-mail]<mailto:[e-mail]>>
Subject: Re: Review of draft-ietf-rtcweb-fec

Thanks for this detailed review.

On Fri, Apr 7, 2017 at 7:17 PM, Mo Zanaty (mzanaty) <[e-mail]<mailto:[e-mail]>> wrote:
Here is my review of draft-ietf-rtcweb-fec-04.

4.1. Recommended Mechanism (for audio FEC), first paragraph says:

OLD:
"When using the Opus codec, use of the built-in Opus FEC mechanism is
RECOMMENDED. This provides reasonable protection of the audio stream
against typical losses, with modest overhead. Note that as indicated
above the built-in Opus FEC only provides single-frame redundancy; if
multi-packet protection is needed, the built-in FEC should be
combined with [RFC2198] redundancy to protect the N-2th, N-3rd, etc.
packets."

The last sentence about multi-packet protection should not be recommended due to overhead. Packing multiple full-size Opus frames, each with their own built-in prior-frame FEC, into RED packets is akin to packing multiple PCMU frames into RED packets. The latter is discouraged 3 paragraphs later:

"When using constant-bitrate codecs, e.g. PCMU, use of [RFC2198]
redundant encoding MAY be used, but note that this will result in a
potentially significant bitrate increase, and that suddenly
increasing bitrate to deal with losses from congestion may actually
make things worse."

These same warnings would apply to multi-packet Opus RED. Therefore I suggest rewording the first paragraph as follows.

NEW:
"When using the Opus codec, use of the built-in Opus FEC mechanism is
RECOMMENDED. This provides reasonable protection of the audio stream
against typical losses, with modest overhead. Note that as indicated
above the built-in Opus FEC only provides single-frame redundancy; if
multi-packet protection is needed, the built-in FEC MAY be
combined with [RFC2198] redundancy to protect prior packet pairs,
but note this will result in significant bitrate increase which may
aggravate congestion losses."

This isn't quite as bad as full PCMU frames, because Opus frames will typically be smaller, but I agree it is inconsistent.

https://github.com/juberti/draughts/issues/38


4.2. Negotiating Support, first sentence says:

OLD:
"Support for redundant encoding MUST be indicated by offering "red"..."

This may be misinterpreted as mandating that WebRTC endpoints MUST offer "red", rather than merely indicating that if they choose to support redundant encoding (which is only RECOMMENDED for VBR codecs without internal FEC in the prior section), then it MUST be indicated by offering "red". I suggest rewording this as:

NEW:
"If redundant encoding is supported, it MUST be indicated by offering "red"..."

The intent here was that all clients should support receipt of 2198, even if they don't support sending a redundant encoding (which is only, as you say, RECOMMENDED). IOW, "red" would be a MTI format.

Mo: MTI "red" only if you support VBR codecs without internal FEC (which are optional not MTI audio codecs), right? If so, I would explicitly indicate this via "Support for redundant encoding of VBR codecs without internal FEC MUST..."
Or do you mean MTI regardless of codecs, like even for Opus? That would surprise me, to make "red" MTI when it is not recommended for the MTI audio codecs, and no browser advertises "red" for audio.

Same comment for Opus in the following paragraph:

OLD:
"For Opus, a receiver MUST indicate that it is prepared to use
incoming FEC data with the "useinbandfec=1" parameter..."

NEW:
"For Opus, a receiver that it is prepared to use incoming FEC data
MUST include the "useinbandfec=1" parameter..."

Similarly here - for WebRTC, the intent was to mandate support for receiving and using Opus FEC.

Mo: Ok, this one makes sense to be MTI.

5.1. Recommended Mechanism (for video FEC) says:

OLD:
"For video content, use of a separate FEC stream with the RTP payload
format described in [I-D.ietf-payload-flexible-fec-scheme] is
RECOMMENDED. The receiver can demultiplex the incoming FEC stream by
SSRC and correlate it with the primary stream via the SSRC field
present in the FEC header."

Flex FEC moved the SSRC binding from the FEC header to the CSRC list. Only the retransmission format still has the SSRC field in the FEC header. Reword as:

NEW:
"For video content, use of a separate FEC stream with the "flexfec" RTP payload
format described in [I-D.ietf-payload-flexible-fec-scheme] is
RECOMMENDED. The receiver can demultiplex the incoming FEC stream by
SSRC and correlate it with the primary stream(s) via the CSRC(s)
in the RTP header of the FEC repair packet, or via the SSRC field
in the FEC header for retransmissions."

OK.  https://github.com/juberti/draughts/issues/40

The next paragraph suggests multiple source streams is a problem.

OLD:
"Support for protecting multiple primary streams with a single FEC
stream is complicated by WebRTC's 1-m-line-per-stream policy, which
does not allow for a m-line dedicated specifically to FEC."

But Flex FEC already supports this with SSRC(s) of primary stream(s) as CSRC(s) of the FEC stream, so strike the above OLD paragraph.

It's still not clear how this should work. Which m= lines would carry FEC info for which other m= lines?

Mo: Flex FEC can protect multiple primary streams as long as they are in the same RTP session (SSRC space), e.g. when bundled. There is no SDP association between source and repair streams for Flex FEC. The association is at the RTP level with SSRCs, so a Flex FEC packet can protect any RTP packet(s) in the same RTP session. In SDP, any bundled m= line(s) can declare the flexfec PT. JSEP requires all of them to declare it, which seems best.

If we are going to support this, I think we need to detail in this document how it should work, and it may have downstream ramifications on JSEP.

Mo: JSEP says the FEC PT must be included in all m= lines, which seems best/correct.

8. Adaptive Use of FEC, first paragraph says:

OLD:
"...methods like RTX [RFC4588], which only transmits redundant data when..."

Flex FEC also supports retransmissions, so reword as:

NEW:
"...methods like RTX [RFC4588] or the "flexfec" retransmission format,
which only transmits redundant data when..."

Same comment in the next paragraph.

OLD:
"Given this, WebRTC implementations SHOULD consider using RTX instead..."

NEW:
"Given this, WebRTC implementations SHOULD consider using RTX or
the "flexfec" retransmission format instead..."


https://github.com/juberti/draughts/issues/41

9. Security Considerations

Add a final paragraph on the order of FEC and SRTP operations.

NEW:
"SRTP [RFC3711] defines the default order of FEC and SRTP as FEC followed by SRTP at the sender, and SRTP followed by FEC at the receiver. DTLS-SRTP [RFC5764] uses this same default order for all SRTP Protection Profiles."

https://github.com/juberti/draughts/issues/42

Editorial:

Abstract and Introduction should use WebRTC "endpoint" as defined in -overview.
Abstract: "... FEC ... used by WebRTC applications" -> WebRTC endpoints
Introduction: "... FEC ... for WebRTC client implementations" -> WebRTC endpoints
Or you could be very generic and just say WebRTC implementations everywhere.

I think WebRTC implementations is best, since these are guidelines for WebRTC implementors, not application implementors.

https://github.com/juberti/draughts/issues/43