Library: add overview column #13638

ronso0 · 2024-09-08T23:37:47Z

This is based on @ninomp's initial POC commit 9932cfc
https://github.com/ninomp/mixxx/tree/overviewsinlibrary

I removed the disabled parts and added some TODOs/concerns.
(I'll split up some of the commits so it's a bit easier to review)

all overview renderers are now in src/waveform/overviews, i.e. WOverView and OverviewDelegate use the same renderer
(I adopted the RGB simplifications made by @Nino MP, and simplified the others, too)
the renderer selected for the decks is also used in the library (incl. live update when changing it in the preferences)

Peforms nicely, though I bet some parts can be optimized.
For example, why pass a mixxx::DbConnectionPoolPtr to static FutureResult OverviewCache::prepareOverview to create a AnyalysisDAO for each request, and not a AnyalysisDAO pointer (probably in TrackCollection)?

ninomp · 2024-09-09T00:07:14Z

For example, why pass a mixxx::DbConnectionPoolPtr to static FutureResult OverviewCache::prepareOverview to create a AnyalysisDAO for each request, and not a AnyalysisDAO pointer (probably in TrackCollection)?

IIUC Each thread needs its own database connection, we cannot share/reuse them. Actually, I'm currently experimenting with using a dedicated thread to render overviews. I'll comment back when I get it working.

ywwg · 2024-09-09T01:01:44Z

I will def review this when you think it's ready for a look

ronso0 · 2024-09-09T09:00:34Z

I will def review this when you think it's ready for a look

In case you refer to this PR (thanks!), I think it's better to wait for @ninomp's update.
I did this primarily to get it working for myself so I can test it in my live builds. (should probably add some VERIFY_OR_DEBUG_ASSERT here and there, just in case.. 😆 )

Swiftb0y

I'm afraid this needs a bunch of work before I trust it...

Swiftb0y · 2024-09-09T11:08:46Z

src/library/basetracktablemodel.cpp

+        if (row == -1) {
+            continue;
+        }


why is this needed here but not in slotOverviewChanged?

Swiftb0y · 2024-09-09T11:09:04Z

src/library/librarytablemodel.cpp

-            column == fieldIndex(ColumnCache::COLUMN_LIBRARYTABLE_WAVESUMMARYHEX) ||
+            // column == fieldIndex(ColumnCache::COLUMN_LIBRARYTABLE_WAVESUMMARYHEX) ||


remove completely, right?

Swiftb0y · 2024-09-09T11:11:13Z

src/library/overviewcache.cpp

+QString pixmapCacheKey(TrackId trackId, QSize size, WOverview::Type type) {
+    return QString("Overview_%1_%2_%3_%4")
+            .arg(static_cast<int>(type))
+            .arg(trackId.toString())
+            .arg(size.width())
+            .arg(size.height());
+}


using a string as a cache key is not particularly efficient. a struct with a qHash implementation would be better.

(I think most of this is adopted from CoverArtCache)

What about this?
https://doc.qt.io/qt-6/qpixmapcache-key.html#details

Use QPixmapCache::insert() to receive an instance of Key generated by the pixmap cache. You can store the key in your own objects for a very efficient one-to-one object-to-pixmap mapping.

Does such a one-to-one map QString <--> Key map/hash make sense performance wise?

Probably not, you're right. I didn't see that this was using QPixmapCache internally. The only improvement then is

Suggested change

QString pixmapCacheKey(TrackId trackId, QSize size, WOverview::Type type) {

return QString("Overview_%1_%2_%3_%4")

.arg(static_cast<int>(type))

.arg(trackId.toString())

.arg(size.width())

.arg(size.height());

}

QString pixmapCacheKey(TrackId trackId, QSize size, WOverview::Type type) {

return QStringLiteral("Overview_%1_%2_%3_%4")

.arg(static_cast<int>(type),

trackId.toString(),

size.width(),

size.height());

}

Swiftb0y · 2024-09-09T11:11:27Z

src/library/overviewcache.cpp

+// The transformation mode when scaling images
+const Qt::TransformationMode kTransformationMode = Qt::SmoothTransformation;
+
+inline QImage resizeImageSize(const QImage& image, QSize size) {


Suggested change

inline QImage resizeImageSize(const QImage& image, QSize size) {

QImage resizeImageSize(const QImage& image, QSize size) {

Swiftb0y · 2024-09-09T11:17:34Z

src/waveform/overviews/waveformoverviewrenderer.h

+class WaveformOverviewRenderer : public Singleton<WaveformOverviewRenderer> {
+  public:
+    QImage render(ConstWaveformPointer, WOverview::Type type);
+    void drawWaveformPartRGB(


this doesn't have internal state, nor does it need to be a singleton. Consider making this free functions instead and removing the static data. Pass it instead when calling the functions.

ronso0 · 2024-09-09T12:43:01Z

@Swiftb0y Thanks for your review, but I'm not inclined to fixup mayself since @ninomp said they would / are about to rework this anyway.

@ninomp Please share your progress early, even if it's rough and dirty.
Thanks!

ronso0 · 2024-09-09T13:04:44Z

FYI I'll keep pushing fixups to hopefully make this safer and maybe a bit faster for my own builds.

ninomp · 2024-09-11T02:23:21Z

I just created a PR that contains the work I've done in the past week or so: #13644

ywwg · 2024-09-24T13:56:56Z

I just created a PR that contains the work I've done in the past week or so: #13644

@ronso0 @ninomp is the intent that we work on that PR instead of this one? If so let's close this one so it's more clear where the active development is

ronso0 · 2024-09-24T14:42:47Z

Jup, I'm only push updates to this one to polish it a bit for my personal branch until #13644 is usable for me.

I can as well close this PR if it creates too much confusion.

ninomp · 2024-09-24T14:54:26Z

Well, personally, I'm not sure how to proceed. I tried implementing so many things, but saw little user visible improvement. Could we have a call/meeting to discuss these things before deciding how to proceed?

When I say things, I mean:

two-level cache for rendered overviews
dedicated thread for rendering overviews
moving all rendering logic away from WOverview so it can be shared/reused
...

ywwg · 2024-09-24T15:38:59Z

yeah I would be willing to be on such a call though I don't have a lot of expertise in this particular area

ronso0 · 2024-09-24T20:53:53Z

I'll close this one, moved it to ronso0#79

ronso0 · 2024-09-24T21:06:16Z

Personally I prefer a written discussion when it comes to technical stuff because I have time to look things up and think about it.

IMO we could as well discuss it on Zulip.
Just some quick feedback:

moving all rendering logic away from WOverview so it can be shared/reused

Yes, if you refer to only the waveform image rendering. IMO we should not overload the tracks table with any kind of markers (except maybe minute markers, but even that could be too much).
The OverviewRenderer works great so far (can be improved, see Swiftb0y's review above).
Not sure if it needs a dedicated thread. I didn't notice any GUI performance regression yet.

two-level cache for rendered overviews

I didn't fully understand the purpose of the first QImage cache. Is it supposed to speed up loading of overviews in WOverview when loading a track from the library? Or just save some CPU?
IMO loading overviews of tracks with waveform data is currently (main and this branch) so fast (fast enough) that I don't see the benefit tbh.

ywwg · 2024-09-25T17:43:53Z

One model that has worked well for me in the past is writing up a short doc with the relevant issues / references and then a meeting to talk it over. I think a meeting can be more effective to reach a consensus, but I agree that having a specific set of text to discuss helps in doing that.

ninomp · 2024-09-25T21:12:11Z

Yes, if you refer to only the waveform image rendering. IMO we should not overload the tracks table with any kind of markers

Actually, I was referring to overview normalization.

I didn't fully understand the purpose of the first QImage cache. Is it supposed to speed up loading of overviews in WOverview when loading a track from the library? Or just save some CPU?

The idea was to copy implementation in WOverview where we have a full sized QImage which is updated as the track is being analyzed so that we can show analysis progress. Yes, I was trying to save a bit of CPU time, but it seems that I was just trading a bit of CPU time for a lot of memory usage and I agree with you that this isn't really worth it.

@ronso0 I'm testing your branch and I can say that you managed to get this working very good. I'm currently thinking/considering closing my PR and going forward with yours. I can easily add some of my refactorings into a subsequent PR.

ronso0 · 2024-09-25T21:34:57Z

@ronso0 I'm testing your branch and I can say that you managed to get this working very good.

Thanks, you provided a great foundation!
I'm very happy with the current state, too, now that clearing and reanalyzing ie. all updates in decks and library works just fine.

FWIW my fork is here https://github.com/ronso0/mixxx/tree/lib-overview-column-ro / ronso0#79

ronso0 · 2024-09-25T21:46:58Z

I'll try to summarize, these would be nice to have:

normalization / ReplayGain
yes, more details are good, but I also like to see if it's a quiet or loud track, WYSIWYG
analysis progress
yes, thinking about it, at least something like a bar as progress indicator would be cool in the library
half / mono-mixdown
need to check with a mockup if that makes sense / looks good when it's bottom-aligned
?

ronso0 · 2024-09-26T17:29:46Z

Ha, all those items are now implemented 🎉

@ninomp I'd appreciate if you'd take a look at ronso0#79 and let me know what you think.

ninomp · 2024-09-26T21:02:01Z

@ronso0 I left a few comments on your PR.

github-actions bot added skins library ui build waveform analyzer labels Sep 8, 2024

ronso0 marked this pull request as draft September 8, 2024 23:39

ronso0 force-pushed the lib-overview-column-ro branch 2 times, most recently from 2fe92b6 to 132976a Compare September 9, 2024 08:45

ronso0 removed skins build analyzer labels Sep 9, 2024

ronso0 force-pushed the lib-overview-column-ro branch from 132976a to 4cf2f4d Compare September 9, 2024 10:28

github-actions bot added skins build analyzer labels Sep 9, 2024

ronso0 force-pushed the lib-overview-column-ro branch 2 times, most recently from 5b84f24 to be96671 Compare September 9, 2024 10:56

Swiftb0y reviewed Sep 9, 2024

View reviewed changes

ronso0 mentioned this pull request Sep 9, 2024

Add waveform overview as possible library column #8207

Open

ronso0 mentioned this pull request Sep 11, 2024

WIP: Overviews in library #13644

Draft

ninomp and others added 3 commits September 24, 2024 12:33

WIP: Overviews in library

295fa7c

Overview column: cleanup, performance fixes, TrackId instead of row etc.

57961f7

Overview: move renderers to WaveformOverviewRenderer

d9ecbb0

ronso0 added 4 commits September 24, 2024 13:13

Overview column: use same type as in decks

b970950

Overview column: update when type is changed in preferences

87945cd

WaveformOverviewRenderer: no singleton anymore

da2e0ba

Overview: move type declaration to src/waveform/overviews/overviewtype.h

45028bd

ronso0 force-pushed the lib-overview-column-ro branch from be96671 to 45028bd Compare September 24, 2024 13:27

github-actions bot added the preferences label Sep 24, 2024

ronso0 closed this Sep 24, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Library: add overview column #13638

Library: add overview column #13638

ronso0 commented Sep 8, 2024 •

edited

Loading

ninomp commented Sep 9, 2024 •

edited

Loading

ywwg commented Sep 9, 2024

ronso0 commented Sep 9, 2024

Swiftb0y left a comment

Swiftb0y Sep 9, 2024

Swiftb0y Sep 9, 2024

Swiftb0y Sep 9, 2024

ronso0 Sep 9, 2024

ronso0 Sep 9, 2024

Swiftb0y Sep 9, 2024

Swiftb0y Sep 9, 2024

Swiftb0y Sep 9, 2024

ronso0 commented Sep 9, 2024

ronso0 commented Sep 9, 2024

ninomp commented Sep 11, 2024

ywwg commented Sep 24, 2024

ronso0 commented Sep 24, 2024

ninomp commented Sep 24, 2024 •

edited

Loading

ywwg commented Sep 24, 2024

ronso0 commented Sep 24, 2024

ronso0 commented Sep 24, 2024 •

edited

Loading

ywwg commented Sep 25, 2024

ninomp commented Sep 25, 2024 •

edited

Loading

ronso0 commented Sep 25, 2024 •

edited

Loading

ronso0 commented Sep 25, 2024 •

edited

Loading

ronso0 commented Sep 26, 2024

ninomp commented Sep 26, 2024

		column == fieldIndex(ColumnCache::COLUMN_LIBRARYTABLE_WAVESUMMARYHEX) \|\|
		// column == fieldIndex(ColumnCache::COLUMN_LIBRARYTABLE_WAVESUMMARYHEX) \|\|

	inline QImage resizeImageSize(const QImage& image, QSize size) {
	QImage resizeImageSize(const QImage& image, QSize size) {

Library: add overview column #13638

Library: add overview column #13638

Conversation

ronso0 commented Sep 8, 2024 • edited Loading

ninomp commented Sep 9, 2024 • edited Loading

ywwg commented Sep 9, 2024

ronso0 commented Sep 9, 2024

Swiftb0y 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

ronso0 commented Sep 9, 2024

ronso0 commented Sep 9, 2024

ninomp commented Sep 11, 2024

ywwg commented Sep 24, 2024

ronso0 commented Sep 24, 2024

ninomp commented Sep 24, 2024 • edited Loading

ywwg commented Sep 24, 2024

ronso0 commented Sep 24, 2024

ronso0 commented Sep 24, 2024 • edited Loading

ywwg commented Sep 25, 2024

ninomp commented Sep 25, 2024 • edited Loading

ronso0 commented Sep 25, 2024 • edited Loading

ronso0 commented Sep 25, 2024 • edited Loading

ronso0 commented Sep 26, 2024

ninomp commented Sep 26, 2024

ronso0 commented Sep 8, 2024 •

edited

Loading

ninomp commented Sep 9, 2024 •

edited

Loading

ninomp commented Sep 24, 2024 •

edited

Loading

ronso0 commented Sep 24, 2024 •

edited

Loading

ninomp commented Sep 25, 2024 •

edited

Loading

ronso0 commented Sep 25, 2024 •

edited

Loading

ronso0 commented Sep 25, 2024 •

edited

Loading