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

UI: Add (eRTMP) multitrack video output #10633

Merged
merged 7 commits into from
Jun 5, 2024

Conversation

palana
Copy link
Contributor

@palana palana commented May 3, 2024

Note that this PR includes a bunch of merges from currently open PRs and a FIXME commit to help with testing, which will go away as soon as the dependency PRs are merged

Description

This implements multitrack video support for Twitch Enhanced Broadcasting and Amazon IVS Multitrack Video

Motivation and Context

Multitrack video/Enhanced Broadcasting allows sending multiple video tracks/renditions from the users machine, resulting in generally higher quality renditions at lower bitrates compared to single track streaming with server side transcoding, since e.g. losses roundtripping frames through another decode/encode step are removed.
Enhanced Broadcasting/multitrack video uses server side configuration to provide easier onboarding for users, and balanced values for the number of renditions and their bitrates.

How Has This Been Tested?

This has been tested in the Twitch Enhanced Broadcasting beta

Types of changes

  • New feature (non-breaking change which adds functionality)

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.
  • All commit messages are properly formatted and commits squashed where appropriate.
  • I have included updates to all appropriate documentation.

@palana palana force-pushed the ruwen/upstream-multitrack-video-ui branch 2 times, most recently from 9a86687 to f910c0f Compare May 3, 2024 14:35
Copy link
Collaborator

@tytan652 tytan652 left a comment

Choose a reason for hiding this comment

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

This feature is clearly not implemented in a future-proof way, it tangles even more service behavior in the UI that should be plugin-ified (with APIs).

Continuously shoehorning service integrations was already a mistake, and this PR goes to the same wrong direction rather than trying to go to a better direction.

UI/window-basic-settings-stream.cpp Outdated Show resolved Hide resolved
UI/goliveapi-postdata.cpp Outdated Show resolved Hide resolved
UI/obs-frontend-api/obs-frontend-api.h Outdated Show resolved Hide resolved
UI/CMakeLists.txt Show resolved Hide resolved
@palana palana force-pushed the ruwen/upstream-multitrack-video-ui branch 2 times, most recently from 7b7c4e7 to 76bde8f Compare May 3, 2024 16:22
@RytoEX RytoEX self-assigned this May 3, 2024
@palana palana force-pushed the ruwen/upstream-multitrack-video-ui branch 7 times, most recently from a853a6b to b4f7a5e Compare May 10, 2024 17:17
@palana palana force-pushed the ruwen/upstream-multitrack-video-ui branch from be16cb1 to 25ffb6c Compare May 14, 2024 13:20
@RytoEX RytoEX force-pushed the ruwen/upstream-multitrack-video-ui branch from 439afe6 to 48cb2a9 Compare May 17, 2024 17:54
@RytoEX
Copy link
Member

RytoEX commented May 17, 2024

Rebased first. Now pushing cleanups:

  1. The multitrack audio commits don't actually need to be in here because the later commits don't touch the same code. VOD Track just won't work without them, but removing the commits reduces some noise here.
  2. We can drop "UI: Keep weak stream output ref in status bar" since it was handled in UI: Keep weak stream output ref in status bar #10680.

@RytoEX RytoEX force-pushed the ruwen/upstream-multitrack-video-ui branch from 48cb2a9 to 932269d Compare May 17, 2024 17:57
@WizardCM WizardCM added the New Feature New feature or plugin label May 18, 2024
@RytoEX RytoEX force-pushed the ruwen/upstream-multitrack-video-ui branch from 932269d to 963dab6 Compare May 20, 2024 20:15
@palana
Copy link
Contributor Author

palana commented May 21, 2024

@RytoEX, @Warchamp7: moved the remaining service settings into groupboxes after #10633 (comment)
image

@palana palana force-pushed the ruwen/upstream-multitrack-video-ui branch 2 times, most recently from c76ef2b to 696e9d9 Compare May 22, 2024 14:50
Copy link
Member

@RytoEX RytoEX left a comment

Choose a reason for hiding this comment

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

Started a review a while back and forgot to submit it. Submitting it now so get this one feedback item raised.

UI/qt-helpers.hpp Outdated Show resolved Hide resolved
Copy link
Member

@RytoEX RytoEX left a comment

Choose a reason for hiding this comment

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

I tried to autosquash locally, but it looks like some of the autosquash commits are trying to apply changes to commits before the actual base changes the autosquash commits try to fixup. Could you squash the commits?

Per @Warchamp7 , the UI itself is okay. @Lain-B and I are still doing code review.

@palana palana force-pushed the ruwen/upstream-multitrack-video-ui branch from 95df074 to 7ec786f Compare May 29, 2024 16:22
@palana
Copy link
Contributor Author

palana commented May 29, 2024

I tried to autosquash locally, but it looks like some of the autosquash commits are trying to apply changes to commits before the actual base changes the autosquash commits try to fixup. Could you squash the commits?

Per @Warchamp7 , the UI itself is okay. @Lain-B and I are still doing code review.

@RytoEX: commits are squashed, apologies for the unclean fixup commits

Copy link
Collaborator

@Lain-B Lain-B left a comment

Choose a reason for hiding this comment

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

Thanks for rebasing. There are a few minor things, such as Linux/macOS just not being implemented at all and failing with "GPU not supported", but generally it has my approval. We'll probably just have to ifdef out the Linux/macOS codepath for it and disable it for the time being but otherwise again it generally has my approval.

Dumping config json from nlohmann requires more than 4096 bytes
@RytoEX RytoEX force-pushed the ruwen/upstream-multitrack-video-ui branch from 7ec786f to 9031cdd Compare June 5, 2024 00:30
@RytoEX
Copy link
Member

RytoEX commented Jun 5, 2024

Rebased to latest master. May push trivial fixes (text nits + minor cleanups or just text nits) before merging.

@koolscooby
Copy link

Thanks for rebasing. There are a few minor things, such as Linux/macOS just not being implemented at all and failing with "GPU not supported", but generally it has my approval. We'll probably just have to ifdef out the Linux/macOS codepath for it and disable it for the time being but otherwise again it generally has my approval.

If possible, we'd like to keep the implementation as-is so we can release server-side updates to start vending ladders for Linux, MacOS, AMD AMF, and Intel QSV without requiring additional changes to OBS.

In the short term, we can improve the error message that is shown so that it makes it more apparent to users that the reason it is unsupported is not the build of OBS Studio, but instead the service that vends the configuration.

@RytoEX RytoEX force-pushed the ruwen/upstream-multitrack-video-ui branch from 9031cdd to 1aa8111 Compare June 5, 2024 00:50
Copy link
Member

@RytoEX RytoEX left a comment

Choose a reason for hiding this comment

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

Leaving a brief review comment for posterity.

UI/system-info-windows.cpp Show resolved Hide resolved
Copy link
Member

@RytoEX RytoEX left a comment

Choose a reason for hiding this comment

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

The CI failures are unrelated. The GitHub Actions Ubuntu runners are restarting due to some issue related to apt packages that indicate they require service restarts.

@RytoEX RytoEX added this to the OBS Studio (Next Version) milestone Jun 5, 2024
@RytoEX RytoEX merged commit 0cc357f into obsproject:master Jun 5, 2024
14 of 15 checks passed
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants