[FFmpeg-devel] [PATCH 2/2] avio: do not export avpriv_io_{move, delete}

[James Almer]

On 1/10/2020 11:30 AM, Anton Khirnov wrote:
> Quoting James Almer (2020-01-10 14:33:06)
>> On 1/10/2020 8:21 AM, Anton Khirnov wrote:
>>> They are private and not used by anything outside of lavf. There is no
>>> reason for them to be exported.
>>> ---
>>>  libavformat/avio.c    |  4 ++--
>>>  libavformat/avio.h    | 19 -------------------
>>>  libavformat/dashenc.c | 10 +++++-----
>>>  libavformat/url.h     | 20 ++++++++++++++++++++
>>>  4 files changed, 27 insertions(+), 26 deletions(-)
>>>
>>> diff --git a/libavformat/avio.c b/libavformat/avio.c
>>> index 2dd2312296..3e390fe719 100644
>>> --- a/libavformat/avio.c
>>> +++ b/libavformat/avio.c
>>> @@ -494,7 +494,7 @@ int avio_check(const char *url, int flags)
>>>      return ret;
>>>  }
>>>  
>>> -int avpriv_io_move(const char *url_src, const char *url_dst)
>>> +int ffurl_move(const char *url_src, const char *url_dst)
>>>  {
>>>      URLContext *h_src, *h_dst;
>>>      int ret = ffurl_alloc(&h_src, url_src, AVIO_FLAG_READ_WRITE, NULL);
>>> @@ -516,7 +516,7 @@ int avpriv_io_move(const char *url_src, const char *url_dst)
>>>      return ret;
>>>  }
>>>  
>>> -int avpriv_io_delete(const char *url)
>>> +int ffurl_delete(const char *url)
>>>  {
>>>      URLContext *h;
>>>      int ret = ffurl_alloc(&h, url, AVIO_FLAG_WRITE, NULL);
>>> diff --git a/libavformat/avio.h b/libavformat/avio.h
>>> index 9141642e75..34c5957791 100644
>>> --- a/libavformat/avio.h
>>> +++ b/libavformat/avio.h
>>> @@ -374,25 +374,6 @@ const char *avio_find_protocol_name(const char *url);
>>>   */
>>>  int avio_check(const char *url, int flags);
>>>  
>>> -/**
>>> - * Move or rename a resource.
>>> - *
>>> - * @note url_src and url_dst should share the same protocol and authority.
>>> - *
>>> - * @param url_src url to resource to be moved
>>> - * @param url_dst new url to resource if the operation succeeded
>>> - * @return >=0 on success or negative on error.
>>> - */
>>> -int avpriv_io_move(const char *url_src, const char *url_dst);
>>> -
>>> -/**
>>> - * Delete a resource.
>>> - *
>>> - * @param url resource to be deleted.
>>> - * @return >=0 on success or negative on error.
>>> - */
>>> -int avpriv_io_delete(const char *url);
>>
>> No, unfortunately and despite the name, these are public. Or rather,
>> exposed in a public header when they were not meant to be public,
>> afaics, so what we can do instead is schedule them for removal in the
>> next bump and not bother with a two year deprecation period. We've done
>> it before for other avpriv_ functions mistakenly exposed in public headers.
> 
> There's (sadly) plenty of stuff in public headers that is not meant to
> be public. For example, the FF_API_* macros, the internal fields of
> structs like AVCodec, etc. IMO we should not take into consideration
> people using private APIs, otherwise we legitimize such behavior. Anyone
> using a private function gets to keep all the pieces when we break it.

Yes, i agree with this if we were talking about clearly marked
non-public structs, fields and defines. In those cases, people using
them should be aware of the consequences. But these are two five years
old documented functions in a public header (Although admittedly not
announced in APIChanges) and even featured in an API example tool.
I don't think the avpriv_ prefix is something we documented outside of
the developer guidelines, so anyone reading avio.h and the example will
have no reason to think these were there by mistake.

You could replace the doxy with a "do not use" notice aside from
wrapping them in a scheduled removal preprocessor check, but at the very
least the symbols should not be removed until the next major bump.

If others agree with you though, I'll not block this.

> 
>>
>> For that matter, it's about time we do a major bump. I'm willing to help
>> doing the required work to properly remove deprecated stuff that i'm
>> sure can't be deleted as is.
> 
> That said, I am certainly not against a major bump.