Skip to content

Implement NACK+RTX support defined by RFC4588 #45

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

Open
wants to merge 24 commits into
base: workflows
Choose a base branch
from

Conversation

murillo128
Copy link

@murillo128 murillo128 commented Jun 10, 2025

RTP retransmission described in RFC4588 (RTX) is an effective packet loss recovery technique for real-time applications with relaxed delay bounds.

This PR provides a minimal implementation for RTX and RTCP NACK (RFC3940) and its associated SDP signaling and negotiation.

@winlinvip
Copy link
Member

Hi Sergio, thank you for your PR. Could you please provide some background information for this PR in the description? The patch will be generated based on the title and description, and it will be sent via email to the FFmpeg community. Therefore, a detailed description is important for the maintainers to review this PR.

@murillo128
Copy link
Author

RTP retransmission described in RFC4588 (RTX) is an effective packet loss recovery technique for real-time applications with relaxed delay bounds.

This PR provides a minimal implementation for RTX and RTCP NACK (RFC3940) and its associated SDP signaling and negotiation.

@winlinvip winlinvip force-pushed the workflows branch 3 times, most recently from 3294e09 to 4aa17ba Compare June 10, 2025 22:58
@JackLau1222 JackLau1222 force-pushed the nack-rtx branch 4 times, most recently from ec1b1bf to 809f0a5 Compare June 14, 2025 12:33
@JackLau1222
Copy link
Collaborator

Good news

This patch has been tested with Pion and Janus, and it works well.

However, this patch need to depend on some other patch(not in FFmepg master for now) otherwise you might run into problems, so I put these patches here:

In the coming days, I'll add pion and janus tests into workflow, improve this patch, then submit this patch to FFmpeg developers mail list

@winlinvip winlinvip changed the title Implement NACK+RTX suppport Implement NACK+RTX support following RFC4588 Jun 27, 2025
@winlinvip winlinvip changed the title Implement NACK+RTX support following RFC4588 Implement NACK+RTX support defined by RFC4588 Jun 27, 2025
@ghost
Copy link

ghost commented Jul 1, 2025

Since my account @JackLau1222 has been suspended by Github, I created this new account to take over the work

Good news

This patch has been tested with Pion and Janus, and it works well.

However, this patch need to depend on some other patch(not in FFmepg master for now) otherwise you might run into problems, so I put these patches here:

#24

#54

#55

In the coming days, I'll add pion and janus tests into workflow, improve this patch, then submit this patch to FFmpeg developers mail list

these PR link can't be opened because the submitter account was suspended, So I'll open new PRs using this temporary account, and then update this comment

@ghost ghost force-pushed the workflows branch from a5e591f to 5a992b3 Compare July 1, 2025 12:45
murillo128 and others added 5 commits July 1, 2025 21:12
decrypt the SRTCP pakcet,  get the right pid and blp data
add more comments

Signed-off-by: Jack Lau <[email protected]>
Signed-off-by: Jack Lau <[email protected]>
unique ssrc need to use unique srtp context,
otherwise the server couldn't decrypted the rtx
packet.

move the "a=ssrc-group:FID" on top of SSRCs

Signed-off-by: Jack Lau <[email protected]>
whip->buf size is 4096 rather than srtcp_len, srtcp data just
use part of its memory, handle it need more logic.

So allocate a temporary pkt to carry the SRTCP data

Signed-off-by: Jack Lau <[email protected]>
need to be changed AV_LOG_DEBUG before official commit

Signed-off-by: Jack Lau <[email protected]>
@ghost ghost force-pushed the nack-rtx branch from 00a55f4 to fecd3dd Compare July 1, 2025 13:13
@ghost ghost mentioned this pull request Jul 1, 2025
@JackLau1222 JackLau1222 force-pushed the workflows branch 2 times, most recently from 1c20a98 to 487998b Compare July 22, 2025 06:19
@JackLau1222 JackLau1222 force-pushed the workflows branch 2 times, most recently from 2c40a5c to 80db6eb Compare August 6, 2025 11:35
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.

3 participants