diff mbox series

[FFmpeg-devel] avformat/hlsenc: move init file write code block to hls_write_header

Message ID 20200507104749.61667-1-lq@chinaffmpeg.org
State New
Headers show
Series [FFmpeg-devel] avformat/hlsenc: move init file write code block to hls_write_header
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 May 7, 2020, 10:47 a.m. UTC
Signed-off-by: Steven Liu <lq@chinaffmpeg.org>
---
 libavformat/hlsenc.c | 37 ++++++++++++++++++++-----------------
 1 file changed, 20 insertions(+), 17 deletions(-)

Comments

Andreas Rheinhardt May 9, 2020, 7:21 p.m. UTC | #1
Steven Liu:
> Signed-off-by: Steven Liu <lq@chinaffmpeg.org>
> ---
>  libavformat/hlsenc.c | 37 ++++++++++++++++++++-----------------
>  1 file changed, 20 insertions(+), 17 deletions(-)
> 
> diff --git a/libavformat/hlsenc.c b/libavformat/hlsenc.c
> index 5695c6cc95..a1eddade7b 100644
> --- a/libavformat/hlsenc.c
> +++ b/libavformat/hlsenc.c
> @@ -2266,6 +2266,26 @@ static int hls_write_header(AVFormatContext *s)
>                  }
>              }
>          }
> +        if (hls->segment_type == SEGMENT_TYPE_FMP4 && !vs->init_range_length) {
> +            int range_length = 0;
> +            int byterange_mode = (hls->flags & HLS_SINGLE_FILE) || (hls->max_seg_size > 0);
> +            av_write_frame(vs->avf, NULL); /* Flush any buffered data */
> +            avio_tell(vs->avf->pb);
> +            avio_flush(vs->avf->pb);
> +            range_length = avio_close_dyn_buf(vs->avf->pb, &vs->init_buffer);
> +            if (range_length <= 0)
> +                return AVERROR(EINVAL);
> +            avio_write(vs->out, vs->init_buffer, range_length);
> +            if (!hls->resend_init_file)
> +                av_freep(&vs->init_buffer);
> +            vs->init_range_length = range_length;
> +            avio_open_dyn_buf(&vs->avf->pb);
> +            vs->packets_written = 0;
> +            vs->start_pos = range_length;
> +            if (!byterange_mode) {
> +                hlsenc_io_close(s, &vs->out, vs->base_output_dirname);
> +            }
> +        }
>      }
>  
>      return ret;
> @@ -2383,23 +2403,6 @@ static int hls_write_packet(AVFormatContext *s, AVPacket *pkt)
>          new_start_pos = avio_tell(oc->pb);
>          vs->size = new_start_pos - vs->start_pos;
>          avio_flush(oc->pb);
> -        if (hls->segment_type == SEGMENT_TYPE_FMP4) {
> -            if (!vs->init_range_length) {
> -                range_length = avio_close_dyn_buf(oc->pb, &vs->init_buffer);
> -                if (range_length <= 0)
> -                    return AVERROR(EINVAL);
> -                avio_write(vs->out, vs->init_buffer, range_length);
> -                if (!hls->resend_init_file)
> -                    av_freep(&vs->init_buffer);
> -                vs->init_range_length = range_length;
> -                avio_open_dyn_buf(&oc->pb);
> -                vs->packets_written = 0;
> -                vs->start_pos = range_length;
> -                if (!byterange_mode) {
> -                    hlsenc_io_close(s, &vs->out, vs->base_output_dirname);
> -                }
> -            }
> -        }
>          if (!byterange_mode) {
>              if (vs->vtt_avf) {
>                  hlsenc_io_close(s, &vs->vtt_avf->pb, vs->vtt_avf->url);
> 

1. Delaying writing the header was introduced in commit
880b299381de1e66f8248abd6c320c7c490466a2 to fix ticket 6825 (where the
number of channels in the input stream switches). Have you tested
whether your patch breaks said ticket again?
2. You don't need to check for vs->init_range_length being zero when
writing the header as this is automatically fulfilled. (After all, one
calls avformat_write_header() only once.)
3. There is also code in hls_write_trailer() that duplicates the code to
write the init file (should actually have been factored out into a
common function...). It has been added in commit
76b8e42c1f0453215244c45114d5aa302e2add7b to address ticket #7166 and if
you always write the init file in write_header, you should also remove
the code in write_trailer.
4. Commit 880b299381de1e66f8248abd6c320c7c490466a2 also added the
packets_written member; it was initialized to 0 in mp4 mode and to 1 in
non-mp4 mode. Since cff309097a3af4f7f2acb5b5f9fb914ba9ae45c3 this was
set to 1 initially just in order to be reset to 0 a few lines below.
Maybe packets_written should be removed completely? (I made a patch [2]
to remove the currently redundant initializations.)
5. The current way of writing the init header (that you intend to remove
in this commit) has a serious issue: If you have a subtitle and an audio
stream, yet no video stream and if the audio is delayed sufficiently,
then a subtitle packet triggers the output of the mp4 init header. But
subtitles are output directly and not via dynamic buffers and yet the
code presumes that the AVIOContext for output is a dynamic buffer. This
leads to segfaults. (If you remember, this is point 6 in [1].) This
patch seems to fix this; but if you drop this patch because of 1., this
still needs to addressed.

- Andreas

[1]: https://ffmpeg.org/pipermail/ffmpeg-devel/2019-December/254420.html
[2]: https://ffmpeg.org/pipermail/ffmpeg-devel/2020-May/262385.html
Steven Liu May 9, 2020, 11:09 p.m. UTC | #2
> 2020年5月10日 上午3:21,Andreas Rheinhardt <andreas.rheinhardt@gmail.com> 写道:
> 
> Steven Liu:
>> Signed-off-by: Steven Liu <lq@chinaffmpeg.org>
>> ---
>> libavformat/hlsenc.c | 37 ++++++++++++++++++++-----------------
>> 1 file changed, 20 insertions(+), 17 deletions(-)
>> 
>> diff --git a/libavformat/hlsenc.c b/libavformat/hlsenc.c
>> index 5695c6cc95..a1eddade7b 100644
>> --- a/libavformat/hlsenc.c
>> +++ b/libavformat/hlsenc.c
>> @@ -2266,6 +2266,26 @@ static int hls_write_header(AVFormatContext *s)
>>                 }
>>             }
>>         }
>> +        if (hls->segment_type == SEGMENT_TYPE_FMP4 && !vs->init_range_length) {
>> +            int range_length = 0;
>> +            int byterange_mode = (hls->flags & HLS_SINGLE_FILE) || (hls->max_seg_size > 0);
>> +            av_write_frame(vs->avf, NULL); /* Flush any buffered data */
>> +            avio_tell(vs->avf->pb);
>> +            avio_flush(vs->avf->pb);
>> +            range_length = avio_close_dyn_buf(vs->avf->pb, &vs->init_buffer);
>> +            if (range_length <= 0)
>> +                return AVERROR(EINVAL);
>> +            avio_write(vs->out, vs->init_buffer, range_length);
>> +            if (!hls->resend_init_file)
>> +                av_freep(&vs->init_buffer);
>> +            vs->init_range_length = range_length;
>> +            avio_open_dyn_buf(&vs->avf->pb);
>> +            vs->packets_written = 0;
>> +            vs->start_pos = range_length;
>> +            if (!byterange_mode) {
>> +                hlsenc_io_close(s, &vs->out, vs->base_output_dirname);
>> +            }
>> +        }
>>     }
>> 
>>     return ret;
>> @@ -2383,23 +2403,6 @@ static int hls_write_packet(AVFormatContext *s, AVPacket *pkt)
>>         new_start_pos = avio_tell(oc->pb);
>>         vs->size = new_start_pos - vs->start_pos;
>>         avio_flush(oc->pb);
>> -        if (hls->segment_type == SEGMENT_TYPE_FMP4) {
>> -            if (!vs->init_range_length) {
>> -                range_length = avio_close_dyn_buf(oc->pb, &vs->init_buffer);
>> -                if (range_length <= 0)
>> -                    return AVERROR(EINVAL);
>> -                avio_write(vs->out, vs->init_buffer, range_length);
>> -                if (!hls->resend_init_file)
>> -                    av_freep(&vs->init_buffer);
>> -                vs->init_range_length = range_length;
>> -                avio_open_dyn_buf(&oc->pb);
>> -                vs->packets_written = 0;
>> -                vs->start_pos = range_length;
>> -                if (!byterange_mode) {
>> -                    hlsenc_io_close(s, &vs->out, vs->base_output_dirname);
>> -                }
>> -            }
>> -        }
>>         if (!byterange_mode) {
>>             if (vs->vtt_avf) {
>>                 hlsenc_io_close(s, &vs->vtt_avf->pb, vs->vtt_avf->url);
>> 
> 
> 1. Delaying writing the header was introduced in commit
> 880b299381de1e66f8248abd6c320c7c490466a2 to fix ticket 6825 (where the
> number of channels in the input stream switches).
Ok, I get it, this is a good point.

> Have you tested
> whether your patch breaks said ticket again?
> 2. You don't need to check for vs->init_range_length being zero when
> writing the header as this is automatically fulfilled. (After all, one
> calls avformat_write_header() only once.)
> 3. There is also code in hls_write_trailer() that duplicates the code to
> write the init file (should actually have been factored out into a
> common function...). It has been added in commit
> 76b8e42c1f0453215244c45114d5aa302e2add7b to address ticket #7166 and if
> you always write the init file in write_header, you should also remove
> the code in write_trailer.
> 4. Commit 880b299381de1e66f8248abd6c320c7c490466a2 also added the
> packets_written member; it was initialized to 0 in mp4 mode and to 1 in
> non-mp4 mode. Since cff309097a3af4f7f2acb5b5f9fb914ba9ae45c3 this was
> set to 1 initially just in order to be reset to 0 a few lines below.
> Maybe packets_written should be removed completely? (I made a patch [2]
> to remove the currently redundant initializations.)
> 5. The current way of writing the init header (that you intend to remove
> in this commit) has a serious issue: If you have a subtitle and an audio
> stream, yet no video stream and if the audio is delayed sufficiently,
> then a subtitle packet triggers the output of the mp4 init header. But
> subtitles are output directly and not via dynamic buffers and yet the
> code presumes that the AVIOContext for output is a dynamic buffer. This
> leads to segfaults. (If you remember, this is point 6 in [1].) This
> patch seems to fix this; but if you drop this patch because of 1., this
> still needs to addressed.
I Get your point. 
Let me think about how to move it out of hls_write_packet,
Maybe hlsenc cannot support LowLatency HLS if it always in hls_write_packet.
Or maybe need modify some code in movenc, I want try only modify in hlsenc.
> 
> - Andreas
> 
> [1]: https://ffmpeg.org/pipermail/ffmpeg-devel/2019-December/254420.html
> [2]: https://ffmpeg.org/pipermail/ffmpeg-devel/2020-May/262385.html
> _______________________________________________
> 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".

Thanks

Steven Liu
diff mbox series

Patch

diff --git a/libavformat/hlsenc.c b/libavformat/hlsenc.c
index 5695c6cc95..a1eddade7b 100644
--- a/libavformat/hlsenc.c
+++ b/libavformat/hlsenc.c
@@ -2266,6 +2266,26 @@  static int hls_write_header(AVFormatContext *s)
                 }
             }
         }
+        if (hls->segment_type == SEGMENT_TYPE_FMP4 && !vs->init_range_length) {
+            int range_length = 0;
+            int byterange_mode = (hls->flags & HLS_SINGLE_FILE) || (hls->max_seg_size > 0);
+            av_write_frame(vs->avf, NULL); /* Flush any buffered data */
+            avio_tell(vs->avf->pb);
+            avio_flush(vs->avf->pb);
+            range_length = avio_close_dyn_buf(vs->avf->pb, &vs->init_buffer);
+            if (range_length <= 0)
+                return AVERROR(EINVAL);
+            avio_write(vs->out, vs->init_buffer, range_length);
+            if (!hls->resend_init_file)
+                av_freep(&vs->init_buffer);
+            vs->init_range_length = range_length;
+            avio_open_dyn_buf(&vs->avf->pb);
+            vs->packets_written = 0;
+            vs->start_pos = range_length;
+            if (!byterange_mode) {
+                hlsenc_io_close(s, &vs->out, vs->base_output_dirname);
+            }
+        }
     }
 
     return ret;
@@ -2383,23 +2403,6 @@  static int hls_write_packet(AVFormatContext *s, AVPacket *pkt)
         new_start_pos = avio_tell(oc->pb);
         vs->size = new_start_pos - vs->start_pos;
         avio_flush(oc->pb);
-        if (hls->segment_type == SEGMENT_TYPE_FMP4) {
-            if (!vs->init_range_length) {
-                range_length = avio_close_dyn_buf(oc->pb, &vs->init_buffer);
-                if (range_length <= 0)
-                    return AVERROR(EINVAL);
-                avio_write(vs->out, vs->init_buffer, range_length);
-                if (!hls->resend_init_file)
-                    av_freep(&vs->init_buffer);
-                vs->init_range_length = range_length;
-                avio_open_dyn_buf(&oc->pb);
-                vs->packets_written = 0;
-                vs->start_pos = range_length;
-                if (!byterange_mode) {
-                    hlsenc_io_close(s, &vs->out, vs->base_output_dirname);
-                }
-            }
-        }
         if (!byterange_mode) {
             if (vs->vtt_avf) {
                 hlsenc_io_close(s, &vs->vtt_avf->pb, vs->vtt_avf->url);