[FFmpeg-devel] [PATCH v1] avformat/hlsenc: fix hls_time not respected

[FFmpeg-devel] [PATCH v1] avformat/hlsenc: fix hls_time not respected

[Ingo Oppermann]

This fixes the criterion when to split the segments based on the elapsed time
for the current segment instead of using the theoretical elapsed time since
start based on hls_time and the number of written segments.

hls_time is used to define the minimum length of a segment, however this is
not respected in all cases when a stream has variable GOP sizes.

Imagine a stream starts with a key frame every 10 seconds for e.g. 40 seconds.
After that, key frames will come every second. This will result in segments
that are first 10 seconds, then 1 second for some time and later 2 seconds as
expected.

How to reproduce:
ffmpeg -t 40 -f lavfi -i testsrc2=s=1280x720:r=25 -codec:v libx264 -g 250 part1.mp4
ffmpeg -t 40 -f lavfi -i testsrc2=s=1280x720:r=25 -codec:v libx264 -g 25 part2.mp4
echo "file part1.mp4\nfile part2.mp4" > list.txt
ffmpeg -f concat -i list.txt -codec copy part.mp4
ffmpeg -i part.mp4 -codec copy -f hls -hls_time 2 -hls_list_size 0 -hls_segment_filename 'part_%d.ts' part.m3u8
cat part.m3u8

Signed-off-by: Ingo Oppermann <ingo@datarhei.com>
---
 libavformat/hlsenc.c | 12 +++---------
 1 file changed, 3 insertions(+), 9 deletions(-)

diff --git a/libavformat/hlsenc.c b/libavformat/hlsenc.c
index 6148685f40..2c5d60500d 100644
--- a/libavformat/hlsenc.c
+++ b/libavformat/hlsenc.c
@@ -2474,7 +2474,6 @@ static int hls_write_packet(AVFormatContext *s, AVPacket *pkt)
     HLSContext *hls = s->priv_data;
     AVFormatContext *oc = NULL;
     AVStream *st = s->streams[pkt->stream_index];
-    int64_t end_pts = 0;
     int is_ref_pkt = 1;
     int ret = 0, can_split = 1, i, j;
     int stream_index = 0;
@@ -2512,14 +2511,9 @@ static int hls_write_packet(AVFormatContext *s, AVPacket *pkt)
         return AVERROR(ENOMEM);
     }
 
-    end_pts = hls->recording_time * vs->number;
-
     if (vs->sequence - vs->nb_entries > hls->start_sequence && hls->init_time > 0) {
-        /* reset end_pts, hls->recording_time at end of the init hls list */
-        int64_t init_list_dur = hls->init_time * vs->nb_entries;
-        int64_t after_init_list_dur = (vs->sequence - hls->start_sequence - vs->nb_entries) * hls->time;
+        /* reset hls->recording_time at end of the init hls list */
         hls->recording_time = hls->time;
-        end_pts = init_list_dur + after_init_list_dur ;
     }
 
     if (vs->start_pts == AV_NOPTS_VALUE) {
@@ -2560,8 +2554,8 @@ static int hls_write_packet(AVFormatContext *s, AVPacket *pkt)
     }
 
     can_split = can_split && (pkt->pts - vs->end_pts > 0);
-    if (vs->packets_written && can_split && av_compare_ts(pkt->pts - vs->start_pts, st->time_base,
-                                                          end_pts, AV_TIME_BASE_Q) >= 0) {
+    if (vs->packets_written && can_split && av_compare_ts(pkt->pts - vs->end_pts, st->time_base,
+                                                          hls->recording_time, AV_TIME_BASE_Q) >= 0) {
         int64_t new_start_pos;
         int byterange_mode = (hls->flags & HLS_SINGLE_FILE) || (hls->max_seg_size > 0);
         double cur_duration;

base-commit: e20ee9f9aec94f8cea1bf4fd8ed3fb096fb205e5
-- 
2.39.5 (Apple Git-154)

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH v1] avformat/hlsenc: fix hls_time not respected

[Gyan Doshi]

On 2025-01-24 05:29 pm, Ingo Oppermann wrote:
> This fixes the criterion when to split the segments based on the elapsed time
> for the current segment instead of using the theoretical elapsed time since
> start based on hls_time and the number of written segments.
>
> hls_time is used to define the minimum length of a segment, however this is
> not respected in all cases when a stream has variable GOP sizes.
>
> Imagine a stream starts with a key frame every 10 seconds for e.g. 40 seconds.
> After that, key frames will come every second. This will result in segments
> that are first 10 seconds, then 1 second for some time and later 2 seconds as
> expected.

Better to make it flexible like how the segment muxer does it, by having 
an optional minimum segment duration parameter. Then it's upto the user.
See d39b34123d.

Also Patchwork warns about the line length in the commit message. Should 
be <=72

Regards,
Gyan


>
> How to reproduce:
> ffmpeg -t 40 -f lavfi -i testsrc2=s=1280x720:r=25 -codec:v libx264 -g 250 part1.mp4
> ffmpeg -t 40 -f lavfi -i testsrc2=s=1280x720:r=25 -codec:v libx264 -g 25 part2.mp4
> echo "file part1.mp4\nfile part2.mp4" > list.txt
> ffmpeg -f concat -i list.txt -codec copy part.mp4
> ffmpeg -i part.mp4 -codec copy -f hls -hls_time 2 -hls_list_size 0 -hls_segment_filename 'part_%d.ts' part.m3u8
> cat part.m3u8
>
> Signed-off-by: Ingo Oppermann <ingo@datarhei.com>
> ---
>   libavformat/hlsenc.c | 12 +++---------
>   1 file changed, 3 insertions(+), 9 deletions(-)
>
> diff --git a/libavformat/hlsenc.c b/libavformat/hlsenc.c
> index 6148685f40..2c5d60500d 100644
> --- a/libavformat/hlsenc.c
> +++ b/libavformat/hlsenc.c
> @@ -2474,7 +2474,6 @@ static int hls_write_packet(AVFormatContext *s, AVPacket *pkt)
>       HLSContext *hls = s->priv_data;
>       AVFormatContext *oc = NULL;
>       AVStream *st = s->streams[pkt->stream_index];
> -    int64_t end_pts = 0;
>       int is_ref_pkt = 1;
>       int ret = 0, can_split = 1, i, j;
>       int stream_index = 0;
> @@ -2512,14 +2511,9 @@ static int hls_write_packet(AVFormatContext *s, AVPacket *pkt)
>           return AVERROR(ENOMEM);
>       }
>   
> -    end_pts = hls->recording_time * vs->number;
> -
>       if (vs->sequence - vs->nb_entries > hls->start_sequence && hls->init_time > 0) {
> -        /* reset end_pts, hls->recording_time at end of the init hls list */
> -        int64_t init_list_dur = hls->init_time * vs->nb_entries;
> -        int64_t after_init_list_dur = (vs->sequence - hls->start_sequence - vs->nb_entries) * hls->time;
> +        /* reset hls->recording_time at end of the init hls list */
>           hls->recording_time = hls->time;
> -        end_pts = init_list_dur + after_init_list_dur ;
>       }
>   
>       if (vs->start_pts == AV_NOPTS_VALUE) {
> @@ -2560,8 +2554,8 @@ static int hls_write_packet(AVFormatContext *s, AVPacket *pkt)
>       }
>   
>       can_split = can_split && (pkt->pts - vs->end_pts > 0);
> -    if (vs->packets_written && can_split && av_compare_ts(pkt->pts - vs->start_pts, st->time_base,
> -                                                          end_pts, AV_TIME_BASE_Q) >= 0) {
> +    if (vs->packets_written && can_split && av_compare_ts(pkt->pts - vs->end_pts, st->time_base,
> +                                                          hls->recording_time, AV_TIME_BASE_Q) >= 0) {
>           int64_t new_start_pos;
>           int byterange_mode = (hls->flags & HLS_SINGLE_FILE) || (hls->max_seg_size > 0);
>           double cur_duration;
>
> base-commit: e20ee9f9aec94f8cea1bf4fd8ed3fb096fb205e5

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH v1] avformat/hlsenc: fix hls_time not respected

[Ingo Oppermann]

> On 24 Jan 2025, at 16:30, Gyan Doshi <ffmpeg@gyani.pro> wrote:
> 
> 
> 
> On 2025-01-24 05:29 pm, Ingo Oppermann wrote:
>> This fixes the criterion when to split the segments based on the elapsed time
>> for the current segment instead of using the theoretical elapsed time since
>> start based on hls_time and the number of written segments.
>> 
>> hls_time is used to define the minimum length of a segment, however this is
>> not respected in all cases when a stream has variable GOP sizes.
>> 
>> Imagine a stream starts with a key frame every 10 seconds for e.g. 40 seconds.
>> After that, key frames will come every second. This will result in segments
>> that are first 10 seconds, then 1 second for some time and later 2 seconds as
>> expected.
> 
> Better to make it flexible like how the segment muxer does it, by having an optional minimum segment duration parameter. Then it's upto the user.
> See d39b34123d.

According to the documentation "hls_time" is already supposed to be the minimum duration of a segment: "Segment will be cut on the next key frame after this time has passed." (https://ffmpeg.org/ffmpeg-formats.html#Options-26). It does this under normal circumstances, e.g.

ffmpeg -t 30 -f lavfi -i testsrc2 -codec:v libx264 -g 25 -f hls -hls_time 2 -hls_list_size 0 split.m3u8

Every segment is 2 seconds even though the GOP size 1 second.


> 
> Also Patchwork warns about the line length in the commit message. Should be <=72

Will fix this in the following version of the patch.

> 
> Regards,
> Gyan
> 
> 
>> 
>> How to reproduce:
>> ffmpeg -t 40 -f lavfi -i testsrc2=s=1280x720:r=25 -codec:v libx264 -g 250 part1.mp4
>> ffmpeg -t 40 -f lavfi -i testsrc2=s=1280x720:r=25 -codec:v libx264 -g 25 part2.mp4
>> echo "file part1.mp4\nfile part2.mp4" > list.txt
>> ffmpeg -f concat -i list.txt -codec copy part.mp4
>> ffmpeg -i part.mp4 -codec copy -f hls -hls_time 2 -hls_list_size 0 -hls_segment_filename 'part_%d.ts' part.m3u8
>> cat part.m3u8
>> 
>> Signed-off-by: Ingo Oppermann <ingo@datarhei.com>
>> ---
>>  libavformat/hlsenc.c | 12 +++---------
>>  1 file changed, 3 insertions(+), 9 deletions(-)
>> 
>> diff --git a/libavformat/hlsenc.c b/libavformat/hlsenc.c
>> index 6148685f40..2c5d60500d 100644
>> --- a/libavformat/hlsenc.c
>> +++ b/libavformat/hlsenc.c
>> @@ -2474,7 +2474,6 @@ static int hls_write_packet(AVFormatContext *s, AVPacket *pkt)
>>      HLSContext *hls = s->priv_data;
>>      AVFormatContext *oc = NULL;
>>      AVStream *st = s->streams[pkt->stream_index];
>> -    int64_t end_pts = 0;
>>      int is_ref_pkt = 1;
>>      int ret = 0, can_split = 1, i, j;
>>      int stream_index = 0;
>> @@ -2512,14 +2511,9 @@ static int hls_write_packet(AVFormatContext *s, AVPacket *pkt)
>>          return AVERROR(ENOMEM);
>>      }
>>  -    end_pts = hls->recording_time * vs->number;
>> -
>>      if (vs->sequence - vs->nb_entries > hls->start_sequence && hls->init_time > 0) {
>> -        /* reset end_pts, hls->recording_time at end of the init hls list */
>> -        int64_t init_list_dur = hls->init_time * vs->nb_entries;
>> -        int64_t after_init_list_dur = (vs->sequence - hls->start_sequence - vs->nb_entries) * hls->time;
>> +        /* reset hls->recording_time at end of the init hls list */
>>          hls->recording_time = hls->time;
>> -        end_pts = init_list_dur + after_init_list_dur ;
>>      }
>>        if (vs->start_pts == AV_NOPTS_VALUE) {
>> @@ -2560,8 +2554,8 @@ static int hls_write_packet(AVFormatContext *s, AVPacket *pkt)
>>      }
>>        can_split = can_split && (pkt->pts - vs->end_pts > 0);
>> -    if (vs->packets_written && can_split && av_compare_ts(pkt->pts - vs->start_pts, st->time_base,
>> -                                                          end_pts, AV_TIME_BASE_Q) >= 0) {
>> +    if (vs->packets_written && can_split && av_compare_ts(pkt->pts - vs->end_pts, st->time_base,
>> +                                                          hls->recording_time, AV_TIME_BASE_Q) >= 0) {
>>          int64_t new_start_pos;
>>          int byterange_mode = (hls->flags & HLS_SINGLE_FILE) || (hls->max_seg_size > 0);
>>          double cur_duration;
>> 
>> base-commit: e20ee9f9aec94f8cea1bf4fd8ed3fb096fb205e5
> 
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".


_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH v1] avformat/hlsenc: fix hls_time not respected

[Gyan Doshi]

On 2025-01-27 01:56 pm, Ingo Oppermann wrote:
>> On 24 Jan 2025, at 16:30, Gyan Doshi <ffmpeg@gyani.pro> wrote:
>>
>>
>>
>> On 2025-01-24 05:29 pm, Ingo Oppermann wrote:
>>> This fixes the criterion when to split the segments based on the elapsed time
>>> for the current segment instead of using the theoretical elapsed time since
>>> start based on hls_time and the number of written segments.
>>>
>>> hls_time is used to define the minimum length of a segment, however this is
>>> not respected in all cases when a stream has variable GOP sizes.
>>>
>>> Imagine a stream starts with a key frame every 10 seconds for e.g. 40 seconds.
>>> After that, key frames will come every second. This will result in segments
>>> that are first 10 seconds, then 1 second for some time and later 2 seconds as
>>> expected.
>> Better to make it flexible like how the segment muxer does it, by having an optional minimum segment duration parameter. Then it's upto the user.
>> See d39b34123d.
> According to the documentation "hls_time" is already supposed to be the minimum duration of a segment: "Segment will be cut on the next key frame after this time has passed." (https://ffmpeg.org/ffmpeg-formats.html#Options-26).

In practice, the logic used is that segment X,  counting from 0, should 
start at X*hls_time. It's not best practice to change long-standing 
behaviour without a fallback or workaround. Your patch doesn't offer that.
HLS is usually a multiple representation format, so this may adversely 
affect rendition switchability. Adding an option like segment muxer 
won't change the outcome of existing command lines but will still allow 
you the behaviour you're looking for.

Regards,
Gyan

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH v1] avformat/hlsenc: fix hls_time not respected

[Ingo Oppermann]

> On 27 Jan 2025, at 11:41, Gyan Doshi <ffmpeg@gyani.pro> wrote:
> 
> 
> 
> On 2025-01-27 01:56 pm, Ingo Oppermann wrote:
>>> On 24 Jan 2025, at 16:30, Gyan Doshi <ffmpeg@gyani.pro> wrote:
>>> 
>>> 
>>> 
>>> On 2025-01-24 05:29 pm, Ingo Oppermann wrote:
>>>> This fixes the criterion when to split the segments based on the elapsed time
>>>> for the current segment instead of using the theoretical elapsed time since
>>>> start based on hls_time and the number of written segments.
>>>> 
>>>> hls_time is used to define the minimum length of a segment, however this is
>>>> not respected in all cases when a stream has variable GOP sizes.
>>>> 
>>>> Imagine a stream starts with a key frame every 10 seconds for e.g. 40 seconds.
>>>> After that, key frames will come every second. This will result in segments
>>>> that are first 10 seconds, then 1 second for some time and later 2 seconds as
>>>> expected.
>>> Better to make it flexible like how the segment muxer does it, by having an optional minimum segment duration parameter. Then it's upto the user.
>>> See d39b34123d.
>> According to the documentation "hls_time" is already supposed to be the minimum duration of a segment: "Segment will be cut on the next key frame after this time has passed." (https://ffmpeg.org/ffmpeg-formats.html#Options-26).
> 
> In practice, the logic used is that segment X,  counting from 0, should start at X*hls_time. It's not best practice to change long-standing behaviour without a fallback or workaround. Your patch doesn't offer that.
> HLS is usually a multiple representation format, so this may adversely affect rendition switchability. Adding an option like segment muxer won't change the outcome of existing command lines but will still allow you the behaviour you're looking for.

That's true. Changing the current behaviour might break existing processes that are based on this specific behaviour. I'll add a new option then.

> 
> Regards,
> Gyan
> 
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".


_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".