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

Update oldestRtpBaseSequenceNumber when advance the queue state #72

Merged
merged 1 commit into from
Nov 1, 2022

Conversation

happyharryh
Copy link
Contributor

@happyharryh happyharryh commented Nov 1, 2022

I met a strange bug related to the RtpAudioQueue recently, when I was using moonlight-qt in a network with very low quality. When some FEC blocks were lost, we decided to give up waiting them, and advanced the queue state in handleMissingPackets. Here, we only updated queue->nextRtpSequenceNumber, but not queue->oldestRtpBaseSequenceNumber. Then, some of the lost blocks (packets) came accidentally. This time, getFecBlockForRtpPacket allocated a new block for the packet, and placed it at the head of the queue, since its sequence number was before those of existed blocks. After that, the behavior of freeFecBlockHead became unexpected. It freed the oldest block (the head of the queue) but not the complete block, so that the later one was still in the queue. As its nextDataPacketIndex had already been up to RTPA_DATA_SHARDS, it would cause crash when being accessed in RtpaGetQueuedPacket.

For an example, the sequenceNumber order of the received packets is:

    ..., 1000, 1001, 1002, 1003, 1060, 1061, 1004, 1005, 1006, 1062, 1063, 1064, 1065, ...

When 1060 came, handleMissingPackets advanced the queue state without updating queue->oldestRtpBaseSequenceNumber. When 1004 came, getFecBlockForRtpPacket allocated a new block (base 1004) for it at the head of the queue. When 1063 came, the block base 1060 was complete. freeFecBlockHead freed the block of base 1004 but not the block of base 1060. The later one was still in the queue, and caused crash when being accessed later.

The way to fix it is simple, that is, updating queue->oldestRtpBaseSequenceNumber when advance the queue state, preventing getFecBlockForRtpPacket from allocating new blocks for the outdated packets.

@cgutman cgutman merged commit 7d9df5b into moonlight-stream:master Nov 1, 2022
@cgutman
Copy link
Member

cgutman commented Nov 1, 2022

Nice work. Thanks for the fix.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants