[FFmpeg-soc] libavfilter audio work - qualification task

[Bobby Bingham]

On Sun, 25 Apr 2010 10:09:19 -0700
"S.N. Hemanth Meenakshisundaram" <smeenaks at ucsd.edu> wrote:

> Index: vf_yadif.c
> ===================================================================
> --- vf_yadif.c	(revision 0)
> +++ vf_yadif.c	(revision 0)
> @@ -0,0 +1,526 @@
> +/*
> + * Copyright (C) 2006 Michael Niedermayer <michaelni at gmx.at>
> + *
> + * This file is part of MPlayer.
> + *
> + * MPlayer 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.
> + *
> + * MPlayer 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 MPlayer; if not, write to the Free Software Foundation, Inc.,
> + * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
> + */

This license header should be updated to say FFmpeg.

> +
> +/**
> + * @file libavfilter/vf_yadif.c

The actual filename is not required.

>[...]
>
> +static void store_ref(YadifContext *yadif, uint8_t *src[3], int src_stride[3], int width, int height) {
> +    int i;
> +
> +    memcpy(yadif->ref[3], yadif->ref[0], sizeof(uint8_t *)*3);
> +    memmove(yadif->ref[0], yadif->ref[1], sizeof(uint8_t *)*3*3);
> +
> +    for(i=0; i<3; i++){
> +        int is_chroma= !!i;
> +        memcpy_pic(yadif->ref[2][i], src[i], width>>is_chroma, height>>is_chroma, yadif->stride[i], src_stride[i]);
> +    }
> +}

Why do you copy the frames?  You should be able to simply store the
input AVFilterPicRef pointers.  You'll need to set rej_perms for your
input pad to reject AV_PERM_REUSE2, and free the references when you7re
done with them.

>[...]
>
> +
> +static void filter(YadifContext *yadif, uint8_t *dst[3], int dst_stride[3],
> +                   int width, int height, int parity, int tff) { 
> +    int y, i;
> +
> +    for(i=0; i<3; i++) {
> +        int is_chroma= !!i;
> +        int w= width >>is_chroma;
> +        int h= height>>is_chroma;

This chroma subsampling is not generic enough to handle other
colorspaces.  See av_pix_fmt_descriptors in libavutil/pixdesc.h.

> +        int refs= yadif->stride[i];
> +
> +        for(y=0; y<h; y++) {
> +            if((y ^ parity) & 1) {
> +                uint8_t *prev= &yadif->ref[0][i][y*refs];
> +                uint8_t *cur = &yadif->ref[1][i][y*refs];
> +                uint8_t *next= &yadif->ref[2][i][y*refs];
> +                uint8_t *dst2= &dst[i][y*dst_stride[i]];
> +                filter_line(yadif, dst2, prev, cur, next, w, refs, parity ^ tff);
> +            } else {
> +                memcpy(&dst[i][y*dst_stride[i]], &yadif->ref[1][i][y*refs], w);
> +            }
> +        }
> +    }
> +}
> +
> +static int config_props_input(AVFilterLink *link)
> +{
> +    YadifContext *yadif = link->dst->priv;
> +    int i, j;
> +
> +    for(i=0; i<3; i++) {
> +        int is_chroma= !!i;
> +        int w= ((link->w     + 31) & (~31))>>is_chroma;
> +        int h= ((link->h + 6 + 31) & (~31))>>is_chroma;
> +
> +        yadif->stride[i]= w;
> +        for(j=0; j<3; j++)
> +            yadif->ref[j][i]= (uint8_t *)(av_mallocz(w*h*sizeof(uint8_t)))+3*w;

If the frame copying is because you need a particular stride alignment,
then it would be better to extend avfilter_get_buffer to let you
specify that requirement.

> +    }
> +    return 0;
> +}
> +
> +static void continue_buffered_image(AVFilterContext *ctx)
> +{
> +    YadifContext *yadif = ctx->priv;
> +    AVFilterPicRef *picref = yadif->buffered_pic;
> +    AVFilterPicRef *dpicref = ctx->outputs[0]->outpic;
> +    AVFilterLink *out = ctx->outputs[0];
> +    int tff = yadif->buffered_tff;
> +    int i;
> +
> +    dpicref = avfilter_get_video_buffer(out, AV_PERM_WRITE, picref->w, picref->h);
> +    if(yadif->start_deinterlace == 0) {
> +        yadif->start_deinterlace = 1;
> +        dpicref->pts = picref->pts;
> +        dpicref->pos = picref->pos;
> +        dpicref->fields = picref->fields;
> +        avfilter_start_frame(out, avfilter_ref_pic(dpicref, ~0));
> +        avfilter_unref_pic(picref);
> +        avfilter_draw_slice(out, 0, dpicref->h, 1);
> +        avfilter_end_frame(out);
> +        avfilter_unref_pic(dpicref);
> +        return;
> +    }

I think it should be okay to not output a frame if you simply don't have
anything to output.

> + 
> +    for(i = 0; i<=(yadif->mode & 1); i++) {
> +        dpicref->pts = picref->pts + i*0.02; // FIXME : Assuming 2*25fps here
> +        dpicref->fields = picref->fields;
> +        dpicref->pos = picref->pos;

You should also copy over the aspect ratio field.

> +        filter(yadif, dpicref->data, dpicref->linesize, picref->w, picref->h, i ^ tff ^ 1, tff);
> +        avfilter_start_frame(out, avfilter_ref_pic(dpicref, ~0));

The call to avfilter_ref_pic is unnecessary.  By passing a picture
reference to avfilter_start_frame, you give ownership of that reference
to the next filter.  If you don't need to use the reference any more in
your own code, you can just give it away, rather than creating a
duplicate reference and unrefing the original.

> +        avfilter_draw_slice(out, 0, dpicref->h, 1);
> +        avfilter_end_frame(out);
> +        avfilter_unref_pic(dpicref);
> +        if(i < (yadif->mode & 1))
> +            dpicref = avfilter_get_video_buffer(out, AV_PERM_WRITE, picref->w, picref->h);
> +    }
> +    avfilter_unref_pic(picref);
> +    return;
> +}
> +
> +static void start_frame(AVFilterLink *link, AVFilterPicRef *picref)
> +{
> +    return;
> +}
> +
> +static void end_frame(AVFilterLink *link)
> +{
> +    YadifContext *yadif = link->dst->priv;
> +    AVFilterPicRef *picref  = link->cur_pic;
> +    int tff;
> +
> +    if(yadif->parity < 0) {
> +        if (picref->fields & MP_IMGFIELD_ORDERED)
> +            tff = !!(picref->fields & MP_IMGFIELD_TOP_FIRST);

These flags should be renamed.  They aren't part of mplayer here.

> +        else
> +            tff = 1;
> +    } else
> +         tff = (yadif->parity&1)^1;
> +
> +    store_ref(yadif, picref->data, picref->linesize, picref->w, picref->h);
> +
> +    yadif->buffered_pic = picref;
> +    yadif->buffered_tff = tff;
> +    yadif->buffered_pts = picref->pts;
> +
> +    continue_buffered_image(link->dst);
> +    return;
> +}
>[...]


-- 
Bobby Bingham
このメールは再利用されたバイトでできている。