On Tue, Jun 18, 2024 at 8:56 AM Rémi Denis-Courmont <r...@remlab.net> wrote:
>
>
> Le 17 juin 2024 19:52:10 GMT+02:00, Paul B Mahol <one...@gmail.com> a
> écrit :
> >On Mon, Jun 17, 2024 at 4:52 PM Rémi Denis-Courmont <r...@remlab.net>
> wrote:
> >
> >>
> >>
> >> Le 17 juin 2024 13:18:11 GMT+02:00, Yigithan Yigit <
> >> yigithanyigitde...@gmail.com> a écrit :
> >> >---
> >> > libavfilter/af_volumedetect.c | 159 ++++++++++++++++++++++++++++------
> >> > 1 file changed, 133 insertions(+), 26 deletions(-)
> >> >
> >> >diff --git a/libavfilter/af_volumedetect.c
> b/libavfilter/af_volumedetect.c
> >> >index 327801a7f9..dbbcd037a5 100644
> >> >--- a/libavfilter/af_volumedetect.c
> >> >+++ b/libavfilter/af_volumedetect.c
> >> >@@ -20,27 +20,51 @@
> >> >
> >> > #include "libavutil/channel_layout.h"
> >> > #include "libavutil/avassert.h"
> >> >+#include "libavutil/mem.h"
> >> > #include "audio.h"
> >> > #include "avfilter.h"
> >> > #include "internal.h"
> >> >
> >> >+#define MAX_DB_FLT 1024
> >> > #define MAX_DB 91
> >> >+#define HISTOGRAM_SIZE 0x10000
> >> >+#define HISTOGRAM_SIZE_FLT (MAX_DB_FLT*2)
> >> >
> >> > typedef struct VolDetectContext {
> >> >- /**
> >> >- * Number of samples at each PCM value.
> >> >- * histogram[0x8000 + i] is the number of samples at value i.
> >> >- * The extra element is there for symmetry.
> >> >- */
> >> >- uint64_t histogram[0x10001];
> >> >+ uint64_t* histogram; ///< for integer number of samples at each
> PCM
> >> value, for float number of samples at each dB
> >> >+ uint64_t nb_samples; ///< number of samples
> >> >+ double sum2; ///< sum of the squares of the samples
> >> >+ double max; ///< maximum sample value
> >> >+ int is_float; ///< true if the input is in floating point
> >> > } VolDetectContext;
> >> >
> >> >-static inline double logdb(uint64_t v)
> >> >+static inline double logdb(double v, enum AVSampleFormat sample_fmt)
> >> > {
> >> >- double d = v / (double)(0x8000 * 0x8000);
> >> >- if (!v)
> >> >- return MAX_DB;
> >> >- return -log10(d) * 10;
> >> >+ if (sample_fmt == AV_SAMPLE_FMT_FLT) {
> >> >+ if (!v)
> >> >+ return MAX_DB_FLT;
> >> >+ return -log10(v) * 10;
> >> >+ } else {
> >> >+ double d = v / (double)(0x8000 * 0x8000);
> >> >+ if (!v)
> >> >+ return MAX_DB;
> >> >+ return -log10(d) * 10;
> >> >+ }
> >> >+}
> >> >+
> >> >+static void update_float_stats(VolDetectContext *vd, float
> *audio_data)
> >> >+{
> >> >+ double sample;
> >> >+ int idx;
> >> >+ if(!isnormal(*audio_data))
> >> >+ return;
> >>
> >> Do we really need to classify floats here? That's probably going to hurt
> >> perfs badly, and makes an otherwise very vectorisable function not so
> >> easily vectored.
> >>
> >
> >This is fast, it should translate to checking few bits of memory.
>
> Sure but the branch is what irks me here, not the classification per se.
> And I don't get why it's needed here, where most of the code base seems to
> assume that floats are always numeric. It's also not clear why subnormals
> are disallowed here.
>
HUGE floats get out of range easily, there is probably nicer way to add
them to some kind of "non-uniform non-linear" histogram.
>
> IMO all that needs justification in the commit message which I find
> lacking. Or if it's unjustified then it shouldn't be there.
>
> >> >+ sample = fabsf(*audio_data);
> >> >+ if (sample > vd->max)
> >> >+ vd->max = sample;
> >> >+ vd->sum2 += sample * sample;
> >> >+ idx = lrintf(floorf(logdb(sample * sample, AV_SAMPLE_FMT_FLT))) +
> >> MAX_DB_FLT;
> >>
> >> You're recomputing the same value again, and you seem to be rounding
> twice
> >> in a row?
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
>
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
