From patchwork Tue Aug 13 02:47:25 2019 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Andreas Rheinhardt X-Patchwork-Id: 14470 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 96A5E44A275 for ; Tue, 13 Aug 2019 05:56:22 +0300 (EEST) Received: from [127.0.1.1] (localhost [127.0.0.1]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTP id 7E37668AB8A; Tue, 13 Aug 2019 05:56:22 +0300 (EEST) X-Original-To: ffmpeg-devel@ffmpeg.org Delivered-To: ffmpeg-devel@ffmpeg.org Received: from mail-wm1-f67.google.com (mail-wm1-f67.google.com [209.85.128.67]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTPS id 74A2868AB40 for ; Tue, 13 Aug 2019 05:56:16 +0300 (EEST) Received: by mail-wm1-f67.google.com with SMTP id 10so73378wmp.3 for ; Mon, 12 Aug 2019 19:56:16 -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=31L1F6JL9VLv0+n8e/gEW9jVVtyQ7McAAdZ11IdOqrI=; b=Sq/eWVIc10CdMWs1ySA3aC2mo5U72jUxutIMN8V1anB/ReTwV9Lt5lQbGrkY5V0oVo oGNxEL0yEsuXHlx7GLcCQ9lMROGmccRflqJvq3u8YOkIUoVHCSI3nNATdMvbvQ1K7wXn 6v+JEEPzi1AdRRmjUJ9VonuH+Oc2hR4YRlV4zxA8/5t2JvaBkSlhj+Z4H59hxHZEcHV1 vds4eo2GQOQNEyzrJ1b80s1z+EDOvNX5pWFrDvvdFWBdnhUTqETsBoQZ3Umg/vIQwOsb aMaYgTqq9/TMPI60wOCxvXAnYbVHuA3DNfQbeAvT78S0U8rdXvdJwZ6+yfOvbEIxzJaI srLQ== 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=31L1F6JL9VLv0+n8e/gEW9jVVtyQ7McAAdZ11IdOqrI=; b=WzIkJ/1QodF4VV99mVgRFXc92Rq3Ts7fP5X9ElgAJ7ApsfQXlw2tc22fOlphTUHma/ +UtjI0ALWeMVN8ZFm4NowcrwvNgqW2axOKfSUVl8KU9PXugnFScuGfjGcdLVrZZsAYA+ Ah/m3Baaj7EeurbrFoH22Qd+uTSyZsRlKYerpdCFWWwdUKE0uzOR0nCeVvWteMEvmlHQ AU0U2WUcS0ffeOOb828TwRYFTVF57cy8hnLu4mtYJ6bNLZQJjk6oubTNksU1XkmvE14k /fxO+VENodgAYxNy4HlRl9S9H/gLzZrW3EDly0zJ23Jg0ZBU+8bVX8l3utmlES/JUUMv mkRw== X-Gm-Message-State: APjAAAWfZEXE/3cI0nEUYt7TTPTHNhNm/W5Pi6yoRYiPKd5V4eThNtLW r229tXW83Cd+a0aNDbYtsJIAkD+A X-Google-Smtp-Source: APXvYqxqQDEA4ZqjZxKROAJI9YiYsaxhxSNHIbLo1/zbTakhUut4SoorJN06BIcQuRewdS2Dd6iIcQ== X-Received: by 2002:a1c:f918:: with SMTP id x24mr166823wmh.132.1565664532581; Mon, 12 Aug 2019 19:48:52 -0700 (PDT) Received: from localhost.localdomain (ipbcc06ceb.dynamic.kabel-deutschland.de. [188.192.108.235]) by smtp.gmail.com with ESMTPSA id k13sm25020613wro.97.2019.08.12.19.48.51 (version=TLS1_3 cipher=AEAD-AES256-GCM-SHA384 bits=256/256); Mon, 12 Aug 2019 19:48:52 -0700 (PDT) From: Andreas Rheinhardt To: ffmpeg-devel@ffmpeg.org Date: Tue, 13 Aug 2019 04:47:25 +0200 Message-Id: <20190813024726.6596-12-andreas.rheinhardt@gmail.com> X-Mailer: git-send-email 2.21.0 In-Reply-To: <20190813024726.6596-1-andreas.rheinhardt@gmail.com> References: <20190813024726.6596-1-andreas.rheinhardt@gmail.com> MIME-Version: 1.0 Subject: [FFmpeg-devel] [PATCH 12/13] avformat/mux: Don't modify packets we don't own 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 documentation of av_write_frame explicitly states that the function does not take ownership of the packets sent to it; while av_write_frame does not directly unreference the packets after having written them, it nevertheless modifies the packet in various ways: 1. The timestamps might be modified either by prepare_input_packet or compute_muxer_pkt_fields. 2. If a bitstream filter gets applied, it takes ownership of the reference and the side-data in the packet sent to it in av_bsf_send_packet. In case of do_packet_auto_bsf, the end result is that the returned packet contains the output of the last bsf in the chain. If an error happens, an empty packet will be returned; a packet may also simply not lead to any output (vp9_superframe). This also implies that side data needs to be really copied and can't be shared with the input packet. The method choosen here minimizes copying of data: When the input isn't refcounted and no bitstream filter is applied, the packet's data will not be copied. Signed-off-by: Andreas Rheinhardt --- I am not sure about this one. If one understands ownership of a packet as "the right to modify a packet as well as the obligation to ultimately free the data" (in case the data is shared, one of course needs to make it writable before modifying it), then it is obvious that av_write_frame needs to preserve the original packet given to it; that was my understanding when writing this patch. But Hendrik has said that "ownership" actually only comes with the obligation to free the data and that av_write_frame does not taking ownership of the packet does not imply that the caller may expect the packet to be returned unmodified. If the latter is correct, then this patch is moot, of course. libavformat/mux.c | 31 ++++++++++++++++++++++++++----- 1 file changed, 26 insertions(+), 5 deletions(-) diff --git a/libavformat/mux.c b/libavformat/mux.c index 55fa0a184c..2dc7fc2dcb 100644 --- a/libavformat/mux.c +++ b/libavformat/mux.c @@ -872,11 +872,12 @@ static int do_packet_auto_bsf(AVFormatContext *s, AVPacket *pkt) { return 1; } -int av_write_frame(AVFormatContext *s, AVPacket *pkt) +int av_write_frame(AVFormatContext *s, AVPacket *in) { + AVPacket local_pkt, *pkt = &local_pkt; int ret; - if (!pkt) { + if (!in) { if (s->oformat->flags & AVFMT_ALLOW_FLUSH) { ret = s->oformat->write_packet(s, NULL); flush_if_needed(s); @@ -887,19 +888,36 @@ int av_write_frame(AVFormatContext *s, AVPacket *pkt) return 1; } + /* We don't own in, so we have to make sure not to modify it. + * The following avoids copying in's data unnecessarily. + * Copying side data is unavoidable as a bitstream filter + * may change it, e.g. free it on errors. */ + pkt->data = in->data; + pkt->size = in->size; + if (in->buf) { + pkt->buf = av_buffer_ref(in->buf); + if (!pkt->buf) + return AVERROR(ENOMEM); + } else { + pkt->buf = NULL; + } + ret = av_packet_copy_props(pkt, in); + if (ret < 0) + goto fail; + ret = prepare_input_packet(s, pkt); if (ret < 0) - return ret; + goto fail; ret = do_packet_auto_bsf(s, pkt); if (ret <= 0) - return ret; + goto fail; #if FF_API_COMPUTE_PKT_FIELDS2 && FF_API_LAVF_AVCTX ret = compute_muxer_pkt_fields(s, s->streams[pkt->stream_index], pkt); if (ret < 0 && !(s->oformat->flags & AVFMT_NOTIMESTAMPS)) - return ret; + goto fail; #endif ret = write_packet(s, pkt); @@ -908,6 +926,9 @@ int av_write_frame(AVFormatContext *s, AVPacket *pkt) if (ret >= 0) s->streams[pkt->stream_index]->nb_frames++; + +fail: + av_packet_unref(pkt); return ret; }