diff mbox series

[FFmpeg-devel,23/50] avformat/mux: use av_packet_alloc() to allocate packets

Message ID 20210204191005.48190-24-jamrial@gmail.com
State New
Headers show
Series deprecate av_init_packet() and sizeof(AVPacket) as part of the ABI
Related show

Checks

Context Check Description
andriy/x86_make success Make finished
andriy/x86_make_fate success Make fate finished
andriy/PPC64_make success Make finished
andriy/PPC64_make_fate success Make fate finished

Commit Message

James Almer Feb. 4, 2021, 7:09 p.m. UTC
Signed-off-by: James Almer <jamrial@gmail.com>
---
 libavformat/internal.h |  5 +++++
 libavformat/mux.c      | 44 ++++++++++++++++++++++--------------------
 libavformat/options.c  |  6 ++++++
 libavformat/utils.c    |  1 +
 4 files changed, 35 insertions(+), 21 deletions(-)

Comments

Andreas Rheinhardt Feb. 8, 2021, 6:22 p.m. UTC | #1
James Almer:
> Signed-off-by: James Almer <jamrial@gmail.com>
> ---
>  libavformat/internal.h |  5 +++++
>  libavformat/mux.c      | 44 ++++++++++++++++++++++--------------------
>  libavformat/options.c  |  6 ++++++
>  libavformat/utils.c    |  1 +
>  4 files changed, 35 insertions(+), 21 deletions(-)
> 
> diff --git a/libavformat/internal.h b/libavformat/internal.h
> index d0db331b96..69a7caff93 100644
> --- a/libavformat/internal.h
> +++ b/libavformat/internal.h
> @@ -92,6 +92,11 @@ struct AVFormatInternal {
>       */
>      struct AVPacketList *parse_queue;
>      struct AVPacketList *parse_queue_end;
> +
> +    /**
> +     * Used to hold temporary packets.
> +     */
> +    AVPacket *pkt;
>      /**
>       * Remaining size available for raw_packet_buffer, in bytes.
>       */
> diff --git a/libavformat/mux.c b/libavformat/mux.c
> index 84c56ac6ba..3600e74a50 100644
> --- a/libavformat/mux.c
> +++ b/libavformat/mux.c
> @@ -1052,7 +1052,9 @@ int ff_interleaved_peek(AVFormatContext *s, int stream,
>      AVPacketList *pktl = s->internal->packet_buffer;
>      while (pktl) {
>          if (pktl->pkt.stream_index == stream) {
> -            *pkt = pktl->pkt;
> +            int ret = av_packet_ref(pkt, &pktl->pkt);
> +            if (ret < 0)
> +                return ret;

This will lead to memleaks, because up until now the callers just
received a non-ownership packets, ergo they did not unref the packet.
(The fate-movenc test fails with this when run under valgrind/asan.)

Returning a pointer to a const AVPacket and signaling the offsetted
timestamps via other pointer arguments would avoid this problem.

>              if (add_offset) {
>                  AVStream *st = s->streams[pkt->stream_index];
>                  int64_t offset = st->internal->mux_ts_offset;
> @@ -1208,7 +1210,7 @@ static int write_packets_common(AVFormatContext *s, AVPacket *pkt, int interleav
>  
>  int av_write_frame(AVFormatContext *s, AVPacket *in)
>  {
> -    AVPacket local_pkt, *pkt = &local_pkt;
> +    AVPacket *pkt = s->internal->pkt;
>      int ret;
>  
>      if (!in) {
> @@ -1229,6 +1231,7 @@ int av_write_frame(AVFormatContext *s, AVPacket *in)
>           * The following avoids copying in's data unnecessarily.
>           * Copying side data is unavoidable as a bitstream filter
>           * may change it, e.g. free it on errors. */
> +        av_packet_unref(pkt);
>          pkt->buf  = NULL;
>          pkt->data = in->data;
>          pkt->size = in->size;
> @@ -1270,14 +1273,14 @@ int av_interleaved_write_frame(AVFormatContext *s, AVPacket *pkt)
>  int av_write_trailer(AVFormatContext *s)
>  {
>      int i, ret1, ret = 0;
> -    AVPacket pkt = {0};
> -    av_init_packet(&pkt);
> +    AVPacket *pkt = s->internal->pkt;
>  
> +    av_packet_unref(pkt);
>      for (i = 0; i < s->nb_streams; i++) {
>          if (s->streams[i]->internal->bsfc) {
> -            ret1 = write_packets_from_bsfs(s, s->streams[i], &pkt, 1/*interleaved*/);
> +            ret1 = write_packets_from_bsfs(s, s->streams[i], pkt, 1/*interleaved*/);
>              if (ret1 < 0)
> -                av_packet_unref(&pkt);
> +                av_packet_unref(pkt);
>              if (ret >= 0)
>                  ret = ret1;
>          }
> @@ -1351,7 +1354,7 @@ static void uncoded_frame_free(void *unused, uint8_t *data)
>  static int write_uncoded_frame_internal(AVFormatContext *s, int stream_index,
>                                          AVFrame *frame, int interleaved)
>  {
> -    AVPacket pkt, *pktp;
> +    AVPacket *pkt = s->internal->pkt;
>  
>      av_assert0(s->oformat);
>      if (!s->oformat->write_uncoded_frame) {
> @@ -1360,18 +1363,17 @@ static int write_uncoded_frame_internal(AVFormatContext *s, int stream_index,
>      }
>  
>      if (!frame) {
> -        pktp = NULL;
> +        pkt = NULL;
>      } else {
>          size_t   bufsize = sizeof(frame) + AV_INPUT_BUFFER_PADDING_SIZE;
>          AVFrame **framep = av_mallocz(bufsize);
>  
>          if (!framep)
>              goto fail;
> -        pktp = &pkt;
> -        av_init_packet(&pkt);
> -        pkt.buf = av_buffer_create((void *)framep, bufsize,
> +        av_packet_unref(pkt);
> +        pkt->buf = av_buffer_create((void *)framep, bufsize,
>                                     uncoded_frame_free, NULL, 0);
> -        if (!pkt.buf) {
> +        if (!pkt->buf) {
>              av_free(framep);
>      fail:
>              av_frame_free(&frame);
> @@ -1379,17 +1381,17 @@ static int write_uncoded_frame_internal(AVFormatContext *s, int stream_index,
>          }
>          *framep = frame;
>  
> -        pkt.data         = (void *)framep;
> -        pkt.size         = sizeof(frame);
> -        pkt.pts          =
> -        pkt.dts          = frame->pts;
> -        pkt.duration     = frame->pkt_duration;
> -        pkt.stream_index = stream_index;
> -        pkt.flags |= AV_PKT_FLAG_UNCODED_FRAME;
> +        pkt->data         = (void *)framep;
> +        pkt->size         = sizeof(frame);
> +        pkt->pts          =
> +        pkt->dts          = frame->pts;
> +        pkt->duration     = frame->pkt_duration;
> +        pkt->stream_index = stream_index;
> +        pkt->flags |= AV_PKT_FLAG_UNCODED_FRAME;
>      }
>  
> -    return interleaved ? av_interleaved_write_frame(s, pktp) :
> -                         av_write_frame(s, pktp);
> +    return interleaved ? av_interleaved_write_frame(s, pkt) :
> +                         av_write_frame(s, pkt);
>  }
>  
>  int av_write_uncoded_frame(AVFormatContext *s, int stream_index,
> diff --git a/libavformat/options.c b/libavformat/options.c
> index 59e0389815..8d7c4fe4cb 100644
> --- a/libavformat/options.c
> +++ b/libavformat/options.c
> @@ -220,6 +220,12 @@ AVFormatContext *avformat_alloc_context(void)
>          av_free(ic);
>          return NULL;
>      }
> +    internal->pkt = av_packet_alloc();
> +    if (!internal->pkt) {
> +        av_free(internal);
> +        av_free(ic);
> +        return NULL;
> +    }
>      avformat_get_context_defaults(ic);
>      ic->internal = internal;
>      ic->internal->offset = AV_NOPTS_VALUE;
> diff --git a/libavformat/utils.c b/libavformat/utils.c
> index fb3299503e..2587bedc05 100644
> --- a/libavformat/utils.c
> +++ b/libavformat/utils.c
> @@ -4445,6 +4445,7 @@ void avformat_free_context(AVFormatContext *s)
>      av_freep(&s->chapters);
>      av_dict_free(&s->metadata);
>      av_dict_free(&s->internal->id3v2_meta);
> +    av_packet_free(&s->internal->pkt);
>      av_freep(&s->streams);
>      flush_packet_queue(s);
>      av_freep(&s->internal);
>
James Almer Feb. 8, 2021, 6:50 p.m. UTC | #2
On 2/8/2021 3:22 PM, Andreas Rheinhardt wrote:
> James Almer:
>> Signed-off-by: James Almer<jamrial@gmail.com>
>> ---
>>   libavformat/internal.h |  5 +++++
>>   libavformat/mux.c      | 44 ++++++++++++++++++++++--------------------
>>   libavformat/options.c  |  6 ++++++
>>   libavformat/utils.c    |  1 +
>>   4 files changed, 35 insertions(+), 21 deletions(-)
>>
>> diff --git a/libavformat/internal.h b/libavformat/internal.h
>> index d0db331b96..69a7caff93 100644
>> --- a/libavformat/internal.h
>> +++ b/libavformat/internal.h
>> @@ -92,6 +92,11 @@ struct AVFormatInternal {
>>        */
>>       struct AVPacketList *parse_queue;
>>       struct AVPacketList *parse_queue_end;
>> +
>> +    /**
>> +     * Used to hold temporary packets.
>> +     */
>> +    AVPacket *pkt;
>>       /**
>>        * Remaining size available for raw_packet_buffer, in bytes.
>>        */
>> diff --git a/libavformat/mux.c b/libavformat/mux.c
>> index 84c56ac6ba..3600e74a50 100644
>> --- a/libavformat/mux.c
>> +++ b/libavformat/mux.c
>> @@ -1052,7 +1052,9 @@ int ff_interleaved_peek(AVFormatContext *s, int stream,
>>       AVPacketList *pktl = s->internal->packet_buffer;
>>       while (pktl) {
>>           if (pktl->pkt.stream_index == stream) {
>> -            *pkt = pktl->pkt;
>> +            int ret = av_packet_ref(pkt, &pktl->pkt);
>> +            if (ret < 0)
>> +                return ret;
> This will lead to memleaks, because up until now the callers just
> received a non-ownership packets, ergo they did not unref the packet.
> (The fate-movenc test fails with this when run under valgrind/asan.)
> 
> Returning a pointer to a const AVPacket and signaling the offsetted
> timestamps via other pointer arguments would avoid this problem.

There's a single user of ff_interleaved_peek(), which is the movenc one, 
and i made it unref the packet right after using it. But that's done in 
the following patch, so i guess i should change this function and its 
one caller at the same time.

Also, i could just return the offsetted pts and dts values and not 
bother at all with packets. No allocations that way.
James Almer Feb. 8, 2021, 10:23 p.m. UTC | #3
On 2/8/2021 3:50 PM, James Almer wrote:
> On 2/8/2021 3:22 PM, Andreas Rheinhardt wrote:
>> James Almer:
>>> Signed-off-by: James Almer<jamrial@gmail.com>
>>> ---
>>>   libavformat/internal.h |  5 +++++
>>>   libavformat/mux.c      | 44 ++++++++++++++++++++++--------------------
>>>   libavformat/options.c  |  6 ++++++
>>>   libavformat/utils.c    |  1 +
>>>   4 files changed, 35 insertions(+), 21 deletions(-)
>>>
>>> diff --git a/libavformat/internal.h b/libavformat/internal.h
>>> index d0db331b96..69a7caff93 100644
>>> --- a/libavformat/internal.h
>>> +++ b/libavformat/internal.h
>>> @@ -92,6 +92,11 @@ struct AVFormatInternal {
>>>        */
>>>       struct AVPacketList *parse_queue;
>>>       struct AVPacketList *parse_queue_end;
>>> +
>>> +    /**
>>> +     * Used to hold temporary packets.
>>> +     */
>>> +    AVPacket *pkt;
>>>       /**
>>>        * Remaining size available for raw_packet_buffer, in bytes.
>>>        */
>>> diff --git a/libavformat/mux.c b/libavformat/mux.c
>>> index 84c56ac6ba..3600e74a50 100644
>>> --- a/libavformat/mux.c
>>> +++ b/libavformat/mux.c
>>> @@ -1052,7 +1052,9 @@ int ff_interleaved_peek(AVFormatContext *s, int 
>>> stream,
>>>       AVPacketList *pktl = s->internal->packet_buffer;
>>>       while (pktl) {
>>>           if (pktl->pkt.stream_index == stream) {
>>> -            *pkt = pktl->pkt;
>>> +            int ret = av_packet_ref(pkt, &pktl->pkt);
>>> +            if (ret < 0)
>>> +                return ret;
>> This will lead to memleaks, because up until now the callers just
>> received a non-ownership packets, ergo they did not unref the packet.
>> (The fate-movenc test fails with this when run under valgrind/asan.)
>>
>> Returning a pointer to a const AVPacket and signaling the offsetted
>> timestamps via other pointer arguments would avoid this problem.
> 
> There's a single user of ff_interleaved_peek(), which is the movenc one, 
> and i made it unref the packet right after using it. But that's done in 
> the following patch, so i guess i should change this function and its 
> one caller at the same time.
> 
> Also, i could just return the offsetted pts and dts values and not 
> bother at all with packets. No allocations that way.

Decided to implement your idea with a few changes in a separate patchset.
diff mbox series

Patch

diff --git a/libavformat/internal.h b/libavformat/internal.h
index d0db331b96..69a7caff93 100644
--- a/libavformat/internal.h
+++ b/libavformat/internal.h
@@ -92,6 +92,11 @@  struct AVFormatInternal {
      */
     struct AVPacketList *parse_queue;
     struct AVPacketList *parse_queue_end;
+
+    /**
+     * Used to hold temporary packets.
+     */
+    AVPacket *pkt;
     /**
      * Remaining size available for raw_packet_buffer, in bytes.
      */
diff --git a/libavformat/mux.c b/libavformat/mux.c
index 84c56ac6ba..3600e74a50 100644
--- a/libavformat/mux.c
+++ b/libavformat/mux.c
@@ -1052,7 +1052,9 @@  int ff_interleaved_peek(AVFormatContext *s, int stream,
     AVPacketList *pktl = s->internal->packet_buffer;
     while (pktl) {
         if (pktl->pkt.stream_index == stream) {
-            *pkt = pktl->pkt;
+            int ret = av_packet_ref(pkt, &pktl->pkt);
+            if (ret < 0)
+                return ret;
             if (add_offset) {
                 AVStream *st = s->streams[pkt->stream_index];
                 int64_t offset = st->internal->mux_ts_offset;
@@ -1208,7 +1210,7 @@  static int write_packets_common(AVFormatContext *s, AVPacket *pkt, int interleav
 
 int av_write_frame(AVFormatContext *s, AVPacket *in)
 {
-    AVPacket local_pkt, *pkt = &local_pkt;
+    AVPacket *pkt = s->internal->pkt;
     int ret;
 
     if (!in) {
@@ -1229,6 +1231,7 @@  int av_write_frame(AVFormatContext *s, AVPacket *in)
          * The following avoids copying in's data unnecessarily.
          * Copying side data is unavoidable as a bitstream filter
          * may change it, e.g. free it on errors. */
+        av_packet_unref(pkt);
         pkt->buf  = NULL;
         pkt->data = in->data;
         pkt->size = in->size;
@@ -1270,14 +1273,14 @@  int av_interleaved_write_frame(AVFormatContext *s, AVPacket *pkt)
 int av_write_trailer(AVFormatContext *s)
 {
     int i, ret1, ret = 0;
-    AVPacket pkt = {0};
-    av_init_packet(&pkt);
+    AVPacket *pkt = s->internal->pkt;
 
+    av_packet_unref(pkt);
     for (i = 0; i < s->nb_streams; i++) {
         if (s->streams[i]->internal->bsfc) {
-            ret1 = write_packets_from_bsfs(s, s->streams[i], &pkt, 1/*interleaved*/);
+            ret1 = write_packets_from_bsfs(s, s->streams[i], pkt, 1/*interleaved*/);
             if (ret1 < 0)
-                av_packet_unref(&pkt);
+                av_packet_unref(pkt);
             if (ret >= 0)
                 ret = ret1;
         }
@@ -1351,7 +1354,7 @@  static void uncoded_frame_free(void *unused, uint8_t *data)
 static int write_uncoded_frame_internal(AVFormatContext *s, int stream_index,
                                         AVFrame *frame, int interleaved)
 {
-    AVPacket pkt, *pktp;
+    AVPacket *pkt = s->internal->pkt;
 
     av_assert0(s->oformat);
     if (!s->oformat->write_uncoded_frame) {
@@ -1360,18 +1363,17 @@  static int write_uncoded_frame_internal(AVFormatContext *s, int stream_index,
     }
 
     if (!frame) {
-        pktp = NULL;
+        pkt = NULL;
     } else {
         size_t   bufsize = sizeof(frame) + AV_INPUT_BUFFER_PADDING_SIZE;
         AVFrame **framep = av_mallocz(bufsize);
 
         if (!framep)
             goto fail;
-        pktp = &pkt;
-        av_init_packet(&pkt);
-        pkt.buf = av_buffer_create((void *)framep, bufsize,
+        av_packet_unref(pkt);
+        pkt->buf = av_buffer_create((void *)framep, bufsize,
                                    uncoded_frame_free, NULL, 0);
-        if (!pkt.buf) {
+        if (!pkt->buf) {
             av_free(framep);
     fail:
             av_frame_free(&frame);
@@ -1379,17 +1381,17 @@  static int write_uncoded_frame_internal(AVFormatContext *s, int stream_index,
         }
         *framep = frame;
 
-        pkt.data         = (void *)framep;
-        pkt.size         = sizeof(frame);
-        pkt.pts          =
-        pkt.dts          = frame->pts;
-        pkt.duration     = frame->pkt_duration;
-        pkt.stream_index = stream_index;
-        pkt.flags |= AV_PKT_FLAG_UNCODED_FRAME;
+        pkt->data         = (void *)framep;
+        pkt->size         = sizeof(frame);
+        pkt->pts          =
+        pkt->dts          = frame->pts;
+        pkt->duration     = frame->pkt_duration;
+        pkt->stream_index = stream_index;
+        pkt->flags |= AV_PKT_FLAG_UNCODED_FRAME;
     }
 
-    return interleaved ? av_interleaved_write_frame(s, pktp) :
-                         av_write_frame(s, pktp);
+    return interleaved ? av_interleaved_write_frame(s, pkt) :
+                         av_write_frame(s, pkt);
 }
 
 int av_write_uncoded_frame(AVFormatContext *s, int stream_index,
diff --git a/libavformat/options.c b/libavformat/options.c
index 59e0389815..8d7c4fe4cb 100644
--- a/libavformat/options.c
+++ b/libavformat/options.c
@@ -220,6 +220,12 @@  AVFormatContext *avformat_alloc_context(void)
         av_free(ic);
         return NULL;
     }
+    internal->pkt = av_packet_alloc();
+    if (!internal->pkt) {
+        av_free(internal);
+        av_free(ic);
+        return NULL;
+    }
     avformat_get_context_defaults(ic);
     ic->internal = internal;
     ic->internal->offset = AV_NOPTS_VALUE;
diff --git a/libavformat/utils.c b/libavformat/utils.c
index fb3299503e..2587bedc05 100644
--- a/libavformat/utils.c
+++ b/libavformat/utils.c
@@ -4445,6 +4445,7 @@  void avformat_free_context(AVFormatContext *s)
     av_freep(&s->chapters);
     av_dict_free(&s->metadata);
     av_dict_free(&s->internal->id3v2_meta);
+    av_packet_free(&s->internal->pkt);
     av_freep(&s->streams);
     flush_packet_queue(s);
     av_freep(&s->internal);