From patchwork Sat May 2 17:16:55 2020 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Andreas Rheinhardt X-Patchwork-Id: 19437 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 215AD44BB45 for ; Sat, 2 May 2020 20:17:56 +0300 (EEST) Received: from [127.0.1.1] (localhost [127.0.0.1]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTP id 013B468C885; Sat, 2 May 2020 20:17:56 +0300 (EEST) X-Original-To: ffmpeg-devel@ffmpeg.org Delivered-To: ffmpeg-devel@ffmpeg.org Received: from mail-wm1-f49.google.com (mail-wm1-f49.google.com [209.85.128.49]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTPS id 66DCA68C3B7 for ; Sat, 2 May 2020 20:17:48 +0300 (EEST) Received: by mail-wm1-f49.google.com with SMTP id 188so3533632wmc.2 for ; Sat, 02 May 2020 10:17:48 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=from:to:cc:subject:date:message-id:in-reply-to:references :mime-version:content-transfer-encoding; bh=k8OOf6sW4Bp1EDT/F2wCbER1+m112jILEetUFJ70PJk=; b=AQFjCKrDH4kPEf4xFi6cBE7WXPaQTgalBBDeSnHHcjAZwVgcteE8Gq3vOuxOwg4uVs ontq7b7PdtLOGHJk0Cn0A+SoSvDpsM9mECupz5RqofJzhBx/O0NN/tA3X/3MtCeNpeuD EglJoFD1jeApKhC3VCUapEvvMo1S3yMhJMXTWnW+dUG6PMqxuh89S3WaV1crsDdCi3Z3 hlTQphu7aIR9Q/HcKMkR7bP/Bdaq1mdbt9ep1kNBnQk7KsXjEqng7Q0dg8pYyBnI+Lx/ SWwSksWUv9inpj4l+buOmblbiMeJ0ZXfZViMqLmIcLdn3NeHMufPmEXDAww+6v3ng9/y cd9Q== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:from:to:cc:subject:date:message-id:in-reply-to :references:mime-version:content-transfer-encoding; bh=k8OOf6sW4Bp1EDT/F2wCbER1+m112jILEetUFJ70PJk=; b=EzYtYjOs03YIdPGYs/URbt5rcVvl42LxE8AhKzXnh3oPt5tOSFYOUEwjrx9k0g9aBC v+kjCCg/GM3mumJxaDk/urk/QJaIlttDjeK5RDHs2q9BFFMthzVvNb+/8WzlvVD0eDml HCt/Wx/wMBc7LBVvXRZv5/XEsfbfEET44r2BXhgKP3oN4LKsidVye7f2UUscWLDRvlx4 y/lKYIyi4S4LA3qw6meWkR/IxMR/8iIl75rSYDZkOLKLX5FQMyQcroq8/FY9B9EwAvff iy6JdkrZ8bq4fkP6p5RIAZLoKLQr622acsVU+/kBtkhGnkxCPeBAfUYasct+ivNV0fim Moww== X-Gm-Message-State: AGi0PuaqcmeeVm5fI4T15pbrJozqPyNBLeUY6ynmjvDWLrQTL+Q5uraN 7uTpPhNBFlShSjUZsWJiVfnFIO1J X-Google-Smtp-Source: APiQypK04FvhTeKFtV8qxbvoWnTpLndiNElZCkuX0dTNt2Y1iuoiwAQFD9PNLlgJv6nYcPAbOb2e9g== X-Received: by 2002:a7b:c0cb:: with SMTP id s11mr5908579wmh.180.1588439867403; Sat, 02 May 2020 10:17:47 -0700 (PDT) Received: from sblaptop.fritz.box (ipbcc1ab57.dynamic.kabel-deutschland.de. [188.193.171.87]) by smtp.gmail.com with ESMTPSA id u74sm5241524wmu.13.2020.05.02.10.17.46 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Sat, 02 May 2020 10:17:46 -0700 (PDT) From: Andreas Rheinhardt To: ffmpeg-devel@ffmpeg.org Date: Sat, 2 May 2020 19:16:55 +0200 Message-Id: <20200502171700.28991-3-andreas.rheinhardt@gmail.com> X-Mailer: git-send-email 2.20.1 In-Reply-To: <20200429222156.29129-1-andreas.rheinhardt@gmail.com> References: <20200429222156.29129-1-andreas.rheinhardt@gmail.com> MIME-Version: 1.0 Subject: [FFmpeg-devel] [PATCH 08/13] avformat/matroskaenc: Remove inconsistencies wrt seekability handling 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 Cc: Andreas Rheinhardt Errors-To: ffmpeg-devel-bounces@ffmpeg.org Sender: "ffmpeg-devel" The Matroska muxer behaves differently in several ways when it thinks that it is in unseekable/livestreaming mode: It does not add Cue entries because they won't be written anyway for a livestream and it writes some elements only preliminarily (with the intention to overwrite them with an updated version at the end) when non-livestreaming etc. There are two ways to set the Matroska muxer into livestreaming mode: Setting an option or by providing an unseekable AVIOContext. Yet the actual checks were not consistent: If the AVIOContext was unseekable and no AAC extradata was available when writing the header, writing the header failed; but if the AVIOContext was seekable, it didn't, because the muxer expected to get the extradata via packet side-data. Here the livestreaming option has not been checked, although one can't use the updated extradata in case it is a livestream. If the reserve_index_space option was used, space for writing Cues would be reserved when writing the header unless the AVIOContext was unseekable. Yet Cues were only written if the livestreaming option was not set and the AVIOContext was seekable (when writing the trailer), so if the AVIOContext was seekable and the livestreaming option set, the reserved space would never be used at all. If the AVIOContext was unseekable and the livestreaming option was not set, it would be attempted to update the main length field at the end. After all, it might be possible that the file is so short that it fits into the AVIOContext's buffer in which case the seek back would work. Yet this is dangerous: It might be that we are not dealing with a simple output file, but that our output gets split into chunks and that each of these chunks is actually seekable. In this case some part of the last chunk (namely the eight bytes that have the same offset as the length field had in the header) will be overwritten with what the muxer wrongly believes to be the filesize. (The livestreaming option has been added to deal with this scenario, yet its documentation ("Write files assuming it is a live stream.") doesn't make this clear at all. At least the segment muxer does not set the option for live and given that the chances of successfully seeking when the output is actually unseekable are slim, it is best to not attempt to update the length field in the unseekable case at all.) All these inconsistencies were fixed by treating the output as seekable if the livestreaming option is not set and if the AVIOContext is seekable. A macro has been used to enforce consistency and improve code readability. Signed-off-by: Andreas Rheinhardt --- I am actually not really satisfied with this: I'd really like an option that says that the output is a single file. The reason for this is that I am working on a patchset for writing attached picture streams as AttachedFile in Matroska (there are several tickets open for this), yet the corresponding packets are only available after writing the header.* My plan is to write the Attachments as soon as possible when seekable (i.e. before the first Cluster when all necessary packets already arrived; at the end otherwise) and if I knew that the output was a single file (even if unseekable) I could also write Attachments before the first Cluster. This commit will btw lead to a misleading-indentation warning. It will be fixed in the cosmetics commit. *: I have an idea to mitigate this (by using AVStream.attached_pic for muxers, too), but it won't solve this problem completely. libavformat/matroskaenc.c | 37 ++++++++++++++++++++----------------- 1 file changed, 20 insertions(+), 17 deletions(-) diff --git a/libavformat/matroskaenc.c b/libavformat/matroskaenc.c index 1a142403fa..ccbd73cbe4 100644 --- a/libavformat/matroskaenc.c +++ b/libavformat/matroskaenc.c @@ -58,6 +58,9 @@ * Info, Tracks, Chapters, Attachments, Tags and Cues */ #define MAX_SEEKHEAD_ENTRIES 6 +#define IS_SEEKABLE(pb, mkv) (((pb)->seekable & AVIO_SEEKABLE_NORMAL) && \ + !(mkv)->is_live) + enum { DEFAULT_MODE_INFER, DEFAULT_MODE_INFER_NO_SUBS, @@ -671,9 +674,9 @@ static int put_flac_codecpriv(AVFormatContext *s, AVIOContext *pb, return 0; } -static int get_aac_sample_rates(AVFormatContext *s, const uint8_t *extradata, - int extradata_size, int *sample_rate, - int *output_sample_rate) +static int get_aac_sample_rates(AVFormatContext *s, MatroskaMuxContext *mkv, + const uint8_t *extradata, int extradata_size, + int *sample_rate, int *output_sample_rate) { MPEG4AudioConfig mp4ac; int ret; @@ -684,7 +687,7 @@ static int get_aac_sample_rates(AVFormatContext *s, const uint8_t *extradata, * first packet. * Abort however if s->pb is not seekable, as we would not be able to seek back * to write the sample rate elements once the extradata shows up, anyway. */ - if (ret < 0 && (extradata_size || !(s->pb->seekable & AVIO_SEEKABLE_NORMAL))) { + if (ret < 0 && (extradata_size || !IS_SEEKABLE(s->pb, mkv))) { av_log(s, AV_LOG_ERROR, "Error parsing AAC extradata, unable to determine samplerate.\n"); return AVERROR(EINVAL); @@ -1141,8 +1144,8 @@ static int mkv_write_track(AVFormatContext *s, MatroskaMuxContext *mkv, return 0; if (par->codec_id == AV_CODEC_ID_AAC) { - ret = get_aac_sample_rates(s, par->extradata, par->extradata_size, &sample_rate, - &output_sample_rate); + ret = get_aac_sample_rates(s, mkv, par->extradata, par->extradata_size, + &sample_rate, &output_sample_rate); if (ret < 0) return ret; } @@ -1907,11 +1910,13 @@ static int mkv_write_header(AVFormatContext *s) put_ebml_void(pb, s->metadata_header_padding); } - if ((pb->seekable & AVIO_SEEKABLE_NORMAL) && mkv->reserve_cues_space) { + if (mkv->reserve_cues_space) { + if (IS_SEEKABLE(pb, mkv)) { mkv->cues_pos = avio_tell(pb); if (mkv->reserve_cues_space == 1) mkv->reserve_cues_space++; put_ebml_void(pb, mkv->reserve_cues_space); + } } av_init_packet(&mkv->cur_audio_pkt); @@ -1920,7 +1925,7 @@ static int mkv_write_header(AVFormatContext *s) // start a new cluster every 5 MB or 5 sec, or 32k / 1 sec for streaming or // after 4k and on a keyframe - if (pb->seekable & AVIO_SEEKABLE_NORMAL) { + if (IS_SEEKABLE(pb, mkv)) { if (mkv->cluster_time_limit < 0) mkv->cluster_time_limit = 5000; if (mkv->cluster_size_limit < 0) @@ -2188,8 +2193,8 @@ static int mkv_check_new_extra_data(AVFormatContext *s, const AVPacket *pkt) case AV_CODEC_ID_AAC: if (side_data_size && mkv->track.bc) { int filler, output_sample_rate = 0; - ret = get_aac_sample_rates(s, side_data, side_data_size, &track->sample_rate, - &output_sample_rate); + ret = get_aac_sample_rates(s, mkv, side_data, side_data_size, + &track->sample_rate, &output_sample_rate); if (ret < 0) return ret; if (!output_sample_rate) @@ -2311,7 +2316,7 @@ static int mkv_write_packet_internal(AVFormatContext *s, const AVPacket *pkt) ret = mkv_write_block(s, pb, MATROSKA_ID_SIMPLEBLOCK, pkt, keyframe); if (ret < 0) return ret; - if ((s->pb->seekable & AVIO_SEEKABLE_NORMAL) && keyframe && + if (keyframe && IS_SEEKABLE(s->pb, mkv) && (par->codec_type == AVMEDIA_TYPE_VIDEO || !mkv->have_video && !track->has_cue)) { ret = mkv_add_cuepoint(mkv, pkt->stream_index, ts, mkv->cluster_pos, relative_packet_pos, -1); @@ -2341,7 +2346,7 @@ FF_ENABLE_DEPRECATION_WARNINGS end_ebml_master(pb, blockgroup); } - if (s->pb->seekable & AVIO_SEEKABLE_NORMAL) { + if (IS_SEEKABLE(s->pb, mkv)) { ret = mkv_add_cuepoint(mkv, pkt->stream_index, ts, mkv->cluster_pos, relative_packet_pos, duration); if (ret < 0) @@ -2451,6 +2456,7 @@ static int mkv_write_trailer(AVFormatContext *s) { MatroskaMuxContext *mkv = s->priv_data; AVIOContext *pb = s->pb; + int64_t endpos, ret64; int ret; // check if we have an audio packet cached @@ -2474,9 +2480,8 @@ static int mkv_write_trailer(AVFormatContext *s) if (ret < 0) return ret; - - if ((pb->seekable & AVIO_SEEKABLE_NORMAL) && !mkv->is_live) { - int64_t endpos, ret64; + if (!IS_SEEKABLE(pb, mkv)) + return 0; endpos = avio_tell(pb); @@ -2592,9 +2597,7 @@ static int mkv_write_trailer(AVFormatContext *s) } avio_seek(pb, endpos, SEEK_SET); - } - if (!mkv->is_live) end_ebml_master(pb, mkv->segment); return mkv->reserve_cues_space < 0 ? AVERROR(EINVAL) : 0;