[FFmpeg-devel] [PATCH 1/2] avfilter/buffersrc: specify pixel format min/max

[Stefano Sabatini]

On date Monday 2013-12-16 11:43:05 +0100, Nicolas George encoded:
> Le septidi 17 frimaire, an CCXXII, Michael Niedermayer a écrit :
> > i was unsure about it as well
> > but for example the min value that is allowed could be -1 or 0
> > depending on if a "not specified" format is allowed
> > though " min = FFMIN(o->min, -1);" in opt.c looks like it would
> > prevent that usecase
> > 
> > about the max the current
> > " max = FFMAX(o->max, nb_fmts-1);" in opt.c would avoid the problem
> > 
> > also iam not sure if the FFMIN/MAX where intended to be what they
> > are or if they where intended to be the other way around
> 
> This part of the code seems indeed rather strange. It looks like defensive
> programming against internal inconsistencies. I believe asserts would be
> more suited for that use: running FATE will detect the inconsistencies and
> allow to fix them.
> 
> For the min case, I suggest simply:
> 
>     av_assert0(o->min == -1 || o->min == 0);
> 
> Since pixel and sample formats are not ordered, there is not much sense in
> restricting the values to a range.
> 
> For the same reason, the max could be something like that:
> 
>     av_assert(o->max < nb_fmts);
> 
> Another, possibly better, solution, would be to continue ignoring max
> for pixel and sample formats and instead change write_number:
> 
>     int max = o->max;
>     if (o->type == AV_OPT_TYPE_PIXEL_FMT)  max = AV_PIX_FMT_NB - 1;
>     if (o->type == AV_OPT_TYPE_SAMPLE_FMT) max = AV_SAMPLE_FMT_NB - 1;
> 

> Stefano, you wrote this bit of code in the first place: what do you think
> about it?

commit c9ff32215b433d505f251c1f212b1fa0a5e17b73
Author: Stefano Sabatini <stefasab at gmail.com>
Date:   Sat Dec 1 01:07:24 2012 +0100

    lavu/opt: allow to set sample and pixel format with av_opt_set_int()
    
    This change requires the user to specify min and max value, and makes
    possible to prevent the user to set AV_{SAMPLE,PIX}_FMT_NONE if
    forbidden.
    
    Add required ifdeffery in case of mixed libraries, when libavutil is
    updated but not the other libraries.
    
    This is a followup of 08d0969c1402ccec4dce44bd430128fb59d7b790.

commit 08d0969c1402ccec4dce44bd430128fb59d7b790
Author: Stefano Sabatini <stefasab at gmail.com>
Date:   Sun Nov 25 15:45:58 2012 +0100

    lavu/opt: change the way default pixel and sample format value is set
    
    Use the i64 field rather than the string value. Using a string to set a
    default sample/pixel format is weird, also the new interface is more
    consistent with the rest of the API.
    
    This is technically an API break, but hopefully there are no applications
    using this feature outside of FFmpeg. In order to save backward
    compatibility with mixed libraries in case libavutil is updated but not
    the other libraries, some ifdeffery hacks are added.
    
    Note that the version check is only performed when class->version != 0,
    since if it is not defined then we assume that no version was defined and
    the class is not affected by the change.
    
    We will luckily get rid of the hack at the next major bump.

...

Previously, we used int to set a pixel format/sample format value,
and IIRC no range check was performed. The range check was added to
allow to set explicitly undefined values, all values outside -1 -
NB_FORMATS-1 should be ignored.

In order to avoid to hardcode a max value depending on PIX_FMT_NB, we
should probably use MAX_INT instead.

Also I wonder if the code should not rather be:
        min = FFMAX(o->min, -1);
        max = FFMIN(o->max, nb_fmts-1);
-- 
FFmpeg = Fiendish Frightening Magical Portentous Extensive Gladiator