[FFmpeg-devel] [RFC] Need an additional maintainer rule when deprecating functions

[Cyril Russo]

Le 04/11/2010 17:14, Anton Khirnov a ?crit :
> On Thu, Nov 04, 2010 at 04:42:02PM +0100, Cyril Russo wrote:
>> Hi all,
>>
>>    I would like to know if it's possible for those pushing
>> "attribute_deprecated" patch to be force to document the reason of
>> the change and mainly, what is the "new" way of doing the same thing
>> as the previous version.
>>
> You're misunderstanding, that patch didn't deprecate anything. It simply
> marked the stuff that _was already_ deprecated, to make transition to
> new APIs easier.
>
Yes, I'm speaking of the addition of attribute_deprecated that's quite 
recent (one or two week old at worst) and that does *add * a warning on 
the user code.
You are right that probably, from FFmpeg developer's point of view, some 
API are deprecated since a while, but from a user point of view, it's 
only when GCC says it, that we will consider it. And believe me, I'm not 
like 90% of devs that'll even wait until it's *actually removed* to react.

The point is that you now get a warning that says:
myfile.c:126: warning: ?something? is deprecated (declared at go_here.h:123)

And, logically, I'll go to "go_here.h" file at line 123, and try to 
figure out what is the good way to update my code.
Currently, there is no pointer whatsoever here (go_here.h:123), and 
that's the point of the discussion, that is, FORCE people to had a 
meaningful comment above like:
go_here.h
122 /* @deprecated Please use the function XYZ instead */
123 attribute_deprecated int something();

Or, at least:
go_here.h
122 /* @deprecated This is deprecated because of ... please refer to 
APIChange.txt by 24/09/10 */
123 attribute_deprecated int something();



>> As an example, recently, the sws_getContext was marked deprecated,
>> but there was not any comment whatsoever about what to use instead.
>> By digging in the source code, one could find sws_getCachedContext
>> but the name is a bit misleading since you might not have a context
>> at first and the documentation didn't say if it was valid to use a
>> NULL context.
>> The other solution is to do what is done in initial getContext
>> (initContext and dealing with the context members), but this means
>> more code in the user's code, with probably more bug too to all
>> projects using this code. When getContext will be removed, it'll be
>> very difficult for the end user to figure out how to even *start*
>> using the swscale library.
> A simple git log+search reveals
> commit 188585c88d0a62d90596847649d76ba6c4beb9f8
> Author: stefano<stefano at b3059339-0415-0410-9bf9-f77b7e298cf2>
> Date:   Tue Sep 28 22:23:58 2010 +0000
>
>      Deprecate sws_getContext(), use sws_alloc_context() and
>          sws_init_context() instead.
>
That's exactly what the @deprecated doxygen says too.
Does it answer the question "how do I do what getContext used to do ?". 
I don't think so, and I guess I'm not alone.
Looking at getContext code, it actually use alloc_context and 
init_context, but also the undocumented SwsContext members.
Please see the other thread where I'm tracking this and propose an idea 
to fix this.
>> Recently, AVFormatContext::genre was deprecated but again, no
>> pointer was added to the right "way" to get the genre's metadata.
> Searching for 'genre' in doc/APIChanges shows that a new metadata API
> was introduced 1.5 yrs ago and the old one was deprecated at the same
> time. Alternatively searching for 'genre' in libavformat/avformat.h
> would tell you how exactly to use the new API.
I know about the metadata API, it's not the point. The point is how do I 
replace my old code to fit the new code.
Said differently, how am I supposed to figure about the metadata API at 
first ?
Wouldn't it be better if the additional comment was here:
/** @deprecated Please use metadata API and specially av_metadata_get 
with "genre"  */

Cheers,
Cyril