[FFmpeg-devel] Resending Patch for hlsenc.c fixes for https://trac.ffmpeg.org/ticket/7281

Steven Liu lq at chinaffmpeg.org
Thu Aug 16 03:21:48 EEST 2018



> On Aug 16, 2018, at 07:41, Ronak Patel <ronak2121 at yahoo.com> wrote:
> 
>> 
>> On Aug 15, 2018, at 11:08 AM, Steven Liu <lq at chinaffmpeg.org> wrote:
>> 
>> 
>> 
>>> On Aug 15, 2018, at 09:31, Ronak <ronak2121-at-yahoo.com at ffmpeg.org> wrote:
>>> 
>>> From: "Ronak Patel" <ronak2121@ <mailto:ronakp at audible.com>yahoo.com>
>>> 
>>> This fixes the creation of the hls manifest in hlsenc.c by writing the entire manifest at the end for VOD playlists. Live & Event Playlists are unaffected.
>>> This also fixes the behavior with HLS_TEMP_FILE to work correctly when -hlsflags temp_file is specified, instead of always relying on use_rename, which caused these problems.
>>> 
>>> Files that would previously take over a week to fragment now take 1 minute on the same hardware. This was a 153 hour audio file (2.2GB of audio).
>>> 
>>> Signed-off-by: Ronak Patel <ronak2121 at yahoo.com <mailto:ronak2121 at yahoo.com>>
>>> ---
>>> libavformat/hlsenc.c | 51 ++++++++++++++++++++++++++++++++++++---------------
>>> 1 file changed, 36 insertions(+), 15 deletions(-)
>>> 
>>> diff --git a/libavformat/hlsenc.c b/libavformat/hlsenc.c
>>> index b5644f0..7933b79 100644
>>> --- a/libavformat/hlsenc.c
>>> +++ b/libavformat/hlsenc.c
>>> @@ -1168,7 +1168,6 @@ static int hls_rename_temp_file(AVFormatContext *s, AVFormatContext *oc)
>>>       return AVERROR(ENOMEM);
>>>   final_filename[len-4] = '\0';
>>>   ret = ff_rename(oc->url, final_filename, s);
>>> -    oc->url[len-4] = '\0';
>>>   av_freep(&final_filename);
>>>   return ret;
>>> }
>>> @@ -1374,7 +1373,7 @@ static int hls_window(AVFormatContext *s, int last, VariantStream *vs)
>>>   char temp_filename[1024];
>>>   int64_t sequence = FFMAX(hls->start_sequence, vs->sequence - vs->nb_entries);
>>>   const char *proto = avio_find_protocol_name(s->url);
>>> -    int use_rename = proto && !strcmp(proto, "file");
>>> +    int use_temp_file = proto && !strcmp(proto, "file") && (s->flags & HLS_TEMP_FILE);
>>>   static unsigned warned_non_file;
>>>   char *key_uri = NULL;
>>>   char *iv_string = NULL;
>>> @@ -1397,11 +1396,11 @@ static int hls_window(AVFormatContext *s, int last, VariantStream *vs)
>>>       hls->version = 7;
>>>   }
>>> -    if (!use_rename && !warned_non_file++)
>>> +    if (!use_temp_file && !warned_non_file++)
>>>       av_log(s, AV_LOG_ERROR, "Cannot use rename on non file protocol, this may lead to races and temporary partial files\n");
>>>   set_http_options(s, &options, hls);
>>> -    snprintf(temp_filename, sizeof(temp_filename), use_rename ? "%s.tmp" : "%s", vs->m3u8_name);
>>> +    snprintf(temp_filename, sizeof(temp_filename), use_temp_file ? "%s.tmp" : "%s", vs->m3u8_name);
>>>   if ((ret = hlsenc_io_open(s, &hls->m3u8_out, temp_filename, &options)) < 0)
>>>       goto fail;
>>> @@ -1472,8 +1471,8 @@ fail:
>>>   av_dict_free(&options);
>>>   hlsenc_io_close(s, &hls->m3u8_out, temp_filename);
>>>   hlsenc_io_close(s, &hls->sub_m3u8_out, vs->vtt_m3u8_name);
>>> -    if (ret >= 0 && use_rename)
>>> -        ff_rename(temp_filename, vs->m3u8_name, s);
>>> +    if (use_temp_file)
>>> +        ff_rename(temp_filename, vs->m3u8_name, s);
>>>   if (ret >= 0 && hls->master_pl_name)
>>>       if (create_master_playlist(s, vs) < 0)
>>> @@ -1488,6 +1487,8 @@ static int hls_start(AVFormatContext *s, VariantStream *vs)
>>>   AVFormatContext *oc = vs->avf;
>>>   AVFormatContext *vtt_oc = vs->vtt_avf;
>>>   AVDictionary *options = NULL;
>>> +    const char *proto = avio_find_protocol_name(s->url);
>>> +    int use_temp_file = proto && !strcmp(proto, "file") && (s->flags & HLS_TEMP_FILE);
>>>   char *filename, iv_string[KEYSIZE*2 + 1];
>>>   int err = 0;
>>> @@ -1583,7 +1584,7 @@ static int hls_start(AVFormatContext *s, VariantStream *vs)
>>>   set_http_options(s, &options, c);
>>> -    if (c->flags & HLS_TEMP_FILE) {
>>> +    if (use_temp_file) {
>>>       char *new_name = av_asprintf("%s.tmp", oc->url);
>>>       if (!new_name)
>>>           return AVERROR(ENOMEM);
>>> @@ -2145,6 +2146,8 @@ static int hls_write_packet(AVFormatContext *s, AVPacket *pkt)
>>>   int ret = 0, can_split = 1, i, j;
>>>   int stream_index = 0;
>>>   int range_length = 0;
>>> +    const char *proto = avio_find_protocol_name(s->url);
>>> +    int use_temp_file = proto && !strcmp(proto, "file") && (s->flags & HLS_TEMP_FILE);
>>>   uint8_t *buffer = NULL;
>>>   VariantStream *vs = NULL;
>>>   AVDictionary *options = NULL;
>>> @@ -2214,10 +2217,10 @@ static int hls_write_packet(AVFormatContext *s, AVPacket *pkt)
>>>   }
>>> +    char *old_filename = NULL;
>>>   if (vs->packets_written && can_split && av_compare_ts(pkt->pts - vs->start_pts, st->time_base,
>>>                                                         end_pts, AV_TIME_BASE_Q) >= 0) {
>>>       int64_t new_start_pos;
>>> -        char *old_filename = NULL;
>>>       int byterange_mode = (hls->flags & HLS_SINGLE_FILE) || (hls->max_seg_size > 0);
>>>       av_write_frame(vs->avf, NULL); /* Flush any buffered data */
>>> @@ -2253,11 +2256,13 @@ static int hls_write_packet(AVFormatContext *s, AVPacket *pkt)
>>>               hlsenc_io_close(s, &vs->vtt_avf->pb, vs->vtt_avf->url);
>>>           }
>>>       }
>>> -        if ((hls->flags & HLS_TEMP_FILE) && oc->url[0]) {
>>> +
>>> +        // look to rename the asset name
>>> +        if (use_temp_file && oc->url[0]) {
>>>           if (!(hls->flags & HLS_SINGLE_FILE) || (hls->max_seg_size <= 0))
>>> -                if ((vs->avf->oformat->priv_class && vs->avf->priv_data) && hls->segment_type != SEGMENT_TYPE_FMP4)
>>> -                    av_opt_set(vs->avf->priv_data, "mpegts_flags", "resend_headers", 0);
>>> -            hls_rename_temp_file(s, oc);
>>> +                if ((vs->avf->oformat->priv_class && vs->avf->priv_data) && hls->segment_type != SEGMENT_TYPE_FMP4) {
>>> +                    av_opt_set(vs->avf->priv_data, "mpegts_flags", "resend_headers", 0);
>>> +                }
>>>       }
>>>       if (vs->fmp4_init_mode) {
>>> @@ -2286,6 +2291,17 @@ static int hls_write_packet(AVFormatContext *s, AVPacket *pkt)
>>>                   return ret;
>>>               }
>>>               ff_format_io_close(s, &vs->out);
>>> +
>>> +                // rename that segment from .tmp to the real one
>>> +                if (use_temp_file && oc->url[0]) {
>>> +                    hls_rename_temp_file(s, oc);
>>> +                    av_free(old_filename);
>>> +                    old_filename = av_strdup(vs->avf->url);
>>> +
>>> +                    if (!old_filename) {
>>> +                        return AVERROR(ENOMEM);
>>> +                    }
>>> +                }
>> All of these is TAB?
> 
> If I switch to spaces, do you have any other comments? Are you happy with this approach overall?
mhmm, maybe have other comments, maybe have no, I don’t think this is a good way, but you need read https://ffmpeg.org/developer.html at first for the code style.
> 
> I don’t want to waste time on an approach you dislike here.
> 
> We still have to go fix dashenc.c for this same problem.
Ok, you can do that first if you dislike continue to hlsenc.

Then, if your patch merged, there have have other problem, for example: 
 people want request the m3u8 list when ffmpeg processing the list.

> 
>>>           }
>>>       }
>>> @@ -2334,10 +2350,12 @@ static int hls_write_packet(AVFormatContext *s, AVPacket *pkt)
>>>           return ret;
>>>       }
>>> -        if (!vs->fmp4_init_mode || byterange_mode)
>>> +        // if we're building a VOD playlist, skip writing the manifest multiple times, and just wait until the end
>>> +        if (hls->pl_type != PLAYLIST_TYPE_VOD) {
>>>           if ((ret = hls_window(s, 0, vs)) < 0) {
>>>               return ret;
>>>           }
>>> +        }
>>>   }
>>>   vs->packets_written++;
>>> @@ -2352,6 +2370,8 @@ static int hls_write_trailer(struct AVFormatContext *s)
>>>   AVFormatContext *oc = NULL;
>>>   AVFormatContext *vtt_oc = NULL;
>>>   char *old_filename = NULL;
>>> +    const char *proto = avio_find_protocol_name(s->url);
>>> +    int use_temp_file = proto && !strcmp(proto, "file") && (s->flags & HLS_TEMP_FILE);
>>>   int i;
>>>   int ret = 0;
>>>   VariantStream *vs = NULL;
>>> @@ -2394,8 +2414,9 @@ failed:
>>>           if (hls->segment_type != SEGMENT_TYPE_FMP4)
>>>               ff_format_io_close(s, &oc->pb);
>>> -            if ((hls->flags & HLS_TEMP_FILE) && oc->url[0]) {
>>> -                hls_rename_temp_file(s, oc);
>>> +            // rename that segment from .tmp to the real one
>>> +            if (use_temp_file && oc->url[0] && !(hls->flags & HLS_SINGLE_FILE)) {
>> Is this TAB?
>>> +                hls_rename_temp_file(s, oc);
>>>               av_free(old_filename);
>>>               old_filename = av_strdup(vs->avf->url);
>>> -- 
>>> 2.6.3
>>> _______________________________________________
>>> ffmpeg-devel mailing list
>>> ffmpeg-devel at ffmpeg.org
>>> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>> 
>> Thanks
>> Steven
>> 
>> 
>> 
>> 
>> 
>> _______________________________________________
>> ffmpeg-devel mailing list
>> ffmpeg-devel at ffmpeg.org
>> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Thanks
Steven







More information about the ffmpeg-devel mailing list