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

rtmp-services: Add multitrack video configuration support #10543

Conversation

palana
Copy link
Contributor

@palana palana commented Apr 15, 2024

Description

These changes are prerequisites for the upcoming multitrack video/Enhanced Broadcasting support, and add support for eRTMP multitrack video configuration URL (and related disclaimers) to rtmp-services (and libobs)

Motivation and Context

Twitch/Amazon IVS uses server side configuration for eRTMP Enhanced Broadcasting/multitrack video stream configuration, so users don't have to manually configure renditions being sent

ertmp_configuration_url (and ertmp_configuration_name) are exposed in the service json configuration to allow other services to adopt the same configuration mechanism without additional code changes

How Has This Been Tested?

This is in use 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.

@WizardCM WizardCM added the New Feature New feature or plugin label Apr 15, 2024
libobs/obs-service.h Outdated Show resolved Hide resolved
@RytoEX RytoEX self-assigned this Apr 17, 2024
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 am curious about the locale string here and if we can remove the service-specific one. No other notes/nits at this time.

plugins/rtmp-services/data/locale/en-US.ini Outdated Show resolved Hide resolved
@derrod
Copy link
Member

derrod commented Apr 20, 2024

Also would echo the sentiment that calling it ertmp is misleading as its not part of that spec, and is in theory protocol-agnostic, e.g. it could be used with MoQ in the future.

@murillo128
Copy link
Contributor

Th PR title is misleading and I think some of the comments on the code.

While the intention may be to add configure multitrack video, this PR and the code in only implements setting a custom configuration URL for an RTMP provider .

Currently these seems locked to twitch and only configuring multitrack video but, if merged, I think this should be generalized (and "standarized") so it doesn't mention twitch and we allow to configure other stuff different than multitrack video.

@palana palana force-pushed the ruwen/multitrack-video-rtmp-additions branch 2 times, most recently from d76bdbf to 4e781fc Compare April 25, 2024 21:07
plugins/rtmp-services/rtmp-common.c Outdated Show resolved Hide resolved
@palana palana force-pushed the ruwen/multitrack-video-rtmp-additions branch 2 times, most recently from ec37d8f to 59c7325 Compare May 2, 2024 12:35
@RytoEX RytoEX changed the title Multitrack video rtmp-services additions rtmp-services: Add multitrack video configuration support May 2, 2024
@palana palana mentioned this pull request May 3, 2024
6 tasks
@Lain-B
Copy link
Collaborator

Lain-B commented May 3, 2024

One of these days we'll have to work on that big service update to make things easier for services to share features as well as to make things easier for individual services to customize (and get that RFC through), but this has my approval

@RytoEX
Copy link
Member

RytoEX commented May 3, 2024

Th PR title is misleading and I think some of the comments on the code.

While the intention may be to add configure multitrack video, this PR and the code in only implements setting a custom configuration URL for an RTMP provider .

Currently these seems locked to twitch and only configuring multitrack video but, if merged, I think this should be generalized (and "standarized") so it doesn't mention twitch and we allow to configure other stuff different than multitrack video.

I've adjusted the PR title.

Twitch is contributing the code, so it aligns with their implementation. That said, there is nothing stopping other platforms/providers from implementing this. See Twitch's Help Article on Multiple Encodes, specifically:

If you’re a third-party video tool or platform and want help, please get in touch.

None of the actual code mentions Twitch anymore. The only places that do are examples in the JSON schema and the strings in the services.json file in Twitch's entry, where they are appropriate.

@palana Please squash the commits here.

@palana palana force-pushed the ruwen/multitrack-video-rtmp-additions branch from 59c7325 to 14602a6 Compare May 6, 2024 12:27
@palana
Copy link
Contributor Author

palana commented May 6, 2024

@palana Please squash the commits here.

@RytoEX: done (also rebased on master)

@RytoEX
Copy link
Member

RytoEX commented May 6, 2024

@palana Please see @Warchamp7 's review comment on #10634.

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.

Commit message nit:

rtmp-services: Allow loading Multitrack Video configuration from services.json

This commit message subject is too long.

Instead, use this:

rtmp-services: Allow loading Multitrack Video config from JSON

Add support for loading Multitrack Video configuration data from
services.json.

@palana palana force-pushed the ruwen/multitrack-video-rtmp-additions branch from 0e61f3f to daf5e65 Compare May 7, 2024 10:33
@palana
Copy link
Contributor Author

palana commented May 7, 2024

Commit message nit:

rtmp-services: Allow loading Multitrack Video configuration from services.json

This commit message subject is too long.

Instead, use this:

rtmp-services: Allow loading Multitrack Video config from JSON

Add support for loading Multitrack Video configuration data from
services.json.

@RytoEX: done

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.

Looks good to me.

@RytoEX RytoEX added this to the OBS Studio (Next Version) milestone May 7, 2024
@RytoEX RytoEX merged commit e92accf into obsproject:master May 8, 2024
14 checks passed
@palana palana deleted the ruwen/multitrack-video-rtmp-additions branch May 8, 2024 12:24
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

9 participants