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

Submitted by Andreas Rheinhardt on Sept. 20, 2019, 8:39 p.m.

Details

Message ID 20190920203916.16904-4-andreas.rheinhardt@gmail.com
State New
Headers show

Commit Message

Andreas Rheinhardt Sept. 20, 2019, 8:39 p.m.
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@gmail.com>
---
 libavformat/utils.c | 32 +++++++++++++++++---------------
 1 file changed, 17 insertions(+), 15 deletions(-)

Comments

James Almer Sept. 25, 2019, 9:27 p.m.
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@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++;
>
Andreas Rheinhardt Sept. 25, 2019, 10:06 p.m.
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@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.
> 
I can add a commit doing this. But there is at least one potential
use-case where using av_packet_make_refcounted() is not good: Uncoded
frames. They aren't currently used in conjunction with this function,
but that may change of course. Should I add a check for this scenario
or another FF_PACKETLIST_FLAG_* flag that says that the packet should
not be made refcounted, but simply moved?

- Andreas
James Almer Sept. 25, 2019, 10:52 p.m.
On 9/25/2019 7:06 PM, Andreas Rheinhardt wrote:
> 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@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.
>>
> I can add a commit doing this. But there is at least one potential
> use-case where using av_packet_make_refcounted() is not good: Uncoded
> frames. They aren't currently used in conjunction with this function,
> but that may change of course. Should I add a check for this scenario
> or another FF_PACKETLIST_FLAG_* flag that says that the packet should
> not be made refcounted, but simply moved?

No, moving a packet to the list should give full ownership of the packet
to the list, and the data described in them shouldn't just suddenly
become invalid because some buffer was freed by some other function.
That's precisely what happened with the bitstream filter API, generating
filtering and decoding failures because of how ffmpeg.c used to handle
packets internally, until an av_packet_make_refcounted() call was added
to av_bsf_send_packet().

And i personally don't consider adding a check like the one you suggest
something worth doing. If you want you can add a mention to
ff_packet_list_put() doxy that it will ensure the packet will be refcounted.
James Almer Sept. 25, 2019, 11:57 p.m.
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@gmail.com>
> ---
>  libavformat/utils.c | 32 +++++++++++++++++---------------
>  1 file changed, 17 insertions(+), 15 deletions(-)

Applied.

Patch hide | download patch | download mbox

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);
     }
 
     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++;