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

[Marton Balint]

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