No clipping indicator; UV meter is newer red

[mixxxbot]

Reported by: daschuer
Date: 2012-08-10T08:52:17Z
Status: Fix Released
Importance: Low
Launchpad Issue: lp1035224
Attachments: clipping_indicator.patch, [Modifies EngineClipping to extend the period that PeakIndicator is active](https://bugs.launchpad.net/bugs/1035224/+attachment/3258684/+files/Modifies EngineClipping to extend the period that PeakIndicator is active), [Modifies EngineClipping to extend PeakIndicator period (Qt bug fix)](https://bugs.launchpad.net/bugs/1035224/+attachment/3262828/+files/Modifies EngineClipping to extend PeakIndicator period (Qt bug fix))

---

The Deck UV Meter and the Master UV Meter will never show clipping because the "PeakIndicator" control simply changes its value to fast.
The EngingeClipping is processed just before the UV Meters. So the VuMeter control shows the clamped signal and does not provide a reliable feedback for setting the gain.

I would like to change the order so the the UV Meters are showing the unclamped samples and reset the "PeakIndicator" earliest after ~200 ms.

What do you think to the idea go a step ahead and merge EngineClipping with EngineUVMeter, This will save some CPU Time and will move all UV-Meter controls to one file.

"SampleUtil::sumAbsPerChannelAndClampBuffer"

[mixxxbot]

Commented by: daschuer
Date: 2012-08-10T23:34:47Z
Attachments: clipping_indicator.patch

---

The attached patch solves the problem, it ensures that the "PeakIndicator" is updated less often and it combines the VU meter with the clipping engine.

By the way:
Can one explain why it is a good idea to clamp the samples to a specific value?
It might be a bad idea to put such a such a ironed waveform to the output. Maybe it is better to leave the waveform untouched and let the hardware decide what to do instead of putting a waveform with an damaging DC part to the output and possibly bypass the hardware’s exception handling for this case.

[mixxxbot]

Commented by: mattmik
Date: 2012-08-11T20:55:42Z

---

Hey Daniel,
I'm currently implementing a feature that allows a compressor and clipper to be applied to any enabled hardware outputs. For example, if the user enables two deck outputs (possibly for vinyl control), each output will have its own compressor and clipper. For this reason, it might not be the best idea to combine the clipper and VU meter. My compressor modification isolates clipping and actual peak detection into separate EngineObjects. After taking a look at your patch, it should be relatively straightforward for me to modify my branch to include a similar fix for this problem.
Thanks for bringing this to our attention!
mattmik

[mixxxbot]

Commented by: daschuer
Date: 2012-08-12T08:38:27Z

---

Hi mattmike,

have you a clue, when will your solution ready for release?
I would like to have a quick fix for the clipping LED in 1.11.
Is my patch suitable for this?

A compressor is definitive a better solution than iron the waveform.
Do you think we can remove the ironing from 1.11? I have no regressions without it.

Thank you,

Daniel

[mixxxbot]

Commented by: mattmik
Date: 2012-08-12T16:03:36Z

---

Hey Daniel,

As far as I can tell, the compressor feature is ready to be included into 1.11, and I believe I've properly modified my code to fix the problem with the PeakIndicator. I'll ask Owen if we're planning to include the compressor in 1.11.

RJ, Owen, and I concluded that the best design scheme is to have a compressor applied to a signal first, and then a clipper to catch any signals that weren't compressed enough. Peak detection will be applied after the compressor and before the clipper.

mattmik

[mixxxbot]

Commented by: mattmik
Date: 2012-08-12T22:52:41Z
Attachments: [Modifies EngineClipping to extend the period that PeakIndicator is active](https://bugs.launchpad.net/mixxx/+bug/1035224/+attachment/3258684/+files/Modifies EngineClipping to extend the period that PeakIndicator is active)

---

Since 1.11 has been feature freezed, compression won't make it into the release. However, I think it's best to keep clipping separate from the VU meter. I've attached a patch for EngineClipping that simply extends the period that PeakIndicator is enabled, keeping it active for a few extra cycles. This should be enough to make any peaking that occurs noticeable. I've set the duration to 20, which we might want to lower in the release.

mattmik

[mixxxbot]

Commented by: daschuer
Date: 2012-08-13T05:59:59Z

---

Hi Mattmik,

Thank you for your patch.

There is a Bug in Qt up to 4.7 that causes to change the order of slot Updates when calling in short intervals.
Can you add a protection against setting m_ctrlClipping when it is already set?

In your patch the duration of the PeakIndikator depends on the audio buffer size (latency). I have figured out that a duration of 250 ms is fine.

Kind regards,

Daniel

[mixxxbot]

Commented by: mattmik
Date: 2012-08-13T15:46:35Z

---

Hey Daniel,

I'll change the patch to protect against this Qt bug. No problem!

So what value do you recommend I set the m_duration integer to? Should I set it to 5, as in your patch? Will that give us a duration of 250 ms?

Thanks,
mattmik

[mixxxbot]

Commented by: daschuer
Date: 2012-08-13T21:12:26Z

---

Hey Mattmik,

the process functions are called from the portaudiaudo callback normaly in regualr intervalls.
But it may happen that they are called twice without a pause. So it is not a very relyable time bas in any case.

But anyhow the engineuvmeter uses a code like this with an update rate = 20 fps. -> 50 ms.

 m_iSamplesCalculated += iBufferSize/2;
 // Are we ready to update the VU meter?:
 if (m_iSamplesCalculated > (44100/2/UPDATE_RATE) )

and I think it is also a suitable solution for this.

The "5" from my patch mutiplys this time to 250 ms for m_ctrlClipping.

Kind regards,

Daniel

[mixxxbot]

Commented by: mattmik
Date: 2012-08-14T00:04:37Z

---

Ah, I understand. Since we're trying to find a simple fix for 1.11 because we've already hit the feature freeze, do you agree that it's best to just keep a simple counter in EngineClipper instead of keeping track of the number of processed samples? The patch I've included seems to work well enough, and we can always change the implementation in the future.

[mixxxbot]

Commented by: daschuer
Date: 2012-08-14T06:28:20Z

---

Mm, I am not sure if Qt will manage to show a red frame when running at 1.3 ms ĺatency and if the Qt Bug is triggered when we choose a simple counter of 20. On the other extreme, the clipping indicator will be lit for 1,7 s at 85 ms latency.

My favorite is the sample counting solution, but i can live without it.

[mixxxbot]

Commented by: mattmik
Date: 2012-08-15T19:39:43Z
Attachments: [Modifies EngineClipping to extend PeakIndicator period (Qt bug fix)](https://bugs.launchpad.net/mixxx/+bug/1035224/+attachment/3262828/+files/Modifies EngineClipping to extend PeakIndicator period (Qt bug fix))

---

Hey Daniel,

I feel that a simple counter is a good fix just for 1.11. As I mentioned earlier, my compressor modification will move peak detection into its own EngineObject (EnginePeaking). I will gladly modify EnginePeaking to use the sample counting method, as in your patch.

I've attached the modification for my patch that includes the protection for the Qt bug you mentioned.

mattmik

[mixxxbot]

Commented by: daschuer
Date: 2012-08-21T18:53:03Z

---

Committed patch #⁠11 fom Mattmik to lp:mixxx/1.11 #⁠3342.
Thank you.

[mixxxbot]

Issue closed with status Fix Released.