[FFmpeg-devel] [PATCH v4 3/9] avformat/utils: Move the reference to the packet list

[James Almer]

On 9/20/2019 5:39 PM, Andreas Rheinhardt wrote:
> Up until now, ff_packet_list_put had a flaw: When it moved a packet to
> the list (meaning, when it ought to move the reference to the packet
> list instead of creating a new one via av_packet_ref), it did not reset
> the original packet, confusing the ownership of the data in the packet.
> This has been done because some callers of this function were not
> compatible with resetting the packet.
> 
> This commit changes these callers and fixes this flaw. In order to
> indicate that the ownership of the packet has moved to the packet list,
> pointers to constant AVPackets are used whenever the target of the
> pointer might already be owned by the packet list.
> 
> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt at gmail.com>
> ---
>  libavformat/utils.c | 32 +++++++++++++++++---------------
>  1 file changed, 17 insertions(+), 15 deletions(-)
> 
> diff --git a/libavformat/utils.c b/libavformat/utils.c
> index e348df3269..58e0db61da 100644
> --- a/libavformat/utils.c
> +++ b/libavformat/utils.c
> @@ -460,10 +460,7 @@ int ff_packet_list_put(AVPacketList **packet_buffer,
>              return ret;
>          }
>      } else {
> -        // TODO: Adapt callers in this file so the line below can use
> -        //       av_packet_move_ref() to effectively move the reference
> -        //       to the list.
> -        pktl->pkt = *pkt;
> +        av_packet_move_ref(&pktl->pkt, pkt);

I think it would be a good idea ensuring the packet is reference counted
here, to have more robust code and not depend on current callers having
done it themselves.
It's a matter of adding an av_packet_make_refcounted() call before this
line, much like it's done in av_bsf_send_packet(). It's essentially a
no-op when the packet is already reference counted.

>      }
>  
>      if (*packet_buffer)
> @@ -835,6 +832,7 @@ int ff_read_packet(AVFormatContext *s, AVPacket *pkt)
>  
>      for (;;) {
>          AVPacketList *pktl = s->internal->raw_packet_buffer;
> +        const AVPacket *pkt1;
>  
>          if (pktl) {
>              st = s->streams[pktl->pkt.stream_index];
> @@ -925,9 +923,10 @@ int ff_read_packet(AVFormatContext *s, AVPacket *pkt)
>              av_packet_unref(pkt);
>              return err;
>          }
> -        s->internal->raw_packet_buffer_remaining_size -= pkt->size;
> +        pkt1 = &s->internal->raw_packet_buffer_end->pkt;
> +        s->internal->raw_packet_buffer_remaining_size -= pkt1->size;
>  
> -        if ((err = probe_codec(s, st, pkt)) < 0)
> +        if ((err = probe_codec(s, st, pkt1)) < 0)
>              return err;
>      }
>  }
> @@ -3035,8 +3034,8 @@ static int has_codec_parameters(AVStream *st, const char **errmsg_ptr)
>  }
>  
>  /* returns 1 or 0 if or if not decoded data was returned, or a negative error */
> -static int try_decode_frame(AVFormatContext *s, AVStream *st, AVPacket *avpkt,
> -                            AVDictionary **options)
> +static int try_decode_frame(AVFormatContext *s, AVStream *st,
> +                            const AVPacket *avpkt, AVDictionary **options)
>  {
>      AVCodecContext *avctx = st->internal->avctx;
>      const AVCodec *codec;
> @@ -3528,7 +3527,7 @@ fail:
>      return ret;
>  }
>  
> -static int extract_extradata(AVStream *st, AVPacket *pkt)
> +static int extract_extradata(AVStream *st, const AVPacket *pkt)
>  {
>      AVStreamInternal *sti = st->internal;
>      AVPacket *pkt_ref;
> @@ -3591,7 +3590,7 @@ int avformat_find_stream_info(AVFormatContext *ic, AVDictionary **options)
>      int64_t read_size;
>      AVStream *st;
>      AVCodecContext *avctx;
> -    AVPacket pkt1, *pkt;
> +    AVPacket pkt1;
>      int64_t old_offset  = avio_tell(ic->pb);
>      // new streams might appear, no options for those
>      int orig_nb_streams = ic->nb_streams;
> @@ -3710,6 +3709,7 @@ FF_ENABLE_DEPRECATION_WARNINGS
>  
>      read_size = 0;
>      for (;;) {
> +        const AVPacket *pkt;
>          int analyzed_all_streams;
>          if (ff_check_interrupt(&ic->interrupt_callback)) {
>              ret = AVERROR_EXIT;
> @@ -3803,14 +3803,16 @@ FF_ENABLE_DEPRECATION_WARNINGS
>              break;
>          }
>  
> -        pkt = &pkt1;
> -
>          if (!(ic->flags & AVFMT_FLAG_NOBUFFER)) {
>              ret = ff_packet_list_put(&ic->internal->packet_buffer,
>                                       &ic->internal->packet_buffer_end,
> -                                     pkt, 0);
> +                                     &pkt1, 0);
>              if (ret < 0)
>                  goto find_stream_info_err;
> +
> +            pkt = &ic->internal->packet_buffer_end->pkt;
> +        } else {
> +            pkt = &pkt1;
>          }
>  
>          st = ic->streams[pkt->stream_index];
> @@ -3888,7 +3890,7 @@ FF_ENABLE_DEPRECATION_WARNINGS
>                         limit,
>                         t, pkt->stream_index);
>                  if (ic->flags & AVFMT_FLAG_NOBUFFER)
> -                    av_packet_unref(pkt);
> +                    av_packet_unref(&pkt1);
>                  break;
>              }
>              if (pkt->duration) {
> @@ -3925,7 +3927,7 @@ FF_ENABLE_DEPRECATION_WARNINGS
>                           (options && i < orig_nb_streams) ? &options[i] : NULL);
>  
>          if (ic->flags & AVFMT_FLAG_NOBUFFER)
> -            av_packet_unref(pkt);
> +            av_packet_unref(&pkt1);
>  
>          st->codec_info_nb_frames++;
>          count++;
>