[FFmpeg-devel] [PATCH v3] avfilter: Add tonemap vaapi filter for H2S

[Song, Ruiling]

> -----Original Message-----
> From: ffmpeg-devel <ffmpeg-devel-bounces at ffmpeg.org> On Behalf Of
> Vittorio Giovara
> Sent: Tuesday, December 3, 2019 2:28 AM
> To: FFmpeg development discussions and patches <ffmpeg-
> devel at ffmpeg.org>
> Cc: Sun, Xinpeng <xinpeng.sun at intel.com>; Zhou, Zachary
> <zachary.zhou at intel.com>
> Subject: Re: [FFmpeg-devel] [PATCH v3] avfilter: Add tonemap vaapi filter for
> H2S
> 
> On Mon, Dec 2, 2019 at 2:19 AM Xinpeng Sun <xinpeng.sun at intel.com>
> wrote:
> 
> > It performs HDR(High Dynamic Range) to SDR(Standard Dynamic Range)
> > conversion
> > with tone-mapping. It only supports HDR10 as input temporarily.
> >
> > An example command to use this filter with vaapi codecs:
> > FFMPEG -hwaccel vaapi -vaapi_device /dev/dri/renderD128
> > -hwaccel_output_format vaapi \
> > -i INPUT -vf 'tonemap_vaapi=format=p010' -c:v hevc_vaapi -profile 2
> OUTPUT
> >
> > Signed-off-by: Xinpeng Sun <xinpeng.sun at intel.com>
> > Signed-off-by: Zachary Zhou <zachary.zhou at intel.com>
> > ---
> >  configure                      |   2 +
> >  doc/filters.texi               |  81 +++++++
> >  libavfilter/Makefile           |   1 +
> >  libavfilter/allfilters.c       |   1 +
> >  libavfilter/vf_tonemap_vaapi.c | 420
> +++++++++++++++++++++++++++++++++
> >  5 files changed, 505 insertions(+)
> >  create mode 100644 libavfilter/vf_tonemap_vaapi.c
> >
[...]
> > +static int tonemap_vaapi_save_metadata(AVFilterContext *avctx,
> AVFrame
> > *input_frame)
> > +{
> > +    HDRVAAPIContext *ctx = avctx->priv;
> > +    AVMasteringDisplayMetadata *hdr_meta;
> > +    AVContentLightMetadata *light_meta;
> > +
> > +    if (input_frame->color_trc != AVCOL_TRC_SMPTE2084) {
> > +        av_log(avctx, AV_LOG_WARNING, "Only support HDR10 as input for
> > vaapi tone-mapping\n");
> > +        input_frame->color_trc = AVCOL_TRC_SMPTE2084;
I think we don't need to modify the input->color_trc here. I am not sure if this has any side-effect, but may be misleading if you want to check that value when debugging.
Simply remove this single line would be ok.

[...]
> > +    err = av_frame_copy_props(output_frame, input_frame);
> > +    if (err < 0)
> > +        return err;
> > +
> > +    if (ctx->color_primaries != AVCOL_PRI_UNSPECIFIED)
> > +        output_frame->color_primaries = ctx->color_primaries;
> > +
> > +    if (ctx->color_transfer != AVCOL_TRC_UNSPECIFIED)
> > +        output_frame->color_trc = ctx->color_transfer;
> > +    else
> > +        output_frame->color_trc = AVCOL_TRC_BT709
> >
> 
> why does only this setting get special treatment?
Basically for other properties we can copy from the source, but for color_trc, we cannot.
And I guess bt709 is a widely used sdr format. So even if user does not give a target transfer characteristic, we use this default one.

[...]
> 
> Overall this lgtm, I'd push it but I don't have a platform to test it on.
Really appreciate that. I borrow an icelake from other team member and have a test on this patch, the tone-mapping result video basically looks good.

Ruiling
> --
> Vittorio
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request at ffmpeg.org with subject "unsubscribe".