Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Library: add overview column #13638

Closed
wants to merge 7 commits into from
Closed

Conversation

ronso0
Copy link
Member

@ronso0 ronso0 commented Sep 8, 2024

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)
image

  • 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
Copy link
Contributor

ninomp commented Sep 9, 2024

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
Copy link
Member

ywwg commented Sep 9, 2024

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

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

ronso0 commented Sep 9, 2024

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.. 😆 )

Copy link
Member

@Swiftb0y Swiftb0y left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment on lines +1227 to +1229
if (row == -1) {
continue;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why is this needed here but not in slotOverviewChanged?

column == fieldIndex(ColumnCache::COLUMN_LIBRARYTABLE_WAVESUMMARYHEX) ||
// column == fieldIndex(ColumnCache::COLUMN_LIBRARYTABLE_WAVESUMMARYHEX) ||
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove completely, right?

Comment on lines 21 to 27
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());
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(I think most of this is adopted from CoverArtCache)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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());
}

// The transformation mode when scaling images
const Qt::TransformationMode kTransformationMode = Qt::SmoothTransformation;

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment on lines 11 to 14
class WaveformOverviewRenderer : public Singleton<WaveformOverviewRenderer> {
public:
QImage render(ConstWaveformPointer, WOverview::Type type);
void drawWaveformPartRGB(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Member Author

ronso0 commented Sep 9, 2024

@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
Copy link
Member Author

ronso0 commented Sep 9, 2024

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

@ninomp
Copy link
Contributor

ninomp commented Sep 11, 2024

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

@ywwg
Copy link
Member

ywwg commented Sep 24, 2024

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
Copy link
Member Author

ronso0 commented Sep 24, 2024

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
Copy link
Contributor

ninomp commented Sep 24, 2024

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
Copy link
Member

ywwg commented Sep 24, 2024

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
Copy link
Member Author

ronso0 commented Sep 24, 2024

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

@ronso0 ronso0 closed this Sep 24, 2024
@ronso0
Copy link
Member Author

ronso0 commented Sep 24, 2024

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
Copy link
Member

ywwg commented Sep 25, 2024

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
Copy link
Contributor

ninomp commented Sep 25, 2024

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
Copy link
Member Author

ronso0 commented Sep 25, 2024

@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
Copy link
Member Author

ronso0 commented Sep 25, 2024

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
Copy link
Member Author

ronso0 commented Sep 26, 2024

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
Copy link
Contributor

ninomp commented Sep 26, 2024

@ronso0 I left a few comments on your PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants