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

[James Almer]

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.