[FFmpeg-devel,4/8] lavfi/vf_vpp_qsv: add vpp_preinit callback

From: Haihao Xiang <haihao.xiang@intel.com>

Set the expected default value for options in this callback, hence we
have the right values even if these options are not included in the
option arrray.

This is in preparation for reusing the code for other QSV filters.

Signed-off-by: Haihao Xiang <haihao.xiang@intel.com>
---
 libavfilter/vf_vpp_qsv.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

Xiang, Haihao:
> From: Haihao Xiang <haihao.xiang@intel.com>
> 
> Set the expected default value for options in this callback, hence we
> have the right values even if these options are not included in the
> option arrray.
> 
> This is in preparation for reusing the code for other QSV filters.
> 
> Signed-off-by: Haihao Xiang <haihao.xiang@intel.com>
> ---
>  libavfilter/vf_vpp_qsv.c | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
> 
> diff --git a/libavfilter/vf_vpp_qsv.c b/libavfilter/vf_vpp_qsv.c
> index cf11cd7fdc..2a7b06fa33 100644
> --- a/libavfilter/vf_vpp_qsv.c
> +++ b/libavfilter/vf_vpp_qsv.c
> @@ -259,6 +259,19 @@ release:
>      return ret;
>  }
>  
> +static av_cold int vpp_preinit(AVFilterContext *ctx)
> +{
> +    VPPContext  *vpp  = ctx->priv;
> +    /* For AV_OPT_TYPE_STRING options, NULL is handled in other way so
> +     * we needn't set default value here
> +     */
> +    vpp->saturation = 1.0;
> +    vpp->contrast = 1.0;
> +    vpp->transpose = -1;
> +
> +    return 0;
> +}
> +
>  static av_cold int vpp_init(AVFilterContext *ctx)
>  {
>      VPPContext  *vpp  = ctx->priv;
> @@ -683,6 +696,7 @@ const AVFilter ff_vf_vpp_qsv = {
>      .name          = "vpp_qsv",
>      .description   = NULL_IF_CONFIG_SMALL("Quick Sync Video VPP."),
>      .priv_size     = sizeof(VPPContext),
> +    .preinit       = vpp_preinit,
>      .init          = vpp_init,
>      .uninit        = vpp_uninit,
>      FILTER_INPUTS(vpp_inputs),

I do not get the point of this at all: None of these options are of type
AV_OPT_TYPE_STRING. You are merely setting the default values that
av_opt_set_defaults() would set lateron anyway.

I also fail to see how this would facilitate reusing this code for other
QSV filters.

- Andreas

On Do, 2023-01-12 at 05:11 +0100, Andreas Rheinhardt wrote:
> Xiang, Haihao:
> > From: Haihao Xiang <haihao.xiang@intel.com>
> > 
> > Set the expected default value for options in this callback, hence we
> > have the right values even if these options are not included in the
> > option arrray.
> > 
> > This is in preparation for reusing the code for other QSV filters.
> > 
> > Signed-off-by: Haihao Xiang <haihao.xiang@intel.com>
> > ---
> >  libavfilter/vf_vpp_qsv.c | 14 ++++++++++++++
> >  1 file changed, 14 insertions(+)
> > 
> > diff --git a/libavfilter/vf_vpp_qsv.c b/libavfilter/vf_vpp_qsv.c
> > index cf11cd7fdc..2a7b06fa33 100644
> > --- a/libavfilter/vf_vpp_qsv.c
> > +++ b/libavfilter/vf_vpp_qsv.c
> > @@ -259,6 +259,19 @@ release:
> >      return ret;
> >  }
> >  
> > +static av_cold int vpp_preinit(AVFilterContext *ctx)
> > +{
> > +    VPPContext  *vpp  = ctx->priv;
> > +    /* For AV_OPT_TYPE_STRING options, NULL is handled in other way so
> > +     * we needn't set default value here
> > +     */
> > +    vpp->saturation= 1.0;
> > +    vpp->contrast = 1.0;
> > +    vpp->transpose = -1;
> > +
> > +    return 0;
> > +}
> > +
> >  static av_cold int vpp_init(AVFilterContext *ctx)
> >  {
> >      VPPContext  *vpp  = ctx->priv;
> > @@ -683,6 +696,7 @@ const AVFilter ff_vf_vpp_qsv = {
> >      .name          = "vpp_qsv",
> >      .description   = NULL_IF_CONFIG_SMALL("Quick Sync Video VPP."),
> >      .priv_size     = sizeof(VPPContext),
> > +    .preinit       = vpp_preinit,
> >      .init          = vpp_init,
> >      .uninit        = vpp_uninit,
> >      FILTER_INPUTS(vpp_inputs),
> 
> I do not get the point of this at all: None of these options are of type
> AV_OPT_TYPE_STRING. You are merely setting the default values that
> av_opt_set_defaults() would set lateron anyway.

> 
> I also fail to see how this would facilitate reusing this code for other
> QSV filters.

Actually this is a part of an old patchset, see 
https://patchwork.ffmpeg.org/project/ffmpeg/list/?series=4467, I want to use use
functions and data structures defined in vf_vpp_qsv.c and qsvvpp.c to implement
scale_qsv and deinterlace_qsv filters, hence we may remove vf_scale_qsv.c and
vf_deinterlace_qsv.c 

E.g.
saturation option is used in vpp_qsv filter but not in other qsv filters, so
av_opt_set_defaults() works for saturation in vpp_qsv filter but not for other
filters, we need to set a default value to vpp->saturation for other filters,
otherwise vpp->saturation is 0 for other other filters. 
 
Thanks
Haihao


> 
> - Andreas
> 
> _______________________________________________
> 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".

Xiang, Haihao:
> On Do, 2023-01-12 at 05:11 +0100, Andreas Rheinhardt wrote:
>> Xiang, Haihao:
>>> From: Haihao Xiang <haihao.xiang@intel.com>
>>>
>>> Set the expected default value for options in this callback, hence we
>>> have the right values even if these options are not included in the
>>> option arrray.
>>>
>>> This is in preparation for reusing the code for other QSV filters.
>>>
>>> Signed-off-by: Haihao Xiang <haihao.xiang@intel.com>
>>> ---
>>>  libavfilter/vf_vpp_qsv.c | 14 ++++++++++++++
>>>  1 file changed, 14 insertions(+)
>>>
>>> diff --git a/libavfilter/vf_vpp_qsv.c b/libavfilter/vf_vpp_qsv.c
>>> index cf11cd7fdc..2a7b06fa33 100644
>>> --- a/libavfilter/vf_vpp_qsv.c
>>> +++ b/libavfilter/vf_vpp_qsv.c
>>> @@ -259,6 +259,19 @@ release:
>>>      return ret;
>>>  }
>>>  
>>> +static av_cold int vpp_preinit(AVFilterContext *ctx)
>>> +{
>>> +    VPPContext  *vpp  = ctx->priv;
>>> +    /* For AV_OPT_TYPE_STRING options, NULL is handled in other way so
>>> +     * we needn't set default value here
>>> +     */
>>> +    vpp->saturation= 1.0;
>>> +    vpp->contrast = 1.0;
>>> +    vpp->transpose = -1;
>>> +
>>> +    return 0;
>>> +}
>>> +
>>>  static av_cold int vpp_init(AVFilterContext *ctx)
>>>  {
>>>      VPPContext  *vpp  = ctx->priv;
>>> @@ -683,6 +696,7 @@ const AVFilter ff_vf_vpp_qsv = {
>>>      .name          = "vpp_qsv",
>>>      .description   = NULL_IF_CONFIG_SMALL("Quick Sync Video VPP."),
>>>      .priv_size     = sizeof(VPPContext),
>>> +    .preinit       = vpp_preinit,
>>>      .init          = vpp_init,
>>>      .uninit        = vpp_uninit,
>>>      FILTER_INPUTS(vpp_inputs),
>>
>> I do not get the point of this at all: None of these options are of type
>> AV_OPT_TYPE_STRING. You are merely setting the default values that
>> av_opt_set_defaults() would set lateron anyway.
> 
>>
>> I also fail to see how this would facilitate reusing this code for other
>> QSV filters.
> 
> Actually this is a part of an old patchset, see 
> https://patchwork.ffmpeg.org/project/ffmpeg/list/?series=4467, I want to use use
> functions and data structures defined in vf_vpp_qsv.c and qsvvpp.c to implement
> scale_qsv and deinterlace_qsv filters, hence we may remove vf_scale_qsv.c and
> vf_deinterlace_qsv.c 
> 
> E.g.
> saturation option is used in vpp_qsv filter but not in other qsv filters, so
> av_opt_set_defaults() works for saturation in vpp_qsv filter but not for other
> filters, we need to set a default value to vpp->saturation for other filters,
> otherwise vpp->saturation is 0 for other other filters. 
>  

If you need this for other filters, then why do you add and use a
preinit callback for the filter that doesn't need it?

- Andreas

On Do, 2023-01-12 at 05:49 +0100, Andreas Rheinhardt wrote:
> Xiang, Haihao:
> > On Do, 2023-01-12 at 05:11 +0100, Andreas Rheinhardt wrote:
> > > Xiang, Haihao:
> > > > From: Haihao Xiang <haihao.xiang@intel.com>
> > > > 
> > > > Set the expected default value for options in this callback, hence we
> > > > have the right values even if these options are not included in the
> > > > option arrray.
> > > > 
> > > > This is in preparation for reusing the code for other QSV filters.
> > > > 
> > > > Signed-off-by: Haihao Xiang <haihao.xiang@intel.com>
> > > > ---
> > > >  libavfilter/vf_vpp_qsv.c | 14 ++++++++++++++
> > > >  1 file changed, 14 insertions(+)
> > > > 
> > > > diff --git a/libavfilter/vf_vpp_qsv.c b/libavfilter/vf_vpp_qsv.c
> > > > index cf11cd7fdc..2a7b06fa33 100644
> > > > --- a/libavfilter/vf_vpp_qsv.c
> > > > +++ b/libavfilter/vf_vpp_qsv.c
> > > > @@ -259,6 +259,19 @@ release:
> > > >      return ret;
> > > >  }
> > > >  
> > > > +static av_cold int vpp_preinit(AVFilterContext *ctx)
> > > > +{
> > > > +    VPPContext  *vpp  = ctx->priv;
> > > > +    /* For AV_OPT_TYPE_STRING options, NULL is handled in other way so
> > > > +     * we needn't set default value here
> > > > +     */
> > > > +    vpp->saturation= 1.0;
> > > > +    vpp->contrast = 1.0;
> > > > +    vpp->transpose = -1;
> > > > +
> > > > +    return 0;
> > > > +}
> > > > +
> > > >  static av_cold int vpp_init(AVFilterContext *ctx)
> > > >  {
> > > >      VPPContext  *vpp  = ctx->priv;
> > > > @@ -683,6 +696,7 @@ const AVFilter ff_vf_vpp_qsv = {
> > > >      .name          = "vpp_qsv",
> > > >      .description   = NULL_IF_CONFIG_SMALL("Quick Sync Video VPP."),
> > > >      .priv_size     = sizeof(VPPContext),
> > > > +    .preinit       = vpp_preinit,
> > > >      .init          = vpp_init,
> > > >      .uninit        = vpp_uninit,
> > > >      FILTER_INPUTS(vpp_inputs),
> > > 
> > > I do not get the point of this at all: None of these options are of type
> > > AV_OPT_TYPE_STRING. You are merely setting the default values that
> > > av_opt_set_defaults() would set lateron anyway.
> > > I also fail to see how this would facilitate reusing this code for other
> > > QSV filters.
> > 
> > Actually this is a part of an old patchset, see 
> > https://patchwork.ffmpeg.org/project/ffmpeg/list/?series=4467, I want to use
> > use
> > functions and data structures defined in vf_vpp_qsv.c and qsvvpp.c to
> > implement
> > scale_qsv and deinterlace_qsv filters, hence we may remove vf_scale_qsv.c
> > and
> > vf_deinterlace_qsv.c 
> > 
> > E.g.
> > saturation option is used in vpp_qsv filter but not in other qsv filters, so
> > av_opt_set_defaults() works for saturation in vpp_qsv filter but not for
> > other
> > filters, we need to set a default value to vpp->saturation for other
> > filters,
> > otherwise vpp->saturation is 0 for other other filters. 
> >  
> 
> If you need this for other filters, then why do you add and use a
> preinit callback for the filter that doesn't need it?

I want to make sure these filters (include vpp_qsv filter) have the same default
values for options at preinit stage. 

Thanks
Haihao

> 
> - Andreas
> 
> _______________________________________________
> 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".