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.

Sign up for GitHub

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

Jump to bottom

Refactor RTP retransmission buffer in a separate and testable RTC::RetransmissionBuffer class #1023

Merged
merged 19 commits into from
Mar 20, 2023
Merged

Refactor RTP retransmission buffer in a separate and testable RTC::RetransmissionBuffer class #1023

merged 19 commits into from
Mar 20, 2023

Conversation

ibc
Copy link
Member

@ibc ibc commented Mar 13, 2023 •
edited
Loading

A complete refactor of the retransmission buffer which now is a separate RetransmissionBuffer class.

@ibc
Fix crash in RtpStreamSend due to this->buffer.size() > MaxSeq
4ca2dae 
- Fixes #975
- For now just some logs added to diagnose the problem.
@ibc ibc requested review from jmillan and nazar-pc March 13, 2023 13:29
@ibc ibc mentioned this pull request Mar 13, 2023
Closed
@nazar-pc nazar-pc mentioned this pull request Mar 16, 2023
Closed
@ibc ibc changed the title Fix crash in RtpStreamSend due to this->buffer.size() > MaxSeq Refactor retransmission buffer Mar 17, 2023
@versatica versatica deleted a comment from jmillan Mar 17, 2023
@versatica versatica deleted a comment from jmillan Mar 17, 2023
@versatica versatica deleted a comment from jmillan Mar 17, 2023
@versatica versatica deleted a comment from jmillan Mar 17, 2023
@versatica versatica deleted a comment from jmillan Mar 17, 2023
@ibc ibc marked this pull request as ready for review March 20, 2023 11:10
ibc
ibc commented Mar 20, 2023
View reviewed changes
worker/include/RTC/RtpStream.hpp Outdated
Copy link
Member Author

Choose a reason for hiding this comment

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

Cosmetic changes in RtpStream.hpp. Let's avoid those long multi-line comments at the right side of class members.

ibc
ibc commented Mar 20, 2023
View reviewed changes
worker/include/RTC/RtpStreamRecv.hpp Outdated
Copy link
Member Author

@ibc ibc Mar 20, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Same. Cosmetic changes in RtpStreamRecv.hpp. Let's avoid those long multi-line comments at the right side of class members.

ibc
ibc commented Mar 20, 2023
View reviewed changes
worker/include/common.hpp Outdated
Copy link
Member Author

Choose a reason for hiding this comment

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

Cosmetic changes in comments. Ignore.

ibc
ibc commented Mar 20, 2023
View reviewed changes
worker/src/RTC/RetransmissionBuffer.cpp
Copy link
Member Author

Choose a reason for hiding this comment

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

A separate class.

ibc
ibc commented Mar 20, 2023
View reviewed changes
worker/src/RTC/RtpStreamRecv.cpp Outdated
Copy link
Member Author

Choose a reason for hiding this comment

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

Cosmetic. Ignore.

ibc
ibc commented Mar 20, 2023
View reviewed changes
worker/src/handles/Timer.cpp Outdated
Copy link
Member Author

Choose a reason for hiding this comment

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

Cosmetic. Ignore.

@ibc
Copy link
Member Author

ibc commented Mar 20, 2023

@jmillan @nazar-pc this is ready for review.

ibc
ibc commented Mar 20, 2023
View reviewed changes
worker/src/RTC/RtpStreamSend.cpp
Comment on lines +14 to +15
// Limit retransmission buffer max size to 2500 items.
static constexpr size_t RetransmissionBufferMaxItems{ 2500u };
Copy link
Member Author

Choose a reason for hiding this comment

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

This is the retransmission buffer max size (passed to RetransmissionBuffer class as argument in the constructor. In my tests this is good enough.

@ibc
Copy link
Member Author

ibc commented Mar 20, 2023

TestRetransmissionBuffer.cpp added!

jmillan
jmillan approved these changes Mar 20, 2023
View reviewed changes
Copy link
Member

@jmillan jmillan left a comment

Choose a reason for hiding this comment

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

Nice refactor 👍

worker/src/RTC/RetransmissionBuffer.cpp Outdated Show resolved Hide resolved
@ibc ibc changed the title Refactor retransmission buffer Refactor RTP retransmission buffer in a separate and testable RTC::RetransmissionBuffer class Mar 20, 2023
@ibc ibc merged commit a6857b6 into v3 Mar 20, 2023
@ibc ibc deleted the fix-issue-975 branch March 20, 2023 16:18
@vpalmisano
Copy link
Contributor

After upgrading to this version, we received a SIGABRT, with this stack trace:

#0  __GI_raise (sig=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:50
#1  0x0000ffffabab7aa0 in __GI_abort () at abort.c:79
#2  0x0000ffffabdad238 in __gnu_cxx::__verbose_terminate_handler() () from /usr/lib/aarch64-linux-gnu/libstdc++.so.6
#3  0x0000ffffabdaad4c in ?? () from /usr/lib/aarch64-linux-gnu/libstdc++.so.6
#4  0x0000ffffabdaadb0 in std::terminate() () from /usr/lib/aarch64-linux-gnu/libstdc++.so.6
#5  0x0000ffffabdab0a4 in __cxa_throw () from /usr/lib/aarch64-linux-gnu/libstdc++.so.6
#6  0x0000ffffabdd3950 in std::__throw_out_of_range_fmt(char const*, ...) () from /usr/lib/aarch64-linux-gnu/libstdc++.so.6
#7  0x0000aaaabc66aa3c in RTC::RetransmissionBuffer::Insert(RTC::RtpPacket*, std::shared_ptr<RTC::RtpPacket>&) ()
#8  0x0000aaaabc68aadc in RTC::RtpStreamSend::ReceivePacket(RTC::RtpPacket*, std::shared_ptr<RTC::RtpPacket>&) ()
#9  0x0000aaaabc6afca4 in RTC::SvcConsumer::SendRtpPacket(RTC::RtpPacket*, std::shared_ptr<RTC::RtpPacket>&) ()
#10 0x0000aaaabc676384 in RTC::Router::OnTransportProducerRtpPacketReceived(RTC::Transport*, RTC::Producer*, RTC::RtpPacket*) ()
#11 0x0000aaaabc663e7c in RTC::Producer::ReceiveRtpPacket(RTC::RtpPacket*) ()
#12 0x0000aaaabc6b4b18 in RTC::Transport::ReceiveRtpPacket(RTC::RtpPacket*) ()
#13 0x0000aaaabc64b0d8 in RTC::PipeTransport::OnUdpSocketPacketReceived(RTC::UdpSocket*, unsigned char const*, unsigned long, sockaddr const*) ()
#14 0x0000aaaabc96cc34 in uv.udp_recvmmsg ()
#15 0x0000aaaabc96d8c4 in uv.udp_io ()
#16 0x0000aaaabc970cc4 in uv.io_poll ()
#17 0x0000aaaabc9646a4 in uv_run ()
#18 0x0000aaaabc5c6074 in DepLibUV::RunLoop() ()
#19 0x0000aaaabc5cf3d0 in Worker::Worker(Channel::ChannelSocket*, PayloadChannel::PayloadChannelSocket*) ()
#20 0x0000aaaabc5c4c14 in mediasoup_worker_run ()
#21 0x0000aaaabc5c3834 in main ()

@ibc
Copy link
Member Author

ibc commented Mar 21, 2023

@vpalmisano please create a separate issue for this.

@vpalmisano vpalmisano mentioned this pull request Mar 21, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jmillan jmillan jmillan approved these changes

@nazar-pc nazar-pc Awaiting requested review from nazar-pc

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

consuming plain producer results in worker crash
3 participants
@ibc @vpalmisano @jmillan