Subject: Re: [payload] FlexFEC draft feedback

From: Rasmus Brandt <[email-masked]>
Date: Thu, 07 Dec 2017 09:48:56 +0000

>
> ---------- Forwarded message ----------
> From: Brian Baldino <[email-masked]>
> To: [email-masked]
> Cc:
> Bcc:
> Date: Wed, 29 Nov 2017 12:18:18 -0800
> Subject: [payload] FlexFEC draft feedback
> I've been implementing flexfec on the jitsi videobridge (starting with
> just the receive side for now) and came across a couple things.  Also Boris
> Grozev has compiled some notes from his readings of the draft; I've tried
> to summarize all our thoughts here:
>
> 1) It would be nice if the draft mentioned that the header extensions of
> the FEC packets are totally independent of the media's header extensions,
> and that one can add extensions to FEC packets without having any effect on
> the recovery.
>
> 2) It would be nice if the draft explicitly stated that RTP header
> extensions (and anything in the header beyond the 12 byte fixed header,
> really) is protected as part of the payload.
>
> 3) I don't think it's explicitly mentioned that the first bit of the mask
> represents a delta value of '0' (and, therefore, a '1' in that bit means
> that the base sequence number itself is protected), it would be nice to
> call this out [in Section 6.3.1.2 maybe?]
>

This is alluded to in Section 3.2
<https://tools.ietf.org/html/draft-ietf-payload-flexible-fec-scheme-05#section-3.2>
:

bitmask: Run-length encoding of packets protected by a FEC packet.
         If the bit i in the mask is set to 1, the source packet number *N + i*
         is protected by this FEC packet.  Here, N is the sequence number
         base, which is indicated in the FEC packet as well.

and in Section 4.2
<https://tools.ietf.org/html/draft-ietf-payload-flexible-fec-scheme-03#section-4.2>
:

The SN base_i (16 bits) field indicates the *lowest sequence
number*, taking wrap around into account, of the source packets for
a particular SSSRC (indicated in SSRC_i) *protected by this repair packet*.

The draft is however not internally consistent, because in Section 4.2 it
also says:

If the F-bit is set to 0, it represents that the source packets of
all the SSRCs protected by this particular repair packet are
indicated by using a flexible bitmask.  Mask is a run-length
encoding of packets for a particular SSRC_i protected by the FEC
packet.  Where a bit j set to 1 indicates that the source packet
with sequence number *(SN base_i + j + 1)* is protected by this FEC
packet.

which seems to be off-by-one compared to the earlier descriptions.

When using the offset-based masks, the base sequence number is also
inclusive:

If M>0, N=0,  is Row FEC, and no column FEC will follow
               Hence, FEC = SN, SN+1, SN+2, ... , SN+(M-1), SN+M.

as well as when doing pure retransmissions:

By setting R to 1, F to 1, this FEC protects only one packet, i.e.,
the FEC payload carries just the packet indicated by SN Base_i, which
is effectively retransmitting the packet.

To make the descriptions consistent, I think *(SN base_i + j + 1)* should
be replaced by *(**SN base_i + j)* in Section 4.2.

4) In some of the diagrams there is a typo making it look like the 'P'
> field is 2 bits instead of 1 (Figures 10, 12, 13, and 15)
>
> 4) From an implementation perspective, I found the extraction of the mask
> to be a bit of a pain due to having to extract the k bits and shift the
> actual mask to get the offsets.  I wondered if there was a reason not to
> move the second k bit next to the first to make them consecutive, followed
> by a consecutive mask.  I thought of two ways this might be done:
>
> a) Put a 2 bit k field at the start of the mask.  There are multiple ways
> to define an encoding for those 2 bits to represent a 14, 46 or 110 mask.
> This has the downside of making the first mask 1 bit smaller (14 bits vs
> 15) but does allow for encoding a 4th mask length, if so desired.
>
> b) Put a variable (either 1 or 2) bit k field at the start of the mask.
> Similar to the current scheme, if the first bit was a '1' it would mean it
> was only a 15 bit mask and the second bit would not be used to represent
> the mask size,  If the first bit was a '0', then the next bit will also be
> used to determine the mask size. I.e.:
> 1 -> 15 bit mask (only one bit spent)
> 01 -> 46 bit mask
> 00 -> 110 bit mask
>

> -brian
> _______________________________________________
> payload mailing list
> [email-masked]
> https://www.ietf.org/mailman/listinfo/payload
>

---

From: "Mo Zanaty (mzanaty)" <[email-masked]>
Date: Thu, 7 Dec 2017 22:21:35 +0000

Hi Rasmus,

Thank you for the review and comments. We are updating the draft to address your comments as noted below (see Mo: inline).

Thanks,
Mo

From: payload <[email-masked]<mailto:[email-masked]>> on behalf of Rasmus Brandt <[email-masked]<mailto:[email-masked]>>
Date: Thursday, December 7, 2017 at 4:48 AM
To: "[email-masked]<mailto:[email-masked]>" <[email-masked]<mailto:[email-masked]>>
Subject: Re: [payload] FlexFEC draft feedback

---------- Forwarded message ----------
From: Brian Baldino <[email-masked]<mailto:[email-masked]>>
To: [email-masked]<mailto:[email-masked]>
Cc:
Bcc:
Date: Wed, 29 Nov 2017 12:18:18 -0800
Subject: [payload] FlexFEC draft feedback
I've been implementing flexfec on the jitsi videobridge (starting with just the receive side for now) and came across a couple things.  Also Boris Grozev has compiled some notes from his readings of the draft; I've tried to summarize all our thoughts here:

1) It would be nice if the draft mentioned that the header extensions of the FEC packets are totally independent of the media's header extensions, and that one can add extensions to FEC packets without having any effect on the recovery.

2) It would be nice if the draft explicitly stated that RTP header extensions (and anything in the header beyond the 12 byte fixed header, really) is protected as part of the payload.

3) I don't think it's explicitly mentioned that the first bit of the mask represents a delta value of '0' (and, therefore, a '1' in that bit means that the base sequence number itself is protected), it would be nice to call this out [in Section 6.3.1.2 maybe?]

This is alluded to in Section 3.2<https://tools.ietf.org/html/draft-ietf-payload-flexible-fec-scheme-05#section-3.2>:

bitmask: Run-length encoding of packets protected by a FEC packet.
         If the bit i in the mask is set to 1, the source packet number N + i
         is protected by this FEC packet.  Here, N is the sequence number
         base, which is indicated in the FEC packet as well.

and in Section 4.2<https://tools.ietf.org/html/draft-ietf-payload-flexible-fec-scheme-03#section-4.2>:

The SN base_i (16 bits) field indicates the lowest sequence
number, taking wrap around into account, of the source packets for
a particular SSSRC (indicated in SSRC_i) protected by this repair packet.

The draft is however not internally consistent, because in Section 4.2 it also says:

with sequence number (SN base_i + j + 1) is protected by this FEC
packet.

When using the offset-based masks, the base sequence number is also inclusive:

To make the descriptions consistent, I think (SN base_i + j + 1) should be replaced by (SN base_i + j) in Section 4.2.

Mo: I can see confusion here if j is zero-based so +j+1 works (which was the intent), or if j is one-based so +j works (which was not the intent). We will clarify this in 3.2, 4.2, 6.2.

Mo: A question to implementors: Do you see any value in signaling whether the SN_base itself is protected or not, rather than always assuming / requiring it to be protected? That is, should the lowest bit in the mask map to SN_base or SN_base+1?

4) In some of the diagrams there is a typo making it look like the 'P' field is 2 bits instead of 1 (Figures 10, 12, 13, and 15)

4) From an implementation perspective, I found the extraction of the mask to be a bit of a pain due to having to extract the k bits and shift the actual mask to get the offsets.  I wondered if there was a reason not to move the second k bit next to the first to make them consecutive, followed by a consecutive mask.  I thought of two ways this might be done:

a) Put a 2 bit k field at the start of the mask.  There are multiple ways to define an encoding for those 2 bits to represent a 14, 46 or 110 mask.  This has the downside of making the first mask 1 bit smaller (14 bits vs 15) but does allow for encoding a 4th mask length, if so desired.

b) Put a variable (either 1 or 2) bit k field at the start of the mask.  Similar to the current scheme, if the first bit was a '1' it would mean it was only a 15 bit mask and the second bit would not be used to represent the mask size,  If the first bit was a '0', then the next bit will also be used to determine the mask size. I.e.:
1 -> 15 bit mask (only one bit spent)
01 -> 46 bit mask
00 -> 110 bit mask

-brian
_______________________________________________
payload mailing list
[email-masked]<mailto:[email-masked]>
https://www.ietf.org/mailman/listinfo/payload

---

From: Brian Baldino <[email-masked]>
Date: Thu, 7 Dec 2017 14:42:57 -0800

>
> Mo: A question to implementors: Do you see any value in signaling whether
> the SN_base itself is protected or not, rather than always assuming /
> requiring it to be protected? That is, should the lowest bit in the mask
> map to SN_base or SN_base+1?

Personally I'd prefer the first bit map to SN_base, so determining all of
the packets which are protected is calculated in the same way (as opposed
to the special case of adding SN_base to the protected list, and then
getting the rest from the mask).

On Thu, Dec 7, 2017 at 2:21 PM, Mo Zanaty (mzanaty) <[email-masked]>
wrote:

> Hi Rasmus,
>
> Thank you for the review and comments. We are updating the draft to
> address your comments as noted below (see Mo: inline).
>
> Thanks,
> Mo
>
> From: payload <[email-masked]> on behalf of Rasmus Brandt <
> [email-masked]>
> To: "[email-masked]" <[email-masked]>
> Mo: I can see confusion here if j is zero-based so +j+1 works (which was
> the intent), or if j is one-based so +j works (which was not the intent).
> We will clarify this in 3.2, 4.2, 6.2.
>
> Mo: A question to implementors: Do you see any value in signaling whether
> the SN_base itself is protected or not, rather than always assuming /
> requiring it to be protected? That is, should the lowest bit in the mask
> map to SN_base or SN_base+1?
>
>

---

From: Rasmus Brandt <[email-masked]>
Date: Fri, 08 Dec 2017 08:45:09 +0000

On Thu, Dec 7, 2017 at 11:42 PM Brian Baldino <[email-masked]> wrote:

> getting the rest from the mask).

I agree with Brian on this. That would also be consistent with how the
packet masks are treated in ULPFEC
<https://tools.ietf.org/html/rfc5109#section-7.4>:

   The mask field in the FEC level header indicates which packets are
   associated with the FEC packet at the current level.  It is either 16
   or 48 bits depending on the value of the L bit.  If bit i in the mask
   is set to 1, then the media packet with sequence number N + i is
   associated with this FEC packet, where N is the SN Base field in the
   FEC packet header.  The most significant bit of the mask corresponds
   to i=0, and the least significant to i=15 when the L bit is set to 0,

   or i=47 when the L bit is set to 1.

>>> 3) I don't think it's explicitly mentioned that the first bit of the
>>> mask represents a delta value of '0' (and, therefore, a '1' in that bit
>>> means that the base sequence number itself is protected), it would be nice
>>> to call this out [in Section 6.3.1.2 maybe?]
>>>
>>
>>> 4) From an implementation perspective, I found the extraction of the
>>> mask to be a bit of a pain due to having to extract the k bits and shift
>>> the actual mask to get the offsets.  I wondered if there was a reason not
>>> to move the second k bit next to the first to make them consecutive,
>>> followed by a consecutive mask.  I thought of two ways this might be done:
>>>
>>> a) Put a 2 bit k field at the start of the mask.  There are multiple
>>> ways to define an encoding for those 2 bits to represent a 14, 46 or 110
>>> mask.  This has the downside of making the first mask 1 bit smaller (14
>>> bits vs 15) but does allow for encoding a 4th mask length, if so desired.
>>>