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

Fix handling of "data" stream parameters in the streaming plugin #3412

Merged
merged 5 commits into from
Sep 4, 2024

Conversation

atoppi
Copy link
Member

@atoppi atoppi commented Jul 25, 2024

Attempt to fix #3411

@atoppi atoppi requested a review from lminiero July 25, 2024 12:33
…nditions to drop packets in relay_rtp_packet
@atoppi atoppi marked this pull request as draft July 26, 2024 08:24
@atoppi
Copy link
Member Author

atoppi commented Jul 26, 2024

Converted the PR into draft since the changes have become more error prone and I'd like to have a review first.

In the current status the sending of the buffered message happens in the data_ready callback. In the callback we iterate the session, looking for streams that have the buffermsg flag set to true.
I'm not sure if that callback might be issued multiple times, in particular when multistream and renegotiation come into play.
Also the condition to drop packets in relay_rtp_packet has been changed to let early data packets come through.

@atoppi atoppi marked this pull request as ready for review July 26, 2024 20:10
@atoppi
Copy link
Member Author

atoppi commented Jul 26, 2024

Updated the patch in order to send a buffered message only the first time data_ready is being called.
I'm still not sure of potential side-effects / bad behaviors in case of renegotiation or restart.

@lminiero
Copy link
Member

I'm still not sure of potential side-effects / bad behaviors in case of renegotiation or restart.

The data_ready callback is sent any time the buffers in the SCTP stack empty, and so it's safe to send again. That's why we also use it as a trigger for when we can start sending the first time. This has nothing to do with renegotiations or restart, since it has to do with the SCTP stack, and using the dataready volatile int should be safe since it's only reset when hangup_media hits.

@atoppi
Copy link
Member Author

atoppi commented Sep 4, 2024

I have reverted a condition that was meaningless.
Merging since it seems to work.

@atoppi atoppi merged commit 212e4fc into master Sep 4, 2024
8 checks passed
@atoppi atoppi deleted the fix-streaming-data-params branch September 13, 2024 13:23
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.

[1.x] databuffermsg/buffermsg not working for streaming plugin
2 participants