[core] Both readData() consider msgttl

[gou4shi1]

I found that in message mode, messages are often sent after TTL due to congestion control.

[gou4shi1]

related to #2064, if you need the full log, I can prepare one for you :)

[maxsharabayko]

So the proposal is to not allow to drop a message on the sender side if TTL is provided and is not yet expired?

I think the proposal is not quite correct.
checkNeedDrop(..) checks the whole SND buffer for too old messages to drop. using the buffer timespan.
The TTL of the currently submitted message must be specific to that message, not the whole buffer.

[ethouris]

Whether the packet is going to be dropped on the sender side, this is currently controlled by SRTO_SNDDROPDELAY option. It allows also to disable sender drops at all.

[gou4shi1]

So the proposal is to not allow to drop a message on the sender side if TTL is provided and is not yet expired?

This PR only add a new place to drop msg.

The TTL of the currently submitted message must be specific to that message, not the whole buffer.

You are right, this PR is not correct if one socket meet different TTL.

Whether the packet is going to be dropped on the sender side, this is currently controlled by SRTO_SNDDROPDELAY option. It allows also to disable sender drops at all.

Not totally, CUDT::packLostData -> CSndBuffer::readData() will consider msgttl in message mode (without SRTO_SNDDROPDELAY)

[gou4shi1]

The problem I want to solve is that, I have specify my message with TTL=200ms, but the congestion control send it after 1s or more (the network is bad at that time), as a result, there are more and more expired messages stuck in send buffer and sent after TTL.
With this PR, it will drop expired messages and try to send the newly committed messages more quickly.

[maxsharabayko]

The TTL value you set on the message is stored in the sender buffer. The value is checked once a packet is read from the buffer.
Please see here in the CSndBuffer::readData(const int offset, ..).

if ((p->m_iTTL >= 0) && (count_milliseconds(steady_clock::now() - p->m_tsOriginTime) > p->m_iTTL))

However, this check is only applied on a packet to be retransmitted. The function CSndBuffer::readData(srt::CPacket& ...) which gets the original packet (to be sent for the first time) does not check TTL.
I am not 100% sure if it should, but might actually make sense in case of congestion. So, 91.5% sure it should :)

I would recommend adding the TTL check to CSndBuffer::readData(srt::CPacket& ...). The check of TTL and drop (if ((p->m_iTTL >= 0)) can be moved to a separate function then, to be used in both readData(..) functions.

[gou4shi1]

@maxsharabayko Thanks for your suggestion, however, I meet some problems when trying to implement you proposal:

Question 1: Dropping in checkNeedDrop() and readData(const int offset, ..) (the retrans one) do fack ack. If we also drop in readData(srt::CPacket& ...) (the fresh one), can we also do fack ack? If not so, getFlightSpan() need to consider dropped fresh packets, right?

Question 2: what is the difference between scheduling sequence and extraction sequence? Why we need to overwrite extraction sequence over scheduling sequence after readData(srt::CPacket& ...)? If we also drop in readData(srt::CPacket& ...), how to handle these two seq?

[maxsharabayko]

@gou4shi1 Right, it turned out a bit trickier. Here is a patch of roughly how I would expect it to work.
ttl-drop-patch.zip

The function CSndBuffer::readData(srt::CPacket ...) should read a packet and update the current position in the SND buffer, then check TTL, and if it is too late, try to read the next packet instead. But also CUDT has to be aware that some packets were skipped because it needs to increment the current SND sequence number accordingly. For that purpose, w_seqnoinc is used.

CSndBuffer::readData. Click to expand/collapse

int CSndBuffer::readData(srt::CPacket& w_packet, steady_clock::time_point& w_srctime, int kflgs, int& w_seqnoinc)
{
    int readlen = 0;
    w_seqnoinc = 0;

    while (m_pCurrBlock != m_pLastBlock)
    {
        // Make the packet REFLECT the data stored in the buffer.
        w_packet.m_pcData = m_pCurrBlock->m_pcData;
        readlen = m_pCurrBlock->m_iLength;
        w_packet.setLength(readlen);
        w_packet.m_iSeqNo = m_pCurrBlock->m_iSeqNo;

        if (kflgs == -1)
        {
            readlen = 0;
        }
        else
        {
            m_pCurrBlock->m_iMsgNoBitset |= MSGNO_ENCKEYSPEC::wrap(kflgs);
        }

        Block* p = m_pCurrBlock;
        w_packet.m_iMsgNo = m_pCurrBlock->m_iMsgNoBitset;
        w_srctime = getSourceTime(*m_pCurrBlock);
        m_pCurrBlock = m_pCurrBlock->m_pNext;

        if ((p->m_iTTL >= 0) && (count_milliseconds(steady_clock::now() - p->m_tsOriginTime) > p->m_iTTL))
        {
            // Skip this packet due to TTL expiry.
            readlen = 0;
            ++w_seqnoinc;
            continue;
        }

        HLOGC(bslog.Debug, log << CONID() << "CSndBuffer: extracting packet size=" << readlen << " to send");
        break;
    }

    return readlen;
}

Changes to CUDT::packData(CPacket& w_packet):

    // ...
            kflg    = m_pCryptoControl->getSndCryptoFlags();
+           int pktskipseqno = 0;
            payload = m_pSndBuffer->readData((w_packet), (origintime), kflg, pktskipseqno);
+           if (pktskipseqno)
+           {
+               // Some packets were skipped due to TTL expiry.
+               m_iSndCurrSeqNo = CSeqNo::incseq(m_iSndCurrSeqNo, pktskipseqno);
+           }

In this case, sender would skip the message, then the receiver would notice that send NAK report, and this time the sender would send the message drop request.
It is worth experimenting with sending the message drop request directly, but it might be a bit tricky on the sender side, and I am not 100% sure in the receiver buffer handling it properly. Still worth checking.

[maxsharabayko]

Question 1: Dropping in checkNeedDrop() and readData(const int offset, ..) (the retrans one) do fack ack. If we also drop in readData(srt::CPacket& ...) (the fresh one), can we also do fack ack? If not so, getFlightSpan() need to consider dropped fresh packets, right?

The solution above also notifies the receiver about this drop. So, yes.
P.S. By "fack" you mean "fake ACK", right?

Question 2: what is the difference between scheduling sequence and extraction sequence? Why we need to overwrite extraction sequence over scheduling sequence after readData(srt::CPacket& ...)? If we also drop in readData(srt::CPacket& ...), how to handle these two seq?

The main reason is the logic of the SND buffer. It is expected to work as a FIFO for original packets. If you would just drop a packet before adding it to the buffer, you would know about the drop on the sender side, but no way to communicate this drop to the receiver. The receiver must see a gap in packet sequence to properly handle this situation.

[gou4shi1]

Here is a patch of roughly how I would expect it to work.

Great! I would have a test ASAP.

P.S. By "fack" you mean "fake ACK", right?

Oops, my typo, lol...

[gou4shi1]

I would have a test ASAP.

test result looks fine :)

[maxsharabayko]

TODO:

• Fix msgttl description in API-functions.md.

msgttl: [IN]. In message and live mode only, specifies the TTL for
sending messages (in [ms]). Not used for receiving messages. If this value
is not negative, it defines the maximum time up to which this message should
stay scheduled for sending for the sake of later retransmission. A message
is always sent for the first time, but the UDP packet carrying it may be
(also partially) lost, and if so, lacking packets will be retransmitted. If
the message is not successfully resent before TTL expires, further retransmission
is given up and the message is discarded.

[maxsharabayko]

An update on this PR.
Changes in SRT behavior have been approved at an internal meeting.
This PR will go together with #2185 (not earlier than after November 22), which also adjusts timestamp usage by the sender.