-
Notifications
You must be signed in to change notification settings - Fork 508
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
[HLS] Support Single File TS for HLS. #891
Conversation
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.
Thanks for working on this feature.
Had a suggestion on using PackedAudio approach instead, which can result in a better and simpler code.
See the inline comments for details.
@@ -1538,6 +1538,13 @@ def testEc3AndHlsSingleSegmentMp4Encrypted(self): | |||
['audio', 'video'], hls=True, test_files=['bear-640x360-ec3.mp4']), | |||
self._GetFlags(encryption=True, output_hls=True)) | |||
self._CheckTestResults('ec3-and-hls-single-segment-mp4-encrypted') | |||
|
|||
def testHlsByterange(self): |
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'd prefer calling it: testHlsSingleSegmentTs, as "hls-byte-range" could be for mp4 or ts.
@@ -33,12 +33,16 @@ | |||
'mp2t_media_parser.h', | |||
'mpeg1_header.cc', | |||
'mpeg1_header.h', | |||
'multi_segment_ts_segmenter.cc', | |||
'multi_segment_ts_segmenter.h', |
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.
Can you align it with the line above it?
} | ||
} else if (output_format == CONTAINER_WEBVTT || | ||
|
||
if (output_format == CONTAINER_WEBVTT || |
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 condition should include output_format == CONTAINER_MPEG2TS ||
too, as TS does not have init segment. Can you also update line 232 too?
@@ -21,7 +23,12 @@ Status TsMuxer::InitializeMuxer() { | |||
if (streams().size() > 1u) | |||
return Status(error::MUXER_FAILURE, "Cannot handle more than one streams."); | |||
|
|||
segmenter_.reset(new TsSegmenter(options(), muxer_listener())); | |||
if (options().segment_template.empty()) { |
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.
Prefer using the approach in PackedAudio: https://github.com/google/shaka-packager/tree/56908a83a76c85450d6b5834f3aa16e0660f4648/packager/media/formats/packed_audio.
Here is the summary:
- Having only one segmenter class instead of having one for SingleSegment and one for MultiSegment.
- The segmenter class writes the data into a temporary buffer first, which is already the case in the current TS segmenter implementation.
- Do not output the temporary buffer to the file in the segmenter, but in the muxer instead.
- In the muxer, the code can decide whether to write to a single file or a segment file based on the configuration. See https://github.com/google/shaka-packager/blob/56908a83a76c85450d6b5834f3aa16e0660f4648/packager/media/formats/packed_audio/packed_audio_writer.cc#L100 for an example.
- The muxer listener events are handled in the muxer. There is no need to handle that in the segmenter.
I think the code is cleaner and easier to maintain with this structure.
What do you think?
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 @kqyang, thanks for the review. Yes that makes sense. Let me try it out.
I had issues with this branch. Creating a new PR and branch at #934 |
Replaced by #934 |
This is based on comments at #891. The muxer is deciding whether to write to a single file or a segment file based on the configuration. Example: ``` ../packager 'in=TOS.ts,stream=video,output=tos_video.ts,playlist_name=tos_video.m3u8' \ 'in=TOS.ts,stream=audio,output=tos_audio.ts,playlist_name=tos_audio.m3u8' \ --hls_master_playlist_output tos.m3u8 ``` Tested the content using Exoplayer. --------- Co-authored-by: Cosmin Stejerean <[email protected]>
This PR is related to #291.
Example:
Tested on Exoplayer.