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

[Andreas Rheinhardt]

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,
> 
Will apply this patch later today unless there are objections.

- Andreas