Expose OBS controls to browser sources #281
No reviewers
Labels
No labels
area/accessibility
area/dependency
Confirmed
Crash Report
Delayed
Design Issue
Duplicate
Feedback required
Good first issue
kind/bug
kind/cleanup
kind/enhancement
platform/linux
platform/macos
platform/windows
priority/high
Question
Request for Comments
Seeking Other Contributors
Seeking Testers
Won't Fix / By Design
No milestone
No assignees
1 participant
Notifications
Due date
No due date set.
Dependencies
No dependencies set
Reference
gh-obsproject/obs-browser!281
Loading…
Reference in a new issue
No description provided.
Delete branch "gxalpha-expose-frontend-api"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Description
Exposes more functions of the Frontend API to the webpage.
Adds a button to the source properties that lets the user decide whether the webpage should be allowed to access it.
Change to UI:
Motivation and Context
I feel like it might be useful for some people to provide some scripting support for JS (especially since the current functionality allows to save the replay buffer but nothing more, which appeared odd to me.)
The button is there to stop webpages from abusing this functionality, I also added a function for the webpage to know whether it is allowed to access the OBS controls. The button is defaulted to false (for obvious reasons).
I also put the current saveReplayBuffer() function behind the permission-button for consistency reasons. This would be a potentially breaking change to existing functionality. I see that this change could be debatable.
How Has This Been Tested?
Successfully built and run on macOS 11.2.3.
Tested all the functions of the buttons in the screenshot above.
Tested functionality of the "Grant webpage control over OBS"-button.
Types of changes
Checklist:
Additional information
These are obviously not all of the frontend-functions which in the long run I'd kind of like to add.
That would give easier access to scripts that other people (and external providers) might make and publish, since you'd just have to add a browser source with the JS script, instead of downloading and pasting it somewhere while having to update it when new functionality is added to the script.
Feedback regarding this idea is appreciated.
Also congrats for reaching the RC-phase of v27 :D
Commits to obs-browser don't need to be prefixed by
obs-browser.Oh yeah makes sense, I'll remove it
Probably not today anymore though
I'll review the capabilities of this properly after v27 is out, but thank you so much for doing this PR. I've been meaning to do it forever.
@ -124,3 +129,2 @@for (std::string name : functions) {for (std::string name : exposedFunctions) {CefRefPtr<CefV8Value> func =As a side note, before I review/merge this PR, I'll first be sure to submit my commit to simplify this code, which'll help this implementation significantly.
It'll become something like this instead:
Please adapt your PR for the following change: https://github.com/obsproject/obs-browser/commit/450ff0
Additionally, one thing I debated when writing a similar capability is the idea of "access levels" - for example, starting the replay buffer is much safer than starting streaming. A simple dropdown of "Allow read-only access", "Allow basic control (start/stop replay buffer, etc)" and "Allow full control (start/stop streaming without warning, etc)."
Alright, will do
Argh, Github closes PRs if you remove the commit you want to merge (temporarily)?
I hope it will automatically reopen it when I put it back in, otherwise I'll open a new one
Alright, here we go.
UI now looks like this (the missing space behind the commas is a macOS-OBS bug and OBS-wide, not related to the en-US.ini):
Default is Read-Only.
There are two additional things I noticed while testing:
@ -11,6 +11,12 @@ RestartCEF="Restart CEF"BrowserSource="Browser"This text does not match the actual permissions you've provided - basic only provides "Save Replay Buffer". Maybe Basic should remain as-is, and then Start/Stop replay buffer & VirtualCam etc without Start/Stop Streaming/Recording should be "Advanced"? I'm not sure.
@ -11,6 +11,12 @@ RestartCEF="Restart CEF"BrowserSource="Browser"Oh, you're right - I meant "Save replay buffer" when it says "Start/Stop", will fix
I'm personally not sure it would be a good idea to give the user even more possibilities, though I have no strong feelings about that
@ -11,6 +11,12 @@ RestartCEF="Restart CEF"BrowserSource="Browser"Yep, I have no strong feelings either way myself.
@ -11,6 +11,12 @@ RestartCEF="Restart CEF"BrowserSource="Browser"Fixed the wording on the current behaviour, left the levels how they are for now.
Maybe someone else has opinions on the above ^
@ -11,6 +11,12 @@ RestartCEF="Restart CEF"BrowserSource="Browser"Update:
Thought about this, an "Advanced" level probably is a good idea.
That could be used as well for say switching scenes (which is something I plan on adding once the initial functionality is merged), and only the most dangerous controls (aka start/stop of the outputs) would be under "all".
The levels would then be:
I still think 5 options might be a little much but probably a drawback worth taking.
If you agree, I'd add this.
@ -11,6 +11,12 @@ RestartCEF="Restart CEF"BrowserSource="Browser"I would say that's a worthwhile distinction.
Added "Advanced" as permission as discussed with WizardCM, also added the required permissions to the functions in the README.
Is there a reason why getControlLevel is callable with the None permission level instead of the ReadOnly level?
I'd like it if the commit message could be more descriptive. Particularly, it would be nice if the commit subject were a bit clearer.
I'm not fond of the huge switch statement in browser-client.cpp, but I suppose it can be cleaned up at a later date.
@ -60,8 +60,24 @@ Descriptions for these events can be [found here](https://obsproject.com/docs/re* obsVirtualcamStoppedNitpick: level
The doc item should match the case of the actual parameter.
@ -65,0 +77,4 @@```#### Get the current scenePermissions required: READ_ONLYInsert a blank line between the end of the last code block and the next header for readability.
@ -350,2 +354,4 @@}bool IsValidFunction(std::string function){I did some simplistic timing and it seems that an
IsValidFunction(name)function that uses a vector or an array with std::find generally performs as good or better than this approach. This approach only performs better if the sought function name is in one of the first few conditions checked (i.e., up to startRecording). The savings are minute, but it looks nicer.If you don't want to change this in this PR, I don't consider it a blocker, and I can address it later.
@ -205,6 +207,30 @@ static obs_properties_t *browser_source_get_properties(void *data)obs_properties_add_bool(props, "restart_when_active",It'd be simpler and a lot less code to just call
obs_property_list_add_intfor each enum entry. I don't think the default case in the switch gains us anything here.Yes. getControlLevel tells the webpage how many permissions it has, so that it knows whether a method is callable or not. Locking it behind ReadOnly would kind a vicious circle - If the permissions would be set to none, the page can't try to find out what level it is, and if it doesn't know which level it is, it doesn't know if it can call that function.
Also, putting the function behind ReadOnly won't actually hide anything - say the user sets the level to none and the function gets called, it would error out, basically telling the page it is set to none.
(I hope this answers your question)
I'll address the other issues.
@ -205,6 +207,30 @@ static obs_properties_t *browser_source_get_properties(void *data)obs_properties_add_bool(props, "restart_when_active",Done.
Now stupidly, clang-format decided to split each of these lines into four lines, removing most readability benefits, so I decided to turn it off for that bit and try to keep the lines at a reasonable length by formatting myself - I hope that ok
@ -205,6 +207,30 @@ static obs_properties_t *browser_source_get_properties(void *data)obs_properties_add_bool(props, "restart_when_active",Readability wasn't my only concern. The change also vastly simplifies the code (removed a for loop and switch statement that was in the loop). Personally, I don't think we need to care about switching clang-format off and on. If clang-format makes each function call four lines long, so be it. That said, I'm also not hung up on it, I just don't see the point. @WizardCM ?
@ -350,2 +354,4 @@}bool IsValidFunction(std::string function){Done
@ -11,6 +11,12 @@ RestartCEF="Restart CEF"BrowserSource="Browser"I'd probably change this to "Page permissions" or "OBS API permissions" (for some reason "webpage" feels off to me in this context) with the additional context that this is access to OBS, eg:
None: No access to OBS
ReadOnly: Read-only access to OBS (current scene and status)
Basic: Basic access to OBS (save replay buffer)
Advanced: Advanced access to OBS (change scenes, start/stop replay buffer)
Full: Full access to OBS (start/stop streaming, recording, etc).
Otherwise I feel "access" is a bit vague as to what exactly it's providing access to, and "None" doesn't fit the style of the other options ("No access" would be better IMO). I feel it's important to get the wording right as this is a potentially very dangerous feature if users don't understand it properly and grant random websites full access.