allshader RGB,Filtered and Stacked using textures for waveform data #13151

m0dB · 2024-04-21T16:12:50Z

This adds the waveforms for RGB, Filtered and Stacked that use textures to transfer the waveform data to the shader.

This uses the code from the legacy GLSL waveforms, but in combination with the rest of the allshader waveform layers.

(There is code duplication, but since we are planning on removing the legacy ones, I don't care about this)

I added an indicator "waveform texture data" (in addition to the existing indicators GLSL and legacy) to show in the waveform combobox which waveforms use textures to transfer the waveform data to the shader.

JoergAtGithub · 2024-04-21T17:14:55Z

Filtered GLSL waveform appears in RGB colors now. Otherwise it works as intended.

m0dB · 2024-04-21T17:17:40Z

Filtered GLSL waveform appears in RGB colors now.

Yes, but that is not due to this PR, it was already the case.
Edit: But I will fix that (renaming to Stacked) and add Filtered that mimics the legacy.
Edit 2: DONE.

ywwg · 2024-05-09T19:47:54Z

this looks fantastic to me -- a good step towards solving #12641 though we still have the curve-at-analyze-time problem to work on

ywwg · 2024-05-09T19:48:42Z

src/waveform/renderers/allshader/waveformrenderertextured.cpp

+    const auto lastVisualIndex = static_cast<GLfloat>(
+            m_waveformRenderer->getLastDisplayedPosition() * trackSamples / audioVisualRatio / 2.0);
+
+    // const int firstIndex = int(firstVisualIndex+0.5);


remove comment cruft?

I just copied this as-is from the legacy code.

ywwg · 2024-05-09T19:59:42Z

curve-at-analyze-time problem

oh sounds like this may be already solved

acolombier · 2024-05-09T22:48:27Z

It feels like this PR is going against the effort to reduce the number of "same" waveform type: from a user perspective, they don't care whether "RGB" is achieved with texture of GLSL and adding another type (even if we eventually retire the legacy one) will still keep the confusion. Could we replace the other allshader instead of adding 3 more type to the existing 13?

m0dB · 2024-05-09T23:24:45Z

I think this would better be discussed in Zulip. I personally prefer the non-textured ones over the textured ones. I don’t think there is a problem with having multiple options, it’s about how to present those options in a useful way.

ywwg · 2024-05-10T14:40:47Z

I posted a comparison in Zulip showing why I prefer textured. I do think we can reduce some of the options, but I would strongly prefer to find a way to keep the extra data that the textured approach shows. (I will maintain my own custom fork if I have to, it makes that much of a difference in my ability to perform)

…he waveform data to the shader (like to legacy GLSL waveforms)

… and dont add to the preferences list

m0dB · 2024-05-12T00:05:24Z

I have deprecated the fixed function pipeline OpenGL widgets and the waveform data texture based widgets that combined with QPainter (replaced with waveform data texture + allshader layers). I put the code in folders waveform/widgets/deprecated and waveform/renderers/deprecated and don't list them in the UI, leaving the preferences dropdown like this:

@ywwg @acolombier maybe we can merge this first, and then merge Antoine's software fallback waveforms, and finally do the waveform preferences reorganisation?

acolombier · 2024-05-12T13:41:03Z

Ideally, I would prefer merging #13220 first so the AllShaderFilter can implement the Option enum directly, and reduce the number of independent waveform widget

Also worth to mention that the combox box is longer on non-apple platform IIRC (3 more waveform widget excluded by pre processor).

ywwg · 2024-05-12T16:12:14Z

I agree with @m0dB to merge this first and clean up the preferences later. Either way there will be a little pain but it feels right to get the backend stuff (waveform algorithms) figured out before cleaning up the UX.

@acolombier is that ok?

ywwg · 2024-05-12T16:15:48Z

deprecated

are we planning to delete them in the future? (Are these the legacy waveforms?) If we are going to remove them, I don't see a big point in moving them to a "deprecated" folder in the meantime. It will make the git history messier, and calling things "legacy" (like the skin parser) and "deprecated" is tempting fate that they will never actually be removed :)

m0dB · 2024-05-12T19:03:59Z

The mean idea behind leaving them for now was for the sake of comparison. But I guess one might as well run 2.4 and 2.5 side by side for that.

I am okay with removing them, but I didn’t want to make that decision.

m0dB · 2024-05-12T19:05:58Z

@acolombier the advantage of merging this first is that it removes all the renderers and widgets that will not be used anymore, so that might make things a bit easier to comprehend.

ywwg · 2024-05-12T19:34:44Z

I am ok with keeping them for now, but I don't think it's worth the trouble to move the files around unless that does something I haven't realized. If we mark them in the UI as deprecated in the next version (2.6, since 2.5 is already cut), and then delete them in 2.7.

acolombier · 2024-05-12T19:45:55Z

I guess it doesn't really matter. As I mentioned on Zulip, integrating these changes in #13220 will require removing a significant amount of changes added by this PR so it felt more natural the other way around.

Either way there will be a little pain but it feels right to get the backend stuff (waveform algorithms) figured out before cleaning up the UX.

The problem is #13220 makes significant amount of changes on the backend itself (factory, allshader renderer)

FWIW, I'm keen to get either merge ASAP so we can move to the next step, as I will soon not have much time to spend on Mixxx anymore.

m0dB · 2024-05-19T17:19:57Z

Do you think anything is missing from this PR, or can we merge it?

ywwg · 2024-05-20T13:40:24Z

Do you think anything is missing from this PR, or can we merge it?

we're going to talk about this at the meeting in a few minutes :)

ywwg

I've been running this patch and it works great. LGTM!

Swiftb0y · 2024-05-20T16:21:16Z

src/waveform/renderers/waveformwidgetrenderer.h

+    template<class T_Renderer, typename T_Arg>
+    inline T_Renderer* addRenderer(T_Arg arg) {
+        T_Renderer* renderer = new T_Renderer(this, arg);
+        m_rendererStack.push_back(renderer);
+        return renderer;
+    }
+


whats the difference between this template and the one above? This shouldn't even compile IMO because its ambiguous which should be chose in the one parameter case.

You are absolutely right, I somehow missed that the above allowed for arguments.

Swiftb0y · 2024-05-20T16:29:37Z

src/waveform/renderers/allshader/waveformrenderertextured.h

+    // shaders
+    bool m_shadersValid;
+    Type m_type;
+    const QString m_pFragShader;


not a pointer

Suggested change

const QString m_pFragShader;

const QString m_fragShader;

Not my code.

Swiftb0y · 2024-05-20T16:30:46Z

src/waveform/renderers/allshader/waveformrenderertextured.cpp

+    default:
+        break;
+    }
+    assert(false);


avoid segfault during set. Use QString if ur returning a QString, otherwise it'll unnecessary do a utf8 to 16 conversion and allocation.

Suggested change

assert(false);

DEBUG_ASSERT(false);

return QStringLiteral(":/shaders/rgbsignal.frag");

Swiftb0y · 2024-05-20T16:36:20Z

src/waveform/renderers/allshader/waveformrenderertextured.cpp

+    ConstWaveformPointer pWaveform = m_waveformRenderer->getWaveform();
+    if (pWaveform) {
+        dataSize = pWaveform->getDataSize();
+        if (dataSize > 1) {
+            data = pWaveform->data();
+        }
+    }


declare variables at point of use, prefer early returns when it doesn't make sense to continue (this is not the linux kernel ;) )

Suggested change

ConstWaveformPointer pWaveform = m_waveformRenderer->getWaveform();

if (pWaveform) {

dataSize = pWaveform->getDataSize();

if (dataSize > 1) {

data = pWaveform->data();

}

}

ConstWaveformPointer pWaveform = m_waveformRenderer->getWaveform();

if (!pWaveform) {

return false;

}

int dataSize = pWaveform->getDataSize();

// is this condition correct? should it actually be just be dataSize == 0?

if (dataSize <= 1) {

return false;

}

const WaveformData* data = pWaveform->data();

This is not my code.

Swiftb0y · 2024-05-20T16:41:32Z

src/waveform/renderers/allshader/waveformrenderertextured.cpp

+    // We create a frame buffer that is 4x the size of the renderer itself to
+    // "oversample" the texture relative to the surface we're drawing on.
+    constexpr int oversamplingFactor = 4;


how is that different from multisampling?

m0dB · 2024-05-20T17:01:48Z

@Swiftb0y I see you are commenting on the waveformrenderertextured.cpp code. This is not my code. I just took the legacy GLSL renderer code and made the minimum changes in order to use it in combination with the allshader renderer layers.

I am all for cleaning up this code, and I am willing to do so in future PRs, but I don't think that just moving existing code should come with the obligation to clean it up. The improvement here is using this GLSL based renderer in combination with all the other GLSL based allshader layers instead of in combination with QPainter based layers.

Swiftb0y · 2024-05-20T18:03:58Z

@Swiftb0y I see you are commenting on the waveformrenderertextured.cpp code. This is not my code.

I'm sorry, that wasn't clear so I was just treating it as new code. Disregard my comments for now then, but please leave them open so they're easy to revisit later.

Swiftb0y

LGTM.

Swiftb0y · 2024-05-20T18:05:39Z

Merging now to keep the velocity. Thank you all for your patience. I'm looking forward to the future cleanup PRs.

github-actions bot added build waveform labels Apr 21, 2024

m0dB mentioned this pull request Apr 21, 2024

Retire Simple GL and GLSL waveforms. #11838

Open

github-actions bot added ui preferences labels Apr 21, 2024

JoergAtGithub mentioned this pull request Apr 22, 2024

allshader RGB: Show a ghosted version of the original waveform behind… #12547

Closed

JoergAtGithub added the needs review label Apr 29, 2024

JoergAtGithub mentioned this pull request May 8, 2024

feat: simplify waveform combobox #13220

Open

ywwg approved these changes May 9, 2024

View reviewed changes

ywwg mentioned this pull request May 10, 2024

Simplify waveform preferences panel #13226

Open

m0dB added 2 commits May 11, 2024 23:40

added rgb,filtered and stacked waveforms using textures to transfer t…

a277cea

…he waveform data to the shader (like to legacy GLSL waveforms)

added upgrades to new allshadertextured types

9a5c616

m0dB force-pushed the allshaders-waveform-data-texture branch from 9298a96 to 9a5c616 Compare May 11, 2024 21:40

m0dB added 4 commits May 12, 2024 01:07

allshader rgb stacked and filtered (added and renamed)

8e03133

fix compilation (adding missing enum to switch)

7ffa947

moved deprecated waveform widgets and renderers to folder deprecrated…

e6a7d27

… and dont add to the preferences list

renamed "Waveform data texture" to "high detail"

26920be

m0dB requested a review from ywwg May 12, 2024 10:41

ywwg approved these changes May 20, 2024

View reviewed changes

Swiftb0y reviewed May 20, 2024

View reviewed changes

removed needless method

40a3f84

Swiftb0y approved these changes May 20, 2024

View reviewed changes

Swiftb0y merged commit 5e7f149 into mixxxdj:main May 20, 2024
13 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

allshader RGB,Filtered and Stacked using textures for waveform data #13151

allshader RGB,Filtered and Stacked using textures for waveform data #13151

m0dB commented Apr 21, 2024

JoergAtGithub commented Apr 21, 2024

m0dB commented Apr 21, 2024 •

edited

ywwg commented May 9, 2024

ywwg May 9, 2024

m0dB May 11, 2024

ywwg commented May 9, 2024

acolombier commented May 9, 2024

m0dB commented May 9, 2024

ywwg commented May 10, 2024

m0dB commented May 12, 2024

acolombier commented May 12, 2024 •

edited

ywwg commented May 12, 2024 •

edited

ywwg commented May 12, 2024

m0dB commented May 12, 2024

m0dB commented May 12, 2024

ywwg commented May 12, 2024

acolombier commented May 12, 2024

m0dB commented May 19, 2024

ywwg commented May 20, 2024

ywwg left a comment

Swiftb0y May 20, 2024

m0dB May 20, 2024

Swiftb0y May 20, 2024

m0dB May 20, 2024

Swiftb0y May 20, 2024

Swiftb0y May 20, 2024

m0dB May 20, 2024

Swiftb0y May 20, 2024

m0dB commented May 20, 2024

Swiftb0y commented May 20, 2024

Swiftb0y left a comment

Swiftb0y commented May 20, 2024

	assert(false);
	DEBUG_ASSERT(false);
	return QStringLiteral(":/shaders/rgbsignal.frag");

-    ConstWaveformPointer pWaveform = m_waveformRenderer->getWaveform();
-    if (pWaveform) {
-        dataSize = pWaveform->getDataSize();
-        if (dataSize > 1) {
-            data = pWaveform->data();
-        }
-    }
+    ConstWaveformPointer pWaveform = m_waveformRenderer->getWaveform();
+    if (!pWaveform) {
+        return false;
+    }
+    int dataSize = pWaveform->getDataSize();
+    // is this condition correct? should it actually be just be dataSize == 0?
+    if (dataSize <= 1) {
+        return false;
+    }
+    const WaveformData* data = pWaveform->data();

allshader RGB,Filtered and Stacked using textures for waveform data #13151

allshader RGB,Filtered and Stacked using textures for waveform data #13151

Conversation

m0dB commented Apr 21, 2024

JoergAtGithub commented Apr 21, 2024

m0dB commented Apr 21, 2024 • edited

ywwg commented May 9, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ywwg commented May 9, 2024

acolombier commented May 9, 2024

m0dB commented May 9, 2024

ywwg commented May 10, 2024

m0dB commented May 12, 2024

acolombier commented May 12, 2024 • edited

ywwg commented May 12, 2024 • edited

ywwg commented May 12, 2024

m0dB commented May 12, 2024

m0dB commented May 12, 2024

ywwg commented May 12, 2024

acolombier commented May 12, 2024

m0dB commented May 19, 2024

ywwg commented May 20, 2024

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

m0dB commented May 20, 2024

Swiftb0y commented May 20, 2024

Swiftb0y left a comment

Choose a reason for hiding this comment

Swiftb0y commented May 20, 2024

m0dB commented Apr 21, 2024 •

edited

acolombier commented May 12, 2024 •

edited

ywwg commented May 12, 2024 •

edited