UI: Add support for backup/redundant streams

[obinnaokechukwu]

Description
This change adds support for having simultaneous streams to multiple backends of the same streaming service provider.

Motivation and Context
To enhance the reliability of a stream, it is sometimes helpful to send multiple (exact) copies of the same video stream to various backends of the same streaming platform. The redundant copy of the video can then be used as a backup when there is a problem with the main video stream.

This change adds the ability to send redundant copies of a given stream to the same streaming service provider.

Redundant streams can be enabled by specifying a backup_urls field in the services.json file and adding a url to it.

                {
                    "name": "Server 1 and Server 2",
                    "url": "rtmps://server1.foo.bar",
                    "backup_urls": [
                        "rtmps://server2.foo.bar"
                    ]
                }

How Has This Been Tested?
Verified changes work in the dual-ingestion-final branch.

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.

[RytoEX]

Per our Contributing guildelines, please make these changes:

1. Do not use periods at the end of commit message subjects (the first line of commit messages).
2. Do not exceed 70 characters for total commit message subject length. Try to keep the text after the prefix to 50 characters.
3. Wrap commit message bodies to 72 characters. Do not exceed 72 characters per line in the commit message body.

Also, please rebase this on the latest git HEAD and resolve any merge conflicts.

[obinnaokechukwu]

Thanks for taking a look. I have fixed the formatting and rebased to HEAD.

[tytan652]

Firstly, I want to specify I have nothing against this feature. But for me backup services should be generated at the last moment (when the stream starts) by the service itself if the UI ask for it:

The actual UI generate a service settings "object" and then create a service "object" (obs_service_t) with it. And then this service is normally manipulated only with libobs Service API functions.

In my opinion the feature should be something like:

The backup URLs became a settings of the service like the URL.

When the stream starts, the UI ask with a libobs function to the service "object" (if there is backup URLs) to generate copy of itself with the server URL replace with a backup one (if there is backup URLs).

And the the UI starts streams for each of those services.

The functions that generate "backup services" should maybe be similar to libobs enum functions and his implementation is provided through registered obs_service_info.

Edit: Actually making a new comment

[jp9000]

I have not really reviewed the code yet, so right now I'm mostly discussing the implementation. There are a few things I have questions for:

1. Let's say we have two streams going specifically for redundant streaming, stream 1 and stream 2.

   • You start them up, and they're sending out the same exact data.
   • However, let's say stream 1 disconnects, so the service says, "okay, we'll use stream 2".
   • Stream 1 reconnects, but due to the way RTMP works, timestamps for stream 1 are reset back to 0.
   • Stream 2 disconnects, but now it has no way to know where Stream 1 is because its timestamps are now desynchronized from the original stream.

   My question would be does it care about timestamps after a disconnect? If it does, then what happens in this scenario? If it doesn't, then I assume it just starts using the alternate stream's data where ever it currently is, correct? I feel like the latter scenario has to be the more likely way that the server would have to handle it, right? Just start reading the alternating stream's data where ever it is, right?

2. What interruption scenarios does this specifically handle? I feel like the only scenario this properly covers is when the ingest or nodes to the ingest have to disconnect the user for whatever reason. It would not handle local internet issues because then both streams would likely have issues, correct? Just kind of throwing this out there but just wanted to say that there's another approach we implement for this: https://github.com/facebookincubator/rtmp-go-away

   I feel like the only way we'd be able to properly handle local internet issues is if each connection were using a different internet provider, which would be a pretty complicated thing to implement.

And to respond to @tytan652, my question for you tytan is are you sure that we should we have this implemented as an actual API? The idea behind this PR that @WizardCM and I had originally discussed was that this would be a "temporary" thing that we would swap out for proper multi-ingest support later when (if?) we reimplement services. (I put temporary in quotes because as the saying goes nothing's more permanent than a temporary solution, but at least the hope is that it would be temporary).

[obinnaokechukwu]

My question would be does it care about timestamps after a disconnect? If it does, then what happens in this scenario? If it doesn't, then I assume it just starts using the alternate stream's data where ever it currently is, correct? I feel like the latter scenario has to be the more likely way that the server would have to handle it, right? Just start reading the alternating stream's data where ever it is, right?

The intent of the implementation was to be protocol agnostic and not specific to RTMP, so it spins up several obs_output_t on startstream() and lets them push the same stream to the server. The server then decides how to handle the redundancy. In our case, the system makes no assumptions about timestamps being aligned across multiple streams.

What interruption scenarios does this specifically handle? I feel like the only scenario this properly covers is when the ingest or nodes to the ingest have to disconnect the user for whatever reason. It would not handle local internet issues because then both streams would likely have issues, correct? ... I feel like the only way we'd be able to properly handle local internet issues is if each connection were using a different internet provider, which would be a pretty complicated thing to implement.

You are correct that ideally the copies are pushed via distinct ISP connections, however the stream ingestions are targeted to distinct geographic regions, so it provides protection from any network/system issues after the two paths split.

[tytan652]

And to respond to @tytan652, my question for you tytan is are you sure that we should we have this implemented as an actual API?

If it's a "temporary" thing then it change some things to how I perceive this PR. The answer would be no.

So below will be a review of the whole PR about the way it's implemented and a little about bugs and mistake that I found while reading the code.

Avoiding to have my RFCs in mind

Settings and Auto-config service UIs

Allow redundant streams should be an option that the user should be able to enable if the selected server is "capable" of it.

I'm completely against adding a brand new server entry containing an already added "Primary" server, I prefer adding a field in the "Primary" server entry which allow to add the backup server in the services.json. And since it's temporary and only one service is affected, only one backup server maximum should be considered.

Also, for now servers are identified by their URLs, changing that just for a "temporary" solution is not a good idea for me.

How Auto-config behave with redundant streams

I just want to say that how redundant configs should be tested against non-redundant configs should be discussed or re-discussed.

How redundant stream servers are stored

The backup server is a server not another service, so storing it since the beginning as a new service is a no-go for me.

Since it's "temporary", I think that backup the URL should be added in the service data and when the stream is started, the backup URL a copy of the actual service should be made with the URL replaced (in the UI not by libobs) and this backup "service", should be stored as a BasicOutputHandler attribute for now.

Front-end API

Since it's temporary, I don't know if giving access to the backup server output will be a good idea.

With my RFCs in mind

If it's possible, this feature should wait until that everything that needs to be fixed about services is fixed and the UI code is made ready to handle multiple streams.

The more we add service/stream features without fixing issue, the longer it will take to improve/fix everything.

Multi-stream should be made first and then multi-ingest, because while multi-ingest is developed people that needs that can do it by using the multi-stream feature and I repeat again only one service seems to provide this feature for now.

My RFCs 45 and 39 are to improve the UI/UX of services, 45 will help with codec compatibility and 39 (once rewritten after 45 and 47 got reviewed and etcetera) while allowing third-party service plugins, it could also lead to multi-streaming feature more easily (at least in my head).

And now mistakes and potentials bugs

• The text is not translatable
• Stats widget outputs lists is not dynamically changed if we switch service/server or profile.
• [window-basic-main-outputs.cpp] Outputs get destroyed before getting disconnected from signals, and also since the output type is usually the same those outputs are not cleared when needed.

[obinnaokechukwu]

Thanks for your comments and suggestions! I'll take a look at the potential issues.

On the first 3 points mentioned (settings, auto config, and storage), I've actually had earlier discussions about these specific points in which it was either directly suggested or okay-ed to take this approach (since I wanted to make sure the design is satisfactory enough before going ahead with its implementation). The point you brought up about removing access to backup services from the frontend-api sounds good, I can move that into the window-basic-main and access the outputs from there if that's okay.

Also, looking at the RFCs you referenced, I'm not actually sure this PR fundamentally makes the RFCs more difficult to implement. And unless I'm missing something here, it might even simplify things a bit. Regarding timelines, since the RFCs mentioned haven't been fully implemented, wouldn't it be fair to say that this PR can go in first? Especially since the actual details of the RFCs are still being ironed out and they seem to have been open for a fair amount of time and we also don't have a landing date at the moment.

Thanks again for your comments and suggestions.

[tytan652]

Also, looking at the RFCs you referenced, I'm not actually sure this PR fundamentally makes the RFCs more difficult to implement. And unless I'm missing something here, it might even simplify things a bit. Regarding timelines, since the RFCs mentioned haven't been fully implemented, wouldn't it be fair to say that this PR can go in first? Especially since the actual details of the RFCs are still being ironed out and they seem to have been open for a fair amount of time and we also don't have a landing date at the moment.

Don't worry about this part of my comment.

Edit: I think my intention there was to tell that I want to avoid having to re-do your work once merged because my RFCs may require it because:

• 39 changes plugins that provide services so how backup server are found will broke.
• 45 change some things in the output handler code.

But this is my problem not yours, so again don't worry about this part of my comment.

[obinnaokechukwu]

I've addressed the issues you brought up -- outputs api removal, translatable text, dynamic stats page and output disconnection.

[tytan652]

I think some things have no need to be set multiple time in the for loop.

Also audio encoder creation should be done once, and used by all outputs.

[obinnaokechukwu]

I have resolved the followup comments.

Also audio encoder creation should be done once, and used by all outputs.

Audio encoders are already created once in the constructor, only when the audio has multiple tracks are there multiple encoders, but this is the case today in OBS as well and not a new addition in this PR (see here).

[norihiro]

https://github.com/torvalds/linux/blob/master/Documentation/process/coding-style.rst

Do not unnecessarily use braces where a single statement will do.

The coding guideline request not to have unnecessary braces.

[Fenrirthviti]

Setting aside the code review itself for a moment, during our review and testing, we've come to realize that the actual problem this is looking to solve is not as clear as we would like. The discussion has been around what the solution does, but not the actual problem itself, so we've been making assumptions and trying to guess what the actual problem here is. We have a few members of the team who are heavily involved in production broadcast (video delivery), and have provided a perspective that this kind of georedundancy solution of multiple ingest points is more or less pointless when the source is sending over the same connection. This introduces an unavoidable bottle neck, both with the source connection itself, and any third party (i.e. ISP) infrastructure along the route that might be shared between both points before it's routed differently. Currently, this doesn't allow for the backup server to be assigned to a separate network interface, though we aren't sure that would even be a valid solution either.

With the limited understanding we have, the idea that an ingest point might need to temporarily go offline, we feel that RTMP Go Away might be the better option here, which has already been implemented in OBS and tested to work properly.

If Go Away does not meet the requirements, we would like to request a clear, detailed, unambiguous technical explanation of the issue and what this is expected to solve, as this is adding a fair level of complexity and potential user confusion for the problem we don't fully understand.

As a side note, we are also not particularly happy with how this UI/UX has turned out in practice, though we recognize that it part of it was our suggestion for things to be implemented this way. Once we understand the true requirements here, we can have a more informed discussion on how this should look as a final end state, and move forward with what changes, if any, are necessary.