Skip to content

Conversation

@oto313
Copy link

@oto313 oto313 commented Oct 29, 2024

Fix for wrong sequence numbers

Currently the nack responder (responder_interceptor.go) generates RTX RTP packet which has not right sequence numbers. It does not increment the sequence number by one according to RFC rfc4588

For either multiplexing scheme, the sequence number has the standard definition; i.e., it MUST be one higher than the sequence number of the preceding packet sent in the retransmission stream.

I am proposing fix for it and appreciate any feedback as this is my first pull request in GO lang.

@Sean-Der
Copy link
Member

Nice find @oto313! How did you find this, I am really impressed! If you run go test -v locally you have some build/test failures.

  • Maybe we make this part of Get? Auto-increments (if needed)
  • We should write some tests that it does the auto increment as expected

@oto313
Copy link
Author

oto313 commented Oct 30, 2024

Hi @Sean-Der. Thanks. I was investigating why retransmission not worked in my use case and I was investigating it with wireshark and I compared retransmission packets with google webrtc implementation.

If you run go test -v locally you have some build/test failures.

Sorry the build/tests are fixed

Maybe we make this part of Get? Auto-increments (if needed)

part of which Get?

@oto313
Copy link
Author

oto313 commented Mar 18, 2025

Hi @Sean-Der, so now i fixed the tests. Hopefully now we can merge it. Thanks

// Rewrite the payload type.
p.header.PayloadType = rtxPayloadType
// Rewrite the sequence number.
p.header.SequenceNumber = m.rtxSequencer.NextSequenceNumber()
Copy link
Member

Choose a reason for hiding this comment

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

m.rtxSequencer seems unused now, maybe delete it?

@aalekseevx aalekseevx linked an issue May 14, 2025 that may be closed by this pull request
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

RTX seqno are not sequential

3 participants