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.rheinha...@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
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
