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

[Anton Khirnov]

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.

-- 
Anton Khirnov