[FFmpeg-devel] [PATCH] lavf/img2dec: add -pattern_type option

[Alexander Strasser]

Stefano Sabatini wrote:
> On date Tuesday 2012-08-07 01:43:44 +0200, Alexander Strasser encoded:
[...]
> > >      if (!s->is_pipe) {
> > > +        if (s->pattern_type == PT_GLOB_SEQUENCE) {
> > > +            av_log(s1, AV_LOG_WARNING, "Pattern type 'glob_sequence' is deprecated: "
> > > +                   "use -pattern_type 'glob' or 'sequence' instead\n");
> > >          s->use_glob = is_glob(s->path);
> > >          if (s->use_glob) {
> > 
> >   I would prefer to only print the warning if is_glob() is true to
> > not spam users not aware of the globbing functionality.
> > 
> >   Also a more ffmpeg tool agnostic message like this could be used:
> > 
> > "Using deprecated pattern_type 'glob_sequence' for globbing: port your pattern to 'glob' because 'glob_sequence' will be removed\n"
> > 
> >   If you want to keep the hint on how it is exposed as ffmpeg
> > option you could still keep the spirit and reduce to say that the
> > user needs to adapt to "-pattern_type glob" as it is unlikely the
> > user wanted to use sequence if we got into "if (s->use_glob)" branch.
> 
> Done this way (note that this also has its drawbacks, e.g. in the case
> the user choose to explicitly use glob_sequence).

  Thanks; and yes I know. It's a compromise. But it is for the better IMHO.

[...]
> + at var{pattern_type} can assume one of the following values.

nit: Not fully sure but "can assume" sounds a bit strange to my
non-native English ears.

  Quick list of alternatives that come to mind:
"accepts", "can be", "can take", "can be set to"

[...]
> + at item glob_sequence
> +Select a mixed glob wildcard/sequence pattern.

  If we really want to remove glob_sequence, then it is probably
a good idea to mention it here too.

  Maybe

  @item glob_sequence (deprecated, will be removed)

or similar.

[remaining patch snipped]

  The remaining parts look good AFAICT. I could not quickly test
as the patch doesn't apply cleanly for me. But assuming it works
for you and you tested it I would say it is push-ready (maybe
wait a little bit longer if someone else wants to comment).

Greetings,
  Alexander