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

[Stefano Sabatini]

On date Wednesday 2012-12-12 20:56:02 +0100, Clément Bœsch encoded:
> 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 

I'm not currently working on it (got sidetracked again).
Still missing:

1. texi2pod.pl fix to properly support @include directives outside @c man
  c ... @c man end stuff (this may be painful)
2. configuration for monolithic documentation (trivial, already
  implemented)
3. remove inconditional inclusion of subcomponent manuals in tools manpage
4. link the component pages from the website documentation section

If 1. seems too complicated I'd suggest to just drop support to
monolithic pages (which right now are close to unreadable), and
proceed with point 3 - 4.

Note that there is nothing preventing you from creating a dedicated
page for libpostproc(3) and maybe a ffmpeg-postprocess-filters(1)
manual.

Feel free to delay this move after the patch application.
 
> > > +
> > > + 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:"

Better to keep each example grammatically independent, and avoid
copy&paste errors.

> 
> [...]
> > > +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.

Would be possible to make pp accept another separator char?
Alternatively we may add (configurable support) to a different
separator.

Anyway setting the option seems a good idea, but still this is not
blocking.

> 
> > > +}
> > > +
> > > +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.

Thanks.

[...] 
-- 
FFmpeg = Free & Frightening MultiPurpose Eccentric Gangster