[FFmpeg-devel] [PATCH 4/5] avcodec/parser: Schedule av_parser_change for removal

[Andreas Rheinhardt]

James Almer:
> On 3/2/2021 5:39 AM, Anton Khirnov wrote:
>> Quoting James Almer (2021-03-01 20:35:55)
>>> On 3/1/2021 12:36 PM, Andreas Rheinhardt wrote:
>>>> James Almer:
>>>>> On 2/26/2021 10:18 AM, Andreas Rheinhardt wrote:
>>>>>> Originally deprecated in 748c2fca7e4d99357c234936aa71212a6282be36,
>>>>>> publically deprecated in 9a07c1332cfe092b57b5758f22b686ca58806c60
>>>>>> (merged into FFmpeg in 1885ffb03d0af28e6bac2bcc8725fa15b93f6ac9).
>>>>>>
>>>>>> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt at gmail.com>
>>>>>> ---
>>>>>>     libavcodec/avcodec.h | 6 +++++-
>>>>>>     libavcodec/parser.c  | 3 ++-
>>>>>>     libavcodec/version.h | 3 +++
>>>>>>     3 files changed, 10 insertions(+), 2 deletions(-)
>>>>>>
>>>>>> diff --git a/libavcodec/avcodec.h b/libavcodec/avcodec.h
>>>>>> index 3178c163b9..a8741df04b 100644
>>>>>> --- a/libavcodec/avcodec.h
>>>>>> +++ b/libavcodec/avcodec.h
>>>>>> @@ -3576,14 +3576,18 @@ int av_parser_parse2(AVCodecParserContext *s,
>>>>>>                          int64_t pts, int64_t dts,
>>>>>>                          int64_t pos);
>>>>>>     +#if FF_API_PARSER_CHANGE
>>>>>>     /**
>>>>>>      * @return 0 if the output buffer is a subset of the input, 1
>>>>>> if it
>>>>>> is allocated and must be freed
>>>>>> - * @deprecated use AVBitStreamFilter
>>>>>> + * @deprecated Use dump_extradata, remove_extra or extract_extradata
>>>>>> + *             bitstream filters instead.
>>>>>>      */
>>>>>> +attribute_deprecated
>>>>>>     int av_parser_change(AVCodecParserContext *s,
>>>>>>                          AVCodecContext *avctx,
>>>>>>                          uint8_t **poutbuf, int *poutbuf_size,
>>>>>>                          const uint8_t *buf, int buf_size, int
>>>>>> keyframe);
>>>>>> +#endif
>>>>>>     void av_parser_close(AVCodecParserContext *s);
>>>>>>       /**
>>>>>> diff --git a/libavcodec/parser.c b/libavcodec/parser.c
>>>>>> index a63f532c48..f4bc00da7d 100644
>>>>>> --- a/libavcodec/parser.c
>>>>>> +++ b/libavcodec/parser.c
>>>>>> @@ -186,6 +186,7 @@ int av_parser_parse2(AVCodecParserContext *s,
>>>>>> AVCodecContext *avctx,
>>>>>>         return index;
>>>>>>     }
>>>>>>     +#if FF_API_PARSER_CHANGE
>>>>>>     int av_parser_change(AVCodecParserContext *s, AVCodecContext
>>>>>> *avctx,
>>>>>>                          uint8_t **poutbuf, int *poutbuf_size,
>>>>>>                          const uint8_t *buf, int buf_size, int
>>>>>> keyframe)
>>>>>> @@ -220,7 +221,7 @@ int av_parser_change(AVCodecParserContext *s,
>>>>>> AVCodecContext *avctx,
>>>>>>           return 0;
>>>>>>     }
>>>>>> -
>>>>>> +#endif
>>>>>>     void av_parser_close(AVCodecParserContext *s)
>>>>>>     {
>>>>>>         if (s) {
>>>>>> diff --git a/libavcodec/version.h b/libavcodec/version.h
>>>>>> index 488def4c23..815df15628 100644
>>>>>> --- a/libavcodec/version.h
>>>>>> +++ b/libavcodec/version.h
>>>>>> @@ -150,6 +150,9 @@
>>>>>>     #ifndef FF_API_MPV_RC_STRATEGY
>>>>>>     #define FF_API_MPV_RC_STRATEGY     (LIBAVCODEC_VERSION_MAJOR <
>>>>>> 59)
>>>>>>     #endif
>>>>>> +#ifndef FF_API_PARSER_CHANGE
>>>>>> +#define FF_API_PARSER_CHANGE       (LIBAVCODEC_VERSION_MAJOR < 59)
>>>>>
>>>>> A doxygen deprecation notice is not enough to consider the function
>>>>> deprecated. There was no APIChanges entry and no attribute added to
>>>>> the
>>>>> function prototype.
>>>>> So this needs the APIChanges entry, and therefore be scheduled as <
>>>>> 60.
>>>>
>>>> I disagree. Of course Doxygen deprecations count and being on the list
>>>> of deprecated things [1] for more than eight years is enough.
>>>
>>> If there was no APIChanges entry that references a library version, then
>>> this can not be considered deprecated
>>
>> I do not agree that an APIChanges entry is necessary for a deprecation.
>> APIChanges is for listing API changes. A deprecation is not by itself an
>> API change, merely a statement of intent wrt future API changes.
>>
>>  From the perspective of an API user it does not make much sense to ask
>> whether a feature X is deprecated in version Y, only whether a
>> replacement API is present. In this specific case, a replacement API has
>> been present since 2006 - for more than half the project's existence.
>> Surely that should be enough.
> 
> Yet the function was used in ffmpeg.c until 2018 when i removed it. It
> was dead code, but it was there and nobody noticed because it made no
> noise, so is it too crazy to think some project down there may still be
> using it because they were never nagged to make their code use the
> *_extradata bsfs?
> 
If someone didn't manage to read the documentation once every 15 years,
I don't have much pity for them.

> Look, I don't really care about this function, and it's so old and part
> of AVCodecParserContext (which nobody outside the project likes) that
> probably nobody is using it (Google seems to agree), so if you really

I also couldn't find any user of this function.

> want it gone now and if it really predates APIChanges then just nuke it.

Ok. But of course I won't nuke it now, only at the bump.

> But for future deprecations, an APIChanges entry and an attribute if
> applicable must be required, not just doxy. Doxy should mainly inform
> the developer of a potential replacement, or details about the
> deprecation (Like you wrote about what happens after
> thread_safe_callbacks is gone). APIChanges should be what keeps a
> history of additions and deprecations (Doxygen only keeps track of
> active deprecations). And of course the attribute so some actual noise
> is made.
> 
I agree that the deprecation process needs to be way better defined (as
long as the rules are only applied for future deprecations).
Just one suggestion in addition to what you said: For deprecated
AVOptions whose targets are ordinary users (not API users) it is enough
(and required) to add the AV_OPT_FLAG_DEPRECATED and to update the
documentation; an APIchanges entry should not be added as that would
just be noise for API users reading said file.

> Can we document this to prevent future pointless discussions about what
> is and what is not the correct process? And for the sake of everyone's
> sanity, don't summon the TC to solve a conflict about if we should add a
> line to two files or just one.

I won't summon the TC for this.

- Andreas

PS: I don't think it's only outside developers that don't like the
current parsing API. At least I don't like it either (it is not based
upon AVPackets, so it breaks side data; it is not refcounted).