diff mbox series

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

Message ID 20210305163339.63164-2-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_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 March 5, 2021, 4:32 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    | 22 ++++++++++++++++++----
 libavcodec/version.h   |  5 ++++-
 libavformat/avformat.h |  4 ++++
 4 files changed, 41 insertions(+), 13 deletions(-)

Comments

Anton Khirnov March 21, 2021, 12:28 p.m. UTC | #1
Quoting James Almer (2021-03-05 17:32:52)
> diff --git a/libavformat/avformat.h b/libavformat/avformat.h
> index 7da2f3d98e..783cc1b591 100644
> --- a/libavformat/avformat.h
> +++ b/libavformat/avformat.h
> @@ -954,7 +954,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

Sorry I'm late to the party, but as we are changing the type of an
existing field, we need to explicitly spell out a way for the callers to
make their code forward compatible. E.g. similarly to what I made for
thread_safe_callbacks deprecation.
James Almer March 21, 2021, 1:54 p.m. UTC | #2
On 3/21/2021 9:28 AM, Anton Khirnov wrote:
> Quoting James Almer (2021-03-05 17:32:52)
>> diff --git a/libavformat/avformat.h b/libavformat/avformat.h
>> index 7da2f3d98e..783cc1b591 100644
>> --- a/libavformat/avformat.h
>> +++ b/libavformat/avformat.h
>> @@ -954,7 +954,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
> 
> Sorry I'm late to the party, but as we are changing the type of an
> existing field, we need to explicitly spell out a way for the callers to
> make their code forward compatible. E.g. similarly to what I made for
> thread_safe_callbacks deprecation.

What do you suggest? The field is not printing deprecation warnings, so 
would a doxy comment be enough for people to notice it?
Have we done a similar change before? I know at least for symbols like 
public functions we never change their signature in an incompatible way, 
and instead replace them altogether.

Maybe we could add the deprecated attribute to attached_pic, introduce a 
temporary getter function that returns a pointer to the packet, mention 
in the doxy that the field is not going away, is just changing types, 
and they have the option of using the getter for a fire-and-forget 
migration process, then maybe deprecate and eventually remove the getter 
after the switch is done.
Anton Khirnov March 21, 2021, 2:16 p.m. UTC | #3
Quoting James Almer (2021-03-21 14:54:22)
> On 3/21/2021 9:28 AM, Anton Khirnov wrote:
> > Quoting James Almer (2021-03-05 17:32:52)
> >> diff --git a/libavformat/avformat.h b/libavformat/avformat.h
> >> index 7da2f3d98e..783cc1b591 100644
> >> --- a/libavformat/avformat.h
> >> +++ b/libavformat/avformat.h
> >> @@ -954,7 +954,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
> > 
> > Sorry I'm late to the party, but as we are changing the type of an
> > existing field, we need to explicitly spell out a way for the callers to
> > make their code forward compatible. E.g. similarly to what I made for
> > thread_safe_callbacks deprecation.
> 
> What do you suggest? The field is not printing deprecation warnings, so 
> would a doxy comment be enough for people to notice it?
> Have we done a similar change before? I know at least for symbols like 
> public functions we never change their signature in an incompatible way, 
> and instead replace them altogether.
> 
> Maybe we could add the deprecated attribute to attached_pic, introduce a 
> temporary getter function that returns a pointer to the packet, mention 
> in the doxy that the field is not going away, is just changing types, 
> and they have the option of using the getter for a fire-and-forget 
> migration process, then maybe deprecate and eventually remove the getter 
> after the switch is done.

This seems like a lot of hoops to jump through. Wouldn't it then be
better to just add a new field with a new name?
James Almer March 21, 2021, 2:24 p.m. UTC | #4
On 3/21/2021 11:16 AM, Anton Khirnov wrote:
> Quoting James Almer (2021-03-21 14:54:22)
>> On 3/21/2021 9:28 AM, Anton Khirnov wrote:
>>> Quoting James Almer (2021-03-05 17:32:52)
>>>> diff --git a/libavformat/avformat.h b/libavformat/avformat.h
>>>> index 7da2f3d98e..783cc1b591 100644
>>>> --- a/libavformat/avformat.h
>>>> +++ b/libavformat/avformat.h
>>>> @@ -954,7 +954,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
>>>
>>> Sorry I'm late to the party, but as we are changing the type of an
>>> existing field, we need to explicitly spell out a way for the callers to
>>> make their code forward compatible. E.g. similarly to what I made for
>>> thread_safe_callbacks deprecation.
>>
>> What do you suggest? The field is not printing deprecation warnings, so
>> would a doxy comment be enough for people to notice it?
>> Have we done a similar change before? I know at least for symbols like
>> public functions we never change their signature in an incompatible way,
>> and instead replace them altogether.
>>
>> Maybe we could add the deprecated attribute to attached_pic, introduce a
>> temporary getter function that returns a pointer to the packet, mention
>> in the doxy that the field is not going away, is just changing types,
>> and they have the option of using the getter for a fire-and-forget
>> migration process, then maybe deprecate and eventually remove the getter
>> after the switch is done.
> 
> This seems like a lot of hoops to jump through. Wouldn't it then be
> better to just add a new field with a new name?

A name change just because it's now a pointer seems overkill. Library 
users that want to support both lavc 59 and 60 will have to add 
preprocessor checks to their code anyway, so using the same name or 
another will not make any difference in that regard. Only a getter would 
prevent ifdeffery, but i agree it's also annoying, and we're about to 
get rid of a dozen of those in the upcoming bump. It also couldn't be 
added to 4.4 since that was already branched out.

How about adding the deprecation attribute to prompt people to read the 
doxy, where we state the field is not going away, just changing types? 
Otherwise i don't think people will notice.
Anton Khirnov March 21, 2021, 2:28 p.m. UTC | #5
Quoting James Almer (2021-03-21 15:24:35)
> On 3/21/2021 11:16 AM, Anton Khirnov wrote:
> > Quoting James Almer (2021-03-21 14:54:22)
> >> On 3/21/2021 9:28 AM, Anton Khirnov wrote:
> >>> Quoting James Almer (2021-03-05 17:32:52)
> >>>> diff --git a/libavformat/avformat.h b/libavformat/avformat.h
> >>>> index 7da2f3d98e..783cc1b591 100644
> >>>> --- a/libavformat/avformat.h
> >>>> +++ b/libavformat/avformat.h
> >>>> @@ -954,7 +954,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
> >>>
> >>> Sorry I'm late to the party, but as we are changing the type of an
> >>> existing field, we need to explicitly spell out a way for the callers to
> >>> make their code forward compatible. E.g. similarly to what I made for
> >>> thread_safe_callbacks deprecation.
> >>
> >> What do you suggest? The field is not printing deprecation warnings, so
> >> would a doxy comment be enough for people to notice it?
> >> Have we done a similar change before? I know at least for symbols like
> >> public functions we never change their signature in an incompatible way,
> >> and instead replace them altogether.
> >>
> >> Maybe we could add the deprecated attribute to attached_pic, introduce a
> >> temporary getter function that returns a pointer to the packet, mention
> >> in the doxy that the field is not going away, is just changing types,
> >> and they have the option of using the getter for a fire-and-forget
> >> migration process, then maybe deprecate and eventually remove the getter
> >> after the switch is done.
> > 
> > This seems like a lot of hoops to jump through. Wouldn't it then be
> > better to just add a new field with a new name?
> 
> A name change just because it's now a pointer seems overkill. Library 
> users that want to support both lavc 59 and 60 will have to add 
> preprocessor checks to their code anyway, so using the same name or 
> another will not make any difference in that regard. Only a getter would 
> prevent ifdeffery, but i agree it's also annoying, and we're about to 
> get rid of a dozen of those in the upcoming bump. It also couldn't be 
> added to 4.4 since that was already branched out.
> 
> How about adding the deprecation attribute to prompt people to read the 
> doxy, where we state the field is not going away, just changing types? 
> Otherwise i don't think people will notice.

That is also an acceptable solution.
Marton Balint March 21, 2021, 6:15 p.m. UTC | #6
On Sun, 21 Mar 2021, James Almer wrote:

> On 3/21/2021 11:16 AM, Anton Khirnov wrote:
>> Quoting James Almer (2021-03-21 14:54:22)
>>> On 3/21/2021 9:28 AM, Anton Khirnov wrote:
>>>> Quoting James Almer (2021-03-05 17:32:52)
>>>>> diff --git a/libavformat/avformat.h b/libavformat/avformat.h
>>>>> index 7da2f3d98e..783cc1b591 100644
>>>>> --- a/libavformat/avformat.h
>>>>> +++ b/libavformat/avformat.h
>>>>> @@ -954,7 +954,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
>>>>
>>>> Sorry I'm late to the party, but as we are changing the type of an
>>>> existing field, we need to explicitly spell out a way for the callers to
>>>> make their code forward compatible. E.g. similarly to what I made for
>>>> thread_safe_callbacks deprecation.
>>>
>>> What do you suggest? The field is not printing deprecation warnings, so
>>> would a doxy comment be enough for people to notice it?
>>> Have we done a similar change before? I know at least for symbols like
>>> public functions we never change their signature in an incompatible way,
>>> and instead replace them altogether.
>>>
>>> Maybe we could add the deprecated attribute to attached_pic, introduce a
>>> temporary getter function that returns a pointer to the packet, mention
>>> in the doxy that the field is not going away, is just changing types,
>>> and they have the option of using the getter for a fire-and-forget
>>> migration process, then maybe deprecate and eventually remove the getter
>>> after the switch is done.
>> 
>> This seems like a lot of hoops to jump through. Wouldn't it then be
>> better to just add a new field with a new name?
>
> A name change just because it's now a pointer seems overkill.

I don't think it is. A name change guarantees that existing code won't 
compile to something that will surely crash. IMHO the clean solution is to 
keep the old field with deprecation, and add a new field. Same what I did 
with AVFormat->filename / AVFormat->url.

Regards,
Marton
Nicolas George March 21, 2021, 6:25 p.m. UTC | #7
Marton Balint (12021-03-21):
> Same what I did with AVFormat->filename / AVFormat->url.

By the way, about that: at some point, we will need to clarify what part
of our API uses real URLs and what parts uses fake URLs.

Because as it is, a lot of code looks like it uses URLs but really it
does not. The worst offender is "file:" it looks like an URL, the prefix
is a 100% standard URL scheme. Yet, it will not find
file:///home/user/Some%20Movie.mkv because it does not obey to the
standard syntax of file:// URLs.

I had the vague intent of proposing a migration path, but never time to
work on it. It involves:

1. Introduce the fs: protocol as a synonym for file:.

2. For some time, detect if the behavior is likely to change in the next
   step and warn the user.

3. Detect file:// and treat it like a standard file:// URL, including
   skipping the server, query and fragment part and performing
   %-unescaping; keep file: without the double slash as an alias for
   fs:.

4. For some more time, warn users if they are using file: but not
   file://.

5. Remove the compatibility file: → fs: alias.

Regards,
James Almer March 21, 2021, 6:37 p.m. UTC | #8
On 3/21/2021 3:15 PM, Marton Balint wrote:
> 
> 
> On Sun, 21 Mar 2021, James Almer wrote:
> 
>> On 3/21/2021 11:16 AM, Anton Khirnov wrote:
>>> Quoting James Almer (2021-03-21 14:54:22)
>>>> On 3/21/2021 9:28 AM, Anton Khirnov wrote:
>>>>> Quoting James Almer (2021-03-05 17:32:52)
>>>>>> diff --git a/libavformat/avformat.h b/libavformat/avformat.h
>>>>>> index 7da2f3d98e..783cc1b591 100644
>>>>>> --- a/libavformat/avformat.h
>>>>>> +++ b/libavformat/avformat.h
>>>>>> @@ -954,7 +954,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
>>>>>
>>>>> Sorry I'm late to the party, but as we are changing the type of an
>>>>> existing field, we need to explicitly spell out a way for the 
>>>>> callers to
>>>>> make their code forward compatible. E.g. similarly to what I made for
>>>>> thread_safe_callbacks deprecation.
>>>>
>>>> What do you suggest? The field is not printing deprecation warnings, so
>>>> would a doxy comment be enough for people to notice it?
>>>> Have we done a similar change before? I know at least for symbols like
>>>> public functions we never change their signature in an incompatible 
>>>> way,
>>>> and instead replace them altogether.
>>>>
>>>> Maybe we could add the deprecated attribute to attached_pic, 
>>>> introduce a
>>>> temporary getter function that returns a pointer to the packet, mention
>>>> in the doxy that the field is not going away, is just changing types,
>>>> and they have the option of using the getter for a fire-and-forget
>>>> migration process, then maybe deprecate and eventually remove the 
>>>> getter
>>>> after the switch is done.
>>>
>>> This seems like a lot of hoops to jump through. Wouldn't it then be
>>> better to just add a new field with a new name?
>>
>> A name change just because it's now a pointer seems overkill.
> 
> I don't think it is. A name change guarantees that existing code won't 
> compile to something that will surely crash. IMHO the clean solution is 
> to keep the old field with deprecation, and add a new field. Same what I 
> did with AVFormat->filename / AVFormat->url.

Compilation will fail if your code doesn't treat the field as a pointer 
after the switch, printing something like

error: 'st->attached_pic' is a pointer; did you mean to use '->'?
   123 |     st->attached_pic.flags |= AV_PKT_FLAG_KEY;
       |                     ^

So it's not like it will compile and then crash at runtime by accessing 
the wrong thing.

But if you still feel more inclined to replace the field, can you 
suggest a name for the new one?
Nicolas George March 21, 2021, 6:40 p.m. UTC | #9
Nicolas George (12021-03-21):
> By the way, about that: at some point, we will need to clarify what part
> of our API uses real URLs and what parts uses fake URLs.
> 
> Because as it is, a lot of code looks like it uses URLs but really it
> does not. The worst offender is "file:" it looks like an URL, the prefix
> is a 100% standard URL scheme. Yet, it will not find
> file:///home/user/Some%20Movie.mkv because it does not obey to the
> standard syntax of file:// URLs.

This is now Trac ticket #9157:

https://trac.ffmpeg.org/ticket/9157

Note that it is a rather simple task for somebody who wants to get
involved in the project.

Regards,
James Almer March 21, 2021, 7:08 p.m. UTC | #10
On 3/21/2021 3:37 PM, James Almer wrote:
> On 3/21/2021 3:15 PM, Marton Balint wrote:
>>
>>
>> On Sun, 21 Mar 2021, James Almer wrote:
>>
>>> On 3/21/2021 11:16 AM, Anton Khirnov wrote:
>>>> Quoting James Almer (2021-03-21 14:54:22)
>>>>> On 3/21/2021 9:28 AM, Anton Khirnov wrote:
>>>>>> Quoting James Almer (2021-03-05 17:32:52)
>>>>>>> diff --git a/libavformat/avformat.h b/libavformat/avformat.h
>>>>>>> index 7da2f3d98e..783cc1b591 100644
>>>>>>> --- a/libavformat/avformat.h
>>>>>>> +++ b/libavformat/avformat.h
>>>>>>> @@ -954,7 +954,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
>>>>>>
>>>>>> Sorry I'm late to the party, but as we are changing the type of an
>>>>>> existing field, we need to explicitly spell out a way for the 
>>>>>> callers to
>>>>>> make their code forward compatible. E.g. similarly to what I made for
>>>>>> thread_safe_callbacks deprecation.
>>>>>
>>>>> What do you suggest? The field is not printing deprecation 
>>>>> warnings, so
>>>>> would a doxy comment be enough for people to notice it?
>>>>> Have we done a similar change before? I know at least for symbols like
>>>>> public functions we never change their signature in an incompatible 
>>>>> way,
>>>>> and instead replace them altogether.
>>>>>
>>>>> Maybe we could add the deprecated attribute to attached_pic, 
>>>>> introduce a
>>>>> temporary getter function that returns a pointer to the packet, 
>>>>> mention
>>>>> in the doxy that the field is not going away, is just changing types,
>>>>> and they have the option of using the getter for a fire-and-forget
>>>>> migration process, then maybe deprecate and eventually remove the 
>>>>> getter
>>>>> after the switch is done.
>>>>
>>>> This seems like a lot of hoops to jump through. Wouldn't it then be
>>>> better to just add a new field with a new name?
>>>
>>> A name change just because it's now a pointer seems overkill.
>>
>> I don't think it is. A name change guarantees that existing code won't 
>> compile to something that will surely crash. IMHO the clean solution 
>> is to keep the old field with deprecation, and add a new field. Same 
>> what I did with AVFormat->filename / AVFormat->url.
> 
> Compilation will fail if your code doesn't treat the field as a pointer 
> after the switch, printing something like
> 
> error: 'st->attached_pic' is a pointer; did you mean to use '->'?
>    123 |     st->attached_pic.flags |= AV_PKT_FLAG_KEY;
>        |                     ^
> 
> So it's not like it will compile and then crash at runtime by accessing 
> the wrong thing.
> 
> But if you still feel more inclined to replace the field, can you 
> suggest a name for the new one?

Also, i wont be able to add a new field until after the bump because of 
the libavdevice franken-ABI situation.
James Almer March 21, 2021, 8:01 p.m. UTC | #11
On 3/21/2021 3:37 PM, James Almer wrote:
> On 3/21/2021 3:15 PM, Marton Balint wrote:
>>
>>
>> On Sun, 21 Mar 2021, James Almer wrote:
>>
>>> On 3/21/2021 11:16 AM, Anton Khirnov wrote:
>>>> Quoting James Almer (2021-03-21 14:54:22)
>>>>> On 3/21/2021 9:28 AM, Anton Khirnov wrote:
>>>>>> Quoting James Almer (2021-03-05 17:32:52)
>>>>>>> diff --git a/libavformat/avformat.h b/libavformat/avformat.h
>>>>>>> index 7da2f3d98e..783cc1b591 100644
>>>>>>> --- a/libavformat/avformat.h
>>>>>>> +++ b/libavformat/avformat.h
>>>>>>> @@ -954,7 +954,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
>>>>>>
>>>>>> Sorry I'm late to the party, but as we are changing the type of an
>>>>>> existing field, we need to explicitly spell out a way for the 
>>>>>> callers to
>>>>>> make their code forward compatible. E.g. similarly to what I made for
>>>>>> thread_safe_callbacks deprecation.
>>>>>
>>>>> What do you suggest? The field is not printing deprecation 
>>>>> warnings, so
>>>>> would a doxy comment be enough for people to notice it?
>>>>> Have we done a similar change before? I know at least for symbols like
>>>>> public functions we never change their signature in an incompatible 
>>>>> way,
>>>>> and instead replace them altogether.
>>>>>
>>>>> Maybe we could add the deprecated attribute to attached_pic, 
>>>>> introduce a
>>>>> temporary getter function that returns a pointer to the packet, 
>>>>> mention
>>>>> in the doxy that the field is not going away, is just changing types,
>>>>> and they have the option of using the getter for a fire-and-forget
>>>>> migration process, then maybe deprecate and eventually remove the 
>>>>> getter
>>>>> after the switch is done.
>>>>
>>>> This seems like a lot of hoops to jump through. Wouldn't it then be
>>>> better to just add a new field with a new name?
>>>
>>> A name change just because it's now a pointer seems overkill.
>>
>> I don't think it is. A name change guarantees that existing code won't 
>> compile to something that will surely crash. IMHO the clean solution 
>> is to keep the old field with deprecation, and add a new field. Same 
>> what I did with AVFormat->filename / AVFormat->url.
> 
> Compilation will fail if your code doesn't treat the field as a pointer 
> after the switch, printing something like
> 
> error: 'st->attached_pic' is a pointer; did you mean to use '->'?
>    123 |     st->attached_pic.flags |= AV_PKT_FLAG_KEY;
>        |                     ^
> 
> So it's not like it will compile and then crash at runtime by accessing 
> the wrong thing.

After looking a bit more, it will fail if you try to access fields like 
i mentioned above, but if you pass it as argument to some function it 
will only warn about mismatching types.

So i guess yeah, a new field with a new name would be the cleanest 
solution. So as i said, field name suggestions welcome.

> 
> But if you still feel more inclined to replace the field, can you 
> suggest a name for the new one?
Marton Balint March 21, 2021, 8:46 p.m. UTC | #12
On Sun, 21 Mar 2021, James Almer wrote:

> On 3/21/2021 3:37 PM, James Almer wrote:
>> On 3/21/2021 3:15 PM, Marton Balint wrote:
>>>
>>>
>>> On Sun, 21 Mar 2021, James Almer wrote:
>>>
>>>> On 3/21/2021 11:16 AM, Anton Khirnov wrote:
>>>>> Quoting James Almer (2021-03-21 14:54:22)
>>>>>> On 3/21/2021 9:28 AM, Anton Khirnov wrote:
>>>>>>> Quoting James Almer (2021-03-05 17:32:52)
>>>>>>>> diff --git a/libavformat/avformat.h b/libavformat/avformat.h
>>>>>>>> index 7da2f3d98e..783cc1b591 100644
>>>>>>>> --- a/libavformat/avformat.h
>>>>>>>> +++ b/libavformat/avformat.h
>>>>>>>> @@ -954,7 +954,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
>>>>>>>
>>>>>>> Sorry I'm late to the party, but as we are changing the type of an
>>>>>>> existing field, we need to explicitly spell out a way for the 
>>>>>>> callers to
>>>>>>> make their code forward compatible. E.g. similarly to what I made for
>>>>>>> thread_safe_callbacks deprecation.
>>>>>>
>>>>>> What do you suggest? The field is not printing deprecation 
>>>>>> warnings, so
>>>>>> would a doxy comment be enough for people to notice it?
>>>>>> Have we done a similar change before? I know at least for symbols like
>>>>>> public functions we never change their signature in an incompatible 
>>>>>> way,
>>>>>> and instead replace them altogether.
>>>>>>
>>>>>> Maybe we could add the deprecated attribute to attached_pic, 
>>>>>> introduce a
>>>>>> temporary getter function that returns a pointer to the packet, 
>>>>>> mention
>>>>>> in the doxy that the field is not going away, is just changing types,
>>>>>> and they have the option of using the getter for a fire-and-forget
>>>>>> migration process, then maybe deprecate and eventually remove the 
>>>>>> getter
>>>>>> after the switch is done.
>>>>>
>>>>> This seems like a lot of hoops to jump through. Wouldn't it then be
>>>>> better to just add a new field with a new name?
>>>>
>>>> A name change just because it's now a pointer seems overkill.
>>>
>>> I don't think it is. A name change guarantees that existing code won't 
>>> compile to something that will surely crash. IMHO the clean solution 
>>> is to keep the old field with deprecation, and add a new field. Same 
>>> what I did with AVFormat->filename / AVFormat->url.
>> 
>> Compilation will fail if your code doesn't treat the field as a pointer 
>> after the switch, printing something like
>> 
>> error: 'st->attached_pic' is a pointer; did you mean to use '->'?
>>    123 |     st->attached_pic.flags |= AV_PKT_FLAG_KEY;
>>        |                     ^
>> 
>> So it's not like it will compile and then crash at runtime by accessing 
>> the wrong thing.
>
> After looking a bit more, it will fail if you try to access fields like 
> i mentioned above, but if you pass it as argument to some function it 
> will only warn about mismatching types.
>
> So i guess yeah, a new field with a new name would be the cleanest 
> solution. So as i said, field name suggestions welcome.

attached_pic_pkt or attached_pkt maybe.

Regards,
Marton
Hendrik Leppkes March 21, 2021, 11:13 p.m. UTC | #13
On Sun, Mar 21, 2021 at 3:24 PM James Almer <jamrial@gmail.com> wrote:
>
> How about adding the deprecation attribute to prompt people to read the
> doxy, where we state the field is not going away, just changing types?
> Otherwise i don't think people will notice.

I really don't like such solutions. You get warning spam in user code
with basically no way to remedy them.

- Hendrik
James Almer March 22, 2021, 12:48 a.m. UTC | #14
On 3/21/2021 8:13 PM, Hendrik Leppkes wrote:
> On Sun, Mar 21, 2021 at 3:24 PM James Almer <jamrial@gmail.com> wrote:
>>
>> How about adding the deprecation attribute to prompt people to read the
>> doxy, where we state the field is not going away, just changing types?
>> Otherwise i don't think people will notice.
> 
> I really don't like such solutions. You get warning spam in user code
> with basically no way to remedy them.
> 
> - Hendrik

I'm going to add a new field in the end. A change of type can't ensure 
projects that don't migrate stop compiling (Like it happens when a field 
is removed), and instead will unexpectedly crash if they try to access 
it the wrong way.

But i can't do it until after the bump because libavdevice is locking 
every AVStream field offset.
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..f07ca4936c 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
  */
@@ -393,10 +395,13 @@  typedef struct AVPacket {
 #endif
 } AVPacket;
 
+#if FF_API_INIT_PACKET
+attribute_deprecated
 typedef struct AVPacketList {
     AVPacket pkt;
     struct AVPacketList *next;
 } AVPacketList;
+#endif
 
 #define AV_PKT_FLAG_KEY     0x0001 ///< The packet contains a keyframe
 #define AV_PKT_FLAG_CORRUPT 0x0002 ///< The packet content is corrupted
@@ -460,6 +465,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 +473,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 2b3757fa07..163dbc1cbf 100644
--- a/libavcodec/version.h
+++ b/libavcodec/version.h
@@ -28,7 +28,7 @@ 
 #include "libavutil/version.h"
 
 #define LIBAVCODEC_VERSION_MAJOR  58
-#define LIBAVCODEC_VERSION_MINOR 128
+#define LIBAVCODEC_VERSION_MINOR 129
 #define LIBAVCODEC_VERSION_MICRO 100
 
 #define LIBAVCODEC_VERSION_INT  AV_VERSION_INT(LIBAVCODEC_VERSION_MAJOR, \
@@ -159,5 +159,8 @@ 
 #ifndef FF_API_GET_FRAME_CLASS
 #define FF_API_GET_FRAME_CLASS     (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 7da2f3d98e..783cc1b591 100644
--- a/libavformat/avformat.h
+++ b/libavformat/avformat.h
@@ -954,7 +954,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