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

[James Almer]

On 9/25/2019 9:05 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.
> 
> Furthermore, the packet is always made refcounted so that its data is
> really owned by the packet. This was already true for the current
> callers.

Ah, my bad. When you said "I can add a commit doing this" i assumed
you'd do it in a separate patch. I already committed the "v4 3/9" one.

I'll apply the doxy changes and the av_packet_make_refcounted() call in
a new patch in a moment.

> 
> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt at gmail.com>
> ---
>  libavformat/internal.h |  3 ++-
>  libavformat/utils.c    | 37 ++++++++++++++++++++++---------------
>  2 files changed, 24 insertions(+), 16 deletions(-)
> 
> diff --git a/libavformat/internal.h b/libavformat/internal.h
> index 163587f416..a37404d823 100644
> --- a/libavformat/internal.h
> +++ b/libavformat/internal.h
> @@ -763,7 +763,8 @@ void ff_format_set_url(AVFormatContext *s, char *url);
>   *
>   * @param head  List head element
>   * @param tail  List tail element
> - * @param pkt   The packet being appended
> + * @param pkt   The packet being appended.  It will be made refcounted
> + *              if it isn't already.
>   * @param flags Any combination of FF_PACKETLIST_FLAG_* flags
>   * @return 0 on success, negative AVERROR value on failure. On failure,
>             the list is unchanged
> diff --git a/libavformat/utils.c b/libavformat/utils.c
> index 4657ba2642..9f8a5bfb63 100644
> --- a/libavformat/utils.c
> +++ b/libavformat/utils.c
> @@ -460,10 +460,12 @@ 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;
> +        ret = av_packet_make_refcounted(pkt);
> +        if (ret < 0) {
> +            av_free(pktl);
> +            return ret;
> +        }
> +        av_packet_move_ref(&pktl->pkt, pkt);
>      }
>  
>      if (*packet_buffer)
> @@ -835,6 +837,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];
> @@ -922,9 +925,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;
>      }
>  }
> @@ -3032,8 +3036,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;
> @@ -3525,7 +3529,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;
> @@ -3588,7 +3592,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;
> @@ -3707,6 +3711,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;
> @@ -3800,14 +3805,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];
> @@ -3885,7 +3892,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) {
> @@ -3922,7 +3929,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++;
>