[FFmpeg-devel] [patch] glob matching for image series

[Alexander Strasser]

Hi,

Nicolas George wrote:
> L'octidi 8 ventôse, an CCXX, Michael Niedermayer a écrit :
> > if there are no further comments / reviews then ill apply this in a
> > few days
> 
> I re-read it, it looks ok.

  I have nothing against it. After reconsidering multiple times I withdraw
my "objection" (well it was never meant as one in the first place).

> Unfortunately, it no longer apply cleanly since the merge of the
> img2dec/img2enc split. Here is a fixed version; it is in my repository, I
> can push it easily.

  Fine for me. I have some more thoughts on this, but as the author of
the patch has gone through a rather longish review process (escpecially
considering the wordexp history) I will not veto if you just go ahead
with this version.

  After having that said, I will still express my opinions on this
feature in the remaining part of this mail in the hope that it will
be of use/interest for some and if it is for entertainment only ;)

  First of all I kind of like the idea of having this functionality in!
I do *not* like how this changes the default behaviour of libavformat
and even introduces a platform-specific behaviour. Please read my
comment below.

[...]
> diff --git a/libavformat/img2dec.c b/libavformat/img2dec.c
> index e8b695f..d10a7a4 100644
> --- a/libavformat/img2dec.c
> +++ b/libavformat/img2dec.c
[...]
> @@ -27,6 +27,25 @@
>  #include "libavutil/parseutils.h"
>  #include "avformat.h"
>  #include "internal.h"
> +#if HAVE_GLOB
> +#include <glob.h>
> +
> +/* Locally define as 0 (bitwise-OR no-op) any missing glob options that
> +   are non-posix glibc/bsd extensions. */
> +#ifndef GLOB_NOMAGIC
> +#define GLOB_NOMAGIC 0
> +#endif
> +#ifndef GLOB_TILDE
> +#define GLOB_TILDE 0
> +#endif
> +#ifndef GLOB_TILDE_CHECK
> +#define GLOB_TILDE_CHECK GLOB_TILDE
> +#endif
> +#ifndef GLOB_BRACE
> +#define GLOB_BRACE 0
> +#endif
> +
> +#endif /* HAVE_GLOB */

  Keep those in mind.

[...]
>      if (!s->is_pipe) {
> +        s->use_glob = is_glob(s->path);
> +        if (s->use_glob) {
> +#if HAVE_GLOB
> +            int gerr;
> +            gerr = glob(s->path, GLOB_NOCHECK|GLOB_BRACE|GLOB_NOMAGIC|GLOB_TILDE_CHECK, NULL, &s->globstate);

  If I understand this correctly we get at least braces depending on
platform and also probably varying tilde handling if any at all.

> +            if (gerr != 0) {
> +                return AVERROR(ENOENT);
> +            }
> +            first_index = 0;
> +            last_index = s->globstate.gl_pathc - 1;
> +#endif
> +        } else {
>          if (find_image_range(&first_index, &last_index, s->path) < 0)
>              return AVERROR(ENOENT);
> +        }

  Might be worth to mention that globs take precedence over image
ranges in the docs part.

[remaining patch snipped]


  Instead I would propose following ideal behaviour:
  1. Support globbing not only for picture input.
  2. Do it only if it was explicitly enabled
  3. Agree on a set of features like ?*[]{} and fail if is not available
     (maybe with the additional possibility to force it on with what is
      available)
  4. Tell the user if he requested to enable globbing but it wasn't
     available at all in his lavf binary.

  I know this is controversial, especially point 3. Also I do think point
1 could left out at first and left as possible future enhancement.

  The described behaviour would allow for implementation of applications
that behave consistently across platforms. That is a property I appreciate
very much and that I would like to see from FFmpeg programs and libraries.

  Alexander