[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:49:21 EEST 2018



> On Aug 16, 2018, at 08:39, Ronak Patel <ronak2121 at yahoo.com> wrote:
> 
>> 
>> On Aug 15, 2018, at 8:21 PM, Steven Liu <lq at chinaffmpeg.org> wrote:
>> 
>> 
>> 
>>>> 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.
> 
> I would like to fix both here (dash and hls), not just one.
> 
> I’ll fix the tabs to spaces here.
> 
> For VOD, I doubt that would happen where we’d need to look at an intermediary point in the playlist. 
ok, maybe i should modify this after your patch, append the fragment info to the list and flush the file every time.
but i think that is waste time double coding for the VOD type hls, i cannot sure there have or have no people to request the list as live streaming when use this mode, I think that maybe better compatible all possibility.
> 
> For EVENT and LIVE it makes more sense.
> 
>>> 
>>>>>         }
>>>>>     }
>>>>> @@ -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
>> 
>> 
>> 
>> 
>> 
>> _______________________________________________
>> 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