| Message ID | 1504260919-24104-1-git-send-email-kjeyapal@akamai.com |
|---|---|
| State | New |
| Headers | show |
2017-09-01 18:15 GMT+08:00 <kjeyapal@akamai.com>: > From: Karthick J <kjeyapal@akamai.com> > > Signed-off-by: Karthick J <kjeyapal@akamai.com> > --- > libavformat/hlsenc.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/libavformat/hlsenc.c b/libavformat/hlsenc.c > index 4a908863..dd36fde 100644 > --- a/libavformat/hlsenc.c > +++ b/libavformat/hlsenc.c > @@ -1,6 +1,7 @@ > /* > * Apple HTTP Live Streaming segmenter > * Copyright (c) 2012, Luca Barbato > + * Copyright (c) 2017 Akamai Technologies, Inc. > * > * This file is part of FFmpeg. > * > @@ -1039,7 +1040,8 @@ static int hls_window(AVFormatContext *s, int last) > > for (en = hls->segments; en; en = en->next) { > if (target_duration <= en->duration) > - target_duration = get_int_from_double(en->duration); > + target_duration = (hls->flags & HLS_ROUND_DURATIONS) ? > + lrint(en->duration) : get_int_from_double(en->duration); > } > > hls->discontinuity_set = 0; > -- > 1.9.1 > > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel LGTM
2017-09-01 18:32 GMT+08:00 Steven Liu <lingjiujianke@gmail.com>: > 2017-09-01 18:15 GMT+08:00 <kjeyapal@akamai.com>: >> From: Karthick J <kjeyapal@akamai.com> >> >> Signed-off-by: Karthick J <kjeyapal@akamai.com> >> --- >> libavformat/hlsenc.c | 4 +++- >> 1 file changed, 3 insertions(+), 1 deletion(-) >> >> diff --git a/libavformat/hlsenc.c b/libavformat/hlsenc.c >> index 4a908863..dd36fde 100644 >> --- a/libavformat/hlsenc.c >> +++ b/libavformat/hlsenc.c >> @@ -1,6 +1,7 @@ >> /* >> * Apple HTTP Live Streaming segmenter >> * Copyright (c) 2012, Luca Barbato >> + * Copyright (c) 2017 Akamai Technologies, Inc. >> * >> * This file is part of FFmpeg. >> * >> @@ -1039,7 +1040,8 @@ static int hls_window(AVFormatContext *s, int last) >> >> for (en = hls->segments; en; en = en->next) { >> if (target_duration <= en->duration) >> - target_duration = get_int_from_double(en->duration); >> + target_duration = (hls->flags & HLS_ROUND_DURATIONS) ? >> + lrint(en->duration) : get_int_from_double(en->duration); >> } >> >> hls->discontinuity_set = 0; >> -- >> 1.9.1 >> >> _______________________________________________ >> ffmpeg-devel mailing list >> ffmpeg-devel@ffmpeg.org >> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > > LGTM Sorry, my mistake. the target_duration must large or equal to the en->duration, can not small than en->duration from HLS version 3.
2017-09-01 18:40 GMT+08:00 Steven Liu <lingjiujianke@gmail.com>: > 2017-09-01 18:32 GMT+08:00 Steven Liu <lingjiujianke@gmail.com>: >> 2017-09-01 18:15 GMT+08:00 <kjeyapal@akamai.com>: >>> From: Karthick J <kjeyapal@akamai.com> >>> >>> Signed-off-by: Karthick J <kjeyapal@akamai.com> >>> --- >>> libavformat/hlsenc.c | 4 +++- >>> 1 file changed, 3 insertions(+), 1 deletion(-) >>> >>> diff --git a/libavformat/hlsenc.c b/libavformat/hlsenc.c >>> index 4a908863..dd36fde 100644 >>> --- a/libavformat/hlsenc.c >>> +++ b/libavformat/hlsenc.c >>> @@ -1,6 +1,7 @@ >>> /* >>> * Apple HTTP Live Streaming segmenter >>> * Copyright (c) 2012, Luca Barbato >>> + * Copyright (c) 2017 Akamai Technologies, Inc. >>> * >>> * This file is part of FFmpeg. >>> * >>> @@ -1039,7 +1040,8 @@ static int hls_window(AVFormatContext *s, int last) >>> >>> for (en = hls->segments; en; en = en->next) { >>> if (target_duration <= en->duration) >>> - target_duration = get_int_from_double(en->duration); >>> + target_duration = (hls->flags & HLS_ROUND_DURATIONS) ? >>> + lrint(en->duration) : get_int_from_double(en->duration); >>> } >>> >>> hls->discontinuity_set = 0; >>> -- >>> 1.9.1 >>> >>> _______________________________________________ >>> ffmpeg-devel mailing list >>> ffmpeg-devel@ffmpeg.org >>> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel >> >> >> LGTM > > Sorry, my mistake. > > the target_duration must large or equal to the en->duration, can not > small than en->duration from HLS version 3. https://tools.ietf.org/html/draft-pantos-http-live-streaming-23#section-4.3.3.1 4.3.3.1. EXT-X-TARGETDURATION The EXT-X-TARGETDURATION tag specifies the maximum Media Segment duration. The EXTINF duration of each Media Segment in the Playlist file, when rounded to the nearest integer, MUST be less than or equal to the target duration; longer segments can trigger playback stalls or other errors. It applies to the entire Playlist file. Its format is: #EXT-X-TARGETDURATION:<s> where s is a decimal-integer indicating the target duration in seconds. The EXT-X-TARGETDURATION tag is REQUIRED.
>2017-09-01 18:32 GMT+08:00 Steven Liu <lingjiujianke@gmail.com<mailto:lingjiujianke@gmail.com>>: >Sorry, my mistake. > >the target_duration must large or equal to the en->duration, can not >small than en->duration from HLS version 3. When “round_durations” is enabled “en->duration” also gets rounded(lrint). So in the playlist, the target duration will never be lesser than any of the segment durations. It will still be in compliance to the HLS spec. Otherwise we see an output where all segment durations are ‘n’ seconds, and the target duration is ‘n+1’ seconds when round_durations is enabled. This behavior causes some players to misbehave and add more latency unnecessarily. Thanks, Karthick
2017-09-01 18:54 GMT+08:00 Jeyapal, Karthick <kjeyapal@akamai.com>: >>2017-09-01 18:32 GMT+08:00 Steven Liu <lingjiujianke@gmail.com<mailto:lingjiujianke@gmail.com>>: >>Sorry, my mistake. >> >>the target_duration must large or equal to the en->duration, can not >>small than en->duration from HLS version 3. > > When “round_durations” is enabled “en->duration” also gets rounded(lrint). So in the playlist, the target duration will never be lesser than any of the segment durations. It will still be in compliance to the HLS spec. > Otherwise we see an output where all segment durations are ‘n’ seconds, and the target duration is ‘n+1’ seconds when round_durations is enabled. This behavior causes some players to misbehave and add more latency unnecessarily. The hls_time is set to the segments duration, not target duration, and the hls_time just a reference value, so the target duration is +1. anyway, this patch can not catch the point. The spec clearly says it has to be greater or equal. So rounding it up is perfectly legal and might even safeguard against some poor player implementations. > > Thanks, > Karthick > > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>The hls_time is set to the segments duration, not target duration, and >the hls_time just a reference value, so the target duration is +1. >anyway, this patch can not catch the point. >The spec clearly says it has to be greater or equal. So rounding it up >is perfectly legal and might even safeguard against some poor player >implementations. I think there is a misunderstanding here. Sorry if I was not clear. I mean to say, even with this patch EXT-X-TARGETDURATION will always be greater than or equal to the EXTINF duration of all the segments in the playlist. Hence it is compliant with the HLS spec. Regards, Karthick
Hi Steven, I am restarting this discussion, just to conclude this thread one way or the other. Maybe I am not thinking correctly. Here is detailed thinking, on why I think this patch will not violate HLS spec. > if (target_duration <= en->duration) > target_duration = (hls->flags & HLS_ROUND_DURATIONS) ? > lrint(en->duration) : get_int_from_double(en->duration); As per the above code, target duration will be maximum of all lrint(en->duration) when HLS_ROUND_DURATIONS is set. > if (hls->flags & HLS_ROUND_DURATIONS) > avio_printf(out, "#EXTINF:%ld,\n", lrint(en->duration)); > else > avio_printf(out, "#EXTINF:%f,\n", en->duration); As per this code #EXTINF duration will be lrint(en->duration) when HLS_ROUND_DURATIONS is set. >https://tools.ietf.org/html/draft-pantos-http-live-streaming-23#section-4.3.3.1 > >4.3.3.1. EXT-X-TARGETDURATION > > The EXT-X-TARGETDURATION tag specifies the maximum Media Segment > duration. The EXTINF duration of each Media Segment in the Playlist > file, when rounded to the nearest integer, MUST be less than or equal > to the target duration; longer segments can trigger playback stalls > or other errors. It applies to the entire Playlist file. Its format > is: > > #EXT-X-TARGETDURATION:<s> > > where s is a decimal-integer indicating the target duration in > seconds. The EXT-X-TARGETDURATION tag is REQUIRED. As rightly pointed by you, spec says “The EXTINF duration of each Media Segment in the Playlist file, when rounded to the nearest integer, MUST be less than or equal to the target duration”. With this patch this is always true, and hence there is no violation of spec. Maybe I misunderstood and missing out something very obvious. Could you let me know the situation in which this code will generate target duration lesser than the EXTINF duration? Thanks for your help. Regards, Karthick
2017-09-06 14:06 GMT+08:00 Jeyapal, Karthick <kjeyapal@akamai.com>: > Hi Steven, > > I am restarting this discussion, just to conclude this thread one way or the other. Maybe I am not thinking correctly. Here is detailed thinking, on why I think this patch will not violate HLS spec. > >> if (target_duration <= en->duration) >> target_duration = (hls->flags & HLS_ROUND_DURATIONS) ? >> lrint(en->duration) : get_int_from_double(en->duration); > > As per the above code, target duration will be maximum of all lrint(en->duration) when HLS_ROUND_DURATIONS is set. > >> if (hls->flags & HLS_ROUND_DURATIONS) >> avio_printf(out, "#EXTINF:%ld,\n", lrint(en->duration)); >> else >> avio_printf(out, "#EXTINF:%f,\n", en->duration); > > As per this code #EXTINF duration will be lrint(en->duration) when HLS_ROUND_DURATIONS is set. > >>https://tools.ietf.org/html/draft-pantos-http-live-streaming-23#section-4.3.3.1 >> >>4.3.3.1. EXT-X-TARGETDURATION >> >> The EXT-X-TARGETDURATION tag specifies the maximum Media Segment >> duration. The EXTINF duration of each Media Segment in the Playlist >> file, when rounded to the nearest integer, MUST be less than or equal >> to the target duration; longer segments can trigger playback stalls >> or other errors. It applies to the entire Playlist file. Its format >> is: >> >> #EXT-X-TARGETDURATION:<s> >> >> where s is a decimal-integer indicating the target duration in >> seconds. The EXT-X-TARGETDURATION tag is REQUIRED. > > As rightly pointed by you, spec says “The EXTINF duration of each Media Segment in the Playlist file, when rounded to the nearest integer, MUST be less than or equal to the target duration”. With this patch this is always true, and hence there is no violation of spec. > > Maybe I misunderstood and missing out something very obvious. Could you let me know the situation in which this code will generate target duration lesser than the EXTINF duration? Thanks for your help. segment1.ts duration = 4.880000 ROUND is 5, not 4, is it? if its duration = 4.040000 ROUND is 4, the real segment1.ts duration is 4.040000, the safe one is TARGETDURATION = 5, the get_int_from_ double API can make the truely, yes, your patch can make 4.880000 to 5, and make 4.040000 to 4, all solution's TARGETDURATION is INT, but just make safe, i know the lrint API, so that is why have not use it, not i don't know lrint. > > Regards, > Karthick > > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>On 9/6/17, 12:37 PM, "Steven Liu" <lingjiujianke@gmail.com<mailto:lingjiujianke@gmail.com>> wrote: >segment1.ts duration = 4.880000 >ROUND is 5, not 4, is it? >if its duration = 4.040000 >ROUND is 4, the real segment1.ts duration is 4.040000, > >the safe one is TARGETDURATION = 5, the get_int_from_ double API can >make the truely, Yes, when duration of segment1.ts is 4.040000, EXTINF duration is 4 when HLS_ROUND_DURATIONS is set. After this patch EXT-X-TARGETDURATION will also be 4(instead of 5), which will be equal to the EXTINF duration and hence there is no violation of spec. And when another segment has duration of 4.880000 both the EXTINF and duration and EXT-X-TARGETDURATION will be 5, still in line with the HLS specification. What is reason to be safe when we know deterministically that EXTINF duration will always be lesser than or equal to EXT-X-TARGETDURATION? >yes, your patch can make 4.880000 to 5, and make 4.040000 to 4, all >solution's TARGETDURATION is INT, but just make safe, i know the lrint >API, so that is why have not use it, not i don't know lrint. lrint was already being used for EXTINF duration when HLS_ROUND_DURATIONS is set. And this patch kicks in only when HLS_ROUND_DURATIONS is set. It still uses the get_int_from_double API otherwise. This patch still doesn’t affect the default path when EXTINF is not rounded. Thanks and Regards, Karthick
2017-09-06 16:46 GMT+08:00 Jeyapal, Karthick <kjeyapal@akamai.com>: >>On 9/6/17, 12:37 PM, "Steven Liu" <lingjiujianke@gmail.com<mailto:lingjiujianke@gmail.com>> wrote: >>segment1.ts duration = 4.880000 >>ROUND is 5, not 4, is it? >>if its duration = 4.040000 >>ROUND is 4, the real segment1.ts duration is 4.040000, >> >>the safe one is TARGETDURATION = 5, the get_int_from_ double API can >>make the truely, > > Yes, when duration of segment1.ts is 4.040000, EXTINF duration is 4 when HLS_ROUND_DURATIONS is set. After this patch EXT-X-TARGETDURATION will also be 4(instead of 5), which will be equal to the EXTINF duration and hence there is no violation of spec. And when another segment has duration of 4.880000 both the EXTINF and duration and EXT-X-TARGETDURATION will be 5, still in line with the HLS specification. What is reason to be safe when we know deterministically that EXTINF duration will always be lesser than or equal to EXT-X-TARGETDURATION? > >>yes, your patch can make 4.880000 to 5, and make 4.040000 to 4, all >>solution's TARGETDURATION is INT, but just make safe, i know the lrint >>API, so that is why have not use it, not i don't know lrint. > > lrint was already being used for EXTINF duration when HLS_ROUND_DURATIONS is set. And this patch kicks in only when HLS_ROUND_DURATIONS is set. It still uses the get_int_from_double API otherwise. This patch still doesn’t affect the default path when EXTINF is not rounded. when 4.040000, the after ROUND EXTINF value is lesser than segment duration. > > Thanks and Regards, > Karthick > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>On 9/6/17, 3:43 PM, "Steven Liu" <lingjiujianke@gmail.com<mailto:lingjiujianke@gmail.com>> wrote: > >>2017-09-06 16:46 GMT+08:00 Jeyapal, Karthick <kjeyapal@akamai.com<mailto:kjeyapal@akamai.com>>: >>>On 9/6/17, 12:37 PM, "Steven Liu" <lingjiujianke at gmail.com<http://ffmpeg.org/mailman/listinfo/ffmpeg-devel><mailto:lingjiujianke at gmail.com<http://ffmpeg.org/mailman/listinfo/ffmpeg-devel>>> wrote: >>>segment1.ts duration = 4.880000 >>>ROUND is 5, not 4, is it? >>>if its duration = 4.040000 >>>ROUND is 4, the real segment1.ts duration is 4.040000, >>> >>>the safe one is TARGETDURATION = 5, the get_int_from_ double API can >>>make the truely, >> >> Yes, when duration of segment1.ts is 4.040000, EXTINF duration is 4 when HLS_ROUND_DURATIONS is set. After this patch EXT-X-TARGETDURATION will also be 4(instead of 5), which will be equal to the EXTINF duration and hence there is no violation of spec. And when another segment has duration of 4.880000 both the EXTINF and duration and EXT-X-TARGETDURATION will be 5, still in line with the HLS specification. What is reason to be safe when we know deterministically that EXTINF duration will always be lesser than or equal to EXT-X-TARGETDURATION? >> >>>yes, your patch can make 4.880000 to 5, and make 4.040000 to 4, all >>>solution's TARGETDURATION is INT, but just make safe, i know the lrint >>>API, so that is why have not use it, not i don't know lrint. >> >> lrint was already being used for EXTINF duration when HLS_ROUND_DURATIONS is set. And this patch kicks in only when HLS_ROUND_DURATIONS is set. It still uses the get_int_from_double API otherwise. This patch still doesn’t affect the default path when EXTINF is not rounded. >when 4.040000, the after ROUND EXTINF value is lesser than segment duration. Thanks for the reply. Yes, that is true. But as per the HLS spec in https://tools.ietf.org/html/draft-pantos-http-live-streaming-23#page-21 specifies “The EXTINF duration of each Media Segment in the Playlist file, when rounded to the nearest integer, MUST be less than or equal to the target duration;” for target duration. Please note that phrase “when rounded to the nearest integer” in that line. HLS specification allows you to specify target duration as maximum of segment duration “after rounding to the nearest integer”. Just to add a little more justification, we have been running Apple’s mediastreamvalidator in our daily regression tests with this patch, and it has never warned about the target duration. While I agree Apple’s mediastreamvalidator is not a conformance tool to check all kinds of HLS errors, but they definitely have quite a few checks in place that flags several duration related errors. >> >> Thanks and Regards, >> Karthick >> _______________________________________________ >> ffmpeg-devel mailing list >> ffmpeg-devel at ffmpeg.org<http://ffmpeg.org/mailman/listinfo/ffmpeg-devel> >> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
2017-09-06 18:31 GMT+08:00 Jeyapal, Karthick <kjeyapal@akamai.com>: > > >>On 9/6/17, 3:43 PM, "Steven Liu" <lingjiujianke@gmail.com<mailto:lingjiujianke@gmail.com>> wrote: >> >>>2017-09-06 16:46 GMT+08:00 Jeyapal, Karthick <kjeyapal@akamai.com<mailto:kjeyapal@akamai.com>>: >>>>On 9/6/17, 12:37 PM, "Steven Liu" <lingjiujianke at gmail.com<http://ffmpeg.org/mailman/listinfo/ffmpeg-devel><mailto:lingjiujianke at gmail.com<http://ffmpeg.org/mailman/listinfo/ffmpeg-devel>>> wrote: >>>>segment1.ts duration = 4.880000 >>>>ROUND is 5, not 4, is it? >>>>if its duration = 4.040000 >>>>ROUND is 4, the real segment1.ts duration is 4.040000, >>>> >>>>the safe one is TARGETDURATION = 5, the get_int_from_ double API can >>>>make the truely, >>> >>> Yes, when duration of segment1.ts is 4.040000, EXTINF duration is 4 when HLS_ROUND_DURATIONS is set. After this patch EXT-X-TARGETDURATION will also be 4(instead of 5), which will be equal to the EXTINF duration and hence there is no violation of spec. And when another segment has duration of 4.880000 both the EXTINF and duration and EXT-X-TARGETDURATION will be 5, still in line with the HLS specification. What is reason to be safe when we know deterministically that EXTINF duration will always be lesser than or equal to EXT-X-TARGETDURATION? >>> >>>>yes, your patch can make 4.880000 to 5, and make 4.040000 to 4, all >>>>solution's TARGETDURATION is INT, but just make safe, i know the lrint >>>>API, so that is why have not use it, not i don't know lrint. >>> >>> lrint was already being used for EXTINF duration when HLS_ROUND_DURATIONS is set. And this patch kicks in only when HLS_ROUND_DURATIONS is set. It still uses the get_int_from_double API otherwise. This patch still doesn’t affect the default path when EXTINF is not rounded. > >>when 4.040000, the after ROUND EXTINF value is lesser than segment duration. > > Thanks for the reply. Yes, that is true. But as per the HLS spec in https://tools.ietf.org/html/draft-pantos-http-live-streaming-23#page-21 specifies “The EXTINF duration of each Media Segment in the Playlist file, when rounded to the nearest integer, MUST be less than or equal to the target duration;” for target duration. Please note that phrase “when rounded to the nearest integer” in that line. HLS specification allows you to specify target duration as maximum of segment duration “after rounding to the nearest integer”. > > Just to add a little more justification, we have been running Apple’s mediastreamvalidator in our daily regression tests with this patch, and it has never warned about the target duration. While I agree Apple’s mediastreamvalidator is not a conformance tool to check all kinds of HLS errors, but they definitely have quite a few checks in place that flags several duration related errors. That is not perfect solution, because that just " that phrase 'when rounded to the nearest integer' in that line", not write in the spec, it not said the EXTINF value is "when rounded to the nearest integer", and now it support have not problem, or can you reproduce problem when not apply this modify? > >>> >>> Thanks and Regards, >>> Karthick >>> _______________________________________________ >>> ffmpeg-devel mailing list >>> ffmpeg-devel at ffmpeg.org<http://ffmpeg.org/mailman/listinfo/ffmpeg-devel> >>> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
diff --git a/libavformat/hlsenc.c b/libavformat/hlsenc.c index 4a908863..dd36fde 100644 --- a/libavformat/hlsenc.c +++ b/libavformat/hlsenc.c @@ -1,6 +1,7 @@ /* * Apple HTTP Live Streaming segmenter * Copyright (c) 2012, Luca Barbato + * Copyright (c) 2017 Akamai Technologies, Inc. * * This file is part of FFmpeg. * @@ -1039,7 +1040,8 @@ static int hls_window(AVFormatContext *s, int last) for (en = hls->segments; en; en = en->next) { if (target_duration <= en->duration) - target_duration = get_int_from_double(en->duration); + target_duration = (hls->flags & HLS_ROUND_DURATIONS) ? + lrint(en->duration) : get_int_from_double(en->duration); } hls->discontinuity_set = 0;