Skip to content

RFC 45-1: Add protocol in Outputs API and check service protocol in the "UI" #5104

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.

Sign up for GitHub

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

Jump to bottom
Closed
tytan652 wants to merge 3 commits into from
Closed

RFC 45-1: Add protocol in Outputs API and check service protocol in the "UI" #5104

tytan652 wants to merge 3 commits into from

Conversation

@tytan652
Copy link
Collaborator

@tytan652 tytan652 commented Aug 9, 2021 •
edited
Loading

Description

This PR is about the RFC 45.

  • Add a "protocols" and "protocols_prefixes" fields in obs_output_info and add protocols to outputs meant for serv
  • Make the UI skip services where the protocol is not registered and also skip RTMPS servers for service providing RTMP and RTMPS server when the later is not registered.

Motivation and Context

Part of the implementation of the RFC 45.

How Has This Been Tested?

Artifacts from Github actions will not much help with testing this PR since it build with all protocol support.

  • Build OBS without FTL support and check if "Glimesh" service is not shown.
  • Build OBS without RTMPS support and check if "YouTube - RTMPS" RTMPS servers, "Facebook Live" service and "STAGE TEN" are not shown, RTMPS servers from other services are also not shown.
  • Build OBS with support for both and check those three services are shown.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
    • Showing services that are not supported is sort of a bug
  • New feature (non-breaking change which adds functionality)
  • Documentation (a change to documentation pages)

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 Enhancement Improvement to existing functionality label Aug 9, 2021
@tytan652 tytan652 marked this pull request as ready for review August 9, 2021 12:26
@tytan652 tytan652 marked this pull request as draft August 9, 2021 12:30
@tytan652 tytan652 mentioned this pull request Aug 9, 2021
Closed
@tytan652 tytan652 changed the title Add Protocol API basics and protocol check in rtmp-services Add protocol in Outputs API and protocol check in rtmp-services Aug 21, 2021
@tytan652
Copy link
Collaborator Author

tytan652 commented Aug 21, 2021 •
edited
Loading

Since I re-written the RFC 45, this PR needed also a rewrite. So rewrite done, only the doc addition is missing.

@tytan652 tytan652 force-pushed the rfcs_phase_1 branch 5 times, most recently from 9ae4dec to dc60332 Compare September 25, 2021 12:37
@tytan652 tytan652 force-pushed the rfcs_phase_1 branch 5 times, most recently from 83dd342 to d375551 Compare October 27, 2021 16:30
@tytan652 tytan652 changed the title Add protocol in Outputs API and protocol check in rtmp-services RFC 45: Add protocol in Outputs API and protocol check in rtmp-services Oct 27, 2021
@tytan652 tytan652 changed the title RFC 45: Add protocol in Outputs API and protocol check in rtmp-services RFC 45 part 1: Add protocol in Outputs API and protocol check in rtmp-services Oct 27, 2021
@tytan652 tytan652 changed the title RFC 45 part 1: Add protocol in Outputs API and protocol check in rtmp-services RFC 45 : Add protocol in Outputs API and protocol check in rtmp-services Oct 29, 2021
@tytan652 tytan652 changed the title RFC 45 : Add protocol in Outputs API and protocol check in rtmp-services RFC 45, PR N°1: Add protocol in Outputs API and protocol check in rtmp-services Oct 29, 2021
@tytan652 tytan652 force-pushed the rfcs_phase_1 branch 3 times, most recently from b49ae81 to f7e1611 Compare October 30, 2021 13:27
@tytan652 tytan652 marked this pull request as ready for review October 30, 2021 17:45
@tytan652 tytan652 force-pushed the rfcs_phase_1 branch 4 times, most recently from 8652586 to 44a39a2 Compare March 1, 2023 09:46
@tytan652 tytan652 force-pushed the rfcs_phase_1 branch 2 times, most recently from c78833b to bba571e Compare March 11, 2023 20:05
@tytan652
Copy link
Collaborator Author

tytan652 commented Mar 11, 2023 •
edited
Loading

Remove prefix related changes as it was done in the RFC.

@tytan652 tytan652 force-pushed the rfcs_phase_1 branch 2 times, most recently from 71552a0 to 2fcb9e4 Compare March 12, 2023 11:21
derrod
derrod reviewed Mar 12, 2023
View reviewed changes
@tytan652 tytan652 force-pushed the rfcs_phase_1 branch 2 times, most recently from 6b03e36 to e7a01eb Compare March 13, 2023 09:44
@tytan652
Copy link
Collaborator Author

tytan652 commented Mar 13, 2023 •
edited
Loading

No longer enforce the "protocol" field in the JSON if the server URL is a RTMP(S) one.

@tytan652 tytan652 requested a review from jp9000 March 13, 2023 09:54
@tytan652 tytan652 force-pushed the rfcs_phase_1 branch 4 times, most recently from 5e55fa0 to 47d5fee Compare March 15, 2023 13:45
tt2468
tt2468 reviewed Mar 16, 2023
View reviewed changes
plugins/obs-ffmpeg/obs-ffmpeg-hls-mux.c
.id = "ffmpeg_hls_muxer",
.flags = OBS_OUTPUT_AV | OBS_OUTPUT_ENCODED | OBS_OUTPUT_MULTI_TRACK |
OBS_OUTPUT_SERVICE,
.protocols = "HLS",
Copy link
Member

@tt2468 tt2468 Mar 16, 2023

Choose a reason for hiding this comment

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

As described in voice discussions, I personally believe that using uppercase for these values does not align with common lowercase OBS implementations like the .encoded_video_codecs field. While I understand the desire to use the strings from this value directly in the UI, I personally believe that it would be better to uppercase convert these values in the UI and use a hash table if we someday get a protocol which has mixed case.

As of this comment, I'm not aware of any protocols which we officially support which use mixed case: RTMP, RTMPS, SRT, RIST, FTL, HLS, WHIP

Copy link
Collaborator Author

@tytan652 tytan652 Mar 16, 2023

Choose a reason for hiding this comment

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

I personally believe that using uppercase for these values does not align with common lowercase OBS implementations like the .encoded_video_codecs field.

If you check #8090, you will see that the whole lowercase/uppercase situation on codecs is not aligned.

Copy link
Member

@tt2468 tt2468 Mar 16, 2023

Choose a reason for hiding this comment

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

I was drawing a parallel to codecs in regards to them being all lowercase, even though when presented to the user they are given in their correctly-capitalized form. I was not using it as an example of the uppercase-convert suggestion.

Copy link
Collaborator Author

@tytan652 tytan652 Mar 16, 2023

Choose a reason for hiding this comment

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

even though when presented to the user they are given in their correctly-capitalized form

Technically we don't, we expose to the user encoders names or presets names.

@tytan652 tytan652 force-pushed the rfcs_phase_1 branch 5 times, most recently from 41f1455 to c51837b Compare March 18, 2023 20:20
tytan652 added 3 commits March 19, 2023 15:29
@tytan652
libobs,docs: Add protocol in Outputs API
201bcda
@tytan652
obs-outputs,obs-ffmpeg: Add protocol to service outputs
855956f
@tytan652
rtmp-services: Add protocols to services JSON
513c6bc 
If the server URL is not an RTMP(S) URL, the protocol field becomes
required.

The output field becomes required on non-RTMP(S) services to keep
backward compatibility.

Also skip service if the protocol is not available.
@jp9000 jp9000 closed this in #8095 Mar 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@tt2468 tt2468 tt2468 left review comments

@jp9000 jp9000 Awaiting requested review from jp9000

+2 more reviewers

@derrod derrod derrod left review comments

@pkviet pkviet pkviet left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Enhancement Improvement to existing functionality

Projects

None yet

Milestone

OBS Studio 29.1

Development

Successfully merging this pull request may close these issues.

8 participants

@tytan652 @jp9000 @derrod @pkviet @tt2468 @RytoEX @WizardCM @cg2121