[FFmpeg-devel] avformat/hlsenc: improve compute target_duration way

Submitted by Steven Liu on Dec. 22, 2017, 11:29 p.m.

Details

Message ID 20171222232956.60998-1-lq@chinaffmpeg.org
State New
Headers show

Commit Message

Steven Liu Dec. 22, 2017, 11:29 p.m.
just use lrint(lrint(duration * 10.0) / 10.0)
fix ticket: 6915

Suggested-by: beloko
Signed-off-by: Steven Liu <lq@chinaffmpeg.org>
---
 libavformat/dashenc.c | 2 +-
 libavformat/hlsenc.c  | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

Comments

kjeyapal@akamai.com Dec. 23, 2017, 10:38 a.m.
On 12/23/17 4:59 AM, Steven Liu wrote:
> just use lrint(lrint(duration * 10.0) / 10.0)

> fix ticket: 6915

>

> Suggested-by: beloko

> Signed-off-by: Steven Liu <lq@chinaffmpeg.org>

> ---

>  libavformat/dashenc.c | 2 +-

>  libavformat/hlsenc.c  | 2 +-

>  2 files changed, 2 insertions(+), 2 deletions(-)

>

> diff --git a/libavformat/dashenc.c b/libavformat/dashenc.c

> index 5368a2334c..102558d17e 100644

> --- a/libavformat/dashenc.c

> +++ b/libavformat/dashenc.c

> @@ -358,7 +358,7 @@ static void output_segment_list(OutputStream *os, AVIOContext *out, DASHContext

>              Segment *seg = os->segments[i];

>              double duration = (double) seg->duration / timescale;

>              if (target_duration <= duration)

> -                target_duration = lrint(duration);

> +                target_duration = lrint(lrint(duration * 10.0) / 10.0);

This will just move the move the corner case from x.5 to x.45(x.46 depending on the double precision errors). Will not solve the original issue.
For example, right now 1.5 is a corner case, as lrint(1.5) = 2, and lrint(1.49) = 1.
After this patch, 1.46 will become the corner case. For example lrint(lrint(1.45 * 10.0) / 10.0) = 1 and lrint(lrint(1.46 * 10.0) / 10.0) = 2
When someone chooses a segment duration of 1.46, this issue will again resurface.

Thanks and regards,
Karthick
>          }

>  

>          ff_hls_write_playlist_header(out_hls, 6, -1, target_duration,

> diff --git a/libavformat/hlsenc.c b/libavformat/hlsenc.c

> index 0095ca4339..bf6f1d9253 100644

> --- a/libavformat/hlsenc.c

> +++ b/libavformat/hlsenc.c

> @@ -1235,7 +1235,7 @@ static int hls_window(AVFormatContext *s, int last, VariantStream *vs)

>  

>      for (en = vs->segments; en; en = en->next) {

>          if (target_duration <= en->duration)

> -            target_duration = lrint(en->duration);

> +            target_duration = lrint(lrint(en->duration * 10.0) / 10.0);

>      }

>  

>      vs->discontinuity_set = 0;
Steven Liu Dec. 23, 2017, 2:32 p.m.
> 在 2017年12月23日,下午6:38,Jeyapal, Karthick <kjeyapal@akamai.com> 写道:
> 
> 
>> On 12/23/17 4:59 AM, Steven Liu wrote:
>> just use lrint(lrint(duration * 10.0) / 10.0)
>> fix ticket: 6915
>> 
>> Suggested-by: beloko
>> Signed-off-by: Steven Liu <lq@chinaffmpeg.org>
>> ---
>> libavformat/dashenc.c | 2 +-
>> libavformat/hlsenc.c  | 2 +-
>> 2 files changed, 2 insertions(+), 2 deletions(-)
>> 
>> diff --git a/libavformat/dashenc.c b/libavformat/dashenc.c
>> index 5368a2334c..102558d17e 100644
>> --- a/libavformat/dashenc.c
>> +++ b/libavformat/dashenc.c
>> @@ -358,7 +358,7 @@ static void output_segment_list(OutputStream *os, AVIOContext *out, DASHContext
>>             Segment *seg = os->segments[i];
>>             double duration = (double) seg->duration / timescale;
>>             if (target_duration <= duration)
>> -                target_duration = lrint(duration);
>> +                target_duration = lrint(lrint(duration * 10.0) / 10.0);
> This will just move the move the corner case from x.5 to x.45(x.46 depending on the double precision errors). Will not solve the original issue.
> For example, right now 1.5 is a corner case, as lrint(1.5) = 2, and lrint(1.49) = 1.
> After this patch, 1.46 will become the corner case. For example lrint(lrint(1.45 * 10.0) / 10.0) = 1 and lrint(lrint(1.46 * 10.0) / 10.0) = 2
> When someone chooses a segment duration of 1.46, this issue will again resurface.
maybe you are right, and I use mediastreamvalidator check again, I focus the error, when the media_0.m3u8 media_1.m3u8 EXT-X-TARGETDURATION different, it no error of the two playlist, I think Roger response is right, maybe our focus is not right. Look at your lrint patch, the test error context is not target duration different error, and I have test use the code before your patch , no target duration error of two playlist. Just fragment duration error to target duration, look it again.
> 
> Thanks and regards,
> Karthick
>>         }
>> 
>>         ff_hls_write_playlist_header(out_hls, 6, -1, target_duration,
>> diff --git a/libavformat/hlsenc.c b/libavformat/hlsenc.c
>> index 0095ca4339..bf6f1d9253 100644
>> --- a/libavformat/hlsenc.c
>> +++ b/libavformat/hlsenc.c
>> @@ -1235,7 +1235,7 @@ static int hls_window(AVFormatContext *s, int last, VariantStream *vs)
>> 
>>     for (en = vs->segments; en; en = en->next) {
>>         if (target_duration <= en->duration)
>> -            target_duration = lrint(en->duration);
>> +            target_duration = lrint(lrint(en->duration * 10.0) / 10.0);
>>     }
>> 
>>     vs->discontinuity_set = 0;
> 
> 
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
kjeyapal@akamai.com Dec. 23, 2017, 2:55 p.m.
On 12/23/17 8:02 PM, Steven Liu wrote:
>

>

>> 在 2017年12月23日,下午6:38,Jeyapal, Karthick <kjeyapal@akamai.com> 写道:

>>

>>

>>> On 12/23/17 4:59 AM, Steven Liu wrote:

>>> just use lrint(lrint(duration * 10.0) / 10.0)

>>> fix ticket: 6915

>>>

>>> Suggested-by: beloko

>>> Signed-off-by: Steven Liu <lq@chinaffmpeg.org>

>>> ---

>>> libavformat/dashenc.c | 2 +-

>>> libavformat/hlsenc.c  | 2 +-

>>> 2 files changed, 2 insertions(+), 2 deletions(-)

>>>

>>> diff --git a/libavformat/dashenc.c b/libavformat/dashenc.c

>>> index 5368a2334c..102558d17e 100644

>>> --- a/libavformat/dashenc.c

>>> +++ b/libavformat/dashenc.c

>>> @@ -358,7 +358,7 @@ static void output_segment_list(OutputStream *os, AVIOContext *out, DASHContext

>>>             Segment *seg = os->segments[i];

>>>             double duration = (double) seg->duration / timescale;

>>>             if (target_duration <= duration)

>>> -                target_duration = lrint(duration);

>>> +                target_duration = lrint(lrint(duration * 10.0) / 10.0);

>> This will just move the move the corner case from x.5 to x.45(x.46 depending on the double precision errors). Will not solve the original issue.

>> For example, right now 1.5 is a corner case, as lrint(1.5) = 2, and lrint(1.49) = 1.

>> After this patch, 1.46 will become the corner case. For example lrint(lrint(1.45 * 10.0) / 10.0) = 1 and lrint(lrint(1.46 * 10.0) / 10.0) = 2

>> When someone chooses a segment duration of 1.46, this issue will again resurface.

> maybe you are right, and I use mediastreamvalidator check again, I focus the error, when the media_0.m3u8 media_1.m3u8 EXT-X-TARGETDURATION different, it no error of the two playlist, I think Roger response is right, maybe our focus is not right. Look at your lrint patch, the test error context is not target duration different error, and I have test use the code before your patch , no target duration error of two playlist. Just fragment duration error to target duration, look it again.

As I said earlier, we need not focus on segment size being 1.5 or 1.45, as it is not a valid use-case. Even hlsenc does support only integral number of seconds as a configuration option(hls_time).
According to me, right now there is no issue in ffmpeg w.r.t target duration as integer segment sizes are handled correctly. We can come back to this problem, when it really becomes a use-case and somebody reports this as an issue.

As you might know, the older spec allowed only integer EXTINF durations as well. Later they added floating point duration as video and audio segments cannot always be cut at exactly at integral multiple of seconds, which was leading to accumulation of errors causing unexpected player behavior. My guess is floating point duration was added to avoid error accumulation, and not for supporting fractional segment sizes as a feature. As far I am aware, no popular video streaming service is using HLS with fractional segment durations like 1.5, 2.5 etc., All of them are using integral multiple of seconds as segment sizes. 
>>

>> Thanks and regards,

>> Karthick

>>>         }

>>>

>>>         ff_hls_write_playlist_header(out_hls, 6, -1, target_duration,

>>> diff --git a/libavformat/hlsenc.c b/libavformat/hlsenc.c

>>> index 0095ca4339..bf6f1d9253 100644

>>> --- a/libavformat/hlsenc.c

>>> +++ b/libavformat/hlsenc.c

>>> @@ -1235,7 +1235,7 @@ static int hls_window(AVFormatContext *s, int last, VariantStream *vs)

>>>

>>>     for (en = vs->segments; en; en = en->next) {

>>>         if (target_duration <= en->duration)

>>> -            target_duration = lrint(en->duration);

>>> +            target_duration = lrint(lrint(en->duration * 10.0) / 10.0);

>>>     }

>>>

>>>     vs->discontinuity_set = 0;

>>

>>

>> _______________________________________________

>> ffmpeg-devel mailing list

>> ffmpeg-devel@ffmpeg.org

>> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

>

>

>

> _______________________________________________

> ffmpeg-devel mailing list

> ffmpeg-devel@ffmpeg.org

> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Patch hide | download patch | download mbox

diff --git a/libavformat/dashenc.c b/libavformat/dashenc.c
index 5368a2334c..102558d17e 100644
--- a/libavformat/dashenc.c
+++ b/libavformat/dashenc.c
@@ -358,7 +358,7 @@  static void output_segment_list(OutputStream *os, AVIOContext *out, DASHContext
             Segment *seg = os->segments[i];
             double duration = (double) seg->duration / timescale;
             if (target_duration <= duration)
-                target_duration = lrint(duration);
+                target_duration = lrint(lrint(duration * 10.0) / 10.0);
         }
 
         ff_hls_write_playlist_header(out_hls, 6, -1, target_duration,
diff --git a/libavformat/hlsenc.c b/libavformat/hlsenc.c
index 0095ca4339..bf6f1d9253 100644
--- a/libavformat/hlsenc.c
+++ b/libavformat/hlsenc.c
@@ -1235,7 +1235,7 @@  static int hls_window(AVFormatContext *s, int last, VariantStream *vs)
 
     for (en = vs->segments; en; en = en->next) {
         if (target_duration <= en->duration)
-            target_duration = lrint(en->duration);
+            target_duration = lrint(lrint(en->duration * 10.0) / 10.0);
     }
 
     vs->discontinuity_set = 0;