[FFmpeg-devel] [PATCH] avpacket: RFC for ABI bump additions
James Almer
jamrial at gmail.com
Wed Jan 27 21:37:10 EET 2021
On 1/27/2021 2:40 PM, Lynne wrote:
> Jan 27, 2021, 16:07 by jamrial at gmail.com:
>
>> On 1/27/2021 6:16 AM, Anton Khirnov wrote:
>>
>>> Quoting James Almer (2021-01-26 20:11:16)
>>>
>>>> On 1/26/2021 1:17 PM, Anton Khirnov wrote:
>>>>
>>>>> We could start by adding a field to AVPacket that would be set to a
>>>>> magic value by av_packet_alloc().
>>>>> Then have e.g. AVCodecContext/AVFormatContext warn when they see a
>>>>> packet without this magic value.
>>>>>
>>>>
>>>> I don't like much the idea of adding a public field just to emit a
>>>> deprecation warning.
>>>>
>>>
>>>
>>> int internal_do_not_touch; // do not touch
>>>
>>> is not really public. It is visible in the public headers, but so are
>>> all the AVFooInternal. I agree that it is not the prettiest thing ever,
>>> but it's not too bad.
>>>
>>> And I believe it would solve a real problem, since we have few other
>>> ways to let our users know they need to change something. Most of them
>>> do not follow development closely, I'd think.
>>>
>>
>> If sizeof(AVPacket) stops being part of the ABI, then av_init_packet() becomes unnecessary, right? av_packet_unref() will be the only valid way for the user to reuse the AVPacket. So that deprecation warning might be enough for stack users.
>>
>
> I think just a compile-time deprecation on av_init_packet() would be enough.
>
>
>
>> Regarding a new internal field that lavf could check, i don't think it's enough since it may be uninitialized for packets on stack. And you can't make av_init_packet() set it to 0/NULL because then av_packet_unref() will also reset it, and lavf will start printing bogus warnings for allocated packet users.
>>
>
> I'd really rather not spam the log of users with API deprecation messages. We're
> already guilty enough of doing that with color_range.
Ok, something like this patch, then?
Assuming we go through with it, I don't know what would be better, if to
push it now for ffmpeg 4.4 (Which should be branched out before the
bump), or after the bump.
The former would give a lot more time for downstreams to adapt, since it
will be in a release for the majority of the deprecation period rather
than exclusively on the master branch until the first post-bump release.
There's also a lot of stack usage for AVPacket within libav*, so we will
have to either remove them all alongside the deprecation, or at least
silence the av_init_packet() warnings while we slowly go through all of
them.
-------------- next part --------------
From 75d82c56aca9d1905865e0b1e40258ec0da61e6b Mon Sep 17 00:00:00 2001
From: James Almer <jamrial at gmail.com>
Date: Wed, 27 Jan 2021 16:24:10 -0300
Subject: [PATCH] avcodec/packet: deprecate av_init_packet()
Once removed, sizeof(AVPacket) will stop being a part of the public ABI.
Signed-off-by: James Almer <jamrial at gmail.com>
---
libavcodec/avpacket.c | 34 ++++++++++++++++++++++++++--------
libavcodec/packet.h | 19 +++++++++++++++----
libavcodec/version.h | 3 +++
3 files changed, 44 insertions(+), 12 deletions(-)
diff --git a/libavcodec/avpacket.c b/libavcodec/avpacket.c
index e4ba403cf6..e6cae39c81 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,27 @@ FF_ENABLE_DEPRECATION_WARNINGS
pkt->side_data = NULL;
pkt->side_data_elems = 0;
}
+#endif
+
+static void packet_init(AVPacket *pkt)
+{
+ pkt->pts = AV_NOPTS_VALUE;
+ pkt->dts = AV_NOPTS_VALUE;
+ pkt->pos = -1;
+ pkt->duration = 0;
+#if FF_API_CONVERGENCE_DURATION
+FF_DISABLE_DEPRECATION_WARNINGS
+ pkt->convergence_duration = 0;
+FF_ENABLE_DEPRECATION_WARNINGS
+#endif
+ pkt->flags = 0;
+ pkt->stream_index = 0;
+ pkt->buf = NULL;
+ pkt->data = NULL;
+ pkt->side_data = NULL;
+ pkt->side_data_elems = 0;
+ pkt->size = 0;
+}
AVPacket *av_packet_alloc(void)
{
@@ -56,7 +78,7 @@ AVPacket *av_packet_alloc(void)
if (!pkt)
return pkt;
- av_init_packet(pkt);
+ packet_init(pkt);
return pkt;
}
@@ -92,7 +114,7 @@ int av_new_packet(AVPacket *pkt, int size)
if (ret < 0)
return ret;
- av_init_packet(pkt);
+ packet_init(pkt);
pkt->buf = buf;
pkt->data = buf->data;
pkt->size = size;
@@ -607,9 +629,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;
+ packet_init(pkt);
}
int av_packet_ref(AVPacket *dst, const AVPacket *src)
@@ -664,9 +684,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;
+ packet_init(src);
}
int av_packet_make_refcounted(AVPacket *pkt)
diff --git a/libavcodec/packet.h b/libavcodec/packet.h
index b9d4c9c2c8..d7f34d109e 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
*/
@@ -460,6 +462,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 +470,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 1e1bedfce6..af8487a4c1 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 */
--
2.30.0
More information about the ffmpeg-devel
mailing list