From patchwork Mon Dec 17 22:30:03 2018 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: Adrian X-Patchwork-Id: 11452 Return-Path: X-Original-To: patchwork@ffaux-bg.ffmpeg.org Delivered-To: patchwork@ffaux-bg.ffmpeg.org Received: from ffbox0-bg.mplayerhq.hu (ffbox0-bg.ffmpeg.org [79.124.17.100]) by ffaux.localdomain (Postfix) with ESMTP id 7978444C601 for ; Tue, 18 Dec 2018 00:36:49 +0200 (EET) Received: from [127.0.1.1] (localhost [127.0.0.1]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTP id A0A4868AC44; Tue, 18 Dec 2018 00:36:49 +0200 (EET) X-Original-To: ffmpeg-devel@ffmpeg.org Delivered-To: ffmpeg-devel@ffmpeg.org Received: from mx-out.tlen.pl (mx-out.tlen.pl [193.222.135.148]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTPS id D94D268AC23 for ; Tue, 18 Dec 2018 00:36:43 +0200 (EET) Received: (wp-smtpd smtp.tlen.pl 4898 invoked from network); 17 Dec 2018 23:30:04 +0100 Received: from afca37.neoplus.adsl.tpnet.pl (HELO [192.168.1.44]) (adrian-007@o2.pl@[95.49.52.37]) (envelope-sender ) by smtp.tlen.pl (WP-SMTPD) with ECDHE-RSA-AES256-GCM-SHA384 encrypted SMTP for ; 17 Dec 2018 23:30:04 +0100 To: ffmpeg-devel@ffmpeg.org References: <5C9CCED4-90A1-40B8-A0C4-6BD6A309B4CE@undev.ru> <694f8d51-1648-d04c-c16c-58b63d11309b@o2.pl> <71ac30aa-0ac1-74e9-1832-5b8a53a6fd5a@o2.pl> From: Adrian Message-ID: <99b26aa2-f641-6a91-32b7-3c68b88859b4@o2.pl> Date: Mon, 17 Dec 2018 23:30:03 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.3.1 MIME-Version: 1.0 In-Reply-To: Content-Language: en-US X-WP-MailID: 279ea5899df493fa8e99d027b788e2b8 X-WP-AV: skaner antywirusowy Poczty o2 X-WP-SPAM: NO 000000A [QSME] Subject: [FFmpeg-devel] [PATCH] Fix usage of temp_file flag in hls_flags option - 2nd attempt X-BeenThere: ffmpeg-devel@ffmpeg.org X-Mailman-Version: 2.1.20 Precedence: list List-Id: FFmpeg development discussions and patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Reply-To: FFmpeg development discussions and patches Errors-To: ffmpeg-devel-bounces@ffmpeg.org Sender: "ffmpeg-devel" 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! From 3976dac67e65e8065f9150528a0b6fa62100f0c0 Mon Sep 17 00:00:00 2001 From: Adrian Guzowski 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