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

[Nicolas George]

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?

Regards,

-- 
  Nicolas George
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20131216/b18366e0/attachment.asc>