[FFmpeg-devel,1/4] lavu/float_dsp: add double-precision scalar product

The function pointer is appended to the structure for backward binary
compatibility. Fortunately, this is allocated by libavutil, not by the
user, so increasing the structure size is safe.
---
 doc/APIchanges        |  3 +++
 libavutil/float_dsp.c | 12 ++++++++++++
 libavutil/float_dsp.h | 14 ++++++++++++++
 libavutil/version.h   |  2 +-
 4 files changed, 30 insertions(+), 1 deletion(-)

Rémi Denis-Courmont:
> The function pointer is appended to the structure for backward binary
> compatibility. Fortunately, this is allocated by libavutil, not by the
> user, so increasing the structure size is safe.
> ---
>  doc/APIchanges        |  3 +++
>  libavutil/float_dsp.c | 12 ++++++++++++
>  libavutil/float_dsp.h | 14 ++++++++++++++
>  libavutil/version.h   |  2 +-
>  4 files changed, 30 insertions(+), 1 deletion(-)
> 
> diff --git a/doc/APIchanges b/doc/APIchanges
> index 60f056b863..50c51c664f 100644
> --- a/doc/APIchanges
> +++ b/doc/APIchanges
> @@ -2,6 +2,9 @@ The last version increases of all libraries were on 2024-03-07
>  
>  API changes, most recent first:
>  
> +2024-05-29 - xxxxxxxxxx - lavu 59.21.100 - float_dsp.h
> +  Add AVFloatDSPContext.scalarproduct_double.

float_dsp.h is not a public header (the allocator is avpriv:
avpriv_float_dsp_alloc), so there must not be an APIchanges entry for this.

> +
>  2024-05-23 - xxxxxxxxxx - lavu 59.20.100 - channel_layout.h
>    Add av_channel_layout_ambisonic_order().
>  
> diff --git a/libavutil/float_dsp.c b/libavutil/float_dsp.c
> index e9fb023466..1c5bb05636 100644
> --- a/libavutil/float_dsp.c
> +++ b/libavutil/float_dsp.c
> @@ -132,6 +132,17 @@ float avpriv_scalarproduct_float_c(const float *v1, const float *v2, int len)
>      return p;
>  }
>  
> +static double ff_scalarproduct_double_c(const double *v1, const double *v2,

Don't use an ff_ prefix for a static function.

> +                                        size_t len)
> +{
> +    double p = 0.0;
> +
> +    for (size_t i = 0; i < len; i++)
> +        p += v1[i] * v2[i];
> +
> +    return p;
> +}
> +
>  av_cold AVFloatDSPContext *avpriv_float_dsp_alloc(int bit_exact)
>  {
>      AVFloatDSPContext *fdsp = av_mallocz(sizeof(AVFloatDSPContext));
> @@ -149,6 +160,7 @@ av_cold AVFloatDSPContext *avpriv_float_dsp_alloc(int bit_exact)
>      fdsp->vector_fmul_reverse = vector_fmul_reverse_c;
>      fdsp->butterflies_float = butterflies_float_c;
>      fdsp->scalarproduct_float = avpriv_scalarproduct_float_c;
> +    fdsp->scalarproduct_double = ff_scalarproduct_double_c;
>  
>  #if ARCH_AARCH64
>      ff_float_dsp_init_aarch64(fdsp);
> diff --git a/libavutil/float_dsp.h b/libavutil/float_dsp.h
> index 342a8715c5..b6b5b0a3b3 100644
> --- a/libavutil/float_dsp.h
> +++ b/libavutil/float_dsp.h
> @@ -19,6 +19,8 @@
>  #ifndef AVUTIL_FLOAT_DSP_H
>  #define AVUTIL_FLOAT_DSP_H
>  
> +#include <stddef.h>
> +
>  typedef struct AVFloatDSPContext {
>      /**
>       * Calculate the entry wise product of two vectors of floats and store the result in
> @@ -187,6 +189,18 @@ typedef struct AVFloatDSPContext {
>       */
>      void (*vector_dmul)(double *dst, const double *src0, const double *src1,
>                          int len);
> +
> +    /**
> +     * Calculate the scalar product of two vectors of doubles.
> +     *
> +     * @param v1  first vector
> +     * @param v2  second vector

Are these supposed to obey additional alignment beyond that imposed by
double? (Does your RISC-V implementation require it?)

> +     * @param len length of vectors
> +     *
> +     * @return inner product of the vectors
> +     */
> +    double (*scalarproduct_double)(const double *v1, const double *v2,
> +                                   size_t len);
>  } AVFloatDSPContext;
>  
>  /**
> diff --git a/libavutil/version.h b/libavutil/version.h
> index 9c7146c228..9d08d56884 100644
> --- a/libavutil/version.h
> +++ b/libavutil/version.h
> @@ -79,7 +79,7 @@
>   */
>  
>  #define LIBAVUTIL_VERSION_MAJOR  59
> -#define LIBAVUTIL_VERSION_MINOR  20
> +#define LIBAVUTIL_VERSION_MINOR  21
>  #define LIBAVUTIL_VERSION_MICRO 100
>  
>  #define LIBAVUTIL_VERSION_INT   AV_VERSION_INT(LIBAVUTIL_VERSION_MAJOR, \

Le keskiviikkona 29. toukokuuta 2024, 18.44.13 EEST Andreas Rheinhardt a écrit 
:
> > +static double ff_scalarproduct_double_c(const double *v1,

> Don't use an ff_ prefix for a static function.

I can see over 300 such identifiers in the code base (many but not all inline), 
and I don't see why that would be a problem.

> > +
> > +    /**
> > +     * Calculate the scalar product of two vectors of doubles.
> > +     *
> > +     * @param v1  first vector
> > +     * @param v2  second vector
> 
> Are these supposed to obey additional alignment beyond that imposed by
> double?

The C and RISC-V implementations require the natural alignment of double, 64-
bit. If somebody wants to increase the required alignment(s), they are free to 
audit the call sites and update the comments accordingly, since this won't be 
an ABI break.

> (Does your RISC-V implementation require it?)

On 5/29/2024 12:51 PM, Rémi Denis-Courmont wrote:
> Le keskiviikkona 29. toukokuuta 2024, 18.44.13 EEST Andreas Rheinhardt a écrit
> :
>>> +static double ff_scalarproduct_double_c(const double *v1,
> 
>> Don't use an ff_ prefix for a static function.
> 
> I can see over 300 such identifiers in the code base (many but not all inline),
> and I don't see why that would be a problem.
> 
>>> +
>>> +    /**
>>> +     * Calculate the scalar product of two vectors of doubles.
>>> +     *
>>> +     * @param v1  first vector
>>> +     * @param v2  second vector
>>
>> Are these supposed to obey additional alignment beyond that imposed by
>> double?
> 
> The C and RISC-V implementations require the natural alignment of double, 64-
> bit. If somebody wants to increase the required alignment(s), they are free to
> audit the call sites and update the comments accordingly, since this won't be
> an ABI break.

Current users seem to all pass AVFrame audio buffers, so 32 byte 
alignment should be fine. And length should be a multiple of 16. So 
basically, same as all other double dsp functions in floatdsp.

On 2024-05-29 18:51 +0300, Rémi Denis-Courmont wrote:
> Le keskiviikkona 29. toukokuuta 2024, 18.44.13 EEST Andreas Rheinhardt a écrit
> :
> > > +static double ff_scalarproduct_double_c(const double *v1,
>
> > Don't use an ff_ prefix for a static function.
>
> I can see over 300 such identifiers in the code base (many but not all inline),
> and I don't see why that would be a problem.

I agree that it's not a problem regarding on the functional side,
OTOH regarding coding conventions we try to consistently follow it's
misleading as the ff_ prefix indicates a bigger scope of sharing.

Maybe I'm missing something, but it looks to me that more than half of
those over 300 instances are correct (the inline ones in the headers).

I think Andreas remark is correct and it would be better to not use ff_
prefix wrongly when adding new code.


Best regards,
  Alexander

[...]

Le sunnuntaina 2. kesäkuuta 2024, 13.04.05 EEST Alexander Strasser via ffmpeg-
devel a écrit :
> On 2024-05-29 18:51 +0300, Rémi Denis-Courmont wrote:
> > Le keskiviikkona 29. toukokuuta 2024, 18.44.13 EEST Andreas Rheinhardt a
> > écrit> 
> > > > +static double ff_scalarproduct_double_c(const double *v1,
> > > 
> > > Don't use an ff_ prefix for a static function.
> > 
> > I can see over 300 such identifiers in the code base (many but not all
> > inline), and I don't see why that would be a problem.
> 
> I agree that it's not a problem regarding on the functional side,
> OTOH regarding coding conventions we try to consistently follow it's
> misleading as the ff_ prefix indicates a bigger scope of sharing.

Anybody can see the 'static' qualifier literally in front to see the function 
is not in a bigger scope of sharing. And if you do somehow miss and try to use 
the function, you will get a linker error.

The only case where this *actually* matters is in debugging. And exactly then 
it is much better to use the ff_ prefix *because* all symbols, including local 
ones like this, end up sharing the namespace.

> I think Andreas remark is correct and it would be better to not use ff_
> prefix wrongly when adding new code.

IMO, it is worse.

Le sunnuntaina 2. kesäkuuta 2024, 13.30.50 EEST Rémi Denis-Courmont a écrit :
> > I think Andreas remark is correct and it would be better to not use ff_
> > prefix wrongly when adding new code.
> 
> IMO, it is worse.

P.S.: It is a moot point because a *different* version of the patch was merged.

On 2024-06-02 13:30 +0300, Rémi Denis-Courmont wrote:
> Le sunnuntaina 2. kesäkuuta 2024, 13.04.05 EEST Alexander Strasser via ffmpeg-
> devel a écrit :
> > On 2024-05-29 18:51 +0300, Rémi Denis-Courmont wrote:
> > > Le keskiviikkona 29. toukokuuta 2024, 18.44.13 EEST Andreas Rheinhardt a
> > > écrit>
> > > > > +static double ff_scalarproduct_double_c(const double *v1,
> > > >
> > > > Don't use an ff_ prefix for a static function.
> > >
> > > I can see over 300 such identifiers in the code base (many but not all
> > > inline), and I don't see why that would be a problem.
> >
> > I agree that it's not a problem regarding on the functional side,
> > OTOH regarding coding conventions we try to consistently follow it's
> > misleading as the ff_ prefix indicates a bigger scope of sharing.
>
> Anybody can see the 'static' qualifier literally in front to see the function
> is not in a bigger scope of sharing. And if you do somehow miss and try to use
> the function, you will get a linker error.
>
> The only case where this *actually* matters is in debugging. And exactly then
> it is much better to use the ff_ prefix *because* all symbols, including local
> ones like this, end up sharing the namespace.

But not at the call site?


> > I think Andreas remark is correct and it would be better to not use ff_
> > prefix wrongly when adding new code.
>
> IMO, it is worse.

I tend to disagree here as it makes the meaning of the ff_ prefix arbitrary.

Anyway if you want to challenge this convention we are using since many years
in this code base, I suggest to do it in a separate discussion thread.


  Alexander