[FFmpeg-devel] [PATCH] Add alphaextract, alphamerge filters

[Steven Robertson]

Thanks for the review. Comments inline.

On Wed, Jul 11, 2012 at 6:43 AM, Stefano Sabatini <stefasab at gmail.com> wrote:
> Nit+: here and below:
> static int config_input(AVFilterLink *link)
> {
> ...

Done.

>> +static int config_output(AVFilterLink *outlink) {
>> +    AVFilterLink *inlink = outlink->src->inputs[0];
>> +    outlink->w = inlink->w;
>> +    outlink->h = inlink->h;
>> +    outlink->time_base = inlink->time_base;
>> +    outlink->sample_aspect_ratio = inlink->sample_aspect_ratio;
>> +    outlink->frame_rate = inlink->frame_rate;
>> +    return 0;
>> +}
>
> Is this required?

Yes, default handler guards against case where there are multiple input pads.

>> +                pin = y * alpha_buf->linesize[0] + x;
>> +                pout = y * main_buf->linesize[0] + x * 4 + merge->rgba_map[A];
>> +                main_buf->data[0][pout] = alpha_buf->data[0][pin];
>
> Nit: pin and pout are usually pointers rather than offsets, so you
> could do:
>                 uint8_t *pin, *pout;
>                 ...
>                 pin = alpha_buf->data[0] + y * alpha_buf->linesize[0] + x;
>                 pout = main_buf->data[0] + y * main_buf->linesize[0] + x * 4 + merge->rgba_map[A];
>                 *pout = *pin;

Done.

>
>> +static void end_frame(AVFilterLink *link) {
>
> nit: link->inlink, helps readability/consistency

Done.

>> +do_lavfi_plain "alphaextract_rgb"   "[in]slicify=random,format=bgra,split[o1][o2];[o1][o2]alphamerge,slicify=random,split[o3][o4];[o4]alphaextract[alpha];[o3][alpha]alphamerge[out]"
>> +do_lavfi_plain "alphaextract_yuv"   "[in]slicify=random,format=yuv420p,split[o1][o2];[o1][o2]alphamerge,slicify=random,split[o3][o4];[o4]alphaextract[alpha];[o3][alpha]alphamerge[out]"
>
> The first part seems a duplicate of the first test, why not a simple:
>
> do_lavfi_plain "alphaextract_rgb"   "[in]slicify=random,split[o3][o4];[o4]alphaextract[alpha];[o3][alpha]alphamerge[out]"
> ?

That suggestion works, but it only operates on the default all-1.0
alpha channel added by the format conversion, so you can't tell if the
filter's doing anything. The original form copies the input's luma to
alpha first, so that the result is visibly different from just a pixel
format change - and it has the nice property that for each colorspace,
merge and extract have the same hash. WDYT?

> Overall nice work.

Thanks!
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Add-alphaextract-alphamerge-filters.patch
Type: application/octet-stream
Size: 17427 bytes
Desc: not available
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20120711/9539acb9/attachment.obj>