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

[Zhou, Zachary]

> -----Original Message-----
> From: ffmpeg-devel [mailto:ffmpeg-devel-bounces at ffmpeg.org] On Behalf Of
> Moritz Barsnick
> Sent: Tuesday, December 25, 2018 6:06 PM
> To: FFmpeg development discussions and patches <ffmpeg-devel at ffmpeg.org>
> Subject: Re: [FFmpeg-devel] [PATCH V1] avfilter: Add tonemap vaapi filter
> 
> 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/cf11abe5e1b9c93ee75cf97407695716
> > 2c1605b9
> 
> BTW, there's a typo in your libva doxygen comments ("berief" <-> "brief"). Did
> doxygen even run successfully?

I am not send patches to libva, but I can report it to them.

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

currently I am not sure about the micro version changes in libva, but I will keep watch the version bumping.

> 
> > +int ff_vaapi_vpp_make_param_buffers2(AVFilterContext *avctx,
> 
> Shouldn't ff_vaapi_vpp_make_param_buffers() be converted to use this?
> Eventually?

ff_vaapi_vpp_make_param_buffers doesn't work for this filter. because this filter read metadata from input AVFrame, and ff_vaapi_vpp_make_param_buffers usually is used the filter buffer parameter doesn't change during the VPP.

> 
> > +#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?

Got it , I will fix this.

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

Got it, I will fix this.

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

Got it, I will fix this.

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

Got it, I will do the change.

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

Thank you for the comments,
Zachary

> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel