-
-
Notifications
You must be signed in to change notification settings - Fork 8.1k
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
UI: Add pre-stream encoder setup wizard #3515
Conversation
** First CI notes to self:**
Windows Linux |
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.
Looks good! I'm no expert in Qt so I glossed over most of that. Let me know if there's things you'd like me to take a closer look at.
43adf52
to
89eb3e2
Compare
a16e52a
to
36c05db
Compare
This product is now viable. I'm requesting testers on Windows and Linux please. |
Will test on Windows and Linux when I get an opportunity. |
36c05db
to
5414d37
Compare
rebased on to |
8b070eb
to
97e361a
Compare
Is this consistently failing?
This is server side / timeout.
…On Wed, Oct 14, 2020 at 14:46 RytoEX ***@***.***> wrote:
While attempting to test this on Windows, I got this error message.
[image: 2020-10-14 17_45_00-Verify Stream Settings]
<https://user-images.githubusercontent.com/624931/96048765-0a1ae880-0e45-11eb-8d4e-d4b0d10372e5.png>
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#3515 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAMB7NHAHXNBHTWL7ZTIOQ3SKYL3BANCNFSM4R6EGL5A>
.
|
Also: thank you testing!
…On Wed, Oct 14, 2020 at 14:46 RytoEX ***@***.***> wrote:
While attempting to test this on Windows, I got this error message.
[image: 2020-10-14 17_45_00-Verify Stream Settings]
<https://user-images.githubusercontent.com/624931/96048765-0a1ae880-0e45-11eb-8d4e-d4b0d10372e5.png>
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#3515 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAMB7NHAHXNBHTWL7ZTIOQ3SKYL3BANCNFSM4R6EGL5A>
.
|
Another thing we had noticed was that this appears to assume the encoder is x264, and returns x264-specific recommendations and settings (or rather, settings that only x264 has options to even change/adjust). How is this planned to be handled for hardware encoders, such as NVENC or QSV which might not support these options? |
Hide buttons that are disabled for better user clarity.
Applies selected settings to the simple encoder setup. Use the Custom Encoder Settings to set specific settings. Additionally, changes the output resolution so the canvas layout is not changed possibly right before a stream. Next to add gating so this wizard only shows in output Simple Mode.
Show pre-stream wizard only when in simple mode since it only operates on simple mode for now. Later, will look into expanding capability. Additionally, fix a bug where was making a copy of a QPair instead of creating a reference so settings were not being written.
97e361a
to
fd92b24
Compare
Removed QtNetwork dependency. Tested and works on Mac. Windows machine is not cooperating with me right now. |
I'll check it out. |
@jp9000 Is there any TLS error this time? |
I'm pretty sure it returned the json from the remote successfully, it's just not populating the final list box for some reason, haven't delved into it deeply. |
When I use the CI build it works correctly... Does it sign differently that could affect TLS ? |
OBS does not ship OpenSSL and has a curl helper already to use.
fd92b24
to
6e53c89
Compare
Updated branch with handling of empty/no data more clearly. |
After debugging, I discovered the reason for the bug: in pre-stream-current-settings.hpp, you use But that's not quite the exact reason why this failed; the reason why this failed is because you use For your global // header file
struct MyStruct {
const char *str1;
const char *str2;
};
extern MyStruct myVar; // source file
MyStruct myVar = {"string 1", "string 2"}; |
The only information I can find about this is the Level Up program mentioned in these two help articles:
There, the recommended bitrate is still "6mbps", and I don't see any mention of a higher bitrate limit. Also, the API is still clamping to a maximum of 30 FPS, despite 60 FPS being permitted when eligible per the above. The API does not seem to care if I set the input_video_bitrate to 80000 Kbps, whereas this would be clamped to 6000 due to specifications from services.json (more on that in a moment). On a default OBS installation, "Enforce streaming service bitrate limits" ([SimpleOutput] EnforceBitrate) defaults to true/enabled, which will force the settings to adhere to the service limits provided in services.json. In Facebook's case, that's:
The audio bitrate setting sent from the API will be overridden by When testing this with a handcrafted x264Settings/x264opts string (the PR still doesn't work for me on Windows, now getting a blank wizard settings window), NVENC and QSV do not use the options. When using x264, I get this:
As you can see, the audio bitrate has been overridden due to the specification in services.json. It appears that Echoing concerns above, a lot of the fine-tuning here seems x264-specific. On systems with NVENC available, OBS defaults to NVENC, so these settings would not take effect. Given that, the apparent prevalence of NVENC-capable systems using OBS, and widespread recommendations that users use NVENC when possible, I'm much more interested in seeing NVENC settings fleshed out. I noticed you mentioned that support for NVENC and Apple Hardware Encoder would come later. Do you have any plans to provide fine tuned settings for QuickSync, AMF, or Apple's Software Encoder? I'm not clear on the benefit of specifying
While OBS still produces a stream in this case, I do not understand why specifying level is required in the first place when the proper minimum level will be auto-selected otherwise. |
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.
I wanted to leave some questions and notes before I forget to do so.
This is usually for like Level Up, this is for specific users we've discussed with e.g., Pro Gamers, TV.
This is being discussed internally but in the mean time one of the reason the settings shows as options you can turn off, for those that know to use it. Like I said above, I might change to make it more hidden in a future commit.
At 720 it will clamp. At 1080 we don't clamp, this is because OBS is not the only user of the API and 1080 was requested not to have ceiling by both other integration partners and our internal team. Other providers of the wizard can clamp as they see fit and I imagine they will. We may as well later.
Right now Audio settings are not applied in OBS's implementation.
I had a hard time with documentation on what the ingest team wanted here, I'm actually going to remove this for this PR.
Fixed. Thanks for that.
Me too. Working on it. But these are settings our ingest servers work with best so are purposefully generalized for now. We are looking to add more API inputs for the specificity but friction from both vendors and internal teams makes the API changes slow so it will not be in the first pass. Are there any analytics on NVEC enabled systems OBS can track to verify how used it is?
In simple mode Apple's Encoders aren't available and are also not recommended for streaming. Will reavaluate after. There's a decent amount of work in Setting's structure to be done to be able to change Advanced controls in a wizard imho.
We may remove that from the API side, we have talked about it. But the limit was on purpose from the encoding team. Like above, other users of the UI wouldn't have to set it at all. |
I see the clamping as described on 720p. However, at 1080p, it will still clamp the framerate to 30 and the GOP size (keyint) to 60. This means that a user would have to disable both of these settings or end up with settings that are not recommended (60 FPS with keyint=60). My comments about the lack of bitrate clamping is related to you specifying the level parameter, because you can exceed the bitrate limit for level 4.1 if you go above 62500 kbps. Similarly, 1920x1080@60 is outside the constraints of level 4.1 - you would need at least level 4.2. I don't know what maximums you allow for other users, but this is one of the reasons why it's better to just leave level at auto. There are possible repercussions to this sort of invalid set of parameters, so I'd think it's better to just not end up with invalid parameters in the first place by just leaving the level at auto. The encoder should auto-select 4.1 if the stream settings are within the constraints of 4.1 anyway.
This comes back to, paraphrased, "the API is recommending these settings but not all users should apply them and those users will just have to know better what they should and should not apply" which has been expressed by @WizardCM , @dodgepong , and @Fenrirthviti above. I'm sort of inclined to agree with what @Fenrirthviti said:
That said, perhaps better wording, and maybe tooltips for each setting, on the settings page to indicate that the user can select/deselect or choose specific settings to apply would help here. Right now, the settings are just presented as "recommended" and that's it. Most users will probably just accept everything that's listed there. Some users will undoubtedly uncheck a few things, like perhaps framerate, but then leave everything else enabled.
I don't understand this limit though. As far as I know, OBS will still push out a stream of 80000kbps video at 1920x1080@60 with level 4.1 set even though those parameters exceed what 4.1 is supposed to handle. On the other hand, decoders might reject or fail to display such a stream because it has invalid parameters. |
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.
With VS2019 on Windows 10, the x264_c_String
ends up as a garbage string. See embedded review note for more info.
Is UI/pre-stream-wizard/CMakeLists.txt
necessary to include?
Various fixes.
3a7da4b
to
bdc3a3e
Compare
So I want to go over this again in a more formal capacity on the PR. The thing I'm a bit concerned about is this basically being similar functionality to the auto-configuration wizard; I agree with others who have stated similar things. As stated in your original plans, this is something you want to combine with the auto-configuration wizard, so my primary concern is the idea of adding this, and then basically combining them later: why not improve the existing wizard from the start? I know that this is just the start of your plans, and that there are many plans to more thoroughly integrate it, but I can't help but to feel like as it is right now, it's currently duplicating a bit too much of the existing functionality. Are you sure that we should duplicate and combine later like this? Shouldn't we be trying to integrate it into the current wizard if that's the intention later on, rather than creating a new, separate wizard with very similar purpose? Another thing that makes me feel a bit uncomfortable is how it's accessed, which is currently via the stream section of settings. I know that you want to expand this eventually, but right now, as it is currently implemented in the PR, user access to it is very limited, and they are unlikely ever to change it again after they press it once -- if they press it once. I don't think the encoder settings for x264/etc are too critical, I think the most important things to modify are usually things like bitrate, resolution, fps, GOP/keyint, maybe profile -- but I don't think things like buffer size or level or anything like that are important because we already handle those automatically behind the scenes. It feels a bit unusual to modify those other settings, and some of those settings are different depending on which encoder implementation you use (x264/NVENC/VCE/etc), whereas the primary settings such as bitrate/res/fps/gop/etc are universal. The settings in the PR are all designed for x264, whereas the majority of our users are probably using NVENC over x264 (NVENC is default if available). So those settings often won't even be set -- thus it's a concern I want to iron out. In my humble opinion, focusing on good bitrate/resolution/fps/keyint combinations is the most important thing. I think that another very easy and simple way to approach integration of the API would be to implement it as a transparent settings sanity-check when they start their stream using the service. We do need more sanity-checks in the program to make sure that people are streaming with solid settings, that much is definitely true. So basically my idea would be: when the user starts the stream, do a transparent check internally to check the API with current stream settings, and if things are within reasonable/expected/required parameters for the service, then start the stream as normal, otherwise if the streaming settings are not ideal or not within requirements, we can present a dialog to the user asking them if they want to change these parameters to the recommended parameters, and show them the recommended parameters that they can apply if they so choose, then start stream. Doesn't even need to be a wizard I think -- perhaps it could be simplified further to make it easier and more straightforward for users. That way, it would be even more simple to implement, and if the API changes server-side, it would affect all users as soon as the API is changed, which is probably one of the other original intentions for the API. It's something that would allow you to roll the API out the door more quickly/easily, while also ensuring more coverage to users to get them using it. I know that the API as-is right now is made more for auto-configuration, but I feel like this all could be adapted (likely without even changing the API) to allow this sort of sanity-check functionality, and the sanity-check functionality would probably be even more effective than an auto-configuration that's run a single time. This is just my idea though, I'm trying to find constructive ways to integrate streaming specifications received via API, while keeping things simple and reducing potentially duplicated functionality of the auto-configuration wizard. And if the service is not Facebook, then we can just calculate reasonable settings for the sanity-check. I think this is something we can calculate/check reasonably well in a service-agnostic way as a fallback if the service doesn't have its own API or something. That being said, I think we can adapt the existing wizard to use the API as well. And if there's issues with the wizard taking too long to use, we should be able to iron those out as well. Or at least that would be my hope. What do you think about this? If I'm off the mark or anything like that, please let me know. |
I do want to additionally state that the placement of it within the streaming section of settings itself is not a bad idea -- in fact, it's smart, because if a user is running on default settings, and they haven't run the auto-configuration, we should probably ensure that they have at least attempted to configure encoding settings or auto-configured encoding settings. We should probably have some check somewhere to make sure they've set encoding settings, and then possibly prompt to run the auto-configuration wizard if they haven't configured settings before they try to start the stream, or suggest good settings to them ala the sanity-check. So I really like the idea of trying to get the user to set good encoding settings early on, and make sure they're not stuck on defaults unintentionally. I realize now that was one of your intentions there, and it's definitely on the mark. #3649 is another good idea as well, we could combine the idea I proposed there with it here. I'm wondering if perhaps we could also try to run the auto-configuration before they try to start their stream if we detect that they haven't set their encoding settings. So, to summarize, on a "Start Streaming" click, the idea for running the auto-config could be:
Or, on "Start Streaming" click, if it knows that the user has set their encoding settings:
Again what do you think on these sort of idea? I feel like they should be simple to implement, and should ensure users are using solid settings at all times. And what does everything else think for that matter? |
I decided to write this as a separate wizard because we have similar ideas on making a Start Streaming flow. And believed in the end it should be different from the AutoConfig first-run/setup wizard. I think there's problem over-reusing UIs in the application already which creates endpoints with a lot of flags, options, and routes to manage. I want to keep this simpler. Additionally, I had started looking at the existing wizard and refactoring it but the PR has been ignored which signaled me it would be best to compartmentalize this UI flow.
Our thinking was, even if not promised every stream, when people are setting up their stream key is the good time to check settings one last time. The original draft had it every stream interrupting Start Stream but we want to make sure it works like this AND get it done in time first.
Agree. We're seeing tens of thousands of streams send over-resolution daily. But encoder settings are important for the ingest backend side; even if x264 is not the majority it will be a good start. Making streams closer match what live stream ingest servers expect reduce latency and streaming failures. Back on the big four (BR/RES/FPS/KF), we agree that the four big data flows are the most important, which is why they’re they a priority of this API for now. I'm okay removing most x264 settings that are undesired here.
Originally this was going to be a checkbox and no UI that checked settings on stream. I thought that users should be more informed that settings are changing. We additionally know we have users who need to make a decision on resolution to make sure it’s either below or at 1080p. These were the biggest driving factors in adding a short UI as well. I started writing this as a sanity check comparison but found a few issues: i) some parts of the settings API in OBS is hard to use to compare values which meant a lot of overhead when we're trying to target the next release, ii) I ended up writing UI component for confirmation anyways, iii) our internal gaming team worried that we would apply a resolution setting that was not desired by the account [hence the confirmation screen now] Overall I believe these should be separate. I'm glad you approve of the placement in setting as well as #3649 — the two combined should paint a picture why I think each flow is important. I think the autoconfig wizard should be on first run for profiles and any fresh start while this flow was designed to quickly add specific encoder settings per provider specific to a provider before a stream.
The only problem I see here is significantly more development time and like we discussed, we were targeting this to try to help the holiday surge we get in November through December. |
Max bitrate and keyframe interval are already things that can be controlled by service requirements in services.json. However, resolution and framerate are currently not things that can be limited by services.json. For a short-term solution I would much rather see support for resolution and fps limits inside services.json instead of actively introducing technical debt to the project via the creation of an additional auto-config wizard. That's a feature any service can find useful. Yes, putting those values in the services.json file makes it harder for Facebook to change those recommendations on the fly (requires submitting a PR to services.json instead of just tweaking the API response), but we're talking about a feature that Facebook are hoping to have released in the wild in less than a month. Consensus on the OBS side seems pretty clear that the current design leaves a lot to be desired in terms of usability and future maintainability, so if expediency is the primary motivating factor here, then augmenting the existing system we have in place for applying sane limits on settings (services.json) seems like the shortest path toward something that is at least somewhat more workable. |
I'll look into this next week. Also working on #3649 when I have a moment |
I will also do my utmost to help to make sure we have a solution for the holidays, the item I've been blocked on has been eliminated off of my plate, so feel free to let me know if there's anything I can do to help. |
Description
This PR adds a wizard workflow specifically for fine-tuning the stream encoder configuration. In this implementation we're using Facebook's no-auth API, a user-anonymous version of the professional API hardware encoders use. The wizard is extensible for other destinations to use.
Settings are applied for users of simple mode. Setting complicated configuration strings automatically for novice users.
If this change includes UI elements, please include screenshots.
Please note that the Facebook-specific text and buttons are only shown for when the Destination is Facebook AND UI will have some changes while in WIP status.
Settings check entry point next to the stream key:
First page of stream check educates user and suggests the most common resolutions. This is configurable per-service:
Loading page should be very short so is a minimal animation of the ellipses on the label
After querying and loading suggested encoder settings, user can select which to apply:
Final page, user applied settings. Respective service can use the footer to show text and link with next steps.
Error page, example is network timeout.
Motivation and Context
Why is this change required? What problem does it solve?
Better Stream Broadcasts
Stream destinations have seen a multiplicative surge in broadcasts from OBS since covid started. This highlighted many takeaways and improvement opportunities for our live streaming platform but for this PR we focus on two problems and solutions: First, that the settings on incoming streams could be better for both ingest to reduce latency as well as increase the quality for the streamer and their viewers. Second, direct feedback from users has shown us that configuration in OBS is still difficult.
Complex encoder settings
I am adding this wizard to apply fine-tuned settings to the existing encoder selection in Output Settings so that the user ideally doesn't even need to change anything in the tab.
Extensible and anonymous
The wizard is being written in a way that other services can use and extend the UI. Any service can write their own provider. Overall we want to make the experience better. Facebook is using an API that doesn't require login, is only used when you will be streaming to Facebook, and doesn't log any personal information; this leverages a new a light version of the Professional Facebook Encoder API but made to be anonymous, stateless, and faster. [ Publicly available documentation link coming soon ]
Goal: Refractor and combine with first-run wizard
Motivation behind starting #3246 "UI: Refactor Wizard Pages into encapsulated files" is that the wizards could be refactored and unified for both start up and go-live. See next, "Goal: Go-Live workflow "
Goal: Go-Live workflow
Eventually would like to have an opt-in flow between "Start Streaming" and Streaming that performs a check on encoder settings leveraging the autoconfig and/or pre-stream wizards.
How Has This Been Tested?
Tested: Macbook Pro, MacOS 10.15.7 (19H2)
Will change this line when Windows testing completes.
Will change this line when Linux testing completes.
Types of changes
This is a Work in Progress with some finished, some upcoming:
In consideration: first version may only apply settings for x264 basic and then add support for NVEC and Apple Hardware Encoder. Provisioning a windows laptop to test and work on NVEC support by roughly next week.
Final Checklist: