diff mbox series

[FFmpeg-devel,01/50] avcodec/packet: deprecate av_init_packet()

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

Checks

Context Check Description
andriy/x86_make_warn warning New warnings during build
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
Once removed, sizeof(AVPacket) will stop being a part of the public ABI.

Signed-off-by: James Almer <jamrial@gmail.com>
---
 libavcodec/avpacket.c  | 23 +++++++++++++++--------
 libavcodec/packet.h    | 23 +++++++++++++++++++----
 libavcodec/version.h   |  3 +++
 libavformat/avformat.h |  4 ++++
 4 files changed, 41 insertions(+), 12 deletions(-)

Comments

James Almer Feb. 8, 2021, 2:32 p.m. UTC | #1
On 2/4/2021 4:09 PM, James Almer wrote:
> Once removed, sizeof(AVPacket) will stop being a part of the public ABI.
> 
> Signed-off-by: James Almer <jamrial@gmail.com>
> ---
>   libavcodec/avpacket.c  | 23 +++++++++++++++--------
>   libavcodec/packet.h    | 23 +++++++++++++++++++----
>   libavcodec/version.h   |  3 +++
>   libavformat/avformat.h |  4 ++++
>   4 files changed, 41 insertions(+), 12 deletions(-)

Will add APIChanges entry and version bump, and apply soon.
Andreas Rheinhardt Feb. 8, 2021, 2:37 p.m. UTC | #2
James Almer:
> On 2/4/2021 4:09 PM, James Almer wrote:
>> Once removed, sizeof(AVPacket) will stop being a part of the public ABI.
>>
>> Signed-off-by: James Almer <jamrial@gmail.com>
>> ---
>>   libavcodec/avpacket.c  | 23 +++++++++++++++--------
>>   libavcodec/packet.h    | 23 +++++++++++++++++++----
>>   libavcodec/version.h   |  3 +++
>>   libavformat/avformat.h |  4 ++++
>>   4 files changed, 41 insertions(+), 12 deletions(-)
> 
> Will add APIChanges entry and version bump, and apply soon.

Why the hurry? In fact, I have just sent a patchset that makes 3/50
redundant.

- Andreas
James Almer Feb. 8, 2021, 2:41 p.m. UTC | #3
On 2/8/2021 11:37 AM, Andreas Rheinhardt wrote:
> James Almer:
>> On 2/4/2021 4:09 PM, James Almer wrote:
>>> Once removed, sizeof(AVPacket) will stop being a part of the public ABI.
>>>
>>> Signed-off-by: James Almer <jamrial@gmail.com>
>>> ---
>>>    libavcodec/avpacket.c  | 23 +++++++++++++++--------
>>>    libavcodec/packet.h    | 23 +++++++++++++++++++----
>>>    libavcodec/version.h   |  3 +++
>>>    libavformat/avformat.h |  4 ++++
>>>    4 files changed, 41 insertions(+), 12 deletions(-)
>>
>> Will add APIChanges entry and version bump, and apply soon.
> 
> Why the hurry? In fact, I have just sent a patchset that makes 3/50
> redundant.

I'm talking about this patch, not the whole set. I'll give the rest some 
more time in case others want to review or test (The vaapi stuff i can't 
test, for example).

And yes, i saw the patches you sent that supersede 3/50, so assuming 
your set is ok i was going to withdraw it.

> 
> - Andreas
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
>
Andreas Rheinhardt Feb. 8, 2021, 2:43 p.m. UTC | #4
James Almer:
> On 2/8/2021 11:37 AM, Andreas Rheinhardt wrote:
>> James Almer:
>>> On 2/4/2021 4:09 PM, James Almer wrote:
>>>> Once removed, sizeof(AVPacket) will stop being a part of the public
>>>> ABI.
>>>>
>>>> Signed-off-by: James Almer <jamrial@gmail.com>
>>>> ---
>>>>    libavcodec/avpacket.c  | 23 +++++++++++++++--------
>>>>    libavcodec/packet.h    | 23 +++++++++++++++++++----
>>>>    libavcodec/version.h   |  3 +++
>>>>    libavformat/avformat.h |  4 ++++
>>>>    4 files changed, 41 insertions(+), 12 deletions(-)
>>>
>>> Will add APIChanges entry and version bump, and apply soon.
>>
>> Why the hurry? In fact, I have just sent a patchset that makes 3/50
>> redundant.
> 
> I'm talking about this patch, not the whole set. I'll give the rest some
> more time in case others want to review or test (The vaapi stuff i can't
> test, for example).
> 
> And yes, i saw the patches you sent that supersede 3/50, so assuming
> your set is ok i was going to withdraw it.
> 
I still don't see the reason to hurry things; actually I don't even this
direction at all, as it just leads to more allocations and frees. The dv
stuff is a good example of this, the mpegvideo_enc another.

- Andreas
James Almer Feb. 8, 2021, 3:15 p.m. UTC | #5
On 2/8/2021 11:43 AM, Andreas Rheinhardt wrote:
> James Almer:
>> On 2/8/2021 11:37 AM, Andreas Rheinhardt wrote:
>>> James Almer:
>>>> On 2/4/2021 4:09 PM, James Almer wrote:
>>>>> Once removed, sizeof(AVPacket) will stop being a part of the public
>>>>> ABI.
>>>>>
>>>>> Signed-off-by: James Almer <jamrial@gmail.com>
>>>>> ---
>>>>>     libavcodec/avpacket.c  | 23 +++++++++++++++--------
>>>>>     libavcodec/packet.h    | 23 +++++++++++++++++++----
>>>>>     libavcodec/version.h   |  3 +++
>>>>>     libavformat/avformat.h |  4 ++++
>>>>>     4 files changed, 41 insertions(+), 12 deletions(-)
>>>>
>>>> Will add APIChanges entry and version bump, and apply soon.
>>>
>>> Why the hurry? In fact, I have just sent a patchset that makes 3/50
>>> redundant.
>>
>> I'm talking about this patch, not the whole set. I'll give the rest some
>> more time in case others want to review or test (The vaapi stuff i can't
>> test, for example).
>>
>> And yes, i saw the patches you sent that supersede 3/50, so assuming
>> your set is ok i was going to withdraw it.
>>
> I still don't see the reason to hurry things;

We're overdue for the bump and a new release before it, so it's best to 
move forward at least a bit. And this change, if committed, should go in 
before either of them.

> actually I don't even this
> direction at all, as it just leads to more allocations and frees. The dv
> stuff is a good example of this, the mpegvideo_enc another.

The mpegvideo_enc was an extreme case the way i wrote it at first, yes. 
And the DV stuff is not an issue for needing allocs, but for being 
shared code between lavd and lavf, with one of the modules, the lavd 
device, being undertested (Does any developer here use libiec61883? The 
massive leak i fixed years ago was there for while, i recall).

New AVPacket fields are being discussed to be added, and if 
sizeof(AVPacket) is public then the window to add them is very small 
(About one month every 2 to 4 years). There's also the issue of field 
initialization. Any new fields could end up uninitialized in the 
scenario of users not calling av_init_packet, which is the reason 
av_read_frame, av_packet_copy_props and av_get_packet do it on their own 
(and it can lead to leaks if you pass them a valid, non empty packet, 
for example).

So I suggested putting AVPacket in line with AVFrame, and two other 
developers agreed. This change removes the above corner cases, relaxes 
the requirements to add new fields, and forces the user to do things 
right, and now that the old decode/encode API is being removed, it's the 
ideal time to do it.
Andreas Rheinhardt Feb. 8, 2021, 3:16 p.m. UTC | #6
James Almer:
> Once removed, sizeof(AVPacket) will stop being a part of the public ABI.
> 
> Signed-off-by: James Almer <jamrial@gmail.com>
> ---
>  libavcodec/avpacket.c  | 23 +++++++++++++++--------
>  libavcodec/packet.h    | 23 +++++++++++++++++++----
>  libavcodec/version.h   |  3 +++
>  libavformat/avformat.h |  4 ++++
>  4 files changed, 41 insertions(+), 12 deletions(-)
> 
> diff --git a/libavcodec/avpacket.c b/libavcodec/avpacket.c
> index e4ba403cf6..ae0cbfb9f9 100644
> --- a/libavcodec/avpacket.c
> +++ b/libavcodec/avpacket.c
> @@ -32,6 +32,7 @@
>  #include "packet.h"
>  #include "packet_internal.h"
>  
> +#if FF_API_INIT_PACKET
>  void av_init_packet(AVPacket *pkt)
>  {
>      pkt->pts                  = AV_NOPTS_VALUE;
> @@ -49,6 +50,16 @@ FF_ENABLE_DEPRECATION_WARNINGS
>      pkt->side_data            = NULL;
>      pkt->side_data_elems      = 0;
>  }
> +#endif
> +
> +static void get_packet_defaults(AVPacket *pkt)
> +{
> +    memset(pkt, 0, sizeof(*pkt));
> +
> +    pkt->pts             = AV_NOPTS_VALUE;
> +    pkt->dts             = AV_NOPTS_VALUE;
> +    pkt->pos             = -1;
> +}
>  
>  AVPacket *av_packet_alloc(void)
>  {
> @@ -56,7 +67,7 @@ AVPacket *av_packet_alloc(void)
>      if (!pkt)
>          return pkt;
>  
> -    av_init_packet(pkt);
> +    get_packet_defaults(pkt);
>  
>      return pkt;
>  }
> @@ -92,7 +103,7 @@ int av_new_packet(AVPacket *pkt, int size)
>      if (ret < 0)
>          return ret;
>  
> -    av_init_packet(pkt);
> +    get_packet_defaults(pkt);
>      pkt->buf      = buf;
>      pkt->data     = buf->data;
>      pkt->size     = size;
> @@ -607,9 +618,7 @@ void av_packet_unref(AVPacket *pkt)
>  {
>      av_packet_free_side_data(pkt);
>      av_buffer_unref(&pkt->buf);
> -    av_init_packet(pkt);
> -    pkt->data = NULL;
> -    pkt->size = 0;
> +    get_packet_defaults(pkt);
>  }
>  
>  int av_packet_ref(AVPacket *dst, const AVPacket *src)
> @@ -664,9 +673,7 @@ AVPacket *av_packet_clone(const AVPacket *src)
>  void av_packet_move_ref(AVPacket *dst, AVPacket *src)
>  {
>      *dst = *src;
> -    av_init_packet(src);
> -    src->data = NULL;
> -    src->size = 0;
> +    get_packet_defaults(src);
>  }
>  
>  int av_packet_make_refcounted(AVPacket *pkt)
> diff --git a/libavcodec/packet.h b/libavcodec/packet.h
> index b9d4c9c2c8..c442b6a6eb 100644
> --- a/libavcodec/packet.h
> +++ b/libavcodec/packet.h
> @@ -319,10 +319,6 @@ typedef struct AVPacketSideData {
>   * packets, with no compressed data, containing only side data
>   * (e.g. to update some stream parameters at the end of encoding).
>   *
> - * AVPacket is one of the few structs in FFmpeg, whose size is a part of public
> - * ABI. Thus it may be allocated on stack and no new fields can be added to it
> - * without libavcodec and libavformat major bump.
> - *
>   * The semantics of data ownership depends on the buf field.
>   * If it is set, the packet data is dynamically allocated and is
>   * valid indefinitely until a call to av_packet_unref() reduces the
> @@ -334,6 +330,12 @@ typedef struct AVPacketSideData {
>   * The side data is always allocated with av_malloc(), copied by
>   * av_packet_ref() and freed by av_packet_unref().
>   *
> + * sizeof(AVPacket) being a part of the public ABI is deprecated. once
> + * av_init_packet() is removed, new packets will only be able to be allocated
> + * with av_packet_alloc(), and new fields may be added to the end of the struct
> + * with a minor bump.
> + *
> + * @see av_packet_alloc
>   * @see av_packet_ref
>   * @see av_packet_unref
>   */
> @@ -394,7 +396,11 @@ typedef struct AVPacket {
>  } AVPacket;
>  
>  typedef struct AVPacketList {
> +#if FF_API_INIT_PACKET
>      AVPacket pkt;
> +#else
> +    AVPacket *pkt;
> +#endif
>      struct AVPacketList *next;
>  } AVPacketList;

As long as the packet-list functions use this structure, there will be
an unnecessary allocation for every packet list entry. This is
unnecessary even if one wants to make sizeof(AVPacket) not part of the
ABI any more: Just move the next pointer to the front.
Of course this will make this structure useless to someone who doesn't
have functions that deal with AVPacketList entries. But given that there
are no public functions dedicated to this task at all and given that
anyone can write a packet list like the above without problems, this
structure should be removed from the public API altogether.
(If the packet list helper functions were to be made public, the above
would need to be revised. But I don't think that adding a packet list
API that forces us to use a linked list should be made public.)

>  
> @@ -460,6 +466,7 @@ AVPacket *av_packet_clone(const AVPacket *src);
>   */
>  void av_packet_free(AVPacket **pkt);
>  
> +#if FF_API_INIT_PACKET
>  /**
>   * Initialize optional fields of a packet with default values.
>   *
> @@ -467,8 +474,16 @@ void av_packet_free(AVPacket **pkt);
>   * initialized separately.
>   *
>   * @param pkt packet
> + *
> + * @see av_packet_alloc
> + * @see av_packet_unref
> + *
> + * @deprecated This function is deprecated. Once it's removed,
> +               sizeof(AVPacket) will not be a part of the ABI anymore.
>   */
> +attribute_deprecated
>  void av_init_packet(AVPacket *pkt);
> +#endif
>  
>  /**
>   * Allocate the payload of a packet and initialize its fields with
> diff --git a/libavcodec/version.h b/libavcodec/version.h
> index 3cac8c5f30..247568ec7f 100644
> --- a/libavcodec/version.h
> +++ b/libavcodec/version.h
> @@ -153,5 +153,8 @@
>  #ifndef FF_API_DEBUG_MV
>  #define FF_API_DEBUG_MV          (LIBAVCODEC_VERSION_MAJOR < 60)
>  #endif
> +#ifndef FF_API_INIT_PACKET
> +#define FF_API_INIT_PACKET         (LIBAVCODEC_VERSION_MAJOR < 60)
> +#endif
>  
>  #endif /* AVCODEC_VERSION_H */
> diff --git a/libavformat/avformat.h b/libavformat/avformat.h
> index 523cf34d55..d2d31e1deb 100644
> --- a/libavformat/avformat.h
> +++ b/libavformat/avformat.h
> @@ -950,7 +950,11 @@ typedef struct AVStream {
>       * decoding: set by libavformat, must not be modified by the caller.
>       * encoding: unused
>       */
> +#if FF_API_INIT_PACKET
>      AVPacket attached_pic;
> +#else
> +    AVPacket *attached_pic;
> +#endif
>  
>      /**
>       * An array of side data that applies to the whole stream (i.e. the
>
James Almer Feb. 8, 2021, 3:25 p.m. UTC | #7
On 2/8/2021 12:16 PM, Andreas Rheinhardt wrote:
>>   typedef struct AVPacketList {
>> +#if FF_API_INIT_PACKET
>>       AVPacket pkt;
>> +#else
>> +    AVPacket *pkt;
>> +#endif
>>       struct AVPacketList *next;
>>   } AVPacketList;
> As long as the packet-list functions use this structure, there will be
> an unnecessary allocation for every packet list entry. This is
> unnecessary even if one wants to make sizeof(AVPacket) not part of the
> ABI any more: Just move the next pointer to the front.
> Of course this will make this structure useless to someone who doesn't
> have functions that deal with AVPacketList entries. But given that there
> are no public functions dedicated to this task at all and given that
> anyone can write a packet list like the above without problems, this
> structure should be removed from the public API altogether.

I agree. I was considering to either make this an opaque struct, so new 
public API that makes use of it can be added down the line without 
worrying about what fields are defined in it, or just remove it.
FFplay was defining its own struct based on it because it needed one 
extra field per element (Which will become unnecessary once an opaque 
user field is added to AVPacket).

> (If the packet list helper functions were to be made public, the above
> would need to be revised. But I don't think that adding a packet list
> API that forces us to use a linked list should be made public.)

A public packet list API could have its internals hidden, and then use a 
FIFO (sizeof(AVPacket) will be allowed within lavc, so no allocs). As 
far as the user can see, the API would simply consume are return packet 
references. But that's for later, if ever.
diff mbox series

Patch

diff --git a/libavcodec/avpacket.c b/libavcodec/avpacket.c
index e4ba403cf6..ae0cbfb9f9 100644
--- a/libavcodec/avpacket.c
+++ b/libavcodec/avpacket.c
@@ -32,6 +32,7 @@ 
 #include "packet.h"
 #include "packet_internal.h"
 
+#if FF_API_INIT_PACKET
 void av_init_packet(AVPacket *pkt)
 {
     pkt->pts                  = AV_NOPTS_VALUE;
@@ -49,6 +50,16 @@  FF_ENABLE_DEPRECATION_WARNINGS
     pkt->side_data            = NULL;
     pkt->side_data_elems      = 0;
 }
+#endif
+
+static void get_packet_defaults(AVPacket *pkt)
+{
+    memset(pkt, 0, sizeof(*pkt));
+
+    pkt->pts             = AV_NOPTS_VALUE;
+    pkt->dts             = AV_NOPTS_VALUE;
+    pkt->pos             = -1;
+}
 
 AVPacket *av_packet_alloc(void)
 {
@@ -56,7 +67,7 @@  AVPacket *av_packet_alloc(void)
     if (!pkt)
         return pkt;
 
-    av_init_packet(pkt);
+    get_packet_defaults(pkt);
 
     return pkt;
 }
@@ -92,7 +103,7 @@  int av_new_packet(AVPacket *pkt, int size)
     if (ret < 0)
         return ret;
 
-    av_init_packet(pkt);
+    get_packet_defaults(pkt);
     pkt->buf      = buf;
     pkt->data     = buf->data;
     pkt->size     = size;
@@ -607,9 +618,7 @@  void av_packet_unref(AVPacket *pkt)
 {
     av_packet_free_side_data(pkt);
     av_buffer_unref(&pkt->buf);
-    av_init_packet(pkt);
-    pkt->data = NULL;
-    pkt->size = 0;
+    get_packet_defaults(pkt);
 }
 
 int av_packet_ref(AVPacket *dst, const AVPacket *src)
@@ -664,9 +673,7 @@  AVPacket *av_packet_clone(const AVPacket *src)
 void av_packet_move_ref(AVPacket *dst, AVPacket *src)
 {
     *dst = *src;
-    av_init_packet(src);
-    src->data = NULL;
-    src->size = 0;
+    get_packet_defaults(src);
 }
 
 int av_packet_make_refcounted(AVPacket *pkt)
diff --git a/libavcodec/packet.h b/libavcodec/packet.h
index b9d4c9c2c8..c442b6a6eb 100644
--- a/libavcodec/packet.h
+++ b/libavcodec/packet.h
@@ -319,10 +319,6 @@  typedef struct AVPacketSideData {
  * packets, with no compressed data, containing only side data
  * (e.g. to update some stream parameters at the end of encoding).
  *
- * AVPacket is one of the few structs in FFmpeg, whose size is a part of public
- * ABI. Thus it may be allocated on stack and no new fields can be added to it
- * without libavcodec and libavformat major bump.
- *
  * The semantics of data ownership depends on the buf field.
  * If it is set, the packet data is dynamically allocated and is
  * valid indefinitely until a call to av_packet_unref() reduces the
@@ -334,6 +330,12 @@  typedef struct AVPacketSideData {
  * The side data is always allocated with av_malloc(), copied by
  * av_packet_ref() and freed by av_packet_unref().
  *
+ * sizeof(AVPacket) being a part of the public ABI is deprecated. once
+ * av_init_packet() is removed, new packets will only be able to be allocated
+ * with av_packet_alloc(), and new fields may be added to the end of the struct
+ * with a minor bump.
+ *
+ * @see av_packet_alloc
  * @see av_packet_ref
  * @see av_packet_unref
  */
@@ -394,7 +396,11 @@  typedef struct AVPacket {
 } AVPacket;
 
 typedef struct AVPacketList {
+#if FF_API_INIT_PACKET
     AVPacket pkt;
+#else
+    AVPacket *pkt;
+#endif
     struct AVPacketList *next;
 } AVPacketList;
 
@@ -460,6 +466,7 @@  AVPacket *av_packet_clone(const AVPacket *src);
  */
 void av_packet_free(AVPacket **pkt);
 
+#if FF_API_INIT_PACKET
 /**
  * Initialize optional fields of a packet with default values.
  *
@@ -467,8 +474,16 @@  void av_packet_free(AVPacket **pkt);
  * initialized separately.
  *
  * @param pkt packet
+ *
+ * @see av_packet_alloc
+ * @see av_packet_unref
+ *
+ * @deprecated This function is deprecated. Once it's removed,
+               sizeof(AVPacket) will not be a part of the ABI anymore.
  */
+attribute_deprecated
 void av_init_packet(AVPacket *pkt);
+#endif
 
 /**
  * Allocate the payload of a packet and initialize its fields with
diff --git a/libavcodec/version.h b/libavcodec/version.h
index 3cac8c5f30..247568ec7f 100644
--- a/libavcodec/version.h
+++ b/libavcodec/version.h
@@ -153,5 +153,8 @@ 
 #ifndef FF_API_DEBUG_MV
 #define FF_API_DEBUG_MV          (LIBAVCODEC_VERSION_MAJOR < 60)
 #endif
+#ifndef FF_API_INIT_PACKET
+#define FF_API_INIT_PACKET         (LIBAVCODEC_VERSION_MAJOR < 60)
+#endif
 
 #endif /* AVCODEC_VERSION_H */
diff --git a/libavformat/avformat.h b/libavformat/avformat.h
index 523cf34d55..d2d31e1deb 100644
--- a/libavformat/avformat.h
+++ b/libavformat/avformat.h
@@ -950,7 +950,11 @@  typedef struct AVStream {
      * decoding: set by libavformat, must not be modified by the caller.
      * encoding: unused
      */
+#if FF_API_INIT_PACKET
     AVPacket attached_pic;
+#else
+    AVPacket *attached_pic;
+#endif
 
     /**
      * An array of side data that applies to the whole stream (i.e. the