Allow maximum deck speed of 4x normal

[Pegasus-RPG]

• Max speed must at least be 2x normal (100%, 1.0) for certain controller pitch range change wraparounds (AA VMS, Reloop TerminalMix) and effects (Stanton SCS.3d) to work. (see Reloop TerminalMix 2/4 :: mapping update #1490 (comment))
• There is a use case for increasing speed beyond 100% such as sample pads or a keyboard sample controller. (Beyond 400% starts to become inaudible so that's a good upper bound. Otherwise there's no need to artificially limit it here.)

[uklotzde]

The DDJ-SX script has just been modified and now explicitly uses the constant 0.9 internally. Might controller scripts be affected by this change?

[Pegasus-RPG]

There's always the possibility if they just assumed it could never go above 90%/0.9. But there's no way for scripts to query the limits of a MixxxControl so they couldn't have known the previous maximum for sure. (Not having this at least at 100%/1.0 does cause problems with some controllers.)

[Pegasus-RPG]

(What does the DDJ-SX script do with this information, out of curiosity?)

[Pegasus-RPG]

Oh, you mean this?

mixxx/res/controllers/Pioneer-DDJ-SX-scripts.js

Line 1389 in 833eae7

if (range === 0.90) {

That should continue to work as-is actually. It just won't go above 90% unless the script is modified.

[Be-ing]

IIRC this limit was implemented because SoundTouch and/or Rubberband don't really work beyond 90%.

[Pegasus-RPG]

Sure, but the use cases I mentioned do not depend on key correction.

[poelzi]

how about drawing the keycorrection button or slider alarming red in case this combination would occurre. for example, if I have speed increase of 100 % the keylock button changes to red. if I have the keylock active and start changing the playback speed over a certain threshold, the slider gets red. when I finish the messaging infrastructure we can safely give user feedback

[Pegasus-RPG]

@Be-ing I think it may have also been so users can't set the range to 100%, move the pitch slider all the way down, then wonder why the deck isn't playing even though Play is lit. @poelzi 's idea would address that problem though. (But can we please do that in another PR?)

[Pegasus-RPG]

Ping! Can we merge this?

[Be-ing]

Have you tested how this behaves at extreme rate changes with both SoundTouch and Rubberband?

[daschuer]

I have just tested this and I cannot see an issue. LGTM

I am planning to improve the usability of the rate slider along with this bug:
https://bugs.launchpad.net/mixxx/+bug/1762172

The first step is to introduce a GUI independent "rate_ratio" control here:
#1765
Can anyone have a look?

[Be-ing]

Please revert this in the 2.1 branch. This should have been merged to master, not a release branch. I raised a serious concern above which was never addressed before merging this.

[daschuer]

Did you mean this concern:

Have you tested how this behaves at extreme rate changes with both SoundTouch and Rubberband?

I have tested it extensively and it works. It also has no negative effect, because no one is using the extended range right now. There is no mapping that uses this as a slider so the change will be not notable.

[Be-ing]

Okay, thank you for clarifying. Your previous comment did not imply that you had tested that.

Regardless, we need to be way more conservative about what goes into release branches. There is no pressing need for this to be changed in 2.1.x and it is hard to predict what side effects it may have by violating an implicit assumption elsewhere in the code. Have we not learned our lesson already from https://bugs.launchpad.net/mixxx/+bug/1777429 ?

[Pegasus-RPG]

Other parts of the code should not be making such assumptions, as they can check the range of this Control. (The only things that have to make assumptions are scripts, and even those are minimized with the set/getParameter Controls..)

[Be-ing]

I agree that other code shouldn't be making such assumptions, but it's not that simple considering the complex relationship between rate_range, rate, and the actual playback speed. There is also lots of sloppy code in Mixxx, especially in controller scripts, and simply saying code shouldn't do that doesn't change that there may be code lurking somewhere that does. Anyway, that doesn't change that this is a really minor issue and the change could have serious unanticipated side effects. #1679 was also fixing a trivial issue that AFAIK no one was really complaining about. Rushing that into a release branch introduced a serious regression that quickly became one of the most discussed issues on the forums for which we still don't have any fix released for users. Release branches receive only minimal testing before the next release is put into users' hands. Thus, changes to release branches should only be for major bugs that users are reporting actually interferes with them being able to use Mixxx. Otherwise, users cannot depend on releases being stable and it is like every release is a beta.

[daschuer]

You express only an abstract fear, not related to mistakes we have made earlier.
What is actual the issue here? I have tested it, and searched for side effects in the scripts and mapping code. I have found nothing.
This is IMHO worth to be included in 2.1.2, because scripts are normally done after release.
This way all work that is done later will benefit from the feature.

[Be-ing]

The issue here is rushing barely tested changes to users and calling them stable. One person manually testing a change like this is an okay standard for merging to the master branch because that gets tested by many people for months before being released. It's really hard to predict what side effects this change may have considering the complexity of the playback speed and sync code. I tested #1679 and it seemed fine -- but no one thought to test whether it might affect hotcues, so we only found out about the regression later from users who had the regression interfere with their use of what they believed was a stable, reliable release.

[Be-ing]

How does this interact with https://github.com/mixxxdj/mixxx/blob/2.1/src/engine/bpmcontrol.cpp#L318 ?

[Pegasus-RPG]

@Be-ing That code correctly uses rate_range to figure out where to put the slider during a sync event. It's not sensitive to what that range actually is.

[kazakore]

With release x.y.z I generally think the z point released should be pretty much purely bug fixes and security updates. New features and anything that even has a slim possibility to break things should go into the y point release.

Additionally I remember a fairly recent conversation about possibly changing the way the interpolation/pitch shifting works. One idea was to force linear interpolation for Scratch Mode. I don't know if this is unique code or related to playback speed (eg anything more than a factor of 2 faster/slower.) If such changes are in the pipeline they should probably be considered in tandem to this change....