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

SRTO_OUTPACEMODE option for Output Pace Control #1383

Closed
wants to merge 5 commits into from

Conversation

jeandube
Copy link
Collaborator

SRTO_OUTPACEMODE option can be use to implement custom sender output bit rate. This option provides all the implicit output pace modes defined by the options SRTO_MAXBW, SRTO_INPUTBW, and SRTO_OHEADBW, and add a new mode combining the internally sampled input rate and the configured input rate to adjust the output rate. Other custom modes can be added taking other factors into account such as he size of the sender buffer, the size of the lost packets list, etc... Documentation has been updated and enhanced. The GET-only option SRTO_SMPINBW has also been added to get the internally sampled input bitrate but this value should probably be added in the stats also.

@ethouris ethouris added [core] Area: Changes in SRT library core [docs] Area: Improvements or additions to documentation Impact: Optional Type: Enhancement Indicates new feature requests labels Jun 26, 2020
@maxsharabayko maxsharabayko added this to the v1.5.0 milestone Jul 13, 2020
@maxsharabayko maxsharabayko self-assigned this Jul 13, 2020
@maxsharabayko maxsharabayko self-requested a review July 13, 2020 09:59
@maxsharabayko maxsharabayko modified the milestones: v1.5.0, v1.5.0 - Sprint 19 Jul 13, 2020
Copy link
Collaborator

@maxsharabayko maxsharabayko left a comment

Choose a reason for hiding this comment

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

Consider some minor changes.

docs/output-pace-control.md Outdated Show resolved Hide resolved
docs/output-pace-control.md Outdated Show resolved Hide resolved
docs/output-pace-control.md Outdated Show resolved Hide resolved
srtcore/core.cpp Outdated Show resolved Hide resolved
srtcore/core.cpp Outdated Show resolved Hide resolved
srtcore/core.cpp Outdated Show resolved Hide resolved
srtcore/core.cpp Outdated Show resolved Hide resolved
srtcore/core.cpp Outdated Show resolved Hide resolved
srtcore/srt.h Outdated Show resolved Hide resolved
jeandube and others added 2 commits July 14, 2020 08:05
Accepted Max's suggestions

Co-authored-by: Maxim Sharabayko <maxlovic@gmail.com>
Copy link
Collaborator

@maxsharabayko maxsharabayko left a comment

Choose a reason for hiding this comment

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

A couple more formatting-related suggestions.

docs/output-pace-control.md Outdated Show resolved Hide resolved
docs/output-pace-control.md Outdated Show resolved Hide resolved
srtcore/queue.cpp Outdated Show resolved Hide resolved
srtcore/queue.cpp Outdated Show resolved Hide resolved
Co-authored-by: Maxim Sharabayko <maxlovic@gmail.com>
@maxsharabayko maxsharabayko modified the milestones: v1.5.0 - Sprint 19, v1.5.0 - Sprint 20 Jul 27, 2020
@mbakholdina mbakholdina modified the milestones: v1.5.0 - Sprint 20, v1.5.0 - Sprint 21 Aug 10, 2020
@maxsharabayko maxsharabayko modified the milestones: v1.5.0 - Sprint 21, v1.5.0 - Sprint 22 Aug 25, 2020
@mbakholdina mbakholdina self-requested a review September 1, 2020 14:24
@mbakholdina mbakholdina self-assigned this Sep 3, 2020
@maxsharabayko maxsharabayko modified the milestones: v1.5.0 - Sprint 22, v1.5.0 - Sprint 23 Sep 7, 2020
@mbakholdina
Copy link
Collaborator

@jeandube, @maxsharabayko

Hi Jean,
I've created a PR #1533 adding the documentation to the current PR with appropriate suggested names for the option and modes so that they are aligned with the RFC. As a result,

  • I suggest changing socket option name from SRTO_OUTPACEMODE to SRTO_PACINGMODE. The module will be name Pacing Control module instead of Output pace control module and I will change it in RFC as well,
  • I suggest changing modes names to SRT_PACING_..., please check my PR for the list of exact names,
  • I've added a very important note in the description of SRTO_PACINGMODE option assuming that we need to have additional checks and throw an error in case of incorrect configuring of SRTO_PACINGMODE and a combination of SRTO_MAXBW, SRTO_INPUTBW and SRTO_OHEADBW options. As I can see from the code, we do not do those checks currently and there is a chance for user to make a mistake. Let's discuss which error can be thrown here and where should be add these checks: setOpt function? @maxsharabayko What do you recommend?

That's my note:
It's important to note that options SRTO_MAXBW, SRTO_INPUTBW and SRT_OHEADBW should be set first, right before setting the SRTO_PACINGMODE option. The order is important. In case the combination of SRTO_MAXBW, SRTO_INPUTBW and SRTO_OHEADBW options is configured wrong for a particular pacing control mode, SRT will throw an error (TODO: discuss what kind of error and put this information here).

  • Please in the code do appropriate renaming of things like m_iOutPaceMode to m_iPacingMode, etc., so that it corresponds to our new notations
  • Please delete your changes related to describing new socket option in API.md, we will merge your PR together with mine [docs] Added SRTO_PACINGMODE option #1533 introducing improved version of documentation
  • Please remove docs/output-pace-control.md file from the PR, we will have a link to RFC from the socket description as discussed

Please let me know if you have any concerns/corrections/suggestions to all mentioned above.

@mbakholdina mbakholdina modified the milestones: v1.5.0 - Sprint 23, v1.5.0 - Sprint 24 Sep 21, 2020
@maxsharabayko maxsharabayko modified the milestones: v1.5.0 - Sprint 24, v1.5.0 - Sprint 25 Sep 21, 2020
@mbakholdina mbakholdina modified the milestones: v1.5.0 - Sprint 25, v1.5.0 Oct 14, 2020
@oviano
Copy link
Contributor

oviano commented Jan 7, 2021

So just on this, I was reading the SRT whitepaper and it states "Another change made in SRT version 1.3 was to add a configuration option called OUTPUTPACEMODE"....just so I'm clear, I assume that was presumably pulled and is now intended for version 1.5 at some point in the future, correct?

However, even before the whitepaper describes the OUTPUTPACEMODE option, it states "SRT version 1.3 combines the two packet pacing methods. The input is measured, but it it falls too low, the nominal input bit rate value for the SRT sender's output is respected. If a measurement is lower than the nominal configuration, it isn't followed. However if the encoder input rate rises above the configured rate, it will be followed as much as possible."

Is that still accurate, or will that only be the case for 1.5?

@jeandube
Copy link
Collaborator Author

jeandube commented Jan 7, 2021

@oviano I tried to introduce this feature in 1.4.2 but the focus was on on other features. After some reviews changes were proposed in the form of another PR. The principle is the same but with some naming changes. I am not sure if it will find its way in 1.4.3 or 1.5.0. I don't even know what the next release will be.

@oviano
Copy link
Contributor

oviano commented Jan 7, 2021

Right ok thanks, so the whitepaper should be ignored then it would seem.

I’ve clearly been using SRT wrong until now by relying on the input rate to be automatically calculated, since specifying number actual encoder bitrate everything is much more stable and consistent.

On the subject of encoder overshoot; if you specify the input bitrate INPUTBW is there any scenario where you wouldn’t want it to use the calculated input rate if it was higher? Seems like it should work this way in any case, if you are specifying the input manually...

Or why not simplify things and just add SRTO_MINBW. Specify this and it would behave as described, ie uses the highest of this and the calculated BW. Adds nice symmetry to the MAXBW option too.

@jeandube
Copy link
Collaborator Author

jeandube commented Jan 7, 2021

@oviano I like the idea of SRTO_MINBW. But unlike SRTO_MAXBW, to replace SRTO_INPUTBW, SRTO_MINBW must be a post-binding option settable at all time. I am using SRTO_INPUTBW when I adjust the encoder's bit rate based on the SRT stats. For your other question I see no reason not to use the measured input bandwidth when higher then configured INPUTBW (overshoot) but I encountered this with adverse effect on decoder only for very low bit rate (~32kbps). I am happy to see other than me needs that feature.

@oviano
Copy link
Contributor

oviano commented Jan 7, 2021

So I was thinking SRTO_MINBW would be an additional option and wouldn’t replace INPUTBW.

Ie If you are guaranteeing a fixed bitrate, you use INPUTBW as now. So nothing changes.

But if you want this new behaviour then you specify MINBW instead, meaning “this is my encoder bitrate, but I’m happy for SRT to go above this if the calculated value is higher.”

@maxsharabayko
Copy link
Collaborator

There was some rework in #1541, but it ended up with a similar conclusion, that SRTO_MININPUTBW would be an easier and a more straightforward approach. Even though it adds another integer to be stored in memory.

@maxsharabayko maxsharabayko mentioned this pull request Feb 5, 2021
1 task
@maxsharabayko
Copy link
Collaborator

Closing this PR.
PR #1791 introduced SRTO_MININPUTBW option covering the required functionality.
The functionality with pacing mode socket option SRTO_PACINGMODE remains in PR #1541, although is likely to be reconsidered.

@mbakholdina mbakholdina modified the milestones: v1.5.0, v1.4.3 May 12, 2021
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 [docs] Area: Improvements or additions to documentation Priority: Low Type: Enhancement Indicates new feature requests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants