PitchShiftEffect: add independent effect #4775

davidchocholaty · 2022-05-30T15:56:21Z

Adds a new independent pitch shift effect.
The pitch shift effect contains for now one knob for setting up
the pitch of a sound. The knob has a fixed range for one octave
up or down and works in linear continuous mode.

The known issue is, that the RubberBand library processing latency
needs to be taken into account for different pitch values.
Based on that the effect works with an amount of latency.

Implements: lp:1299035 "Add a Transpose / Pitch shift effect"
Reported by: RJ Skerry-Ryan

This commit adds a new independent pitch shift effect. The pitch shift effect contains one knob for setting up the pitch of a sound. The knob has a fixed range for one octave up or down. The knob works in linear continuous mode. The known issue is, that RubberBand library processing latency needs to be taken into account for different pitch values. Based on that the effect works with an amount of latency.

Swiftb0y

Thank you. Code looks very good already.

src/effects/backends/builtin/pitchshifteffect.cpp

src/effects/backends/builtin/pitchshifteffect.h

src/effects/backends/builtin/pitchshifteffect.cpp

Swiftb0y · 2022-05-30T17:31:43Z

Your header seems to mismatch the implementation

 Error: /home/runner/work/mixxx/mixxx/src/effects/backends/builtin/pitchshifteffect.cpp:5:23: error: function previously declared with an implicit exception specification redeclared with an explicit exception specification [-Werror,-Wimplicit-exception-spec-mismatch]
PitchShiftGroupState::~PitchShiftGroupState() noexcept {
                      ^
/home/runner/work/mixxx/mixxx/src/effects/backends/builtin/pitchshifteffect.h:29:13: note: previous declaration is here
    virtual ~PitchShiftGroupState();

Swiftb0y · 2022-05-30T18:13:33Z

After you have addressed my review comments, I suggest you work on getting the usual Rubberband kinks worked out (latency compensation) primarily.

davidchocholaty · 2022-05-30T20:18:17Z

Thank you for the tips and improvement proposals. I agree with mentioned fixes and then I will focus on solving the latency issue.

Swiftb0y · 2022-05-30T21:44:43Z

Be warned: I don't know how much we can do about the latency without large changes to the effects system. Please discuss your findings and challenges early.

uklotzde · 2022-05-30T21:48:03Z

src/effects/backends/builtin/pitchshifteffect.cpp

+    SampleUtil::free(m_retrieveBuffer[0]);
+    SampleUtil::free(m_retrieveBuffer[1]);
+
+    VERIFY_OR_DEBUG_ASSERT(m_pRubberBand) {


All this code is probably redundant.

I couldn't find the other part of the code, where it should be done. This code was based on how other built-in effects use SampleUtil::free in their destructors. For example, see:

mixxx/src/effects/backends/builtin/linkwitzriley8eqeffect.cpp

Line 53 in 5158e62

LinkwitzRiley8EQEffectGroupState::~LinkwitzRiley8EQEffectGroupState() {

or

mixxx/src/effects/backends/builtin/lvmixeqbase.h

Line 47 in 5158e62

~LVMixEQEffectGroupState() override {

This can be a normal debug assert.

uklotzde · 2022-05-30T21:49:16Z

src/effects/backends/builtin/pitchshifteffect.cpp

+
+// static
+QString PitchShiftEffect::getId() {
+    return "org.mixxx.effects.pitchshift";


This could become a constant.

And also a QLatin1String

I guess the result is expected to be a QString so using QLatin1String would not make sense here.

Mhmm I figured it was faster but looking at the Qt implemenation, you are right its not. We should change that in the future.

src/effects/backends/builtin/pitchshifteffect.h

daschuer · 2022-05-31T12:18:17Z

src/effects/backends/builtin/pitchshifteffect.cpp

+
+// static
+QString PitchShiftEffect::getId() {
+    return "org.mixxx.effects.pitchshift";


I think you mean a QStringLiteral

I used QString because all other built-in effects use it too.

daschuer · 2022-05-31T12:22:06Z

src/effects/backends/builtin/pitchshifteffect.cpp

+    pitch->setName(QObject::tr("Pitch"));
+    pitch->setShortName(QObject::tr("Pitch"));
+    pitch->setDescription(QObject::tr(
+            "The new pitch of a sound."));


I think it is "The pitch shift applied to the sound"

IMO we can tweak the exact wording later. Discussing this is bikeshedding.

That's true, that I planned to improve effect descriptions a little bit later. Anyway, the proposal by @daschuer sounds better, than my version. I will use it. Thank you.

daschuer · 2022-05-31T12:26:09Z

src/effects/backends/builtin/pitchshifteffect.cpp

+    pitch->setDescription(QObject::tr(
+            "The new pitch of a sound."));
+    pitch->setValueScaler(EffectManifestParameter::ValueScaler::Linear);
+    pitch->setUnitsHint(EffectManifestParameter::UnitsHint::Unknown);


You may also consider to extend unitsHint. I am afraid it is currently unused but we may consider to inform the user about the units.

I agree, a percentage or a ratio UnitsHint would make sense, but we can do that later.

src/effects/backends/builtin/pitchshifteffect.cpp

daschuer · 2022-05-31T12:59:20Z

src/effects/backends/builtin/pitchshifteffect.cpp

+            framesAvailable,
+            engineParameters.framesPerBuffer());
+    SINT receivedFrames = pState->m_pRubberBand->retrieve(
+            (float* const*)pState->m_retrieveBuffer,


Remove the cast here as well.

src/effects/backends/builtin/pitchshifteffect.h

daschuer · 2022-05-31T21:57:38Z

I have just played with it and "Nathan Evans - Wellerman" sounds great with the high pitched voice and the (unwanted) echo and the dry/wet in center position :-)

This echo needs to be compensated. Maybe you can actually expose this compensation delay as additional parameter for making funny things without extra CPU cycles.

The pitch knob is crackling at the center position which is a known issue. In case of the center position this is a known issue. I have tried to fix it here without success: #4030
Unfortunately I hear also crackling in other positions, not sure if we have solved that somehow in the mixer implementation.
You can best observe it with a 440 Hz sine wave than van be generated by Audacity.

Actually the crossfade soluton is a solution at the wrong end. It would be much better to have this solved inside the library.
Maybe we should discuss this with the upstream developers. (Of cause, it also depends to in which direction your project is heading)

daschuer · 2022-05-31T21:59:15Z

Another issue is that your new effect is not visible by default, like all other internal effects.

Swiftb0y · 2022-05-31T22:55:28Z

Another issue is that your new effect is not visible by default,

I think thats fine until this effect is finished. The problem is that I don't know how we can make it visible by default once we want that.

This echo needs to be compensated.

I agree, I pointed that out earlier, but I don't know how easy that is. Regardless, its still a major priority.

daschuer · 2022-06-01T13:51:25Z

We have a latency compensation in our bessel filter. It uses

mixxx/src/engine/filters/enginefilterdelay.h

Line 8 in f2d670e

class EngineFilterDelay : public EngineObjectConstIn {

The calculation of the delay is done here. The "issue" is that the delay buffer works only with full sample size delays:

mixxx/src/engine/filters/enginefilterbessel8.cpp

Line 34 in f2d670e

const double delayRatioTable[] = {

davidchocholaty · 2022-06-01T20:02:17Z

I have just played with it and "Nathan Evans - Wellerman" sounds great with the high pitched voice and the (unwanted) echo and the dry/wet in center position :-)

This echo needs to be compensated. Maybe you can actually expose this compensation delay as additional parameter for making funny things without extra CPU cycles.

The pitch knob is crackling at the center position which is a known issue. In case of the center position this is a known issue. I have tried to fix it here without success: #4030 Unfortunately I hear also crackling in other positions, not sure if we have solved that somehow in the mixer implementation. You can best observe it with a 440 Hz sine wave than van be generated by Audacity.

Actually the crossfade soluton is a solution at the wrong end. It would be much better to have this solved inside the library. Maybe we should discuss this with the upstream developers. (Of cause, it also depends to in which direction your project is heading)

Thank you. That's a cool idea with a compensation delay. Maybe I can start a new topic on Zulip for this issue. Do you agree?

davidchocholaty · 2022-06-01T20:26:29Z

Another issue is that your new effect is not visible by default, like all other internal effects.

My idea was the same as @Swiftb0y mentioned, to make it visible after the effect will be done. Anyway, I could do it now. I am not sure too if I know, how to make the effect visible by default.

davidchocholaty · 2022-06-01T20:33:32Z

We have a latency compensation in our bessel filter. It uses

mixxx/src/engine/filters/enginefilterdelay.h

Line 8 in f2d670e

class EngineFilterDelay : public EngineObjectConstIn {

The calculation of the delay is done here. The "issue" is that the delay buffer works only with full sample size delays:

mixxx/src/engine/filters/enginefilterbessel8.cpp

Line 34 in f2d670e

const double delayRatioTable[] = {

Thank you for the tips. I just let to stir up the conversation for the first pull request commit. I was fixing the code continuously, so I just will make the last changes for the following fixing commit and I will focus on the latency issue.

daschuer · 2022-06-01T20:45:46Z

My idea was the same as @Swiftb0y mentioned, to make it visible after the effect will be done.

That's reasonable.
However I am afraid we have no feature to handle new effects. After a fresh installation (not verified) all internal effects will be shown, from the next start this is used as user settings, so new effects are not shown, since they are not listed in theses settings. This is probably a topic of another PR.

Since this effect is already fun, it we can merge is any time when you think this PR is finished. And we can discuss the delay issue at Zulip.

This commit fixes destructor exception specification error and refactors code. The code changes are based on the discussion for the previous PitchShiftEffect commit. Major changes in this commit are: fix destructor explicit exception specification error, complex methods movement from the header file into the .cpp file for PitchShiftGroupState class, use lambda expression for the pitch value assignment, remove unnecessary casts, add the QStringLiteral to the getId method and the PitchShiftEffect the class was set as final.

src/effects/backends/builtin/pitchshifteffect.cpp

This commit changes m_pRubberBand stereo buffer size to the count of the frames per buffer for current engine parameters.

Swiftb0y

LGTM, anyone else willing to take another look before merging?

daschuer · 2022-06-03T04:48:28Z

src/effects/backends/builtin/pitchshifteffect.cpp

+        return;
+    }
+
+    m_pRubberBand->reset();


This is redundant, because it will be called anyway invisibility. No need to care abeut the whole thing of an unique_ptr.

Can you elaborate? This calls RubberBandStretcher::reset() not, unique_ptr::reset()... RubberBandStretcher::reset() will not be called implicitly, it still might be redundant though since we're in the dtor anyways.

Oh, yes I have actually misread the code as m_pRubberBand.reset() sorry.

But you assumption that it is redundant anyway is also correct. It all ends here
https://github.com/breakfastquay/rubberband/blob/bad529f81e8ae66bd4535a6af392efa38c7fc6b1/src/StretcherImpl.cpp#L222 with almost the same code in the destructor.

daschuer · 2022-06-03T04:51:33Z

src/effects/backends/builtin/pitchshifteffect.cpp

+    SampleUtil::free(m_retrieveBuffer[0]);
+    SampleUtil::free(m_retrieveBuffer[1]);
+
+    VERIFY_OR_DEBUG_ASSERT(m_pRubberBand) {


This can be a normal debug assert.

davidchocholaty · 2022-06-03T10:45:02Z

This can be a normal debug assert.

Okay. Anyway, if I remove the duplicated reset of the unique_ptr, IMO it isn't necessary to check it at all.

The commit removes duplicated RubberBandStretcher::reset() call from the PitchShiftGroupState destructor. The functionality of the reset method is done by RubberBandStretcher on its own.

daschuer · 2022-06-03T15:24:42Z

Thank you! @uklotzde Are your findings also solved?

Swiftb0y · 2022-06-09T19:43:58Z

I think we've waited long enough for @uklotzde. Since this is fast-moving GSoC Project and all of the review comments have been addressed, I propose to merge this as-is so we don't hold up any progress. Do you agree @daschuer?

uklotzde · 2022-06-09T20:14:13Z

I think we've waited long enough for @uklotzde. Since this is fast-moving GSoC Project and all of the review comments have been addressed, I propose to merge this as-is so we don't hold up any progress. Do you agree @daschuer?

The title of the PR says WIP? Is this considered a draft or not?

Swiftb0y · 2022-06-09T20:45:19Z

I think we've waited long enough for @uklotzde. Since this is fast-moving GSoC Project and all of the review comments have been addressed, I propose to merge this as-is so we don't hold up any progress. Do you agree @daschuer?

The title of the PR says WIP? Is this considered a draft or not?

ping @davidchocholaty
Is this intentional or did you forget about removing that? In the future, just open a draft PR

davidchocholaty · 2022-06-10T07:56:41Z

Oh, I'm sorry. At the time I created the PR I didn't expect, that the PR will be merged until all effect parts and features will be done. I will remove the [WIP] tag.

daschuer · 2022-06-10T08:26:48Z

Thank you. That's a nice first step.

What will you do next?

My approach with the Bessel EQ was this:

I have created a 440 Hz sine wave using Audacity, with gaps of silence. Than play it with half dry wet and record it via Mixxx. Than load the recording into Audacy. You should clearly see the latency. I Have noted the latency into a spread sheet. Repeat this with different settings to get a feeling what is the issue we deal with.
Than I but the data into a diagram and try to find experimental a curve that was created by a math formular with similar values.

You may also confirm that with different frequencies. Alternatively you may use a logarithmic sweep of 20 Hz to 22 kHz with gaps to see the whole spectrum in a single file.

The optimum solution would be to report the latency to the comon dry/wet Mixer.

An intermediate solution would be to put an extra dry/wet knob into the effect.

Not sure if this approach fits here as well. What do you think?

davidchocholaty · 2022-06-10T12:54:03Z

Thank you so much for merging my first PR for Mixxx. It's really motivating for me. Your research for the Bessel EQ really impressed me. Thank you for all the tips. I will beggin with the reseach for the Pitch shift effect latency in the same way. It seems like a good idea to sweep in range of 20Hz to 22 kHz. I will inform you and other developers about the results of the research on the Zulip chat. I wouldn't be afraid of the better optimal solution according to your proposals. IMO the effect fits better into the effect rack and will be more intuitive for the user.

github-actions bot added the build label May 30, 2022

Swiftb0y requested changes May 30, 2022

View reviewed changes

uklotzde reviewed May 30, 2022

View reviewed changes

uklotzde suggested changes May 30, 2022

View reviewed changes

daschuer reviewed May 31, 2022

View reviewed changes

davidchocholaty requested review from uklotzde and Swiftb0y June 2, 2022 08:23

Swiftb0y reviewed Jun 2, 2022

View reviewed changes

src/effects/backends/builtin/pitchshifteffect.cpp Outdated Show resolved Hide resolved

PitchShiftGroupState: change frames buffer size

c6fd671

This commit changes m_pRubberBand stereo buffer size to the count of the frames per buffer for current engine parameters.

davidchocholaty requested a review from Swiftb0y June 2, 2022 20:59

Swiftb0y approved these changes Jun 2, 2022

View reviewed changes

daschuer requested changes Jun 3, 2022

View reviewed changes

PitchShiftGroupState: remove duplicated reset

b8c1d4c

The commit removes duplicated RubberBandStretcher::reset() call from the PitchShiftGroupState destructor. The functionality of the reset method is done by RubberBandStretcher on its own.

davidchocholaty requested a review from daschuer June 3, 2022 13:42

daschuer approved these changes Jun 3, 2022

View reviewed changes

Swiftb0y mentioned this pull request Jun 7, 2022

RuntimeLoggingCategory: Fix compile error with Qt 6.3.0 #4788

Merged

uklotzde approved these changes Jun 9, 2022

View reviewed changes

davidchocholaty changed the title ~~[WIP] PitchShiftEffect: add independent effect~~ PitchShiftEffect: add independent effect Jun 10, 2022

daschuer merged commit da71f8d into mixxxdj:main Jun 10, 2022

mixxxbot mentioned this pull request Aug 22, 2022

Add a Transpose / Pitch shift effect #7389

Closed

PitchShiftEffect: add independent effect #4775

PitchShiftEffect: add independent effect #4775

Conversation

davidchocholaty commented May 30, 2022

Swiftb0y left a comment

Choose a reason for hiding this comment

Swiftb0y commented May 30, 2022

Swiftb0y commented May 30, 2022

davidchocholaty commented May 30, 2022

Swiftb0y commented May 30, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daschuer May 31, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daschuer commented May 31, 2022

daschuer commented May 31, 2022

Swiftb0y commented May 31, 2022

daschuer commented Jun 1, 2022

davidchocholaty commented Jun 1, 2022

davidchocholaty commented Jun 1, 2022

davidchocholaty commented Jun 1, 2022

daschuer commented Jun 1, 2022

Swiftb0y left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidchocholaty commented Jun 3, 2022

daschuer commented Jun 3, 2022

Swiftb0y commented Jun 9, 2022

uklotzde commented Jun 9, 2022

Swiftb0y commented Jun 9, 2022

davidchocholaty commented Jun 10, 2022

daschuer commented Jun 10, 2022

davidchocholaty commented Jun 10, 2022

daschuer May 31, 2022 •

edited

Loading