MusicBrainz: Extended metadata support (pre-gsoc2020) #2522

uklotzde · 2020-02-27T17:17:56Z

https://bugs.launchpad.net/mixxx/+bug/1581256
https://www.mixxx.org/wiki/doku.php/gsoc2020ideas

    QUuid artistId;
    QUuid albumArtistId;
    QUuid albumReleaseId;
    QUuid recordingId;
    QUuid releaseGroupId;
    QUuid trackReleaseId;

    QString title;
    QString artist;
    QString albumTitle;
    QString albumArtist;
    QString releaseGroupType;
    QString mediumFormat;
    QString trackNumber;
    QString trackTotal;
    QString discNumber;
    QString discTotal;
    QString date;

Fetch and store additional metadata from MusicBrainz
New MusicBrainz XML parser
- the existing parser was really poor with no proper error handling
New networking micro-framework
- thin wrappers and utilities for QNetwork stuff
- adopted from and battle-tested in [WiP] aoide - External music library #2282

This branch has undergone multiple transformations with intermediate steps since then. In order to get rid of obsolete code while moving to the new networking framework I had to recombine commits into larger chunks.

Holzhaus · 2020-02-28T18:50:52Z

https://www.mixxx.org/wiki/doku.php/gsoc2020ideas

Are you planning to apply? 😆

uklotzde · 2020-02-28T19:22:13Z

https://www.mixxx.org/wiki/doku.php/gsoc2020ideas

Are you planning to apply? laughing

;) I have completed most of the work some time ago, but these changes have not yet been published. It's a prerequisite in case someone wants to continue working on extended topics, other than importing metadata.

The missing step was to rewrite the HTTP clients for AcoustID and MusicBrainz to reuse the recently finished infrastructure that is used for aoide. It is far more complicated and inconvenient than even legacy async Rust (i.e. with futures 0.1 from 2018/2019), but at least it allows to chain multiple requests.

uklotzde · 2020-02-28T19:23:25Z

Now the title should no longer be misleading.

uklotzde · 2020-02-29T12:11:42Z

Also considered ready for 2.3.0.

The improvements around handling of network requests and of the XML parser alone are worth to include it. Only some additional fields (like album artist and track numbers) are imported from MusicBrainz if __EXTRA_METADATA__ is not set.

daschuer

Some first comments.

daschuer · 2020-02-27T17:56:54Z

src/library/dlgtagfetcher.cpp

+namespace {
+
+QStringList track2row(const Track& track) {
+    const auto trackNumbers = TrackNumbers::joinStrings(


This reads as if the return value is a QStringList. I think this is not the case so it is not a good use case for auto.

We should also consider to rename the function to makeNumberOfTotalString()
or something more intelligent.

daschuer · 2020-02-27T18:04:28Z

src/library/dlgtagfetcher.cpp

+
+namespace {
+
+QStringList track2row(const Track& track) {


Why 2row? Track2MetadataList.

It look like the underlying issue is, that the resultAvailable signal returning tracks.

It would be easier to understand and is probably more performant if it will directly return a custom track object containing the results (a single row) only. Can this be TrackRelease?

The string list solution here draws the column sorting into the management code which is also subobtimal. I think we should at least name the indexes of the list.

Thinking about all of this ... can't we deduplicate the results earlyer just after receiving them.

A custom deduplicate code, working in the temporary track objects would also be a solution.

Ideas?

I have updated the dialog only as needed. I will not implements further improvements other than passing-through of mixxx::musicbrainz::TrackRelease as suggested.

PS: Performance doesn't matter here. I only changed the signal types, because the status quo of using QObjects for this purpose was wrong.

The dialogue needs to be extended anyway in the future. Whoever will do this can improve the deduplication logic and replace the QStringList. The deduplication currently depends on the displayed columns that are actually displayed and therefore must be done here.

The simplistic deduplication logic before is really displaced and the code is difficult to follow. Now everything is co-located on the display layer were it actually matters. It's not optimal, but without a rationale for deduplication this is the best we could do for now.

daschuer · 2020-02-28T07:16:10Z

src/library/dlgtagfetcher.cpp

+                    importedMetadata.getTrackInfo().getMusicBrainzReleaseId()
+            );
+        }
+        if (!importedMetadata.getAlbumInfo().getMusicBrainzArtistId().isNull()) {


I wonder how the artist Id compared to the string representation. I think we need to handle them at once.
For example if the user changes the Artist, it does not match the string stored for this Id at music brains.
This can be an issue when handling Artist with special characters like Tiësto or P!ink or features like "Lena feat. Nico Santos"
For my feeling it is required to use the consolodated string when assigning the Artist ID.
What do you think?

What about the recording id and the title? I will not make up a custom correlation logic without any foundation.

I can only offer to implement one of the following options:

Overwrite all fields unconditionally

Set single fields one-by-one conditionally (current version)

If someone knows how to correlate the fields in a consistent manner, please do so. I refuse to do it, because I am not able to come up with a solution that is supposed to be "correct".

daschuer · 2020-03-03T09:04:23Z

src/musicbrainz/musicbrainzxml.cpp

+            DEBUG_ASSERT(reader.tokenType() != QXmlStreamReader::EndDocument);
+            DEBUG_ASSERT(reader.tokenType() != QXmlStreamReader::DTD);
+            DEBUG_ASSERT(reader.tokenType() != QXmlStreamReader::EntityReference);
+            // ignore other token types


I think it is Characters, Comment and ProcessingInstructions.

Please name them explicit.

daschuer · 2020-03-04T06:43:21Z

Thank you. I will do some manual test before merge.

Unfortunately CI build fails:

�[01m�[Ksrc/library/dlgtagfetcher.cpp:�[m�[K In member function â€˜�[01m�[Kvoid DlgTagFetcher::apply()�[m�[Kâ€™:
�[01m�[Ksrc/library/dlgtagfetcher.cpp:162:25:�[m�[K �[01;31m�[Kerror: �[m�[Kqualified-id in declaration before â€˜�[01m�[K(�[m�[Kâ€™ token
 void DlgTagFetcher::quit() {
�[01;32m�[K                         ^�[m�[K
�[01m�[Ksrc/library/dlgtagfetcher.cpp:167:37:�[m�[K �[01;31m�[Kerror: �[m�[Kqualified-id in declaration before â€˜�[01m�[K(�[m�[Kâ€™ token
 void DlgTagFetcher::fetchTagProgress(QString text) {
�[01;32m�[K                                     ^�[m�[K

daschuer · 2020-03-04T23:00:07Z

Thank you, it Looks and works good.

uklotzde added 4 commits February 27, 2020 16:26

Add (Json)WebTask framework

540c901

Add AcoustIdLookupTask

745f336

Add MusicBrainzRecordingsTask

c4e5da7

Use web task framework and fetch additional MusicBrainz metadata

daacb90

uklotzde added the library label Feb 27, 2020

uklotzde changed the title ~~MusicBrainz: Extended metadata support~~ MusicBrainz: Extended metadata support (gsoc2020ideas) Feb 28, 2020

Merge branch 'master' into musicbrainz

505322f

uklotzde changed the title ~~MusicBrainz: Extended metadata support (gsoc2020ideas)~~ MusicBrainz: Extended metadata support (pre-gsoc2020) Feb 28, 2020

uklotzde added this to the 2.3.0 milestone Feb 29, 2020

Merge branch 'master' into musicbrainz

b0f5991

daschuer requested changes Mar 3, 2020

View reviewed changes

uklotzde added 3 commits March 4, 2020 01:05

Rename function to join track number strings

631b946

Explicitly ignore XML token types

064993c

Pass-through MusicBrainz data type over signal/slot connection

ea59945

daschuer closed this Mar 4, 2020

uklotzde reopened this Mar 4, 2020

uklotzde added 2 commits March 4, 2020 09:09

Fix placement of curly braces

c369b63

Avoid unnecessary conversion

b805083

daschuer approved these changes Mar 4, 2020

View reviewed changes

daschuer merged commit 1e0a1f5 into mixxxdj:master Mar 4, 2020

uklotzde deleted the musicbrainz branch March 4, 2020 23:50

Holzhaus added this to Done in 2.3 release Mar 14, 2020

mixxxbot mentioned this pull request Aug 22, 2022

Store/save MBIDs from matched MusicBrainz lookups #8549

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MusicBrainz: Extended metadata support (pre-gsoc2020) #2522

MusicBrainz: Extended metadata support (pre-gsoc2020) #2522

uklotzde commented Feb 27, 2020 •

edited

Loading

Holzhaus commented Feb 28, 2020

uklotzde commented Feb 28, 2020 •

edited

Loading

uklotzde commented Feb 28, 2020

uklotzde commented Feb 29, 2020

daschuer left a comment

daschuer Feb 27, 2020

daschuer Feb 27, 2020

uklotzde Mar 4, 2020 •

edited

Loading

uklotzde Mar 4, 2020

uklotzde Mar 4, 2020

daschuer Feb 28, 2020

uklotzde Mar 3, 2020

daschuer Mar 3, 2020

uklotzde Mar 4, 2020

daschuer commented Mar 4, 2020

daschuer commented Mar 4, 2020

MusicBrainz: Extended metadata support (pre-gsoc2020) #2522

MusicBrainz: Extended metadata support (pre-gsoc2020) #2522

Conversation

uklotzde commented Feb 27, 2020 • edited Loading

Holzhaus commented Feb 28, 2020

uklotzde commented Feb 28, 2020 • edited Loading

uklotzde commented Feb 28, 2020

uklotzde commented Feb 29, 2020

daschuer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

uklotzde Mar 4, 2020 • edited Loading

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

daschuer commented Mar 4, 2020

daschuer commented Mar 4, 2020

uklotzde commented Feb 27, 2020 •

edited

Loading

uklotzde commented Feb 28, 2020 •

edited

Loading

uklotzde Mar 4, 2020 •

edited

Loading