-
-
Notifications
You must be signed in to change notification settings - Fork 220
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
Changes from 8 commits
7efe718
08b9d0e
ed56081
17f47ca
bc5050c
3119454
c62cb8f
8f3bde7
61aa1fb
76010bc
45af9ed
faa3f86
7da16a1
a439af8
4872cae
5ee1e84
28e83ad
2723f2b
2d62117
0833239
44f2dae
95316d6
07d9448
3a6aefd
6f76da0
06cf701
c7e95fd
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -317,6 +317,53 @@ stream: | |
# > default is false | ||
mobile: false | ||
|
||
hls: | ||
- # Whether the output is enabled. | ||
# > default is false | ||
enabled: true | ||
# > 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 commentThe 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 commentThe 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 |
||
format: mpegts | ||
# > segment_duration (default:2.0) | ||
segment_duration: 2.0 | ||
# > segments count (default:5) | ||
segment_count: 5 | ||
# > segments_overhead (default:5) | ||
segments_overhead: 5 | ||
# Output public url, If not defined, the value will be generated from | ||
# the [general.public_url] hostname, the output port and mount. | ||
public_url: | ||
# web server host. | ||
# > default is localhost | ||
host: localhost | ||
# webs server server port. | ||
# > default is 80 | ||
port: 80 | ||
# hls mount point | ||
# > this field is REQUIRED | ||
mount: hls/main | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The word "mount" is from the icecast/shoutcast vocabulary, I am not sure we want to have if in the context of HLS. I think this should also include the playlist extension .m3u8 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ok for m3u8 but i don't know if we need to change the mount property name it's quite coherent for a user perspective... There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I believe playlist is the word used in the spec. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Are you okay with these settings?: stream:
outputs:
# HLS output manifests.
# > max items is 10
hls:
- # Whether the output is enabled.
# > default is false
enabled: false
# Output public url. If not defined, the value will be generated from
# the [general.public_url] hostname/port.
public_url:
# hls playlist name.
# > this field is REQUIRED
playlist: "main.m3u8"
# > segment_duration (default:2.0)
segment_duration: 2.0
# > segments count: segments count buffered (default:5)
segment_count: 5
# > segments_overhead: segments count kept past the playlist size for those listeners who are still listening on outdated segments. (default:5)
segments_overhead: 5
streams:
- # > prefix of generated segments
# > this field is REQUIRED
segments_prefix: mp3_low
# > format of the container must be one of ('mpegts', 'adts')
# > this field is REQUIRED
format: mpegts
# > codec used for the stream, must be one of ( 'libmp3lame', 'aac')
# > this field is REQUIRED
codec: libmp3lame
# > bitrate of the stream
# > this field is REQUIRED
bitrate: 32k
# > audio sampling rate
# > this field is REQUIRED
sample_rate: 44100
- fragment_prefix: mp3_hifi
codec: libmp3lame
bitrate: 256k
sample_rate: 44100
|
||
streams: | ||
- # > prefix of generated fragment | ||
# > this field is REQUIRED | ||
fragment_prefix: mp3_low | ||
# > must be one of ( 'libmp3lame', 'flac', 'aac', 'libopus', 'libvorbis') | ||
# > this field is REQUIRED | ||
codec: libmp3lame | ||
# > bitrate of the stream | ||
# > this field is REQUIRED | ||
bitrate: 32k | ||
# > sampling rate (default: 44100Hz) | ||
sample_rate: 44100 | ||
- fragment_prefix: mp3_med | ||
codec: libmp3lame | ||
bitrate: 128k | ||
- fragment_prefix: mp3_hifi | ||
mp3butcher marked this conversation as resolved.
Show resolved
Hide resolved
|
||
codec: libmp3lame | ||
bitrate: 256k | ||
# Whether the stream should be used for mobile devices. | ||
# > default is false | ||
mobile: false | ||
|
||
# System outputs. | ||
# > max items is 1 | ||
system: | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -204,6 +204,37 @@ public function getConfigTreeBuilder() | |
/* */->booleanNode('mobile')->defaultFalse()->end() | ||
/**/->end()->end()->end() | ||
|
||
// Hls outputs | ||
/**/->arrayNode('hls')->arrayPrototype()->children() | ||
/* */->arrayNode('streams')->arrayPrototype()->children() | ||
/* */->scalarNode('fragment_prefix')->end() | ||
/* */->scalarNode('bitrate')->isRequired()->end() | ||
/* */->IntegerNode('sample_rate')->defaultValue(44100)->end() | ||
/* */->scalarNode('codec')->cannotBeEmpty() | ||
/* */->validate()->ifNotInArray(['aac', 'libmp3lame', 'flac', 'libopus', 'libvorbis']) | ||
/* */->thenInvalid('invalid stream.outputs.hls.streams.codec %s') | ||
/* */->end() | ||
/* */->end() | ||
/* */->end()->end()->end() | ||
/* */->scalarNode('format')->cannotBeEmpty() | ||
/* */->validate()->ifNotInArray(['mpegts', 'mp3', 'adts', 'mp4']) | ||
/* */->thenInvalid('invalid stream.outputs.hls.format %s') | ||
/* */->end() | ||
/* */->end() | ||
/* */->floatNode('segment_duration')->defaultValue(2.0)->end() | ||
/* */->integerNode('segment_count')->defaultValue(5)->end() | ||
/* */->integerNode('segments_overhead')->defaultValue(5)->end() | ||
/* */->booleanNode('enabled')->defaultFalse()->end() | ||
|
||
/* */->enumNode('kind')->values(['hls'])->defaultValue('hls')->end() | ||
/* */->scalarNode('public_url')->end() | ||
/* */->scalarNode('host')->defaultValue('localhost')->end() | ||
/* */->integerNode('port')->defaultValue(80)->end() | ||
/* */->scalarNode('mount')->cannotBeEmpty() | ||
/* */->validate()->ifString()->then($trim_leading_slash)->end() | ||
/* */->end() | ||
/* */->booleanNode('mobile')->defaultFalse()->end() | ||
/**/->end()->end()->end() | ||
// System outputs | ||
/**/->arrayNode('system')->arrayPrototype()->children() | ||
/* */->booleanNode('enabled')->defaultFalse()->end() | ||
|
@@ -270,7 +301,8 @@ private static function load() | |
// Merge Icecast and Shoutcast outputs | ||
$values['stream']['outputs']['merged'] = array_merge( | ||
$values['stream']['outputs']['icecast'], | ||
$values['stream']['outputs']['shoutcast'] | ||
$values['stream']['outputs']['shoutcast'], | ||
$values['stream']['outputs']['hls'] | ||
Comment on lines
+301
to
+302
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe 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 commentThe 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 commentThe 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 |
||
); | ||
|
||
self::$values = $values; | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. make a function because output.file.hls varies accross liquidsoap versions |
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-)