diff mbox series

[FFmpeg-devel] avformat/hlsenc: check the segment duration valid

Message ID 20200817071146.73636-1-lq@chinaffmpeg.org
State New
Headers show
Series [FFmpeg-devel] avformat/hlsenc: check the segment duration valid
Related show

Checks

Context Check Description
andriy/default pending
andriy/make success Make finished
andriy/make_fate success Make fate finished

Commit Message

Steven Liu Aug. 17, 2020, 7:11 a.m. UTC
Output a warning message if the target duration of the segment is negative.
Suggest user increase the hls_time value,
and modify the target duration to one packet duration,
because there maybe have bframe and then split not by keyframe,
and the segment is very very small.

Signed-off-by: Steven Liu <lq@chinaffmpeg.org>
---
 libavformat/hlsenc.c | 6 ++++++
 1 file changed, 6 insertions(+)

Comments

zhilizhao(赵志立) Aug. 16, 2020, 9:06 p.m. UTC | #1
> On Aug 17, 2020, at 4:05 PM, Nicolas George <george@nsup.org> wrote:
> 
> Steven Liu (12020-08-17):
>> Output a warning message if the target duration of the segment is negative.
>> Suggest user increase the hls_time value,
>> and modify the target duration to one packet duration,
>> because there maybe have bframe and then split not by keyframe,
>> and the segment is very very small.
> 
> Fiddling randomly with durations seem wrong. It will probably result in
> invalid seeking. Have you found the exact cause of the issue?
> 

The split_by_time flag which enabled split segments at any position (non-key
frame) is the root cause of the issue. The specification (RFC8216, 6.2.1)
says explicitly:

The server SHOULD attempt to divide the source media at points that
   support effective decode of individual Media Segments, e.g., on
   packet and key frame boundaries.

> Preventing duration from being negative in the first place would be a
> much better fix than adjusting it arbitrarily.
> 
> Regards,
> 
> -- 
>  Nicolas George
> 
> _______________________________________________
> 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".
Nicolas George Aug. 17, 2020, 8:05 a.m. UTC | #2
Steven Liu (12020-08-17):
> Output a warning message if the target duration of the segment is negative.
> Suggest user increase the hls_time value,
> and modify the target duration to one packet duration,
> because there maybe have bframe and then split not by keyframe,
> and the segment is very very small.

Fiddling randomly with durations seem wrong. It will probably result in
invalid seeking. Have you found the exact cause of the issue?

Preventing duration from being negative in the first place would be a
much better fix than adjusting it arbitrarily.

Regards,
Steven Liu Aug. 17, 2020, 9:17 a.m. UTC | #3
zhilizhao <quinkblack@foxmail.com> 于2020年8月17日周一 下午5:06写道:
>
>
>
> > On Aug 17, 2020, at 4:05 PM, Nicolas George <george@nsup.org> wrote:
> >
> > Steven Liu (12020-08-17):
> >> Output a warning message if the target duration of the segment is negative.
> >> Suggest user increase the hls_time value,
> >> and modify the target duration to one packet duration,
> >> because there maybe have bframe and then split not by keyframe,
> >> and the segment is very very small.
> >
> > Fiddling randomly with durations seem wrong. It will probably result in
> > invalid seeking. Have you found the exact cause of the issue?
> >
>
> The split_by_time flag which enabled split segments at any position (non-key
> frame) is the root cause of the issue. The specification (RFC8216, 6.2.1)
> says explicitly:
>
> The server SHOULD attempt to divide the source media at points that
>    support effective decode of individual Media Segments, e.g., on
>    packet and key frame boundaries.
When hls muxer have beed support split_by_time flag add, the 8216 have
not beed release,
and there have some guys order this feature, for example me.
And I saw lots of people are using this flag, so this maybe cannot be
removed now.
So I think give a warning message and modify it to one packet duration is ok.

>
> > Preventing duration from being negative in the first place would be a
> > much better fix than adjusting it arbitrarily.
> >
> > Regards,
> >
> > --
> >  Nicolas George
> >
> > _______________________________________________
> > 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".
Steven Liu Aug. 17, 2020, 9:23 a.m. UTC | #4
Nicolas George <george@nsup.org> 于2020年8月17日周一 下午4:06写道:
>
> Steven Liu (12020-08-17):
> > Output a warning message if the target duration of the segment is negative.
> > Suggest user increase the hls_time value,
> > and modify the target duration to one packet duration,
> > because there maybe have bframe and then split not by keyframe,
> > and the segment is very very small.
>
> Fiddling randomly with durations seem wrong. It will probably result in
> invalid seeking. Have you found the exact cause of the issue?
>
> Preventing duration from being negative in the first place would be a
> much better fix than adjusting it arbitrarily.

As Zhili Zhao said, the problem is the user is using hls_time with too
short duration and want split not by keyframe.
I have been think about how to make the user know it don't support
short than 2 frames and with bframes,
but this is not the better way.
I think give a suggestion and warning message is better.
Feel free comment better ways.


Thanks
Steven
Nicolas George Aug. 17, 2020, 9:24 a.m. UTC | #5
Steven Liu (12020-08-17):
> When hls muxer have beed support split_by_time flag add, the 8216 have
> not beed release,
> and there have some guys order this feature, for example me.
> And I saw lots of people are using this flag, so this maybe cannot be
> removed now.
> So I think give a warning message and modify it to one packet duration is ok.

No, it is not ok until we understand properly the problem.

Have you observed the timestamps and type of several packets around the
issue, plus the places where the muxer chooses to split?

If yes, then please show the result; even better include it in the
commit message.

If no, then your analysis of the issue is way too shallow to judge
whether this is a proper fix.

Regards,
Nicolas George Aug. 17, 2020, 9:25 a.m. UTC | #6
Steven Liu (12020-08-17):
> As Zhili Zhao said, the problem is the user is using hls_time with too
> short duration and want split not by keyframe.
> I have been think about how to make the user know it don't support
> short than 2 frames and with bframes,
> but this is not the better way.
> I think give a suggestion and warning message is better.
> Feel free comment better ways.

See my other reply.

Regards,
Steven Liu Aug. 17, 2020, 9:40 a.m. UTC | #7
Nicolas George <george@nsup.org> 于2020年8月17日周一 下午5:24写道:
>
> Steven Liu (12020-08-17):
> > When hls muxer have beed support split_by_time flag add, the 8216 have
> > not beed release,
> > and there have some guys order this feature, for example me.
> > And I saw lots of people are using this flag, so this maybe cannot be
> > removed now.
> > So I think give a warning message and modify it to one packet duration is ok.
>
> No, it is not ok until we understand properly the problem.
>
> Have you observed the timestamps and type of several packets around the
> issue, plus the places where the muxer chooses to split?
Good point, Thanks.
>
> If yes, then please show the result; even better include it in the
> commit message.
>
> If no, then your analysis of the issue is way too shallow to judge
> whether this is a proper fix.
>
> Regards,
New version will submit.

Thanks
Steven
diff mbox series

Patch

diff --git a/libavformat/hlsenc.c b/libavformat/hlsenc.c
index cb31d6aed7..79ee39aa23 100644
--- a/libavformat/hlsenc.c
+++ b/libavformat/hlsenc.c
@@ -2512,6 +2512,12 @@  static int hls_write_packet(AVFormatContext *s, AVPacket *pkt)
 
         if (vs->start_pos || hls->segment_type != SEGMENT_TYPE_FMP4) {
             double cur_duration =  (double)(pkt->pts - vs->end_pts) * st->time_base.num / st->time_base.den;
+            if (cur_duration < 0) {
+                av_log(s, AV_LOG_WARNING, "The target duration incorrect, maybe you splited a too short segment, "
+                                        "you should increase the hls_time, "
+                                        "now the duration will set to 1 packet duration, but maybe incorrect too.\n");
+                cur_duration = vs->duration;
+            }
             ret = hls_append_segment(s, hls, vs, cur_duration, vs->start_pos, vs->size);
             vs->end_pts = pkt->pts;
             vs->duration = 0;