[FFmpeg-devel] [PATCH] avfilter: add radial and circular blur video filter

[Alexander Strasser]

Hi Paul!

Only doc comments. Feel free to apply the changes when the real
review has completed.

On 2020-07-12 13:01 +0200, Paul B Mahol wrote:
> Signed-off-by: Paul B Mahol <onemda at gmail.com>
> ---
>  doc/filters.texi         |  46 ++++
>  libavfilter/Makefile     |   2 +
>  libavfilter/allfilters.c |   2 +
>  libavfilter/vf_rblur.c   | 476 +++++++++++++++++++++++++++++++++++++++
>  4 files changed, 526 insertions(+)
>  create mode 100644 libavfilter/vf_rblur.c
>
> diff --git a/doc/filters.texi b/doc/filters.texi
> index ccc066a9ab..8c5006ce04 100644
> --- a/doc/filters.texi
> +++ b/doc/filters.texi
> @@ -7231,6 +7231,29 @@ Set planes to filter. Default value is to filter all
>  planes except alpha plane.
>  @end table
>
> + at section cblur

> +Apply Circular blur filter.

I suggest not upper casing circular and using the indefinite article:

    Apply a circular blur filter.


> +
> +The filter accepts the following options:
> +
> + at table @option
> + at item centerx, centery
> +Set central point position of circular blur. Default is @code{0.5}.
> +
> + at item angle
> +Set angle of circular blur in degrees. Default is @code{5}.
> +
> + at item planes
> +Set which planes to filter. By default all planes are filtered.
> + at end table
> +
> + at subsection Commands

> +This filter supports same @ref{commands} as options.

To me the sentence sounds a bit weird. I suggest:

    All options are supported as @ref{commands}.

If you prefer, use that instead.

Now on the content itself. It bears the danger, that at some point
options and commands could diverge. So one needs to keep in mind
to update the documenation accordingly; should it ever happen.

I agree that not duplicating anything is preferable for now.


> +The command accepts the same syntax of the corresponding option.

Maybe:

   Each command accepts the syntax of the corresponding option.


> +
> +If the specified expression is not valid, it is kept at its current
> +value.
> +

All my remarks apply too the rblur filter too.


  Alexander

[...]