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

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(-)

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.

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.

> -
>  /**
>   * Open directory for reading.
>   *
> diff --git a/libavformat/dashenc.c b/libavformat/dashenc.c
> index b84736881f..46041337c2 100644
> --- a/libavformat/dashenc.c
> +++ b/libavformat/dashenc.c
> @@ -540,7 +540,7 @@ static void write_hls_media_playlist(OutputStream *os, AVFormatContext *s,
>      dashenc_io_close(s, &c->m3u8_out, temp_filename_hls);
>  
>      if (use_rename)
> -        if (avpriv_io_move(temp_filename_hls, filename_hls) < 0) {
> +        if (ffurl_move(temp_filename_hls, filename_hls) < 0) {
>              av_log(os->ctx, AV_LOG_WARNING, "renaming file %s to %s failed\n\n", temp_filename_hls, filename_hls);
>          }
>  }
> @@ -1037,7 +1037,7 @@ static int write_manifest(AVFormatContext *s, int final)
>      dashenc_io_close(s, &c->mpd_out, temp_filename);
>  
>      if (use_rename) {
> -        if ((ret = avpriv_io_move(temp_filename, s->url)) < 0)
> +        if ((ret = ffurl_move(temp_filename, s->url)) < 0)
>              return ret;
>      }
>  
> @@ -1119,7 +1119,7 @@ static int write_manifest(AVFormatContext *s, int final)
>          }
>          dashenc_io_close(s, &c->m3u8_out, temp_filename);
>          if (use_rename)
> -            if ((ret = avpriv_io_move(temp_filename, filename_hls)) < 0)
> +            if ((ret = ffurl_move(temp_filename, filename_hls)) < 0)
>                  return ret;
>          c->master_playlist_created = 1;
>      }
> @@ -1507,7 +1507,7 @@ static void dashenc_delete_file(AVFormatContext *s, char *filename) {
>          av_dict_free(&http_opts);
>          ff_format_io_close(s, &out);
>      } else {
> -        int res = avpriv_io_delete(filename);
> +        int res = ffurl_delete(filename);
>          if (res < 0) {
>              char errbuf[AV_ERROR_MAX_STRING_SIZE];
>              av_strerror(res, errbuf, sizeof(errbuf));
> @@ -1619,7 +1619,7 @@ static int dash_flush(AVFormatContext *s, int final, int stream)
>              dashenc_io_close(s, &os->out, os->temp_path);
>  
>              if (use_rename) {
> -                ret = avpriv_io_move(os->temp_path, os->full_path);
> +                ret = ffurl_move(os->temp_path, os->full_path);
>                  if (ret < 0)
>                      break;
>              }
> diff --git a/libavformat/url.h b/libavformat/url.h
> index 4750bfff82..98ac6e3ccd 100644
> --- a/libavformat/url.h
> +++ b/libavformat/url.h
> @@ -340,4 +340,24 @@ const AVClass *ff_urlcontext_child_class_next(const AVClass *prev);
>  const URLProtocol **ffurl_get_protocols(const char *whitelist,
>                                          const char *blacklist);
>  
> +/**
> + * 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 ffurl_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 ffurl_delete(const char *url);
> +
> +
>  #endif /* AVFORMAT_URL_H */
>

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.

> 
> 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.

On Fri, 10 Jan 2020, 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.
>
>> 
>> 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.

Yes, please wait with these till the bump. A release should be made before 
the bump though.

Regards,
Marton

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.

On Fri, Jan 10, 2020 at 7:31 PM James Almer <jamrial@gmail.com> wrote:
>
> 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.
>

If we plan to do a bump relatively soon anyway, then there is no
reason to really rush anything, and just let them in until such a time
that there is a bump. Considering their only not-really-public nature
we can forego the 2 year deprecation and just throw them out when we
open the ABI.

- Hendrik

Quoting Hendrik Leppkes (2020-01-10 23:40:37)
> On Fri, Jan 10, 2020 at 7:31 PM James Almer <jamrial@gmail.com> wrote:
> > 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.
> >
> 
> If we plan to do a bump relatively soon anyway, then there is no
> reason to really rush anything, and just let them in until such a time
> that there is a bump. Considering their only not-really-public nature
> we can forego the 2 year deprecation and just throw them out when we
> open the ABI.

Ok, I can wait with this patch until the bump if it's going to happen
soon.

Quoting Anton Khirnov (2020-01-13 11:33:01)
> Quoting Hendrik Leppkes (2020-01-10 23:40:37)
> > On Fri, Jan 10, 2020 at 7:31 PM James Almer <jamrial@gmail.com> wrote:
> > > 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.
> > >
> > 
> > If we plan to do a bump relatively soon anyway, then there is no
> > reason to really rush anything, and just let them in until such a time
> > that there is a bump. Considering their only not-really-public nature
> > we can forego the 2 year deprecation and just throw them out when we
> > open the ABI.
> 
> Ok, I can wait with this patch until the bump if it's going to happen
> soon.

pushed