[core] Avoid reporting packets rebuilt by FEC as lost.

[disigma]

For rebuilt packets, * FEC * may still report it as a lost packet. This happens in row-only mode.

The following transmission is configured as cols:10,rows:1,layout:even,arq:onreq:

UDT type: data seqno: 90 msgno: 91
UDT type: ack  seqno: 90
UDT type: ack2
UDT type: data seqno: 91 msgno: 92
UDT type: ack  seqno: 92
UDT type: ack2
UDT type: data seqno: 92 msgno: 93
UDT type: data seqno: 93 msgno: 94
UDT type: ack  seqno: 93
UDT type: ack2
UDT type: ack  seqno: 94
UDT type: ack2
UDT type: data seqno: 94 msgno: 95
UDT type: data seqno: 95 msgno: 96
UDT type: data seqno: 96 msgno: 97
UDT type: ack  seqno: 97
UDT type: ack2
UDT type: data seqno: 98 msgno: 99
UDT type: data seqno: 99 msgno: 100
UDT type: data seqno: 99 msgno: 0
UDT type: ack  seqno: 100
UDT type: ack2
UDT type: data seqno: 100 msgno: 101
UDT type: data seqno: 101 msgno: 102
UDT type: data seqno: 102 msgno: 103
UDT type: ack  seqno: 101
UDT type: ack2
UDT type: data seqno: 103 msgno: 104
UDT type: ack  seqno: 104
UDT type: data seqno: 105 msgno: 106
UDT type: data seqno: 106 msgno: 107
UDT type: data seqno: 107 msgno: 108
UDT type: data seqno: 109 msgno: 110
UDT type: data seqno: 109 msgno: 0
UDT type: ack  seqno: 104
UDT type: ack2
UDT type: data seqno: 110 msgno: 111
UDT type: data seqno: 111 msgno: 112
UDT type: nak missing:97
UDT type: data seqno: 112 msgno: 113
UDT type: data seqno: 113 msgno: 114

The packet seqno == 97 is lost, but the same row packets and the control packet is received successfully, so it can be rebuilt. It was indeed successfully rebuilt because of the next ack seqno is 100.

The problem is: when rebuilt with rows == 1, the cell is not marked as received, because of the following codes:

// https://github.com/Haivision/srt/blob/v1.4.1/srtcore/fec.cpp#L1340
// void FECFilterBuiltin::RcvRebuild(Group& g, int32_t seqno, Group::Type tp)

// If this is a single request (filled from row and m_number_cols == 1),
// do not attempt recursive rebuilding
if (tp == Group::SINGLE)
    return;

// Mark this packet received
MarkCellReceived(seqno);

[maxsharabayko]

Hi @disigma. Thank you for your contribution.
Do you think this case can be covered by a unit test?

[disigma]

Hi @disigma. Thank you for your contribution.
Do you think this case can be covered by a unit test?

I have no idea about how to add unit tests in this case.

Another problem I've found just now, for the following code:

rcv.order_required = rpkt.getMsgOrderFlag();

This assumes the order flag will keep unchanged, but the order flag of the FEC control packet will be always zero and may be different from the other packets.

[ethouris]

Another problem I've found just now, for the following code:

rcv.order_required = rpkt.getMsgOrderFlag();

This assumes the order flag will keep unchanged, but the order flag of the FEC control packet will be always zero and may be different from the other packets.

In Live mode the the packet flags are interpreted differently than in the message mode (there's a different function used for extraction), and FEC is only predicted to be used in the live mode. This flag is currently not used in live mode, although there is confusion as to what value it should normally have. It is also unlikely to be reused together with FEC in the future because FEC requires that the conditional dropping be allowed, and when it is allowed, then there's no theoretical situation of delivering later received packets before earlier lost packets with waiting for the latter to be possibly delivered later - they are simply dropped.

[ethouris]

Ok, I've analyzed the code a bit and yes, both changes are required.

Initially the MarkCellReceived was only required for the further recursive rebuilding, which didn't happen in case of 1 row configuration. But then it was also used to report lost packets for onreq.