Add max bitrate option - #1463

Closed
utkarshdalal wants to merge 26 commits into
LizardByte:nightlyfrom
utkarshdalal:master
Closed

Add max bitrate option#1463
utkarshdalal wants to merge 26 commits into
LizardByte:nightlyfrom
utkarshdalal:master

Conversation

@utkarshdalal

@utkarshdalalutkarshdalal commented Jul 23, 2023•
edited
Loading

Copy link
Copy Markdown
Contributor

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

image

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

ReenigneArcherand others added 19 commits October 30, 2022 15:05
@ReenigneArcher
Merge pull requestLizardByte#332from LizardByte/nightly
660d8e1
v0.15.0
@ReenigneArcher
Merge pull requestLizardByte#456from LizardByte/nightly
1e6d9da
v0.15.0 resubmit
@ReenigneArcher
Merge pull requestLizardByte#458from LizardByte/nightly
8509ee7
v0.15.0 resubmit
@ReenigneArcher
Merge pull requestLizardByte#460from LizardByte/nightly
e1b112c
v0.15.0 resubmit
@ReenigneArcher
Merge pull requestLizardByte#467from LizardByte/nightly
08d623c
v0.16.0
@ReenigneArcher
Merge pull requestLizardByte#567from LizardByte/nightly
b2fe042
v0.17.0
@ReenigneArcher
Merge pull requestLizardByte#738from LizardByte/nightly
b2c5da2
v0.18.0
@ReenigneArcher
Merge pull requestLizardByte#866from LizardByte/nightly
09dff34
v0.18.1
@ReenigneArcher
Merge pull requestLizardByte#877from LizardByte/nightly
446825b
v0.18.2
@ReenigneArcher
Merge pull requestLizardByte#929from LizardByte/nightly
9563419
v0.18.3
@ReenigneArcher
Merge pull requestLizardByte#954from LizardByte/nightly
0dfbcfd
v0.18.4
@ReenigneArcher
Merge pull requestLizardByte#969from LizardByte/nightly
483e225
v0.19.0
@ReenigneArcher
Merge pull requestLizardByte#1105from LizardByte/nightly
d70d084
v0.19.1
@ReenigneArcher
Merge pull requestLizardByte#1115from LizardByte/nightly
31e8b79
v0.20.0
Utkarsh Dalal
added max bitrate variable that overrides bitrate set by moonlight cl…
9cfa228
…ient.
Utkarsh Dalal
corrected syntax errors
6d39591
Utkarsh Dalal
updated variable in publish.cpp for windows to build in github action
23fbeae
Utkarsh Dalal
added empty line to force release
d8b62ff
Utkarsh Dalal
Updated description for max_bitrate param
72551fe
@github-actions
github-actionsBot changed the base branch from master to nightlyJuly 23, 2023 12:00
@github-actions

github-actionsBot commented Jul 23, 2023

Copy link
Copy Markdown

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
ReenigneArcher requested changes Jul 23, 2023
View reviewed changes

@ReenigneArcherReenigneArcher left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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.

Comment threadsrc/platform/windows/publish.cpp Outdated

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

@ReenigneArcherReenigneArcherJul 23, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We don't really name variables "my_variable".

@utkarshdalalutkarshdalalJul 24, 2023

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

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.
image

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

Comment threadREADME.rst Outdated
Comment threadsrc/config.cpp Outdated
Comment threadsrc_assets/common/assets/web/config.html Outdated
Comment threadsrc_assets/common/assets/web/config.html Outdated
@ReenigneArcher

ReenigneArcher commented Jul 23, 2023

Copy link
Copy Markdown
Member

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

Utkarsh Dalal added 2 commits July 24, 2023 22:33
Utkarsh Dalal
Addressed code review comments
3fb13de
Utkarsh Dalal
Merge branch 'nightly' into add_max_bitrate
5f49733
# Conflicts:
#	src/platform/windows/publish.cpp
#	src_assets/common/assets/web/config.html
@utkarshdalal

utkarshdalal commented Jul 24, 2023

Copy link
Copy Markdown
ContributorAuthor

@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
ReenigneArcher requested changes Jul 24, 2023
View reviewed changes

@ReenigneArcherReenigneArcher left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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?)

Comment threadsrc/config.cpp
}, // supported resolutions

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

@ReenigneArcherReenigneArcherJul 24, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
0, // max bitrate
0, // max bitrate

2 spaces (linting rules)

Comment threadsrc_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

@ReenigneArcherReenigneArcherJul 24, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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.

Comment threadsrc/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);

@ReenigneArcherReenigneArcherJul 24, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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

@utkarshdalal

utkarshdalal commented Jul 25, 2023

Copy link
Copy Markdown
ContributorAuthor

@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

ReenigneArcher commented Jul 25, 2023

Copy link
Copy Markdown
Member

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

@ReenigneArcher

This comment was marked as resolved.

Sign in to view
@utkarshdalal

utkarshdalal commented Jul 29, 2023

Copy link
Copy Markdown
ContributorAuthor

@ReenigneArcher signed.

ReenigneArcher
ReenigneArcher requested changes Aug 18, 2023
View reviewed changes

@ReenigneArcherReenigneArcher left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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

Comment threadsrc_assets/common/assets/web/config.html
type="number"
class="form-control"
id="max_bitrate"
placeholder="5000"

@ReenigneArcherReenigneArcherAug 18, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
placeholder="5000"
placeholder="0"

@utkarshdalalutkarshdalalAug 28, 2023

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

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

Utkarsh Dalal
Merge branch 'nightly' into add_max_bitrate
3955d2d
# Conflicts:
#	docs/source/about/advanced_usage.rst
ReenigneArcher
ReenigneArcher previously approved these changes Sep 13, 2023
View reviewed changes
@ReenigneArcherReenigneArcher mentioned this pull request Oct 7, 2023
Closed
2 tasks
@ReenigneArcherReenigneArcher changed the title Add max bitrate option in Sunshine UIAdd max bitrate optionOct 7, 2023
@utkarshdalal

utkarshdalal commented Nov 22, 2023

Copy link
Copy Markdown
ContributorAuthor

Anything remaining to be done on this issue?

@ReenigneArcher

ReenigneArcher commented Nov 23, 2023

Copy link
Copy Markdown
Member

Anything remaining to be done on this issue?

Agreement from other team members to merge this.

@ReenigneArcher
ReenigneArcher requested review from cgutman and removed request for cgutman and ns6089January 1, 2024 03:18
@LizardByte-bot

LizardByte-bot commented Mar 31, 2024

Copy link
Copy Markdown
Member

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

utkarshdalal commented Mar 31, 2024 via email

Copy link
Copy Markdown
ContributorAuthor

@ReenigneArcher

ReenigneArcher commented Apr 5, 2024

Copy link
Copy Markdown
Member

@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.

@ReenigneArcherReenigneArcher mentioned this pull request Feb 9, 2025
Merged
10 tasks
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ReenigneArcherReenigneArcherReenigneArcher left review comments
@cgutmancgutmanAwaiting requested review from cgutman

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@utkarshdalal@ReenigneArcher@LizardByte-bot
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Add copy buttons to all 
 blocks\n(function() {\n function addCopyButtons() {\n document.querySelectorAll('pre code').forEach(function(codeBlock) {\n if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;\n codeBlock.parentElement.setAttribute('data-copy-added', 'true');\n \n var btn = document.createElement('button');\n btn.textContent = 'Copy';\n btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';\n btn.onmouseover = function() { this.style.opacity = '1'; };\n btn.onmouseout = function() { this.style.opacity = '0.7'; };\n btn.onclick = function() {\n navigator.clipboard.writeText(codeBlock.textContent).then(function() {\n btn.textContent = 'Copied!';\n setTimeout(function() { btn.textContent = 'Copy'; }, 1500);\n });\n };\n codeBlock.parentElement.style.position = 'relative';\n codeBlock.parentElement.appendChild(btn);\n });\n }\n \n addCopyButtons();\n \n // Re-run on dynamic content\n var observer = new MutationObserver(addCopyButtons);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Add Copy Buttons to Code Blocks");
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '

Skip to content
Add max bitrate option - #1463
Closed
utkarshdalal wants to merge 26 commits into
LizardByte:nightlyfrom 
utkarshdalal:master
Closed
Add max bitrate option#1463
utkarshdalal wants to merge 26 commits into
LizardByte:nightlyfrom 
utkarshdalal:master
Conversation
@utkarshdalal
@utkarshdalalutkarshdalal
commented
Jul 23, 2023•

edited

Loading

Copy link


Copy Markdown

Contributor
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
image
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
ReenigneArcherand others
added 19 commits
October 30, 2022 15:05
@ReenigneArcher
Merge pull requestLizardByte#332from LizardByte/nightly
660d8e1
v0.15.0
@ReenigneArcher
Merge pull requestLizardByte#456from LizardByte/nightly
1e6d9da
v0.15.0 resubmit
@ReenigneArcher
Merge pull requestLizardByte#458from LizardByte/nightly
8509ee7
v0.15.0 resubmit
@ReenigneArcher
Merge pull requestLizardByte#460from LizardByte/nightly
e1b112c
v0.15.0 resubmit
@ReenigneArcher
Merge pull requestLizardByte#467from LizardByte/nightly
08d623c
v0.16.0
@ReenigneArcher
Merge pull requestLizardByte#567from LizardByte/nightly
b2fe042
v0.17.0
@ReenigneArcher
Merge pull requestLizardByte#738from LizardByte/nightly
b2c5da2
v0.18.0
@ReenigneArcher
Merge pull requestLizardByte#866from LizardByte/nightly
09dff34
v0.18.1
@ReenigneArcher
Merge pull requestLizardByte#877from LizardByte/nightly
446825b
v0.18.2
@ReenigneArcher
Merge pull requestLizardByte#929from LizardByte/nightly
9563419
v0.18.3
@ReenigneArcher
Merge pull requestLizardByte#954from LizardByte/nightly
0dfbcfd
v0.18.4
@ReenigneArcher
Merge pull requestLizardByte#969from LizardByte/nightly
483e225
v0.19.0
@ReenigneArcher
Merge pull requestLizardByte#1105from LizardByte/nightly
d70d084
v0.19.1
@ReenigneArcher
Merge pull requestLizardByte#1115from LizardByte/nightly
31e8b79
v0.20.0
Utkarsh Dalal
added max bitrate variable that overrides bitrate set by moonlight cl…
9cfa228
…ient.
Utkarsh Dalal
corrected syntax errors
6d39591
Utkarsh Dalal
updated variable in publish.cpp for windows to build in github action
23fbeae
Utkarsh Dalal
added empty line to force release
d8b62ff
Utkarsh Dalal
Updated description for max_bitrate param
72551fe
@github-actions
github-actionsBot
changed the base branch from
master
to
nightlyJuly 23, 2023 12:00
@github-actions
github-actionsBot
commented
Jul 23, 2023

Copy link


Copy Markdown

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
ReenigneArcher
requested changes
Jul 23, 2023
View reviewed changes
@ReenigneArcherReenigneArcher
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

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.
Comment threadsrc/platform/windows/publish.cpp
Outdated


extern "C" {
constexpr auto DNS_REQUEST_PENDING = 9506L;
constexpr auto MY_DNS_REQUEST_PENDING = 9506L;
@ReenigneArcherReenigneArcherJul 23, 2023

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

We don't really name variables "my_variable".
@utkarshdalalutkarshdalalJul 24, 2023

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

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.
image
I can name it something else also if you like, but this variable name was conflicting with a reserved variable name.
Comment threadREADME.rst
Outdated

Comment threadsrc/config.cpp
Outdated

Comment threadsrc_assets/common/assets/web/config.html
Outdated

Comment threadsrc_assets/common/assets/web/config.html
Outdated

@ReenigneArcher
ReenigneArcher
commented
Jul 23, 2023

Copy link


Copy Markdown

Member
Also, there was no change to the CI. Guessing you were working off the master branch, and fixed something we already fixed in nightly?
Utkarsh Dalal
added 2 commits
July 24, 2023 22:33
Utkarsh Dalal
Addressed code review comments
3fb13de
Utkarsh Dalal
Merge branch 'nightly' into add_max_bitrate
5f49733
# Conflicts:
#	src/platform/windows/publish.cpp
#	src_assets/common/assets/web/config.html
@utkarshdalal
utkarshdalal
commented
Jul 24, 2023

Copy link


Copy Markdown

ContributorAuthor
@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
ReenigneArcher
requested changes
Jul 24, 2023
View reviewed changes
@ReenigneArcherReenigneArcher
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

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?)
Comment threadsrc/config.cpp
 }, // supported resolutions

 { 10, 30, 60, 90, 120 }, // supported fps
 0, // max bitrate
@ReenigneArcherReenigneArcherJul 24, 2023

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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


Suggested change

0, // max bitrate
0, // max bitrate
2 spaces (linting rules)
Comment threadsrc_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
@ReenigneArcherReenigneArcherJul 24, 2023

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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


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.
Comment threadsrc/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);
@ReenigneArcherReenigneArcherJul 24, 2023

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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


Suggested change

int_f(vars, "max_bitrate", nvhttp.max_bitrate);
int_between_f(vars, "max_bitrate", nvhttp.max_bitrate, { 0, 150000 });
@utkarshdalal
utkarshdalal
commented
Jul 25, 2023

Copy link


Copy Markdown

ContributorAuthor
@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
ReenigneArcher
commented
Jul 25, 2023

Copy link


Copy Markdown

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

This comment was marked as resolved.

Sign in to view
@utkarshdalal
utkarshdalal
commented
Jul 29, 2023

Copy link


Copy Markdown

ContributorAuthor
@ReenigneArcher signed.
ReenigneArcher
ReenigneArcher
requested changes
Aug 18, 2023
View reviewed changes
@ReenigneArcherReenigneArcher
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

I found an additional item to change. Placeholder should be the default value.
Comment threadsrc_assets/common/assets/web/config.html
 type="number"
 class="form-control"
 id="max_bitrate"
 placeholder="5000"
@ReenigneArcherReenigneArcherAug 18, 2023

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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


Suggested change

 placeholder="5000"
 placeholder="0"
@utkarshdalalutkarshdalalAug 28, 2023

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

It's done, sorry for the delay I missed the notification.
Utkarsh Dalal
Merge branch 'nightly' into add_max_bitrate
3955d2d
# Conflicts:
#	docs/source/about/advanced_usage.rst
ReenigneArcher
ReenigneArcher
previously approved these changes
Sep 13, 2023
View reviewed changes
@ReenigneArcherReenigneArcher
mentioned this pull request
Oct 7, 2023

Closed

2 tasks
@ReenigneArcherReenigneArcher
changed the title
Add max bitrate option in Sunshine UIAdd max bitrate optionOct 7, 2023
@utkarshdalal
utkarshdalal
commented
Nov 22, 2023

Copy link


Copy Markdown

ContributorAuthor
Anything remaining to be done on this issue?
@ReenigneArcher
ReenigneArcher
commented
Nov 23, 2023

Copy link


Copy Markdown

Member
Anything remaining to be done on this issue?
Agreement from other team members to merge this.
@ReenigneArcher
ReenigneArcher
requested review from
cgutman
and removed request for
cgutman and ns6089January 1, 2024 03:18
@LizardByte-bot
LizardByte-bot
commented
Mar 31, 2024

Copy link


Copy Markdown

Member
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
utkarshdalal
commented
Mar 31, 2024
via email 

Copy link


Copy Markdown

ContributorAuthor
@ReenigneArcher
ReenigneArcher
commented
Apr 5, 2024

Copy link


Copy Markdown

Member
@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.
@ReenigneArcherReenigneArcher
mentioned this pull request
Feb 9, 2025
 Merged

10 tasks
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@ReenigneArcherReenigneArcherReenigneArcher left review comments
@cgutmancgutmanAwaiting requested review from cgutman

Assignees


No one assigned


Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

3 participants

@utkarshdalal@ReenigneArcher@LizardByte-bot

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Force GitHub README to respect dark mode\n(function() {\n var style = document.createElement('style');\n style.textContent = '\n .markdown-body {\n color-scheme: dark light;\n }\n .markdown-body pre { background: #161b22 !important; }\n .markdown-body code { background: rgba(110, 118, 129, 0.4) !important; }\n .markdown-body table th, .markdown-body table td { border-color: #30363d !important; }\n .markdown-body img { background: #0d1117; }\n .markdown-body blockquote { border-left-color: #8b949e; }\n .markdown-body hr { border-color: #30363d; }\n ';\n document.head.appendChild(style);\n})();", "GitHub Dark Mode README Fix");
}
} catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
Add max bitrate option - #1463
Closed
utkarshdalal wants to merge 26 commits into
LizardByte:nightlyfrom 
utkarshdalal:master
Closed
Add max bitrate option#1463
utkarshdalal wants to merge 26 commits into
LizardByte:nightlyfrom 
utkarshdalal:master
Conversation
@utkarshdalal
@utkarshdalalutkarshdalal
commented
Jul 23, 2023•

edited

Loading

Copy link


Copy Markdown

Contributor
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
image
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
ReenigneArcherand others
added 19 commits
October 30, 2022 15:05
@ReenigneArcher
Merge pull requestLizardByte#332from LizardByte/nightly
660d8e1
v0.15.0
@ReenigneArcher
Merge pull requestLizardByte#456from LizardByte/nightly
1e6d9da
v0.15.0 resubmit
@ReenigneArcher
Merge pull requestLizardByte#458from LizardByte/nightly
8509ee7
v0.15.0 resubmit
@ReenigneArcher
Merge pull requestLizardByte#460from LizardByte/nightly
e1b112c
v0.15.0 resubmit
@ReenigneArcher
Merge pull requestLizardByte#467from LizardByte/nightly
08d623c
v0.16.0
@ReenigneArcher
Merge pull requestLizardByte#567from LizardByte/nightly
b2fe042
v0.17.0
@ReenigneArcher
Merge pull requestLizardByte#738from LizardByte/nightly
b2c5da2
v0.18.0
@ReenigneArcher
Merge pull requestLizardByte#866from LizardByte/nightly
09dff34
v0.18.1
@ReenigneArcher
Merge pull requestLizardByte#877from LizardByte/nightly
446825b
v0.18.2
@ReenigneArcher
Merge pull requestLizardByte#929from LizardByte/nightly
9563419
v0.18.3
@ReenigneArcher
Merge pull requestLizardByte#954from LizardByte/nightly
0dfbcfd
v0.18.4
@ReenigneArcher
Merge pull requestLizardByte#969from LizardByte/nightly
483e225
v0.19.0
@ReenigneArcher
Merge pull requestLizardByte#1105from LizardByte/nightly
d70d084
v0.19.1
@ReenigneArcher
Merge pull requestLizardByte#1115from LizardByte/nightly
31e8b79
v0.20.0
Utkarsh Dalal
added max bitrate variable that overrides bitrate set by moonlight cl…
9cfa228
…ient.
Utkarsh Dalal
corrected syntax errors
6d39591
Utkarsh Dalal
updated variable in publish.cpp for windows to build in github action
23fbeae
Utkarsh Dalal
added empty line to force release
d8b62ff
Utkarsh Dalal
Updated description for max_bitrate param
72551fe
@github-actions
github-actionsBot
changed the base branch from
master
to
nightlyJuly 23, 2023 12:00
@github-actions
github-actionsBot
commented
Jul 23, 2023

Copy link


Copy Markdown

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
ReenigneArcher
requested changes
Jul 23, 2023
View reviewed changes
@ReenigneArcherReenigneArcher
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

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.
Comment threadsrc/platform/windows/publish.cpp
Outdated


extern "C" {
constexpr auto DNS_REQUEST_PENDING = 9506L;
constexpr auto MY_DNS_REQUEST_PENDING = 9506L;
@ReenigneArcherReenigneArcherJul 23, 2023

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

We don't really name variables "my_variable".
@utkarshdalalutkarshdalalJul 24, 2023

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

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.
image
I can name it something else also if you like, but this variable name was conflicting with a reserved variable name.
Comment threadREADME.rst
Outdated

Comment threadsrc/config.cpp
Outdated

Comment threadsrc_assets/common/assets/web/config.html
Outdated

Comment threadsrc_assets/common/assets/web/config.html
Outdated

@ReenigneArcher
ReenigneArcher
commented
Jul 23, 2023

Copy link


Copy Markdown

Member
Also, there was no change to the CI. Guessing you were working off the master branch, and fixed something we already fixed in nightly?
Utkarsh Dalal
added 2 commits
July 24, 2023 22:33
Utkarsh Dalal
Addressed code review comments
3fb13de
Utkarsh Dalal
Merge branch 'nightly' into add_max_bitrate
5f49733
# Conflicts:
#	src/platform/windows/publish.cpp
#	src_assets/common/assets/web/config.html
@utkarshdalal
utkarshdalal
commented
Jul 24, 2023

Copy link


Copy Markdown

ContributorAuthor
@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
ReenigneArcher
requested changes
Jul 24, 2023
View reviewed changes
@ReenigneArcherReenigneArcher
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

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?)
Comment threadsrc/config.cpp
 }, // supported resolutions

 { 10, 30, 60, 90, 120 }, // supported fps
 0, // max bitrate
@ReenigneArcherReenigneArcherJul 24, 2023

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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


Suggested change

0, // max bitrate
0, // max bitrate
2 spaces (linting rules)
Comment threadsrc_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
@ReenigneArcherReenigneArcherJul 24, 2023

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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


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.
Comment threadsrc/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);
@ReenigneArcherReenigneArcherJul 24, 2023

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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


Suggested change

int_f(vars, "max_bitrate", nvhttp.max_bitrate);
int_between_f(vars, "max_bitrate", nvhttp.max_bitrate, { 0, 150000 });
@utkarshdalal
utkarshdalal
commented
Jul 25, 2023

Copy link


Copy Markdown

ContributorAuthor
@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
ReenigneArcher
commented
Jul 25, 2023

Copy link


Copy Markdown

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

This comment was marked as resolved.

Sign in to view
@utkarshdalal
utkarshdalal
commented
Jul 29, 2023

Copy link


Copy Markdown

ContributorAuthor
@ReenigneArcher signed.
ReenigneArcher
ReenigneArcher
requested changes
Aug 18, 2023
View reviewed changes
@ReenigneArcherReenigneArcher
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

I found an additional item to change. Placeholder should be the default value.
Comment threadsrc_assets/common/assets/web/config.html
 type="number"
 class="form-control"
 id="max_bitrate"
 placeholder="5000"
@ReenigneArcherReenigneArcherAug 18, 2023

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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


Suggested change

 placeholder="5000"
 placeholder="0"
@utkarshdalalutkarshdalalAug 28, 2023

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

It's done, sorry for the delay I missed the notification.
Utkarsh Dalal
Merge branch 'nightly' into add_max_bitrate
3955d2d
# Conflicts:
#	docs/source/about/advanced_usage.rst
ReenigneArcher
ReenigneArcher
previously approved these changes
Sep 13, 2023
View reviewed changes
@ReenigneArcherReenigneArcher
mentioned this pull request
Oct 7, 2023

Closed

2 tasks
@ReenigneArcherReenigneArcher
changed the title
Add max bitrate option in Sunshine UIAdd max bitrate optionOct 7, 2023
@utkarshdalal
utkarshdalal
commented
Nov 22, 2023

Copy link


Copy Markdown

ContributorAuthor
Anything remaining to be done on this issue?
@ReenigneArcher
ReenigneArcher
commented
Nov 23, 2023

Copy link


Copy Markdown

Member
Anything remaining to be done on this issue?
Agreement from other team members to merge this.
@ReenigneArcher
ReenigneArcher
requested review from
cgutman
and removed request for
cgutman and ns6089January 1, 2024 03:18
@LizardByte-bot
LizardByte-bot
commented
Mar 31, 2024

Copy link


Copy Markdown

Member
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
utkarshdalal
commented
Mar 31, 2024
via email 

Copy link


Copy Markdown

ContributorAuthor
@ReenigneArcher
ReenigneArcher
commented
Apr 5, 2024

Copy link


Copy Markdown

Member
@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.
@ReenigneArcherReenigneArcher
mentioned this pull request
Feb 9, 2025
 Merged

10 tasks
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@ReenigneArcherReenigneArcherReenigneArcher left review comments
@cgutmancgutmanAwaiting requested review from cgutman

Assignees


No one assigned


Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

3 participants

@utkarshdalal@ReenigneArcher@LizardByte-bot

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Highlight search terms from Google/DuckDuckGo/Bing referrer\n(function() {\n var ref = document.referrer;\n var terms = [];\n \n if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) {\n var url = new URL(ref);\n var q = url.searchParams.get('q') || url.searchParams.get('p');\n if (q) {\n terms = q.split(/\\s+/).filter(function(t) { return t.length > 2; });\n }\n }\n \n if (terms.length === 0) return;\n \n var style = document.createElement('style');\n style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }';\n document.head.appendChild(style);\n \n function highlight(node) {\n if (node.nodeType === 3) { // text node\n var text = node.textContent;\n var found = false;\n terms.forEach(function(term) {\n var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\\]\\\\]/g, '\\\\') + ')', 'gi');\n if (regex.test(text)) {\n found = true;\n var frag = document.createDocumentFragment();\n var parts = text.split(regex);\n parts.forEach(function(part, i) {\n if (i % 2 === 0) {\n frag.appendChild(document.createTextNode(part));\n } else {\n var span = document.createElement('span');\n span.className = 'userscript-highlight';\n span.textContent = part;\n frag.appendChild(span);\n }\n });\n node.parentNode.replaceChild(frag, node);\n }\n });\n } else if (node.nodeType === 1 && node.childNodes) { // element\n var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT'];\n if (!skipTags.includes(node.tagName)) {\n Array.from(node.childNodes).forEach(highlight);\n }\n }\n }\n \n highlight(document.body);\n \n // Re-highlight on dynamic content\n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1 || node.nodeType === 3) highlight(node);\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Highlight Search Terms");
}
} catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
Add max bitrate option - #1463
Closed
utkarshdalal wants to merge 26 commits into
LizardByte:nightlyfrom 
utkarshdalal:master
Closed
Add max bitrate option#1463
utkarshdalal wants to merge 26 commits into
LizardByte:nightlyfrom 
utkarshdalal:master
Conversation
@utkarshdalal
@utkarshdalalutkarshdalal
commented
Jul 23, 2023•

edited

Loading

Copy link


Copy Markdown

Contributor
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
image
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
ReenigneArcherand others
added 19 commits
October 30, 2022 15:05
@ReenigneArcher
Merge pull requestLizardByte#332from LizardByte/nightly
660d8e1
v0.15.0
@ReenigneArcher
Merge pull requestLizardByte#456from LizardByte/nightly
1e6d9da
v0.15.0 resubmit
@ReenigneArcher
Merge pull requestLizardByte#458from LizardByte/nightly
8509ee7
v0.15.0 resubmit
@ReenigneArcher
Merge pull requestLizardByte#460from LizardByte/nightly
e1b112c
v0.15.0 resubmit
@ReenigneArcher
Merge pull requestLizardByte#467from LizardByte/nightly
08d623c
v0.16.0
@ReenigneArcher
Merge pull requestLizardByte#567from LizardByte/nightly
b2fe042
v0.17.0
@ReenigneArcher
Merge pull requestLizardByte#738from LizardByte/nightly
b2c5da2
v0.18.0
@ReenigneArcher
Merge pull requestLizardByte#866from LizardByte/nightly
09dff34
v0.18.1
@ReenigneArcher
Merge pull requestLizardByte#877from LizardByte/nightly
446825b
v0.18.2
@ReenigneArcher
Merge pull requestLizardByte#929from LizardByte/nightly
9563419
v0.18.3
@ReenigneArcher
Merge pull requestLizardByte#954from LizardByte/nightly
0dfbcfd
v0.18.4
@ReenigneArcher
Merge pull requestLizardByte#969from LizardByte/nightly
483e225
v0.19.0
@ReenigneArcher
Merge pull requestLizardByte#1105from LizardByte/nightly
d70d084
v0.19.1
@ReenigneArcher
Merge pull requestLizardByte#1115from LizardByte/nightly
31e8b79
v0.20.0
Utkarsh Dalal
added max bitrate variable that overrides bitrate set by moonlight cl…
9cfa228
…ient.
Utkarsh Dalal
corrected syntax errors
6d39591
Utkarsh Dalal
updated variable in publish.cpp for windows to build in github action
23fbeae
Utkarsh Dalal
added empty line to force release
d8b62ff
Utkarsh Dalal
Updated description for max_bitrate param
72551fe
@github-actions
github-actionsBot
changed the base branch from
master
to
nightlyJuly 23, 2023 12:00
@github-actions
github-actionsBot
commented
Jul 23, 2023

Copy link


Copy Markdown

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
ReenigneArcher
requested changes
Jul 23, 2023
View reviewed changes
@ReenigneArcherReenigneArcher
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

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.
Comment threadsrc/platform/windows/publish.cpp
Outdated


extern "C" {
constexpr auto DNS_REQUEST_PENDING = 9506L;
constexpr auto MY_DNS_REQUEST_PENDING = 9506L;
@ReenigneArcherReenigneArcherJul 23, 2023

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

We don't really name variables "my_variable".
@utkarshdalalutkarshdalalJul 24, 2023

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

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.
image
I can name it something else also if you like, but this variable name was conflicting with a reserved variable name.
Comment threadREADME.rst
Outdated

Comment threadsrc/config.cpp
Outdated

Comment threadsrc_assets/common/assets/web/config.html
Outdated

Comment threadsrc_assets/common/assets/web/config.html
Outdated

@ReenigneArcher
ReenigneArcher
commented
Jul 23, 2023

Copy link


Copy Markdown

Member
Also, there was no change to the CI. Guessing you were working off the master branch, and fixed something we already fixed in nightly?
Utkarsh Dalal
added 2 commits
July 24, 2023 22:33
Utkarsh Dalal
Addressed code review comments
3fb13de
Utkarsh Dalal
Merge branch 'nightly' into add_max_bitrate
5f49733
# Conflicts:
#	src/platform/windows/publish.cpp
#	src_assets/common/assets/web/config.html
@utkarshdalal
utkarshdalal
commented
Jul 24, 2023

Copy link


Copy Markdown

ContributorAuthor
@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
ReenigneArcher
requested changes
Jul 24, 2023
View reviewed changes
@ReenigneArcherReenigneArcher
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

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?)
Comment threadsrc/config.cpp
 }, // supported resolutions

 { 10, 30, 60, 90, 120 }, // supported fps
 0, // max bitrate
@ReenigneArcherReenigneArcherJul 24, 2023

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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


Suggested change

0, // max bitrate
0, // max bitrate
2 spaces (linting rules)
Comment threadsrc_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
@ReenigneArcherReenigneArcherJul 24, 2023

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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


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.
Comment threadsrc/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);
@ReenigneArcherReenigneArcherJul 24, 2023

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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


Suggested change

int_f(vars, "max_bitrate", nvhttp.max_bitrate);
int_between_f(vars, "max_bitrate", nvhttp.max_bitrate, { 0, 150000 });
@utkarshdalal
utkarshdalal
commented
Jul 25, 2023

Copy link


Copy Markdown

ContributorAuthor
@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
ReenigneArcher
commented
Jul 25, 2023

Copy link


Copy Markdown

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

This comment was marked as resolved.

Sign in to view
@utkarshdalal
utkarshdalal
commented
Jul 29, 2023

Copy link


Copy Markdown

ContributorAuthor
@ReenigneArcher signed.
ReenigneArcher
ReenigneArcher
requested changes
Aug 18, 2023
View reviewed changes
@ReenigneArcherReenigneArcher
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

I found an additional item to change. Placeholder should be the default value.
Comment threadsrc_assets/common/assets/web/config.html
 type="number"
 class="form-control"
 id="max_bitrate"
 placeholder="5000"
@ReenigneArcherReenigneArcherAug 18, 2023

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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


Suggested change

 placeholder="5000"
 placeholder="0"
@utkarshdalalutkarshdalalAug 28, 2023

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

It's done, sorry for the delay I missed the notification.
Utkarsh Dalal
Merge branch 'nightly' into add_max_bitrate
3955d2d
# Conflicts:
#	docs/source/about/advanced_usage.rst
ReenigneArcher
ReenigneArcher
previously approved these changes
Sep 13, 2023
View reviewed changes
@ReenigneArcherReenigneArcher
mentioned this pull request
Oct 7, 2023

Closed

2 tasks
@ReenigneArcherReenigneArcher
changed the title
Add max bitrate option in Sunshine UIAdd max bitrate optionOct 7, 2023
@utkarshdalal
utkarshdalal
commented
Nov 22, 2023

Copy link


Copy Markdown

ContributorAuthor
Anything remaining to be done on this issue?
@ReenigneArcher
ReenigneArcher
commented
Nov 23, 2023

Copy link


Copy Markdown

Member
Anything remaining to be done on this issue?
Agreement from other team members to merge this.
@ReenigneArcher
ReenigneArcher
requested review from
cgutman
and removed request for
cgutman and ns6089January 1, 2024 03:18
@LizardByte-bot
LizardByte-bot
commented
Mar 31, 2024

Copy link


Copy Markdown

Member
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
utkarshdalal
commented
Mar 31, 2024
via email 

Copy link


Copy Markdown

ContributorAuthor
@ReenigneArcher
ReenigneArcher
commented
Apr 5, 2024

Copy link


Copy Markdown

Member
@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.
@ReenigneArcherReenigneArcher
mentioned this pull request
Feb 9, 2025
 Merged

10 tasks
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@ReenigneArcherReenigneArcherReenigneArcher left review comments
@cgutmancgutmanAwaiting requested review from cgutman

Assignees


No one assigned


Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

3 participants

@utkarshdalal@ReenigneArcher@LizardByte-bot

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Strip utm_, fbclid, gclid, etc. from all links on page\n(function() {\n var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content',\n 'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid',\n 'ref', 'ref_src', 'source', 'medium', 'campaign'];\n \n function cleanUrl(url) {\n try {\n var u = new URL(url, window.location.origin);\n var changed = false;\n trackingParams.forEach(function(p) {\n if (u.searchParams.has(p)) {\n u.searchParams.delete(p);\n changed = true;\n }\n });\n return changed ? u.toString() : url;\n } catch (e) {\n return url;\n }\n }\n \n function cleanLinks() {\n document.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n \n cleanLinks();\n \n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1) {\n if (node.tagName === 'A') cleanLinks();\n node.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Remove Tracking Parameters from Links");
}
} catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); }
})();
(function(){
try {
var __m = "youtube.com";
var __re = new RegExp('^' + "youtube\\.com" + '

Skip to content
Add max bitrate option - #1463
Closed
utkarshdalal wants to merge 26 commits into
LizardByte:nightlyfrom 
utkarshdalal:master
Closed
Add max bitrate option#1463
utkarshdalal wants to merge 26 commits into
LizardByte:nightlyfrom 
utkarshdalal:master
Conversation
@utkarshdalal
@utkarshdalalutkarshdalal
commented
Jul 23, 2023•

edited

Loading

Copy link


Copy Markdown

Contributor
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
image
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
ReenigneArcherand others
added 19 commits
October 30, 2022 15:05
@ReenigneArcher
Merge pull requestLizardByte#332from LizardByte/nightly
660d8e1
v0.15.0
@ReenigneArcher
Merge pull requestLizardByte#456from LizardByte/nightly
1e6d9da
v0.15.0 resubmit
@ReenigneArcher
Merge pull requestLizardByte#458from LizardByte/nightly
8509ee7
v0.15.0 resubmit
@ReenigneArcher
Merge pull requestLizardByte#460from LizardByte/nightly
e1b112c
v0.15.0 resubmit
@ReenigneArcher
Merge pull requestLizardByte#467from LizardByte/nightly
08d623c
v0.16.0
@ReenigneArcher
Merge pull requestLizardByte#567from LizardByte/nightly
b2fe042
v0.17.0
@ReenigneArcher
Merge pull requestLizardByte#738from LizardByte/nightly
b2c5da2
v0.18.0
@ReenigneArcher
Merge pull requestLizardByte#866from LizardByte/nightly
09dff34
v0.18.1
@ReenigneArcher
Merge pull requestLizardByte#877from LizardByte/nightly
446825b
v0.18.2
@ReenigneArcher
Merge pull requestLizardByte#929from LizardByte/nightly
9563419
v0.18.3
@ReenigneArcher
Merge pull requestLizardByte#954from LizardByte/nightly
0dfbcfd
v0.18.4
@ReenigneArcher
Merge pull requestLizardByte#969from LizardByte/nightly
483e225
v0.19.0
@ReenigneArcher
Merge pull requestLizardByte#1105from LizardByte/nightly
d70d084
v0.19.1
@ReenigneArcher
Merge pull requestLizardByte#1115from LizardByte/nightly
31e8b79
v0.20.0
Utkarsh Dalal
added max bitrate variable that overrides bitrate set by moonlight cl…
9cfa228
…ient.
Utkarsh Dalal
corrected syntax errors
6d39591
Utkarsh Dalal
updated variable in publish.cpp for windows to build in github action
23fbeae
Utkarsh Dalal
added empty line to force release
d8b62ff
Utkarsh Dalal
Updated description for max_bitrate param
72551fe
@github-actions
github-actionsBot
changed the base branch from
master
to
nightlyJuly 23, 2023 12:00
@github-actions
github-actionsBot
commented
Jul 23, 2023

Copy link


Copy Markdown

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
ReenigneArcher
requested changes
Jul 23, 2023
View reviewed changes
@ReenigneArcherReenigneArcher
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

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.
Comment threadsrc/platform/windows/publish.cpp
Outdated


extern "C" {
constexpr auto DNS_REQUEST_PENDING = 9506L;
constexpr auto MY_DNS_REQUEST_PENDING = 9506L;
@ReenigneArcherReenigneArcherJul 23, 2023

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

We don't really name variables "my_variable".
@utkarshdalalutkarshdalalJul 24, 2023

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

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.
image
I can name it something else also if you like, but this variable name was conflicting with a reserved variable name.
Comment threadREADME.rst
Outdated

Comment threadsrc/config.cpp
Outdated

Comment threadsrc_assets/common/assets/web/config.html
Outdated

Comment threadsrc_assets/common/assets/web/config.html
Outdated

@ReenigneArcher
ReenigneArcher
commented
Jul 23, 2023

Copy link


Copy Markdown

Member
Also, there was no change to the CI. Guessing you were working off the master branch, and fixed something we already fixed in nightly?
Utkarsh Dalal
added 2 commits
July 24, 2023 22:33
Utkarsh Dalal
Addressed code review comments
3fb13de
Utkarsh Dalal
Merge branch 'nightly' into add_max_bitrate
5f49733
# Conflicts:
#	src/platform/windows/publish.cpp
#	src_assets/common/assets/web/config.html
@utkarshdalal
utkarshdalal
commented
Jul 24, 2023

Copy link


Copy Markdown

ContributorAuthor
@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
ReenigneArcher
requested changes
Jul 24, 2023
View reviewed changes
@ReenigneArcherReenigneArcher
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

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?)
Comment threadsrc/config.cpp
 }, // supported resolutions

 { 10, 30, 60, 90, 120 }, // supported fps
 0, // max bitrate
@ReenigneArcherReenigneArcherJul 24, 2023

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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


Suggested change

0, // max bitrate
0, // max bitrate
2 spaces (linting rules)
Comment threadsrc_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
@ReenigneArcherReenigneArcherJul 24, 2023

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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


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.
Comment threadsrc/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);
@ReenigneArcherReenigneArcherJul 24, 2023

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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


Suggested change

int_f(vars, "max_bitrate", nvhttp.max_bitrate);
int_between_f(vars, "max_bitrate", nvhttp.max_bitrate, { 0, 150000 });
@utkarshdalal
utkarshdalal
commented
Jul 25, 2023

Copy link


Copy Markdown

ContributorAuthor
@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
ReenigneArcher
commented
Jul 25, 2023

Copy link


Copy Markdown

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

This comment was marked as resolved.

Sign in to view
@utkarshdalal
utkarshdalal
commented
Jul 29, 2023

Copy link


Copy Markdown

ContributorAuthor
@ReenigneArcher signed.
ReenigneArcher
ReenigneArcher
requested changes
Aug 18, 2023
View reviewed changes
@ReenigneArcherReenigneArcher
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

I found an additional item to change. Placeholder should be the default value.
Comment threadsrc_assets/common/assets/web/config.html
 type="number"
 class="form-control"
 id="max_bitrate"
 placeholder="5000"
@ReenigneArcherReenigneArcherAug 18, 2023

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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


Suggested change

 placeholder="5000"
 placeholder="0"
@utkarshdalalutkarshdalalAug 28, 2023

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

It's done, sorry for the delay I missed the notification.
Utkarsh Dalal
Merge branch 'nightly' into add_max_bitrate
3955d2d
# Conflicts:
#	docs/source/about/advanced_usage.rst
ReenigneArcher
ReenigneArcher
previously approved these changes
Sep 13, 2023
View reviewed changes
@ReenigneArcherReenigneArcher
mentioned this pull request
Oct 7, 2023

Closed

2 tasks
@ReenigneArcherReenigneArcher
changed the title
Add max bitrate option in Sunshine UIAdd max bitrate optionOct 7, 2023
@utkarshdalal
utkarshdalal
commented
Nov 22, 2023

Copy link


Copy Markdown

ContributorAuthor
Anything remaining to be done on this issue?
@ReenigneArcher
ReenigneArcher
commented
Nov 23, 2023

Copy link


Copy Markdown

Member
Anything remaining to be done on this issue?
Agreement from other team members to merge this.
@ReenigneArcher
ReenigneArcher
requested review from
cgutman
and removed request for
cgutman and ns6089January 1, 2024 03:18
@LizardByte-bot
LizardByte-bot
commented
Mar 31, 2024

Copy link


Copy Markdown

Member
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
utkarshdalal
commented
Mar 31, 2024
via email 

Copy link


Copy Markdown

ContributorAuthor
@ReenigneArcher
ReenigneArcher
commented
Apr 5, 2024

Copy link


Copy Markdown

Member
@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.
@ReenigneArcherReenigneArcher
mentioned this pull request
Feb 9, 2025
 Merged

10 tasks
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@ReenigneArcherReenigneArcherReenigneArcher left review comments
@cgutmancgutmanAwaiting requested review from cgutman

Assignees


No one assigned


Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

3 participants

@utkarshdalal@ReenigneArcher@LizardByte-bot

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Auto-enable theater mode on YouTube\n(function() {\n function tryTheater() {\n var btn = document.querySelector('button[aria-label=\"Theater mode\"], ytd-player #player button[title=\"Theater mode\"]');\n if (btn && !btn.classList.contains('activated')) {\n btn.click();\n }\n }\n \n // Try immediately\n tryTheater();\n \n // Try after navigation (SPA)\n var lastUrl = location.href;\n setInterval(function() {\n if (location.href !== lastUrl) {\n lastUrl = location.href;\n setTimeout(tryTheater, 500);\n }\n }, 1000);\n \n // Also try on player load\n var observer = new MutationObserver(tryTheater);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "YouTube Theater Mode Default");
}
} catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
Add max bitrate option - #1463
Closed
utkarshdalal wants to merge 26 commits into
LizardByte:nightlyfrom 
utkarshdalal:master
Closed
Add max bitrate option#1463
utkarshdalal wants to merge 26 commits into
LizardByte:nightlyfrom 
utkarshdalal:master
Conversation
@utkarshdalal
@utkarshdalalutkarshdalal
commented
Jul 23, 2023•

edited

Loading

Copy link


Copy Markdown

Contributor
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
image
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
ReenigneArcherand others
added 19 commits
October 30, 2022 15:05
@ReenigneArcher
Merge pull requestLizardByte#332from LizardByte/nightly
660d8e1
v0.15.0
@ReenigneArcher
Merge pull requestLizardByte#456from LizardByte/nightly
1e6d9da
v0.15.0 resubmit
@ReenigneArcher
Merge pull requestLizardByte#458from LizardByte/nightly
8509ee7
v0.15.0 resubmit
@ReenigneArcher
Merge pull requestLizardByte#460from LizardByte/nightly
e1b112c
v0.15.0 resubmit
@ReenigneArcher
Merge pull requestLizardByte#467from LizardByte/nightly
08d623c
v0.16.0
@ReenigneArcher
Merge pull requestLizardByte#567from LizardByte/nightly
b2fe042
v0.17.0
@ReenigneArcher
Merge pull requestLizardByte#738from LizardByte/nightly
b2c5da2
v0.18.0
@ReenigneArcher
Merge pull requestLizardByte#866from LizardByte/nightly
09dff34
v0.18.1
@ReenigneArcher
Merge pull requestLizardByte#877from LizardByte/nightly
446825b
v0.18.2
@ReenigneArcher
Merge pull requestLizardByte#929from LizardByte/nightly
9563419
v0.18.3
@ReenigneArcher
Merge pull requestLizardByte#954from LizardByte/nightly
0dfbcfd
v0.18.4
@ReenigneArcher
Merge pull requestLizardByte#969from LizardByte/nightly
483e225
v0.19.0
@ReenigneArcher
Merge pull requestLizardByte#1105from LizardByte/nightly
d70d084
v0.19.1
@ReenigneArcher
Merge pull requestLizardByte#1115from LizardByte/nightly
31e8b79
v0.20.0
Utkarsh Dalal
added max bitrate variable that overrides bitrate set by moonlight cl…
9cfa228
…ient.
Utkarsh Dalal
corrected syntax errors
6d39591
Utkarsh Dalal
updated variable in publish.cpp for windows to build in github action
23fbeae
Utkarsh Dalal
added empty line to force release
d8b62ff
Utkarsh Dalal
Updated description for max_bitrate param
72551fe
@github-actions
github-actionsBot
changed the base branch from
master
to
nightlyJuly 23, 2023 12:00
@github-actions
github-actionsBot
commented
Jul 23, 2023

Copy link


Copy Markdown

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
ReenigneArcher
requested changes
Jul 23, 2023
View reviewed changes
@ReenigneArcherReenigneArcher
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

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.
Comment threadsrc/platform/windows/publish.cpp
Outdated


extern "C" {
constexpr auto DNS_REQUEST_PENDING = 9506L;
constexpr auto MY_DNS_REQUEST_PENDING = 9506L;
@ReenigneArcherReenigneArcherJul 23, 2023

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

We don't really name variables "my_variable".
@utkarshdalalutkarshdalalJul 24, 2023

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

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.
image
I can name it something else also if you like, but this variable name was conflicting with a reserved variable name.
Comment threadREADME.rst
Outdated

Comment threadsrc/config.cpp
Outdated

Comment threadsrc_assets/common/assets/web/config.html
Outdated

Comment threadsrc_assets/common/assets/web/config.html
Outdated

@ReenigneArcher
ReenigneArcher
commented
Jul 23, 2023

Copy link


Copy Markdown

Member
Also, there was no change to the CI. Guessing you were working off the master branch, and fixed something we already fixed in nightly?
Utkarsh Dalal
added 2 commits
July 24, 2023 22:33
Utkarsh Dalal
Addressed code review comments
3fb13de
Utkarsh Dalal
Merge branch 'nightly' into add_max_bitrate
5f49733
# Conflicts:
#	src/platform/windows/publish.cpp
#	src_assets/common/assets/web/config.html
@utkarshdalal
utkarshdalal
commented
Jul 24, 2023

Copy link


Copy Markdown

ContributorAuthor
@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
ReenigneArcher
requested changes
Jul 24, 2023
View reviewed changes
@ReenigneArcherReenigneArcher
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

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?)
Comment threadsrc/config.cpp
 }, // supported resolutions

 { 10, 30, 60, 90, 120 }, // supported fps
 0, // max bitrate
@ReenigneArcherReenigneArcherJul 24, 2023

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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


Suggested change

0, // max bitrate
0, // max bitrate
2 spaces (linting rules)
Comment threadsrc_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
@ReenigneArcherReenigneArcherJul 24, 2023

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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


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.
Comment threadsrc/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);
@ReenigneArcherReenigneArcherJul 24, 2023

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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


Suggested change

int_f(vars, "max_bitrate", nvhttp.max_bitrate);
int_between_f(vars, "max_bitrate", nvhttp.max_bitrate, { 0, 150000 });
@utkarshdalal
utkarshdalal
commented
Jul 25, 2023

Copy link


Copy Markdown

ContributorAuthor
@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
ReenigneArcher
commented
Jul 25, 2023

Copy link


Copy Markdown

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

This comment was marked as resolved.

Sign in to view
@utkarshdalal
utkarshdalal
commented
Jul 29, 2023

Copy link


Copy Markdown

ContributorAuthor
@ReenigneArcher signed.
ReenigneArcher
ReenigneArcher
requested changes
Aug 18, 2023
View reviewed changes
@ReenigneArcherReenigneArcher
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

I found an additional item to change. Placeholder should be the default value.
Comment threadsrc_assets/common/assets/web/config.html
 type="number"
 class="form-control"
 id="max_bitrate"
 placeholder="5000"
@ReenigneArcherReenigneArcherAug 18, 2023

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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


Suggested change

 placeholder="5000"
 placeholder="0"
@utkarshdalalutkarshdalalAug 28, 2023

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

It's done, sorry for the delay I missed the notification.
Utkarsh Dalal
Merge branch 'nightly' into add_max_bitrate
3955d2d
# Conflicts:
#	docs/source/about/advanced_usage.rst
ReenigneArcher
ReenigneArcher
previously approved these changes
Sep 13, 2023
View reviewed changes
@ReenigneArcherReenigneArcher
mentioned this pull request
Oct 7, 2023

Closed

2 tasks
@ReenigneArcherReenigneArcher
changed the title
Add max bitrate option in Sunshine UIAdd max bitrate optionOct 7, 2023
@utkarshdalal
utkarshdalal
commented
Nov 22, 2023

Copy link


Copy Markdown

ContributorAuthor
Anything remaining to be done on this issue?
@ReenigneArcher
ReenigneArcher
commented
Nov 23, 2023

Copy link


Copy Markdown

Member
Anything remaining to be done on this issue?
Agreement from other team members to merge this.
@ReenigneArcher
ReenigneArcher
requested review from
cgutman
and removed request for
cgutman and ns6089January 1, 2024 03:18
@LizardByte-bot
LizardByte-bot
commented
Mar 31, 2024

Copy link


Copy Markdown

Member
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
utkarshdalal
commented
Mar 31, 2024
via email 

Copy link


Copy Markdown

ContributorAuthor
@ReenigneArcher
ReenigneArcher
commented
Apr 5, 2024

Copy link


Copy Markdown

Member
@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.
@ReenigneArcherReenigneArcher
mentioned this pull request
Feb 9, 2025
 Merged

10 tasks
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@ReenigneArcherReenigneArcherReenigneArcher left review comments
@cgutmancgutmanAwaiting requested review from cgutman

Assignees


No one assigned


Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

3 participants

@utkarshdalal@ReenigneArcher@LizardByte-bot

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Remove or un-stick sticky/fixed headers that block content\n(function() {\n function unstick() {\n document.querySelectorAll('header, nav, [role=\"banner\"], .header, .navbar, .sticky, .fixed-top, [style*=\"position: fixed\"], [style*=\"position:sticky\"]').forEach(function(el) {\n if (el.style.position === 'fixed' || el.style.position === 'sticky' || \n getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') {\n el.style.position = 'static';\n el.style.top = 'auto';\n el.style.zIndex = 'auto';\n }\n });\n }\n \n unstick();\n \n var observer = new MutationObserver(unstick);\n observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] });\n})();", "Kill Sticky Headers");
}
} catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
Add max bitrate option - #1463
Closed
utkarshdalal wants to merge 26 commits into
LizardByte:nightlyfrom 
utkarshdalal:master
Closed
Add max bitrate option#1463
utkarshdalal wants to merge 26 commits into
LizardByte:nightlyfrom 
utkarshdalal:master
Conversation
@utkarshdalal
@utkarshdalalutkarshdalal
commented
Jul 23, 2023•

edited

Loading

Copy link


Copy Markdown

Contributor
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
image
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
ReenigneArcherand others
added 19 commits
October 30, 2022 15:05
@ReenigneArcher
Merge pull requestLizardByte#332from LizardByte/nightly
660d8e1
v0.15.0
@ReenigneArcher
Merge pull requestLizardByte#456from LizardByte/nightly
1e6d9da
v0.15.0 resubmit
@ReenigneArcher
Merge pull requestLizardByte#458from LizardByte/nightly
8509ee7
v0.15.0 resubmit
@ReenigneArcher
Merge pull requestLizardByte#460from LizardByte/nightly
e1b112c
v0.15.0 resubmit
@ReenigneArcher
Merge pull requestLizardByte#467from LizardByte/nightly
08d623c
v0.16.0
@ReenigneArcher
Merge pull requestLizardByte#567from LizardByte/nightly
b2fe042
v0.17.0
@ReenigneArcher
Merge pull requestLizardByte#738from LizardByte/nightly
b2c5da2
v0.18.0
@ReenigneArcher
Merge pull requestLizardByte#866from LizardByte/nightly
09dff34
v0.18.1
@ReenigneArcher
Merge pull requestLizardByte#877from LizardByte/nightly
446825b
v0.18.2
@ReenigneArcher
Merge pull requestLizardByte#929from LizardByte/nightly
9563419
v0.18.3
@ReenigneArcher
Merge pull requestLizardByte#954from LizardByte/nightly
0dfbcfd
v0.18.4
@ReenigneArcher
Merge pull requestLizardByte#969from LizardByte/nightly
483e225
v0.19.0
@ReenigneArcher
Merge pull requestLizardByte#1105from LizardByte/nightly
d70d084
v0.19.1
@ReenigneArcher
Merge pull requestLizardByte#1115from LizardByte/nightly
31e8b79
v0.20.0
Utkarsh Dalal
added max bitrate variable that overrides bitrate set by moonlight cl…
9cfa228
…ient.
Utkarsh Dalal
corrected syntax errors
6d39591
Utkarsh Dalal
updated variable in publish.cpp for windows to build in github action
23fbeae
Utkarsh Dalal
added empty line to force release
d8b62ff
Utkarsh Dalal
Updated description for max_bitrate param
72551fe
@github-actions
github-actionsBot
changed the base branch from
master
to
nightlyJuly 23, 2023 12:00
@github-actions
github-actionsBot
commented
Jul 23, 2023

Copy link


Copy Markdown

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
ReenigneArcher
requested changes
Jul 23, 2023
View reviewed changes
@ReenigneArcherReenigneArcher
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

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.
Comment threadsrc/platform/windows/publish.cpp
Outdated


extern "C" {
constexpr auto DNS_REQUEST_PENDING = 9506L;
constexpr auto MY_DNS_REQUEST_PENDING = 9506L;
@ReenigneArcherReenigneArcherJul 23, 2023

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

We don't really name variables "my_variable".
@utkarshdalalutkarshdalalJul 24, 2023

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

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.
image
I can name it something else also if you like, but this variable name was conflicting with a reserved variable name.
Comment threadREADME.rst
Outdated

Comment threadsrc/config.cpp
Outdated

Comment threadsrc_assets/common/assets/web/config.html
Outdated

Comment threadsrc_assets/common/assets/web/config.html
Outdated

@ReenigneArcher
ReenigneArcher
commented
Jul 23, 2023

Copy link


Copy Markdown

Member
Also, there was no change to the CI. Guessing you were working off the master branch, and fixed something we already fixed in nightly?
Utkarsh Dalal
added 2 commits
July 24, 2023 22:33
Utkarsh Dalal
Addressed code review comments
3fb13de
Utkarsh Dalal
Merge branch 'nightly' into add_max_bitrate
5f49733
# Conflicts:
#	src/platform/windows/publish.cpp
#	src_assets/common/assets/web/config.html
@utkarshdalal
utkarshdalal
commented
Jul 24, 2023

Copy link


Copy Markdown

ContributorAuthor
@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
ReenigneArcher
requested changes
Jul 24, 2023
View reviewed changes
@ReenigneArcherReenigneArcher
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

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?)
Comment threadsrc/config.cpp
 }, // supported resolutions

 { 10, 30, 60, 90, 120 }, // supported fps
 0, // max bitrate
@ReenigneArcherReenigneArcherJul 24, 2023

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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


Suggested change

0, // max bitrate
0, // max bitrate
2 spaces (linting rules)
Comment threadsrc_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
@ReenigneArcherReenigneArcherJul 24, 2023

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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


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.
Comment threadsrc/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);
@ReenigneArcherReenigneArcherJul 24, 2023

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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


Suggested change

int_f(vars, "max_bitrate", nvhttp.max_bitrate);
int_between_f(vars, "max_bitrate", nvhttp.max_bitrate, { 0, 150000 });
@utkarshdalal
utkarshdalal
commented
Jul 25, 2023

Copy link


Copy Markdown

ContributorAuthor
@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
ReenigneArcher
commented
Jul 25, 2023

Copy link


Copy Markdown

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

This comment was marked as resolved.

Sign in to view
@utkarshdalal
utkarshdalal
commented
Jul 29, 2023

Copy link


Copy Markdown

ContributorAuthor
@ReenigneArcher signed.
ReenigneArcher
ReenigneArcher
requested changes
Aug 18, 2023
View reviewed changes
@ReenigneArcherReenigneArcher
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

I found an additional item to change. Placeholder should be the default value.
Comment threadsrc_assets/common/assets/web/config.html
 type="number"
 class="form-control"
 id="max_bitrate"
 placeholder="5000"
@ReenigneArcherReenigneArcherAug 18, 2023

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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


Suggested change

 placeholder="5000"
 placeholder="0"
@utkarshdalalutkarshdalalAug 28, 2023

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

It's done, sorry for the delay I missed the notification.
Utkarsh Dalal
Merge branch 'nightly' into add_max_bitrate
3955d2d
# Conflicts:
#	docs/source/about/advanced_usage.rst
ReenigneArcher
ReenigneArcher
previously approved these changes
Sep 13, 2023
View reviewed changes
@ReenigneArcherReenigneArcher
mentioned this pull request
Oct 7, 2023

Closed

2 tasks
@ReenigneArcherReenigneArcher
changed the title
Add max bitrate option in Sunshine UIAdd max bitrate optionOct 7, 2023
@utkarshdalal
utkarshdalal
commented
Nov 22, 2023

Copy link


Copy Markdown

ContributorAuthor
Anything remaining to be done on this issue?
@ReenigneArcher
ReenigneArcher
commented
Nov 23, 2023

Copy link


Copy Markdown

Member
Anything remaining to be done on this issue?
Agreement from other team members to merge this.
@ReenigneArcher
ReenigneArcher
requested review from
cgutman
and removed request for
cgutman and ns6089January 1, 2024 03:18
@LizardByte-bot
LizardByte-bot
commented
Mar 31, 2024

Copy link


Copy Markdown

Member
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
utkarshdalal
commented
Mar 31, 2024
via email 

Copy link


Copy Markdown

ContributorAuthor
@ReenigneArcher
ReenigneArcher
commented
Apr 5, 2024

Copy link


Copy Markdown

Member
@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.
@ReenigneArcherReenigneArcher
mentioned this pull request
Feb 9, 2025
 Merged

10 tasks
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@ReenigneArcherReenigneArcherReenigneArcher left review comments
@cgutmancgutmanAwaiting requested review from cgutman

Assignees


No one assigned


Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

3 participants

@utkarshdalal@ReenigneArcher@LizardByte-bot

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Universal Dark Mode - works on any site\n(function() {\n var enabled = true;\n \n function applyDarkMode() {\n if (!enabled) return;\n \n // Create style element if it doesn't exist\n var style = document.getElementById('universal-dark-mode-style');\n if (!style) {\n style = document.createElement('style');\n style.id = 'universal-dark-mode-style';\n document.head.appendChild(style);\n }\n \n // Dark mode CSS - inverts colors but preserves images/video\n style.textContent = '\n /* Invert everything except media */\n html {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #1a1a2e !important;\n }\n \n /* Restore images, videos, iframes, canvas */\n img, video, iframe, canvas, svg, picture, [style*=\"background-image\"] {\n filter: invert(1) hue-rotate(180deg) !important;\n }\n \n /* Preserve specific elements that should not be inverted */\n .no-dark-mode, .no-dark-mode *,\n [data-theme=\"light\"], [data-theme=\"light\"],\n .ace_editor, .ace_editor *,\n .CodeMirror, .CodeMirror *,\n .monaco-editor, .monaco-editor *,\n .markdown-body pre, .markdown-body pre *,\n .highlight, .highlight *,\n pre code, pre code * {\n filter: none !important;\n }\n \n /* Fix common UI elements */\n .modal, .popup, .dropdown-menu, .tooltip, .popover {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #2d2d44 !important;\n border-color: #444 !important;\n }\n \n /* Scrollbars */\n ::-webkit-scrollbar { background: #1a1a2e !important; }\n ::-webkit-scrollbar-thumb { background: #444 !important; }\n ::-webkit-scrollbar-thumb:hover { background: #555 !important; }\n \n /* Selection */\n ::selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ::-moz-selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ';\n }\n \n function removeDarkMode() {\n var style = document.getElementById('universal-dark-mode-style');\n if (style) style.remove();\n }\n \n // Toggle with Alt+Shift+D\n document.addEventListener('keydown', function(e) {\n if (e.altKey && e.shiftKey && e.key === 'D') {\n e.preventDefault();\n enabled = !enabled;\n if (enabled) {\n applyDarkMode();\n console.log('[Universal Dark Mode] Enabled');\n } else {\n removeDarkMode();\n console.log('[Universal Dark Mode] Disabled');\n }\n }\n });\n \n // Apply on load\n applyDarkMode();\n \n // Re-apply on dynamic content\n var observer = new MutationObserver(function(mutations) {\n if (enabled && !document.getElementById('universal-dark-mode-style')) {\n applyDarkMode();\n }\n });\n observer.observe(document.head, { childList: true });\n \n console.log('[Universal Dark Mode] Loaded - Press Alt+Shift+D to toggle');\n})();", "Universal Dark Mode");
}
} catch(__e) { console.warn('[Userscript:Universal Dark Mode]', __e); }
})();
})();

Skip to content
Add max bitrate option - #1463
Closed
utkarshdalal wants to merge 26 commits into
LizardByte:nightlyfrom 
utkarshdalal:master
Closed
Add max bitrate option#1463
utkarshdalal wants to merge 26 commits into
LizardByte:nightlyfrom 
utkarshdalal:master
Conversation
@utkarshdalal
@utkarshdalalutkarshdalal
commented
Jul 23, 2023•

edited

Loading

Copy link


Copy Markdown

Contributor
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
image
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
ReenigneArcherand others
added 19 commits
October 30, 2022 15:05
@ReenigneArcher
Merge pull requestLizardByte#332from LizardByte/nightly
660d8e1
v0.15.0
@ReenigneArcher
Merge pull requestLizardByte#456from LizardByte/nightly
1e6d9da
v0.15.0 resubmit
@ReenigneArcher
Merge pull requestLizardByte#458from LizardByte/nightly
8509ee7
v0.15.0 resubmit
@ReenigneArcher
Merge pull requestLizardByte#460from LizardByte/nightly
e1b112c
v0.15.0 resubmit
@ReenigneArcher
Merge pull requestLizardByte#467from LizardByte/nightly
08d623c
v0.16.0
@ReenigneArcher
Merge pull requestLizardByte#567from LizardByte/nightly
b2fe042
v0.17.0
@ReenigneArcher
Merge pull requestLizardByte#738from LizardByte/nightly
b2c5da2
v0.18.0
@ReenigneArcher
Merge pull requestLizardByte#866from LizardByte/nightly
09dff34
v0.18.1
@ReenigneArcher
Merge pull requestLizardByte#877from LizardByte/nightly
446825b
v0.18.2
@ReenigneArcher
Merge pull requestLizardByte#929from LizardByte/nightly
9563419
v0.18.3
@ReenigneArcher
Merge pull requestLizardByte#954from LizardByte/nightly
0dfbcfd
v0.18.4
@ReenigneArcher
Merge pull requestLizardByte#969from LizardByte/nightly
483e225
v0.19.0
@ReenigneArcher
Merge pull requestLizardByte#1105from LizardByte/nightly
d70d084
v0.19.1
@ReenigneArcher
Merge pull requestLizardByte#1115from LizardByte/nightly
31e8b79
v0.20.0
Utkarsh Dalal
added max bitrate variable that overrides bitrate set by moonlight cl…
9cfa228
…ient.
Utkarsh Dalal
corrected syntax errors
6d39591
Utkarsh Dalal
updated variable in publish.cpp for windows to build in github action
23fbeae
Utkarsh Dalal
added empty line to force release
d8b62ff
Utkarsh Dalal
Updated description for max_bitrate param
72551fe
@github-actions
github-actionsBot
changed the base branch from
master
to
nightlyJuly 23, 2023 12:00
@github-actions
github-actionsBot
commented
Jul 23, 2023

Copy link


Copy Markdown

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
ReenigneArcher
requested changes
Jul 23, 2023
View reviewed changes
@ReenigneArcherReenigneArcher
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

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.
Comment threadsrc/platform/windows/publish.cpp
Outdated


extern "C" {
constexpr auto DNS_REQUEST_PENDING = 9506L;
constexpr auto MY_DNS_REQUEST_PENDING = 9506L;
@ReenigneArcherReenigneArcherJul 23, 2023

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

We don't really name variables "my_variable".
@utkarshdalalutkarshdalalJul 24, 2023

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

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.
image
I can name it something else also if you like, but this variable name was conflicting with a reserved variable name.
Comment threadREADME.rst
Outdated

Comment threadsrc/config.cpp
Outdated

Comment threadsrc_assets/common/assets/web/config.html
Outdated

Comment threadsrc_assets/common/assets/web/config.html
Outdated

@ReenigneArcher
ReenigneArcher
commented
Jul 23, 2023

Copy link


Copy Markdown

Member
Also, there was no change to the CI. Guessing you were working off the master branch, and fixed something we already fixed in nightly?
Utkarsh Dalal
added 2 commits
July 24, 2023 22:33
Utkarsh Dalal
Addressed code review comments
3fb13de
Utkarsh Dalal
Merge branch 'nightly' into add_max_bitrate
5f49733
# Conflicts:
#	src/platform/windows/publish.cpp
#	src_assets/common/assets/web/config.html
@utkarshdalal
utkarshdalal
commented
Jul 24, 2023

Copy link


Copy Markdown

ContributorAuthor
@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
ReenigneArcher
requested changes
Jul 24, 2023
View reviewed changes
@ReenigneArcherReenigneArcher
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

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?)
Comment threadsrc/config.cpp
 }, // supported resolutions

 { 10, 30, 60, 90, 120 }, // supported fps
 0, // max bitrate
@ReenigneArcherReenigneArcherJul 24, 2023

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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


Suggested change

0, // max bitrate
0, // max bitrate
2 spaces (linting rules)
Comment threadsrc_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
@ReenigneArcherReenigneArcherJul 24, 2023

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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


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.
Comment threadsrc/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);
@ReenigneArcherReenigneArcherJul 24, 2023

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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


Suggested change

int_f(vars, "max_bitrate", nvhttp.max_bitrate);
int_between_f(vars, "max_bitrate", nvhttp.max_bitrate, { 0, 150000 });
@utkarshdalal
utkarshdalal
commented
Jul 25, 2023

Copy link


Copy Markdown

ContributorAuthor
@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
ReenigneArcher
commented
Jul 25, 2023

Copy link


Copy Markdown

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

This comment was marked as resolved.

Sign in to view
@utkarshdalal
utkarshdalal
commented
Jul 29, 2023

Copy link


Copy Markdown

ContributorAuthor
@ReenigneArcher signed.
ReenigneArcher
ReenigneArcher
requested changes
Aug 18, 2023
View reviewed changes
@ReenigneArcherReenigneArcher
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

I found an additional item to change. Placeholder should be the default value.
Comment threadsrc_assets/common/assets/web/config.html
 type="number"
 class="form-control"
 id="max_bitrate"
 placeholder="5000"
@ReenigneArcherReenigneArcherAug 18, 2023

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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


Suggested change

 placeholder="5000"
 placeholder="0"
@utkarshdalalutkarshdalalAug 28, 2023

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

It's done, sorry for the delay I missed the notification.
Utkarsh Dalal
Merge branch 'nightly' into add_max_bitrate
3955d2d
# Conflicts:
#	docs/source/about/advanced_usage.rst
ReenigneArcher
ReenigneArcher
previously approved these changes
Sep 13, 2023
View reviewed changes
@ReenigneArcherReenigneArcher
mentioned this pull request
Oct 7, 2023

Closed

2 tasks
@ReenigneArcherReenigneArcher
changed the title
Add max bitrate option in Sunshine UIAdd max bitrate optionOct 7, 2023
@utkarshdalal
utkarshdalal
commented
Nov 22, 2023

Copy link


Copy Markdown

ContributorAuthor
Anything remaining to be done on this issue?
@ReenigneArcher
ReenigneArcher
commented
Nov 23, 2023

Copy link


Copy Markdown

Member
Anything remaining to be done on this issue?
Agreement from other team members to merge this.
@ReenigneArcher
ReenigneArcher
requested review from
cgutman
and removed request for
cgutman and ns6089January 1, 2024 03:18
@LizardByte-bot
LizardByte-bot
commented
Mar 31, 2024

Copy link


Copy Markdown

Member
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
utkarshdalal
commented
Mar 31, 2024
via email 

Copy link


Copy Markdown

ContributorAuthor
@ReenigneArcher
ReenigneArcher
commented
Apr 5, 2024

Copy link


Copy Markdown

Member
@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.
@ReenigneArcherReenigneArcher
mentioned this pull request
Feb 9, 2025
 Merged

10 tasks
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@ReenigneArcherReenigneArcherReenigneArcher left review comments
@cgutmancgutmanAwaiting requested review from cgutman

Assignees


No one assigned


Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

3 participants

@utkarshdalal@ReenigneArcher@LizardByte-bot