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

[Alexander Strasser]

Hi Stefano,

  thanks for tackling this!

  On removal of glob_sequence pattern we can also remove
is_glob function (and therefore adjust read_probe). Just
writing it here as a reminder that we not forget about that.

Stefano Sabatini wrote:
> Allow to override the default 'glob_sequence' value, which is deprecated
> in favor of the new 'glob' and 'sequence' options.
> 
> The new pattern types should be easier on the user since they are more
> predictable than 'glob_sequence', and do not require awkward escaping.
> 
> FIXME: bump micro before pushing
> ---
>  libavformat/img2dec.c |   34 +++++++++++++++++++++++++++++++++-
>  1 files changed, 33 insertions(+), 1 deletions(-)
> 
> diff --git a/libavformat/img2dec.c b/libavformat/img2dec.c
> index 0443b1a..a108656 100644
> --- a/libavformat/img2dec.c
> +++ b/libavformat/img2dec.c
> @@ -41,6 +41,8 @@
>  
>  #endif /* HAVE_GLOB */
>  
> +enum PatternType { PT_GLOB_SEQUENCE, PT_GLOB, PT_SEQUENCE };
> +
>  typedef struct {
>      const AVClass *class;  /**< Class for private options. */
>      int img_first;
> @@ -54,6 +56,7 @@ typedef struct {
>      char *video_size;       /**< Set by a private option. */
>      char *framerate;        /**< Set by a private option. */
>      int loop;
> +    enum PatternType pattern_type;
>      int use_glob;
>  #if HAVE_GLOB
>      glob_t globstate;
> @@ -233,6 +236,9 @@ static int read_header(AVFormatContext *s1)
>      }
>  
>      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.

>  #if HAVE_GLOB
> @@ -260,7 +266,9 @@ static int read_header(AVFormatContext *s1)
>              first_index = 0;
>              last_index = s->globstate.gl_pathc - 1;
>  #endif
> -        } else {
> +        }
> +        }
> +        if ((s->pattern_type == PT_GLOB_SEQUENCE && !s->use_glob) || s->pattern_type == PT_SEQUENCE) {
>              if (find_image_range(&first_index, &last_index, s->path,
>                                   s->start_number, s->start_number_range) < 0) {
>                  av_log(s1, AV_LOG_ERROR,
> @@ -268,7 +276,25 @@ static int read_header(AVFormatContext *s1)
>                         s->path, s->start_number, s->start_number + s->start_number_range - 1);
>                  return AVERROR(ENOENT);
>              }
> +        } else if (s->pattern_type == PT_GLOB) {
> +            int gerr;
> +            if (!HAVE_GLOB) {
> +                av_log(s1, AV_LOG_ERROR, "Pattern type 'glob' was selected but is not supported by libavformat\n");

nit: "Pattern type 'glob' was selected but is not supported by this libavformat build\n");
or   "is not available in this libavformat build"
or similar.

  Leave it as is or change to your liking.

[...]

  Besides the missing #ifdef Nicolas mentioned, the patch looks fine to me.

  I also like this solution better then the current solution. A plus are the
clear failure modes.

  Alexander