-
-
Notifications
You must be signed in to change notification settings - Fork 7.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
obs-outputs: Add dynamic variable bitrate #1086
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi! I've reviewed this PR.
Normally, I would wait to do a style review on a PR that's actively seeking feedback or reviews on functional stuff, but I've found that if I put it off, I'll just forget.
There's a few other places that I haven't commented on yet for long lines, partly because they might get refactored, and partly because I'm not certain how best to line break them.
In addition to my Consider putting blank lines before and after if
/else
blocks to make them stand out better from the surrounding code.
Lastly, please change the commit subject's prefix to "obs-outputs" and make sure the commit subject starts with a capitalized verb, perhaps like this:
obs-outputs: Add dynamic variable bitrate
thanks again ! |
On HoldFollowing a suggestion of Xaymar, I'll add enc-amf support. For now this plugin works only for x264. Edit: |
PR UpdatedSupport for enc-amf and QSV added. |
Update and to do
edit: last task done (02/22/18) |
Update (02/22/2018) Applied the suggestions in the various reviews. NOTE (reminder): |
|
|
|
|
Oh, about the theme icons, I mean - custom (user) themes, for example: https://obsproject.com/forum/resources/categories/themes.10/ The description is nice. Unfortunately it is huge. And thus tool tip with all this info is a bad idea. Also, user may don't know about the encoder's buffer, it fullness etc. Let's imagine that All what I (user) see is: auto option; current bitrate value; up/down arrows; frame drop or quality drops (from stream watchers feedback). And all this, is connected to user settings of the mentioned option (no matter how it works). |
|
In general I don't know how to select exact object. QLabel is selectable but all 3 states (up/down/static) is QPixmap picture that replaces each other. Thus I can give only one style to QLabel itself but not designate it states. After picture updates, the style needs to be updated in the code too (at least for this element), the simplest way is to set style with empty comment string. Also, to be able to address each state I need unique name for it (the simplest way is to add string to properties, for example "themeID"="up_bitrate") this name should change as soon as new state switched, but before style update. Thus it will be possible to use .qss:
Also, it is desirable to have unique and predefined "name" property for the widget (QLabel). Because potentially you can share same picture to other widgets and adjust only size of it for each widget type individually. The chance to be implemented is small because I'm talking about the changes that were not accepted. It is different, but the root is the same. #856 |
Update 03/14/18: fixed some UI bugs (never use Qt Creator on obs ui files !) |
Upon discussion in the discord, I actually think this feature should be enabled by default. For the average user, I think they would prefer to degrade quality and not drop frames rather than drop frames with static quality. |
Update: |
I found using the default NVENC encoder rather than the one marked (new) didn't work as desired with dynamic variable bitrate. |
I just compiled it for 64 bit windows: https://drive.google.com/file/d/17tO5KRQaSUFxcmlUtYVdagbcXazrc3VU/view |
Firstly, it's true that the new nvenc is better than the other encoders as to dynamic bitrate.
|
update
|
If some pro can compile every new Version of Obs with this fantastic feature would be fantastic for nabs people like me 😋 thx anyway and amazing work |
f024afa
to
789bc11
Compare
6b2f09d
to
7fb1e77
Compare
update 03/30/19
For the sake of comparison I've left in the code an implementation of R1CH algo (adjust_bitrate-r1ch) written by Jim; it will have to be removed eventually. Here's a comparison with R1CH original algo and the PR algo:
For details of the comparison between the algos with screen captures, check: https://docdro.id/Z5lwYX8 |
Also modifies UI. Extensive rewrite of R1CH code from OBS Classic with some changes from Woodbyte fork. Strain parameter has been replaced by rtmp congestion. This allows compatibility with all platforms (win, macOS, linux). Default parameters have been chosen for best compatibility with Twitch.
This tags both nvenc encoders as supporting dynamic bitrate.
This hasn't been touched in a month, could this be re-reviewed please? I have been using this build of OBS for a while now and I'd love to be able to switch back to mainline. |
It hasn't been touched because work has been more on getting 23.2 out the door. That said, Jim has been doing quite a bit of testing and tweaking with the algorithm internally lately. No need to be impatient. |
For clarification I'm not implementing it myself, I just modified the algorithm. I'll be contributing to this PR and adding some updates. I'll also update it for clang-format. |
feature coded by Jim with improvements has been merged so closing this. |
Suggestions link : here
Updated (5/5/2018)
Partly based on R1CH code from OBS Classic with some changes from Woodbyte fork.
I'm removing the experimental tag since it's working quite well.
The algo has seen a series of improvements, notably support for all h264 encoders supported by obs
(x264, enc-amf, nvenc, mac-vth264, quicksync, etc).
warning
What this does:
Some screenshots
(1) enable dynamic bitrate in Simple output
(2) Advanced settings for dynamic bitrate
(3) Bitrate decrease from 2500 kbs due to congestion (downward red arrow)
(4) Bitrate stationary during congestion (not increasing yet because congestion hasn't cleared, arrow is now orange and not red as in this screenshot, see discussion with Dodgepong)
(5) Bitrate increasing due to congestion clearing (upward green arrow (red in this screenshot))