make scaling of GLSL RGB and RGB L/R waveform amplitudes consistent with simple waveform #12205

m0dB · 2023-10-21T23:58:23Z

As pointed out by @ronso0 in #12115 , the amplitudes of the waveform rendering is inconsistent between waveform types. I discuss the cause there.

This PR fixes the scaling, so the waveform amplitudes are the same as the simple waveform (when gains are 0), while still maintaining the adaptation of the waveform amplitude to the low/mid/high gains, by scaling the amplitude as drawn with the ratio between the low+mid+high magnitude with gains applies and without gains applied.

(Note that the wrong scaling is still present in the legacy waveforms. I will not fix those)

See #12115 for some screenshots that show the issue before this fix. With this fix, compare Simple with RGB and RGB L/R.

This is RGB and RGB L/R with some gain applied (high and low bands turned down)

…ith and without gains

m0dB · 2023-11-18T19:37:53Z

It would be good to have this merged for 2.4, but it could wait for a 2.4.1 (assuming that is on the road map).

ywwg · 2023-11-22T20:51:41Z

yesssss thank you

ywwg

thank you for this! I have a couple small questions. Leaving these as comments because I have a bad habit of not following up

ywwg · 2023-11-22T20:54:12Z

src/waveform/renderers/allshader/waveformrendererlrrgb.cpp

-                maxMid = math_max(maxMid, filteredMid);
-                maxHigh = math_max(maxHigh, filteredHigh);
+            // Cast to float
+            float maxLow = static_cast<float>(u8maxLow);


Not sure I understand why maxLow is a different type than u8maxLow. are we casting from uchar to float because of math max?

yes, this way we cast only once, rather than casting for every iteration in the for loop in line 148.

ywwg · 2023-11-22T20:55:49Z

src/waveform/renderers/allshader/waveformrendererlrrgb.cpp

-    DEBUG_ASSERT(linesReserved == m_vertices.size());
-    DEBUG_ASSERT(colorsReserved == m_colors.size());
+    DEBUG_ASSERT(reserved == m_vertices.size());
+    DEBUG_ASSERT(reserved == m_colors.size());


so I guess m_colors and m_vertices are the same size? (do we need to check both these things? This seems like an odd place to enforce this restriction rather than when reserved is calculated)

yes, they are the same size. we could remove these asserts, it's just to make sure i didn't make a mistake when filling these vectors. but they don't enforce anything, they merly check. and checking doesn't hurt.

Got it. Wouldn't it be better to check them back up where reserved is created, then? If there's something wrong with that size, then we will encounter problems before we get to this check.

What is being checked with this assert is that the amount of data that we push to the m_vertices / m_colors vectors is correct (in m_vertices.addRectangle and m_colors.addForRectangle), to make sure we reserve exactly the amount we are going to push.

src/waveform/renderers/allshader/waveformrendererrgb.cpp

ywwg · 2023-11-22T23:21:45Z

With this change, the waveforms seem over-zoomed at 1.0:

Compare to 0.9 zoom:

And RGB (not marked legacy) is a lot smaller:

ywwg · 2023-11-22T23:22:29Z

my best friend is still the softer / higher rez legacy glsl at 1.5:

daschuer

Just two comments, not yet tested.

daschuer · 2023-11-23T10:21:35Z

src/waveform/renderers/allshader/waveformrendererlrrgb.cpp

+            // The maxAll value will be used to draw the amplitude. We scale them according to
+            // magnitude of the gained maxLow, maxMid and maxHigh values


A bit more background info is useful here.
because ....

daschuer · 2023-11-23T10:39:46Z

src/waveform/renderers/allshader/waveformrendererrgb.cpp

        maxLow *= lowGain;
        maxMid *= midGain;
        maxHigh *= highGain;

+        // Calculate the magnitude of the gained maxLow, maxMid and maxHigh values
+        const float magnitudeGained = std::sqrt(


You can use only one std::sqrt() if you build the quotient (factor below) before.

Sorry, I understand what you mean here

You can make use of the rule:
factor = Sqrt(A)/Sqrt(B) = Sqrt(A/B)
To get rid of one expensive Sqrt call.

ah, well spotted! i'll make the change.

m0dB · 2023-11-23T16:23:51Z

With this change, the waveforms seem over-zoomed at 1.0:

Yes, I noticed this too, but you will see that happens to the simple waveform as well, so I guess that's "correct"

ronso0 · 2023-11-24T02:12:35Z

Are you sure this is not an issue with the pre-gain?
(didn't test this PR yet, will do asap)

…estion)

…naming and use correct range (was double!). TODO: re-add comments and apply to other waveform renderers

ywwg

This look great! and the /2 fix makes the new waveforms look SO much better. thank you!

daschuer

LGTM, Thank you.

m0dB · 2023-11-26T01:06:15Z

Thanks for merging @daschuer . I also have fixed the drawing resolution for HSV, Filtered and Simple. I wanted to include that in this PR but I will create a new one.

m0dB · 2023-11-26T01:38:28Z

#12356

fwcd · 2023-12-18T23:26:02Z

Quick ping here since this seems to be the cause for #12448.

This PR fixes the scaling, so the waveform amplitudes are the same as the simple waveform [...] Note that the wrong scaling is still present in the legacy waveforms. I will not fix those

I think the new scaling makes the waveforms a bit harder to read, e.g. very bass heavy tracks are almost "rectangular" until I turn down the visual gain manually. The old visual gain struck a nice balance by providing some "breathing space" for the waveform IMO.

Perhaps we could do it the other way around and lower the simple waveforms instead?

scale amplitude drawn with ratio between the low+mid+high magnitude w…

d26931f

…ith and without gains

m0dB mentioned this pull request Oct 22, 2023

Why do waveform types have different visual gains? #12115

Closed

m0dB requested review from daschuer and ronso0 October 22, 2023 00:20

removed unused variable

f036b0a

JoergAtGithub added the needs review label Nov 5, 2023

ywwg reviewed Nov 22, 2023

View reviewed changes

daschuer reviewed Nov 23, 2023

View reviewed changes

m0dB added 2 commits November 24, 2023 19:16

optimization: use 1 sqrt instead of two (thanks daschuer for the sugg…

52a0956

…estion)

avoid confusion between visual waveform samples and frames by better …

20bbf26

…naming and use correct range (was double!). TODO: re-add comments and apply to other waveform renderers

ywwg approved these changes Nov 25, 2023

View reviewed changes

daschuer approved these changes Nov 25, 2023

View reviewed changes

daschuer merged commit 367c2e4 into mixxxdj:2.4 Nov 25, 2023
12 checks passed

daschuer added this to the 2.4.0 milestone Nov 25, 2023

daschuer added waveform and removed needs review labels Nov 25, 2023

m0dB mentioned this pull request Nov 26, 2023

avoid confusion between visual waveform samples and frames #12356

Merged

fwcd mentioned this pull request Dec 18, 2023

New waveforms have much higher visual gain than legacy ones #12448

Closed

m0dB mentioned this pull request Oct 27, 2023

Manual: explain what the different waveform types are? mixxxdj/manual#603

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

make scaling of GLSL RGB and RGB L/R waveform amplitudes consistent with simple waveform #12205

make scaling of GLSL RGB and RGB L/R waveform amplitudes consistent with simple waveform #12205

m0dB commented Oct 21, 2023 •

edited

Loading

m0dB commented Nov 18, 2023

ywwg commented Nov 22, 2023

ywwg left a comment

ywwg Nov 22, 2023

m0dB Nov 23, 2023

ywwg Nov 22, 2023

m0dB Nov 23, 2023

ywwg Nov 25, 2023

m0dB Nov 25, 2023

ywwg commented Nov 22, 2023

ywwg commented Nov 22, 2023

daschuer left a comment

daschuer Nov 23, 2023

daschuer Nov 23, 2023

m0dB Nov 24, 2023

daschuer Nov 24, 2023

m0dB Nov 24, 2023

m0dB Nov 24, 2023

m0dB commented Nov 23, 2023

ronso0 commented Nov 24, 2023 •

edited

Loading

ywwg left a comment

daschuer left a comment

m0dB commented Nov 26, 2023

m0dB commented Nov 26, 2023

fwcd commented Dec 18, 2023

		// The maxAll value will be used to draw the amplitude. We scale them according to
		// magnitude of the gained maxLow, maxMid and maxHigh values

make scaling of GLSL RGB and RGB L/R waveform amplitudes consistent with simple waveform #12205

make scaling of GLSL RGB and RGB L/R waveform amplitudes consistent with simple waveform #12205

Conversation

m0dB commented Oct 21, 2023 • edited Loading

m0dB commented Nov 18, 2023

ywwg commented Nov 22, 2023

ywwg 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ywwg commented Nov 22, 2023

ywwg commented Nov 22, 2023

daschuer 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

m0dB commented Nov 23, 2023

ronso0 commented Nov 24, 2023 • edited Loading

ywwg left a comment

Choose a reason for hiding this comment

daschuer left a comment

Choose a reason for hiding this comment

m0dB commented Nov 26, 2023

m0dB commented Nov 26, 2023

fwcd commented Dec 18, 2023

m0dB commented Oct 21, 2023 •

edited

Loading

ronso0 commented Nov 24, 2023 •

edited

Loading