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/UI: Add Bitmovin #6502

Conversation

CMThF
Copy link
Contributor

@CMThF CMThF commented May 17, 2022

Description

Added an rtmp common service for Bitmovin.
Bitmovin's configuration and connection workflow is a little different from the others, so the default UI and rtmp service handling had to be slightly adapted for it:

  • instead of a stream key, an API key is required. That key authenticates the user. Relabeled the key input field for Bitmovin service.
  • with the API key, all available live streams of a user can be fetched.
  • in order to do so, a SIGNAL was introduced that notifies when the key field gets input. That way we can fetch as soon as a valid key is available.
  • to allow the user to select a stream, the server dropdown menu was repurposed (and relabeled).

bitcodin_settings_none
bitcodin_settings_with_stream_selected

Motivation and Context

Built-in rtmp service support for Bitmovin live streaming.

How Has This Been Tested?

Manual tests on Ubuntu 20.04 and 18.04, Lenovo Thinkpad P14S G2.
Tested various scenarios of streaming, connecting, disconnecting, killing streams while streaming, invalid/valid API keys, persisted settings on startup, switching between rtmp services in settings, with valid and invalid streams, streams that invalidate between setting and starting, ...

Most changes only affect the bitmovin service. Changes in UI are only in effect if the service name is Bitmovin, similar to service-specific handling of Dacast or Facebook.

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.

@CMThF CMThF force-pushed the feature/bitmovin-rtmp-service branch from f4d2463 to abe0e29 Compare May 17, 2022 14:46
@gxalpha gxalpha added the Services These are modifications to the Service list and are not tied to the release schedule label May 17, 2022
@CMThF CMThF changed the title rtmp-services/UI: Add Bitmovin rtmp service rtmp-services/UI: Add Bitmovin May 18, 2022
@CMThF CMThF force-pushed the feature/bitmovin-rtmp-service branch 2 times, most recently from 5d9575a to 0ec2a52 Compare May 18, 2022 10:56
@CMThF CMThF marked this pull request as ready for review May 18, 2022 12:31
@jp9000
Copy link
Member

jp9000 commented May 21, 2022

What an absolute nightmare.

@CMThF
Copy link
Contributor Author

CMThF commented May 23, 2022

@jp9000 Hi Jim,
can you be a bit more specific please?

I think in terms of how the framework for these services is structured, there is not much different to other rtmp services, which rename fields, pull dynamic content or have dedicated handling (facebook, dacast, twitch...) as well and in the same way.

I also talked with @skeletonbow on discord about how one can modify the dialog from within the service - for those that have different work-flows. He also suggested the way Yt plugin does it - which is even more invasive.

Do you have a suggestion, how I can make this less nightmare'y? :)

edit: rebased on current state - heh, so schema validation for the json files has been introduced, and now we need at least one entry for servers.
'description': 'RTMP(S) or HLS URL of the ingest server.'
The url is not known at this point. Bitmovin creates an ingest on any cloud provider (customer's choice). The ingest url is known no earlier than the start of the stream.

@CMThF CMThF force-pushed the feature/bitmovin-rtmp-service branch from 0ec2a52 to 852e104 Compare May 23, 2022 07:07
@derrod
Copy link
Member

derrod commented May 23, 2022

The url is not known at this point. Bitmovin creates an ingest on any cloud provider (customer's choice). The ingest url is known no earlier than the start of the stream.

Since you're presumably hiding the normal server list anyway, you could just add a dummy entry here. Not sure if we wish to alter the schema for a such a special case. But we'll have to add it to the ignore list for the service checker anyway.

@CMThF
Copy link
Contributor Author

CMThF commented May 23, 2022

@derrod Yes, I thought that is the straight-forward solution. I'll do that.
However, I fear that will not improve @jp9000's general opinion on it

@derrod
Copy link
Member

derrod commented May 23, 2022

Yes, unfortunately the current service system kind of sucks.

We want to change it. But it's also one of those things that will take a lot of time and effort to redo, and nobody has sat down and done it yet since there are many design problems that need to be solved first. So we've ended up with things that we don't really want (service-specific code) and decisions we'd rather not make (who gets in and who doesn't).

There are a number of efforts underway to change this, such as obsproject/rfcs#39 and obsproject/rfcs#10 as well as a plugin manager/repository system. Then we can let services ship their own plugins without having to be in the main codebase and/or make it very simple for users to add them via a URI handler. Unfortunately that's still a bit off, so for now we'll have to deal with this.

I don't want to speak for Jim, but I believe his comment is an expression of the frustration at this situation plus having another larger PR to review, rather than a comment on the implementation or code quality itself.

@CMThF CMThF force-pushed the feature/bitmovin-rtmp-service branch from 852e104 to 6f78bb1 Compare May 23, 2022 09:38
@CMThF
Copy link
Contributor Author

CMThF commented May 23, 2022

Thanks for the clarification!
I fully understand. I was a bit taken aback by how much I had to change in the core source, or that I even had to touch anything in the core at all.

Love to see that there is improvement in the works!
I'm going to read into it, maybe I can contribute a bit in the future.

@CMThF CMThF force-pushed the feature/bitmovin-rtmp-service branch from 6f78bb1 to a8da4de Compare May 23, 2022 10:07
@jp9000
Copy link
Member

jp9000 commented May 23, 2022

I apologize for my rather terse response.

Could you give me exactly what a person would have to go through, from first contact with bitmovin to stream?

i.e. to give you an example of what someone would have to do to stream on twitch, let's go through the steps:
1.) Make an account on Twitch
2.) Either copy your stream key from your twitch settings into OBS' settings, or login to the account in OBS
3.) Press the start streaming button

For yours, it seems a slight bit more complicated than that, because you have this concept of "API Keys" and "Running Streams". I take it your service is not normally intended for the average person? Would I be correct in making the assumption that this is aimed at a particular type of power user rather than a regular content creator?

@jp9000
Copy link
Member

jp9000 commented May 23, 2022

The json file being unintentionally completely reformatted isn't really the issue I have with this PR, although you're going to have to fix that.

There are two things that probably cause me the utmost of frustration with this PR: one is the code of course, the amount of code required just to get your service actually seemingly functional, I guess. The second is that from a user's standpoint, it's still complicated and unpleasant for the average content creator, to the point to where I'd argue I probably wouldn't want to use it if I were an average content creator. But by judging from your web page, this is not for the average user, which I guess might make your complicated setup make sense, but I still can't help but wonder why, just why do things always have to be so god forsakenly complicated.

Everything about this whole situation just feels unpleasant to me. I don't want to necessarily be one to judge what a service chooses to do or how it chooses to do things, but when someone asks me to merge their code, I can't really help but see all this super custom code that deviates from what services normally do and wonder, just what on earth happened with that service to make decide to go with a seemingly unpleasant user workflow. And you know, complexity and features or fine; the issue I have is that you require it.

I just look at this situation and think to myself, "why didn't they just provide a stream key, update the services file, and call it a day instead? Or maybe the concept of logging in might make more sense." Everything about this just deviates from the norm and it just causes me to put both of my hands on my face and just slowly drag them down off of my face and blink a few times with a bewildered expression while taking a deep sigh.

I apologize that you have to deal with me here, because I'm sure you'd rather be doing anything else. Maybe this workflow wasn't something you decided, maybe it was someone else in the company, in which case I apologize for making you have to listen to me. I just needed to express some feelings about this.

@CMThF CMThF force-pushed the feature/bitmovin-rtmp-service branch from a8da4de to b6227b8 Compare May 24, 2022 10:57
@CMThF
Copy link
Contributor Author

CMThF commented May 24, 2022

Thanks for the feedback, this is actually brilliant insight into the kind of frustration we want to avoid - so note taken on the need to simplify it further.

You are totally correct that today our users are not Twitch streamers or single users streaming to 3rd party platforms. Most of our large accounts are big media companies. However, recently in 2020 and 2021 we have seen an increase in demand from smaller companies, that use video with their business. Typically they have some technical expertise but not a specialist in video, we often see people in these companies using OBS and the RTMP service - well actually we also hear from big media companies where engineers use OBS for testing too - currently users can only use the custom profile and need to know a bit about RTMP, we wrote this guide to help them get going (https://bitmovin.com/docs/encoding/tutorials/contribution-encoder-obs-studio-example). We wrote this plugin to make life simpler for them, now they just need an account with Bitmovin, create some live encoding streams then select the one they want in OBS. The workflow works like:

  1. Create an account - (most likely to be either Free trial, PAYG or Starter Pack)
  2. Using either our API or UI create a new Live Encoding, specifying it should have an RTMP input) - take a look at the tutorial link up there (or with the More Info button). Spinning up encoding instances can take a while though.
  3. Copy/Paste your API key in the field (Get API key links you directly to your account page)
  4. The service instantly fetches all your running live streams (that could be many, shared through your whole organisation) - it does so as soon as it sees a valid API key.
  5. Select a stream, and you're done. The service fetches address and key and generates the rtmp URL for you.

Not perfect, but an improvement and hopefully easier for the users with restrictions we have server side today.
If we see a lot of people connecting via OBS then we'll invest further in doing the server side work that would allow us to support this type of simplified workflow. With our current system it is a major undertaking to get there.
We would be happy to provide you with a free trial account, to test if that helps validate this workflow.

@CMThF CMThF force-pushed the feature/bitmovin-rtmp-service branch from b6227b8 to f115832 Compare May 30, 2022 13:49
@CMThF CMThF force-pushed the feature/bitmovin-rtmp-service branch from f115832 to f756c1e Compare June 9, 2022 07:00
@gxalpha gxalpha added the UI/UX Anything to do with changes or additions to UI/UX elements. label Aug 3, 2022
@CMThF CMThF mentioned this pull request Dec 2, 2022
6 tasks
@gxalpha
Copy link
Member

gxalpha commented Dec 2, 2022

With #7886 merged, is this PR still necessary?

@CMThF CMThF closed this Dec 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Services These are modifications to the Service list and are not tied to the release schedule UI/UX Anything to do with changes or additions to UI/UX elements.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants