Message ID | 20240529145955.32189-1-remi@remlab.net |
---|---|
State | New |
Headers | show |
Series | [FFmpeg-devel,1/4] lavu/float_dsp: add double-precision scalar product | expand |
Context | Check | Description |
---|---|---|
yinshiyou/make_loongarch64 | success | Make finished |
yinshiyou/make_fate_loongarch64 | success | Make fate finished |
andriy/make_x86 | success | Make finished |
andriy/make_fate_x86 | success | Make fate finished |
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
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. + 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, + 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 + * @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, \