diff mbox series

[FFmpeg-devel] avpacket: ABI bump additions

Message ID MZ9v9ux--3-2@lynne.ee
State New
Headers show
Series [FFmpeg-devel] avpacket: ABI bump additions
Related show

Checks

Context Check Description
andriy/configure warning Failed to apply patch
andriy/configure warning Failed to apply patch

Commit Message

Lynne April 25, 2021, 9:25 p.m. UTC
This is the same patch sent in January, rebased on top of the ABI bump
patchset.

Two additions mirror exactly what AVFrame has - an opaque field
and an opaque_ref for user-side private data.
For justification on the void *opaque field, you can read the archives,
since the question was brought up in January.

As for the time_base field, for now, it will only be used to inform the user,
and will not alter the behavior of the libraries. That change will come as an
optional flag.

Patch attached.
Subject: [PATCH] avpacket: ABI bump additions

---
 libavcodec/avpacket.c |  5 +++++
 libavcodec/packet.h   | 21 +++++++++++++++++++++
 2 files changed, 26 insertions(+)

Comments

Marton Balint April 26, 2021, 12:36 a.m. UTC | #1
On Sun, 25 Apr 2021, Lynne wrote:

> This is the same patch sent in January, rebased on top of the ABI bump
> patchset.
>
> Two additions mirror exactly what AVFrame has - an opaque field
> and an opaque_ref for user-side private data.
> For justification on the void *opaque field, you can read the archives,
> since the question was brought up in January.
>
> As for the time_base field, for now, it will only be used to inform the user,
> and will not alter the behavior of the libraries. That change will come as an
> optional flag.

I would like to see some documentation and code which shows when the 
time_base is respected and when it is not before it is added to AVPacket. 
After the bump we usually have a 1-2 months of cooldown when the ABI can 
change, so there really is no rush.

Thanks,
Marton
Andreas Rheinhardt April 26, 2021, 1:27 a.m. UTC | #2
Lynne:
> From 097aed2ac33dda0bb2052d8b0402711ce95079ba Mon Sep 17 00:00:00 2001
> From: Lynne <dev@lynne.ee>
> Date: Sat, 23 Jan 2021 19:56:18 +0100
> Subject: [PATCH] avpacket: ABI bump additions
> 
> ---
>  libavcodec/avpacket.c |  5 +++++
>  libavcodec/packet.h   | 21 +++++++++++++++++++++
>  2 files changed, 26 insertions(+)
> 
> diff --git a/libavcodec/avpacket.c b/libavcodec/avpacket.c
> index e32c467586..03b73b3b53 100644
> --- a/libavcodec/avpacket.c
> +++ b/libavcodec/avpacket.c
> @@ -382,6 +382,10 @@ int av_packet_copy_props(AVPacket *dst, const AVPacket *src)
>      dst->flags                = src->flags;
>      dst->stream_index         = src->stream_index;
>  
> +    i = av_buffer_replace(&dst->opaque_ref, src->opaque_ref);
> +    if (i < 0)
> +        return i;

1. Don't use i here; add a new variable.
2. Up until now, av_packet_ref() and av_packet_copy_props() treat the
destination packet as uninitialized and make no attempt at unreferencing
its content; yet you try to reuse opaque_ref. Even worse, you might
return potentially dangerous packets: If the properties were
uninitialized and av_packet_copy_props() failed, then the caller were
not allowed to unreference the packet even when the non-properties were
set to sane values. The easiest way to fix this is to move setting
opaque ref to the place after initializing side_data below and either
set dst->opaque_ref to NULL before av_buffer_replace() or to not use
av_buffer_replace(). It may also be best to unref it again if copying
side data fails.

> +
>      dst->side_data            = NULL;
>      dst->side_data_elems      = 0;
>      for (i = 0; i < src->side_data_elems; i++) {
> @@ -403,6 +407,7 @@ int av_packet_copy_props(AVPacket *dst, const AVPacket *src)
>  void av_packet_unref(AVPacket *pkt)
>  {
>      av_packet_free_side_data(pkt);
> +    av_buffer_unref(&pkt->opaque_ref);
>      av_buffer_unref(&pkt->buf);
>      get_packet_defaults(pkt);
>  }
> diff --git a/libavcodec/packet.h b/libavcodec/packet.h
> index fad8341c12..c29ad18a2b 100644
> --- a/libavcodec/packet.h
> +++ b/libavcodec/packet.h
> @@ -383,6 +383,27 @@ typedef struct AVPacket {
>      int64_t duration;
>  
>      int64_t pos;                            ///< byte position in stream, -1 if unknown
> +
> +    /**
> +     * for some private data of the user
> +     */
> +    void *opaque;

The corresponding AVFrame field is copied when copying props.

> +
> +    /**
> +     * AVBufferRef for free use by the API user. FFmpeg will never check the
> +     * contents of the buffer ref. FFmpeg calls av_buffer_unref() on it when
> +     * the packet is unreferenced. av_packet_copy_props() calls create a new
> +     * reference with av_buffer_ref() for the target packet's opaque_ref field.
> +     *
> +     * This is unrelated to the opaque field, although it serves a similar
> +     * purpose.
> +     */
> +    AVBufferRef *opaque_ref;
> +
> +    /**
> +     * Time base of the packet's timestamps.
> +     */
> +    AVRational time_base;
>  } AVPacket;
>  
>  #if FF_API_INIT_PACKET
Lynne April 26, 2021, 11:14 a.m. UTC | #3
Apr 26, 2021, 02:36 by cus@passwd.hu:

>
>
> On Sun, 25 Apr 2021, Lynne wrote:
>
>> This is the same patch sent in January, rebased on top of the ABI bump
>> patchset.
>>
>> Two additions mirror exactly what AVFrame has - an opaque field
>> and an opaque_ref for user-side private data.
>> For justification on the void *opaque field, you can read the archives,
>> since the question was brought up in January.
>>
>> As for the time_base field, for now, it will only be used to inform the user,
>> and will not alter the behavior of the libraries. That change will come as an
>> optional flag.
>>
>
> I would like to see some documentation and code which shows when the time_base is respected and when it is not before it is added to AVPacket. After the bump we usually have a 1-2 months of cooldown when the ABI can change, so there really is no rush.
>

I didn't mean to say that it'll be done during the bump, but rather after it.
Once we add time_base, there are no ABI-breaking changes to support
what I was thinking of.
Or do you still want to see some code? It's not a small amount of work.
James Almer April 30, 2021, 8:47 p.m. UTC | #4
On 4/25/2021 10:27 PM, Andreas Rheinhardt wrote:
> Lynne:
>>  From 097aed2ac33dda0bb2052d8b0402711ce95079ba Mon Sep 17 00:00:00 2001
>> From: Lynne <dev@lynne.ee>
>> Date: Sat, 23 Jan 2021 19:56:18 +0100
>> Subject: [PATCH] avpacket: ABI bump additions
>>
>> ---
>>   libavcodec/avpacket.c |  5 +++++
>>   libavcodec/packet.h   | 21 +++++++++++++++++++++
>>   2 files changed, 26 insertions(+)
>>
>> diff --git a/libavcodec/avpacket.c b/libavcodec/avpacket.c
>> index e32c467586..03b73b3b53 100644
>> --- a/libavcodec/avpacket.c
>> +++ b/libavcodec/avpacket.c
>> @@ -382,6 +382,10 @@ int av_packet_copy_props(AVPacket *dst, const AVPacket *src)
>>       dst->flags                = src->flags;
>>       dst->stream_index         = src->stream_index;
>>   
>> +    i = av_buffer_replace(&dst->opaque_ref, src->opaque_ref);
>> +    if (i < 0)
>> +        return i;
> 
> 1. Don't use i here; add a new variable.
> 2. Up until now, av_packet_ref() and av_packet_copy_props() treat the
> destination packet as uninitialized and make no attempt at unreferencing
> its content; yet you try to reuse opaque_ref. Even worse, you might
> return potentially dangerous packets: If the properties were
> uninitialized and av_packet_copy_props() failed, then the caller were
> not allowed to unreference the packet even when the non-properties were
> set to sane values. The easiest way to fix this is to move setting
> opaque ref to the place after initializing side_data below and either
> set dst->opaque_ref to NULL before av_buffer_replace() or to not use
> av_buffer_replace(). It may also be best to unref it again if copying
> side data fails.

Fwiw, the idea is that once sizeof(AVPacket) is not a part of the ABI 
anymore, there will no longer be such thing as an uninitialized packet 
as far as proper API usage goes. And as such the lines below...

> 
>> +
>>       dst->side_data            = NULL;
>>       dst->side_data_elems      = 0;

          ^^^^^^^^^^^^^^^^^^^^

...should be replaced by a av_packet_free_side_data(dst) call.
Similarly, at that point using av_buffer_replace() on opaque_ref should 
become safe. But until then, you're right that doing so *will* go wrong 
on uninitialized packets.

>>       for (i = 0; i < src->side_data_elems; i++) {
>> @@ -403,6 +407,7 @@ int av_packet_copy_props(AVPacket *dst, const AVPacket *src)
>>   void av_packet_unref(AVPacket *pkt)
>>   {
>>       av_packet_free_side_data(pkt);
>> +    av_buffer_unref(&pkt->opaque_ref);
>>       av_buffer_unref(&pkt->buf);
>>       get_packet_defaults(pkt);
>>   }
>> diff --git a/libavcodec/packet.h b/libavcodec/packet.h
>> index fad8341c12..c29ad18a2b 100644
>> --- a/libavcodec/packet.h
>> +++ b/libavcodec/packet.h
>> @@ -383,6 +383,27 @@ typedef struct AVPacket {
>>       int64_t duration;
>>   
>>       int64_t pos;                            ///< byte position in stream, -1 if unknown
>> +
>> +    /**
>> +     * for some private data of the user
>> +     */
>> +    void *opaque;
> 
> The corresponding AVFrame field is copied when copying props.
> 
>> +
>> +    /**
>> +     * AVBufferRef for free use by the API user. FFmpeg will never check the
>> +     * contents of the buffer ref. FFmpeg calls av_buffer_unref() on it when
>> +     * the packet is unreferenced. av_packet_copy_props() calls create a new
>> +     * reference with av_buffer_ref() for the target packet's opaque_ref field.
>> +     *
>> +     * This is unrelated to the opaque field, although it serves a similar
>> +     * purpose.
>> +     */
>> +    AVBufferRef *opaque_ref;
>> +
>> +    /**
>> +     * Time base of the packet's timestamps.
>> +     */
>> +    AVRational time_base;
>>   } AVPacket;
>>   
>>   #if FF_API_INIT_PACKET
> 
> _______________________________________________
> 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".
>
Marton Balint April 30, 2021, 9:50 p.m. UTC | #5
On Mon, 26 Apr 2021, Lynne wrote:

> Apr 26, 2021, 02:36 by cus@passwd.hu:
>
>>
>>
>> On Sun, 25 Apr 2021, Lynne wrote:
>>
>>> This is the same patch sent in January, rebased on top of the ABI bump
>>> patchset.
>>>
>>> Two additions mirror exactly what AVFrame has - an opaque field
>>> and an opaque_ref for user-side private data.
>>> For justification on the void *opaque field, you can read the archives,
>>> since the question was brought up in January.
>>>
>>> As for the time_base field, for now, it will only be used to inform the user,
>>> and will not alter the behavior of the libraries. That change will come as an
>>> optional flag.
>>>
>>
>> I would like to see some documentation and code which shows when the time_base is respected and when it is not before it is added to AVPacket. After the bump we usually have a 1-2 months of cooldown when the ABI can change, so there really is no rush.
>>
>
> I didn't mean to say that it'll be done during the bump, but rather after it.
> Once we add time_base, there are no ABI-breaking changes to support
> what I was thinking of.
> Or do you still want to see some code? It's not a small amount of work.

But if it is far from trivial, then does it really worth introducing it? I 
am asking, because I am still not sure about the benefits or the issues it 
is trying to solve.

You wrote earlier that you don't like that you have to pass packets to the
muxer in a timebase as set by the muxer's init function. Solving this by
adding a muxer open flag which saves the preferred time base of the user
and rescales all packets from the user's preferred time base to the real
time base before processing seems much more managable than introducing the
AVPacket->time_base support everywhere and as far as I see it solves this
problem just the same.

Are there similar problems elsewhere? If there are, then is it not more 
managable to allow the user to specify a preferred input or output 
timebase during init instead of allowing per-packet timebases? By adding 
time_base to AVPacket you basically say that it is possible
that each packet of a stream have a different time base. But in realitly,
this never happens.

Regards,
Marton
Lynne June 3, 2021, 4:58 a.m. UTC | #6
Apr 26, 2021, 03:27 by andreas.rheinhardt@outlook.com:

> Lynne:
>
>> From 097aed2ac33dda0bb2052d8b0402711ce95079ba Mon Sep 17 00:00:00 2001
>> From: Lynne <dev@lynne.ee>
>> Date: Sat, 23 Jan 2021 19:56:18 +0100
>> Subject: [PATCH] avpacket: ABI bump additions
>>
>> ---
>>  libavcodec/avpacket.c |  5 +++++
>>  libavcodec/packet.h   | 21 +++++++++++++++++++++
>>  2 files changed, 26 insertions(+)
>>
>> diff --git a/libavcodec/avpacket.c b/libavcodec/avpacket.c
>> index e32c467586..03b73b3b53 100644
>> --- a/libavcodec/avpacket.c
>> +++ b/libavcodec/avpacket.c
>> @@ -382,6 +382,10 @@ int av_packet_copy_props(AVPacket *dst, const AVPacket *src)
>>  dst->flags                = src->flags;
>>  dst->stream_index         = src->stream_index;
>>  
>> +    i = av_buffer_replace(&dst->opaque_ref, src->opaque_ref);
>> +    if (i < 0)
>> +        return i;
>>
>
> 1. Don't use i here; add a new variable.
> 2. Up until now, av_packet_ref() and av_packet_copy_props() treat the
> destination packet as uninitialized and make no attempt at unreferencing
> its content; yet you try to reuse opaque_ref. Even worse, you might
> return potentially dangerous packets: If the properties were
> uninitialized and av_packet_copy_props() failed, then the caller were
> not allowed to unreference the packet even when the non-properties were
> set to sane values. The easiest way to fix this is to move setting
> opaque ref to the place after initializing side_data below and either
> set dst->opaque_ref to NULL before av_buffer_replace() or to not use
> av_buffer_replace(). It may also be best to unref it again if copying
> side data fails.
>
>> +
>>  dst->side_data            = NULL;
>>  dst->side_data_elems      = 0;
>>  for (i = 0; i < src->side_data_elems; i++) {
>> @@ -403,6 +407,7 @@ int av_packet_copy_props(AVPacket *dst, const AVPacket *src)
>>  void av_packet_unref(AVPacket *pkt)
>>  {
>>  av_packet_free_side_data(pkt);
>> +    av_buffer_unref(&pkt->opaque_ref);
>>  av_buffer_unref(&pkt->buf);
>>  get_packet_defaults(pkt);
>>  }
>> diff --git a/libavcodec/packet.h b/libavcodec/packet.h
>> index fad8341c12..c29ad18a2b 100644
>> --- a/libavcodec/packet.h
>> +++ b/libavcodec/packet.h
>> @@ -383,6 +383,27 @@ typedef struct AVPacket {
>>  int64_t duration;
>>  
>>  int64_t pos;                            ///< byte position in stream, -1 if unknown
>> +
>> +    /**
>> +     * for some private data of the user
>> +     */
>> +    void *opaque;
>>
>
> The corresponding AVFrame field is copied when copying props.
>

Fixed both, thanks. Also copied the time_base field and made av_init_packet()
initialize all fields.
Patch attached. Sorry it's taking me so long to work on this and the Vulkan ABI changes.
diff mbox series

Patch

diff --git a/libavcodec/avpacket.c b/libavcodec/avpacket.c
index e32c467586..03b73b3b53 100644
--- a/libavcodec/avpacket.c
+++ b/libavcodec/avpacket.c
@@ -382,6 +382,10 @@  int av_packet_copy_props(AVPacket *dst, const AVPacket *src)
     dst->flags                = src->flags;
     dst->stream_index         = src->stream_index;
 
+    i = av_buffer_replace(&dst->opaque_ref, src->opaque_ref);
+    if (i < 0)
+        return i;
+
     dst->side_data            = NULL;
     dst->side_data_elems      = 0;
     for (i = 0; i < src->side_data_elems; i++) {
@@ -403,6 +407,7 @@  int av_packet_copy_props(AVPacket *dst, const AVPacket *src)
 void av_packet_unref(AVPacket *pkt)
 {
     av_packet_free_side_data(pkt);
+    av_buffer_unref(&pkt->opaque_ref);
     av_buffer_unref(&pkt->buf);
     get_packet_defaults(pkt);
 }
diff --git a/libavcodec/packet.h b/libavcodec/packet.h
index fad8341c12..c29ad18a2b 100644
--- a/libavcodec/packet.h
+++ b/libavcodec/packet.h
@@ -383,6 +383,27 @@  typedef struct AVPacket {
     int64_t duration;
 
     int64_t pos;                            ///< byte position in stream, -1 if unknown
+
+    /**
+     * for some private data of the user
+     */
+    void *opaque;
+
+    /**
+     * AVBufferRef for free use by the API user. FFmpeg will never check the
+     * contents of the buffer ref. FFmpeg calls av_buffer_unref() on it when
+     * the packet is unreferenced. av_packet_copy_props() calls create a new
+     * reference with av_buffer_ref() for the target packet's opaque_ref field.
+     *
+     * This is unrelated to the opaque field, although it serves a similar
+     * purpose.
+     */
+    AVBufferRef *opaque_ref;
+
+    /**
+     * Time base of the packet's timestamps.
+     */
+    AVRational time_base;
 } AVPacket;
 
 #if FF_API_INIT_PACKET