Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[core] Fixed CRcvBuffer::dropMessage. #2657

Merged
merged 5 commits into from
Feb 10, 2023

Conversation

maxsharabayko
Copy link
Collaborator

@maxsharabayko maxsharabayko commented Feb 9, 2023

Fixed CRcvBuffer::dropMessage(..).

  • Tell what to do with existing packets.
  • Fixed pkt seqno dropping loop range.

The function is used in two situations:

  1. Incoming message drop request from the sender.
  2. Dropping a packet with decryption failure.

If msgno is valid, sender has requested to drop the whole message by TTL. In this case it has to also provide a pkt seqno range.
However, if a message has been partially acknowledged and already removed from the SND buffer,
the seqnolo might specify some position in the middle of the message, not the very first packet.
If those packets have been acknowledged, they must exist in the receiver buffer unless already read.
In this case the msgno should be used to determine starting packets of the message.
Some packets of the message can be missing on the receiver, therefore the actual drop should still be performed by pkt seqno range.
If message number is 0 or SRT_MSGNO_NONE, then use sequence numbers to locate sequence range to drop [seqnolo, seqnohi].
A SOLO message packet can be kept depending on actionOnExisting value.
This is done to avoid dropping existing packet when the sender was asked to re-transmit a packet from an outdated loss report, which is already not available in the SND buffer.

Dropping a packet with decryption failure means the existing packet must be dropped.
Dropping a packet by incoming message drop request means the existing packet must be kept, only missing packets or partially missing messages must be dropped.

TODO:

  • Fix drop by msgno.

@maxsharabayko maxsharabayko added Type: Bug Indicates an unexpected problem or unintended behavior [core] Area: Changes in SRT library core labels Feb 9, 2023
@maxsharabayko maxsharabayko added this to the v1.5.2 milestone Feb 9, 2023
@maxsharabayko maxsharabayko marked this pull request as ready for review February 10, 2023 10:37
srtcore/buffer_rcv.cpp Outdated Show resolved Hide resolved
srtcore/buffer_rcv.cpp Outdated Show resolved Hide resolved
if (!m_entries[i].pUnit)
continue;

// TODO: Break the loop if a massege has been found. No need to search further.
const int32_t msgseq = packetAt(i).getMsgSeq(m_bPeerRexmitFlag);
if (msgseq == msgno)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that message numbers are also increasing with rollover. It might be a good idea to check if the message number from the current packet is already PAST the searched message number. If so, there's no point in continuing with the loop because remaining packets in the buffer won't contain this message.

@maxsharabayko
Copy link
Collaborator Author

The tricky situation is to drop a message that is ahead of the current RCV buffer position.

msgno 1:
| 1 | 2 | 3 | 4 | 5 |

in RCV buffer:
| 1 | x | 3 | 4 |

MSG drop request
msgno = 1
seqnolo = 1
seqnohi = 5

After the drop the RCV buffer should be:

| d | d | d | d | d |

srtcore/buffer_rcv.cpp Outdated Show resolved Hide resolved
@maxsharabayko maxsharabayko merged commit 3ffc93f into Haivision:master Feb 10, 2023
@maxsharabayko maxsharabayko deleted the hotfix/rcv-dropmsg branch February 10, 2023 16:28
@@ -310,15 +309,21 @@ int CRcvBuffer::dropMessage(int32_t seqnolo, int32_t seqnohi, int32_t msgno)
return 0;
Copy link
Contributor

@gou4shi1 gou4shi1 Feb 11, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • should return iDropCnt?
  • should not skip this part?
    const bool needUpdateNonreadPos = (minDroppedOffset != -1 && minDroppedOffset <= getRcvDataSize());
    releaseNextFillerEntries();
    if (needUpdateNonreadPos)
    {
        m_iFirstNonreadPos = m_iStartPos;
        updateNonreadPos();
    }
    if (!m_tsbpd.isEnabled() && m_bMessageAPI)
    {
        if (!checkFirstReadableOutOfOrder())
            m_iFirstReadableOutOfOrder = -1;
        updateFirstReadableOutOfOrder();
    }

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, good catch!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[core] Area: Changes in SRT library core Priority: High Type: Bug Indicates an unexpected problem or unintended behavior
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants