From patchwork Mon Aug 19 21:56:23 2019 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Andreas Rheinhardt X-Patchwork-Id: 14595 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 3A67F444AEE for ; Tue, 20 Aug 2019 01:04:06 +0300 (EEST) Received: from [127.0.1.1] (localhost [127.0.0.1]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTP id 25F0568AB6A; Tue, 20 Aug 2019 01:04:06 +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 DAA7168AA99 for ; Tue, 20 Aug 2019 01:03:58 +0300 (EEST) Received: by mail-wm1-f67.google.com with SMTP id l2so871811wmg.0 for ; Mon, 19 Aug 2019 15:03:58 -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=yGkMeSHckJN/Qpv2QtPiHSR4xSpqLJxZOt4h48qpF74=; b=d5+/dzvyj4S8xddQ0VbneReVeM3l79ac7LfMKfxu5oMsEhe9f/+KhPSB5djWfRZo+I oC2s7ZIYDn7oIXxC+e8eM5SokSXqfvlkc8HO/72N50pfrtb640By/TYQegz3eGl6vWzv 9slvy1V3NA8es+E3jUlTQQjtKRoruzBGTvl1SO/Q4QBFeC1hrs/ifxSGpIEzuFnS/TWW O/hqD3a4wMESYo95StqLSqh1qTFKj88aPlFOGXFK7Dm2rgp28nZ3YdEg9QUy71qjPV3p f5nUxLXKJZU8xbb/qCtq8eYAhUynenWMmDR4NHZtL9ikQbIjZqWxbx4gOuEnQia5o+6I OUCQ== 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=yGkMeSHckJN/Qpv2QtPiHSR4xSpqLJxZOt4h48qpF74=; b=Cb/8gleh5Hyhx1DabnjIzj5ebP6I+2oeWM3qYkWyldNsI5sVicPseNBq3fELo+/4Kd s0yVOrih3oKoEktqp1D7NyuzvRrAGGu2aVUz7jlEUCsh0EBKcY2cjRcX+pz9RPRL6MW7 eBsK0+3fAjHWs+fCsWCGdbEvj1gKfu7+kkK/9xiZ3qdCw/TIKmpd3Kw2OiJG8mRcr6Xn OzcDTtnuIQ+F2Ls1XnFtzGF/6GoXCXUcdvKr9VvLpCE7I48ZWQNeGnxTG7XWMrGjFz0Z GpSHSE7gzQtTLB4xzqvdI0zHvtngdHbo3uboGUV9TyBBmsiMOXg7viK+4v8bpiizUc/q DmqQ== X-Gm-Message-State: APjAAAUe8TGcDY0CJiorH9VJaT5fN5bwJKKIZwx8u7WzEj0HbhUtGfDE dNEoKWJ/vXrXLHzzj2xrV5q7wDCJ X-Google-Smtp-Source: APXvYqwCNZrTA7MCThrVJgIKaevszfe5txDK6gELfwwMXPqa7yCayx3uzpWceYpaMUfg1+6iPi0/Ug== X-Received: by 2002:a1c:107:: with SMTP id 7mr22562774wmb.84.1566252238100; Mon, 19 Aug 2019 15:03:58 -0700 (PDT) Received: from localhost.localdomain (ipbcc08c5c.dynamic.kabel-deutschland.de. [188.192.140.92]) by smtp.gmail.com with ESMTPSA id s64sm32952293wmf.16.2019.08.19.15.03.57 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 19 Aug 2019 15:03:57 -0700 (PDT) From: Andreas Rheinhardt To: ffmpeg-devel@ffmpeg.org Date: Mon, 19 Aug 2019 23:56:23 +0200 Message-Id: <20190819215624.49795-7-andreas.rheinhardt@gmail.com> X-Mailer: git-send-email 2.21.0 In-Reply-To: <20190819215624.49795-1-andreas.rheinhardt@gmail.com> References: <20190819215624.49795-1-andreas.rheinhardt@gmail.com> MIME-Version: 1.0 Subject: [FFmpeg-devel] [PATCH v2 7/8] avformat/utils: Remove unnecessary packet copies 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" Up until now, read_frame_internal in avformat/utils.c uses a spare packet on the stack that serves no real purpose: At no point in this function is there a need for another packet besides the packet destined for output: 1. If the packet doesn't need a parser, but is output as is, the content of the spare packet (that at this point contains a freshly read packet) is simply copied into the output packet (via simple assignment, not av_packet_move_ref, thereby confusing ownership). 2. If the packet needs parsing, the spare packet will be reset after parsing and any packets resulting from the packet read will be put into a packet list; the output packet is not used here at all. 3. If the stream should be discarded, the spare packet will be unreferenced; the output packet is not used here at all. Therefore the spare packet and the copies can be removed in priniple. In practice, one more thing needs to be taken care of: If ff_read_packet failed, this did not affect the output packet, now it does. This potential problem is solved by making sure that ff_read_packet always resets the output packet in case of errors. Signed-off-by: Andreas Rheinhardt --- Side note: This skip_to_keyframe stuff which is touched in this commit has been introduced in 4a95876f to be able to drop non-keyframes after the parser in case the keyframes are wrongly marked in the file. But the parser returns packets by putting them in the packet queue and not by returning them via its pkt parameter, so that st->skip_to_keyframe will never be unset and no packet will be dropped because of it for a stream that gets parsed. It actually only works ("works" means that an error message will be displayed for a broken file where the keyframes are not correctly marked) for the file for which it was intended (the one from issue 1003) by accident. Maybe it should be removed altogether? libavformat/utils.c | 51 ++++++++++++++++++++++----------------------- 1 file changed, 25 insertions(+), 26 deletions(-) diff --git a/libavformat/utils.c b/libavformat/utils.c index db0f53869f..d6d615b690 100644 --- a/libavformat/utils.c +++ b/libavformat/utils.c @@ -830,6 +830,10 @@ int ff_read_packet(AVFormatContext *s, AVPacket *pkt) int ret, i, err; AVStream *st; + pkt->data = NULL; + pkt->size = 0; + av_init_packet(pkt); + for (;;) { AVPacketList *pktl = s->internal->raw_packet_buffer; const AVPacket *pkt1; @@ -847,11 +851,11 @@ int ff_read_packet(AVFormatContext *s, AVPacket *pkt) } } - pkt->data = NULL; - pkt->size = 0; - av_init_packet(pkt); ret = s->iformat->read_packet(s, pkt); if (ret < 0) { + pkt->data = NULL; + pkt->size = 0; + av_init_packet(pkt); /* Some demuxers return FFERROR_REDO when they consume data and discard it (ignored streams, junk, extradata). We must re-call the demuxer to get the real packet. */ @@ -1579,10 +1583,9 @@ static int read_frame_internal(AVFormatContext *s, AVPacket *pkt) while (!got_packet && !s->internal->parse_queue) { AVStream *st; - AVPacket cur_pkt; /* read next packet */ - ret = ff_read_packet(s, &cur_pkt); + ret = ff_read_packet(s, pkt); if (ret < 0) { if (ret == AVERROR(EAGAIN)) return ret; @@ -1597,7 +1600,7 @@ static int read_frame_internal(AVFormatContext *s, AVPacket *pkt) break; } ret = 0; - st = s->streams[cur_pkt.stream_index]; + st = s->streams[pkt->stream_index]; /* update context if required */ if (st->internal->need_context_update) { @@ -1615,7 +1618,7 @@ static int read_frame_internal(AVFormatContext *s, AVPacket *pkt) ret = avcodec_parameters_to_context(st->internal->avctx, st->codecpar); if (ret < 0) { - av_packet_unref(&cur_pkt); + av_packet_unref(pkt); return ret; } @@ -1624,7 +1627,7 @@ FF_DISABLE_DEPRECATION_WARNINGS /* update deprecated public codec context */ ret = avcodec_parameters_to_context(st->codec, st->codecpar); if (ret < 0) { - av_packet_unref(&cur_pkt); + av_packet_unref(pkt); return ret; } FF_ENABLE_DEPRECATION_WARNINGS @@ -1633,23 +1636,23 @@ FF_ENABLE_DEPRECATION_WARNINGS st->internal->need_context_update = 0; } - if (cur_pkt.pts != AV_NOPTS_VALUE && - cur_pkt.dts != AV_NOPTS_VALUE && - cur_pkt.pts < cur_pkt.dts) { + if (pkt->pts != AV_NOPTS_VALUE && + pkt->dts != AV_NOPTS_VALUE && + pkt->pts < pkt->dts) { av_log(s, AV_LOG_WARNING, "Invalid timestamps stream=%d, pts=%s, dts=%s, size=%d\n", - cur_pkt.stream_index, - av_ts2str(cur_pkt.pts), - av_ts2str(cur_pkt.dts), - cur_pkt.size); + pkt->stream_index, + av_ts2str(pkt->pts), + av_ts2str(pkt->dts), + pkt->size); } if (s->debug & FF_FDEBUG_TS) av_log(s, AV_LOG_DEBUG, "ff_read_packet stream=%d, pts=%s, dts=%s, size=%d, duration=%"PRId64", flags=%d\n", - cur_pkt.stream_index, - av_ts2str(cur_pkt.pts), - av_ts2str(cur_pkt.dts), - cur_pkt.size, cur_pkt.duration, cur_pkt.flags); + pkt->stream_index, + av_ts2str(pkt->pts), + av_ts2str(pkt->dts), + pkt->size, pkt->duration, pkt->flags); if (st->need_parsing && !st->parser && !(s->flags & AVFMT_FLAG_NOPARSE)) { st->parser = av_parser_init(st->codecpar->codec_id); @@ -1669,7 +1672,6 @@ FF_ENABLE_DEPRECATION_WARNINGS if (!st->need_parsing || !st->parser) { /* no parsing needed: we just output the packet as is */ - *pkt = cur_pkt; compute_pkt_fields(s, st, NULL, pkt, AV_NOPTS_VALUE, AV_NOPTS_VALUE); if ((s->iformat->flags & AVFMT_GENERIC_INDEX) && (pkt->flags & AV_PKT_FLAG_KEY) && pkt->dts != AV_NOPTS_VALUE) { @@ -1679,7 +1681,7 @@ FF_ENABLE_DEPRECATION_WARNINGS } got_packet = 1; } else if (st->discard < AVDISCARD_ALL) { - if ((ret = parse_packet(s, &cur_pkt, cur_pkt.stream_index)) < 0) + if ((ret = parse_packet(s, pkt, pkt->stream_index)) < 0) return ret; st->codecpar->sample_rate = st->internal->avctx->sample_rate; st->codecpar->bit_rate = st->internal->avctx->bit_rate; @@ -1688,15 +1690,12 @@ FF_ENABLE_DEPRECATION_WARNINGS st->codecpar->codec_id = st->internal->avctx->codec_id; } else { /* free packet */ - av_packet_unref(&cur_pkt); + av_packet_unref(pkt); } if (pkt->flags & AV_PKT_FLAG_KEY) st->skip_to_keyframe = 0; if (st->skip_to_keyframe) { - av_packet_unref(&cur_pkt); - if (got_packet) { - *pkt = cur_pkt; - } + av_packet_unref(pkt); got_packet = 0; } }