use WaveformWidgetType::AllShaderRGBWaveform as autoChooseWidgetType #11822

m0dB · 2023-08-13T06:41:08Z

No description provided.

daschuer

LGTM

However this makes me think what is the best way to get rid of the old GLSL types that are known buggy #11786 #11399 #9589 with MIXXX_USE_QOPENGL defined.

How about remove the enum value "WaveformWidgetType::AllShaderRGBWaveform = 17" and use the old "WaveformWidgetType::GLSLRGBWaveform = 12" value for both conditionally via the MIXXX_USE_QOPENGL macro? We may do this for all GLSL types.

The drawback is that the 2-4-alpha use that have selected a different "All Shaders" type will end up with RGB.

What do you think?

A related issue is #8639 btw.

Swiftb0y · 2023-08-13T11:05:34Z

So this changes the default, but how do we make sure that existing installations also get changed on update?

ronso0 · 2023-08-13T12:20:19Z

You mean map the legacy types to the respective new type?
Either src/preferences/upgrade.cpp or some upgrade logic in WaveformWidgetFactory::setConfig.
I'd prefer the latter to have the config operations in one place.

daschuer · 2023-08-13T15:50:06Z

How about my idea to just remove the enums for the All Shader types and instead replace the GLSL type with them according to the MIXXX_USE_QOPENGL macro.

This allows a smooth transition forward and backward between 2.3 and 2.4 and users without accelerated types are not bothered.

Swiftb0y · 2023-08-13T17:16:46Z

How about my idea to just remove the enums for the All Shader types and instead replace the GLSL type with them according to the MIXXX_USE_QOPENGL macro.

That still leaves people on the GL variants which are also broken.

daschuer · 2023-08-13T20:46:24Z

That still leaves people on the GL variants #11805 (comment).

For my understanding the Shader variants have higher GPU requirements than the GL variants. So I consider the GL users use them for some reasons. At least when they where introduced they did not run everywhere. What is the situation now?

daschuer · 2023-08-13T21:01:02Z

At least we check QOpenGLShaderProgram::hasOpenGLShaderPrograms(). Maybe this is also an issue with openGLES on Rasberry Pi and friends.

But anyway, the question of migrating users from GL to All-Shaders can be decided separate form the question to use one settings for GLSL and All Shaders depending on the Mixxx version/build flags.

Swiftb0y · 2023-08-14T12:26:54Z

For my understanding the Shader variants have higher GPU requirements than the GL variants. So I consider the GL users use them for some reasons. At least when they where introduced they did not run everywhere. What is the situation now?

not sure. I think its pretty safe now. The shaders all only require OpenGL 2.1 and might also be compatible with OpenGL ES. But I have no experince with GLES so idk.

m0dB · 2023-08-14T14:39:09Z

I expect the all-shaders waveform types to be faster on all platforms and they should work on OpenGL ES (I haven't tested this but I don't see why not).

I would advocate for forcing all users that upgrade to 2.4 to the default (RGB all-shaders) either by using the upgrade mechanism @ronso0 mentioned (not in front of a computer so not sure about this) or by bumping the value of the old enums to a higher value so the switch will fail on fall back to the default (but they will still be available for selection).

m0dB · 2023-08-16T07:37:21Z

I would also like to change the default fps to 60, as it should run smooth even on older hardware and the initial impression is much nicer. What do you think?

m0dB · 2023-08-16T08:04:52Z

With all the discussion here and in zulip, and looking at the code, it's still not clear to me what it is we want to do. So let me resume what I think it the objective:

The allshaders waveform types are generally preferred
When possible we maintain for now the other waveform types
New users should start with a good default (I would say "RGB (all-shaders)")
Old users with waveform types that don't exist anymore should either
4.a. start with the same as 3
4.b. start with an allshader waveform type that matches the old config as good as possible
Old users with waveform types that still exist should be proactively moved to an allshader waveform type but have the possibility to go back. The waveform type they are moved to is either
5.a. the same as 3
5.b. an allshader waveform type that matches the old config as good as possible

Is this correct?

If so, I don't think it's worth it bothering with 4.b. and 5.b.; we could simply move all users to "RGB (all-shaders)", users that changed the waveform type in the past will know how to change it again.

Swiftb0y · 2023-08-16T11:10:44Z

Is this correct?

In my opinion, this is completely correct, though I don't know whether every team member agrees with me here.

If so, I don't think it's worth it bothering with 4.b. and 5.b.; we could simply move all users to "RGB (all-shaders)", users that changed the waveform type in the past will know how to change it again.

Right, its not super necessary, but IMO trivial enough to implement (just a giant switch case mapping in a separate function somewhere) that we should still do it. Though I'm willing to change my opinion if it turns out that it does require more work than the 20 SLOC I estimate for that switch-case.

Swiftb0y · 2023-08-16T11:12:58Z

As ronso0 pointed out already (thank you). The config migration is likely trivial to implement here:

mixxx/src/preferences/upgrade.cpp

Line 453 in 28ad941

daschuer · 2023-08-16T17:15:43Z

The most work for me.

Did you consider the idea to hard replace the GLSL types with the all Shader types? This way we don't even need to migrate any config.

We can do the same for the GL types if we are confident they are no longer needed and buggy anyway. This is all around the question if there is hardware that has openGL but no OpenGLShaderProgram::hasOpenGLShaderPrograms()

From the history of Mixxx I remebers some issues with badly configured GPU drivers. There is also a blacklist in Qt. But things may have changed. My old EPC was able to use the GL waveforms with Qt4 but not with Qt5. And how about devices using Angle?

The 60 Hz default is a good idea in any case.

m0dB · 2023-08-16T18:50:33Z

Yes, I would be very happy to remove all the old waveform types and it would certainly make things easier. I think that by now the all-shaders waveform types have been thoroughly tested, they out-perform the other waveform types (which in addition have known issues), the code is cleaner and has been through much more scrutiny. All the redundancy is not user friendly and increases maintenance burden. An additional advantage would be getting rid of the "(all-shaders)" tag. I never have been happy with that nomenclature.

But honestly I don't think this is my decision to make. So I'll do this if there is consensus that this is the way to go. In any case, I would not remove the implementations, only the possibility to select them. We could even add a command line option --showAllWaveformTypes.

I am pretty sure that hardware that doesn't support this very basic OpenGL used by the all-shaders waveforms will not be able to run Mixxx decently anyway!

Swiftb0y · 2023-08-16T19:00:00Z

Did you consider the idea to hard replace the GLSL types with the all Shader types? This way we don't even need to migrate any config.

Yes I did consider that but I have thre issues with that. In ascending importance:

I don't like the idea of switching out these implementations by changing the contents of an enum (something that probably shouldn't have been exposed in the first place).
I have with that is that considering the timing of the all-shader introduction we should preserve as much of the previous waveforms since I'm not confident they will replace the legacy waveforms in 100% of all cases.
In order to confidently retire the legacy waveforms (GL & GLSL) we need to be confident they can actually serve as a replacement. In order to test that, we should do a test rollout (this would usually only be done to a fraction of users, but in this case I'd argue we could do it for everyone). In order to confidently remove the GL waveforms in future versions (which, let me remind you, will be required for Qt6!) we need to ensure the all-shaders can actually replace them.

From the history of Mixxx I remebers some issues with badly configured GPU drivers. There is also a blacklist in Qt. But things may have changed. My old EPC was able to use the GL waveforms with Qt4 but not with Qt5. And how about devices using Angle?

Right, that's one of the reasons for 3. If people experience issues, they can reported them and we can instruct them to manually switch back.

m0dB · 2023-08-16T19:23:57Z

Ok, those are valid points, @Swiftb0y . But in that case I do think it would be good to not only actively move users to the all-shaders waveform types (in upgrade.cpp), but also to mark the other waveform types as "legacy". This way old and new users will understand that the all-shaders ones are the ones to use by default. And it allows me to get rid of the "all-shaders" label ;-)

Would that work for you?

Swiftb0y · 2023-08-16T19:24:54Z

Yes, lets mark them as legacy.

m0dB · 2023-08-16T19:25:56Z

Okay, I'll add these changes to this PR.

…pes, relabel the legacy types as (legacy)

m0dB · 2023-08-17T11:45:37Z

I have made the following changes, which I hope match what we discussed and agreed upon:

Changed the default framerate to 60 fps and the default waveform type to AllShaderRGBWaveform
When upgrading from pre-2.4.0 to 2.4.0:
map the WaveformType as set in the config to the most similar allshader waveform widget type and change the config
set the framerate to 60 fps
In the preference dialog, the "(all-shader)" postfix has been removed, and instead the old waveform widget types are now marked "(legacy)"
I did this by changing the static bool developerOnly() to static WaveformWidgetCategory category(). Based on the category a different string is added to the name in the preferences panel. So developerOnly is now WaveformWidgetCategory::DeveloperOnly.

While doing the mapping I realized I never added the all-shader implementation of the HSV waveform type (which got lost in some branch). I have added this.

To test if the upgrade mechanism works, you will either have to edit your mixxx.cfg file and set the Version to 2.3.9, or you will have to set(MIXXX_VERSION_PRERELEASE "") in the CmakeLists.txt

Swiftb0y

Thank you very much for your continued time-consuming effort. I have a couple minor comments.

src/preferences/upgrade.cpp

src/waveform/renderers/allshader/waveformrendererhsv.cpp

src/waveform/renderers/allshader/waveformrendererhsv.h

src/waveform/waveformwidgetfactory.cpp

src/waveform/widgets/allshader/hsvwaveformwidget.h

Co-authored-by: Swiftb0y <12380386+Swiftb0y@users.noreply.github.com>

Swiftb0y

LGTM otherwise. Also please go back and rewrite the last couple of commits to apply pre-commit on the failing commit. Thank you

src/preferences/upgrade.cpp

Swiftb0y

Thank you. waiting for CI and someone more knowledgeable to clear up my unresolved review comment.

m0dB · 2023-08-18T18:04:17Z

@Swiftb0y your comment has been resolved, right? The upgrades are now all in ascending version order. I have tripple checked and I am sure the code is correct. Besides, it's all a bit academic; apart from the upgrade I added, the remaining upgrade code is for versions that are very old.

I would like to see this PR approved and merged as soon as possible. It is unfortunate to have to tell people all the time "Try 2.4, but please make sure you change the waveform type first" 😅

JoergAtGithub · 2023-08-18T18:11:57Z

If I see this right, @ronso0 wants to have a final look here. But he's away from keyboard for some days.

ronso0 · 2023-08-18T18:58:25Z

Not quite, since I'm also not super familiar with upgrade.cpp I'd just be that second pair of eyes. I doubt I find time before sunday, so if we're in a hurry someone else needs to take a look.

m0dB · 2023-08-26T21:31:35Z

Ping!

Swiftb0y

ronso0 seems busy so I took another review lap. Found this minor nitpick but LGTM otherwise.

src/preferences/upgrade.cpp

m0dB · 2023-08-28T11:35:00Z

Thanks, @Swiftb0y . I applied your suggested change.

ronso0 · 2023-08-28T12:33:29Z

src/preferences/upgrade.cpp

+    case WWT::GLSLRGBStackedWaveform:
+        return WaveformWidgetType::AllShaderHSVWaveform;
+    case WWT::QtHSVWaveform:
+        return WaveformWidgetType::AllShaderHSVWaveform;


Suggested change

case WWT::GLSLRGBStackedWaveform:

return WaveformWidgetType::AllShaderHSVWaveform;

case WWT::QtHSVWaveform:

return WaveformWidgetType::AllShaderHSVWaveform;

case WWT::GLSLRGBStackedWaveform:

case WWT::QtHSVWaveform:

return WaveformWidgetType::AllShaderHSVWaveform;

And I assumed we always declare the default: case, but I'm not sure whether that is a rule or just habit.

Good catch, but your suggestion looks wrong. Do this instead:

Suggested change

case WWT::GLSLRGBStackedWaveform:

return WaveformWidgetType::AllShaderHSVWaveform;

case WWT::QtHSVWaveform:

return WaveformWidgetType::AllShaderHSVWaveform;

case WWT::GLSLRGBStackedWaveform:

return WaveformWidgetType::AllShaderRGBWaveform;

case WWT::QtHSVWaveform:

return WaveformWidgetType::AllShaderHSVWaveform;

And I assumed we always declare the default: case, but I'm not sure whether that is a rule or just habit.

I'm not sure either. In this case it doesn't really make a difference. If there is no default: then it will just execute the following code... IMO its better to have an unconditional return statement at the end of the function instead of having a return statement in all possible branches. But that's a style choice really...

Good catch, but your suggestion looks wrong. Do this instead:

Yes, I didn't check the upgrade behaviour in detail, just the proposed code, so yeah: migrating RGB -> RGB makes sense, of course.

ronso0 · 2023-08-28T12:37:18Z

ronso0 seems busy so I took another review lap. Found this minor nitpick but LGTM otherwise.

Yes, I was busy and afk, and this slipped off my ToDo list tbh, sorry for that.
I commented on the upgrad.cpp changes.

src/preferences/upgrade.cpp

Co-authored-by: Swiftb0y <12380386+Swiftb0y@users.noreply.github.com>

m0dB · 2023-09-01T20:35:42Z

I rebased the last 3 commits (grouping per case, use of WWT) into 1 commit, not sure if I should have done that.
Anyway, I think it's all good now and ready to be merged! Thanks for you help.

Swiftb0y

I rebased the last 3 commits (grouping per case, use of WWT) into 1 commit, not sure if I should have done that.

Thank you, that's perfect. LGTM

daschuer · 2023-09-03T23:08:55Z

Now the box looks more overcrowded than ever. I think it needs some love.
I have just filed: #11915

daschuer approved these changes Aug 13, 2023

View reviewed changes

use WaveformWidgetType::AllShaderRGBWaveform as autoChooseWidgetType

7359d47

proactively move upgrading users to the all-shader waveform widget ty…

27490c0

…pes, relabel the legacy types as (legacy)

m0dB force-pushed the default-waveform-allshaders branch from 95229d4 to 27490c0 Compare August 17, 2023 11:29

github-actions bot added build ui labels Aug 17, 2023

default framerate 60

10f938f

Swiftb0y requested changes Aug 17, 2023

View reviewed changes

m0dB and others added 3 commits August 17, 2023 16:02

Update src/waveform/renderers/allshader/waveformrendererhsv.cpp

d323ba4

Co-authored-by: Swiftb0y <12380386+Swiftb0y@users.noreply.github.com>

Update src/waveform/waveformwidgetfactory.cpp

9fd3489

Co-authored-by: Swiftb0y <12380386+Swiftb0y@users.noreply.github.com>

removed unneeded destructors, dont upgrade Software waveform types

248bced

m0dB requested a review from Swiftb0y August 17, 2023 15:40

Swiftb0y reviewed Aug 17, 2023

View reviewed changes

src/preferences/upgrade.cpp Outdated Show resolved Hide resolved

m0dB added 2 commits August 17, 2023 20:45

formatting

f26601b

proper order of upgrades and bumped cleanVersion to 2.4.0

0023259

Swiftb0y approved these changes Aug 17, 2023

View reviewed changes

Swiftb0y reviewed Aug 27, 2023

View reviewed changes

src/preferences/upgrade.cpp Outdated Show resolved Hide resolved

ronso0 reviewed Aug 28, 2023

View reviewed changes

Swiftb0y requested changes Aug 28, 2023

View reviewed changes

src/preferences/upgrade.cpp Outdated Show resolved Hide resolved

group case by return value and use WWT alias

f569ff0

Co-authored-by: Swiftb0y <12380386+Swiftb0y@users.noreply.github.com>

m0dB force-pushed the default-waveform-allshaders branch from 9aa0f46 to f569ff0 Compare September 1, 2023 20:29

Swiftb0y approved these changes Sep 2, 2023

View reviewed changes

Swiftb0y merged commit df702f7 into mixxxdj:2.4 Sep 2, 2023
11 checks passed

use WaveformWidgetType::AllShaderRGBWaveform as autoChooseWidgetType #11822

use WaveformWidgetType::AllShaderRGBWaveform as autoChooseWidgetType #11822

Conversation

m0dB commented Aug 13, 2023

daschuer left a comment

Choose a reason for hiding this comment

Swiftb0y commented Aug 13, 2023

ronso0 commented Aug 13, 2023

daschuer commented Aug 13, 2023

Swiftb0y commented Aug 13, 2023

daschuer commented Aug 13, 2023

daschuer commented Aug 13, 2023

Swiftb0y commented Aug 14, 2023

m0dB commented Aug 14, 2023

m0dB commented Aug 16, 2023

m0dB commented Aug 16, 2023

Swiftb0y commented Aug 16, 2023

Swiftb0y commented Aug 16, 2023

daschuer commented Aug 16, 2023

m0dB commented Aug 16, 2023

Swiftb0y commented Aug 16, 2023

m0dB commented Aug 16, 2023

Swiftb0y commented Aug 16, 2023

m0dB commented Aug 16, 2023

m0dB commented Aug 17, 2023

Swiftb0y left a comment

Choose a reason for hiding this comment

Swiftb0y left a comment

Choose a reason for hiding this comment

Swiftb0y left a comment

Choose a reason for hiding this comment

m0dB commented Aug 18, 2023

JoergAtGithub commented Aug 18, 2023

ronso0 commented Aug 18, 2023

m0dB commented Aug 26, 2023

Swiftb0y left a comment

Choose a reason for hiding this comment

m0dB commented Aug 28, 2023

ronso0 Aug 28, 2023

Choose a reason for hiding this comment

ronso0 Aug 28, 2023

Choose a reason for hiding this comment

Swiftb0y Aug 28, 2023

Choose a reason for hiding this comment

ronso0 Aug 28, 2023

Choose a reason for hiding this comment

ronso0 commented Aug 28, 2023

m0dB commented Sep 1, 2023

Swiftb0y left a comment

Choose a reason for hiding this comment

daschuer commented Sep 3, 2023