Message ID | 20210312105303.1844599-2-andreas.rheinhardt@gmail.com |
---|---|
State | Accepted |
Headers | show |
Series | [FFmpeg-devel,1/2] avformat/adp, svs: Remove redundant av_shrink_packet() | expand |
Context | Check | Description |
---|---|---|
andriy/x86_make | success | Make finished |
andriy/x86_make_fate | success | Make fate finished |
andriy/PPC64_make | success | Make finished |
andriy/PPC64_make_fate | success | Make fate finished |
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@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
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,
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@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(-)