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

[James Almer]

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?