PitchShiftEffect: extend effect options #4901

davidchocholaty · 2022-08-19T19:53:53Z

This PR extends the settings options of the Pitch shift effect. The implemented options are Range Mode, Semitones Mode and Wide Range option.

The Range Mode changes the range of the Pitch knob. The neutral mode (knob in the default position) keeps the range from the lowest pitch to the highest pitch. The positive mode (knob in the maximum position) keeps the range from the default pitch to the highest pitch. The negative mode (knob in the minimum position) keeps the range from the default pitch to the lowest pitch. Every mode works correctly with set the Wide Range option too.

The Wide Range option doubles the default Pitch knob scale. So, when the default scale of the pitch knob is from -1 octave to +1 octave, with set the option on the range is from -2 octaves to +2 octaves.

Lastly, the semitones mode changes the Pitch knob scale from the continuous mode to the semitones mode which works in the chromatic scale. By default, this option is on.

Another option, that will be implemented, will be the Formant option which works with the same-named option of the RubberBand library.

TODO:

Implementation of the Formant option.

This commit adds the Wide Range option for the Pitch shift effect. This option doubles the scale of the pitch knob, which means, that with the toggle on, the knob range is not in -1 octave and +1 octave, but in doubled range, so in range -2 octaves and +2 octaves. Based on the changes, the knob now works for both situations in the exponential scale of the power of 2.

This commit adds the Semitones option for the Pitch shift effect. This option switches the pitch scale into the semitones mode in a musical approach known. So, turning the Pitch knob is moved along the chromatic scale.

This commit adds the function for calculating the signum function. This function returns the sign of the value. For example, for integer values, it means 1 for positive values, 0 for zero values and -1 for negative values.

This commit adds the Range option for the Pitch shift effect. This option changes the range of the pitch knob. There are three possible options based on the knob position. The neutral mode (knob in the default position) keeps the range from the lowest pitch to the highest pitch. The positive mode (knob in the maximum position) keeps the range from the default pitch to the highest pitch. The negative mode (knob in the minimum position) keeps the range from the default pitch to the lowest pitch. Every mode works correctly with set the Wide Range option too.

This commit adds the Formant Preserving option for the Pitch shift effect. This option changes the settings of formant processing using the RubberBand library. The default option doesn't use any special formant processing. When the option is turned on, the handling of the formant shape is activated and the spectral envelope of the unshifted signal is preserved.

This commit removes the forgotten TODO for the Range Mode option description.

src/util/math.h

src/effects/backends/builtin/pitchshifteffect.cpp

This commit cleans the sgn function which implements the sign function as widely known. The static assert for checking, if a type is an arithmetic, is replaced using the required keyword for the same situation. Newly introduced is checking if the type is unsigned. If yes, the calculation can be simplified. Implicit casts are replaced with explicit casts.

This commit replaces the Semitones Mode toggle description with a more intuitive version.

This commit replaces the Formant toggle description with a more intuitive version.

This commit removes the Range Mode option and Wide Range option for the Pitch shift effect. These options should be replaced with only one option which will handle the Range of the Pitch knob.

This commit adds the Range knob for the Pitch shift effect. The Range option changes the range of the Pitch knob. The Range knob works linearly and the Pitch knob range is from 0 to 2 octaves. The default Range knob position is in the middle, which means that the range of the Pitch knob is from -1 octave to +1 octave.

daschuer · 2022-08-22T12:37:29Z

src/effects/backends/builtin/pitchshifteffect.cpp

+    semitonesMode->setName(QObject::tr("Semitones"));
+    semitonesMode->setShortName(QObject::tr("Semitones"));
+    semitonesMode->setDescription(QObject::tr(
+            "Change pitch in semitone-steps instead of smoothly."));


Suggested change

"Change pitch in semitone-steps instead of smoothly."));

"Change pitch in semitone-steps instead of continuously."));

Sounds more natural for me, not sure though ...
You could have even a smooth change to full semitone steps, which is a different feature.

I purposefully suggested "smooth" instead of "continuous" because these texts should be as easy to understand as possible. IMO knowing the word "continuous" requires a higher level of english skills than "smooth". Even though "continuous" would probably be the better antonym to "semitone-steps".

Sounds more natural for me, not sure though ...
You could have even a smooth change to full semitone steps, which is a different feature.

I agree, based on some grammar upgrades, the "Change the pitch in semitone steps instead of continuously." version looks good to me.

@Swiftb0y sorry, based my browser did not update your answer, I see it until now. IMHO the "continuously" looks more accurately in some way to me. To go a little bit deeper in the English point of view, both words belong to the B2 level, but it's true that the "continuously" word is rather for more technical English users. Of course, I am open-minded to both versions.

Since this is a translated text, there is no much need to be understandable for non English speaker. As long it is correct English it works for me.

It just jumps to my eyes initially because It look unusual to me. But I can*t really judge. Maybe a native speaker can jump in, @ywwg? This is not a blocking issue, though.

daschuer · 2022-08-22T13:12:16Z

src/effects/backends/builtin/pitchshifteffect.cpp

+    formant->setName(QObject::tr("Formant"));
+    formant->setShortName(QObject::tr("Formant"));
+    formant->setDescription(QObject::tr(
+            "Preserve formants."));


Suggested change

"Preserve formants."));

"Preserve formants, the resonance frequencies of human vocal tract and other instruments."));

I must admit I was not clear what exactly formats are when starting play with pitch shifting.
Maybe this one liner is enough to give at least a hint, for others like me.

Or we may pick a sentence that is more catchy but less exact.

"Preserve formants to compensate chipmunk or growling voices."

Suggested change

"Preserve formants."));

"Preserve the resonant frequencies of human vocal tract and other instruments (formants)"));

Thats better imo. Easy explanation upfront and then the technical term afterwards for those that are familiar with it.

Or we may pick a sentence that is more catchy but less exact.
"Preserve formants to compensate chipmunk or growling voices."

Looks cool 👍, anyway, I would prefer the proposed version due to it mentions the best usage (vocal tracks and instruments) too. Or, the easier way to understand can be added in brackets:

"Preserve formants, the resonance frequencies of the human vocal tract and other instruments (hint: "compensates chipmunk or growling voices")."

What do you think about that? I agree, that the phrase "resonance frequencies" does not have to be quite helpful still.

Pick what works for you, or an entire different string. As long we explain the user a bit more what this is about it should be fine.

daschuer · 2022-08-22T13:17:49Z

src/effects/backends/builtin/pitchshifteffect.cpp

+
+    EffectManifestParameterPointer formant = pManifest->addParameter();
+    formant->setId("formant");
+    formant->setName(QObject::tr("Formant"));


It looks like RB "engine fine" actually supports to shift the formants. If we consider this, the name "Formant" is better used for the formant rotary knob. This one is "Formant Preserving" in case we use the LV2 variant. Is the length still OK?

Yeah, in the R3 version the formant can be shifted using a knob. With introducing the R3 version in the Pitch shift effect, this option of course could be added and if it would be, the identical toggle name will be the problem too. On the other hand, the "Formant Preserving" name is too wide, see the print screen:

It looks like the maximum number of characters is about 10.

Then just name it formant and put the detailed explanation in the tooltip imo.

Ok, but at least the Id should not be "formant". Since the name can be changed at any time, but not the ID.

Okay, that looks good to me.

The commit updates the description of the semitones mode. To be more concrete, the word "smoothly" is replaced with "continuously" and some grammar updates are added too.

The commit extends the description of the Formant option with a more exact version and with an easy-to-understand hint.

The commit changes the formant parameter Id onto the formantPreserving name. Every dependent variable on this option is renamed in the same way.

daschuer · 2022-08-24T06:23:46Z

A conflict has developed ...

Swiftb0y · 2022-08-24T11:53:31Z

My fault. fixed and pushed to this branch. @davidchocholaty, just git pull to get your local branch up-to-date again.

src/effects/backends/builtin/pitchshifteffect.cpp

davidchocholaty · 2022-08-24T19:51:36Z

My fault. fixed and pushed to this branch. @davidchocholaty, just git pull to get your local branch up-to-date again.

👍 No problem.

daschuer · 2022-08-25T06:14:33Z

There is an issue in general if when an effect changes its parameters. They are not visible because of the old content in the /home/sperry/.mixxx/effects/defaults folder. This is a regression from the effect refactoring we suffer here now. Unfortunately we need to solve it else no one will see the new parameters.

daschuer · 2022-08-25T06:19:11Z

After deleting the related xml file I can see the new parameters, but Mixxx is now segfaulting:

Thread 66 "mixxx" received signal SIGSEGV, Segmentation fault.
[Switching to Thread 0x7fff08ff9700 (LWP 85411)]
PitchShiftEffect::processChannel (this=0x7fff98264010, pState=0x55555c283a70, 
    pInput=0x7fffbf762010, pOutput=0x7fff996b9010, engineParameters=..., 
    enableState=EffectEnableState::Enabling, groupFeatures=...)
    at ../../src/effects/backends/builtin/pitchshifteffect.cpp:126
126	    if (m_currentFormant != m_pFormantPreservingParameter->toBool()) {
(gdb) bt
#0  PitchShiftEffect::processChannel(PitchShiftGroupState*, float const*, float*, mixxx::EngineParameters const&, EffectEnableState, GroupFeatureState const&)
    (this=0x7fff98264010, pState=0x55555c283a70, pInput=0x7fffbf762010, pOutput=0x7fff996b9010, engineParameters=..., enableState=EffectEnableState::Enabling, groupFeatures=...)
    at ../../src/effects/backends/builtin/pitchshifteffect.cpp:126
#1  0x000055555594f0d3 in EngineEffect::process(ChannelHandle const&, ChannelHandle const&, float const*, float*, unsigned int, unsigned int, EffectEnableState, GroupFeatureState const&)
    (this=this@entry=0x55555c04a550, inputHandle=..., outputHandle=..., pInput=pInput@entry=0x7fffbf762010, pOutput=pOutput@entry=0x7fff996b9010, numSamples=numSamples@entry=2048, sampleRate=48000, chainEnableState=<optimized out>, groupFeatures=...) at /usr/include/c++/9/bits/unique_ptr.h:360
#2  0x0000555555953939 in EngineEffectChain::process(ChannelHandle const&, ChannelHandle const&, float*, float*, unsigned int, unsigned int, GroupFeatureState const&)
    (this=0x7fff99756010, inputHandle=..., outputHandle=..., pIn=pIn@entry=0x7fffbf762010, pOut=pOut@entry=0x7fffbf762010, numSamples=2048, sampleRate=48000, groupFeatures=...) at ../../src/engine/effects/engineeffectchain.cpp:280
#3  0x000055555595776d in EngineEffectsManager::processInner(SignalProcessingStage, ChannelHandle const&, ChannelHandle const&, float*, float*, unsigned int, unsigned int, GroupFeatureState const&, float, float)
    (this=<optimized out>, stage=<optimized out>, inputHandle=..., outputHandle=--Type <RET> for more, q to quit, c to continue without paging--

This commit renames the kSemitones constant into the kSemitonesPerOctave name because of its more appropriate meaning.

Swiftb0y · 2022-08-25T14:32:04Z

m_pFormantPreservingParameter seems to be nullptr for some reason. I may look into it if I find the time.

This commit replaces flip-flopping approach to the boolean value assignment with a cleaner value directly from the parameter. The init-statement is included in the if statement before the condition to reduce the variable scope (supported since C++17).

src/effects/backends/builtin/pitchshifteffect.cpp

daschuer

I think we need to update the version number:

pManifest->setVersion("2.0");

Add a version number to the default files and discard it when there is a version mismatch.

src/effects/backends/builtin/pitchshifteffect.cpp

The commit fixes the segmentation fault due to a bug in the name of the parameter value (the old id "formant" was used instead of the new one, which is "formatPreserving").

The commit updates the version of the effect manifest from version 1.0 onto version 2.0.

This commits creates literal constants in the anonymous namespace. The constants values are the same as the manifest parameter identifiers. The constants are created to avoid grammar bugs due to the value has to be used twice on the other code places. Due to the literal being used, the segmentation fault has occurred without previous warnings or errors if the entered identifier names for the specific parameter are not the same.

daschuer · 2022-08-29T06:53:26Z

Thank you. This looks good now. We only have the pending issue that no one of our alpha users will see the new options.
What is you plan? Does this PR fit to your future plan?
I think this can be merged, but we need at least an issue with the 2.4 milestone to fix it.
Do we have one?

davidchocholaty · 2022-08-29T09:10:12Z

What is you plan? Does this PR fit to your future plan?

The extension was implemented for the current Pitch shift effect implementation, anyway, it can be quite cool to test the wider range for the new implementation in #4898. IMO the clearest way is to just merge the 'upstream/main' including the extended options into the mentioned PR. As a second, TBH merged PR looks quite better for the GSoC blog than opened, but it doesn't really matter.

I think this can be merged, but we need at least an issue with the 2.4 milestone to fix it. Do we have one?

Yeah, of course. I have not found any issue related to this problem. If the PR will be merged, should I create the issue or do you want to do it on your own because you found the problem and maybe will better describe the situation?

Swiftb0y · 2022-08-29T12:07:10Z

Thank you. This looks good now. We only have the pending issue that no one of our alpha users will see the new options.

I may try to find a workaround during the beta. I'll open a tracking issue.

davidchocholaty · 2022-08-29T12:29:05Z

Thank you. This looks good now. We only have the pending issue that no one of our alpha users will see the new options.

I may try to find a workaround during the beta. I'll open a tracking issue.

OK, thank you very much.

daschuer · 2022-08-29T12:29:34Z

This one: #10834

Swiftb0y · 2022-08-29T12:31:42Z

Thanks

davidchocholaty · 2022-08-29T12:32:20Z

Thank you.

davidchocholaty added 6 commits August 19, 2022 10:30

PitchShiftEffect: add semitones mode option

ab124c3

This commit adds the Semitones option for the Pitch shift effect. This option switches the pitch scale into the semitones mode in a musical approach known. So, turning the Pitch knob is moved along the chromatic scale.

math: add sgn function

b913b7e

This commit adds the function for calculating the signum function. This function returns the sign of the value. For example, for integer values, it means 1 for positive values, 0 for zero values and -1 for negative values.

PitchShiftEffect: remove insignificant TODO

2c57997

This commit removes the forgotten TODO for the Range Mode option description.

Swiftb0y requested changes Aug 21, 2022

View reviewed changes

davidchocholaty added 4 commits August 21, 2022 20:17

PitchShiftEffect: semitones mode description

9ea8322

This commit replaces the Semitones Mode toggle description with a more intuitive version.

PitchShiftEffect: replace formant description

c49712a

This commit replaces the Formant toggle description with a more intuitive version.

PitchShiftEffect: remove range mode and wide range

ba5e5d4

This commit removes the Range Mode option and Wide Range option for the Pitch shift effect. These options should be replaced with only one option which will handle the Range of the Pitch knob.

davidchocholaty force-pushed the extend_pitch_shift_effect_options branch from af71fea to ba5e5d4 Compare August 22, 2022 11:06

daschuer requested changes Aug 22, 2022

View reviewed changes

davidchocholaty added 3 commits August 22, 2022 18:11

PitchShiftEffect: update semitones description

c60407c

The commit updates the description of the semitones mode. To be more concrete, the word "smoothly" is replaced with "continuously" and some grammar updates are added too.

PitchShiftEffect: extend formant description

0d9bc0c

The commit extends the description of the Formant option with a more exact version and with an easy-to-understand hint.

PitchShiftEffect: formant into formant preserving

5901cfd

The commit changes the formant parameter Id onto the formantPreserving name. Every dependent variable on this option is renamed in the same way.

Swiftb0y mentioned this pull request Aug 23, 2022

refactor(effects): fix wrong method return type #8817

Merged

daschuer approved these changes Aug 24, 2022

View reviewed changes

Merge branch 'main' into extend_pitch_shift_effect_options

9c42ae2

Swiftb0y reviewed Aug 24, 2022

View reviewed changes

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

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

PitchShiftEffect: rename kSemitones constant

7eef608

This commit renames the kSemitones constant into the kSemitonesPerOctave name because of its more appropriate meaning.

Swiftb0y approved these changes Aug 25, 2022

View reviewed changes

daschuer requested changes Aug 25, 2022

View reviewed changes

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

daschuer requested changes Aug 25, 2022

View reviewed changes

Swiftb0y reviewed Aug 25, 2022

View reviewed changes

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

davidchocholaty added 3 commits August 26, 2022 07:03

PitchShiftEffect: fix segmentation fault

7d40f6d

The commit fixes the segmentation fault due to a bug in the name of the parameter value (the old id "formant" was used instead of the new one, which is "formatPreserving").

PitchShiftEffect: update version

5d98c7f

The commit updates the version of the effect manifest from version 1.0 onto version 2.0.

davidchocholaty requested a review from daschuer August 26, 2022 07:34

Swiftb0y approved these changes Aug 26, 2022

View reviewed changes

daschuer approved these changes Aug 29, 2022

View reviewed changes

daschuer merged commit c076a79 into mixxxdj:main Aug 29, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PitchShiftEffect: extend effect options #4901

PitchShiftEffect: extend effect options #4901

davidchocholaty commented Aug 19, 2022 •

edited

Loading

daschuer Aug 22, 2022

Swiftb0y Aug 22, 2022 •

edited

Loading

davidchocholaty Aug 22, 2022

davidchocholaty Aug 22, 2022

daschuer Aug 23, 2022

daschuer Aug 22, 2022

Swiftb0y Aug 22, 2022

davidchocholaty Aug 22, 2022

daschuer Aug 22, 2022

daschuer Aug 22, 2022

davidchocholaty Aug 22, 2022

Swiftb0y Aug 22, 2022

daschuer Aug 22, 2022

davidchocholaty Aug 23, 2022

daschuer commented Aug 24, 2022

Swiftb0y commented Aug 24, 2022

davidchocholaty commented Aug 24, 2022

daschuer commented Aug 25, 2022

daschuer commented Aug 25, 2022

Swiftb0y commented Aug 25, 2022

daschuer left a comment

daschuer commented Aug 29, 2022 •

edited

Loading

davidchocholaty commented Aug 29, 2022

Swiftb0y commented Aug 29, 2022

davidchocholaty commented Aug 29, 2022

daschuer commented Aug 29, 2022

Swiftb0y commented Aug 29, 2022

davidchocholaty commented Aug 29, 2022

	"Change pitch in semitone-steps instead of smoothly."));
	"Change pitch in semitone-steps instead of continuously."));

	"Preserve formants."));
	"Preserve formants, the resonance frequencies of human vocal tract and other instruments."));

	"Preserve formants."));
	"Preserve the resonant frequencies of human vocal tract and other instruments (formants)"));

PitchShiftEffect: extend effect options #4901

PitchShiftEffect: extend effect options #4901

Conversation

davidchocholaty commented Aug 19, 2022 • edited Loading

Choose a reason for hiding this comment

Swiftb0y Aug 22, 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

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 Aug 24, 2022

Swiftb0y commented Aug 24, 2022

davidchocholaty commented Aug 24, 2022

daschuer commented Aug 25, 2022

daschuer commented Aug 25, 2022

Swiftb0y commented Aug 25, 2022

daschuer left a comment

Choose a reason for hiding this comment

daschuer commented Aug 29, 2022 • edited Loading

davidchocholaty commented Aug 29, 2022

Swiftb0y commented Aug 29, 2022

davidchocholaty commented Aug 29, 2022

daschuer commented Aug 29, 2022

Swiftb0y commented Aug 29, 2022

davidchocholaty commented Aug 29, 2022

davidchocholaty commented Aug 19, 2022 •

edited

Loading

Swiftb0y Aug 22, 2022 •

edited

Loading

daschuer commented Aug 29, 2022 •

edited

Loading