[FFmpeg-devel] [PATCH V1] avfilter: Add tonemap vaapi filter

[Moritz Barsnick]

On Tue, Dec 25, 2018 at 15:16:17 +0800, Zachary Zhou wrote:
> It supports ICL platform.
> H2H (HDR to HDR): P010 -> RGB10
> H2S (HDR to SDR): P010 -> RGB8
> 
> libva commit for HDR10
> https://github.com/intel/libva/commit/cf11abe5e1b9c93ee75cf974076957162c1605b9

BTW, there's a typo in your libva doxygen comments ("berief" <->
"brief"). Did doxygen even run successfully?

[...]
> +check_struct "va/va.h va/va_vpp.h" "VAProcPipelineParameterBuffer" output_hdr_metadata

Does libva really not do any micro version bumping or the likes when
adding features?

> +int ff_vaapi_vpp_make_param_buffers2(AVFilterContext *avctx,

Shouldn't ff_vaapi_vpp_make_param_buffers() be converted to use this?
Eventually?

> +#define LAV_UINT32(a) (a.num)
> +            in_metadata->max_display_mastering_luminance =
> +                LAV_UINT32(hdr_meta->max_luminance);
> +            in_metadata->min_display_mastering_luminance =
> +                LAV_UINT32(hdr_meta->min_luminance);
> +#undef LAV_UINT16

What are you undefining here? Didn't you mean to undef LAV_UINT32?

> +                if (!metadata) {
> +                    av_log(avctx, AV_LOG_ERROR, "Can't new side data for output\n");

"new" is not really a verb. ;-) "Can't create new ..."? ("Cannot"
preferred, actually.)

> +                if (!metadata_lt) {
> +                    av_log(avctx, AV_LOG_ERROR, "Can't new side data for output\n");

Ditto.

> +    if ((err = ff_formats_ref(ff_make_format_list(pix_in_fmts),
> +                              &avctx->inputs[0]->out_formats)) < 0)
> +        return err;

There was some discussion here that the style
    err = ff_formats_ref(ff_make_format_list(pix_in_fmts), &avctx->inputs[0]->out_formats)
    if (err < 0)
        return err;

is preferred for readability.

Sorry that some remarks are in form of questions. I'm not totally sure.

Cheers,
Moritz