[FFmpeg-devel] [RFC][PATCH 2/2] ffmpeg_opt: assert_file_overwrite: Work for all file protocol outputs

[Alexander Strasser]

On 2014-01-07 17:33 +0100, Stefano Sabatini wrote:
> On date Monday 2014-01-06 17:15:18 +0100, Alexander Strasser encoded:
> > On 2014-01-06 00:11 +0100, Stefano Sabatini wrote:
> > > On date Sunday 2014-01-05 03:30:27 +0100, Alexander Strasser encoded:
> > > > Currently the file overwrite check does not work for paths that
> > > > contain a colon.
> > > > 
> > > > Use avio_find_protocol to always execute the existence check if
> > > > the file protocol is going to be used.
> > > > 
> > > > Fix other part of ticket #3249.
> > > > 
> > > > Signed-off-by: Alexander Strasser <eclipse7 at gmx.net>
> > > > ---
> > > >  ffmpeg_opt.c | 4 +---
> > > >  1 file changed, 1 insertion(+), 3 deletions(-)
> > > > 
> > > > diff --git a/ffmpeg_opt.c b/ffmpeg_opt.c
> > > > index d267c6d..1040a1c 100644
> > > > --- a/ffmpeg_opt.c
> > > > +++ b/ffmpeg_opt.c
> > > > @@ -697,9 +697,7 @@ static void assert_file_overwrite(const char *filename)
> > > >          exit_program(1);
> > > >      }
> > > >  
> > > > -    if (!file_overwrite &&
> > > > -        (strchr(filename, ':') == NULL || filename[1] == ':' ||
> > > > -         av_strstart(filename, "file:", NULL))) {
> > > > +    if (!file_overwrite && !strcmp(avio_find_protocol(filename), "file")) {
> > > >          if (avio_check(filename, 0) == 0) {
> > > >              if (stdin_interaction && !no_file_overwrite) {
> > > >                  fprintf(stderr,"File '%s' already exists. Overwrite ? [y/N] ", filename);
> > > 
> > > Looks nice to me, thanks.
> > 
> 
> >   I went for the empty string as "no protocol found" because that
> > way it turns out nicer for applications where you want to directly
> > compare the result of the avio_find_protocol call.
> > 
> 
> >   If I return NULL to signal "no protocol found", this involves
> > an extra intermediate variable and an extra NULL check.
> 
> Yes it is more work for the programmer but this will force her to
> implement saner/more robust logic, since she will be forced to think
> about the NULL case. Or you will have messages in the form:
> "The choosen protocol is ''."

  I do not agree with this. Returning exceptional values like
NULL is often a plague and does result in errors and/or clumsy
code.

> See also:
> av_get_pix_fmt_name()
> av_get_sample_fmt_name()
> av_get_profile_name()
> avcodec_get_name()
> ...
> 
> Whenever we return a string, the API usually returns NULL in case it
> was not possible to find a corresponding string.

  I agree that it is more natural to return NULL in comparison
with most functions that serve a similar purpose.

  Currently I have no motivation to scan the entire API
to find out if there are any cases that do it differently.
Though I can imagine there are (probably not as return
value though).

> > Alternatively a custom string function could be used that handles
> > NULL inputs.
> 
> This means usually more work and no visible gain, now you have to do a
> strcmp to understand if there was no recognized element.

  Makes no sense to me in this context. Maybe we misunderstood
each other?

  Anyway, I will change avio_find_protocol to return NULL
and adapt this code to use it in the more long winded and
verbose way I described initially.

  If I hear no more comments I will push in a few days.

Thank you
  Alexander
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 181 bytes
Desc: not available
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20140107/5b17f698/attachment.asc>