[FFmpeg-devel] lsws/swscale.h: introduce sws_get_gaussian_vec

Use in place of sws_getGaussianVec.

The new function enable better log handling, and provide better naming
for the variance variable, now named standard_deviation to reflect the
meaning of the parameter.
---
 doc/APIchanges             |  3 +++
 libswscale/swscale.h       | 21 ++++++++++++++++++-
 libswscale/utils.c         | 41 +++++++++++++++++++++++++++++---------
 libswscale/version.h       |  2 +-
 libswscale/version_major.h |  4 ++++
 5 files changed, 60 insertions(+), 11 deletions(-)

Stefano Sabatini:
> Use in place of sws_getGaussianVec.
> 
> The new function enable better log handling, and provide better naming

Better log handling? Why?

> for the variance variable, now named standard_deviation to reflect the
> meaning of the parameter.
> ---
>  doc/APIchanges             |  3 +++
>  libswscale/swscale.h       | 21 ++++++++++++++++++-
>  libswscale/utils.c         | 41 +++++++++++++++++++++++++++++---------
>  libswscale/version.h       |  2 +-
>  libswscale/version_major.h |  4 ++++
>  5 files changed, 60 insertions(+), 11 deletions(-)
> 
> diff --git a/doc/APIchanges b/doc/APIchanges
> index ad1efe708d..bad2d61027 100644
> --- a/doc/APIchanges
> +++ b/doc/APIchanges
> @@ -2,6 +2,9 @@ The last version increases of all libraries were on 2023-02-09
>  
>  API changes, most recent first:
>  
> +2023-08-26 - xxxxxxxxxx - lsws 7.4.100 - swscale.h
> +  Introduce sws_get_gaussian_vec, use in place of sws_getGaussianVec.
> +
>  2023-08-18 - xxxxxxxxxx - lavu 58.17.100 - channel_layout.h
>    All AV_CHANNEL_LAYOUT_* macros are now compatible with C++ 17 and older.
>  
> diff --git a/libswscale/swscale.h b/libswscale/swscale.h
> index 9d4612aaf3..f002b5c7d2 100644
> --- a/libswscale/swscale.h
> +++ b/libswscale/swscale.h
> @@ -355,11 +355,30 @@ int sws_getColorspaceDetails(struct SwsContext *c, int **inv_table,
>   */
>  SwsVector *sws_allocVec(int length);
>  
> +#if FF_API_SWS_GET_GAUSSIAN_VEC
>  /**
> - * Return a normalized Gaussian curve used to filter stuff
> + * Return a normalized Gaussian curve used to filter stuff.
> + *
>   * quality = 3 is high quality, lower is lower quality.
>   */
>  SwsVector *sws_getGaussianVec(double variance, double quality);

Missing attribute_deprecated as well as the @deprecated doxygen thing
refering to its replacement.

> +#endif
> +
> +/**
> + * Compute and return a normalized Gaussian vector.
> + *
> + * @param vecp: pointer where the computed vector is put in case of
> + *        success
> + * @param standard_deviation the standard deviation used to generate
> + *        the Gaussian vector, must be a non-negative value
> + * @param quality the quality of the generated Gaussian vector, must
> + *        be a non-negative value. It affects the lenght of the generated
> + *        vector. A value equal to 3 corresponds to high quality.
> + *
> + * @return a negative error code on error, non negative otherwise
> + */
> +int sws_get_gaussian_vec(SwsVector **vecp,
> +                         double standard_deviation, double quality);
>  
>  /**
>   * Scale all the coefficients of a by the scalar value.
> diff --git a/libswscale/utils.c b/libswscale/utils.c
> index 8e74c6603e..96034af1e0 100644
> --- a/libswscale/utils.c
> +++ b/libswscale/utils.c
> @@ -2139,31 +2139,54 @@ SwsVector *sws_allocVec(int length)
>      return vec;
>  }
>  
> -SwsVector *sws_getGaussianVec(double variance, double quality)
> +int sws_get_gaussian_vec(SwsVector **vecp,
> +                         double standard_deviation, double quality)
>  {
> -    const int length = (int)(variance * quality + 0.5) | 1;
> +    const int length = (int)(standard_deviation * quality + 0.5) | 1;
>      int i;
>      double middle  = (length - 1) * 0.5;
>      SwsVector *vec;
> +    const double variance = standard_deviation * standard_deviation;
>  
> -    if(variance < 0 || quality < 0)
> -        return NULL;
> +    if (standard_deviation < 0 || quality < 0) {
> +        av_log(NULL, AV_LOG_ERROR,
> +               "Invalid negative standard deviation %f or quality %f provided as input to the sws_get_gaussian_vec function\n",
> +               standard_deviation, quality);
> +        return AVERROR(EINVAL);
> +    }
>  
>      vec = sws_allocVec(length);
> -
> -    if (!vec)
> -        return NULL;
> +    if (!vec) {
> +        av_log(NULL, AV_LOG_ERROR,
> +               "Could not allocate vector for the sws_get_gaussian_vec function\n");
> +        return AVERROR(ENOMEM);
> +    }
>  
>      for (i = 0; i < length; i++) {
>          double dist = i - middle;
> -        vec->coeff[i] = exp(-dist * dist / (2 * variance * variance)) /
> -                        sqrt(2 * variance * M_PI);
> +        vec->coeff[i] = exp(-dist * dist / (2 * variance)) /
> +                        sqrt(2 * standard_deviation * M_PI);
>      }
>  
>      sws_normalizeVec(vec, 1.0);
> +    *vecp = vec;
>  
> +    return 0;
> +}
> +
> +#if FF_API_SWS_GET_GAUSSIAN_VEC
> +SwsVector *sws_getGaussianVec(double variance, double quality)
> +{
> +    SwsVector *vec;
> +    int ret;
> +
> +    ret = sws_get_gaussian_vec(&vec, variance, quality);
> +    if (ret < 0) {
> +        return NULL;
> +    }
>      return vec;
>  }
> +#endif // FF_API_SWS_GET_GAUSSIAN_VEC
>  
>  /**
>   * Allocate and return a vector with length coefficients, all
> diff --git a/libswscale/version.h b/libswscale/version.h
> index 51eb013a29..12412bd538 100644
> --- a/libswscale/version.h
> +++ b/libswscale/version.h
> @@ -28,7 +28,7 @@
>  
>  #include "version_major.h"
>  
> -#define LIBSWSCALE_VERSION_MINOR   3
> +#define LIBSWSCALE_VERSION_MINOR   4
>  #define LIBSWSCALE_VERSION_MICRO 100
>  
>  #define LIBSWSCALE_VERSION_INT  AV_VERSION_INT(LIBSWSCALE_VERSION_MAJOR, \
> diff --git a/libswscale/version_major.h b/libswscale/version_major.h
> index 88577a2b42..aa0baef7c6 100644
> --- a/libswscale/version_major.h
> +++ b/libswscale/version_major.h
> @@ -32,4 +32,8 @@
>   * the public API and may change, break or disappear at any time.
>   */
>  
> +#ifndef FF_API_SWS_GET_GAUSSIAN_VEC
> +#define FF_API_SWS_GET_GAUSSIAN_VEC       (LIBSWSCALE_VERSION_MAJOR < 8)
> +#endif
> +
>  #endif /* SWSCALE_VERSION_MAJOR_H */

Quoting Stefano Sabatini (2023-08-26 14:23:28)
> Use in place of sws_getGaussianVec.
> 
> The new function enable better log handling, and provide better naming
> for the variance variable, now named standard_deviation to reflect the
> meaning of the parameter.

Logging to NULL does not seem like an improvement to me.

Renaming a function parameter does not require an API break.

On date Saturday 2023-08-26 17:15:36 +0200, Anton Khirnov wrote:
> Quoting Stefano Sabatini (2023-08-26 14:23:28)
> > Use in place of sws_getGaussianVec.
> > 
> > The new function enable better log handling, and provide better naming
> > for the variance variable, now named standard_deviation to reflect the
> > meaning of the parameter.
> 

> Logging to NULL does not seem like an improvement to me.

Adding the log_ctx.

> Renaming a function parameter does not require an API break.

The main point was improving the naming of the variable, but while at
it I'm also adding the logging context and providing a return code to
specify an error failure, and moving to snake_case convention which is
the one used by the new API additions.

On date Saturday 2023-08-26 17:15:27 +0200, Andreas Rheinhardt wrote:
> Stefano Sabatini:
> > Use in place of sws_getGaussianVec.
> > 
> > The new function enable better log handling, and provide better naming
> 
> Better log handling? Why?
> 
> > for the variance variable, now named standard_deviation to reflect the
> > meaning of the parameter.
> > ---
> >  doc/APIchanges             |  3 +++
> >  libswscale/swscale.h       | 21 ++++++++++++++++++-
> >  libswscale/utils.c         | 41 +++++++++++++++++++++++++++++---------
> >  libswscale/version.h       |  2 +-
> >  libswscale/version_major.h |  4 ++++
> >  5 files changed, 60 insertions(+), 11 deletions(-)
> > 
> > diff --git a/doc/APIchanges b/doc/APIchanges
> > index ad1efe708d..bad2d61027 100644
> > --- a/doc/APIchanges
> > +++ b/doc/APIchanges
> > @@ -2,6 +2,9 @@ The last version increases of all libraries were on 2023-02-09
> >  
> >  API changes, most recent first:
> >  
> > +2023-08-26 - xxxxxxxxxx - lsws 7.4.100 - swscale.h
> > +  Introduce sws_get_gaussian_vec, use in place of sws_getGaussianVec.
> > +
> >  2023-08-18 - xxxxxxxxxx - lavu 58.17.100 - channel_layout.h
> >    All AV_CHANNEL_LAYOUT_* macros are now compatible with C++ 17 and older.
> >  
> > diff --git a/libswscale/swscale.h b/libswscale/swscale.h
> > index 9d4612aaf3..f002b5c7d2 100644
> > --- a/libswscale/swscale.h
> > +++ b/libswscale/swscale.h
> > @@ -355,11 +355,30 @@ int sws_getColorspaceDetails(struct SwsContext *c, int **inv_table,
> >   */
> >  SwsVector *sws_allocVec(int length);
> >  
> > +#if FF_API_SWS_GET_GAUSSIAN_VEC
> >  /**
> > - * Return a normalized Gaussian curve used to filter stuff
> > + * Return a normalized Gaussian curve used to filter stuff.
> > + *
> >   * quality = 3 is high quality, lower is lower quality.
> >   */
> >  SwsVector *sws_getGaussianVec(double variance, double quality);
> 
> Missing attribute_deprecated as well as the @deprecated doxygen thing
> refering to its replacement.

Updated.

Stefano Sabatini:
> +int sws_get_gaussian_vec(SwsVector **vecp,
> +                         AVClass *log_ctx,
> +                         double standard_deviation, double quality);
>  

Seriously? A pointer to an AVClass as log_ctx? It is actually AVClass**
(the logcontext must have a pointer to an AVClass as its first member),
but we always use NULL.

Apart from that: I am not really convinced that the improvement is worth
the hassle.

> 
> +    if (standard_deviation < 0 || quality < 0) {
> +        av_log(NULL, AV_LOG_ERROR,
> +               "Invalid negative standard deviation %f or quality %f provided as input to the sws_get_gaussian_vec function\n",
> +               standard_deviation, quality);

Here you are not even using the logctx.

- Andreas

On date Thursday 2023-08-31 18:51:52 +0200, Andreas Rheinhardt wrote:
> Stefano Sabatini:
> > +int sws_get_gaussian_vec(SwsVector **vecp,
> > +                         AVClass *log_ctx,
> > +                         double standard_deviation, double quality);
> >  
> 
> Seriously? A pointer to an AVClass as log_ctx? It is actually AVClass**
> (the logcontext must have a pointer to an AVClass as its first member),
> but we always use NULL.

Sorry, sloppy editing on my side.

> Apart from that: I am not really convinced that the improvement is worth
> the hassle.

This is not a high-profile function (probably it's never used outside
lavfi) and provides an opportunity to move the API to the correct
direction.

Quoting Stefano Sabatini (2023-08-31 17:06:06)
> On date Saturday 2023-08-26 17:15:36 +0200, Anton Khirnov wrote:
> > Quoting Stefano Sabatini (2023-08-26 14:23:28)
> > > Use in place of sws_getGaussianVec.
> > > 
> > > The new function enable better log handling, and provide better naming
> > > for the variance variable, now named standard_deviation to reflect the
> > > meaning of the parameter.
> > 
> 
> > Logging to NULL does not seem like an improvement to me.
> 
> Adding the log_ctx.
> 
> > Renaming a function parameter does not require an API break.
> 
> The main point was improving the naming of the variable, but while at
> it I'm also adding the logging context and providing a return code to
> specify an error failure, and moving to snake_case convention which is
> the one used by the new API additions.

As I already said above - function parameter names in a prototype are
purely cosmetic and have no effect on anything besides doxygen. You can
change them at will and even remove them entirely without breaking API
or ABI.

The other reasons do not strike me as strong enough to warrant an API
break.

On Thu, Aug 31, 2023 at 07:16:20PM +0200, Stefano Sabatini wrote:
> On date Thursday 2023-08-31 18:51:52 +0200, Andreas Rheinhardt wrote:
> > Stefano Sabatini:
> > > +int sws_get_gaussian_vec(SwsVector **vecp,
> > > +                         AVClass *log_ctx,
> > > +                         double standard_deviation, double quality);
> > >  
> > 
> > Seriously? A pointer to an AVClass as log_ctx? It is actually AVClass**
> > (the logcontext must have a pointer to an AVClass as its first member),
> > but we always use NULL.
> 
> Sorry, sloppy editing on my side.
> 
> > Apart from that: I am not really convinced that the improvement is worth
> > the hassle.
> 
> This is not a high-profile function (probably it's never used outside
> lavfi) and provides an opportunity to move the API to the correct
> direction.

>  doc/APIchanges             |    3 +++
>  libswscale/swscale.h       |   27 ++++++++++++++++++++++++++-
>  libswscale/utils.c         |   42 +++++++++++++++++++++++++++++++++---------
>  libswscale/version.h       |    2 +-
>  libswscale/version_major.h |    4 ++++
>  5 files changed, 67 insertions(+), 11 deletions(-)
> b91b721cea2752b28a51aaeab2a464b2699dfb49  0001-lsws-swscale.h-introduce-sws_get_gaussian_vec.patch
> From 69c33f62e15de3d199d54187d38c0856418f0981 Mon Sep 17 00:00:00 2001
> From: Stefano Sabatini <stefasab@gmail.com>
> Date: Sat, 26 Aug 2023 14:20:35 +0200
> Subject: [PATCH 1/2] lsws/swscale.h: introduce sws_get_gaussian_vec
> 
> Use in place of sws_getGaussianVec.
> 
> The new function enable better error handling, and provide better naming
> for the variance variable, now named standard_deviation to reflect the
> meaning of the parameter.
> ---
>  doc/APIchanges             |  3 +++
>  libswscale/swscale.h       | 27 +++++++++++++++++++++++-
>  libswscale/utils.c         | 42 ++++++++++++++++++++++++++++++--------
>  libswscale/version.h       |  2 +-
>  libswscale/version_major.h |  4 ++++
>  5 files changed, 67 insertions(+), 11 deletions(-)
> 
> diff --git a/doc/APIchanges b/doc/APIchanges
> index ad1efe708d..bad2d61027 100644
> --- a/doc/APIchanges
> +++ b/doc/APIchanges
> @@ -2,6 +2,9 @@ The last version increases of all libraries were on 2023-02-09
>  
>  API changes, most recent first:
>  
> +2023-08-26 - xxxxxxxxxx - lsws 7.4.100 - swscale.h
> +  Introduce sws_get_gaussian_vec, use in place of sws_getGaussianVec.
> +
>  2023-08-18 - xxxxxxxxxx - lavu 58.17.100 - channel_layout.h
>    All AV_CHANNEL_LAYOUT_* macros are now compatible with C++ 17 and older.
>  
> diff --git a/libswscale/swscale.h b/libswscale/swscale.h
> index 9d4612aaf3..55f2fc4a48 100644
> --- a/libswscale/swscale.h
> +++ b/libswscale/swscale.h
> @@ -355,11 +355,36 @@ int sws_getColorspaceDetails(struct SwsContext *c, int **inv_table,
>   */
>  SwsVector *sws_allocVec(int length);
>  
> +#if FF_API_SWS_GET_GAUSSIAN_VEC
>  /**
> - * Return a normalized Gaussian curve used to filter stuff
> + * Return a normalized Gaussian curve used to filter stuff.
> + *
>   * quality = 3 is high quality, lower is lower quality.
> + * @deprecated use sws_get_gaussian_vector()
>   */
> +attribute_deprecated
>  SwsVector *sws_getGaussianVec(double variance, double quality);
> +#endif
> +
> +/**
> + * Compute and return a normalized Gaussian vector.
> + *
> + * @param vecp: pointer where the computed vector is put in case of
> + *        success
> + * @param standard_deviation the standard deviation used to generate
> + *        the Gaussian vector, must be a non-negative value
> + * @param quality the quality of the generated Gaussian vector, must
> + *        be a non-negative value. It affects the lenght of the generated
> + *        vector. A value equal to 3 corresponds to high quality.
> + * @param log_ctx a pointer to an arbitrary struct of which the first
> + *        field is a pointer to an AVClass struct (used for av_log)
> + *        used for logging, can be NULL
> + *
> + * @return a negative error code on error, non negative otherwise
> + */
> +int sws_get_gaussian_vec(SwsVector **vecp,
> +                         double standard_deviation, double quality,
> +                         void *log_ctx);

which of the two do you consider better?

First, here the central part we return is the vector

SwsVector *gaus_vec = sws_getGaussianVec(NULL, 1, 2);
SwsVector *temp_vec = sws_ConvolveVec(NULL, in_vec, gaus_vec);
sws_averageVec(temp_vec, temp_vec, in_vec);

av_free(gaus_vec);
return temp_vec; // Error checking here happens by temp_vec being NULL in all cases of error

vs.

Second, here the central part we return is the error code

SwsVector *gaus_vec = NULL;
SwsVector *temp_vec = NULL;
int err = sws_getGaussianVec(&gaus_vec, 1, 2);
if (err<0)
    goto fail;

err = sws_ConvolveVec(&temp_vec, in_vec, gaus_vec);
if (err<0)
    goto fail;

err = sws_averageVec(&temp_vec, temp_vec, in_vec);
if (err<0)
    goto fail;

*ret_argument = temp_vec
return 0;
fail:
    av_free(gaus_vec)
    av_free(temp_vec)
return ret;

thx

[...]

On date Friday 2023-09-01 17:50:56 +0200, Anton Khirnov wrote:
> Quoting Stefano Sabatini (2023-08-31 17:06:06)
> > On date Saturday 2023-08-26 17:15:36 +0200, Anton Khirnov wrote:
> > > Quoting Stefano Sabatini (2023-08-26 14:23:28)
> > > > Use in place of sws_getGaussianVec.
> > > > 
> > > > The new function enable better log handling, and provide better naming
> > > > for the variance variable, now named standard_deviation to reflect the
> > > > meaning of the parameter.
> > > 
> > 
> > > Logging to NULL does not seem like an improvement to me.
> > 
> > Adding the log_ctx.
> > 
> > > Renaming a function parameter does not require an API break.
> > 
> > The main point was improving the naming of the variable, but while at
> > it I'm also adding the logging context and providing a return code to
> > specify an error failure, and moving to snake_case convention which is
> > the one used by the new API additions.
> 
> As I already said above - function parameter names in a prototype are
> purely cosmetic and have no effect on anything besides doxygen. You can
> change them at will and even remove them entirely without breaking API
> or ABI.
> 

> The other reasons do not strike me as strong enough to warrant an API
> break.

I disagree on this: the function is probably only used internally and
by libavfilter, and the migration is trivial enough so should cause no
disruption anyway.

On date Friday 2023-09-01 18:54:40 +0200, Michael Niedermayer wrote:
> On Thu, Aug 31, 2023 at 07:16:20PM +0200, Stefano Sabatini wrote:
[...]
> > +/**
> > + * Compute and return a normalized Gaussian vector.
> > + *
> > + * @param vecp: pointer where the computed vector is put in case of
> > + *        success
> > + * @param standard_deviation the standard deviation used to generate
> > + *        the Gaussian vector, must be a non-negative value
> > + * @param quality the quality of the generated Gaussian vector, must
> > + *        be a non-negative value. It affects the lenght of the generated
> > + *        vector. A value equal to 3 corresponds to high quality.
> > + * @param log_ctx a pointer to an arbitrary struct of which the first
> > + *        field is a pointer to an AVClass struct (used for av_log)
> > + *        used for logging, can be NULL
> > + *
> > + * @return a negative error code on error, non negative otherwise
> > + */
> > +int sws_get_gaussian_vec(SwsVector **vecp,
> > +                         double standard_deviation, double quality,
> > +                         void *log_ctx);
> 
> which of the two do you consider better?
> 
> First, here the central part we return is the vector
> 
> SwsVector *gaus_vec = sws_getGaussianVec(NULL, 1, 2);
> SwsVector *temp_vec = sws_ConvolveVec(NULL, in_vec, gaus_vec);
> sws_averageVec(temp_vec, temp_vec, in_vec);
> 
> av_free(gaus_vec);
> return temp_vec; // Error checking here happens by temp_vec being NULL in all cases of error
> 
> vs.
> 
> Second, here the central part we return is the error code
> 
> SwsVector *gaus_vec = NULL;
> SwsVector *temp_vec = NULL;
> int err = sws_getGaussianVec(&gaus_vec, 1, 2);
> if (err<0)
>     goto fail;
> 
> err = sws_ConvolveVec(&temp_vec, in_vec, gaus_vec);
> if (err<0)
>     goto fail;
> 
> err = sws_averageVec(&temp_vec, temp_vec, in_vec);
> if (err<0)
>     goto fail;

The latter pattern enables differentiation between error codes (ENOMEM
or EINVAL) and provides feedback in the log message. With the former
you only know if it fails, but you don't know why (relevant in case
e.g. we make the parameter tunable by a filter and we don't want to
add additional validation and logging at the filter level).

On Fri, Sep 01, 2023 at 08:38:26PM +0200, Stefano Sabatini wrote:
> On date Friday 2023-09-01 18:54:40 +0200, Michael Niedermayer wrote:
> > On Thu, Aug 31, 2023 at 07:16:20PM +0200, Stefano Sabatini wrote:
> [...]
> > > +/**
> > > + * Compute and return a normalized Gaussian vector.
> > > + *
> > > + * @param vecp: pointer where the computed vector is put in case of
> > > + *        success
> > > + * @param standard_deviation the standard deviation used to generate
> > > + *        the Gaussian vector, must be a non-negative value
> > > + * @param quality the quality of the generated Gaussian vector, must
> > > + *        be a non-negative value. It affects the lenght of the generated
> > > + *        vector. A value equal to 3 corresponds to high quality.
> > > + * @param log_ctx a pointer to an arbitrary struct of which the first
> > > + *        field is a pointer to an AVClass struct (used for av_log)
> > > + *        used for logging, can be NULL
> > > + *
> > > + * @return a negative error code on error, non negative otherwise
> > > + */
> > > +int sws_get_gaussian_vec(SwsVector **vecp,
> > > +                         double standard_deviation, double quality,
> > > +                         void *log_ctx);
> > 
> > which of the two do you consider better?
> > 
> > First, here the central part we return is the vector
> > 
> > SwsVector *gaus_vec = sws_getGaussianVec(NULL, 1, 2);
> > SwsVector *temp_vec = sws_ConvolveVec(NULL, in_vec, gaus_vec);
> > sws_averageVec(temp_vec, temp_vec, in_vec);
> > 
> > av_free(gaus_vec);
> > return temp_vec; // Error checking here happens by temp_vec being NULL in all cases of error
> > 
> > vs.
> > 
> > Second, here the central part we return is the error code
> > 
> > SwsVector *gaus_vec = NULL;
> > SwsVector *temp_vec = NULL;
> > int err = sws_getGaussianVec(&gaus_vec, 1, 2);
> > if (err<0)
> >     goto fail;
> > 
> > err = sws_ConvolveVec(&temp_vec, in_vec, gaus_vec);
> > if (err<0)
> >     goto fail;
> > 
> > err = sws_averageVec(&temp_vec, temp_vec, in_vec);
> > if (err<0)
> >     goto fail;
> 
> The latter pattern enables differentiation between error codes (ENOMEM
> or EINVAL) and provides feedback in the log message. With the former
> you only know if it fails, but you don't know why (relevant in case
> e.g. we make the parameter tunable by a filter and we don't want to
> add additional validation and logging at the filter level).

can the API be designed so that optionally the user could choose to
only check the error code after several steps ?
(this would avoid the need for 1 check per call where the fine grained
 information is not needed)
I mean similar to the concept of NAN in floating point so that a failure
can be propagated and only at the end checked.

thx

[...]

On date Saturday 2023-09-02 22:07:53 +0200, Michael Niedermayer wrote:
> On Fri, Sep 01, 2023 at 08:38:26PM +0200, Stefano Sabatini wrote:
> > On date Friday 2023-09-01 18:54:40 +0200, Michael Niedermayer wrote:
> > > On Thu, Aug 31, 2023 at 07:16:20PM +0200, Stefano Sabatini wrote:
> > [...]
> > > > +/**
> > > > + * Compute and return a normalized Gaussian vector.
> > > > + *
> > > > + * @param vecp: pointer where the computed vector is put in case of
> > > > + *        success
> > > > + * @param standard_deviation the standard deviation used to generate
> > > > + *        the Gaussian vector, must be a non-negative value
> > > > + * @param quality the quality of the generated Gaussian vector, must
> > > > + *        be a non-negative value. It affects the lenght of the generated
> > > > + *        vector. A value equal to 3 corresponds to high quality.
> > > > + * @param log_ctx a pointer to an arbitrary struct of which the first
> > > > + *        field is a pointer to an AVClass struct (used for av_log)
> > > > + *        used for logging, can be NULL
> > > > + *
> > > > + * @return a negative error code on error, non negative otherwise
> > > > + */
> > > > +int sws_get_gaussian_vec(SwsVector **vecp,
> > > > +                         double standard_deviation, double quality,
> > > > +                         void *log_ctx);
> > > 
> > > which of the two do you consider better?
> > > 
> > > First, here the central part we return is the vector
> > > 
> > > SwsVector *gaus_vec = sws_getGaussianVec(NULL, 1, 2);
> > > SwsVector *temp_vec = sws_ConvolveVec(NULL, in_vec, gaus_vec);
> > > sws_averageVec(temp_vec, temp_vec, in_vec);
> > > 
> > > av_free(gaus_vec);
> > > return temp_vec; // Error checking here happens by temp_vec being NULL in all cases of error
> > > 
> > > vs.
> > > 
> > > Second, here the central part we return is the error code
> > > 
> > > SwsVector *gaus_vec = NULL;
> > > SwsVector *temp_vec = NULL;
> > > int err = sws_getGaussianVec(&gaus_vec, 1, 2);
> > > if (err<0)
> > >     goto fail;
> > > 
> > > err = sws_ConvolveVec(&temp_vec, in_vec, gaus_vec);
> > > if (err<0)
> > >     goto fail;
> > > 
> > > err = sws_averageVec(&temp_vec, temp_vec, in_vec);
> > > if (err<0)
> > >     goto fail;
> > 
> > The latter pattern enables differentiation between error codes (ENOMEM
> > or EINVAL) and provides feedback in the log message. With the former
> > you only know if it fails, but you don't know why (relevant in case
> > e.g. we make the parameter tunable by a filter and we don't want to
> > add additional validation and logging at the filter level).
> 

> can the API be designed so that optionally the user could choose to
> only check the error code after several steps ?
> (this would avoid the need for 1 check per call where the fine grained
>  information is not needed)
> I mean similar to the concept of NAN in floating point so that a failure
> can be propagated and only at the end checked.

Well, with the new approach you can do:

SwsVector *gaus_vec, *temp_vec, *avg_vec;

sws_get_gaussian_vec(&gaus_vec, 1, 2);
sws_get_convolution_vec(&temp_vec, in_vec, gaus_vec);
sws_get_average_vec(&avg_vec, temp_vec, in_vec);
 
av_free(gaus_vec);
av_free(temp_vec);
return avg_vec; // Error checking here happens by avg_vec being NULL in all cases of error

If you want to disable the log we could add a log_ctx_offset parameter.

On Sun, Sep 03, 2023 at 02:25:07AM +0200, Stefano Sabatini wrote:
> On date Saturday 2023-09-02 22:07:53 +0200, Michael Niedermayer wrote:
> > On Fri, Sep 01, 2023 at 08:38:26PM +0200, Stefano Sabatini wrote:
> > > On date Friday 2023-09-01 18:54:40 +0200, Michael Niedermayer wrote:
> > > > On Thu, Aug 31, 2023 at 07:16:20PM +0200, Stefano Sabatini wrote:
> > > [...]
> > > > > +/**
> > > > > + * Compute and return a normalized Gaussian vector.
> > > > > + *
> > > > > + * @param vecp: pointer where the computed vector is put in case of
> > > > > + *        success
> > > > > + * @param standard_deviation the standard deviation used to generate
> > > > > + *        the Gaussian vector, must be a non-negative value
> > > > > + * @param quality the quality of the generated Gaussian vector, must
> > > > > + *        be a non-negative value. It affects the lenght of the generated
> > > > > + *        vector. A value equal to 3 corresponds to high quality.
> > > > > + * @param log_ctx a pointer to an arbitrary struct of which the first
> > > > > + *        field is a pointer to an AVClass struct (used for av_log)
> > > > > + *        used for logging, can be NULL
> > > > > + *
> > > > > + * @return a negative error code on error, non negative otherwise
> > > > > + */
> > > > > +int sws_get_gaussian_vec(SwsVector **vecp,
> > > > > +                         double standard_deviation, double quality,
> > > > > +                         void *log_ctx);
> > > > 
> > > > which of the two do you consider better?
> > > > 
> > > > First, here the central part we return is the vector
> > > > 
> > > > SwsVector *gaus_vec = sws_getGaussianVec(NULL, 1, 2);
> > > > SwsVector *temp_vec = sws_ConvolveVec(NULL, in_vec, gaus_vec);
> > > > sws_averageVec(temp_vec, temp_vec, in_vec);
> > > > 
> > > > av_free(gaus_vec);
> > > > return temp_vec; // Error checking here happens by temp_vec being NULL in all cases of error
> > > > 
> > > > vs.
> > > > 
> > > > Second, here the central part we return is the error code
> > > > 
> > > > SwsVector *gaus_vec = NULL;
> > > > SwsVector *temp_vec = NULL;
> > > > int err = sws_getGaussianVec(&gaus_vec, 1, 2);
> > > > if (err<0)
> > > >     goto fail;
> > > > 
> > > > err = sws_ConvolveVec(&temp_vec, in_vec, gaus_vec);
> > > > if (err<0)
> > > >     goto fail;
> > > > 
> > > > err = sws_averageVec(&temp_vec, temp_vec, in_vec);
> > > > if (err<0)
> > > >     goto fail;
> > > 
> > > The latter pattern enables differentiation between error codes (ENOMEM
> > > or EINVAL) and provides feedback in the log message. With the former
> > > you only know if it fails, but you don't know why (relevant in case
> > > e.g. we make the parameter tunable by a filter and we don't want to
> > > add additional validation and logging at the filter level).
> > 
> 
> > can the API be designed so that optionally the user could choose to
> > only check the error code after several steps ?
> > (this would avoid the need for 1 check per call where the fine grained
> >  information is not needed)
> > I mean similar to the concept of NAN in floating point so that a failure
> > can be propagated and only at the end checked.
> 
> Well, with the new approach you can do:
> 
> SwsVector *gaus_vec, *temp_vec, *avg_vec;
> 
> sws_get_gaussian_vec(&gaus_vec, 1, 2);
> sws_get_convolution_vec(&temp_vec, in_vec, gaus_vec);
> sws_get_average_vec(&avg_vec, temp_vec, in_vec);
>  
> av_free(gaus_vec);
> av_free(temp_vec);
> return avg_vec; // Error checking here happens by avg_vec being NULL in all cases of error

ok

> 
> If you want to disable the log we could add a log_ctx_offset parameter.

as long as theres a pointer to a log context that should be enough.
A log context can modify the log level

passing a seperate and mandatory level offset per call would be a bit ugly

thx

[...]

Quoting Stefano Sabatini (2023-09-01 20:28:33)
> On date Friday 2023-09-01 17:50:56 +0200, Anton Khirnov wrote:
> > Quoting Stefano Sabatini (2023-08-31 17:06:06)
> > > On date Saturday 2023-08-26 17:15:36 +0200, Anton Khirnov wrote:
> > > > Quoting Stefano Sabatini (2023-08-26 14:23:28)
> > > > > Use in place of sws_getGaussianVec.
> > > > > 
> > > > > The new function enable better log handling, and provide better naming
> > > > > for the variance variable, now named standard_deviation to reflect the
> > > > > meaning of the parameter.
> > > > 
> > > 
> > > > Logging to NULL does not seem like an improvement to me.
> > > 
> > > Adding the log_ctx.
> > > 
> > > > Renaming a function parameter does not require an API break.
> > > 
> > > The main point was improving the naming of the variable, but while at
> > > it I'm also adding the logging context and providing a return code to
> > > specify an error failure, and moving to snake_case convention which is
> > > the one used by the new API additions.
> > 
> > As I already said above - function parameter names in a prototype are
> > purely cosmetic and have no effect on anything besides doxygen. You can
> > change them at will and even remove them entirely without breaking API
> > or ABI.
> > 
> 
> > The other reasons do not strike me as strong enough to warrant an API
> > break.
> 
> I disagree on this: the function is probably only used internally and
> by libavfilter, and the migration is trivial enough so should cause no
> disruption anyway.

The migration is not trivial for someone who is not familiar with the
code (such as a distro package maintainer), since the new function has a
different signature. I really do not think we should break APIs for
frivolous reasons, which includes cosmetic ones.

On date Tuesday 2023-09-05 13:19:00 +0200, Anton Khirnov wrote:
> Quoting Stefano Sabatini (2023-09-01 20:28:33)
> > On date Friday 2023-09-01 17:50:56 +0200, Anton Khirnov wrote:
> > > Quoting Stefano Sabatini (2023-08-31 17:06:06)
> > > > On date Saturday 2023-08-26 17:15:36 +0200, Anton Khirnov wrote:
> > > > > Quoting Stefano Sabatini (2023-08-26 14:23:28)
> > > > > > Use in place of sws_getGaussianVec.
> > > > > > 
> > > > > > The new function enable better log handling, and provide better naming
> > > > > > for the variance variable, now named standard_deviation to reflect the
> > > > > > meaning of the parameter.
> > > > > 
> > > > 
> > > > > Logging to NULL does not seem like an improvement to me.
> > > > 
> > > > Adding the log_ctx.
> > > > 
> > > > > Renaming a function parameter does not require an API break.
> > > > 
> > > > The main point was improving the naming of the variable, but while at
> > > > it I'm also adding the logging context and providing a return code to
> > > > specify an error failure, and moving to snake_case convention which is
> > > > the one used by the new API additions.
> > > 
> > > As I already said above - function parameter names in a prototype are
> > > purely cosmetic and have no effect on anything besides doxygen. You can
> > > change them at will and even remove them entirely without breaking API
> > > or ABI.
> > > 
> > 
> > > The other reasons do not strike me as strong enough to warrant an API
> > > break.
> > 

> > I disagree on this: the function is probably only used internally and
> > by libavfilter, and the migration is trivial enough so should cause no
> > disruption anyway.
> 

> The migration is not trivial for someone who is not familiar with the
> code (such as a distro package maintainer), since the new function has a
> different signature. I really do not think we should break APIs for
> frivolous reasons, which includes cosmetic ones.

Following this logic every API change should be considered not trivial
for someone not familiar with the code, and therefore should not be
committed. Also there is no evidence that external components are
using this function.

Besides the naming change, there are ergonomic and functional changes
making the behavior of the code more correct.

(Also probably it would be worth moving the naming of all the
remaining functions to snake_case to provide a consistent API - and
reduce the cognitive overload on the API user).

Quoting Stefano Sabatini (2023-09-06 00:59:44)
> > > > As I already said above - function parameter names in a prototype are
> > > > purely cosmetic and have no effect on anything besides doxygen. You can
> > > > change them at will and even remove them entirely without breaking API
> > > > or ABI.
> > > > 
> > > 
> > > > The other reasons do not strike me as strong enough to warrant an API
> > > > break.
> > > 
> 
> > > I disagree on this: the function is probably only used internally and
> > > by libavfilter, and the migration is trivial enough so should cause no
> > > disruption anyway.
> > 
> 
> > The migration is not trivial for someone who is not familiar with the
> > code (such as a distro package maintainer), since the new function has a
> > different signature. I really do not think we should break APIs for
> > frivolous reasons, which includes cosmetic ones.
> 
> Following this logic every API change should be considered not trivial
> for someone not familiar with the code,

A simple rename is a trivial API change. Almost everything else is not.

> and therefore should not be committed.

Yes, the baseline for every API change is that it is undesirable and you
must supply sufficiently strong arguments to overcome that.

> Also there is no evidence that external components are using this
> function.
>
> Besides the naming change, there are ergonomic and functional changes
> making the behavior of the code more correct.

I do not see the code being made more correct, but Michael observed in
this thread that it becomes longer and more convoluted.

I am not convinced that adding logging to this function is an
improvement. You have to pass an extra parameter to every call, making
the code longer and less readable. We do not need a dedicated error
message for every malloc.

On date Wednesday 2023-09-06 13:13:25 +0200, Anton Khirnov wrote:
> Quoting Stefano Sabatini (2023-09-06 00:59:44)
[...]
> A simple rename is a trivial API change. Almost everything else is not.
> 
> > and therefore should not be committed.
> 
> Yes, the baseline for every API change is that it is undesirable and you
> must supply sufficiently strong arguments to overcome that.
> 
> > Also there is no evidence that external components are using this
> > function.
> >

> > Besides the naming change, there are ergonomic and functional changes
> > making the behavior of the code more correct.
> 

> I do not see the code being made more correct, but Michael observed in
> this thread that it becomes longer and more convoluted.

It is a different construct, not necessarily more convoluted. Also
keep in mind this is needed in order to achive two goals:

1. return a proper error code since we can have two types of failures
here (memalloc or invalid argument)

2. log the invalid argument reason so that the failure reason is in
the log, without having to repeat the validation logic in the caller

> I am not convinced that adding logging to this function is an
> improvement. You have to pass an extra parameter to every call, making
> the code longer and less readable. We do not need a dedicated error
> message for every malloc.

See above, in this case we can have ENOMEM/EINVAL and it should be
possible to distinguish between the two.