On 9/20/2019 5:39 PM, Andreas Rheinhardt wrote:
> Up until now, read_frame_internal in avformat/utils.c uses a spare
> packet on the stack that serves no real purpose: At no point in this
> function is there a need for another packet besides the packet destined
> for output:
> 1. If the packet doesn't need a parser, but is output as is, the content
> of the spare packet (that at this point contains a freshly read packet)
> is simply copied into the output packet (via simple assignment, not
> av_packet_move_ref, thereby confusing ownership).
> 2. If the packet needs parsing, the spare packet will be reset after
> parsing and any packets resulting from the packet read will be put into
> a packet list; the output packet is not used here at all.
> 3. If the stream should be discarded, the spare packet will be
> unreferenced; the output packet is not used here at all either.
>
> Therefore the spare packet and the copies can be removed in principle.
> In practice, one more thing needs to be taken care of: If ff_read_packet
> failed, the output packet was not affected, now it is. But given that
> ff_read_packet returns a blank (as if reset via av_packet_unref) packet
> on failure, there is no problem from this side either.
There's still the "if (!pktl && st->request_probe <= 0)" check in
ff_read_packet(), which returns without unreferencing the packet.
>
> Signed-off-by: Andreas Rheinhardt <andreas.rheinha...@gmail.com>
> ---
> libavformat/utils.c | 41 ++++++++++++++++++-----------------------
> 1 file changed, 18 insertions(+), 23 deletions(-)
>
> diff --git a/libavformat/utils.c b/libavformat/utils.c
> index 4eb063c54a..291084f6de 100644
> --- a/libavformat/utils.c
> +++ b/libavformat/utils.c
> @@ -1583,10 +1583,9 @@ static int read_frame_internal(AVFormatContext *s,
> AVPacket *pkt)
>
> while (!got_packet && !s->internal->parse_queue) {
> AVStream *st;
> - AVPacket cur_pkt;
>
> /* read next packet */
> - ret = ff_read_packet(s, &cur_pkt);
> + ret = ff_read_packet(s, pkt);
> if (ret < 0) {
> if (ret == AVERROR(EAGAIN))
> return ret;
> @@ -1601,7 +1600,7 @@ static int read_frame_internal(AVFormatContext *s,
> AVPacket *pkt)
> break;
> }
> ret = 0;
> - st = s->streams[cur_pkt.stream_index];
> + st = s->streams[pkt->stream_index];
>
> /* update context if required */
> if (st->internal->need_context_update) {
> @@ -1619,7 +1618,7 @@ static int read_frame_internal(AVFormatContext *s,
> AVPacket *pkt)
>
> ret = avcodec_parameters_to_context(st->internal->avctx,
> st->codecpar);
> if (ret < 0) {
> - av_packet_unref(&cur_pkt);
> + av_packet_unref(pkt);
> return ret;
> }
>
> @@ -1628,7 +1627,7 @@ FF_DISABLE_DEPRECATION_WARNINGS
> /* update deprecated public codec context */
> ret = avcodec_parameters_to_context(st->codec, st->codecpar);
> if (ret < 0) {
> - av_packet_unref(&cur_pkt);
> + av_packet_unref(pkt);
> return ret;
> }
> FF_ENABLE_DEPRECATION_WARNINGS
> @@ -1637,23 +1636,23 @@ FF_ENABLE_DEPRECATION_WARNINGS
> st->internal->need_context_update = 0;
> }
>
> - if (cur_pkt.pts != AV_NOPTS_VALUE &&
> - cur_pkt.dts != AV_NOPTS_VALUE &&
> - cur_pkt.pts < cur_pkt.dts) {
> + if (pkt->pts != AV_NOPTS_VALUE &&
> + pkt->dts != AV_NOPTS_VALUE &&
> + pkt->pts < pkt->dts) {
> av_log(s, AV_LOG_WARNING,
> "Invalid timestamps stream=%d, pts=%s, dts=%s, size=%d\n",
> - cur_pkt.stream_index,
> - av_ts2str(cur_pkt.pts),
> - av_ts2str(cur_pkt.dts),
> - cur_pkt.size);
> + pkt->stream_index,
> + av_ts2str(pkt->pts),
> + av_ts2str(pkt->dts),
> + pkt->size);
> }
> if (s->debug & FF_FDEBUG_TS)
> av_log(s, AV_LOG_DEBUG,
> "ff_read_packet stream=%d, pts=%s, dts=%s, size=%d,
> duration=%"PRId64", flags=%d\n",
> - cur_pkt.stream_index,
> - av_ts2str(cur_pkt.pts),
> - av_ts2str(cur_pkt.dts),
> - cur_pkt.size, cur_pkt.duration, cur_pkt.flags);
> + pkt->stream_index,
> + av_ts2str(pkt->pts),
> + av_ts2str(pkt->dts),
> + pkt->size, pkt->duration, pkt->flags);
>
> if (st->need_parsing && !st->parser && !(s->flags &
> AVFMT_FLAG_NOPARSE)) {
> st->parser = av_parser_init(st->codecpar->codec_id);
> @@ -1673,7 +1672,6 @@ FF_ENABLE_DEPRECATION_WARNINGS
>
> if (!st->need_parsing || !st->parser) {
> /* no parsing needed: we just output the packet as is */
> - *pkt = cur_pkt;
> compute_pkt_fields(s, st, NULL, pkt, AV_NOPTS_VALUE,
> AV_NOPTS_VALUE);
> if ((s->iformat->flags & AVFMT_GENERIC_INDEX) &&
> (pkt->flags & AV_PKT_FLAG_KEY) && pkt->dts !=
> AV_NOPTS_VALUE) {
> @@ -1683,7 +1681,7 @@ FF_ENABLE_DEPRECATION_WARNINGS
> }
> got_packet = 1;
> } else if (st->discard < AVDISCARD_ALL) {
> - if ((ret = parse_packet(s, &cur_pkt, cur_pkt.stream_index)) < 0)
> + if ((ret = parse_packet(s, pkt, pkt->stream_index)) < 0)
> return ret;
> st->codecpar->sample_rate = st->internal->avctx->sample_rate;
> st->codecpar->bit_rate = st->internal->avctx->bit_rate;
> @@ -1692,15 +1690,12 @@ FF_ENABLE_DEPRECATION_WARNINGS
> st->codecpar->codec_id = st->internal->avctx->codec_id;
> } else {
> /* free packet */
> - av_packet_unref(&cur_pkt);
> + av_packet_unref(pkt);
> }
> if (pkt->flags & AV_PKT_FLAG_KEY)
> st->skip_to_keyframe = 0;
> if (st->skip_to_keyframe) {
> - av_packet_unref(&cur_pkt);
> - if (got_packet) {
> - *pkt = cur_pkt;
> - }
> + av_packet_unref(pkt);
> got_packet = 0;
> }
> }
>
_______________________________________________
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".
