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

[Steven Robertson]

On Thu, Jul 19, 2012 at 12:56 AM, Stefano Sabatini <stefasab at gmail.com> wrote:
> On date Sunday 2012-07-15 11:35:03 -0700, Steven Robertson encoded:
>> On Sat, Jul 14, 2012 at 4:03 PM, Nicolas George
> [...]
>> --- /dev/null
>> +++ b/libavfilter/vf_alphaextract.c
> [...]
>> +static int config_input(AVFilterLink *link)
>
> nit: inlink

Done.

>> +{
>> +    AlphaExtractContext *extract = link->dst->priv;
>> +    extract->is_packed_rgb =
>> +        ff_fill_rgba_map(extract->rgba_map, link->format) >= 0;
>> +    return 0;
>> +}
>> +
>> +static void draw_slice(AVFilterLink *link, int y0, int h, int slice_dir)
>
> nit: inlink

Done.

>> +{
>> +    AlphaExtractContext *extract = link->dst->priv;
>> +    AVFilterBufferRef *cur_buf = link->cur_buf;
>> +    AVFilterBufferRef *out_buf = link->dst->outputs[0]->out_buf;
>> +
>> +    if (extract->is_packed_rgb) {
>> +        int x, y;
>> +        uint8_t *pin, *pout;
>> +        for (y = y0; y < (y0 + h); y++) {
>> +            pin = cur_buf->data[0] + y * cur_buf->linesize[0] + extract->rgba_map[A];
>> +            pout = out_buf->data[0] + y * out_buf->linesize[0];
>> +            for (x = 0; x < out_buf->video->w; x++) {
>> +                *pout = *pin;
>> +                pout += 1;
>> +                pin += 4;
>> +            }
>> +        }
>
>> +    } else {
>> +        const int linesize = cur_buf->linesize[A];
>> +        av_assert0(linesize == out_buf->linesize[Y]);
>
> I'm not sure this is always correct, indeed I never assume it is in
> the code. For example I guess the following may fail:
>
> testsrc,pad=2*iw:2*iw,alphaextract
>
> You could do:
> else if (cur_buf->linesize[A] == out_buf->linesize[Y]) {
>      memmove
> } else {
>      copy line by line
> }

Done.

>> +
>> +        memmove(out_buf->data[Y] + y0 * linesize,
>> +                cur_buf->data[A] + y0 * linesize,
>> +                linesize * h);
>> +    }
>> +    ff_draw_slice(link->dst->outputs[0], y0, h, slice_dir);
>> +}
>> +
>> +AVFilter avfilter_vf_alphaextract = {
>> +    .name           = "alphaextract",
>> +    .description    = NULL_IF_CONFIG_SMALL("Extract an alpha channel as a "
>> +                      "grayscale image component."),
>> +    .priv_size      = sizeof(AlphaExtractContext),
>> +    .query_formats  = query_formats,
>> +
>> +    .inputs    = (const AVFilterPad[]) {
>> +        { .name             = "default",
>> +          .type             = AVMEDIA_TYPE_VIDEO,
>> +          .config_props     = config_input,
>> +          .draw_slice       = draw_slice,
>> +          .min_perms        = AV_PERM_READ },
>> +        { .name = NULL }
>> +    },
>> +    .outputs   = (const AVFilterPad[]) {
>> +      { .name               = "default",
>> +        .type               = AVMEDIA_TYPE_VIDEO, },
>> +      { .name = NULL }
>> +    },
>> +};
>> diff --git a/libavfilter/vf_alphamerge.c b/libavfilter/vf_alphamerge.c
>> new file mode 100644
>> index 0000000..dc5f1c9
>> --- /dev/null
>> +++ b/libavfilter/vf_alphamerge.c
>> @@ -0,0 +1,203 @@
>> +/*
>> + * Copyright (c) 2012 Steven Robertson
>> + *
>> + * This file is part of FFmpeg.
>> + *
>> + * FFmpeg is free software; you can redistribute it and/or
>> + * modify it under the terms of the GNU Lesser General Public
>> + * License as published by the Free Software Foundation; either
>> + * version 2.1 of the License, or (at your option) any later version.
>> + *
>> + * FFmpeg is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
>> + * Lesser General Public License for more details.
>> + *
>> + * You should have received a copy of the GNU Lesser General Public
>> + * License along with FFmpeg; if not, write to the Free Software
>> + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
>> + */
>> +
>> +/**
>> + * @file
>> + * copy an alpha component from another video's luma
>> + */
>> +
>> +#include <string.h>
>> +
>> +#include "libavutil/pixfmt.h"
>> +#include "avfilter.h"
>> +#include "bufferqueue.h"
>> +#include "drawutils.h"
>> +#include "formats.h"
>> +#include "internal.h"
>> +#include "video.h"
>> +
>> +enum { Y, U, V, A };
>> +
>> +typedef struct {
>> +    int frame_requested;
>> +    int is_packed_rgb;
>> +    uint8_t rgba_map[4];
>> +    struct FFBufQueue queue_main;
>> +    struct FFBufQueue queue_alpha;
>> +} AlphaMergeContext;
>> +
>> +static av_cold void uninit(AVFilterContext *ctx)
>> +{
>> +    AlphaMergeContext *merge = ctx->priv;
>> +    ff_bufqueue_discard_all(&merge->queue_main);
>> +    ff_bufqueue_discard_all(&merge->queue_alpha);
>> +}
>> +
>> +static int query_formats(AVFilterContext *ctx)
>> +{
>> +    enum PixelFormat main_fmts[] = {
>> +        PIX_FMT_YUVA444P, PIX_FMT_YUVA422P, PIX_FMT_YUVA420P,
>> +        PIX_FMT_RGBA, PIX_FMT_BGRA, PIX_FMT_ARGB, PIX_FMT_ABGR,
>> +        PIX_FMT_NONE
>> +    };
>> +    enum PixelFormat alpha_fmts[] = { PIX_FMT_GRAY8, PIX_FMT_NONE };
>> +    AVFilterFormats *main_formats = ff_make_format_list(main_fmts);
>> +    AVFilterFormats *alpha_formats = ff_make_format_list(alpha_fmts);
>> +    ff_formats_ref(main_formats, &ctx->inputs[0]->out_formats);
>> +    ff_formats_ref(alpha_formats, &ctx->inputs[1]->out_formats);
>> +    ff_formats_ref(main_formats, &ctx->outputs[0]->in_formats);
>> +    return 0;
>> +}
>> +
>> +static int config_input_main(AVFilterLink *inlink)
>> +{
>> +    AlphaMergeContext *merge = inlink->dst->priv;
>> +    merge->is_packed_rgb =
>> +        ff_fill_rgba_map(merge->rgba_map, inlink->format) >= 0;
>> +    return 0;
>> +}
>> +
>> +static int config_output(AVFilterLink *outlink)
>> +{
>> +    AVFilterLink *mainlink = outlink->src->inputs[0];
>> +    AVFilterLink *alphalink = outlink->src->inputs[1];
>
>> +    if (mainlink->w != alphalink->w || mainlink->h != alphalink->h)
>> +      return AVERROR(EINVAL);
>
> Weird indent, also tell the poor user what's going wrong.

Done.

>> +
>> +    outlink->w = mainlink->w;
>> +    outlink->h = mainlink->h;
>> +    outlink->time_base = mainlink->time_base;
>> +    outlink->sample_aspect_ratio = mainlink->sample_aspect_ratio;
>> +    outlink->frame_rate = mainlink->frame_rate;
>> +    return 0;
>> +}
>> +
>> +static void start_frame(AVFilterLink *inlink, AVFilterBufferRef *picref) {}
>> +static void draw_slice(AVFilterLink *inlink, int y, int h, int slice_dir) {}
>> +
>> +static void draw_frame(AVFilterContext *ctx,
>> +                       AVFilterBufferRef *main_buf,
>> +                       AVFilterBufferRef *alpha_buf)
>> +{
>> +    AlphaMergeContext *merge = ctx->priv;
>> +    int h = main_buf->video->h;
>> +
>> +    if (merge->is_packed_rgb) {
>> +        int x, y;
>> +        uint8_t *pin, *pout;
>> +        for (y = 0; y < h; y++) {
>> +            pin = alpha_buf->data[0] + y * alpha_buf->linesize[0];
>> +            pout = main_buf->data[0] + y * main_buf->linesize[0] + merge->rgba_map[A];
>> +            for (x = 0; x < main_buf->video->w; x++) {
>> +                *pout = *pin;
>> +                pin += 1;
>> +                pout += 4;
>> +            }
>> +        }
>> +    } else {
>> +        int y;
>
>> +        int main_line = main_buf->linesize[A];
>> +        int alpha_line = alpha_buf->linesize[Y];
>
> nit: *_linesize, "line" has a pretty different meaning (denotes a
> buffer rather than a size). Also maybe "const int" can help the
> compiler to optimize.

Done.

>
>> +
>> +        for (y = 0; y < h && y < alpha_buf->video->h; y++) {
>> +            memmove(main_buf->data[A] + y * main_line,
>> +                    alpha_buf->data[Y] + y * alpha_line,
>> +                    FFMIN(main_line, alpha_line));
>
> Any special reason to use memmove rather than memcpy? Can the two
> arrays overlap?

I guess not.

> Also why are you copying the entire line, rather than just the image
> data?

So that the filters will work with 16-bit data just by extending the
pad configuration (which I may look into soon).

>
> [...]
>> +AVFilter avfilter_vf_alphamerge = {
>> +    .name           = "alphamerge",
>
>> +    .description    = NULL_IF_CONFIG_SMALL("Copy the luma value of grayscale "
>> +                      "frames into the alpha channel of another video."),
>
> Maybe more clear (although a bit longer):
> "Copy the luma value of the second input into the alpha channel of the first input."

Done.

> [...]
>> diff --git a/tests/lavfi-regression.sh b/tests/lavfi-regression.sh
>> index dd5e2da..224fc8f 100755
>> --- a/tests/lavfi-regression.sh
>> +++ b/tests/lavfi-regression.sh
>> @@ -13,21 +13,25 @@ eval do_$test=y
>>
>>  do_video_filter() {
>>      label=$1
>> -    filters=$2
>> +    filters="$2"
>>      shift 2
>>      printf '%-20s' $label
>>      run_avconv $DEC_OPTS -f image2 -vcodec pgmyuv -i $raw_src    \
>>          $ENC_OPTS -vf "$filters" -vcodec rawvideo $* -f nut md5:
>>  }
>>
>> -do_lavfi() {
>> -    vfilters="slicify=random,$2"
>> +do_lavfi_plain() {
>> +    vfilters="$2"
>>
>>      if [ $test = $1 ] ; then
>>          do_video_filter $test "$vfilters"
>>      fi
>>  }
>>
>> +do_lavfi() {
>> +    do_lavfi_plain $1 "slicify=random,$2"
>> +}
>> +
>>  do_lavfi_colormatrix() {
>>      do_lavfi "${1}1" "$1=$4:$5,$1=$5:$3,$1=$3:$4,$1=$4:$3,$1=$3:$5,$1=$5:$2"
>>      do_lavfi "${1}2" "$1=$2:$3,$1=$3:$2,$1=$2:$4,$1=$4:$2,$1=$2:$5,$1=$5:$4"
>> @@ -60,6 +64,11 @@ do_lavfi "vflip"              "vflip"
>>  do_lavfi "vflip_crop"         "vflip,crop=iw-100:ih-100:100:100"
>>  do_lavfi "vflip_vflip"        "vflip,vflip"
>>
>
>> +do_lavfi_plain "alphamerge_rgb"     "[in]slicify=random,format=bgra,split[o1][o2];[o1][o2]alphamerge[out]"
>
> Here and below, the syntax can be simplified to:
> [in]slicify=random,format=bgra,split,alphamerge[out]

Done.

> which looks less noisy and thus more readable to me.
>
> According to docs (filtergraph syntax, filters.texi):
> |If an output pad is not labelled, it is linked by default to the first
> |unlabelled input pad of the next filter in the filterchain.
>
>> +do_lavfi_plain "alphamerge_yuv"     "[in]slicify=random,format=yuv420p,split[o1][o2];[o1][o2]alphamerge[out]"
>> +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]"
>> +
>
> [...]
>
> Should be good otherwise.
> --
> FFmpeg = Funny and Fierce Multimedia Prodigious Educated Gospel
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Add-alphaextract-alphamerge-filters.patch
Type: application/octet-stream
Size: 18394 bytes
Desc: not available
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20120719/c269a15f/attachment.obj>