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

[Justin Uberti <[e-mail]>]

These issues have all been resolved in the latest version of the draft,
https://tools.ietf.org/html/draft-ietf-rtcweb-fec-06.

On Sat, Apr 22, 2017 at 12:39 PM, Justin Uberti <[e-mail]> wrote:

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