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

[Justin Uberti <[e-mail]>]

On Mon, Apr 17, 2017 at 12:07 PM, Mo Zanaty (mzanaty) <[e-mail]>
wrote:

> Agreed on all responses, except some further clarifications inline, see
> Mo:.
>
> From: Justin Uberti <[e-mail]>
> Date: Friday, April 14, 2017 at 6:45 PM
> To: mzanaty <[e-mail]>
> Cc: "[e-mail]" <[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]>
> 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.
>

This makes sense to me. As indicated above, there isn't enough rationale to
mandate "red" support.

However, I am thinking of making "red" support SHOULD strength, since at
present it's our best available tool for handling burst losses.

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


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

OK, this is a welcome development. I will update this section accordingly
to give an overview of how this should work, which will be a nontrivial
update.

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

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