Skip to content
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

Add RFC: Service Overhaul #39

Closed
wants to merge 36 commits into from

Conversation

tytan652
Copy link
Contributor

@tytan652 tytan652 commented May 28, 2021

I'm sorry to have stupidly pushed a PR about this suddenly without talking about it with you.
So now I tried to write this RFC, it's not perfect, but we are here to discuss about it.

This RFC is now split into two RFCs. They are both quite dependent of each other.

RFC 39: Service Overhaul

Summary

  • Make OBS able to accept third-party service plugins
  • Attribute a id for each services rather than a common one
  • A service can be provided with multiple protocols
  • Separate Twitch, Restream and YouTube integration and make them plugins

Motivation and Context

With those changes, developer will be able to create third-party service plugins.
And also service integrations will be separated as plugins.

Link to RFC

Link to an implementation of the RFC

@henke37
Copy link

henke37 commented May 28, 2021

Services kinda suck in OBS. They can't setup outputs directly. They can't offer extra features such as the RPC of rtmp. In fact, rtmp doesn't even have a formal password/key system! Each service should be able to do its own rtmp setup as desired.

I hope that this redesign helps combat this problem.

Another aspect that is missing is services being able to pick which server to use given certain conditions.

@tytan652
Copy link
Contributor Author

tytan652 commented May 28, 2021

The plugin obs-services will mainly register services which uses stream key.

obs-services2 is meant to be used to implement services with specific behavior, right now there is Dacast, and many one with ingest.

Edit: Also there is also the ability to create and use third-party service plugin. Which can help for really specific case.
Edit2: They can sorta set up output with recommended settings stuff

@henke37
Copy link

henke37 commented May 28, 2021

"Sorta set up" isn't good enough for me. I'd like the service to be an output factory, taking control from centralized code for maximum control over the output.

@tytan652 tytan652 force-pushed the service_overhaul branch 2 times, most recently from 41ee862 to 1dd1073 Compare May 29, 2021 07:23
@tytan652
Copy link
Contributor Author

They can't offer extra features such as the RPC of rtmp. In fact, rtmp doesn't even have a formal password/key system! Each service should be able to do its own rtmp setup as desired.

  • RPC falls under obs-outputs RTMP implementation, not services. Please don't mix up outputs with services.

  • I don't know if obs-outputs or even stream services follow the RTMP specification. But if it's not in the spec, in my opinion there is no "obligation" to implement it.

  • I don't know when the team decided to need a "representative" from the stream services when it comes to make PR with purpose to add a services. But there was a period where they were not put by "representatives".

  • But if they really needed to implement it a certain way they were free to push a PR or ask for an addition of a mechanism to add third-party plugins if they don't want to be stuck with OBS release cycle.

@henke37
Copy link

henke37 commented May 29, 2021

Correct, RPC in RTMP would be a feature of the output plugin. But the service should be able of encapsulating it in the form of a nice usable api. Which includes providing the login.

@tytan652
Copy link
Contributor Author

tytan652 commented May 29, 2021

They are already usename and password in the service API, I think they are just not used except for Dacast and Custom services.
But I don't know if RPC is implemented.

@henke37
Copy link

henke37 commented May 29, 2021

RPC is not implemented in the rtmp client that obs uses. At least not in a good reusable way. I'm honestly more concerned about websocket based apis and such. It'd be very nice if obs could do stuff like subscriber alerts without the aid of 3rd party websites.

@tytan652
Copy link
Contributor Author

tytan652 commented May 29, 2021

It'd be very nice if obs could do stuff like subscriber alerts without the aid of 3rd party websites.

Not implemented imperatively in OBS, but in a plugin meant to supply a complete integration of the stream service.
I think those type of things, could be one of the use case of the possibility to add a third-party service plugin by the user.

@tytan652 tytan652 force-pushed the service_overhaul branch 8 times, most recently from 41afe69 to bf2bf5c Compare June 4, 2021 15:47
@tytan652 tytan652 force-pushed the service_overhaul branch 3 times, most recently from 159a417 to b285201 Compare June 8, 2021 10:08
@tytan652
Copy link
Contributor Author

tytan652 commented Jul 2, 2021

Update: This RFC draft is split into two RFCs. I put them in the same PR because they are dependent of each other.
If you want me to separate them in two PR tell me.

PS: It's also easier to work on both of them at the same time.

@tytan652 tytan652 changed the title Add RFC: Service Overhaul Add RFCs: Protocol API and Service Overhaul Jul 2, 2021
@tytan652 tytan652 force-pushed the service_overhaul branch 4 times, most recently from aa1a14d to bf8f4d1 Compare July 3, 2021 17:15
Copy link
Member

@pkviet pkviet left a comment

Choose a reason for hiding this comment

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

Some spelling/typo corrections to easen the reading.
I'll come back later to a more general in depth review.

text/0039-service-overhaul.md Outdated Show resolved Hide resolved
text/0039-service-overhaul.md Outdated Show resolved Hide resolved
text/0039-service-overhaul.md Outdated Show resolved Hide resolved
text/0039-service-overhaul.md Show resolved Hide resolved
text/0039-service-overhaul.md Outdated Show resolved Hide resolved
text/0039-service-overhaul.md Outdated Show resolved Hide resolved
text/0039-service-overhaul.md Outdated Show resolved Hide resolved
text/0039-service-overhaul.md Outdated Show resolved Hide resolved
text/0039-service-overhaul.md Outdated Show resolved Hide resolved
text/0039-service-overhaul.md Outdated Show resolved Hide resolved
@tytan652 tytan652 marked this pull request as ready for review July 8, 2023 16:46
Copy link
Member

@DDRBoxman DDRBoxman left a comment

Choose a reason for hiding this comment

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

I think figuring out how to split this up into smaller pieces that we can incrementally build on instead of a giant overhaul would be a good thing to look into.

Especially if we can make the underlying parts divorced from the UI changes.

As a generality OBS could benefit from getting away tight UI coupling as we build features into OBS rather than building based on the UI we have today.

text/0039-service-overhaul.md Show resolved Hide resolved
Adding to `obs_service_info`:
- `uint32_t flags` with the following flags:
- `OBS_SERVICE_DEPRECATED`: The service is marked as deprecated.
- `OBS_SERVICE_INTERNAL`: The service is meant to be used internally in some plugin (e.g., WebSocket, Scripting) and usually not directly exposed in the UI.
Copy link
Member

Choose a reason for hiding this comment

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

Can you expand on this with a more concrete example? I'm not clear on how web socket and scripting plugins quite match up to a service?

text/0039-service-overhaul.md Outdated Show resolved Hide resolved
text/0039-json-schemas/service.json Show resolved Hide resolved
Service types that support only the protocol inside their id (`"custom_`*insert lowercase protocol acronym*`"`) for any first-party protocol except FTL (deprecation).

Services that will only support the first-party protocol related to their id/type.
The flag `OBS_SERVICE_INTERNAL` will be applied to not show them in the UI list. Those are meant for being used in WebSocket and scripting.
Copy link
Member

Choose a reason for hiding this comment

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

What does the migration path look like for scripts / apps using the old method?

Can we just provide a built in way to identify the old behavior and translate to the new one instead of keeping these around?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

rtmp_common and rtmp_custom are not removed but deprecated.
At some point we have to remove them.

@tytan652
Copy link
Contributor Author

tytan652 commented Jul 9, 2023

I think figuring out how to split this up into smaller pieces that we can incrementally build on instead of a giant overhaul would be a good thing to look into.

We can't since service integration is too tied to the UI and the UI is too tied to rtmp-services.

Edit: Once you touch a piece, you need to replace a whole block.

@DDRBoxman
Copy link
Member

It's probably worth doing an investigation to see if there are parts of the UI we can decouple as a prerequisite before the service overhaul. Smaller changes help lower the risk and allow easier review.

@tytan652
Copy link
Contributor Author

tytan652 commented Jul 19, 2023

It's probably worth doing an investigation to see if there are parts of the UI we can decouple as a prerequisite before the service overhaul.

Beside the YouTube Actions UI form, most of the service UI is refactored from Qt to PropertiesViews. So there is not much to decouple.

@tytan652
Copy link
Contributor Author

I made a cleanup around old review conversations, some of them were closed with a comment some not because the RFC has really changed since then.

@Fenrirthviti
Copy link
Member

Based on feedback on this thread, and in off-thread discussions, the project maintainers agree that this RFC needs to be scaled back and broken up in to smaller parts to approach in a more incremental way.

The project largely agrees with the end goal of what this RFC is proposing: a more customizable, flexible, and pluggable state for services and outputs in OBS.

However, the size and scope of the current approach makes these changes difficult, if not near impossible, to do a complete review and provide specific feedback. It is so large in scope that it is unapproachable to others seeking to review it, and is not likely to be accepted in the current state.

It was not an easy decision, but if we want to start making progress towards this goal, a new approach is necessary.

To that end, a better approach would be to split out the changes in to separate RFCs/PRs. We suggest a split along the following topics:

  • Start by taking the YouTube, Twitch, and Restream integrations and make them pluggable. This would be an invisible change to the user.
  • Once complete, evaluate where the overlap is between those plugins, and migrate functions in the OBS API to reduce duplication. Again, invisible to the user, no UI changes.
  • Once parity has been reached with current implementation, start work towards refactoring a new UI and adding functionality as distinct and separate tasks.

We again thank everyone for their work on this RFC, and we hope that with these changes we can reach some of its goals sooner rather than later.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants