[FFmpeg-devel] [PATCH v3 5/5] lavfi: addroi filter

[Moritz Barsnick]

On Tue, Jun 04, 2019 at 00:19:05 +0100, Mark Thompson wrote:
> This can be used to add region of interest side data to video frames.

Very valuable addition for use of ROI the command line tool!

> +Mark regions of interest in a video frame.

Since you're using the plural, it's probably worth mentioning how to
specify several regions.

> + at item x
> +Region distance in pixels from the left edge of the frame.

Mention that it's an expression, and that it (or all four) support "iw"
and "ih".

> + at item qoffset
> +Quantisation offset to apply within the region.
> +
> +Must be in the range -1 to +1.

It would be nice to mention somewhere that it's a float (even though
the text implies that.)

> + at item clear
> +Remove any existing regions of interest marked on the frame before
> +adding the new one.

Mention that it needs to be "1", or that it's a boolean.

Overall, very well described. An example or two would be very welcome
though.

> +    for (i = 0; i < 4; i++) {
> +        int max_value;
> +        switch (i) {

I like avoiding magic numbers such as 4, and would prefer sizeof(addroi_param_names)
- but that's probably just me.

> +        av_assert0(old_roi_size && sd->size % old_roi_size == 0);

Someone recently posted a patch splitting up composite assert()s. I
don't recall whether it was merged though.

> +static av_cold int addroi_init(AVFilterContext *avctx)
> +{
> +    AddROIContext *ctx = avctx->priv;
> +    int i, err;
> +
> +    for (i = 0; i < 4; i++) {

Magic 4 again (also in uninit()).

Cheers,
Moritz