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

obs-webrtc: Add Simulcast Support #10885

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Sean-Der
Copy link
Contributor

@Sean-Der Sean-Der commented Jun 19, 2024

Description

This PR adds supports for Simulcast to the obs-webrtc output. Simulcast allows for multiple quality levels to be sent over one track in WebRTC.

RFC 8853 is a technical description. For a less technical explanation Wowza, Dolby and LiveKit have all written articles on it.

This PR starts with adding a Spinbox when WHIP is selected. This allows the user to choose how many layers to send in total. The acceptable range (for now) is 1-4.
defaultPage

The Height/Width/Bitrate is scaled from the users global choice. So depending on the users choice.

  • 1 Layers (
    • 100% Height/Width/Bitrate
  • 2 Layers
    • 100% Height/Width/Bitrate,
    • 50% Height/Width/Bitrate
  • 3 Layers
    • 100% Height/Width/Bitrate
    • 66% Height/Width/Bitrate
    • 33% Height/Width/Bitrate
  • 4 Layers
    • 100% Height/Width/Bitrate
    • 75% Height/Width/Bitrate
    • 50% Height/Width/Bitrate
    • 25% Height/Width/Bitrate

A server may wish to only accept a subset of the layers offered. Today if the user rejects any layers we throw an error and say how many layers were accepted. The user can lower the amount of layers they are sending and try again. Services in their documentation will be expected to tell users how they want video encoded and how many layers they are willing to accept.

failedToAcceptLayer

Motivation and Context

  • Low Latency - Decoding/Re-encoding adds too much latency for interactive content
  • Make self hosting easier - Running ffmpeg is costly and difficult to scale
  • Streamers more control - If streamers can generate transcodes they control the quality
  • E2E Encryption/Prevent Modification- Streamers can distribute keys to viewers and servers can't modify video anymore.

Some questions that have been asked by reviewers already.

What about Enhanced Broadcasting? - Simulcast is part of an existing standard that already sees a billion minutes per day so it is here to stay and is already deployed massively. I would like to see Enhanced Broadcasting in the JSON blob be able to specify the protocol. When a user starts a enhanced broadcasting session it should be able to switch between RTMP/WebRTC/$x. I think both flows can/should exist because they serve different purposes.

UI/UX Considerations - A single opt-in is non-intrusive and respects the user. It would be good to get feedback from all OBS WebRTC users (Dolby, LiveKit, Cloudflare etc...) that this is a sound decision though.

How Has This Been Tested?

This has been tested with the nvenc, qsv, and x264.

I have tested this against Twitch, Broadcast Box, LiveKit and Dolby.

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 Jun 19, 2024
@Sean-Der

This comment was marked as outdated.

@tytan652
Copy link
Collaborator

tytan652 commented Jun 20, 2024

Hi @tytan652 I see you gave a 👎 was that an accident?

No, I used that to avoid always adding an "off-topic" to PRs saying that the PR adds code complexity to the streaming codepath which adds more work to my plans for a Service Overhaul.

Do you have any feedback/things I can improve?

No, this is why I just reacted.

@czoli1976
Copy link

About time we get Simulcast for WHIP, holy cow.

@Sean-Der

This comment was marked as outdated.

@koolscooby
Copy link

This would be great. We'd like to have Twitch Stream Together users be able to use OBS and use Simulcast to send multiple qualities to improve the participant experience. It's a much better quality encoding & transmission experience compared to in-browser content contribution.

One UI nit / question: does checking the box force simulcast, or does it just enable it if the server/SDP exchange also indicates support for simulcast? If the latter, I'd recommend changing the UI label to Enable simulcast which makes it a bit clear that it is enabled on the client, but also requires the server to support it.

@Sean-Der

This comment was marked as outdated.

@koolscooby
Copy link

koolscooby commented Jul 19, 2024

@koolscooby Fixed! Changed from Use -> Enabled

If the server rejects Simulcast it will only accept the high layer.

In the future the server should be able to drive more (how many layers, suggestions around bitrate/resolution). For a V1 I think it just being .5x and .25x is a good trade-off on complexity vs value.

This seems like a completely sensible tradeoff to start getting users testing the underlying simulcast functionality in OBS & on the server side in a bigger set of network scenarios.

edit: ... without creating an enormous amount UI code or complicated UX.

@murillo128
Copy link
Contributor

Anything we can do to get this merged sooner (review/test/etc..)? From Dolby Millicast side we are really interested in this feature which would allow us to finally deprecate our custom fork.

@Fenrirthviti
Copy link
Member

We're aware this is here, and will get to it in due time. More testing/documentation is always welcome to help things.

@Sean-Der

This comment was marked as outdated.

@davidzhao
Copy link

There are a lot of LiveKit users that are excited about the ability to simulcast with OBS. Let me know if we can help in getting this across the finish line

@Fenrirthviti
Copy link
Member

Logging for posterity that this has an open RFC for OBS with unresolved feedback that has not been accepted.

While not impossible this is merged first, it's a little strange to submit a functional PR of a design that has not been accepted by the project or finalized yet. I appreciate the enthusiasm but "move fast, fix later" is at odds with how the project operates in general.

This is not a request for discussion on this point, I am posting for visibility of process.

@Sean-Der

This comment was marked as outdated.

@nils-ohlmeier
Copy link

We as Cloudflare are in support of adding Simulcast support to OBS. Having Simulcast support in OBS will enable new use cases for certain users and services.
I think the default values chosen in this PR for 1/2 and 1/4 resolution are used by a lot of video conferencing services in their Simulcast settings and make perfect sense as the initial values for this feature.

@Sean-Der Sean-Der force-pushed the simulcast branch 3 times, most recently from 010134b to f35d3e7 Compare August 24, 2024 03:29
@kwindla
Copy link

kwindla commented Aug 26, 2024

I'd like to weigh in to say that we (Daily.co) have users who need this. In general, our experience is that simulcast is critical for expanding the use cases and kinds of users who can stream successfully. Two examples: 1) users who don't have control over their upstream bandwidth. Perhaps they are on a corporate network with unpredictable congestion. 2) users who want to live stream and send the stream out to multiple different protocols/channels in real-time without re-processing.

@Sean-Der
Copy link
Contributor Author

Sean-Der commented Aug 29, 2024

I have had conversations with OBS Maintainers, WHIP Services and the WHIP spec author about Simulcast and have updated the PR from that feedback. We want to get an MVP into OBS Studio so we can build towards the next/future items and would appreciate another round of review now that I've refactored the implementation to follow the MVP.

(MVP) - Users are given a Spinbox to select how many Simulcast layers to send. If the remote server doesn't accept the configuration throw an error modal. A screenshot of this is in the PR description.

(Next) - Explore options to allow servers to configure/hint layers even before the user hits 'Start Streaming', but would not dynamically change while connected. Users wouldn't need to configure anything. The behavior would feel closer to Twitch's Enhanced Broadcasting.

(Future) Implement fully dynamic simulcast. OBS could enable/disable layers on the fly depending on CPU/GPU/Network usage, similar to how browsers implement WebRTC Simulcast.

@Sean-Der Sean-Der force-pushed the simulcast branch 2 times, most recently from 4fab92b to 847ad6d Compare September 18, 2024 14:21
@Sean-Der Sean-Der force-pushed the simulcast branch 3 times, most recently from 5ad9d03 to e808187 Compare December 24, 2024 18:20
@Bleuzen
Copy link

Bleuzen commented Dec 25, 2024

In this current form this is only really compatible with CBR mode (but the UI doesn't make that clear).
For example I can set the encoder to VBR mode and it only changes resolution and bitrate for each layer but the max bitrate stays the same for all layers which doesn't make sense.

So for this first version I suggest to either only allow users to enable Simulcast in CBR mode or add some special handling for other rate control modes (at least VBR makes sense because in my experience some browsers don't like the bitstream padding from CBR so VBR is more stable for WebRTC streams.)

For the future it would be cool to have entirely separate settings per layer. I'm not a fan of the calculated resolutions & bitrates here and would rather choose my own and also do things like having one layer with AV1 and another layer with H264. But that is something for the future / another PR I guess.

@Sean-Der
Copy link
Contributor Author

@Bleuzen those are all great enhancements. In the future I would like to see

  • Dynamic toggling of tracks (don’t generate if no one is viewing)
  • Multi codec
  • User selection of resolution/bitrate

It’s overwhelmingly complicated though. I hope we can keep this PR as is and just get it merged. If people find it useful I would love to do all this :)

<item row="1" column="1">
<layout class="QHBoxLayout" name="horizontalLayout_34" stretch="0,0">
<item>
<widget class="QSpinBox" name="simulcastTotalLayers">
Copy link
Member

Choose a reason for hiding this comment

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

Looks like the encoding size / bitrate information box from the simulcast RFC is missing

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@DDRBoxman I forgot to remove this from RFC. I was told this is too busy/too much UI.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated RFC

Copy link
Member

Choose a reason for hiding this comment

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

I think that this ends up with us back in a similar situation to the opaque checkbox we didn't like in the first place.

Can we add a link below there that pops up a dialog that has static text to explain how the different layers will be calculated and what else to expect.

UI/data/locale/en-US.ini Outdated Show resolved Hide resolved
</spacer>
</item>
<item>
<widget class="QLabel" name="simulcastInfo">
Copy link
Member

Choose a reason for hiding this comment

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

nit: it might be useful to put all of simulcast under a toggle. That way users don't have to think about an advanced setting even if the default is only the 1 layer.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't see this as an advanced setting. If you are streaming you are probably going to set this to more then 1 (because your streaming provider suggests it)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So the default flow is going to have an additional click.

plugins/obs-webrtc/whip-output.cpp Show resolved Hide resolved
plugins/obs-webrtc/whip-output.cpp Outdated Show resolved Hide resolved
UI/window-basic-main-outputs.cpp Outdated Show resolved Hide resolved
@Sean-Der Sean-Der force-pushed the simulcast branch 4 times, most recently from d4af744 to be19187 Compare December 27, 2024 15:19
@Sean-Der
Copy link
Contributor Author

@DDRBoxman I have moved Simulcast into a dedicated file. Is that a better feel?

@DDRBoxman
Copy link
Member

Yeah having that code in the new file does feel cleaner 👍

Since it's not applicable to not whip it might be worth changing the naming in window-basic-settings-stream and window-basic-main-outputs from just simulcast* to something more specific like whipSimulcast' so that future adventurers don't think that it's a more generic feature.

Aside from that I left a comment about having some other ui to at least explain what is in the layers.

@Sean-Der
Copy link
Contributor Author

@DDRBoxman Updated everything to be whipSimulcast instead of simulcast

Could we defer on the UI? I will schedule a 30 min meeting and we can go over that part again. Discussing UI over GitHub/Discord I have a hard time communicating details.

@Sean-Der Sean-Der requested a review from DDRBoxman December 28, 2024 04:34
@DDRBoxman
Copy link
Member

We should see if we can get @Warchamp7 on the same call so we're all on the same page

@Sean-Der
Copy link
Contributor Author

@DDRBoxman I sent a DM on discord.

Should I do the static text? Do you have an example that I should clone/copy in OBS already? I would love to just knock it out. Then I can start giving out builds!

I am just worried that if I make UI/UX changes that didn't get discussed I will get myself in trouble again

@DDRBoxman
Copy link
Member

Since the client will error if the number of layers doesn't match we're good on that front.

Warchamp suggested we could make a new OBS knowledge base page for this that we can link to. (not sure what the process to add one is but should be easy if we have the copy ready)

I was thinking that just adding a QLabel with an anchor tag that links there would be fine

@Sean-Der
Copy link
Contributor Author

Great! I will figure that out and make the page, and update PR.

thanks for getting the ‘next step’ on that :)

@Sean-Der Sean-Der force-pushed the simulcast branch 2 times, most recently from 6981fcc to af06f9b Compare January 8, 2025 21:24
@Sean-Der Sean-Der requested a review from Fenrirthviti January 8, 2025 22:59
@Sean-Der
Copy link
Contributor Author

Sean-Der commented Jan 8, 2025

I created a wiki page here https://github.com/obsproject/obs-studio/wiki/Streaming-With-WHIP#simulcast and linked to it.

Could I get another review, thank you!

@DDRBoxman
Copy link
Member

Yeah the href seems like a fine way to do that. I think this is supposed to be in the knowledge base and not the wiki though since that's more developer focused.

@Sean-Der
Copy link
Contributor Author

@DDRBoxman I am not able to do that, do you have access to it?

Once it is up I can change link right away!

@DDRBoxman
Copy link
Member

I think you'll have to ask in the Discord I don't actually know anything about the knowledge base system

@Sean-Der
Copy link
Contributor Author

@DDRBoxman done!

@Fenrirthviti offered to update it https://discord.com/channels/348973006581923840/939687060162363452/1327845941780156459

When that is done I will update and request another review, thanks again for all the help

@Sean-Der
Copy link
Contributor Author

@DDRBoxman PR has been updated with official URL. I think I got it this time :)

thank you @Fenrirthviti

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.

Cool, that covers the things that are on my radar 👍

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.