-
-
Notifications
You must be signed in to change notification settings - Fork 8.4k
rtmp-services: Fix bugs blocking WHIP services #12033
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
base: master
Are you sure you want to change the base?
Conversation
Anything I could do to help get this prioritized? |
"if": { "required": ["protocol"], "properties": { "protocol": { "pattern": "^(HLS|SRT|RIST|WHIP)$" } } }, | ||
"if": { "required": ["protocol"], "properties": { "protocol": { "pattern": "^(HLS|SRT|RIST)$" } } }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This removes the requirement of the recommended
field for services using WHIP. Why?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Never mind. I have re-read the change and the PR description. It's removing the requirement for the output
key within the recommended
key. I would have to investigate more to better understand what is "best" here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mentioned that as point #2 in my description - that field is deprecated and there's no appropriate value for WHIP services. The existing options are:
"rtmp_output",
"ffmpeg_hls_muxer",
"ffmpeg_mpegts_muxer"
WHIP is none of those. I could certainly add a value called whip_output
in there, but it would be an additional enum on a deprecated field used by nothing and read by nobody. What's the point?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oop, missed your reply, all good :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use the commit prefix rtmp-services:
for both commits. Use Sentence case for the text after the prefix (Capitalize the first letter of the first word).
62212ed
to
701eb6a
Compare
@RytoEX Done! |
Description
Two small changes:
services.json
— previously there was a segfault when attempting to callrtmp_common_get_connect_info(data, OBS_SERVICE_CONNECT_INFO_BEARER_TOKEN)
. Shoutout to @Sean-Der to making this possible at all and @ImLunaHey for finding the fix!output
field. It wouldn't be correct for them to use a value likertmp_output
. I also didn't want to add an incorrectwhip_output
field to a deprecated enum? So I madeoutput
optional for services withprotocol: WHIP
. Not sure what the right answer is there.Motivation and Context
Enabling WHIP services to be added to services.json, such as Streamplace.
How Has This Been Tested?
Tested by building and streaming using macOS 15.3.1 (in conjunction with #12032)
Checklist: