[FFmpeg-devel] [PATCH] lavfi: add pp filter.

[Clément Bœsch]

On Wed, Dec 12, 2012 at 01:18:55PM +0100, Stefano Sabatini wrote:
[...]
> > + at section pp
> > +
> > +Enables the specified chain of postprocessing subfilters using libpostproc.
> 
> Nit: "Enable" for overall consistency.
> 

Fixed locally.

> Also mention that this needs libpostproc to be enabled (mentioning
> --enable-gpl may also avoid some user complaints).
> 

Added "This library should be automatically selected with a GPL build
(@code{--enable-gpl})."

> > +Subfilters must be separated by '/' and can be disabled by prepending a '-'.
> > +Each subfilter and some options have a short and a long name that can be used
> > +interchangeably, i.e. dr/dering are the same.
> 
> Missing description of how to specify options.
> 

After the option list, just added: "These options can be appended after
the subfilter name, separated by a ':'."

> > +All subfilters share common options to determine their scope:
> 
> I believe that libpp parameters/library deserve a dedicated document
> (for the same reason libswresample/libswscale parameters should not be
> documented in filters.texi).
> 

Yes, I just merely copy pasted the filter documentation from MPlayer, I
believe it can be moved later. What's the state of the 

> > +
> > + at table @option
> > + at item a/autoq
> > +Automatically switch the subfilter off if the CPU is too slow.
> > +

Note: this option needs to be documented differently; the idea is to be
able to switch the quality using commands. The CPU-too-slow trigger does
not exist in FFmpeg, so the quality never changes (unless you use the
command system).

[...]
> > + at itemize
> > + at item
> > +Horizontal and vertical deblocking, deringing and automatic brightness/contrast:
> > + at example
> > +-vf pp=hb/vb/dr/al
> > + at end example
> 
> Note: we usually omit the -vf part, and for good reasons, since
> filters.texi is not meant to be bound to ff* tools syntax.
> 

Yep, forgot to remove, fixed locally.

> > +
> > + at item
> > +Default filters without brightness/contrast correction:
> > + at example
> > +-vf pp=de/-al
> > + at end example
> > +
> > + at item
> > +Enable default filters & temporal denoiser:
> > + at example
> > +-vf pp=default/tmpnoise:1:2:3
> > + at end example
> > +
> > + at item
> 
> > +Horizontal deblocking on luminance only and switch vertical deblocking on or
> 
> The verb missing.
> 

It's just like the first two example "[here is how to do] horizontal
deblock + switch vertical deblock on/off:"

[...]
> > +static av_cold int pp_init(AVFilterContext *ctx, const char *args)
> > +{
> > +    int i;
> > +    PPFilterContext *pp = ctx->priv;
> > +
> > +    if (!args || !*args)
> > +        args = "de";
> > +
> > +    for (i = 0; i <= PP_QUALITY_MAX; i++) {
> > +        pp->modes[i] = pp_get_mode_by_name_and_quality(args, i);
> > +        if (!pp->modes[i])
> > +            return AVERROR_EXTERNAL;
> > +    }
> > +    pp->mode_id = PP_QUALITY_MAX;
> > +    return 0;
> 
> Not blocking, but maybe we can think about adding support for
> options. For example it may be useful to be able to set quality.
> 

Well, given that pp uses ':' separator, good luck with this without making
the usage insane. You can still change quality using the command system
for subfilters with the 'a' switch.

> > +}
> > +
> > +static int pp_process_command(AVFilterContext *ctx, const char *cmd, const char *args,
> > +                              char *res, int res_len, int flags)
> > +{
> > +    PPFilterContext *pp = ctx->priv;
> > +
> > +    if (!strcmp(cmd, "quality")) {
> > +        pp->mode_id = av_clip(strtol(args, NULL, 10), 0, PP_QUALITY_MAX);
> > +        return 0;
> > +    }
> > +    return AVERROR(ENOSYS);
> > +}
> > +
> > +static int pp_query_formats(AVFilterContext *ctx)
> > +{
> > +    static const enum PixelFormat pix_fmts[] = {
> > +        AV_PIX_FMT_YUV420P,
> > +        AV_PIX_FMT_YUV422P,
> > +        AV_PIX_FMT_YUV411P,
> > +        AV_PIX_FMT_YUV444P,
> > +        AV_PIX_FMT_NONE
> > +    };
> > +    ff_set_common_formats(ctx, ff_make_format_list(pix_fmts));
> > +    return 0;
> > +}
> > +
> > +static int pp_config_props(AVFilterLink *inlink)
> > +{
> > +    int flags = PP_CPU_CAPS_AUTO;
> > +    PPFilterContext *pp = inlink->dst->priv;
> > +
> > +    switch (inlink->format) {
> > +    case AV_PIX_FMT_YUV420P: flags |= PP_FORMAT_420; break;
> > +    case AV_PIX_FMT_YUV422P: flags |= PP_FORMAT_422; break;
> > +    case AV_PIX_FMT_YUV411P: flags |= PP_FORMAT_411; break;
> > +    case AV_PIX_FMT_YUV444P: flags |= PP_FORMAT_444; break;
> > +    default: av_assert0(0);
> > +    }
> > +
> > +    pp->pp_ctx = pp_get_context(inlink->w, inlink->h, flags);
> > +    if (!pp->pp_ctx)
> > +        return AVERROR(ENOMEM);
> > +    return 0;
> > +}
> > +
> 
> > +static int pp_filter_frame(AVFilterLink *inlink, AVFilterBufferRef *in)
> 
> I hate too generic variable names (in... inlink? inframe? inpad?) as
> they harm readability and slow me down when reading code.
> 

Renamed in/out to inbuf/outbuf.

> > +{
> > +    AVFilterContext *ctx = inlink->dst;
> > +    PPFilterContext *pp = ctx->priv;
> > +    AVFilterLink *outlink = ctx->outputs[0];
> > +    const int aligned_w = FFALIGN(outlink->w, 8);
> > +    const int aligned_h = FFALIGN(outlink->h, 8);
> > +    AVFilterBufferRef *out;
> > +
> > +    out = ff_get_video_buffer(outlink, AV_PERM_WRITE, aligned_w, aligned_h);
> > +    if (!out) {
> > +        avfilter_unref_buffer(in);
> > +        return AVERROR(ENOMEM);
> > +    }
> > +    avfilter_copy_buffer_ref_props(out, in);
> > +
> > +    pp_postprocess((const uint8_t **)in->data, in->linesize,
> > +                   out->data, out->linesize,
> > +                   aligned_w, outlink->h,
> > +                   out->video->qp_table,
> > +                   out->video->qp_table_linesize,
> > +                   pp->modes[pp->mode_id],
> > +                   pp->pp_ctx,
> > +                   out->video->pict_type);
> > +
> > +    avfilter_unref_buffer(in);
> > +    return ff_filter_frame(outlink, out);
> > +}
> > +
> > +static av_cold void pp_uninit(AVFilterContext *ctx)
> > +{
> > +    int i;
> > +    PPFilterContext *pp = ctx->priv;
> > +
> > +    for (i = 0; i <= PP_QUALITY_MAX; i++)
> > +        pp_free_mode(pp->modes[i]);
> > +    if (pp->pp_ctx)
> > +        pp_free_context(pp->pp_ctx);
> > +}
> > +
> > +static const AVFilterPad pp_inputs[] = {
> > +    {
> > +        .name         = "default",
> > +        .type         = AVMEDIA_TYPE_VIDEO,
> > +        .config_props = pp_config_props,
> > +        .filter_frame = pp_filter_frame,
> > +        .min_perms    = AV_PERM_READ,
> > +    },
> > +    { NULL }
> > +};
> > +
> > +static const AVFilterPad pp_outputs[] = {
> > +    {
> > +        .name = "default",
> > +        .type = AVMEDIA_TYPE_VIDEO,
> > +    },
> > +    { NULL }
> > +};
> > +
> > +AVFilter avfilter_vf_pp = {
> > +    .name            = "pp",
> 
> > +    .description     = NULL_IF_CONFIG_SMALL("filter video using libpostproc"),
> 
> "Filter video ... libpostproc."
> 

This was obviously a new attempt to check if you were still checking the
description of filters, changed locally.

[...]

Thanks for the review :)

-- 
Clément B.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 490 bytes
Desc: not available
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20121212/5eafb0a5/attachment.asc>