[FFmpeg-soc] Help needed with new concatenate filter

[Stefano Sabatini]

On date Wednesday 2010-03-31 18:00:46 -0400, Brandon Mintern encoded:
> I'm trying to add a filter to concatenate multiple inputs together.
> The idea is that the inputs should already be the same size and frame
> rate, but I don't actually enforce that, I just ensure that the output
> buffer is big enough to hold the biggest height/width. Here is a
> sample usage:
> 
> ./ffmpeg_g -y -i input1.wmv -vfilters "movie=0:wmv3:input2.wmv [in2];
> [in][in2] concatenate" -r 15 out.wmv
> 
> Where input1.wmv and input2.wmv are both 15 fps and 800x600. As
> written now, out.wmv seems to only be input1.wmv. I would appreciate
> any insight into what I'm doing wrong. The patch is located at
> http://bmintern.homeunix.com/~brandon/vf_concatenate.patch and is
> included below.
> 
> Thanks,
> Brandon
> 
> Index: allfilters.c
> ===================================================================
> --- allfilters.c	(revision 5726)
> +++ allfilters.c	(working copy)
> @@ -35,6 +35,7 @@
>      initialized = 1;
> 
>      REGISTER_FILTER (ASPECT,      aspect,      vf);
> +    REGISTER_FILTER (CONCATENATE, concatenate, vf);
>      REGISTER_FILTER (CROP,        crop,        vf);
>      REGISTER_FILTER (DRAWBOX,     drawbox,     vf);
>      REGISTER_FILTER (FIFO,        fifo,        vf);
> Index: vf_concatenate.c
> ===================================================================
> --- vf_concatenate.c	(revision 0)
> +++ vf_concatenate.c	(revision 0)
> @@ -0,0 +1,109 @@
> +/*
> + * video concatenate filter
> + * copyright (c) 2007 Bobby Bingham
> + *
> + * 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
> + */
> +
> +#include "avfilter.h"
> +
> +typedef struct {
> +    int first_input_consumed;
> +    AVFilterPicRef *pic;
> +} ConcatenateContext;
> +
> +static av_cold void uninit(AVFilterContext *ctx)
> +{
> +    ConcatenateContext *con = ctx->priv;
> +    if (con->pic)
> +        avfilter_unref_pic(con->pic);

This is not the way picref are supposed to be unref-fed. They should
be unref-ed during processing, so this is not needed.

> +}
> +
> +static int config_props(AVFilterLink *link)
> +{
> +    if (link->src->inputs[0]->w > link->src->inputs[1]->w)
> +        link->w = link->src->inputs[0]->w;
> +    else
> +        link->w = link->src->inputs[1]->w;
> +    if (link->src->inputs[1]->h > link->src->inputs[1]->h)
> +        link->h = link->src->inputs[0]->h;
> +    else
> +        link->h = link->src->inputs[1]->h;
> +    return 0;
> +}

Variable video frame size is not supported, that means that something
bad will likely happen when the size of the video will change from the
first to the second source. 

> +
> +static void start_frame(AVFilterLink *link, AVFilterPicRef *picref)
> +{
> +    ConcatenateContext *con = link->dst->priv;
> +    con->pic = picref;
> +}

You need to propagate the start_frame to the next filter. Also I
suppose your filter is not changing the passed in video frame, so a
simple avfilter_null_start_frame should be enough.

> +static void end_frame(AVFilterLink *link)
> +{
> +}
> +
> +static int poll_frame(AVFilterLink *link)
> +{
> +    ConcatenateContext *con = link->src->priv;
> +    if (con->first_input_consumed)
> +        return avfilter_poll_frame(link->src->inputs[1]);
> +    return avfilter_poll_frame(link->src->inputs[0]);
> +}
> +
> +static int request_frame(AVFilterLink *link)
> +{
> +    ConcatenateContext *con = link->src->priv;
> +    if(!con->first_input_consumed &&
> +       avfilter_request_frame(link->src->inputs[0]))
> +        con->first_input_consumed = 1;
> +    if(con->first_input_consumed &&
> +       avfilter_request_frame(link->src->inputs[1]))
> +        return AVERROR_EOF;

> +    avfilter_start_frame(link, con->pic);
> +    avfilter_draw_slice(link, 0, con->pic->h, 1);
> +    avfilter_end_frame(link);
> +    con->pic = NULL;

This can't work, suppose request_frame() is called, it is propagated
to the source, finally the source calls start_frame() but it is not
propagated, then draw_slice() is called, which is propagated (see
avfilter_default_slice()), this will result in a crash.

In other words the rule is:

avfilter_start_frame(), _draw_slice() and _end_frame() must be
propagated in this order from the source to the destination filter.

Check the vf_null.c filter, this filter is very similar as it only
*propagates* frames from a selected source, in the case of the null
filter you have only one source, in this case you need to select the
right source in the request_frame() callback.

HTH, regards.