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

From: Justin Uberti <[email-masked]>
Date: Tue, 4 Jul 2017 19:47:44 -0700

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 <[email-masked]> wrote:

>
>
> On Mon, Apr 17, 2017 at 12:07 PM, Mo Zanaty (mzanaty) <[email-masked]>
> wrote:
>
>> Agreed on all responses, except some further clarifications inline, see
>> Mo:.
>>
>> From: Justin Uberti <[email-masked]>
>> Date: Friday, April 14, 2017 at 6:45 PM
>> To: mzanaty <[email-masked]>
>> Cc: "[email-masked]" <[email-masked]>
>> 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) <[email-masked]>
>> 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
>>
>
>

---

From: "Mo Zanaty (mzanaty)" <[email-masked]>
Date: Wed, 5 Jul 2017 03:30:44 +0000

All changes look good to me.

Thanks,
Mo

On Jul 4, 2017, at 10:48 PM, Justin Uberti <[email-masked]<mailto:[email-masked]>> wrote:

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 <[email-masked]<mailto:[email-masked]>> wrote:

On Mon, Apr 17, 2017 at 12:07 PM, Mo Zanaty (mzanaty) <[email-masked]<mailto:[email-masked]>> wrote:
Agreed on all responses, except some further clarifications inline, see Mo:.

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

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

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:

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.

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.

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.

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

OLD:

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.

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

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

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

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

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.