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

[WiP] aoide - External music library #2282

Closed
wants to merge 39 commits into from
Closed

[WiP] aoide - External music library #2282

wants to merge 39 commits into from

Conversation

uklotzde
Copy link
Contributor

@uklotzde uklotzde commented Sep 15, 2019

Only test this PR if you are familiar with how to reset the database schema version in the settings table for reapplying any schema migrations that will precede integration of this foundational feature!!

Just a convenient starting point to demonstrate the current progress. The branch will be rebased on master frequently and the following instructions will be updated as required.

Installation

Mixxx build

Enabled with the feature flag -DAOIDE=ON in the CMake build.

aoide Executable (Rust)

Requires a working version of aoide in the program or settings directory. A statically linked executable (Linux, x86_64) that works out of the box is available on the release page. Otherwise you need to install a Rust development environment and build it with cargo build or cargo build --release yourself.

I recommend placing the executable file in your personal Mixxx settings folder for testing purposes.

Usage

Startup/Shutdown

The backend service is started implicitly as a child process by Mixxx on startup and terminated on exit. Upon the first start it will create a new database aoide.sqlite in the settings directory, right next to mixxxdb.sqlite.

Synchronization

Every save and purge action is propagated from Mixxx to aoide. You won't notice, because this is performed asynchronously in a separate thread. If you play tracks in Mixxx they will appear automatically in aoide after a while, because modifying the play count results in a save action eventually.

To initially populate the aoide database select some tracks in Mixxx and invoke Send to aoide from the metadata context menu. Repeat to enforce a synchronization. Unfortunately there is no progress dialog, so start experimenting with a small number of tracks.

View

The feature in the side pane is empty after startup. You need to load a tree with prepared queries from a file (right click on feature title). An example file is provided in res/preferences/aoide_prepared_queries_example.json.

Copy this file into your settings folder, because this is the default location when opening the file dialog. The syntax and schema of this file is not fixed yet, so don't spend too much work into tuning it. Don't forget to replace your copied version with an updated version from the repo, if necessary.

aoide

This is just a dumb, read-only view and all track interactions are forwarded to Mixxx.

Monitoring & Logging

The Mixxx log level is applied to aoide and all aoide logs are picked up by Mixxx. By setting the log level to debug aoide will generate many logs in your Mixxx log file. This will reveal the progress of batch operations and all the inter-process communication.

Tagging

One of the distinctive features of aoide is the extensible tagging system that allows to support multi-valued fields. Artists, titles, and some other fields are stored as predefined properties, but anything else like genres, comments, grouping, lyrics, ... must be mapped to custom tags.

Genre tags

Mixxx only supports a single genre per track. But you could encode multiple genres into this field by using a separator like " - " (space dash space): "Pop - Rock".

In aoide the genre is stored in a dedicated tag with the facet "genre". This is just a convention. Multiple tags with different labels like "Pop" and "Rock" can be assigned to a track. The order of the genres is determined by their score between 0.0 and 1.0. If missing the implicit default score for any tag is 1.0.

The export from Mixxx to aoide splits the genre field into separate tags by using a customizable separator and an attenuation value for assigning a diminishing score to sub-genres. The default values are not stored in the Mixxx configuration file, but they would look like in this example:

[aoide]
multiGenreSeparator " - "
multiGenreAttenuation 0.75

The separator is enclosed in single ('...') or double ("...") quotes to avoid that leading and trailing whitespace gets cut off when editing the .cfg file. The 1st (= main) genre gets a score of 0.75^0 = 1, the 2nd genre gets a score of 0.75^1 = 0.75, the 3rd genre a score of 0.75^2 = 0.5625 and so on.

In our simple example the following tags would be exported to aoide:

  • facet = "genre", label = "Pop", score = 1.0
  • facet = "genre", label = "Rock", score = 0.75

When receiving tracks from aoide both the separator and the assigned score is used to restore the single-valued genre field for the view. The genre tags are sorted in descending order by their score and then their labels are joined with the separator from the configuration.

The MultiGenreTagger implements this bidirectional mapping.

Side effects

None ...as far as I'm aware! You can safely switch back to master after testing any time you want. The Mixxx database is not affected in any way.

You may need or want to delete the aoide SQLite database from time to time. I will not provide any schema migrations until v1.0 has been released. Use your Mixxx library as the source for repopulating the aoide library from scratch.

Path forward

Some extensions need to be merged first. After all preliminary PRs have been merged this PR should finally only contain commits that add the actual integration.

@uklotzde uklotzde changed the title [WiP] aoide Preview [WiP] aoide Preview (Experimental) Sep 16, 2019
@daschuer
Copy link
Member

Thank you very much for all the love and time you put into this. It looks very promising. :-)

Skimming through the code the use of QPointer jumps up.

Qt docs:

QPointers are useful whenever you need to store a pointer to a QObject that is owned by someone else, and therefore might be destroyed while you still hold a reference to it. You can safely test the pointer for validity.

Agent() for example holds Subsystem as QPointer which is correct because it is a Pointer owned by someone else. Reading only the Agent() code, the it tells me QPointer can become nullptr at any time, so I would expect to use null checks for every access.

In this case it we can omit the checks because the calling code guarantees that Subsystem lives longer.
QPointer implies the opposite. Unfortunately we can also not use a plain pointer, because with this, the function grantees to not store the pointer which is also not true.

So I think we have a gap here between QPointer<T> parented_ptr::toWeakRef() and T* parented_ptr::get() we should close.

https://en.cppreference.com/w/cpp/experimental/observer_ptr could be an alternative for our purpose but the lifetime is also not documented.

gsl::not_null<T*> can also be used ...
https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#Rf-nullptr

But how about create an own pointer type for documentation purpose:
outliving_ptr

@uklotzde
Copy link
Contributor Author

@daschuer I've deliberately used QPointer instead of plain or other managed pointers to quickly detect any lifetime conflicts between Qt-managed objects. A crash by dereferencing a nullptr is the most reliable way to reveal those dependencies. QPointer guarantees that we don't end up with dangling pointers.

This behavior can be tuned and adjusted after the design of all subsystem components and their relationships have been finalized.

@daschuer
Copy link
Member

Yes, right, during debugging a crash from object de-referenced from a nullptr is more easy to find than a crash from a dangling pointer. Unfortunately it is still a crash and a QPointer does not come for free, it installs the reference counting calling potentially locking new().

These crashes are a result from a misuse of pointers, probably caused by misinterpreting existing code.
The idea was to prevent this before the crash happens by unambiguous define the live time of the referenced Object. For me QPointer does the opposite.

If we can effort the reference counting overhead, we may just inherit a outliving_ptr from QPointer for that purpose. But that should not be necessary because In many cases it can be easily guaranteed that the outliving_ptr guarantee is valid. If this is not obvious, QPointer with manual null check can be used. or a parent-less QObject managed by a QSharedPointer.

Other Ideas?

@uklotzde
Copy link
Contributor Author

I have reduced the usage of QPointer to unowned members. It works exactly as documented and intended, i.e. an implicit weak pointer.

m_trackCollection(trackCollection),
m_trackLoader(new AsyncTrackLoader(trackCollection, this)),
m_subsystem(new Subsystem(userSettings, m_trackLoader, this)),
m_agent(new Agent(m_subsystem, this)) {
Copy link
Member

Choose a reason for hiding this comment

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

Can we remove that whole Agent?
For me it just makes the code harder to read due to the signal slot connections and it is a stateless class anyway.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No. The agent coordinates the selection of an active collection without user interaction. Signals are needed because the whole process is asynchronous.

Copy link
Member

Choose a reason for hiding this comment

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

I don't get that from the code.
It looks like as if it is possible to inline all agent code. Is there a worker thread involved? Where is it?

Copy link
Contributor Author

@uklotzde uklotzde Oct 18, 2019

Choose a reason for hiding this comment

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

I've added a comment to Agent. All child components of TrackCollection (Agent, Subsystem, AsyncTrackLoader) are running on the UI event loop as expected.

The worker thread and all its components are controlled internally by Subsystem that acts as a facade. This class also handles startup and shutdown of the child process. The name has been chosen deliberately.

switch (cue.getType()) {
case Cue::CUE:
case Cue::LOAD:
setStart(cue.getPosition() * cuePositionToMillis);
Copy link
Member

Choose a reason for hiding this comment

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

It looks like a getPositionMillis() will streamline the code here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not possible, because unfortunately cue position and length are measured in sample offsets, assuming that channel samples are interleaved. The latter is one of the main design flaws that complicates the engine code in so many places. Moreover, these cue properties depend on the context, namely the sample rate and channel count of the PCM signal.

cuePositionToMillis = 1000.0 / (sampleRate * channelCount)

aoide fixes this by storing the values in plain time units, independent from the properties of the underlying PCM signal.

Copy link
Member

Choose a reason for hiding this comment

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

Ha, my bad. Can you explain this inline.

Copy link
Contributor Author

@uklotzde uklotzde Oct 18, 2019

Choose a reason for hiding this comment

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

I have added a comment in the TrackExporter where this value is calculated. Cue points of tracks with an invalid sample rate or number of channels cannot be exported. This only occurs for corrupt or unsupported files.

class AoidePositionMarker : public AoideJsonObject {
public:
AoidePositionMarker(const Cue& cue, double cuePositionToMillis);
explicit AoidePositionMarker(QJsonObject jsonObject = QJsonObject())
Copy link
Member

Choose a reason for hiding this comment

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

I think it would be better to name the file like the class

@uklotzde
Copy link
Contributor Author

I've improved the timeout and error handling so that network request processing doesn't get stuck. This is essential, because write requests are queued and processed sequentially to guarantee that all modifications are applied in order.

@uklotzde
Copy link
Contributor Author

Mixxx now saves the file path with the last tree view configuration that has been loaded and automatically restores it on startup. No need to store this information in the database.

@uklotzde
Copy link
Contributor Author

v0.5.0 now actively prevents adding different tracks with the same URIs into a single collection. This should help to detect bugs in the Mixxx external collection synchronization code like those fixed by #2404.

@Be-ing
Copy link
Contributor

Be-ing commented Apr 11, 2020

You need to load a tree with prepared queries from a file (right click on feature title). An example file is provided in res/aoide/example_prepared_queries.json.

Please update the first post. I got confused because you have moved this JSON file to res/preferences.

@uklotzde
Copy link
Contributor Author

You need to load a tree with prepared queries from a file (right click on feature title). An example file is provided in res/aoide/example_prepared_queries.json.

Please update the first post. I got confused because you have moved this JSON file to res/preferences.

Yes, sorry, forgot about it. After the custom tags are also loaded from a file i have move both into a common place.

@uklotzde
Copy link
Contributor Author

uklotzde commented Apr 11, 2020

@Be-ing I have also deleted the "Custom tags" section. This does no longer apply after I removed the hand-written pseudo-parse. This will be replaced by proper custom tags support in Mixxx soon that is be compatible with a aoide tagging scheme.

@Be-ing
Copy link
Contributor

Be-ing commented Apr 11, 2020

The feature in the side pane is empty after startup. You need to load a tree with prepared queries from a file (right click on feature title).

This was quite confusing to me at first. I expected clicking "aoide" in the tree to show all tracks in Aoide, just like clicking on Tracks.

@Be-ing Be-ing marked this pull request as draft April 11, 2020 13:26
@uklotzde
Copy link
Contributor Author

uklotzde commented Apr 11, 2020

The feature in the side pane is empty after startup. You need to load a tree with prepared queries from a file (right click on feature title).

This was quite confusing to me at first. I expected clicking "aoide" in the tree to show all tracks in Aoide, just like clicking on Tracks.

Showing all tracks without any filtering and sorting doesn't make sense. It would display the first arbitrary 250 tracks. We have to drop this "load the whole database into memory" concept, that doesn't fit here.

Sorting should be configurable independent of the view. Example: In Mixxx I perfer to have the main view ordered by album and then by date added in descending order, i.e. to show the new tracks. This always requires 3 clicks on table columns (interrupted by long UI pauses to finish the "load the whole database task" although I discard 2 of them immediately) if I need to restore this view, very cumbersome. In the aoide view this is just a prepared query that I can save and recall whenever I need it.

@Be-ing
Copy link
Contributor

Be-ing commented Apr 11, 2020

Showing all tracks without any filtering and sorting doesn't make sense.

Yes, showing everything with no filtering nor sorting makes no sense. But we need a default query with no filtering and only sorting for the user to scroll through everything. I don't care much what the default sort order is.

@uklotzde
Copy link
Contributor Author

Showing all tracks without any filtering and sorting doesn't make sense.

Yes, showing everything with no filtering nor sorting makes no sense. But we need a default query with no filtering and only sorting for the user to scroll through everything. I don't care much what the default sort order is.

The "Recently Added" query is ideal for this purpose. In conjunction with "Recently Modified" this could be provided as a built-in default that does not need to be loaded on a fresh install. We could either bundle or generate a default configuration for this purpose.

Is suggest that the main view is reserved for an overview of the feature, not for displaying tracks. It could include quick links and diagnostic/statistical information.

@Be-ing
Copy link
Contributor

Be-ing commented Apr 11, 2020

This branch currently does not build:

/home/be/sw/mixxx/src/track/trackrecord.cpp:108:19: error: use of undeclared identifier 'mergedTrackInfo'
    copyIfNotNull(mergedTrackInfo.refConductor(), importedTrackInfo.getConductor());
                  ^
/home/be/sw/mixxx/src/track/trackrecord.cpp:109:19: error: use of undeclared identifier 'mergedTrackInfo'
    copyIfNotNull(mergedTrackInfo.refDiscNumber(), importedTrackInfo.getDiscNumber());
                  ^
/home/be/sw/mixxx/src/track/trackrecord.cpp:110:19: error: use of undeclared identifier 'mergedTrackInfo'
    copyIfNotNull(mergedTrackInfo.refDiscTotal(), importedTrackInfo.getDiscTotal());
                  ^
/home/be/sw/mixxx/src/track/trackrecord.cpp:111:19: error: use of undeclared identifier 'mergedTrackInfo'
    copyIfNotNull(mergedTrackInfo.refEncoder(), importedTrackInfo.getEncoder());
                  ^
/home/be/sw/mixxx/src/track/trackrecord.cpp:112:19: error: use of undeclared identifier 'mergedTrackInfo'
    copyIfNotNull(mergedTrackInfo.refEncoderSettings(), importedTrackInfo.getEncoderSettings());
                  ^
/home/be/sw/mixxx/src/track/trackrecord.cpp:113:19: error: use of undeclared identifier 'mergedTrackInfo'
    copyIfNotNull(mergedTrackInfo.refISRC(), importedTrackInfo.getISRC());
                  ^
/home/be/sw/mixxx/src/track/trackrecord.cpp:114:19: error: use of undeclared identifier 'mergedTrackInfo'
    copyIfNotNull(mergedTrackInfo.refLanguage(), importedTrackInfo.getLanguage());
                  ^
/home/be/sw/mixxx/src/track/trackrecord.cpp:115:19: error: use of undeclared identifier 'mergedTrackInfo'
    copyIfNotNull(mergedTrackInfo.refLyricist(), importedTrackInfo.getLyricist());
                  ^
/home/be/sw/mixxx/src/track/trackrecord.cpp:116:19: error: use of undeclared identifier 'mergedTrackInfo'
    copyIfNotNull(mergedTrackInfo.refMood(), importedTrackInfo.getMood());
                  ^
/home/be/sw/mixxx/src/track/trackrecord.cpp:117:19: error: use of undeclared identifier 'mergedTrackInfo'
    copyIfNotNull(mergedTrackInfo.refMovement(), importedTrackInfo.getMovement());
                  ^
/home/be/sw/mixxx/src/track/trackrecord.cpp:118:19: error: use of undeclared identifier 'mergedTrackInfo'
    copyIfNotNull(mergedTrackInfo.refMusicBrainzArtistId(), importedTrackInfo.getMusicBrainzArtistId());
                  ^
/home/be/sw/mixxx/src/track/trackrecord.cpp:119:19: error: use of undeclared identifier 'mergedTrackInfo'
    copyIfNotNull(mergedTrackInfo.refMusicBrainzRecordingId(), importedTrackInfo.getMusicBrainzRecordingId());
                  ^
/home/be/sw/mixxx/src/track/trackrecord.cpp:120:19: error: use of undeclared identifier 'mergedTrackInfo'
    copyIfNotNull(mergedTrackInfo.refMusicBrainzReleaseId(), importedTrackInfo.getMusicBrainzReleaseId());
                  ^
/home/be/sw/mixxx/src/track/trackrecord.cpp:121:19: error: use of undeclared identifier 'mergedTrackInfo'
    copyIfNotNull(mergedTrackInfo.refMusicBrainzWorkId(), importedTrackInfo.getMusicBrainzWorkId());
                  ^
/home/be/sw/mixxx/src/track/trackrecord.cpp:122:19: error: use of undeclared identifier 'mergedTrackInfo'
    copyIfNotNull(mergedTrackInfo.refRemixer(), importedTrackInfo.getRemixer());
                  ^
/home/be/sw/mixxx/src/track/trackrecord.cpp:123:20: error: use of undeclared identifier 'mergedTrackInfo'
    copyIfNotEmpty(mergedTrackInfo.refSeratoTags(), importedTrackInfo.getSeratoTags());
                   ^
/home/be/sw/mixxx/src/track/trackrecord.cpp:124:19: error: use of undeclared identifier 'mergedTrackInfo'
    copyIfNotNull(mergedTrackInfo.refSubtitle(), importedTrackInfo.getSubtitle());
                  ^
/home/be/sw/mixxx/src/track/trackrecord.cpp:125:19: error: use of undeclared identifier 'mergedTrackInfo'
    copyIfNotNull(mergedTrackInfo.refWork(), importedTrackInfo.getWork());
                  ^
18 errors generated.
gmake[2]: *** [CMakeFiles/mixxx-lib.dir/build.make:4955: CMakeFiles/mixxx-lib.dir/src/track/trackrecord.cpp.o] Error 1
gmake[2]: *** Waiting for unfinished jobs....
gmake[1]: *** [CMakeFiles/Makefile2:1562: CMakeFiles/mixxx-lib.dir/all] Error 2
gmake: *** [Makefile:180: all] Error 2

When loading a track from the database check if the underlying file
has been modified. If the time stamp is newer than the timestamp of
the last synchronization then update track metadata from file tags.

But only if export of track metadata into file tags has been enabled
by the user to reduce surprising side-effects to a minimum. Introducing
a new configuration option for this feature has deliberately been avoided
to reduce the number of possible configurations. Either you want to
keep your file tags synchronized or not, no exceptions!

# Conflicts:
#	src/sources/metadatasourcetaglib.cpp
Only tag labels are filtered, facets and scores are ignored. The
resulting subselect (if not empty) is added to the main query with
OR if the query contains no tag-specific search terms. If the query
contains at least one token "label:<text>" then AND will be used.
@uklotzde
Copy link
Contributor Author

Closing due to lack of motivation. Please do not reopen!

@uklotzde uklotzde closed this Sep 27, 2021
@afalout
Copy link

afalout commented Sep 28, 2021

Such a shame to give up after a great effort. I had high hopes for this very much needed functionality. Is there anything that can be done to help you continue this work?
Thanks, Andrej

@uklotzde
Copy link
Contributor Author

I will maintain a stripped down Mixxx integration layer for personal use. But I won't sink more time into the Mixxx integration.

@ronso0
Copy link
Member

ronso0 commented Oct 3, 2021

:( what a pity
Admittedly I didn't bother testing this because I'm served well with my abuse of comments, and don't use crates anymore.
Nevertheless, I appreciated your continuous work, hoping this could be the foundation of a new library design (especially the search part).

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.

8 participants