[FFmpeg-devel] [PATCH] add top video filter

[Stefano Sabatini]

On date Monday 2011-03-28 14:47:20 +0100, Mark Himsley encoded:
> Dear developers,
> 
> Converting to and from interlaced PAL DV files, with their
> bottom-field-first interlace field order, can be a pain. Converting
> tff files to DV results in tff DV files, which are hard to work with
> in editing software.
> 
> The attached filter can:
> 
> Convert field order by either moving all of the lines in the picture
> up by 1 line (bff to tff conversion) or down by 1 line (tff to bff
> conversion). The remaining line, the bottom line in bff to tff
> transforms or the top line in tff to bff transforms, is filled by
> copying the closest line in that field.
> 
> Previous to this filter I have used a filter chain like this to do
> bff to tff conversion.
> 
> format=yuv422p,crop=720:575:0:1,pad=720:576:0:0:black
> 
> but that chain does not fill the remaining line.

swapfields looks a good name to me, more comments follow.

[...]
> +static av_cold int init(AVFilterContext *ctx, const char *args, void *opaque)
> +{
> +    TopContext *top = ctx->priv;
> +
> +    if(args && sscanf(args, "%u", &top->dst_tff) == 1) {

Nit+++: if_(

(I'll fix it when committing if you don't care, so feel free to skip
all the nits comments).
 
I prefer to adopt positive logic, that is

   if (!args || !sscanf(args, "%u", &top->dst_tff) != 1) {
       av_log(ctx, AV_LOG_ERROR,
              "Expected 1 argument '#':'%s'\n", args);
       return AVERROR(EINVAL);
  }

  top->dst_tff = !!top->dst_tff;
  av_log(ctx, AV_LOG_INFO, "ttf:%d\n", top->dst_tff);
  return 0;

it should also be slightly more readable.

> +static int query_formats(AVFilterContext *ctx)
> +{
> +    enum PixelFormat pix_fmts[] = {
> +        PIX_FMT_YUV444P,  PIX_FMT_YUV422P,
> +        PIX_FMT_YUVJ444P, PIX_FMT_YUVJ422P,
> +        PIX_FMT_RGB24,    PIX_FMT_BGR24,
> +        PIX_FMT_ARGB,     PIX_FMT_ABGR,
> +        PIX_FMT_RGBA,     PIX_FMT_BGRA,
> +        PIX_FMT_NONE
> +    };

Same consideration by Michael, RGB48 (recently added) should be
already supported by this logic. Another cool idea would be to select
formats based on chroma shift.

> +static int config_input(AVFilterLink *link)

Nit: link -> inlink
here and below.

> +{
> +    TopContext *top = link->dst->priv;
> +    int is_packed_rgba;
> +    int plane;
> +
> +    is_packed_rgba = 1;
> +    switch (link->format) {
> +    case PIX_FMT_ARGB:
> +    case PIX_FMT_ABGR:
> +    case PIX_FMT_RGBA:
> +    case PIX_FMT_BGRA:
> +    case PIX_FMT_RGB24:
> +    case PIX_FMT_BGR24: break;
> +    default:
> +        is_packed_rgba = 0;
> +    }
> +
> +    if (is_packed_rgba) {
> +        top->line_step[0] = (av_get_bits_per_pixel(&av_pix_fmt_descriptors[link->format]))>>3;

Nit: unnecessary parentheses around (av_get_bits_per_pixel...)

> +    } else {
> +        for (plane = 0; plane < 4; plane++) {
> +            top->line_step[plane] = 1;
> +        }
> +    }

Maybe you can use libavutil/imgutils.h:av_image_fill_max_pixsteps().

> +    return 0;
> +}
> +
> +static AVFilterBufferRef *get_video_buffer(AVFilterLink *link, int perms, int w, int h)
> +{
> +    return avfilter_get_video_buffer(link->dst->outputs[0], perms, w, h);
> +}
> +
> +static void start_frame(AVFilterLink *link, AVFilterBufferRef *picref)
> +{
> +    AVFilterBufferRef *outpicref = avfilter_ref_buffer(picref, ~0);
> +
> +    link->dst->outputs[0]->out_buf = outpicref;
> +
> +    avfilter_start_frame(link->dst->outputs[0], outpicref);
> +}
> +
> +static void end_frame(AVFilterLink *link)
> +{
> +    TopContext *top      = link->dst->priv;

AVFilterLink *outlink = link->dst->outputs[0];

may improve readability.

> +
> +    if (link->cur_buf->video->interlaced) {
> +        link->dst->outputs[0]->out_buf->video->top_field_first = top->dst_tff;
> +    }
> +
> +    avfilter_end_frame(link->dst->outputs[0]);
> +    avfilter_unref_buffer(link->cur_buf);
> +}
> +
> +static void draw_slice(AVFilterLink *link, int y, int h, int slice_dir)
> +{
> +    AVFilterContext *ctx = link->dst;
> +    TopContext *top      = ctx->priv;
> +
> +    AVFilterBufferRef *inpic = link->cur_buf;
> +    AVFilterBufferRef *outpic = link->dst->outputs[0]->out_buf;
> +    int i, plane;
> +    uint8_t *cpy_dst;
> +
> +    if (inpic->video->interlaced) {
> +        if (inpic->video->top_field_first != top->dst_tff) {

> +            av_log(ctx, AV_LOG_DEBUG,
> +                    "picture will move %s one line\n",
> +                    top->dst_tff ? "up" : "down");

av_dlog(ctx, "...", ) or this will spam the log with -loglevel debug

(and put a /* #define DEBUG */ at the top of the file)

> +            for (plane = 0; plane < 4 && outpic->data[plane]; plane++) {
> +                cpy_dst = outpic->data[plane] + y * outpic->linesize[plane];
> +                if (top->dst_tff) {
> +                    for (i = 0; i < h; i++) {
> +                        if (1 + y + i < outpic->video->h) {
> +                            memcpy(cpy_dst, cpy_dst + outpic->linesize[plane], outpic->linesize[plane]);

You're copying more than it is required, outpic->linesize[plane] -> line_step[plane] * w;

> +                        } else {
> +                            memcpy(cpy_dst, cpy_dst - 2 * outpic->linesize[plane], outpic->linesize[plane]);
> +                        }

If I'm correct this is copy the line which is tow lines up the current
one, that is:
...
line N-2
line N-1
line N-2

Can you confirm this is the standard behavior? (also it should be
possibly documented).

> +                        cpy_dst += outpic->linesize[plane];
> +                    }
> +                } else {
> +                    cpy_dst += (h -1) * outpic->linesize[plane];
> +                    for (i = h -1; i >= 0 ; i--) {
> +                        if ( y + i > 0) {
> +                            memcpy( cpy_dst, cpy_dst - outpic->linesize[plane], outpic->linesize[plane] );
> +                        } else {
> +                            memcpy( cpy_dst, cpy_dst + 2 * outpic->linesize[plane], outpic->linesize[plane] );
> +                        }
> +                        cpy_dst -= outpic->linesize[plane];
> +                    }
> +                }
> +            }

> +        } else {

> +            av_log(ctx, AV_LOG_DEBUG,
> +                    "field order already correct\n");

av_dlog()

> +        }
> +    }
> +
> +    avfilter_draw_slice(link->dst->outputs[0], y, h, slice_dir);
> +}

Is top->line_step() used?
-- 
FFmpeg = Faithful and Foolish Minimal Peaceful Eretic Gem