[FFmpeg-devel] [PATCH 2/2] avformat/utils: Fix confusing return value for ff_read_packet()

[Andreas Rheinhardt]

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 <andreas.rheinhardt at gmail.com>
---
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,
-- 
2.27.0