[FFmpeg-devel] [SOCIS] Porting vf_field.c from libmpcodecs [WIP]

[Stefano Sabatini]

On date Tuesday 2012-07-31 18:57:43 +0200, David Keller encoded:
> Hi,
> 
> I am in the process of porting the filter vf_field.c from libmpcodecs and
> almost done. I just haven't figured out which configuration inputs I should
> support and the formats in query_format are presumably not complete.
> 
> I have attached the code I've written so far.
> 
> David

> diff --git a/libavfilter/vf_field.c b/libavfilter/vf_field.c
> new file mode 100644
> index 0000000..687b504
> --- /dev/null
> +++ b/libavfilter/vf_field.c
> @@ -0,0 +1,162 @@

> +/* This file is part of FFmpeg, ported from MPlayer.

Feel free to add your copyright, together with the copyright of the
original author (unless the code is trivial).

> + *
> + * FFmpeg is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 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 General Public License for more details.
> + *
> + * You should have received a copy of the GNU 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
> + * field filter, ported from MPlayer
> + * libmpcodecs/vf_field.c.
> + */
> +
> +// Original includes
> +/*#include <stdio.h>
> +#include <stdlib.h>
> +#include <string.h>
> +
> +#include "config.h"
> +#include "mp_msg.h"
> +
> +#include "mp_image.h"
> +#include "vf.h"
> +*/

Please remove all the remainings of the ported filter.

> +
> +#include "libavutil/pixdesc.h"
> +#include "avfilter.h"
> +#include "libavutil/avassert.h"
> +#include "formats.h"
> +#include "internal.h"
> +#include "video.h"

nit:
#include "libavutil/avassert.h"
#include "libavutil/pixdesc.h"
#include "avfilter.h"
#include "formats.h"
#include "internal.h"
#include "video.h"

that is: external libs includes, and internal header

> +
> +typedef struct {
> +    int field;
> +} FieldContext;
> +
> +
> +// Still work in progress
> +static int config_input(AVFilterLink *inlink)
> +{
> +    FieldContext *field = inlink->dst->priv;
> +
> +    return 0;

I suppose you have to configure the *output* link with outlink->h =
inlink->h/2.

> +}
> +
> +
> +static int end_frame(AVFilterLink *inlink)
> +{
> +    FieldContext *field = inlink->dst->priv;
> +    AVFilterLink *outlink = inlink->dst->outputs[0];
> +    AVFilterBufferRef *inpic  = inlink ->cur_buf;
> +    AVFilterBufferRef *outpic = outlink->out_buf;
> +
> +    int ret;
> +

> +    outpic->data[0] = inpic->data[0] + inpic->linesize[0] * field->field;
> +
> +    outpic->linesize[0] = 2 * inpic->linesize[0];
> +
> +//    if (vf->dmpi->flags&MP_IMGFLAG_PLANAR) {
> +
> +    outpic->data[1] = inpic->data[1];
> +
> +    inpic->linesize[1] * field->field;
> +
> +    outpic->linesize[1] = 2 * inpic->linesize[1];
> +
> +    outpic->data[2] = inpic->data[2] +
> +
> +                      inpic->linesize[2] * field->field;
> +
> +    outpic->linesize[2] = 2 * inpic->linesize[2];

Could be factored with an iteration.

Also this code is allocating unnecessary data.

What's happening: you use the standard start_frame() callback, which
stores in outlink->out_buf a new buffer (which is a newly allocated
buffer).

Now you're doing:
   outpic->data[plane] = inpic->data[plane] + inpic->linesize[plane] * field->field;

that is storing in outpic the data which was allocated in inpic. This
is clearly not very efficient, and you're introducing an
inconsistency, since out_buf will point to a buffer different from the
original buffer pointed by out_buf.

When you release the inpic buffer this may cause a crash when the same
buffer is accessed through the outpic reference.

The logic of this filter is somewhat similar to swapuv (where some
buffer reference trickery is applied), checking its code may be
instructive.

> +
> +/*      else
> +        vf->dmpi->planes[1]=mpi->planes[1]; // passthru bgr8 palette!!!
> +
> +        outpic->data[1] = inpic->data[1]; // passthru bgr8 palette
> +*/
> +
> +    if ((ret = ff_draw_slice(outlink, 0, inpic->video->h, 1)) < 0 ||
> +            (ret = ff_end_frame(outlink)) < 0)
> +        return ret;
> +
> +    return 0;
> +}
> +
> +
> +static int query_formats(AVFilterContext *ctx)
> +{
> +    // Presumably not complete
> +    static const enum PixelFormat pix_fmts[] = {
> +            PIX_FMT_YUV444P,  PIX_FMT_YUV422P,  PIX_FMT_YUV420P,
> +            PIX_FMT_YUV411P,  PIX_FMT_YUV410P,
> +            PIX_FMT_YUVJ444P, PIX_FMT_YUVJ422P, PIX_FMT_YUVJ420P,
> +            PIX_FMT_YUV440P,  PIX_FMT_YUVJ440P
> +    };
> +
> +    ff_set_common_formats(ctx, ff_make_format_list(pix_fmts));

return ff_set_common_formats(...) if I'm not mistaken, better than
ignoring the unlikely failure of this call.
 
> +
> +    return 0;
> +}
> +
> +/*static void uninit(struct vf_instance *vf)
> +{
> +        free(vf->priv);
> +}*/
> +

> +static av_cold void uninit(AVFilterContext *ctx)
> +{
> +    FieldContext *field = ctx->priv;
> +
> +    av_freep(&field->field);

There is nothing to free in an int (and will crash is field->field is
1).

> +}
> +
> +static av_cold int init(AVFilterContext *ctx, const char *args)
> +{
> +    FieldContext *field = ctx->priv;
> +
> +    if (args)
> +        sscanf(args, "%d", field->field);

Missing input validation. What if the user passes "foo"?

> +
> +    return 0;
> +}
> +
> +static int null_draw_slice(AVFilterLink *link, int y, int h, int slice_dir)
> +{
> +    return 0;
> +}
> +
> +AVFilter avfilter_vf_field = {
> +    .name          = "field",
> +    .description   = NULL_IF_CONFIG_SMALL("Extract a single field."),
> +
> +    .priv_size     = sizeof(FieldContext),
> +    .init          = init,
> +    .uninit        = uninit,
> +    .query_formats = query_formats,
> +
> +    .inputs    = (const AVFilterPad[]) {
> +        { .name             = "default",
> +          .type             = AVMEDIA_TYPE_VIDEO,
> +          .draw_slice       = null_draw_slice,
> +          .config_props     = config_input,
> +          .end_frame        = end_frame },
> +        { .name = NULL}
> +    },
> +    .outputs   = (const AVFilterPad[]) {
> +        { .name             = "default",
> +          .type             = AVMEDIA_TYPE_VIDEO },
> +        { .name = NULL}
> +    },
> +};

Also please add documentation to doc/filters.texi.

Also you should check that the output of mp=field matches with the
output of this filter, the framemd5 output muxer and showinfo will
help.
-- 
FFmpeg = Friendly and Fierce Multimedia Portable Elitarian Ghost