[FFmpeg-devel] [PATCH] lavfi: add histeq filter (WIP)

[Jérémy TRAN]

2012/10/15 Clément Bœsch <ubitux at gmail.com>:
> fist review? mmh...

Woops…

>> + at section histeq
>> +From the original author's description:
>
> I wonder if that sentence is useful

Since I just copied and adapted it a little bit I didn’t know I had to
specify that, but if it’s not necessary I’m fine with removing it.

>> +
>> +    if (args)
>> +        sscanf(args, "%d:%d:%s", &histeq->strength, &histeq->intensity, antibanding_str);
>
> This is not safe; add some length protection for %s. Also, why no
> alternative av_set_options_string() like in vf hue and some other filters?

Yes, you’re right.

>> +            histeq->rgba_map[R] = 2;
>> +            histeq->rgba_map[G] = 1;
>> +            histeq->rgba_map[B] = 0;
>> +            histeq->rgba_map[A] = 3;
>> +            break;
>> +        case PIX_FMT_RGBA:
>> +            histeq->rgba_map[R] = 3;
>> +            histeq->rgba_map[G] = 2;
>> +            histeq->rgba_map[B] = 1;
>> +            histeq->rgba_map[A] = 0;
>> +            break;
>> +        case PIX_FMT_ABGR:
>> +            histeq->rgba_map[R] = 0;
>> +            histeq->rgba_map[G] = 1;
>> +            histeq->rgba_map[B] = 2;
>> +            histeq->rgba_map[A] = 3;
>> +            break;
>> +        case PIX_FMT_BGRA:
>> +            histeq->rgba_map[R] = 1;
>> +            histeq->rgba_map[G] = 2;
>> +            histeq->rgba_map[B] = 3;
>> +            histeq->rgba_map[A] = 0;
>> +            break;
>
> sounds like this could be macrotized, but do as you please

Yep, seems like there’s already something to do that in the API.

Thanks for the review !

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