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

[Justin Uberti <[e-mail]>]

Thanks for this detailed review.

On Fri, Apr 7, 2017 at 7:17 PM, Mo Zanaty (mzanaty) <[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.


> 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.


> 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?

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.


> 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