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

Improved receiver buffer implementation #1964

Merged

Conversation

maxsharabayko
Copy link
Collaborator

@maxsharabayko maxsharabayko commented Apr 21, 2021

This PR introduces a new receiver buffer.
To start with, both old and new versions of the receiver buffer are available.
A build option ENABLE_NEW_RCVBUFFER can be used to build SRT with the new version.

Main Features

  • Lower latency: not waiting for packets to be acknowledged to allow reading. Resolves the minimum 10ms latency of SRT.
  • Handling of sender's drop requests. The old version does not allow to properly drop a message in the middle of the receiver buffer, limiting live CC.
  • Out of order reading. SRT with disabled TSBPD handled the "order" flag O of a data packet, allowing to read packets out of order. It is not working properly with the old receiver buffer.
  • TBD

Addresses #1674

Description to be improved.

TODO

  • In file config currently ACK and isRcvDataReady are not handled properly. See also Transmission.FileUpload test.
  • Fix the build with -DENABLE_NEW_RCVBUFFER=OFF.
  • Rebase on the latest SRT master.
  • CRcvBufferNew::getRcvDataSize(..): counting acknowledged packets requires modifications.
  • Check receiver buffer fullness MAvg (takes 2 seconds, which is too long). The same behavior is in the old RCV buffer. Create a separate issue?

image

@maxsharabayko maxsharabayko added Type: Maintenance Work required to maintain or clean up the code [core] Area: Changes in SRT library core labels Apr 21, 2021
@maxsharabayko maxsharabayko added this to the v1.4.4 milestone Apr 21, 2021
@maxsharabayko maxsharabayko self-assigned this Apr 21, 2021
@maxsharabayko maxsharabayko force-pushed the develop/rcv-buffer-noack branch 2 times, most recently from 6aece5a to 8d9b68a Compare May 17, 2021 13:51

template <class T>
class FixedArray
{
Copy link
Contributor

Choose a reason for hiding this comment

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

  1. utilities.h is included by common.h
  2. common.h use something defined by utilities.h, e.g., HAVE_FULL_CXX11
  3. Something defined by common.h can be useful for utilities.h, e.g., SRT_ASSERT, SRT_STATIC_ASSERT, but can't due to circular header include

utilities.h is more than utility, it needs some cleanup work. Before that, I prefer the "include what you use" method, and use a separate header file. Of course, the cleanup work can be done after the patch. So the comments above doesn't related to the patch strictly.

test/test_buffer2.cpp Outdated Show resolved Hide resolved
@maxsharabayko maxsharabayko marked this pull request as ready for review October 22, 2021 12:45
@maxsharabayko maxsharabayko changed the title Improved receiver buffer implementation (WIP) Improved receiver buffer implementation Oct 22, 2021
@maxsharabayko maxsharabayko merged commit 427cece into Haivision:master Oct 22, 2021
@maxsharabayko maxsharabayko deleted the develop/rcv-buffer-noack branch October 22, 2021 13:32
maxsharabayko added a commit to maxsharabayko/srt that referenced this pull request Nov 9, 2021
maxsharabayko added a commit that referenced this pull request Nov 10, 2021
Only the new RCV buffer (PR #1964) is affected.
@gou4shi1
Copy link
Contributor

gou4shi1 commented Dec 6, 2021

I finally found time to look at this great refactor.
It can simply #2046 a lot :)
However, it seems to lack the optimization introduced by #2026

@maxsharabayko
Copy link
Collaborator Author

However, it seems to lack the optimization introduced by #2026

Yes, this optimization is missing.
The ultimate goal is to eventually share a single receiver buffer between all member sockets.
However, it is not there yet. But the optimization is useful. Maybe it makes sense to drop all packets up to the group base before reading to minimize the scope of the reading function. 🤔

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[core] Area: Changes in SRT library core Type: Maintenance Work required to maintain or clean up the code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants