[FFmpeg-devel] Fix usage of temp_file flag in hls_flags option - 2nd attempt

Submitted by Adrian on Dec. 17, 2018, 10:30 p.m.

Details

Message ID 99b26aa2-f641-6a91-32b7-3c68b88859b4@o2.pl
State New
Headers show

Commit Message

Adrian Dec. 17, 2018, 10:30 p.m.
Thanks for the explanation, now your intent is clear. I agree that 
keeping playlist file always as temp as long as it's a file protocol 
makes sense and won't do harm, though I'm still curious why original 
change to accept HLS_TEMP_FILE flag was sent to include and was included 
in official release, I believe there must be a reason.

Given that, I've prepared another patch with fix for original problem + 
proper file picked up for computing 'use_temp_file' variable value. I 
don't know what is the exact flow here, so I'll send it as another 
patch, previous can be discarded and hopefully this one won't raise any 
more objections, though I'd still appreciate a review.

Thanks for your input Aleksey!

Regards
Adrian Guzowski

W dniu 17.12.2018 o 22:35, Aleksey Skripka pisze:
> Evening!
>
> First of all, about playlist writeout:
> before 223d2bde22ce33dcbcb6f17f234b609cb98f1fb6 - playlist was always(!) creating via .tmp file.
> after 223d2bde22ce33dcbcb6f17f234b609cb98f1fb6 - playlists .tmp logic become dependent on +temp_file flag.
>
> I suggest to return to original logic.
> Non-atomic playlist writeout - is a disaster to just any hls player. This way we will force everyone (except offline VOD) to start using +temp_file. Always!
> Also, if somebody relying on this - they will get broken streams just after upgrade (It seems to me, that it can be a lot).
>
> So, here maintainer should decide how to be.
>
>
> and about single_file:
> if it will be decided to stay with current way (playlist's .tmp logic controlled by +temp_file flag), you are right - temp_file+single_file case become unreal.
> if it will be decided to return original behaviour (playlist always via .tmp), someone can choose how to writeout media file (despite i understand temp+single is very-very rare case).
>
> Thanks!

Comments

Aleksey Skripka Dec. 17, 2018, 11:58 p.m.
Thanks for your attention, Adrian!

just one moment,
in hls_write_packet():

instead:
+        if (use_temp_file) {
+            hls_rename_temp_file(s, oc);

should be:
+        if (use_temp_file && !(hls->flags & HLS_SINGLE_FILE))
+            hls_rename_temp_file(s, oc);

or we a killing 'single' filename in few iterations.



Steven,
i recall my version of patch. Adrian's version fixes more issues and hope v3 will be the best :)


Aleksey Skripka

> On 18 Dec 2018, at 01:30, Adrian <adrian-007@o2.pl> wrote:
> 
> Thanks for the explanation, now your intent is clear. I agree that keeping playlist file always as temp as long as it's a file protocol makes sense and won't do harm, though I'm still curious why original change to accept HLS_TEMP_FILE flag was sent to include and was included in official release, I believe there must be a reason.
> 
> Given that, I've prepared another patch with fix for original problem + proper file picked up for computing 'use_temp_file' variable value. I don't know what is the exact flow here, so I'll send it as another patch, previous can be discarded and hopefully this one won't raise any more objections, though I'd still appreciate a review.
> 
> Thanks for your input Aleksey!
> 
> Regards
> Adrian Guzowski
> 
> W dniu 17.12.2018 o 22:35, Aleksey Skripka pisze:
>> Evening!
>> 
>> First of all, about playlist writeout:
>> before 223d2bde22ce33dcbcb6f17f234b609cb98f1fb6 - playlist was always(!) creating via .tmp file.
>> after 223d2bde22ce33dcbcb6f17f234b609cb98f1fb6 - playlists .tmp logic become dependent on +temp_file flag.
>> 
>> I suggest to return to original logic.
>> Non-atomic playlist writeout - is a disaster to just any hls player. This way we will force everyone (except offline VOD) to start using +temp_file. Always!
>> Also, if somebody relying on this - they will get broken streams just after upgrade (It seems to me, that it can be a lot).
>> 
>> So, here maintainer should decide how to be.
>> 
>> 
>> and about single_file:
>> if it will be decided to stay with current way (playlist's .tmp logic controlled by +temp_file flag), you are right - temp_file+single_file case become unreal.
>> if it will be decided to return original behaviour (playlist always via .tmp), someone can choose how to writeout media file (despite i understand temp+single is very-very rare case).
>> 
>> Thanks!
> <0001-Fix-usage-of-temp_file-flag-in-hls_flags-option-2nd-attempt.patch>_______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Patch hide | download patch | download mbox

From 3976dac67e65e8065f9150528a0b6fa62100f0c0 Mon Sep 17 00:00:00 2001
From: Adrian Guzowski <adrian-007@o2.pl>
Date: Mon, 17 Dec 2018 23:14:53 +0100
Subject: [PATCH] Fix usage of temp_file flag in hls_flags option.

This is a regression introduced by 223d2bde22ce33dcbcb6f17f234b609cb98f1fb6.
It appears that regression was introduced in 4.1, 4.0.x does not share
this behaviour.

Temp files were not created for MPEG-TS segments options - HLS_TEMP_FILE
flag was never set on AVFormatContext, it is however set on HLSContext object.
In order to fix this issue, proper flags field must be checked. In addition,
renaming code was messed up and apparently was working only for MP4 files.

NOTE: This patch is a remake of an original proposal with changes from mailing
list suggestions about keeping playlist always as temp file and stick
to original logic of single file concept.
---
 libavformat/hlsenc.c | 59 ++++++++++++++++++++++++--------------------
 1 file changed, 32 insertions(+), 27 deletions(-)

diff --git a/libavformat/hlsenc.c b/libavformat/hlsenc.c
index bdd2a113bd..52ba4c65f1 100644
--- a/libavformat/hlsenc.c
+++ b/libavformat/hlsenc.c
@@ -1357,8 +1357,8 @@  static int hls_window(AVFormatContext *s, int last, VariantStream *vs)
     int ret = 0;
     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_temp_file = proto && !strcmp(proto, "file") && (s->flags & HLS_TEMP_FILE);
+    const char *proto = avio_find_protocol_name(vs->m3u8_name);
+    int use_temp_file = proto && !strcmp(proto, "file");
     static unsigned warned_non_file;
     char *key_uri = NULL;
     char *iv_string = NULL;
@@ -1381,7 +1381,7 @@  static int hls_window(AVFormatContext *s, int last, VariantStream *vs)
         hls->version = 7;
     }
 
-    if (!use_temp_file && !warned_non_file++)
+    if (!use_temp_file && (hls->flags & HLS_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);
@@ -1477,8 +1477,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);
+    const char *proto = NULL;
+    int use_temp_file = 0;
     char *filename, iv_string[KEYSIZE*2 + 1];
     int err = 0;
 
@@ -1574,6 +1574,9 @@  static int hls_start(AVFormatContext *s, VariantStream *vs)
 
     set_http_options(s, &options, c);
 
+    proto = avio_find_protocol_name(oc->url);
+    use_temp_file = proto && !strcmp(proto, "file") && (c->flags & HLS_TEMP_FILE);
+
     if (use_temp_file) {
         char *new_name = av_asprintf("%s.tmp", oc->url);
         if (!new_name)
@@ -2142,8 +2145,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);
+    const char *proto = NULL;
+    int use_temp_file = 0;
     uint8_t *buffer = NULL;
     VariantStream *vs = NULL;
     AVDictionary *options = NULL;
@@ -2254,19 +2257,22 @@  static int hls_write_packet(AVFormatContext *s, AVPacket *pkt)
             }
         }
 
+        if (oc->url[0]) {
+            proto = avio_find_protocol_name(oc->url);
+            use_temp_file = proto && !strcmp(proto, "file") && (hls->flags & HLS_TEMP_FILE);
+        }
+
         // look to rename the asset name
-        if (use_temp_file && oc->url[0]) {
+        if (use_temp_file) {
             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) {
+                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 (hls->segment_type == SEGMENT_TYPE_FMP4) {
             if (hls->flags & HLS_SINGLE_FILE) {
                 ret = flush_dynbuf(vs, &range_length);
                 if (ret < 0) {
-                    av_free(old_filename);
                     return ret;
                 }
                 vs->size = range_length;
@@ -2284,20 +2290,13 @@  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);
-                    }
-                }
             }
         }
 
+        if (use_temp_file) {
+            hls_rename_temp_file(s, oc);
+        }
+
         old_filename = av_strdup(vs->avf->url);
         if (!old_filename) {
             return AVERROR(ENOMEM);
@@ -2366,8 +2365,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);
+    const char *proto = NULL;
+    int use_temp_file = 0;
     int i;
     int ret = 0;
     VariantStream *vs = NULL;
@@ -2378,6 +2377,7 @@  static int hls_write_trailer(struct AVFormatContext *s)
         oc = vs->avf;
         vtt_oc = vs->vtt_avf;
         old_filename = av_strdup(vs->avf->url);
+        use_temp_file = 0;
 
         if (!old_filename) {
             return AVERROR(ENOMEM);
@@ -2421,15 +2421,20 @@  static int hls_write_trailer(struct AVFormatContext *s)
 
 failed:
         av_write_trailer(oc);
+
+        if (oc->url[0]) {
+            proto = avio_find_protocol_name(oc->url);
+            use_temp_file = proto && !strcmp(proto, "file") && (hls->flags & HLS_TEMP_FILE);
+        }
+
         if (oc->pb) {
             if (hls->segment_type != SEGMENT_TYPE_FMP4) {
                 vs->size = avio_tell(vs->avf->pb) - vs->start_pos;
-            }
-            if (hls->segment_type != SEGMENT_TYPE_FMP4)
                 ff_format_io_close(s, &oc->pb);
+            }
 
             // rename that segment from .tmp to the real one
-            if (use_temp_file && oc->url[0] && !(hls->flags & HLS_SINGLE_FILE)) {
+            if (use_temp_file && !(hls->flags & HLS_SINGLE_FILE)) {
                 hls_rename_temp_file(s, oc);
                 av_free(old_filename);
                 old_filename = av_strdup(vs->avf->url);
-- 
2.19.2