Rubber band like waveform scratch at high latency

[Daniel Schürmann]

Due to the last change in Waveform scratch in 1.11 i have now a funny effect when trying to scratch with the waveform.
Like pulling on a rubber band. It is extreme on 92 ms latency. My desktop system does not suffer this problem.

[RJ Skerry-Ryan (rryan)]

I've noticed the waveform zoom level can make it feel more springy if you
are zoomed in all the way.

On Wed, Dec 5, 2012 at 4:13 PM, Daniel Schürmann <<email address hidden>
> wrote:

> Public bug reported:
>
> Due to the last change in Waveform scratch in 1.11 i have now a funny
> effect when trying to scratch with the waveform.
> Like pulling on a rubber band. It is extreme on 92 ms latency. My desktop
> system does not suffer this problem.
>
> ** Affects: mixxx
> Importance: Undecided
> Status: New
>
> --
> You received this bug notification because you are a member of Mixxx
> Development Team, which is subscribed to Mixxx.
> https://bugs.launchpad.net/bugs/1086999
>
> Title:
> Rubber band like waveform scratch on Netbook
>
> To manage notifications about this bug go to:
> https://bugs.launchpad.net/mixxx/+bug/1086999/+subscriptions
>

[Daniel Schürmann (daschuer)]

The issue is independent from the zoom factor.
If you drag the waveform in one direction and then release the mouse, it spins a bit in the opposite direction, then back, and than forward again. This cycles about 5 to 10 time depended on the latency, until it finally stops at the desired position.
The whole process is visible and audible.

[Daniel Schürmann (daschuer)]

IMO this is a 1.11.0 bug because of the very bad experience on low end hardware.

[Owen Williams (ywwg)]

Is there a way to reproduce it on good hardware? And is this effect caused by the waveform stretching code that I wrote? It's supposed to disable fast changes of stretching when the user is scratching to prevent springy appearances.

[Daniel Schürmann (daschuer)]

Yes, I can reproduce it on a recent Linux 64 bit system with maximum latency settings.

I think this a regression from Bug #881907
Maybe here?:
http://bazaar.launchpad.net/~mixxxdevelopers/mixxx/release-1.11.x/revision/3485

[Daniel Schürmann (daschuer)]

This can be reproduced on all my Linux systems and on Win XP in a virtual box.

It can be easy reproduced if you move the Mouse with left button pressed on the waveform.
The waveform is bouncing around the target position and settled down finally.

[Daniel Schürmann (daschuer)]

For me this is a 1.11.0 blocker because every new user will try at least waveform scratch and hitting this bug will lead to a bad first impression.

I looks like, it can be solved by tweaking the PID controller for different latency's.
Currently I have not managed to find a parameter set that is suitable for low and high latencies.

I am not sure yet if there is no underlying issue, but for 1.11.0 we should at least get rid of the oscillation with any latancy at cost of responsiveness

@RJ: Do you have own test results for different PID setting?

[RJ Skerry-Ryan (rryan)]

Have you tried deriving the PID parameters from the latency? Worst-case we
could have a set of parameters for when the latency is above a certain
value and a set for below.

On Wed, Feb 6, 2013 at 2:18 AM, Daniel Schürmann <<email address hidden>
> wrote:

> For me this is a 1.11.0 blocker because every new user will try at least
> waveform scratch and hitting this bug will lead to a bad first
> impression.
>
> I looks like, it can be solved by tweaking the PID controller for
> different latency's.
> Currently I have not managed to find a parameter set that is suitable for
> low and high latencies.
>
> I am not sure yet if there is no underlying issue, but for 1.11.0 we
> should at least get rid of the oscillation with any latancy at cost of
> responsiveness
>
> @RJ: Do you have own test results for different PID setting?
>
> --
> You received this bug notification because you are a member of Mixxx
> Development Team, which is subscribed to Mixxx.
> https://bugs.launchpad.net/bugs/1086999
>
> Title:
> Rubber band like waveform scratch at high latency
>
> To manage notifications about this bug go to:
> https://bugs.launchpad.net/mixxx/+bug/1086999/+subscriptions
>

[shanxS (shanx-shashank)]

> Have you tried deriving the PID parameters from the latency? Worst-case we
> could have a set of parameters for when the latency is above a certain
> value and a set for below.
>

Here is an idea, deriving PID params based on latency is not the worst
thing, but it should be this way..

Currently we are using only P controller (and it is working quite well). If
you notice, the behavior of scratching varies across latencies. For lower
latencies system reaches the target position faster as compared to higher
latencies. And for very high latency (85.3ms) system overshoots because the
update rate of current position is slow and the corrections applied shoots
beyond the target position, hence the rubber band like behavior.

One of the things that we dont want is to over shoot the target position.
The P gain should vary with latency and error (difference between target
position and current position).
Like, if latency == 10 ms

if fabs(error) > 60000

m_p = UPPER_THRESHOLD // apply corrections aggressively

else

m_p = LOWER_THRESHOLD // slow down as we approach target value, to avoid
overshoot

This kind if thing is more required for higher latency.
On my dev box, following works well,
 if latency == 85.3 ms

if fabs(error) > 160000

m_p = 0.0002 // the current value that is being used

else

m_p = 0.00005 // slow down as we approach target value, to avoid overshoot

but I dont see this as a good solution because this will not guarantee
similar behavior across latencies, m_p should vary as (some kind of
exponential) function which depends on error and latency in use in order to
make scratching more 'consistent'

my two cents.

>
>
> On Wed, Feb 6, 2013 at 2:18 AM, Daniel Schürmann <
> <email address hidden>
> > wrote:
>
> > For me this is a 1.11.0 blocker because every new user will try at least
> > waveform scratch and hitting this bug will lead to a bad first
> > impression.
> >
> > I looks like, it can be solved by tweaking the PID controller for
> > different latency's.
> > Currently I have not managed to find a parameter set that is suitable for
> > low and high latencies.
> >
> > I am not sure yet if there is no underlying issue, but for 1.11.0 we
> > should at least get rid of the oscillation with any latancy at cost of
> > responsiveness
> >
> > @RJ: Do you have own test results for different PID setting?
> >
> > --
> > You received this bug notification because you are a member of Mixxx
> > Development Team, which is subscribed to Mixxx.
> > https://bugs.launchpad.net/bugs/1086999
> >
> > Title:
> > Rubber band like waveform scratch at high latency
> >
> > To manage notifications about this bug go to:
> > https://bugs.launchpad.net/mixxx/+bug/1086999/+subscriptions
> >
>
> --
> You received this bug notification because you are subscribed to Mixxx.
> Matching subscriptions: Mixxx Dev
> https://bugs.launchpad.net/bugs/1086999
>
> Title:
> Rubber band like waveform scratch at high latency
>
> Status in Mixxx:
> In Progress
>
> Bug description:
> Due to the last change in Waveform scratch in 1.11 i have now a funny
> effect when trying to scratch with the waveform.
> Like pulling on a rubber band. It is extreme on...

[Daniel Schürmann (daschuer)]

Hi shanxS

Thank you for this good Idea, I am sure it it worth to try it If one want to spend more time in optimizing waveform scratch.

For now I have build more a band aid patch that deals with the PID parameter without introduce a non linearity.
For 1.11 I am quite satisfied with the results. See attached.

[RJ Skerry-Ryan (rryan)]

Thanks for working on this Daniel. I prefer that we do nothing to fix this. The real problem is that at high latencies the buffer size is way too high for us to be able to reasonably guide the player to a particular position because with purely proportional control we over-shoot the target in each direction every callback. I'm pretty sure it's not a solvable problem from a signals perspective. With such a low control update rate we will have instability.

This is easily solved by making a smaller user buffer (as in the attached patch). At 80ms latency with a user buffer size of 128 the waveform scratching is perfectly smooth, just delayed by 80ms. As I mentioned before I feel like we don't have adequate testing of user buffer != host buffer on all 3 platforms :-/.

[RJ Skerry-Ryan (rryan)]

(The effect of a smaller user buffer is that the rate can dynamically change every 128 samples instead of the controller having to pick a single rate for all 80ms of callback samples. This allows the rate to gradually slow and eventually stop as the player reaches the target instead of overshooting the target over and over.)

[Daniel Schürmann (daschuer)]

Thank you for looking at this. We have to be careful, not to get out of focus with tis bug.

This bug covers the overshooting of the PID controller for big big buffer sizes (control delays).
Even If we are able to set up a independent user buffer some day the controller must not overshoot at any buffer size.

My patch tries to deal with, it so that we have a fast responding control with no overshooting.
Of cause this comes with the cost of not that fast responding at higher latencies.

The user buffer size is covered in Bug #884694. I will leave additional comments there.

To me it is important to eliminate all overshooting in 1.11. Is my patch form #11 suitable for this?

[RJ Skerry-Ryan (rryan)]

Hey Daniel,

At good latencies (between 1 and 5ms) the patch from #11 sounds really bad (the audio is garbled while scratching). This is because the rate control is too stiff w/ exponential control.

Adding a workaround for high latencies only is fine with me, but I would prefer you make a high-latency scratch controller class that modularly replaces PositionScratchController so that users with good latency are not affected by the workaround at all. Basically, instead of hacking up the PID controller code to not be a PID controller anymore, I think there should be a drop-in replacement class that is only enabled when the user has high latency.

BTW -- the patch from #11 also re-introduces Bug #779179. In the current implementation m_dLastPlaypos is used to accumulate m_dPositionDeltaSum. This essentially keeps track of "samples travelled" by the engine. The velocity controller operates on samples travelled instead of position targets because otherwise the velocity controller will go crazy when it tries to reach an absolute position that lies beyond a loop boundary. It will hit the loop boundary and get farther away from the target, which will only cause it to try even harder to accelerate towards the target which result in a feedback loop.

[Daniel Schürmann (daschuer)]

Hi RJ,

thank you for review and your explanations.
Attached you will find a patch witch preserve the old scratch behavior for latencies up to 11 ms.
Greater latencies are controlled with a P-Controller and constant 0.3 p value.
The patch still includes some clean up as base for fixing the other waveform scratch bugs.

Because the difference between low and high latency is only minor, I think it is not worth introducing modularity here.

[RJ Skerry-Ryan (rryan)]

Thanks for the updated patch. I can't try this code out right now because I'm at work so I'm just running it in my head -- I just wanted to get you comments as soon as possible.

* WWaveformViewer::mousePressEvent -- I don't see why you remove the m_pScratchPosition->slotSet(0.0)? Does it matter?

* In your patches lately you're changing a lot of constructor initializer lists. I haven't mentioned it sofar because I didn't want to slow you down but the way you are changing it from is the preferred way we do it in Mixxx because
1) Many classes are written this way and
2) The Google C++ Style Guide specifies it this way (and that's the style guide we aim to follow).
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Constructor_Initializer_Lists

In the future could you keep the initializer lists in Google C++ style?

* there are some tabs in the patch

* Instead of hard-coding the PID values, please use the existing CO's that contain them m_pScratchControllerP, m_pScratchControllerI, m_pScratchControllerD (or just delete the COs, they are useful for testing though so I would like to keep them).

* I don't think you should make any changes to VelocityController. The changes you made no longer make it a PID controller. Also, since we are only using P I think that you shouldn't change the I and D codepaths since there's no need. I could be reading this wrong but I think the change you made to the P code path (dividing the position and target_position by dt) is completely equivalent to dividing the P parameter by the latency which is what I suggest in #8 and IMO cleaner / simpler.

* Oops, I just noticed when trying to figure out why you were multiplying the P component for high latency by the buffer size that this is to cancel out dividing by the buffer size in the delta-position/set-point (which on my first read I read as you dividing by dt) Instead of the buffer size we should always use time because buffer size varies with both latency and sample rate whereas time only varies with latency. Using buffer size changing your sample rate between 44.1/44/96 will change the behavior of scratching at the same latencies.

* Looking back on the m_dScratchTime variable, I have no clue what that was for :). I think it's incorrect that it accumulates dt. The observation time argument should just be 'dt' as you have changed it to be.

* Why have you made the change to stop accumulating the error in VelocityController if the error is less than 0.1? Did you see issues with denormals accumulating or other numerical precision issues?

* I prefer to keep the set-point of the PID built into VelocityController via the reset()/setTarget() method since then the controller is in charge of calculating its own error. Plus it keeps your patch simpler -- it's already hard enough to tell what's going on since this is a complicated part of Mixxx.

* As we talked about in Bug #1117762, if you click the waveform it's intentional that you have "grabbed" it and the track stops.
I'll have to try out your patch to see how this works in practice but I think that when first enabling the scratch controller the rate should be controlled by the velocity controlle...

[Daniel Schürmann (daschuer)]

Thank you for your quick response and your detailed review!

> * WWaveformViewer::mousePressEvent -- I don't see why you remove the
> m_pScratchPosition->slotSet(0.0)? Does it matter?
This posbbibly leads to unwanted jumps like Bug #1117768.
I am not able to test this, but for me there is no case where it must be there,
so it is better to remove it.

> In the future could you keep the initializer lists in Google C++ style?
Ah, I just noticed that Eclipse supports the Google style well in this case.
No Problem.

> * there are some tabs in the patch
I will remove them

> * Instead of hard-coding the PID values, ...
If you need them, I will put them in again. Should they be still
undocumented? The things will getting complicated due to the buffer
size dependency.

> * I don't think you should make any changes to VelocityController. ..
It is still a PID controller. The changes I have made are just removing the sample
rate dependency from the control parameters.
My changes normalize the controller to 1 because the rate output was also normalized to 1
As you also noticed my changes for small buffer sizes are
reintroducing this sample rate dependency. This is exactly the old behavior,
But we should make It sample rate independent as well.
So I will fix this.

> * Why have you made the change to stop accumulating the error in
>VelocityController if the error is less than 0.1? ..
I have found this in a reference PID code and while tweaking the I part
I have notice that this will be required. Maybe there is also a overflow
protection, like in the reference design required, if we use the I part.
The current value 0.1 is just random.

> * reset()/setTarget() ..
For me it was not the question of a simple patch but of a simple code.
For me it is cleaner to have one calculating function with all relevant
data as parameter instead of dealing with member variables.

> * As we talked about in Bug #1117762, ..
I have removed the tweaks for scratch start. There should be no notable change
in the scratch start behavior due to my patch but I will make a patch for
Bug #1117762 soon.

[RJ Skerry-Ryan (rryan)]

So, if I understand correctly you meant to normalize dt so that the time step is 1. Now I understand but I do not think that this works. If the normalization is just a notation change then it should have no effect on the calculated values. But you can see that it does have an effect because the integral and derivative terms are calculated differently. The integral is discretely evaluated by summing error * dt whereas the derivative term is error/dt. If you divide the inputs to observation() by dt then I agree the derivative term is equivalent to having removed the /dt factor but the integral term is not the same as if you had not applied the transformation.

This is all a little moot since we do not use I or D control but we should stick to the standard equation:
http://en.wikipedia.org/wiki/PID_controller#PID_controller_theory

[RJ Skerry-Ryan (rryan)]

> > * reset()/setTarget() ..
> For me it was not the question of a simple patch but of a simple code.
> For me it is cleaner to have one calculating function with all relevant
> data as parameter instead of dealing with member variables.

Well, you just moved the member variable from one class to another :). I think of a 'set point' as being a property of a PID controller which is why I put it in there.

[RJ Skerry-Ryan (rryan)]

> > * Instead of hard-coding the PID values, ...
> If you need them, I will put them in again. Should they be still
> undocumented? The things will getting complicated due to the buffer
> size dependency.

*shrug* actually they aren't that useful so feel free to delete them. either way works

[Daniel Schürmann (daschuer)]

In reply to #20. dt

Ah, sorry for the confusion! I have left misleading comments from the previous version in the patch.
A universal PID controller needs to have dt inside. Because then, it is possible to deal with changing sample rates.
Lucky we have a constant sample rate during a scratch. So I have took a simplified PID controller where dt is calculated into i and d. I will put in a proper comment if we want to use D and I part one day.

One other thing is more painful: The non constant mouse sample rate, leading to bug Bug #1117806.

[Daniel Schürmann (daschuer)]

Attached you find an updated patch.
In addition it was required to deal with baserate for a correct normalization.

[Daniel Schürmann (daschuer)]

Attached you find a patch with an optimized scratch start.
It starts with a saddled p-controller, fixing also Bug #1117762

[jus (jus)]

Hi Daniel, thanks for the patch which solved the rubberband effect.

* Is the delay intentional that occurs when you hold the waveform? It is noticeable and feels a little strange. The higher the latency, the more significant the effect, nearly brake-like at high latency. If you touch a vinyl, it holds immediately. Thats what i would expect from our mouse scratch too.
* Unsure if part of this bug, but at waveform zoom level 100% (max.zoom in) the ratio between mouse movement<>waveform movement when throwing a waveform feels really good. OTOH at waveform zoom level 16% (max.zoom out) the ratio is too high and waveform trowing sounds artificial. I`d suggest to use a value same to/ little higher than at 100% zoom level.

[Daniel Schürmann (daschuer)]

Hi Jus, thank you for testing!

I have accidentally doubled the throw time. In the code is was documented as 2 seconds, but actually it was only 1 second.
Due to my other fixes it becomes actually 2 second. The attached patch restores the old behavior.

The difference from the zoom factor you have noticed was introduced by Bug #1094887. Throwing is still zoom factor independent but you can achieve a higher initial speed on minimum zoom. If you have an idea to make this more natural, pleas fill a separate bug.

In the attached patch I have removed the waveform scratch start fix.
I will put a comment at Bug #1117762

[Daniel Schürmann (daschuer)]

In addition this patch takes MIN_SEEK_SPEED into account to avoid overshooting with low rates.
I would like to commit it to lp:mixxx/1.11 or are there still any objections?

[jus (jus)]

Thanks Daniel, tested the latest patch and scratching feels good now. No objections from my side.

If these changes are commited, we can mark lp: 883819 as fixed too IMO.

[Daniel Schürmann (daschuer)]

Thank you Jus, for testing again!
Committed to lp:mixxx/1.11 revision 3759

[Swiftb0y (swiftb0y)]

Mixxx now uses GitHub for bug tracking. This bug has been migrated to:
https://github.com/mixxxdj/mixxx/issues/6746