Add max bitrate option #1463

utkarshdalal · 2023-07-23T12:00:29Z

Description

Added a parameter for max bitrate to the web UI, config file and video.cpp. Additionally, made some small changes to fix CI deployment script for Windows - renamed a conflicting parameter.

Screenshot

Issues Fixed or Closed

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)
Dependency update (updates to dependencies)
Documentation update (changes to documentation)
Repository update (changes to repository files, e.g. .github/...)

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 in code docstring/documentation-blocks for new or existing methods/components

Branch Updates

LizardByte requires that branches be up-to-date before merging. This means that after any PR is merged, this branch
must be updated before it can be merged. You must also
Allow edits from maintainers.

I want maintainers to keep my branch updated

v0.15.0

v0.15.0 resubmit

v0.16.0

v0.17.0

v0.18.0

v0.18.1

v0.18.2

v0.18.3

v0.18.4

v0.19.0

v0.19.1

v0.20.0

…ient.

github-actions · 2023-07-23T12:00:42Z

Your PR was set to master, PRs should be sent to nightly.
The base branch of this PR has been automatically changed to nightly.
Please check that there are no merge conflicts

ReenigneArcher

Thanks for the PR. I have some changes to request. In addition to the other comments, we also need to update the advanced usage section of the docs.

ReenigneArcher · 2023-07-23T12:05:51Z

src/platform/windows/publish.cpp


 extern "C" {
-constexpr auto DNS_REQUEST_PENDING = 9506L;
+constexpr auto MY_DNS_REQUEST_PENDING = 9506L;


We don't really name variables "my_variable".

This was the change I had to make for the CI, it was failing with this error otherwise - https://github.com/utkarshdalal/Sunshine-GameAway/actions/runs/5632865670/job/15261219026. Screenshot below.

I can name it something else also if you like, but this variable name was conflicting with a reserved variable name.

README.rst

src/config.cpp

src_assets/common/assets/web/config.html

ReenigneArcher · 2023-07-23T12:14:56Z

Also, there was no change to the CI. Guessing you were working off the master branch, and fixed something we already fixed in nightly?

# Conflicts: # src/platform/windows/publish.cpp # src_assets/common/assets/web/config.html

utkarshdalal · 2023-07-24T17:40:10Z

@ReenigneArcher The PR is updated, the only thing that I didn't change was renaming the variable to DNS_REQUEST_PENDING from MY_DNS_REQUEST_PENDING. As I mentioned in the comment, doing this made my Windows build fail. Does this work for you? If so I can change it but I think renaming the variable is necessary.

ReenigneArcher

I have a few more requested changes.

Regarding the variable, I think it should be changed back. I'm a bit confused why it failed for you as we have many builds going daily and not had any failures from this recently. My first inclination is that this was something that we fixed a while back in the nightly branch, but you were working from the master branch, so didn't have our fixes. (@cgutman thoughts?)

ReenigneArcher · 2023-07-24T18:40:31Z

src/config.cpp

    },  // supported resolutions

    { 10, 30, 60, 90, 120 },  // supported fps
+    0, // max bitrate


Suggested change

0, // max bitrate

0, // max bitrate

2 spaces (linting rules)

ReenigneArcher · 2023-07-24T18:42:40Z

src_assets/common/assets/web/config.html

+            v-model="config.max_bitrate"
+          />
+          <div class="form-text">
+            Maximum bitrate for streaming in Kbps. If not specified, the default bitrate is used


Suggested change

Maximum bitrate for streaming in Kbps. If not specified, the default bitrate is used

Maximum bitrate for streaming in Kbps.

We always use the default value if not specified (or if the value provided by the user is outside an acceptable range). Actually we should probably enforce a max value for the setting.

ReenigneArcher · 2023-07-24T18:44:45Z

src/config.cpp

    list_string_f(vars, "resolutions"s, nvhttp.resolutions);
    list_int_f(vars, "fps"s, nvhttp.fps);
    list_prep_cmd_f(vars, "global_prep_cmd", config::sunshine.prep_cmds);
+    int_f(vars, "max_bitrate", nvhttp.max_bitrate);


Suggested change

int_f(vars, "max_bitrate", nvhttp.max_bitrate);

int_between_f(vars, "max_bitrate", nvhttp.max_bitrate, { 0, 150000 });

utkarshdalal · 2023-07-25T15:37:08Z

@ReenigneArcher I've addressed all your comments including renaming the variable. Maybe you haven't seen it break because the action (Windows Build) only runs when pushing to the master branch. Anyway, whenever you want to make a new version, if it breaks changing the name of this variable is the fix.

ReenigneArcher · 2023-07-25T15:55:05Z

It runs all all PRs, as well as push events to nightly and master branches.

utkarshdalal · 2023-07-29T19:44:42Z

@ReenigneArcher signed.

ReenigneArcher

I found an additional item to change. Placeholder should be the default value.

ReenigneArcher · 2023-08-18T01:59:30Z

src_assets/common/assets/web/config.html

+            type="number"
+            class="form-control"
+            id="max_bitrate"
+            placeholder="5000"


Suggested change

placeholder="5000"

placeholder="0"

It's done, sorry for the delay I missed the notification.

# Conflicts: # docs/source/about/advanced_usage.rst

v0.21.0

utkarshdalal · 2023-11-22T09:53:41Z

Anything remaining to be done on this issue?

Add max bitrate 2

ReenigneArcher · 2023-11-23T02:53:14Z

Anything remaining to be done on this issue?

Agreement from other team members to merge this.

LizardByte-bot · 2024-03-31T10:05:47Z

It looks like this PR has been idle for 90 days. If it's still something you're working on or would like to pursue, please leave a comment or update your branch. Otherwise, we'll be closing this PR in 10 days to reduce our backlog. Thanks!

utkarshdalal · 2024-03-31T10:29:35Z

I mean you guys aren't reviewing it so not sure what you want from me

…

On Sun, 31 Mar, 2024, 14:06 LizardByte-bot, ***@***.***> wrote: It looks like this PR has been idle for 90 days. If it's still something you're working on or would like to pursue, please leave a comment or update your branch. Otherwise, we'll be closing this PR in 10 days to reduce our backlog. Thanks! — Reply to this email directly, view it on GitHub <#1463 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AA4SWC4UXGUOAMDZHV4KMGDY27NZDAVCNFSM6AAAAAA2UO5LB2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDAMRYGYZDIMZTGY> . You are receiving this because you were mentioned.Message ID: ***@***.***>

ReenigneArcher · 2024-04-05T02:00:04Z

@utkarshdalal I apologize, but at this time our team doesn't feel that this feature would be valuable to Sunshine. We can possibly revisit it in the future if there is more interest from users.

ReenigneArcher and others added 19 commits October 30, 2022 15:05

Merge pull request LizardByte#332 from LizardByte/nightly

660d8e1

v0.15.0

Merge pull request LizardByte#456 from LizardByte/nightly

1e6d9da

v0.15.0 resubmit

Merge pull request LizardByte#458 from LizardByte/nightly

8509ee7

v0.15.0 resubmit

Merge pull request LizardByte#460 from LizardByte/nightly

e1b112c

v0.15.0 resubmit

Merge pull request LizardByte#467 from LizardByte/nightly

08d623c

v0.16.0

Merge pull request LizardByte#567 from LizardByte/nightly

b2fe042

v0.17.0

Merge pull request LizardByte#738 from LizardByte/nightly

b2c5da2

v0.18.0

Merge pull request LizardByte#866 from LizardByte/nightly

09dff34

v0.18.1

Merge pull request LizardByte#877 from LizardByte/nightly

446825b

v0.18.2

Merge pull request LizardByte#929 from LizardByte/nightly

9563419

v0.18.3

Merge pull request LizardByte#954 from LizardByte/nightly

0dfbcfd

v0.18.4

Merge pull request LizardByte#969 from LizardByte/nightly

483e225

v0.19.0

Merge pull request LizardByte#1105 from LizardByte/nightly

d70d084

v0.19.1

Merge pull request LizardByte#1115 from LizardByte/nightly

31e8b79

v0.20.0

added max bitrate variable that overrides bitrate set by moonlight cl…

9cfa228

…ient.

corrected syntax errors

6d39591

updated variable in publish.cpp for windows to build in github action

23fbeae

added empty line to force release

d8b62ff

Updated description for max_bitrate param

72551fe

github-actions bot changed the base branch from master to nightly July 23, 2023 12:00

ReenigneArcher requested changes Jul 23, 2023

View reviewed changes

Utkarsh Dalal added 2 commits July 24, 2023 22:33

Addressed code review comments

3fb13de

Merge branch 'nightly' into add_max_bitrate

5f49733

# Conflicts: # src/platform/windows/publish.cpp # src_assets/common/assets/web/config.html

ReenigneArcher requested changes Jul 24, 2023

View reviewed changes

This comment was marked as resolved.

Sign in to view

ReenigneArcher requested a review from cgutman July 29, 2023 20:38

ReenigneArcher requested changes Aug 18, 2023

View reviewed changes

Merge branch 'nightly' into add_max_bitrate

3955d2d

# Conflicts: # docs/source/about/advanced_usage.rst

LizardByte-bot added the autoupdate label Sep 13, 2023

ReenigneArcher previously approved these changes Sep 13, 2023

View reviewed changes

ReenigneArcher removed the autoupdate label Sep 15, 2023

ReenigneArcher requested a review from ns6089 September 18, 2023 23:15

ReenigneArcher mentioned this pull request Oct 7, 2023

AMF Encoder on Radeon 5000 and newer overshoots encoding target at higher constant bitrates #1040

Closed

2 tasks

ReenigneArcher changed the title ~~Add max bitrate option in Sunshine UI~~ Add max bitrate option Oct 7, 2023

Merge pull request LizardByte#1334 from LizardByte/nightly

5bca024

v0.21.0

utkarshdalal dismissed ReenigneArcher’s stale review via 5bca024 November 22, 2023 10:00

utkarshdalal force-pushed the master branch from ad45dcb to 5bca024 Compare November 22, 2023 10:00

Utkarsh Dalal added 3 commits November 22, 2023 15:51

Merge pull request #3 from utkarshdalal/add_max_bitrate_2

415fe56

Add max bitrate 2

Merge branch 'nightly' into master

66e8b9f

changes to match remote nightly

82416e8

ReenigneArcher requested review from cgutman and removed request for cgutman and ns6089 January 1, 2024 03:18

LizardByte-bot added the stale label Mar 31, 2024

LizardByte-bot removed the stale label Apr 1, 2024

ReenigneArcher closed this Apr 5, 2024

ReenigneArcher mentioned this pull request Feb 9, 2025

feat: Add Max Bitrate option #3628

Merged

10 tasks

	Maximum bitrate for streaming in Kbps. If not specified, the default bitrate is used
	Maximum bitrate for streaming in Kbps.

	int_f(vars, "max_bitrate", nvhttp.max_bitrate);
	int_between_f(vars, "max_bitrate", nvhttp.max_bitrate, { 0, 150000 });

Uh oh!

Add max bitrate option #1463

Add max bitrate option #1463

Uh oh!

Conversation

utkarshdalal commented Jul 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Screenshot

Issues Fixed or Closed

Type of Change

Checklist

Branch Updates

Uh oh!

github-actions bot commented Jul 23, 2023

Uh oh!

ReenigneArcher left a comment

Choose a reason for hiding this comment

Uh oh!

ReenigneArcher Jul 23, 2023

Choose a reason for hiding this comment

Uh oh!

utkarshdalal Jul 24, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ReenigneArcher commented Jul 23, 2023

Uh oh!

utkarshdalal commented Jul 24, 2023

Uh oh!

ReenigneArcher left a comment

Choose a reason for hiding this comment

Uh oh!

ReenigneArcher Jul 24, 2023

Choose a reason for hiding this comment

Uh oh!

ReenigneArcher Jul 24, 2023

Choose a reason for hiding this comment

Uh oh!

ReenigneArcher Jul 24, 2023

Choose a reason for hiding this comment

Uh oh!

utkarshdalal commented Jul 25, 2023

Uh oh!

ReenigneArcher commented Jul 25, 2023

Uh oh!

This comment was marked as resolved.

utkarshdalal commented Jul 29, 2023

Uh oh!

ReenigneArcher left a comment

Choose a reason for hiding this comment

Uh oh!

ReenigneArcher Aug 18, 2023

Choose a reason for hiding this comment

Uh oh!

utkarshdalal Aug 28, 2023

Choose a reason for hiding this comment

Uh oh!

utkarshdalal commented Nov 22, 2023

Uh oh!

ReenigneArcher commented Nov 23, 2023

Uh oh!

LizardByte-bot commented Mar 31, 2024

Uh oh!

utkarshdalal commented Mar 31, 2024 via email

Uh oh!

ReenigneArcher commented Apr 5, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

utkarshdalal commented Jul 23, 2023 •

edited

Loading