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

RFC 39: Overhaul services #9220

Closed
wants to merge 65 commits into from
Closed

Conversation

tytan652
Copy link
Collaborator

@tytan652 tytan652 commented Jul 8, 2023

Description

This PR is related to obsproject/rfcs#39, it does not include:

  • changes about output settings which replace network settings
  • CI replacement for the services JSON checks

This PR overhaul services to replace the unique rtmp-common/rtmp-custom combo by a service per streaming services and one for custom (and also internal service per protocol).

This PR provides:

  • Move dock state to profile (required for service integrations plugin)
  • Add to services the API to indicate if it use "1 audio track"/"1 track + 1 archive track"/"multiple audio track"
  • Move service integration outside of the UI as plugins (obs-restream, obs-twitch and obs-youtube)
    • The UUID system in those plugins is mostly for future-proofing
    • Note that Browser docks were already quite crash-prone on Linux if you test on it
  • Add API in Services API to enabled multi-protocol per service support and avoid "H264 only" API.
  • Deprecated APIs that are too much "H264 only"
  • Add bandwidth test related API in the Services API
  • Add a broadcast flow API in the frontend API for service like YouTube and third-parties
  • Add obs-browser related function in the frontend API
  • Migrate old rtmp-common/rtmp-custom to the new services in a new file
  • Add CI to keep obs-services JSON parser and its JSON schema aligned

If I forgot to mention something that you think is necessary, tell me.

Twitch with integration:
Screenshot from 2023-07-08 21-02-53

YouTube with integration:
Screenshot from 2023-07-08 21-03-34

Custom service:
Screenshot from 2023-07-08 21-03-24
Screenshot from 2023-07-08 21-03-11
Screenshot from 2023-07-08 21-03-07

Motivation and Context

How Has This Been Tested?

Types of changes

  • New feature (non-breaking change which adds functionality)
  • Code cleanup (non-breaking change which makes code smaller or more readable)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Documentation (a change to documentation pages)

Checklist:

  • My code has been run through clang-format.
  • I have read the contributing document.
  • My code is not on the master branch.
  • The code has been tested but it needs to be tested by more people than myself because it's big.
  • All commit messages are properly formatted and commits squashed where appropriate. (Tell me if think there is more to squash)
  • I have included updates to all appropriate documentation.

@tytan652 tytan652 added Seeking Testers Build artifacts on CI New Feature New feature or plugin labels Jul 8, 2023
@tytan652 tytan652 force-pushed the service_overhaul branch 3 times, most recently from c9980ae to ceaecb2 Compare July 8, 2023 21:09
@tytan652 tytan652 force-pushed the service_overhaul branch 7 times, most recently from 9a930a9 to 0008f40 Compare July 15, 2023 08:08
@PatTheMav
Copy link
Member

I do have feedback on some specific changes made in the PR, but I'd rather focus on the architectural issues first, as they only focus on where files are moved and how we set up our repo:

  • I like the extraction of OAuth functionality into its own Qt module

  • I also agree with the overall approach of having a service being a separate module that implements core and frontend APIs

  • I would suggest placing services in a new subdirectory called "services" instead of lumping them in with other functional "plugins" - this would require adding that directory as a new location for runtime modules but could otherwise work the same. Another benefit would be that services-related code is cleanly separated from the rest

  • I would also suggest not adding new things to the "deps" directory - its purpose was for vendors external dependencies, but this PR extends its use for "shared modules". I'd much rather have us use a new directory called "shared", "common", or some other that communicates this purpose.

  • I am very much against symlinking files from somewhere else in the source tree into the services directories. That's not a solution, it's very much a hack, because every CMake target needs to be self-contained and should not rely on files existing "somewhere" relative to their location.

    • The solution to this would be to move UI elements shared by modules/plugins to be moved into the same "shared" directory and be made a standalone library (much like the OAuth functionality)
    • Then the service modules could include that library (whether of STATIC or INTERFACE type)
    • That would also fix this outstanding issue with the frontend plugins which "magically" pull in sources from the UI source tree that they shouldn't actually know about

I think the changes go in the right direction, but it's also a chance to create new paradigms/architectural decisions that clean up existing issues and we don't have to repeat old mistakes (i.e., throwing stuff into the deps directory, linking/including files from across the source tree, etc.).


void OBSBroadcastFlow::ManageBroadcast(bool streamingActive)
{
if (flow.manage_broadcast2) {
Copy link
Member

Choose a reason for hiding this comment

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

Isn't the broadcast flow an entirely new class? Why is the function called manage_broadcast2?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Check the new structure in the OBS Frontend API and it's related documentation.

TLDR: There is two variant of the manage_broadcast.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I didn't check if the a certain flag is set 🤦 while checking for the 2 variant.

Copy link
Member

Choose a reason for hiding this comment

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

Got it - I dunno what other maintainers think about this, but in general I really don't like the current practise of just slapping a number to an existing function when we extend its signature - it's a quick and dirty workaround, but not good API design.

Given that we are creating a new API from scratch here, we can be more elegant and this case I'd rather give the function a more descriptive name, because so far "2" means "new variant, use this", but these 2 functions can and should be used depending on functionality of the service.

So I'd suggest using something closer to manage_broadcast_with_stream_status or something similar. ObjC and Swift both make extensive use of identical functions with different signatures (allowing different variations of parameters) and verbose function names that tell you by themselves what they do.

@tytan652
Copy link
Collaborator Author

* I like the extraction of OAuth functionality into its own Qt module

Technically only the login dialogs module are Qt, but ❤️.

* I would suggest placing services in a new subdirectory called "services" instead of lumping them in with other functional "plugins" - this would require adding that directory as a new location for runtime modules but could otherwise work the same. Another benefit would be that services-related code is cleanly separated from the rest

Unless, outputs plugin are put in in their own and same for other plugin types. They are technically plugins, separating them from other plugin is not good in my opinion.

* I would also suggest not adding new things to the "deps" directory - its purpose was for vendors external dependencies, but this PR extends its use for "shared modules". I'd much rather have us use a new directory called "shared", "common", or some other that communicates this purpose.

I'm thinking about doing a PR about doing this.

* I am very much _against_ symlinking files from somewhere else in the source tree into the services directories. That's not a solution, it's very much a hack, because every CMake target needs to be self-contained and should not rely on files existing "somewhere" relative to their location.
  
  * The solution to this would be to move UI elements shared by modules/plugins to be moved into the same "shared" directory and be made a standalone library (much like the OAuth functionality)
  * Then the service modules could include that library (whether of `STATIC` or `INTERFACE` type)
  * That would also fix this outstanding issue with the frontend plugins which "magically" pull in sources from the UI source tree that they shouldn't actually know about

I'm thinking to add that to the "doing a PR" idea.

I think the changes go in the right direction, but it's also a chance to create new paradigms/architectural decisions that clean up existing issues and we don't have to repeat old mistakes (i.e., throwing stuff into the deps directory, linking/including files from across the source tree, etc.).

👍

PS: The next push will only be a rebase + reformatting.

@Fenrirthviti
Copy link
Member

In an effort to adhere more strictly to our current RFC process, we are opting to close this PR until the linked RFC has been finalized.

Linking to and discussion of the branch itself on the RFC remains completely welcome, but we do not feel it is appropriate to open a PR for an unfinalized RFC, especially one that has as much open discussion as #39.

We are looking forward to re-opening this when the RFC has been completed and merged!

@tytan652
Copy link
Collaborator Author

We are looking forward to re-opening this when the RFC has been completed and merged!

Just to specify a thing, when you force-push a branch is linked to a closed PR, the PR is made unable to be re-opened again. So it will to be a new PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
New Feature New feature or plugin Seeking Testers Build artifacts on CI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants