Merge pull request #13654 from Swiftb0y/review/daschauer/gh13611-metr…

…onome-effect-fix

refactor: use higher-level `std::span` based logic

src/effects/backends/builtin/metronomeeffect.cpp

@@ -1,46 +1,99 @@
 #include "metronomeeffect.h"
 
+#include <cmath>
+#include <cstddef>
+#include <limits>
+#include <optional>
+#include <span>
+
+#include "audio/types.h"
 #include "effects/backends/effectmanifest.h"
+#include "effects/backends/effectmanifestparameter.h"
 #include "engine/effects/engineeffectparameter.h"
+#include "engine/engine.h"
 #include "metronomeclick.h"
 #include "util/math.h"
 #include "util/sample.h"
+#include "util/types.h"
 
 namespace {
 
-void playMonoSamples(std::span<const CSAMPLE> monoSource, std::span<CSAMPLE> output) {
-    const auto outputBufferFrames = output.size() / mixxx::kEngineChannelOutputCount;
-    SINT framesPlayed = std::min(monoSource.size(), outputBufferFrames);
-    SampleUtil::addMonoToStereo(output.data(), monoSource.data(), framesPlayed);
+std::size_t playMonoSamplesWithGain(std::span<const CSAMPLE> monoSource,
+        std::span<CSAMPLE> output,
+        CSAMPLE_GAIN gain) {
+    const std::size_t outputBufferFrames = output.size() / mixxx::kEngineChannelOutputCount;
+    std::size_t framesPlayed = std::min(monoSource.size(), outputBufferFrames);
+    SampleUtil::addMonoToStereoWithGain(gain, output.data(), monoSource.data(), framesPlayed);
+    return framesPlayed;
+}
+
+template<class T>
+std::span<T> subspan_clamped(std::span<T> in, typename std::span<T>::size_type offset) {
+    // TODO (Swiftb0y): should we instead create a wrapper type that implements
+    // UB-free "clamped" operations?
+    return in.subspan(std::min(offset, in.size()));
+}
+
+constexpr std::size_t framesPerBeat(
+        mixxx::audio::SampleRate framesPerSecond, double beatsPerMinute) {
+    double framesPerMinute = framesPerSecond * 60;
+    return static_cast<std::size_t>(framesPerMinute / beatsPerMinute);
+}
+// returns where in the output buffer to start playing the next click.
+// If there the span is empty, no click should be played yet.
+std::span<CSAMPLE> syncedClickOutput(double beatFractionBufferEnd,
+        std::optional<GroupFeatureBeatLength> beatLengthAndScratch,
+        std::span<CSAMPLE> output) {
+    if (!beatLengthAndScratch.has_value() || beatLengthAndScratch->scratch_rate == 0.0) {
+        return {};
+    }
+    double beatLength = beatLengthAndScratch->frames / beatLengthAndScratch->scratch_rate;
+
+    const bool needsPreviousBeat = beatLength < 0;
+    double beatToBufferEndFrames = std::abs(beatLength) *
+            (needsPreviousBeat ? (1 - beatFractionBufferEnd)
+                               : beatFractionBufferEnd);
+    std::size_t beatToBufferEndSamples =
+            static_cast<std::size_t>(beatToBufferEndFrames) *
+            mixxx::kEngineChannelOutputCount;
+
+    if (beatToBufferEndSamples <= output.size()) {
+        return output.last(beatToBufferEndSamples);
+    }
+    return {};
 }
 
-double framesPerBeat(mixxx::audio::SampleRate sampleRate, double bpm) {
-    double framesPerMinute = sampleRate * 60;
-    return framesPerMinute / bpm;
+std::span<CSAMPLE> unsyncedClickOutput(mixxx::audio::SampleRate framesPerSecond,
+        std::size_t framesSinceLastClick,
+        double bpm,
+        std::span<CSAMPLE> output) {
+    std::size_t offset = framesSinceLastClick %
+            framesPerBeat(framesPerSecond, bpm);
+    return subspan_clamped(output, offset * mixxx::kEngineChannelOutputCount);
 }
 
 } // namespace
 
 // static
 QString MetronomeEffect::getId() {
-    return "org.mixxx.effects.metronome";
+    return QStringLiteral("org.mixxx.effects.metronome");
 }
 
 // static
 EffectManifestPointer MetronomeEffect::getManifest() {
-    EffectManifestPointer pManifest(new EffectManifest());
+    auto pManifest = EffectManifestPointer::create();
     pManifest->setId(getId());
     pManifest->setName(QObject::tr("Metronome"));
-    pManifest->setAuthor("The Mixxx Team");
-    pManifest->setVersion("1.0");
+    pManifest->setAuthor(QObject::tr("The Mixxx Team"));
+    pManifest->setVersion(QStringLiteral("1.0"));
     pManifest->setDescription(QObject::tr("Adds a metronome click sound to the stream"));
     pManifest->setEffectRampsFromDry(true);
 
     // Period
     // The maximum is at 128 + 1 allowing 128 as max value and
     // enabling us to pause time when the parameter is above
     EffectManifestParameterPointer period = pManifest->addParameter();
-    period->setId("bpm");
+    period->setId(QStringLiteral("bpm"));
     period->setName(QObject::tr("BPM"));
     period->setDescription(QObject::tr("Set the beats per minute value of the click sound"));
     period->setValueScaler(EffectManifestParameter::ValueScaler::Logarithmic);
@@ -49,21 +102,35 @@ EffectManifestPointer MetronomeEffect::getManifest() {
 
     // Period unit
     EffectManifestParameterPointer periodUnit = pManifest->addParameter();
-    periodUnit->setId("sync");
+    periodUnit->setId(QStringLiteral("sync"));
     periodUnit->setName(QObject::tr("Sync"));
     periodUnit->setDescription(QObject::tr(
             "Synchronizes the BPM with the track if it can be retrieved"));
     periodUnit->setValueScaler(EffectManifestParameter::ValueScaler::Toggle);
     periodUnit->setUnitsHint(EffectManifestParameter::UnitsHint::Unknown);
     periodUnit->setRange(0, 1, 1);
 
+    EffectManifestParameterPointer gain = pManifest->addParameter();
+    gain->setId(QStringLiteral("gain"));
+    gain->setName(QObject::tr("Gain"));
+    gain->setDescription(QObject::tr(
+            "Set the gain of metronome click sound"));
+    gain->setValueScaler(EffectManifestParameter::ValueScaler::Linear);
+    gain->setUnitsHint(EffectManifestParameter::UnitsHint::Decibel);
+    gain->setDefaultLinkType(EffectManifestParameter::LinkType::Linked);
+    gain->setRange(-24.0, 0.0, 3.0); // decibel
+    // 0db on the range above, assumes scale is linear default=(max-min)x+min (solve for x)
+    // TODO: move this generally to ControlPotmeterBehavior?
+    gain->setNeutralPointOnScale(24.0 / 27.0);
+
     return pManifest;
 }
 
 void MetronomeEffect::loadEngineEffectParameters(
         const QMap<QString, EngineEffectParameterPointer>& parameters) {
-    m_pBpmParameter = parameters.value("bpm");
-    m_pSyncParameter = parameters.value("sync");
+    m_pBpmParameter = parameters.value(QStringLiteral("bpm"));
+    m_pSyncParameter = parameters.value(QStringLiteral("sync"));
+    m_pGainParameter = parameters.value(QStringLiteral("gain"));
 }
 
 void MetronomeEffect::processChannel(
@@ -83,73 +150,42 @@ void MetronomeEffect::processChannel(
     MetronomeGroupState* gs = pGroupState;
 
     const std::span<const CSAMPLE> click = clickForSampleRate(engineParameters.sampleRate());
-    SINT clickSize = click.size();
 
     if (pOutput != pInput) {
         SampleUtil::copy(pOutput, pInput, engineParameters.samplesPerBuffer());
     }
 
     const bool shouldSync = m_pSyncParameter->toBool();
+    const bool hasBeatInfo = groupFeatures.beat_length.has_value() &&
+            groupFeatures.beat_fraction_buffer_end.has_value();
 
     if (enableState == EffectEnableState::Enabling) {
-        if (shouldSync && groupFeatures.beat_fraction_buffer_end.has_value()) {
+        if (shouldSync && hasBeatInfo) {
             // Skip first click and sync phase
-            gs->m_framesSinceClickStart = clickSize;
-        } else {
-            // click right away after enabling
-            gs->m_framesSinceClickStart = 0;
-        }
-    }
-
-    if (gs->m_framesSinceClickStart < clickSize) {
-        // In click region, write remaining click frames.
-        playMonoSamples(click.subspan(gs->m_framesSinceClickStart), output);
-    }
-
-    double bufferEnd = gs->m_framesSinceClickStart + engineParameters.framesPerBuffer();
-
-    double nextClickStart = bufferEnd; // default to "no new click";
-    if (shouldSync && groupFeatures.beat_fraction_buffer_end.has_value()) {
-        // Sync enabled and have a track with beats
-        if (groupFeatures.beat_length.has_value() &&
-                groupFeatures.beat_length->scratch_rate != 0.0) {
-            double beatLength = groupFeatures.beat_length->frames /
-                    groupFeatures.beat_length->scratch_rate;
-            double beatToBufferEnd;
-            if (beatLength > 0) {
-                beatToBufferEnd =
-                        beatLength *
-                        *groupFeatures.beat_fraction_buffer_end;
-            } else {
-                beatToBufferEnd =
-                        beatLength * -1 *
-                        (1 - *groupFeatures.beat_fraction_buffer_end);
-            }
-
-            if (bufferEnd > beatToBufferEnd) {
-                // We have a new beat before the current buffer ends
-                nextClickStart = bufferEnd - beatToBufferEnd;
-            }
+            gs->framesSinceLastClick = click.size();
         } else {
-            // no transport, continue until the current click has been fully played
-            if (gs->m_framesSinceClickStart < clickSize) {
-                gs->m_framesSinceClickStart += engineParameters.framesPerBuffer();
-            }
-            return;
+            gs->framesSinceLastClick = 0;
         }
-    } else {
-        nextClickStart = framesPerBeat(engineParameters.sampleRate(), m_pBpmParameter->value());
     }
 
-    if (bufferEnd > nextClickStart) {
-        // We need to start a new click
-        SINT outputOffset = static_cast<SINT>(nextClickStart) - gs->m_framesSinceClickStart;
-        if (outputOffset > 0 && outputOffset < engineParameters.framesPerBuffer()) {
-            playMonoSamples(click, output.subspan(outputOffset * 2));
-        }
-        // Due to seeking, we may have missed the start position of the click.
-        // We pretend that it has been played to stay in phase
-        gs->m_framesSinceClickStart = -outputOffset;
+    const CSAMPLE_GAIN gain = db2ratio(static_cast<float>(m_pGainParameter->value()));
+
+    playMonoSamplesWithGain(subspan_clamped(click, gs->framesSinceLastClick), output, gain);
+    gs->framesSinceLastClick += engineParameters.framesPerBuffer();
+
+    std::span<CSAMPLE> outputBufferOffset = shouldSync && hasBeatInfo
+            ? syncedClickOutput(*groupFeatures.beat_fraction_buffer_end,
+                      groupFeatures.beat_length,
+                      output)
+            : unsyncedClickOutput(
+                      engineParameters
+                              .sampleRate(), // engineParameters::sampleRate()
+                                             // in reality returns the frameRate
+                      gs->framesSinceLastClick,
+                      m_pBpmParameter->value(),
+                      output);
+
+    if (!outputBufferOffset.empty()) {
+        gs->framesSinceLastClick = playMonoSamplesWithGain(click, outputBufferOffset, gain);
     }
-    gs->m_framesSinceClickStart += engineParameters.framesPerBuffer();
 }

src/effects/backends/builtin/metronomeeffect.h

@@ -9,12 +9,10 @@
 class MetronomeGroupState final : public EffectState {
   public:
     MetronomeGroupState(const mixxx::EngineParameters& engineParameters)
-            : EffectState(engineParameters),
-              m_framesSinceClickStart(0) {
-    }
+            : EffectState(engineParameters) {};
     ~MetronomeGroupState() override = default;
 
-    SINT m_framesSinceClickStart;
+    std::size_t framesSinceLastClick = 0;
 };
 
 class MetronomeEffect : public EffectProcessorImpl<MetronomeGroupState> {
@@ -39,6 +37,7 @@ class MetronomeEffect : public EffectProcessorImpl<MetronomeGroupState> {
   private:
     EngineEffectParameterPointer m_pBpmParameter;
     EngineEffectParameterPointer m_pSyncParameter;
+    EngineEffectParameterPointer m_pGainParameter;
 
     DISALLOW_COPY_AND_ASSIGN(MetronomeEffect);
 };

src/util/sample.cpp

@@ -842,17 +842,31 @@ void SampleUtil::copyMonoToDualMono(CSAMPLE* M_RESTRICT pDest,
 }
 
 // static
-void SampleUtil::addMonoToStereo(CSAMPLE* M_RESTRICT pDest,
-        const CSAMPLE* M_RESTRICT pSrc, SINT numFrames) {
+void SampleUtil::addMonoToStereoWithGain(CSAMPLE_GAIN gain,
+        CSAMPLE* M_RESTRICT pDest,
+        const CSAMPLE* M_RESTRICT pSrc,
+        SINT numFrames) {
+    if (gain == 0.0) {
+        // no need to add silence
+        return;
+    }
     // forward loop
     // note: LOOP VECTORIZED.
     for (SINT i = 0; i < numFrames; ++i) {
-        const CSAMPLE s = pSrc[i];
+        const CSAMPLE s = pSrc[i] * gain;
         pDest[i * 2] += s;
         pDest[i * 2 + 1] += s;
     }
 }
 
+// static
+void SampleUtil::addMonoToStereo(CSAMPLE* M_RESTRICT pDest,
+        const CSAMPLE* M_RESTRICT pSrc,
+        SINT numFrames) {
+    // lets hope the compiler inlines here and optimizes the multiplication away
+    return addMonoToStereoWithGain(1, pDest, pSrc, numFrames);
+}
+
 // static
 void SampleUtil::stripMultiToStereo(
         CSAMPLE* pBuffer,

src/util/sample.h

@@ -347,6 +347,15 @@ class SampleUtil {
     static void copyMonoToDualMono(CSAMPLE* pDest, const CSAMPLE* pSrc,
             SINT numFrames);
 
+    // Scales, adds and doubles the mono samples in pSrc to dual mono samples
+    // to pDest
+    // (numFrames) samples will be read from pSrc
+    // (numFrames * 2) samples will be added to pDest
+    static void addMonoToStereoWithGain(CSAMPLE_GAIN gain,
+            CSAMPLE* pDest,
+            const CSAMPLE* pSrc,
+            SINT numFrames);
+
     // Adds and doubles the mono samples in pSrc to dual mono samples
     // to pDest.
     // (numFrames) samples will be read from pSrc