-
-
Notifications
You must be signed in to change notification settings - Fork 221
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
feat: add hls support #2794
base: main
Are you sure you want to change the base?
feat: add hls support #2794
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #2794 +/- ##
==========================================
+ Coverage 70.35% 70.44% +0.09%
==========================================
Files 149 149
Lines 4034 4067 +33
==========================================
+ Hits 2838 2865 +27
- Misses 1196 1202 +6
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
i don't understand ci message (sorry I have versions problems with prettier...can't use git formatting) |
I hope to properly test this in the coming weeks. Could you also please update the docs describing this feature and how to configure/use it? |
The config file template is pretty self explanatory...HLS is composed of several streams with different quality embed in container with a format so you have to describe the container format and streams via a tuple (streamname,codec,bitrate)... |
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.
make a function because output.file.hls varies accross liquidsoap versions
Is there a way to stream HLS to nginx instead of doing it via a file? |
5f95254
to
bd4a99d
Compare
http://nginx-rtmp.blogspot.com/2017/07/introducing-nginx-ts-module-for-hls-and.html can be a lead |
After discussion, I think we should continue with the file-based solution instead - it is cleaner and induces less delay |
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.
Hi, I made first pass.
I think we should implement a simple working HLS feature, and iterate to add more options. Supporting too many formats/codecs is also risky, let's do not diverge from the spec.
I already started working on HLS a while back, did you take a look at my PR: #2234
$values['stream']['outputs']['shoutcast'], | ||
$values['stream']['outputs']['hls'] |
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.
Let's not merge HLS streams in the icecast/shoutcast streams. We can benefit from a fresh/clean settings schema, and I don't see the benefit from merging this in the "merged" streams.
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.
It's mandatory to do that, it doesn't work if i remove this line (merged is the array parsed in StreamSettings.php)
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.
Why is it mandatory? "It doesn't work" doesn't seem like a valid argument sorry 😉
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.
Streams settings data (url,codec..) presented in the player are retrieved from this merged outputs array
installer/config.yml
Outdated
# > must be one of ('mpegts', 'mp3', 'adts', 'mp4') | ||
# > this field is REQUIRED |
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.
Let's reduce the amount of format for now, extra format can be added and tested in future PRs.
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.
AC3/ADTS is in the spec, further it work as it should with hls.js so i'll let it .
I'll remove mp4 because i haven't achieve anything with liquidsoap 1.4 even if in spec...:/
ok to remove mp3
Also, I'll will move format property as stream property to fit liquidsoap stream model (it was a mistake to put it as hls property)
edit mp4 seems to require movflags=+dash+skip_sidx+skip_trailer+frag_custom+global_header
install
Outdated
@@ -628,6 +628,7 @@ link_python_app libretime-playout-notify | |||
|
|||
info "creating libretime-playout working directory" | |||
mkdir_and_chown "$LIBRETIME_USER" "$WORKING_DIR/playout" | |||
mkdir_and_chown "$LIBRETIME_USER" "$WORKING_DIR/playout/hls" |
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 prefer to have playout create the "$WORKING_DIR/playout/hls" directory, the "$WORKING_DIR/playout" is writable by libretime.
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.
the hls directory must be created before nginx starts so we can't let playout create this directory...no?
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.
Why ? We only read in this directory during a request, and response with 404 if no files are found. Sound doable to me.
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.
if you think it's safe to launch nginx and serve a not-yet-created directory, so i'm good with removing this line but it will require testing (with docker it's required -liquidsoap can't create output directory-)
$result = array_merge($result, [ | ||
$prefix . 'port' => $_SERVER['SERVER_PORT'], | ||
$prefix . 'host' => $_SERVER['SERVER_NAME'], | ||
$prefix . 'type' => 'hls', | ||
$prefix . 'bitrate' => '', | ||
]); |
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.
Hmm, I think we prefer to use the config public url and not the webserver configs.
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.
in the present configuration, you can use both as with other outputs:
- default is webserver
- if public url is provided it's use instead
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.
The rest of the project doesn't rely on the web server configuration, this would diverge from the current expectation.
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.
so you tell me to add settings for web server host/port?
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.
We use the Config::get('general.public_url')
(with the _raw key you get access to the URL object (League\Uri\Uri;).
libretime/legacy/application/models/StreamSetting.php
Lines 70 to 81 in e2cfbf4
if (!$result[$prefix . 'public_url']) { | |
$host = Config::get('general.public_url_raw')->getHost(); | |
$port = $result[$prefix . 'port']; | |
$mount = $result[$prefix . 'mount']; | |
$result[$prefix . 'public_url'] = "http://{$host}:{$port}/{$mount}"; | |
if ($result[$prefix . 'output'] == 'shoutcast') { | |
// The semi-colon is important to make Shoutcast stream URLs play instead turn into a page. | |
$result[$prefix . 'public_url'] .= ';'; | |
} | |
} |
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.
But I think we create an new function and not use the backward compatible and complex Application_Model_StreamConfig::getOutput function.
8b9dea9
to
29db68a
Compare
|
||
{% for stream in output.streams -%} | ||
|
||
streamout = %ffmpeg(format="{{output.format}}" , |
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.
note that %ffmpeg (as well as %audio for liquidsoap>2.0) arguments can't be dynamically passed through a function we can customize for different lq versions (as i did for output.file.hls).....So we can't maintain hls for both lq 1.4 and 2.0
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.
For the futur, if you really need to handle the %functions, you may use jinja templating and check against the version during the startup of playout.
Description
implement HLS .
rely on nginx to serve files as liquidsoap hls.harbor is not recommended for direct audience.
The cons is we loose listeners stats if hls.
a mount point have been created in nginx libretime.conf:
Note
Testing