Re: [FFmpeg-devel] [PATCH 1/2] avfilter/vf_showinfo: add AVFilmGrainAOMParams support

[Stefano Sabatini <stefasab@gmail.com>]

On date Tuesday 2024-02-27 14:43:12 +0100, Niklas Haas wrote:
> From: Niklas Haas <git@haasn.dev>
> 

> For my own testing purposes.

You can drop "my own".

> ---
>  libavfilter/vf_showinfo.c | 42 +++++++++++++++++++++++++++++++++++++--
>  1 file changed, 40 insertions(+), 2 deletions(-)
> 
> diff --git a/libavfilter/vf_showinfo.c b/libavfilter/vf_showinfo.c
> index 309de28df91..830170363bc 100644
> --- a/libavfilter/vf_showinfo.c
> +++ b/libavfilter/vf_showinfo.c
> @@ -462,8 +462,46 @@ static void dump_sei_film_grain_params_metadata(AVFilterContext *ctx, const AVFr
>  
>      switch (fgp->type) {
>      case AV_FILM_GRAIN_PARAMS_NONE:
> -    case AV_FILM_GRAIN_PARAMS_AV1:
> -        return;
> +        break;
> +    case AV_FILM_GRAIN_PARAMS_AV1: {
> +        const AVFilmGrainAOMParams *aom = &fgp->codec.aom;
> +        const int num_ar_coeffs_y = 2 * aom->ar_coeff_lag * (aom->ar_coeff_lag + 1);
> +        const int num_ar_coeffs_uv = num_ar_coeffs_y + !!aom->num_y_points;
> +        av_log(ctx, AV_LOG_INFO, "y_points={ ");

> +        for (int i = 0; i < aom->num_y_points; i++)
> +            av_log(ctx, AV_LOG_INFO, "{%d, %d} ", aom->y_points[i][0], aom->y_points[i][1]);

nit++: other instances make use of () to delimit vector values

> +        av_log(ctx, AV_LOG_INFO, "}; chroma_scaling_from_luma=%d; ", aom->chroma_scaling_from_luma);
> +        for (int uv = 0; uv < 2; uv++) {
> +            av_log(ctx, AV_LOG_INFO, "uv_points[%d]={ ", uv);
> +            for (int i = 0; i < aom->num_uv_points[uv]; i++)
> +                av_log(ctx, AV_LOG_INFO, "{%d %d} ", aom->uv_points[uv][i][0], aom->uv_points[uv][i][1]);
> +            av_log(ctx, AV_LOG_INFO, "}; ");
> +        }
> +        av_log(ctx, AV_LOG_INFO, "scaling_shift=%d; ", aom->scaling_shift);
> +        av_log(ctx, AV_LOG_INFO, "ar_coeff_lag=%d; ", aom->ar_coeff_lag);

nit: using "," seems more consistent with other instances, here and below

> +        if (num_ar_coeffs_y) {
> +            av_log(ctx, AV_LOG_INFO, "ar_coeffs_y={ ");
> +            for (int i = 0; i < num_ar_coeffs_y; i++)
> +                av_log(ctx, AV_LOG_INFO, "%d ", aom->ar_coeffs_y[i]);
> +            av_log(ctx, AV_LOG_INFO, "}; ");
> +        }
> +        for (int uv = 0; num_ar_coeffs_uv && uv < 2; uv++) {
> +            av_log(ctx, AV_LOG_INFO, "ar_coeffs_uv[%d]={ ", uv);
> +            for (int i = 0; i < num_ar_coeffs_uv; i++)
> +                av_log(ctx, AV_LOG_INFO, "%d ", aom->ar_coeffs_uv[uv][i]);
> +            av_log(ctx, AV_LOG_INFO, "}; ");
> +        }
> +        av_log(ctx, AV_LOG_INFO, "ar_coeff_shift=%d; ", aom->ar_coeff_shift);
> +        av_log(ctx, AV_LOG_INFO, "grain_scale_shift=%d; ", aom->grain_scale_shift);
> +        for (int uv = 0; uv < 2; uv++) {
> +            av_log(ctx, AV_LOG_INFO, "uv_mult[%d] = %d; ", uv, aom->uv_mult[uv]);
> +            av_log(ctx, AV_LOG_INFO, "uv_mult_luma[%d] = %d; ", uv, aom->uv_mult_luma[uv]);
> +            av_log(ctx, AV_LOG_INFO, "uv_offset[%d] = %d; ", uv, aom->uv_offset[uv]);
> +        }
> +        av_log(ctx, AV_LOG_INFO, "overlap_flag=%d; ", aom->overlap_flag);
> +        av_log(ctx, AV_LOG_INFO, "limit_output_range=%d; ", aom->limit_output_range);
> +        break;
> +    }
>      case AV_FILM_GRAIN_PARAMS_H274: {
>          const AVFilmGrainH274Params *h274 = &fgp->codec.h274;
>          const char *color_range_str     = av_color_range_name(h274->color_range);
> -- 

LGTM otherwise (and no need to send another patch).

Unrelated note: probably we should define a serialization method at
the library level, then we might employ the same code to display the
information here and in ffprobe (this was the work proposed by
Nicolas).
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".