From patchwork Sat Apr 11 21:19:50 2020 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Andreas Rheinhardt X-Patchwork-Id: 18875 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 766B444A8DB for ; Sun, 12 Apr 2020 00:20:16 +0300 (EEST) Received: from [127.0.1.1] (localhost [127.0.0.1]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTP id 5A1E368B816; Sun, 12 Apr 2020 00:20:16 +0300 (EEST) X-Original-To: ffmpeg-devel@ffmpeg.org Delivered-To: ffmpeg-devel@ffmpeg.org Received: from mail-wm1-f68.google.com (mail-wm1-f68.google.com [209.85.128.68]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTPS id 60C9168B0BE for ; Sun, 12 Apr 2020 00:20:10 +0300 (EEST) Received: by mail-wm1-f68.google.com with SMTP id z6so6169135wml.2 for ; Sat, 11 Apr 2020 14:20:10 -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:mime-version :content-transfer-encoding; bh=oTSwzZoTK2ViCcqkq1Qbq96M4z5KzqMJKMR0O2WdV18=; b=h3FXW3bv+SjRH4BVMKE+uZVJMUbetUzoqMc6PpXnS/P72KMyRI2YupUr4ZHjUdXy3K ppMCzPKkpCO6fvc/1cruHkAaoRJROkPd25v+wkrT55aIQyD0OptiAJOxJJCv6YOJixsO 6aRun16G4GMb9DIbcTors2RsqXJCUjT+r3Lt843tows3ywkKNCaX5jBLQDO4kGu6xt83 BPy7LDdRgf9h0vH/hGZcXeAzEn9BRoN3dykU9HVbcEzRiMFzYNWkmKo1lWAlpeqgRNBn 1VtL5hNhI8tQXS+MKc+7ju+c4pGvs/a9zHLo22Lh3c+27PGMSkCkQ7jwDzpfEsEcBEmw 2bQA== 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:mime-version :content-transfer-encoding; bh=oTSwzZoTK2ViCcqkq1Qbq96M4z5KzqMJKMR0O2WdV18=; b=Fkz2j9sbM3Cn4MFx9jaexpeYLlXl4ZoXu596UjKI0XGlXrVL51rso6lqfq09BQDSe5 lYDbGJimjs9EE1klr0ULI2nrrtIFS2gUtoFz9NpuXlUZQyEDnXvWI7qBvSruonmYGe+P F96LElEeyRxLBkDD87CmJKdiYodYC68C3ioR8YxXshdLc9qPKPa4UP3jSAnmOK0T+ctE R1c/eTtgSQRuQEi6w2DQGTEGjJEuuH96PpIEe8pjFMn0ORglnst7DMCq/XYGdS4dvIu6 XSoUlmUF2b+iAQuEz9KUJ0HBSMqVkUR+YR1lM1qRzXj1UKR7/KmXQeZqOYAbiA5ZfCOa 4SRQ== X-Gm-Message-State: AGi0PuZFxme39VyQA/TvnmpD+S0U4tU5YfgJuU0POOzIhSfVhe+xUjGB igel+lfsRvOO8JCBcaQdpz1Ab3iw X-Google-Smtp-Source: APiQypKDei7mIrKXTII26f0HnjXRLHd9+QDnT2OV2rn5wv0R8QGcap7c6gpq5L/OvSP60tdbH/kzOg== X-Received: by 2002:a1c:23d6:: with SMTP id j205mr11229128wmj.22.1586640009400; Sat, 11 Apr 2020 14:20:09 -0700 (PDT) Received: from sblaptop.fritz.box (ipbcc1ab57.dynamic.kabel-deutschland.de. [188.193.171.87]) by smtp.gmail.com with ESMTPSA id h2sm7820650wmb.16.2020.04.11.14.20.08 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Sat, 11 Apr 2020 14:20:08 -0700 (PDT) From: Andreas Rheinhardt To: ffmpeg-devel@ffmpeg.org Date: Sat, 11 Apr 2020 23:19:50 +0200 Message-Id: <20200411211955.20843-1-andreas.rheinhardt@gmail.com> X-Mailer: git-send-email 2.20.1 MIME-Version: 1.0 Subject: [FFmpeg-devel] [PATCH 1/6] avformat/mux: Make uncoded frames av_packet_unref() compatible 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" Currently uncoded frames (i.e. packets whose data actually points to an AVFrame) are not refcounted. As a consequence, calling av_packet_unref() on them will not free them, but may simply make sure that they leak by losing the pointer to the frame. This commit changes this by mimicking what is being done for wrapped AVFrames: Ownership of the AVFrame is passed to a special AVBuffer with a custom free function that properly frees the AVFrame. The packet is equipped with an AVBufferRef referencing this AVBuffer. Thereby the packet becomes compatible with av_packet_unref(). This already has three advantages, all in interleaved mode: 1. If an error happens at the preparatory steps (before the packet is put into the interleavement queue), the frame is properly freed. 2. If the trailer is never written, the frames still in the interleavement queue will now be properly freed by ff_packet_list_free(). 3. The custom code for moving the packet to the packet list in ff_interleave_add_packet() can be removed. It will also simplify fixing further memleaks in future commits. Given that the AVFrame is now owned by an AVBuffer, the muxer may no longer take ownership of the AVFrame, because the function used to call the muxer when writing uncoded frames does not allow to transfer ownership of the reference contained in the packet. (Changing the function signature is not possible (except at a major version bump), because most of these muxers reside in libavdevice.) But this is no loss as none of the muxers ever made use of this. This change has been documented. Signed-off-by: Andreas Rheinhardt --- The new semantic of AVOutputFormat.write_uncoded_frame() basically boils down to treat frame as AVFrame * const *. I wonder whether I should simply set it that way and remove the then redundant comment. libavformat/avformat.h | 4 ++-- libavformat/mux.c | 43 ++++++++++++++++++++++++------------------ 2 files changed, 27 insertions(+), 20 deletions(-) diff --git a/libavformat/avformat.h b/libavformat/avformat.h index 39b99b4481..89207b9823 100644 --- a/libavformat/avformat.h +++ b/libavformat/avformat.h @@ -578,8 +578,8 @@ typedef struct AVOutputFormat { * * See av_write_uncoded_frame() for details. * - * The library will free *frame afterwards, but the muxer can prevent it - * by setting the pointer to NULL. + * The muxer must not change *frame, but it can keep the content of **frame + * by av_frame_move_ref(). */ int (*write_uncoded_frame)(struct AVFormatContext *, int stream_index, AVFrame **frame, unsigned flags); diff --git a/libavformat/mux.c b/libavformat/mux.c index cc2d1e275a..712ba0c319 100644 --- a/libavformat/mux.c +++ b/libavformat/mux.c @@ -550,12 +550,6 @@ fail: #define AV_PKT_FLAG_UNCODED_FRAME 0x2000 -/* Note: using sizeof(AVFrame) from outside lavu is unsafe in general, but - it is only being used internally to this file as a consistency check. - The value is chosen to be very unlikely to appear on its own and to cause - immediate failure if used anywhere as a real size. */ -#define UNCODED_FRAME_PACKET_SIZE (INT_MIN / 3 * 2 + (int)sizeof(AVFrame)) - #if FF_API_COMPUTE_PKT_FIELDS2 && FF_API_LAVF_AVCTX FF_DISABLE_DEPRECATION_WARNINGS @@ -747,9 +741,10 @@ static int write_packet(AVFormatContext *s, AVPacket *pkt) if ((pkt->flags & AV_PKT_FLAG_UNCODED_FRAME)) { AVFrame *frame = (AVFrame *)pkt->data; - av_assert0(pkt->size == UNCODED_FRAME_PACKET_SIZE); + av_assert0(pkt->size == sizeof(*frame)); ret = s->oformat->write_uncoded_frame(s, pkt->stream_index, &frame, 0); - av_frame_free(&frame); + av_assert0((void*)frame == pkt->data); + av_packet_unref(pkt); } else { ret = s->oformat->write_packet(s, pkt); } @@ -926,14 +921,9 @@ int ff_interleave_add_packet(AVFormatContext *s, AVPacket *pkt, this_pktl = av_malloc(sizeof(AVPacketList)); if (!this_pktl) return AVERROR(ENOMEM); - if (pkt->flags & AV_PKT_FLAG_UNCODED_FRAME) { - av_assert0(pkt->size == UNCODED_FRAME_PACKET_SIZE); - av_assert0(((AVFrame *)pkt->data)->buf); - } else { - if ((ret = av_packet_make_refcounted(pkt)) < 0) { - av_free(this_pktl); - return ret; - } + if ((ret = av_packet_make_refcounted(pkt)) < 0) { + av_free(this_pktl); + return ret; } av_packet_move_ref(&this_pktl->pkt, pkt); @@ -1319,22 +1309,39 @@ int ff_write_chained(AVFormatContext *dst, int dst_stream, AVPacket *pkt, return ret; } +static void uncoded_frame_free(void *unused, uint8_t *data) +{ + AVFrame *frame = (AVFrame *)data; + + av_frame_free(&frame); +} + static int write_uncoded_frame_internal(AVFormatContext *s, int stream_index, AVFrame *frame, int interleaved) { AVPacket pkt, *pktp; av_assert0(s->oformat); - if (!s->oformat->write_uncoded_frame) + if (!s->oformat->write_uncoded_frame) { + av_frame_free(&frame); return AVERROR(ENOSYS); + } if (!frame) { pktp = NULL; } else { pktp = &pkt; av_init_packet(&pkt); + pkt.buf = av_buffer_create((uint8_t *)frame, sizeof(*frame), + uncoded_frame_free, NULL, + AV_BUFFER_FLAG_READONLY); + if (!pkt.buf) { + av_frame_free(&frame); + return AVERROR(ENOMEM); + } + pkt.data = (void *)frame; - pkt.size = UNCODED_FRAME_PACKET_SIZE; + pkt.size = sizeof(*frame); pkt.pts = pkt.dts = frame->pts; pkt.duration = frame->pkt_duration;