Skip to content

video: use a better vbv-bufsize & correct software bitrate calculation #33

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
Merged
merged 1 commit into from
Jan 20, 2022
Merged

video: use a better vbv-bufsize & correct software bitrate calculation #33

merged 1 commit into from
Jan 20, 2022

Conversation

PapyKahan
Copy link
Contributor

Description

Change cherry-picked from @psyke83 repository. It has a real impact on stream quality on my various tests.

  • Increase vbv-bufsize to 1/10 of requested bitrate. The previous value
    was too low, which was resulting in poor encoding quality and failure to
    stabilize at the requested bitrate. Setting to 1/10 of bitrate is a
    good compromise, as it avoids quality loss but also prevents bandwidth
    spikes far above the bitrate/vbv-maxrate.
  • With vbv-bufsize set to a more appropriate value, testing shows that
    the average bitrate vs client-requested bandwidth overshoots by ~20%.
    Adjust for this scenario in the software encoding case only.

Screenshot

N/A

Issues Fixed or Closed

N/A

Type of Change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have added or updated the documentation blocks for new or existing components

@psyke83 @PapyKahan
video: use a better vbv-bufsize & correct software bitrate calculation
1b6bf8f 
* Increase vbv-bufsize to 1/10 of requested bitrate. The previous value
  was too low, which was resulting in poor encoding quality and failure to
  stabilize at the requested bitrate. Setting to 1/10 of bitrate is a
  good compromise, as it avoids quality loss but also prevents bandwidth
  spikes far above the bitrate/vbv-maxrate.
* With vbv-bufsize set to a more appropriate value, testing shows that
  the average bitrate vs client-requested bandwidth overshoots by ~20%.
  Adjust for this scenario in the software encoding case only.
@ReenigneArcher
Copy link
Member

@cfajardo thanks for submitting this! I'd like to give the original author a change to submit it themselves if they feel that's necessary.

@psyke83 if you'd like to submit this yourself, please let me know or just open a separate PR.

Thanks!

@psyke83
Copy link
Contributor

psyke83 commented Jan 19, 2022

@ReenigneArcher

Thanks for the heads up. I've no issue with this PR as-is; just keep in mind that I already submitted this change via my own PR to upstream, just in case that causes you issues when merging/rebasing (if/when it gets accepted).

@ReenigneArcher
Copy link
Member

Thanks for the heads up. I've no issue with this PR as-is; just keep in mind that I already submitted this change via my own PR to upstream, just in case that causes you issues when merging/rebasing (if/when it gets accepted).

Sounds good. Thanks again! Also this repo is an import of the original project, not a fork. So as far as I know any changes here would need to be copied into a fork of the main repo and then submitted. I decided to use an import instead of a fork because forks cannot be searched and it makes it more difficult to submit PRs. Not sure what's going on with Loki, but hopefully nothing serious. The goal here is to make things more manageable and somewhat automated.

@PapyKahan
Copy link
Contributor Author

PapyKahan commented Jan 19, 2022 •
edited
Loading

Tbh I felt uncomfortable with submitting this PR as it's not my own code. Thanks @psyke83 for your openness.

TheElixZammuto
TheElixZammuto approved these changes Jan 19, 2022
View reviewed changes
Copy link
Member

@TheElixZammuto TheElixZammuto left a comment

Choose a reason for hiding this comment

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

Haven't tested this, but LGTM since the explication seems good enough and the modification is very simple

@ReenigneArcher ReenigneArcher merged commit de9136c into LizardByte:nightly Jan 20, 2022
@PapyKahan PapyKahan deleted the psyke83-bufsize-fix branch January 20, 2022 12:23
ReenigneArcher added a commit that referenced this pull request Jan 21, 2022
@ReenigneArcher
Update CHANGELOG.md
4126a1a 
- Update based on #33
- Fix year for 2021 releases
@ReenigneArcher
Copy link
Member

Just for reference: this is the same as loki-47-6F-64/sunshine#288

@Forusim Forusim mentioned this pull request Nov 6, 2022
Closed
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
1 more reviewer

@TheElixZammuto TheElixZammuto TheElixZammuto approved these changes

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@PapyKahan @ReenigneArcher @psyke83 @TheElixZammuto