From patchwork Fri Mar 12 10:53:03 2021 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Andreas Rheinhardt X-Patchwork-Id: 26349 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 D974244B258 for ; Fri, 12 Mar 2021 12:54:46 +0200 (EET) Received: from [127.0.1.1] (localhost [127.0.0.1]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTP id B136A68B0FD; Fri, 12 Mar 2021 12:54:46 +0200 (EET) X-Original-To: ffmpeg-devel@ffmpeg.org Delivered-To: ffmpeg-devel@ffmpeg.org Received: from mail-wr1-f48.google.com (mail-wr1-f48.google.com [209.85.221.48]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTPS id 64C4B68AF73 for ; Fri, 12 Mar 2021 12:54:40 +0200 (EET) Received: by mail-wr1-f48.google.com with SMTP id v15so4550303wrx.4 for ; Fri, 12 Mar 2021 02:54:40 -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:reply-to :mime-version:content-transfer-encoding; bh=QI0RB9xqTSEqjOy8R1MnvG04wsRPxd8ZJCWqpMxoHq8=; b=pvqcpo9vzROmb3ZUIabPI6/5cD7QPpQHU5lz4iQvZ3npEAkh0vrji+EcFIl+vzU5Mo LJZe+3boXHPgKLxMQbfSGcaJ4SMNURe0jIVP8NjzxDgFctV+k5T1UKVFGMhyfedsEb4R 4zuDt1qD8ozIuOYfEBidJIZBedmonxb5tVWCwHxpoY223oMSXSJ3VJHWREgob6f4SIll mCoJSuf7p3Xxo+nIXbFfGaPZIJ4QSxgbBg+Tf1bFI0SCtTTnf/TWSdO1ckE93CFAc1fW 6tJFhz9pQP7sOAPMtQ1o0u7k/UWIeH6pcQOPct1Jz/j4SJszlgBv6Iesi8flVwOy/r5n iiKw== 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:reply-to:mime-version:content-transfer-encoding; bh=QI0RB9xqTSEqjOy8R1MnvG04wsRPxd8ZJCWqpMxoHq8=; b=DFkhD2rAsIVA9lr+429+qXNG+/pY0Pd4RPIoesAm2A3vLRqkTWMGobFvR1pUbZTaPV xEVDDL3FwsZglsDyeBj/1j/xRvvYJvXm/Csq110cs7Zv4/x7NaUjhU9CjekhR2LzgUYa vyZ0PDlCLa5czUvQZgHp6n2+yD0fZqhi2NPvcF0d6q9+k9EMz/PlNKRfA2/Ir5CgIDxK 5rccZOa44tGLwD/KAtbELcU156k5d95VO/Et1SJ8Hk2ASjnOx6fu20ixzRHJq+2CbgkZ puEGiqE5P//Qk/9ubcUXGExgZzBeS2ONjjOClNiRPqNuzl+ZLF37lxDbZNhzcfc0FgD3 f8rQ== X-Gm-Message-State: AOAM5301p5epwxqPbLFOBWxf8JzK1qHaTWBpmYZaVvpFbz75UGBqdyoa mljZOnhrbxxKBGz1c1f5NVJjSTfddnk= X-Google-Smtp-Source: ABdhPJzwPcqUa4LG9yb1IWi5tPG0H4iUY5TvOs4441/HBuD2vCJ7E91UVUnu+cQv5y3UNh8WVlKWNA== X-Received: by 2002:adf:ea0e:: with SMTP id q14mr13681595wrm.389.1615546479626; Fri, 12 Mar 2021 02:54:39 -0800 (PST) Received: from sblaptop.fritz.box (ipbcc08960.dynamic.kabel-deutschland.de. [188.192.137.96]) by smtp.gmail.com with ESMTPSA id v18sm8242994wrf.41.2021.03.12.02.54.39 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Fri, 12 Mar 2021 02:54:39 -0800 (PST) From: Andreas Rheinhardt To: ffmpeg-devel@ffmpeg.org Date: Fri, 12 Mar 2021 11:53:03 +0100 Message-Id: <20210312105303.1844599-2-andreas.rheinhardt@gmail.com> X-Mailer: git-send-email 2.27.0 In-Reply-To: <20210312105303.1844599-1-andreas.rheinhardt@gmail.com> References: <20210312105303.1844599-1-andreas.rheinhardt@gmail.com> MIME-Version: 1.0 Subject: [FFmpeg-devel] [PATCH 2/2] avformat/utils: Fix confusing return value for ff_read_packet() 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, ff_read_packet() sometimes forwards the return value of AVInputFormat.read_packet() (which should be zero on success, but isn't for all demuxers) and sometimes it overwrites this with zero. Furthermore, it uses two variables, one for the read_packet return value and one for other errors, which is a bit confusing; it is also unnecessary given that the documentation explicitly states that ff_read_packet() never returns positive values. Returning a positive value would lead to leaks with some callers (namely asfrtp_parse_packet and estimate_timings_from_pts). So always return zero in case of success. (This behaviour stems from a time before av_read_packet sanitized the return value of read_packet at all: It was added in commit 626004690c23c981f67228ea325dde3f35193988 and was unnecessary since 88b00723906f68b7563214c30333e48888dddf78.) Signed-off-by: Andreas Rheinhardt --- asfrtp_parse_packet contains this: for (;;) { int i; res = ff_read_packet(rt->asf_ctx, pkt); rt->asf_pb_pos = avio_tell(pb); if (res != 0) break; for (i = 0; i < s->nb_streams; i++) { if (s->streams[i]->id == rt->asf_ctx->streams[pkt->stream_index]->id) { pkt->stream_index = i; return 1; // FIXME: return 0 if last packet } } av_packet_unref(pkt); } return res == 1 ? -1 : res; It's the very last line that bothers me: ff_read_packet and av_read_packet (its predecessor that was used when this code has originally been added) are not allowed to return anything > 0 and they didn't for the asf demuxer. Does someone see a hidden intent in this or was it just wrong from the beginning? libavformat/utils.c | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/libavformat/utils.c b/libavformat/utils.c index 8573117694..a3de4af9a1 100644 --- a/libavformat/utils.c +++ b/libavformat/utils.c @@ -804,7 +804,7 @@ static int update_wrap_reference(AVFormatContext *s, AVStream *st, int stream_in int ff_read_packet(AVFormatContext *s, AVPacket *pkt) { - int ret, i, err; + int err, i; AVStream *st; pkt->data = NULL; @@ -828,17 +828,17 @@ int ff_read_packet(AVFormatContext *s, AVPacket *pkt) } } - ret = s->iformat->read_packet(s, pkt); - if (ret < 0) { + err = s->iformat->read_packet(s, pkt); + if (err < 0) { av_packet_unref(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. */ - if (ret == FFERROR_REDO) + if (err == FFERROR_REDO) continue; - if (!pktl || ret == AVERROR(EAGAIN)) - return ret; + if (!pktl || err == AVERROR(EAGAIN)) + return err; for (i = 0; i < s->nb_streams; i++) { st = s->streams[i]; if (st->probe_packets || st->internal->request_probe > 0) @@ -892,7 +892,7 @@ int ff_read_packet(AVFormatContext *s, AVPacket *pkt) pkt->dts = pkt->pts = av_rescale_q(av_gettime(), AV_TIME_BASE_Q, st->time_base); if (!pktl && st->internal->request_probe <= 0) - return ret; + return 0; err = avpriv_packet_list_put(&s->internal->raw_packet_buffer, &s->internal->raw_packet_buffer_end,