| 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 | expand |
| 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 |
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.
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
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". >
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
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.
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 >
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 --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
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(-)