[FFmpeg-devel] [PATCH] avfilter: add mergeplanes

[Paul B Mahol]

On 9/30/13, Nicolas George <george at nsup.org> wrote:
> Le nonidi 9 vendemiaire, an CCXXII, Paul B Mahol a ecrit :
>> Signed-off-by: Paul B Mahol <onemda at gmail.com>
>> ---
>>  doc/filters.texi             |  20 ++++
>>  libavfilter/Makefile         |   1 +
>>  libavfilter/allfilters.c     |   1 +
>>  libavfilter/vf_mergeplanes.c | 227
>> +++++++++++++++++++++++++++++++++++++++++++
>>  4 files changed, 249 insertions(+)
>>  create mode 100644 libavfilter/vf_mergeplanes.c
>>
>> diff --git a/doc/filters.texi b/doc/filters.texi
>> index bd39495..9d8c7cc 100644
>> --- a/doc/filters.texi
>> +++ b/doc/filters.texi
>> @@ -5248,6 +5248,26 @@ lutyuv=y='bitand(val, 128+64+32)'
>>  @end example
>>  @end itemize
>>
>> + at section mergeplanes
>> +
>> +Merge color channel components from several video streams.
>
> Good idea.
>
>> +
>> +This filter accepts the following options:
>> + at table @option
>> + at item inputs
>> +Set the number of inputs. Default is 2.
>> + at end table
>
> I have concerns about usability: why does 2 mean YUV+A instead of GRAY+A?

Indeed, but there is no planar gray8a, but adding support for packed
merge is not hard at all.

It could be allowed to merge various stuff, but should I handle that
in query_formats?
Or just put supported I/O formats there and add checks in config_output?

> And if 3 means Y+U+V, how do you express R+G+B? Maybe something like that
> would be more appropriate:
>
> 	mode=y_u_v
> 	mode=yuv_a
>
> Or, possibly using additive features:
>
> 	mode=y+u+v
> 	mode=yuv+a
>
> (note that in that case, yuv must be a different value from y+u+v; I
> suppose
> each of y, u, v, yuv, r, g, b, rgb, alpha need its own bit)

So mode explicitly sets what input and output formats filter will
accept?

>
> Support for more formats can come later, of course, but the user interface
> need to be ready to avoid compatibility concerns.

I wonder what is most sane/logical/useful filter syntax/whatever.

>

[...]

>> +    AVFilterContext *ctx = fs->parent;
>> +    AVFilterLink *outlink = ctx->outputs[0];
>> +    MergePlanesContext *s = fs->opaque;
>> +    AVFrame *in[4] = { NULL };
>> +    AVFrame *out;
>> +    int i, ret = 0;
>> +
>> +    av_assert0(s->nb_inputs <= 4);
>> +    for (i = 0; i < s->nb_inputs; i++) {
>
>> +        if ((ret = ff_framesync_get_frame(&s->fs, i, &in[i], s->nb_inputs
>> - (i + 1))) < 0)
>
> I wonder about the last argument: "s->nb_inputs - (i + 1)". It looks like
> you are using the input frames read-only, therefore, 0 should be just fine.

Will try.

>
>> +            return ret;
>> +    }
>> +
>> +    out = ff_get_video_buffer(outlink, outlink->w, outlink->h);
>> +    if (!out)
>> +        return AVERROR(ENOMEM);
>> +
>
>> +    out->pts = av_rescale_q(in[0]->pts, s->fs.time_base,
>> outlink->time_base);
>
> The correct timestamp is in s->fs.pts; in[0]->pts may be wrong if the same
> frame is duplicated to sync with the other streams.

Thanks. Fixed.

>
>> +    merge_planes(ctx, in, out);
>> +    ret = ff_filter_frame(outlink, out);
>> +
>> +    return ret;
>> +}
>> +
>> +static int config_output(AVFilterLink *outlink)
>> +{
>> +    AVFilterContext *ctx = outlink->src;
>> +    MergePlanesContext *s = ctx->priv;
>> +    FFFrameSyncIn *in = s->fs.in;
>> +    int i, ret;
>> +
>> +    ff_framesync_init(&s->fs, ctx, s->nb_inputs);
>> +    s->fs.opaque = s;
>> +    s->fs.on_event = process_frame;
>> +
>> +    for (i = 0; i < s->nb_inputs; i++) {
>> +        AVFilterLink *inlink = ctx->inputs[i];
>> +        const AVPixFmtDescriptor *desc =
>> av_pix_fmt_desc_get(inlink->format);
>> +
>> +        if ((ret = av_image_fill_linesizes(s->planewidth[i],
>> inlink->format, inlink->w)) < 0)
>> +            return ret;
>> +
>> +        s->planeheight[i][1] =
>> +        s->planeheight[i][2] = FF_CEIL_RSHIFT(inlink->h,
>> desc->log2_chroma_h);
>> +        s->planeheight[i][0] =
>> +        s->planeheight[i][3] = inlink->h;
>> +        s->nb_planes[i] = av_pix_fmt_count_planes(inlink->format);
>
> I do not understand the logic here: unless I am missing something, there
> should be a check that all input streams have compatible sizes (i.e., since
> only 444 is supported, all the same size) (and pixel aspect ratio too).

Yes, I'm just lazy...

>
>> +
>> +        in[i].time_base = inlink->time_base;
>> +        in[i].sync   = 1;
>> +        in[i].before = EXT_STOP;
>> +        in[i].after  = EXT_STOP;
>> +    }
>> +
>> +    return ff_framesync_configure(&s->fs);
>> +}
>> +

[...]