diff mbox

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

Message ID 2C7C175B-C73C-4F19-AB27-4D5D0A78BAFC@yahoo.com
State Superseded
Headers show

Commit Message

Ronak Patel Aug. 15, 2018, 1:31 a.m. UTC 
From: "Ronak Patel" <ronak2121@ <mailto:ronakp@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@yahoo.com <mailto:ronak2121@yahoo.com>>
---
libavformat/hlsenc.c | 51 ++++++++++++++++++++++++++++++++++++---------------
1 file changed, 36 insertions(+), 15 deletions(-)

     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);
+					}
+				}
             }
         }
@@ -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)) {
+				hls_rename_temp_file(s, oc);
                 av_free(old_filename);
                 old_filename = av_strdup(vs->avf->url);

Comments

Liu Steven Aug. 15, 2018, 2:57 a.m. UTC | #1 
> 在 2018年8月15日，上午9:31，Ronak <ronak2121-at-yahoo.com@ffmpeg.org> 写道：
> 
> From: "Ronak Patel" <ronak2121@ <mailto:ronakp@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@yahoo.com <mailto:ronak2121@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);
> +					}
> +				}
>             }
>         }
> @@ -2334,10 +2350,12 @@ static int hls_write_packet(AVFormatContext *s, AVPacket *pkt)
>             return ret;
>         }
> -        if (!vs->fmp4_init_mode || byterange_mode)
why do you remove this line?
> +        // 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) {
maybe your mean is just write one time when PLAYLIST_TYPE_VOD, that is happen at the end of the full list, is that right?
>             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)) {
> +				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@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Ronak Patel Aug. 15, 2018, 10:37 a.m. UTC | #2 
> On Aug 14, 2018, at 10:57 PM, Liu Steven <lq@chinaffmpeg.org> wrote:
> 
> 
> 
>> 在 2018年8月15日，上午9:31，Ronak <ronak2121-at-yahoo.com@ffmpeg.org> 写道：
>> 
>> From: "Ronak Patel" <ronak2121@ <mailto:ronak2121@yahoo.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@yahoo.com <mailto:ronak2121@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);
>> +                    }
>> +                }
>>            }
>>        }
>> @@ -2334,10 +2350,12 @@ static int hls_write_packet(AVFormatContext *s, AVPacket *pkt)
>>            return ret;
>>        }
>> -        if (!vs->fmp4_init_mode || byterange_mode)
> why do you remove this line?

Because the above code sets fmp4_init_mode to 0 and this is always true. I tested mpegts and fmp4 in single file and multiple file modes and this is always true.

>> +        // 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) {
> maybe your mean is just write one time when PLAYLIST_TYPE_VOD, that is happen at the end of the full list, is that right?

Yes. Like I mentioned before there is no need to refresh the whole file every single time.

This is not the best fix in my opinion either but it is the simplest. The best way would be to split the code based on the playlist type so it was easier to follow. This is altogether confusing code.

>>            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)) {
>> +                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@ffmpeg.org
>> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> 
> 
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Liu Steven Aug. 15, 2018, 10:46 a.m. UTC | #3 
> 在 2018年8月15日，下午6:37，Ronak Patel <ronak2121@yahoo.com> 写道：
> 
>> 
>> On Aug 14, 2018, at 10:57 PM, Liu Steven <lq@chinaffmpeg.org> wrote:
>> 
>> 
>> 
>>> 在 2018年8月15日，上午9:31，Ronak <ronak2121-at-yahoo.com@ffmpeg.org> 写道：
>>> 
>>> From: "Ronak Patel" <ronak2121@ <mailto:ronak2121@yahoo.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@yahoo.com <mailto:ronak2121@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);
>>> +                    }
>>> +                }
>>>           }
>>>       }
>>> @@ -2334,10 +2350,12 @@ static int hls_write_packet(AVFormatContext *s, AVPacket *pkt)
>>>           return ret;
>>>       }
>>> -        if (!vs->fmp4_init_mode || byterange_mode)
>> why do you remove this line?
> 
> Because the above code sets fmp4_init_mode to 0 and this is always true. I tested mpegts and fmp4 in single file and multiple file modes and this is always true.
There have a logic:

 771     if (hls->segment_type == SEGMENT_TYPE_FMP4) {
 772         if (hls->max_seg_size > 0) {
 773             av_log(s, AV_LOG_WARNING, "Multi-file byterange mode is currently unsupported in the HLS muxer.\n");
 774             return AVERROR_PATCHWELCOME;
 775         }
 776
 777         vs->packets_written = 0;
 778         vs->init_range_length = 0;
 779         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) {
>> maybe your mean is just write one time when PLAYLIST_TYPE_VOD, that is happen at the end of the full list, is that right?
> 
> Yes. Like I mentioned before there is no need to refresh the whole file every single time.
> 
> This is not the best fix in my opinion either but it is the simplest. The best way would be to split the code based on the playlist type so it was easier to follow. This is altogether confusing code.
> 
>>>           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)) {
>>> +                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@ffmpeg.org
>>> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>> 
>> 
>> 
>> _______________________________________________
>> ffmpeg-devel mailing list
>> ffmpeg-devel@ffmpeg.org
>> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Ronak Patel Aug. 15, 2018, 11:19 a.m. UTC | #4 
> On Aug 15, 2018, at 6:46 AM, Liu Steven <lq@chinaffmpeg.org> wrote:
> 
> 
> 
>>> 在 2018年8月15日，下午6:37，Ronak Patel <ronak2121@yahoo.com> 写道：
>>> 
>>> 
>>> On Aug 14, 2018, at 10:57 PM, Liu Steven <lq@chinaffmpeg.org> wrote:
>>> 
>>> 
>>> 
>>>> 在 2018年8月15日，上午9:31，Ronak <ronak2121-at-yahoo.com@ffmpeg.org> 写道：
>>>> 
>>>> From: "Ronak Patel" <ronak2121@ <mailto:ronak2121@yahoo.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@yahoo.com <mailto:ronak2121@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);
>>>> +                    }
>>>> +                }
>>>>          }
>>>>      }
>>>> @@ -2334,10 +2350,12 @@ static int hls_write_packet(AVFormatContext *s, AVPacket *pkt)
>>>>          return ret;
>>>>      }
>>>> -        if (!vs->fmp4_init_mode || byterange_mode)
>>> why do you remove this line?
>> 
>> Because the above code sets fmp4_init_mode to 0 and this is always true. I tested mpegts and fmp4 in single file and multiple file modes and this is always true.
> There have a logic:
> 
> 771     if (hls->segment_type == SEGMENT_TYPE_FMP4) {
> 772         if (hls->max_seg_size > 0) {
> 773             av_log(s, AV_LOG_WARNING, "Multi-file byterange mode is currently unsupported in the HLS muxer.\n");
> 774             return AVERROR_PATCHWELCOME;
> 775         }
> 776
> 777         vs->packets_written = 0;
> 778         vs->init_range_length = 0;
> 779         vs->fmp4_init_mode = !byterange_mode;
> 

This is not called between the lines that sets fmp4_init_mode to 0. It’s also not called by “hls_start” either. hls_mux_init is not invoked here. It is only invoked once at init.

Even then, this logic sets fmp4_init_mode to be the converse of byterange_mode. In the if statement I removed, this will always be true.

>> 
>>>> +        // 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) {
>>> maybe your mean is just write one time when PLAYLIST_TYPE_VOD, that is happen at the end of the full list, is that right?
>> 
>> Yes. Like I mentioned before there is no need to refresh the whole file every single time.
>> 
>> This is not the best fix in my opinion either but it is the simplest. The best way would be to split the code based on the playlist type so it was easier to follow. This is altogether confusing code.
>> 
>>>>          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)) {
>>>> +                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@ffmpeg.org
>>>> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>>> 
>>> 
>>> 
>>> _______________________________________________
>>> ffmpeg-devel mailing list
>>> ffmpeg-devel@ffmpeg.org
>>> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> 
> 
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Liu Steven Aug. 15, 2018, 3:08 p.m. UTC | #5 
> On Aug 15, 2018, at 09:31, Ronak <ronak2121-at-yahoo.com@ffmpeg.org> wrote:
> 
> From: "Ronak Patel" <ronak2121@ <mailto:ronakp@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@yahoo.com <mailto:ronak2121@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?
>             }
>         }
> @@ -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@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Thanks
Steven
Ronak Patel Aug. 15, 2018, 11:41 p.m. UTC | #6 
> On Aug 15, 2018, at 11:08 AM, Steven Liu <lq@chinaffmpeg.org> wrote:
> 
> 
> 
>> On Aug 15, 2018, at 09:31, Ronak <ronak2121-at-yahoo.com@ffmpeg.org> wrote:
>> 
>> From: "Ronak Patel" <ronak2121@ <mailto:ronakp@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@yahoo.com <mailto:ronak2121@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?

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.

>>            }
>>        }
>> @@ -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@ffmpeg.org
>> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> Thanks
> Steven
> 
> 
> 
> 
> 
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Liu Steven Aug. 16, 2018, 12:21 a.m. UTC | #7 
> On Aug 16, 2018, at 07:41, Ronak Patel <ronak2121@yahoo.com> wrote:
> 
>> 
>> On Aug 15, 2018, at 11:08 AM, Steven Liu <lq@chinaffmpeg.org> wrote:
>> 
>> 
>> 
>>> On Aug 15, 2018, at 09:31, Ronak <ronak2121-at-yahoo.com@ffmpeg.org> wrote:
>>> 
>>> From: "Ronak Patel" <ronak2121@ <mailto:ronakp@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@yahoo.com <mailto:ronak2121@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@ffmpeg.org
>>> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>> 
>> Thanks
>> Steven
>> 
>> 
>> 
>> 
>> 
>> _______________________________________________
>> ffmpeg-devel mailing list
>> ffmpeg-devel@ffmpeg.org
>> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Thanks
Steven
Ronak Patel Aug. 16, 2018, 12:39 a.m. UTC | #8 
> On Aug 15, 2018, at 8:21 PM, Steven Liu <lq@chinaffmpeg.org> wrote:
> 
> 
> 
>>> On Aug 16, 2018, at 07:41, Ronak Patel <ronak2121@yahoo.com> wrote:
>>> 
>>> 
>>> On Aug 15, 2018, at 11:08 AM, Steven Liu <lq@chinaffmpeg.org> wrote:
>>> 
>>> 
>>> 
>>>> On Aug 15, 2018, at 09:31, Ronak <ronak2121-at-yahoo.com@ffmpeg.org> wrote:
>>>> 
>>>> From: "Ronak Patel" <ronak2121@ <mailto:ronakp@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@yahoo.com <mailto:ronak2121@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. 

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@ffmpeg.org
>>>> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>>> 
>>> Thanks
>>> Steven
>>> 
>>> 
>>> 
>>> 
>>> 
>>> _______________________________________________
>>> ffmpeg-devel mailing list
>>> ffmpeg-devel@ffmpeg.org
>>> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> Thanks
> Steven
> 
> 
> 
> 
> 
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Ronak Patel Aug. 16, 2018, 12:41 a.m. UTC | #9 
> On Aug 15, 2018, at 8:39 PM, Ronak Patel <ronak2121@yahoo.com> wrote:
> 
> 
>> On Aug 15, 2018, at 8:21 PM, Steven Liu <lq@chinaffmpeg.org> wrote:
>> 
>> 
>> 
>>>> On Aug 16, 2018, at 07:41, Ronak Patel <ronak2121@yahoo.com> wrote:
>>>> 
>>>> 
>>>> On Aug 15, 2018, at 11:08 AM, Steven Liu <lq@chinaffmpeg.org> wrote:
>>>> 
>>>> 
>>>> 
>>>>> On Aug 15, 2018, at 09:31, Ronak <ronak2121-at-yahoo.com@ffmpeg.org> wrote:
>>>>> 
>>>>> From: "Ronak Patel" <ronak2121@ <mailto:ronakp@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@yahoo.com <mailto:ronak2121@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. 
> 
> For EVENT and LIVE it makes more sense.
> 

I’d also like to point out this problem only happens in ffmpeg, not mp4box, Apple’s fragmenter or bento4. 

And I doubt they support the use case you describe here for looking at a partial VOD manifest.

>>> 
>>>>>         }
>>>>>     }
>>>>> @@ -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@ffmpeg.org
>>>>> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>>>> 
>>>> Thanks
>>>> Steven
>>>> 
>>>> 
>>>> 
>>>> 
>>>> 
>>>> _______________________________________________
>>>> ffmpeg-devel mailing list
>>>> ffmpeg-devel@ffmpeg.org
>>>> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>> 
>> Thanks
>> Steven
>> 
>> 
>> 
>> 
>> 
>> _______________________________________________
>> ffmpeg-devel mailing list
>> ffmpeg-devel@ffmpeg.org
>> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Liu Steven Aug. 16, 2018, 12:49 a.m. UTC | #10 
> On Aug 16, 2018, at 08:39, Ronak Patel <ronak2121@yahoo.com> wrote:
> 
>> 
>> On Aug 15, 2018, at 8:21 PM, Steven Liu <lq@chinaffmpeg.org> wrote:
>> 
>> 
>> 
>>>> On Aug 16, 2018, at 07:41, Ronak Patel <ronak2121@yahoo.com> wrote:
>>>> 
>>>> 
>>>> On Aug 15, 2018, at 11:08 AM, Steven Liu <lq@chinaffmpeg.org> wrote:
>>>> 
>>>> 
>>>> 
>>>>> On Aug 15, 2018, at 09:31, Ronak <ronak2121-at-yahoo.com@ffmpeg.org> wrote:
>>>>> 
>>>>> From: "Ronak Patel" <ronak2121@ <mailto:ronakp@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@yahoo.com <mailto:ronak2121@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@ffmpeg.org
>>>>> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>>>> 
>>>> Thanks
>>>> Steven
>>>> 
>>>> 
>>>> 
>>>> 
>>>> 
>>>> _______________________________________________
>>>> ffmpeg-devel mailing list
>>>> ffmpeg-devel@ffmpeg.org
>>>> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>> 
>> Thanks
>> Steven
>> 
>> 
>> 
>> 
>> 
>> _______________________________________________
>> ffmpeg-devel mailing list
>> ffmpeg-devel@ffmpeg.org
>> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Thanks
Steven
diff mbox

Patch

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);