[FFmpeg-devel] [PATCH 2/2] vf_hue: add named options syntax for the filter parameters

[Jérémy TRAN]

Thanks for the review.

2012/8/12 Stefano Sabatini <stefasab at gmail.com>:
> On date Sunday 2012-08-12 13:26:18 +0200, Jérémy Tran encoded:
>>  typedef struct {
>> -    float    hue;
>> -    float    saturation;
>> -    int      hsub;
>> -    int      vsub;
>> -    uint32_t hue_sin;
>> -    uint32_t hue_cos;
>> +    const AVClass *class;
>> +    float         h;
>> +    char*         hrad; /* expr in radian */
>> +    float         s;
>
> why the rename (h->hue, s->saturation?)

I did that in order to keep the same names than the command-line parameters.

> Also please add documentation to filters.texi.

My bad, I was sure I included them.

>> -    if (s < 0 || s > 2) {
>
>> +    if (hue->s < 0 || hue->s > 2)
>>          av_log(ctx, AV_LOG_ERROR,
>> -               "Invalid value for s:%0.1f : must be included in the range 0-2\n",
>> -               s);
>> -        return AVERROR(EINVAL);
>> +               "Invalid value for s:%1.0f : must be included in the range 0-2\n", s);
>
> unrelated cosemtics

Is it fine to put the small unrelated stuff in another patch ?

>> +
>> +    if (expr) {
>> +        av_expr_parse_and_eval(&expr_res, expr,
>> +                               var_names, var_values,
>> +                               NULL, NULL, NULL, NULL, NULL, 0, hue);
>> +        hue->h = expr_res;
>> +    }
>
> Note that this should not be required. opt supports expressions (with
> PI and other constants) out of the box.

Oh that’s good then, I’ll update the patch !

-- 
Jérémy Tran
ACU 2013
EPITA GISTRE 2013