GNOME Bugzilla – Bug 776928
hlsdemux: Set fragment header uri if exists
Last modified: 2018-11-03 14:02:58 UTC
To allow downloading fragment hearder, set its uri if there is available "Media Initialization" parsed from EXT-X-MAP tag
Created attachment 342995 [details] [review] hls: m3u8: Parsing EXT-X-MAP tag to store initialization data EXT-X-MAP tag informs media initialization data, such as moov box in ISOBMFF case and PAT/PMT for MPEG TS stream.
Created attachment 342996 [details] [review] hlsdemux: Set fragment header uri if exists
MP4 test m3u8 http://tungsten.aaplimg.com/VOD/bipbop_adv_fmp4_example/master.m3u8
Review of attachment 342995 [details] [review]: ::: ext/hls/m3u8.c @@ +562,3 @@ + gst_m3u8_media_file_unref (self->init_media); + + self->init_media = file; Don't you need to set the offset and size? ::: ext/hls/m3u8.h @@ +66,3 @@ GList *files; + GstM3U8MediaFile *init_media; /* Media Initialization written in EXT-X-MAP */ HLS spec allows multiple initialization segments to happen. "It applies to every Media Segment that appears after it in the Playlist until the next EXT-X-MAP tag or until the end of the playlist." It should be designed to allow that. Maybe GstM3U8MediaFile needs a pointer to itself to store its initialization segment if available. Another option is to interleave it in the media files as it were a regular file. I'd prefer the first one to have it logically distinguished in the code. Or, if we do the second, add a boolean to indicate if it is a header.
Created attachment 343150 [details] [review] hls: m3u8: Parsing EXT-X-MAP tag to store initialization data
Review of attachment 343150 [details] [review]: ::: ext/hls/m3u8.c @@ +546,3 @@ + } else if (strcmp (a, "BYTERANGE") == 0) { + if (int64_from_string (v, &v, &size)) { + if (*v == '@' && !int64_from_string (v + 1, &v, &offset)) { Did you mean to capture into init_size and init_offset variables here?
(In reply to Jan Schmidt from comment #6) > Review of attachment 343150 [details] [review] [review]: > > ::: ext/hls/m3u8.c > @@ +546,3 @@ > + } else if (strcmp (a, "BYTERANGE") == 0) { > + if (int64_from_string (v, &v, &size)) { > + if (*v == '@' && !int64_from_string (v + 1, &v, &offset)) { > > Did you mean to capture into init_size and init_offset variables here? Right. It was omitted mistakenly. Now I'm preparing new patch to allow multiple init segments in a m3u8. I think Thiago's first suggestion (make pointer to access init segment per media segment) is correct approach and I also prefer it.
Created attachment 343397 [details] [review] hls: m3u8: Parsing EXT-X-MAP tag to store initialization data
Created attachment 343398 [details] [review] hlsdemux: Set fragment header uri if exists
Created attachment 343399 [details] [review] hlsdemux: Store initialization segment until pushing the first buffer Initialization Media segment has independent url, and the amount of initialization data may not be enought to do typefind. If so, since hlsdemux clears pending data of previous segment, the initialization data (included in typefind buffer at this moment) also be removed.
Hello all, I have question about supporting HLS fMP4. I'm keeping on test using following test vectors <APPLE provided> https://tungsten.aaplimg.com/VOD/bipbop_adv_fmp4_example/master.m3u8 <eurofins-digitaltesting> https://hlstests.eurofins-digitaltesting.com/ Mostly (not yet tests all vectors), the first buffer time stamp of fMP4 is not ZERO but starts from 10sec. That means, "base decode time" of the first fragment's tfdt box is 10 sec. Because HLS spec does not specify the starting PTS (like presentation time offset on MPEGDASH), hlsdemux never knows the starting buffer PTS. In this case, who has responsibility of parsing this? It's critical when setting up running/stream time on SEGMENT event. If hlsdemux should do, hlsdemux needs to parse moov (to figure out timescale) and the first moof (to figure out base decode time). the other option is, modifying TIME format SEGMENT in qtdemux based on the first PTS. Which one is correct approach?
Created attachment 344380 [details] [review] hls: m3u8: Parsing EXT-X-MAP tag to store initialization data Rebased
Created attachment 344503 [details] [review] hlsdemux: Do not clear/advance fragment by finished header downloading Header data must be forwarded to downstream, but if demux does not finish to finding type (e.g., ts, mp4 and etc), this header data can be cleared by _stream_clear_pending_data(). Moreover, although demux finish downloading header data, still it has fragment date to be downloaded, fragment sequence shouldn't be advanced yet at that moment.
(In reply to Seungha Yang from comment #11) > If hlsdemux should do, hlsdemux needs to parse moov (to figure out > timescale) and the first moof (to figure out base decode time). > the other option is, modifying TIME format SEGMENT in qtdemux based on the > first PTS. Parsing in hlsdemux, however qtdemux already takes the tfdt into account so why does it not work? See also https://bugzilla.gnome.org/show_bug.cgi?id=777825 for parsing
Review of attachment 343398 [details] [review]: ::: ext/hls/gsthlsdemux.c @@ +1049,3 @@ + if (header_file->size != -1) { + stream->fragment.header_range_end = + header_file->offset + header_file->size - 1; I assume you checked that the -1 here is absolutely correct?
Review of attachment 344380 [details] [review]: ::: ext/hls/m3u8.c @@ +715,3 @@ + last_init_file = file; + + self->init_files = g_list_append (self->init_files, file); Either prepend() and reverse() in the end, or use a GQueue. Repeated g_list_append() is quadratic ::: ext/hls/m3u8.h @@ +102,3 @@ + + GstM3U8MediaFile *init_file; /* Media Initialization corresponding to + * current segment, if exist (doesn't hold ref) */ This seems unfortunate as you have to keep both pointers in sync from two different lists now. It would be better to store a new struct inside the list of files that contains both.
Review of attachment 344503 [details] [review]: Makes sense
(In reply to Sebastian Dröge (slomo) from comment #14) > (In reply to Seungha Yang from comment #11) > Parsing in hlsdemux, however qtdemux already takes the tfdt into account so > why does it not work? See also > https://bugzilla.gnome.org/show_bug.cgi?id=777825 for parsing hlsdemux send segment.start to ZERO (with starting playback without seek). Let's assume that the tfdt/trun of the first fragment indicates the first buffer's PTS is 10 sec. Obviously qtdemux can set correct PTS of the first buffer PTS to 10 sec. But, segment.start (indicated by hlsdemux) was ZERO, basesink will wait 10 sec. The "10 sec" should be handled likewise "presentationTimeOffset" of dash I think. Finally, in above case, segment (from hlsdemux) should be start = position = 10 sec time = base = 0 I also agree with your opinion that setting segment should be done by hlsdemux. To allow it, I'm working on bug #777825 :)
(In reply to Sebastian Dröge (slomo) from comment #15) > Review of attachment 343398 [details] [review] [review]: > > ::: ext/hls/gsthlsdemux.c > @@ +1049,3 @@ > + if (header_file->size != -1) { > + stream->fragment.header_range_end = > + header_file->offset + header_file->size - 1; > > I assume you checked that the -1 here is absolutely correct? hls spec 4.3.2.5. EXT-X-MAP is saying that BYTERANGE attribute of EXT-X-MAP tag is optional. So it can be "-1"
Created attachment 344687 [details] [review] hls: m3u8: Parsing EXT-X-MAP tag to store initialization data - Defined new structure for init segment. - Removed list for init segment files, and each media files will have its own pointer for init segment
Created attachment 344688 [details] [review] hlsdemux: Set fragment header uri if exists Updated init segment's structure format
Created attachment 344689 [details] [review] tests: hls: Add a test case for EXT-X-MAP tag
Created attachment 344772 [details] [review] hls: m3u8: Parsing EXT-X-MAP tag to store initialization data Fix invalid pointer access
Created attachment 362962 [details] [review] [1/3] hls: m3u8: Parsing EXT-X-MAP tag to store initialization data rebase patch
Created attachment 362963 [details] [review] [2/3] hlsdemux: Set fragment header uri if exists
Created attachment 362964 [details] [review] [3/3] tests: hls: Add a test case for EXT-X-MAP tag
above patches are not enough to fMP4 + HLS support (iOS already support it). To do that, we need to use isoff parser to extract timestamp in hlsdemux
-- GitLab Migration Automatic Message -- This bug has been migrated to freedesktop.org's GitLab instance and has been closed from further activity. You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.freedesktop.org/gstreamer/gst-plugins-bad/issues/504.