From patchwork Wed Jan 22 19:27:16 2020 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Andreas Rheinhardt X-Patchwork-Id: 17474 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 A6F8544B208 for ; Wed, 22 Jan 2020 21:27:44 +0200 (EET) Received: from [127.0.1.1] (localhost [127.0.0.1]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTP id 8E86268B433; Wed, 22 Jan 2020 21:27:44 +0200 (EET) X-Original-To: ffmpeg-devel@ffmpeg.org Delivered-To: ffmpeg-devel@ffmpeg.org Received: from mail-wr1-f68.google.com (mail-wr1-f68.google.com [209.85.221.68]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTPS id 0575868089B for ; Wed, 22 Jan 2020 21:27:38 +0200 (EET) Received: by mail-wr1-f68.google.com with SMTP id g17so360736wro.2 for ; Wed, 22 Jan 2020 11:27:37 -0800 (PST) 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=Pd0poDYGjGyGeMZs1XilFdKSgmzEsSuPuf8aP/R4/+s=; b=I5HwavO6FwIoZLciRyafJI/8Us7lJS925+7L5QLQN4jltadaZXspm1xZvLIIEG1ZeW oC5fNWNoHC3M86ETZ83smu1DfJszV4jOX1OXfyZJIK5LdUcFNowXkPqzmo5Zq2fhrAWB bh6Dwc1QM9IG0TKUDYvdM3l8wfvLx3LtzfAdu5P9C9JmiCudZzftc+2L4w+rwmmFY62G XeERU6iu0W7aWWScml5tMPVLdgD8Zd1waYgMdKA4gRqgHoFud2vOJ0JSwXUO7sCPQOlX qypSlTIGp6Nr/bCVO9FrxCWtOTaf0dFPkEo+khx+WBszr4Nke2LRpa5ydbxVf5EIZTDL Awag== 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=Pd0poDYGjGyGeMZs1XilFdKSgmzEsSuPuf8aP/R4/+s=; b=ZHI/dywkbQU7v5qYwTvFsNBdGiIiI/Z2irS/2rBu3dzatz9YklLP0t0eOWX8v6EuxJ D4lG1oFR07Dtnq7n4Lwnak0AVieH0siLBHw90eXdTJoA5cVbURSIDNjRn6Rem4aYJ09j BLrDFsf3KGHrooWrWWm/6AJeCGhAd5rhMvXaKwLHTwRkrWoTtDLfs669Zmu5fFxaQKbh u0PxFJ0JmBDIHx93z6/hF+SSofDiQ/sukLKq3IPTnxajZFbrHzGXjjbV81D+dHa8ShZ7 EFbsR8Zf+fyiPfVSNtHZUnHxV+DyVcyNacQ9EIRMyPdMLnoB+2I8JQJp91r0Ld0d23X+ eUpA== X-Gm-Message-State: APjAAAVMQ8S/6AWerO/Qy25XYIxH4H1U7INwTGF82YEa9ozfCnHugRgG LlFYF6oY7wcJfTv5woD3CmAQ139z X-Google-Smtp-Source: APXvYqy/8Vnt7jSWqkCocqFTG2QjSnBNmuraQpfvtFmGbDKHAB8Ra422BK4YW77wj4UsIrwdhmz/vw== X-Received: by 2002:adf:e550:: with SMTP id z16mr12578397wrm.315.1579721257248; Wed, 22 Jan 2020 11:27:37 -0800 (PST) Received: from sblaptop.fritz.box (ipbcc08bbf.dynamic.kabel-deutschland.de. [188.192.139.191]) by smtp.gmail.com with ESMTPSA id v3sm58196792wru.32.2020.01.22.11.27.36 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 22 Jan 2020 11:27:36 -0800 (PST) From: Andreas Rheinhardt To: ffmpeg-devel@ffmpeg.org Date: Wed, 22 Jan 2020 20:27:16 +0100 Message-Id: <20200122192717.11678-2-andreas.rheinhardt@gmail.com> X-Mailer: git-send-email 2.20.1 In-Reply-To: <20200101005837.11356-1-andreas.rheinhardt@gmail.com> References: <20200101005837.11356-1-andreas.rheinhardt@gmail.com> MIME-Version: 1.0 Subject: [FFmpeg-devel] [PATCH 27/28] avformat/matroskaenc: Remove unnecessary avio_tell(), avio_seek() 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" avio_close_dyn_buf() has a bug: When the write pointer does not point to the end of the written data when calling it (i.e. when one has performed a seek back to update already written data), it would not add padding to the end of the buffer, but to the current position, overwriting other data; furthermore the reported size would be wrong (off by the amount of data it has overwritten with padding). In order not to run into this when updating already written elements or elements for which size has only been reserved, the Matroska muxer would first record the current position of the dynamic buffer, then seek to the desired position, perform the update and seek back to the earlier position. But now that end_ebml_master_crc32() does not make use of avio_close_dyn_buf() any more, this is no longer necessary. Signed-off-by: Andreas Rheinhardt --- avio_close_dyn_buf() even has more bugs: Besides the design flaw of freeing a resource without setting the pointer to it to NULL, it returns a size of -AV_INPUT_BUFFER_PADDING_SIZE if a memory allocation failure happened (but not if the arbitrary limit of INT_MAX/2 has been surpassed); and this despite its documentation not allowing returning negative values at all. This will definitely need to be fixed (and this muxer will need to check for whether the allocations failed). libavformat/matroskaenc.c | 11 ----------- 1 file changed, 11 deletions(-) diff --git a/libavformat/matroskaenc.c b/libavformat/matroskaenc.c index 8bbacf5cd3..354096266d 100644 --- a/libavformat/matroskaenc.c +++ b/libavformat/matroskaenc.c @@ -2211,7 +2211,6 @@ static int mkv_check_new_extra_data(AVFormatContext *s, AVPacket *pkt) case AV_CODEC_ID_AAC: if (side_data_size && (s->pb->seekable & AVIO_SEEKABLE_NORMAL) && !mkv->is_live) { int filler, output_sample_rate = 0; - int64_t curpos; ret = get_aac_sample_rates(s, side_data, side_data_size, &track->sample_rate, &output_sample_rate); if (ret < 0) @@ -2222,7 +2221,6 @@ static int mkv_check_new_extra_data(AVFormatContext *s, AVPacket *pkt) if (ret < 0) return ret; memcpy(par->extradata, side_data, side_data_size); - curpos = avio_tell(mkv->tracks_bc); avio_seek(mkv->tracks_bc, track->codecpriv_offset, SEEK_SET); mkv_write_codecprivate(s, mkv->tracks_bc, par, 1, 0); filler = MAX_PCE_SIZE + 2 + 4 - (avio_tell(mkv->tracks_bc) - track->codecpriv_offset); @@ -2231,7 +2229,6 @@ static int mkv_check_new_extra_data(AVFormatContext *s, AVPacket *pkt) avio_seek(mkv->tracks_bc, track->sample_rate_offset, SEEK_SET); put_ebml_float(mkv->tracks_bc, MATROSKA_ID_AUDIOSAMPLINGFREQ, track->sample_rate); put_ebml_float(mkv->tracks_bc, MATROSKA_ID_AUDIOOUTSAMPLINGFREQ, output_sample_rate); - avio_seek(mkv->tracks_bc, curpos, SEEK_SET); } else if (!par->extradata_size && !track->sample_rate) { // No extradata (codecpar or packet side data). av_log(s, AV_LOG_ERROR, "Error parsing AAC extradata, unable to determine samplerate.\n"); @@ -2241,7 +2238,6 @@ static int mkv_check_new_extra_data(AVFormatContext *s, AVPacket *pkt) case AV_CODEC_ID_FLAC: if (side_data_size && (s->pb->seekable & AVIO_SEEKABLE_NORMAL) && !mkv->is_live) { AVCodecParameters *codecpriv_par; - int64_t curpos; if (side_data_size != par->extradata_size) { av_log(s, AV_LOG_ERROR, "Invalid FLAC STREAMINFO metadata for output stream %d\n", pkt->stream_index); @@ -2256,10 +2252,8 @@ static int mkv_check_new_extra_data(AVFormatContext *s, AVPacket *pkt) return ret; } memcpy(codecpriv_par->extradata, side_data, side_data_size); - curpos = avio_tell(mkv->tracks_bc); avio_seek(mkv->tracks_bc, track->codecpriv_offset, SEEK_SET); mkv_write_codecprivate(s, mkv->tracks_bc, codecpriv_par, 1, 0); - avio_seek(mkv->tracks_bc, curpos, SEEK_SET); avcodec_parameters_free(&codecpriv_par); } break; @@ -2271,7 +2265,6 @@ static int mkv_check_new_extra_data(AVFormatContext *s, AVPacket *pkt) AVIOContext *dyn_cp; uint8_t *codecpriv; int codecpriv_size; - int64_t curpos; ret = avio_open_dyn_buf(&dyn_cp); if (ret < 0) return ret; @@ -2281,12 +2274,10 @@ static int mkv_check_new_extra_data(AVFormatContext *s, AVPacket *pkt) av_free(codecpriv); return AVERROR_INVALIDDATA; } - curpos = avio_tell(mkv->tracks_bc); avio_seek(mkv->tracks_bc, track->codecpriv_offset, SEEK_SET); // Do not write the OBUs as we don't have space saved for them put_ebml_binary(mkv->tracks_bc, MATROSKA_ID_CODECPRIVATE, codecpriv, 4); av_free(codecpriv); - avio_seek(mkv->tracks_bc, curpos, SEEK_SET); ret = ff_alloc_extradata(par, side_data_size); if (ret < 0) return ret; @@ -2578,7 +2569,6 @@ static int mkv_write_trailer(AVFormatContext *s) // update stream durations if (!mkv->is_live) { int i; - int64_t curr = avio_tell(mkv->tags_bc); for (i = 0; i < s->nb_streams; ++i) { AVStream *st = s->streams[i]; mkv_track *track = &mkv->tracks[i]; @@ -2599,7 +2589,6 @@ static int mkv_write_trailer(AVFormatContext *s) put_ebml_binary(mkv->tags_bc, MATROSKA_ID_TAGSTRING, duration_string, 20); } } - avio_seek(mkv->tags_bc, curr, SEEK_SET); } if (mkv->tags_bc && !mkv->is_live) { avio_seek(pb, mkv->tags_pos, SEEK_SET);