[FFmpeg-devel] [PATCH v4 7/9] avformat/utils: Avoid copying packets unnecessarily

James Almer jamrial at gmail.com
Thu Sep 26 19:23:19 EEST 2019


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.rheinhardt at 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;
>          }
>      }
> 



More information about the ffmpeg-devel mailing list