[WiP] Track metadata import/export #2336

uklotzde · 2019-10-29T08:18:50Z

Please ignore, out of date. Needs to be rebased on a set of library design changes with various fixes. I'm currently no longer able to maintain all my pending fixes and have put all new development tasks on hold.

This PR introduces a simpler, more robust, and better maintainable mapping or synchronization between track metadata that is both stored in the database and in file tags. The code is still complicated, but it's a step in the right direction. It is also another prerequisite for #2282.

We currently only store a subset of properties in the database. Adding support for new fields in the future is non-trivial as we need to incrementally import the missing properties from file tags and must avoid to overwrite any existing file tags when exporting metadata.

The default value for newly added database fields is supposed to be NULL (= missing). A special case from the past is total tracks that has been initialized with "//". Those properties must be replaced with the corresponding file tags step by step whenever a track is loaded.

When enabling __EXTRA_METADATA__ track objects will also be populated with all properties that are available as file tags but are not (yet) stored in the database. The values stored in the database take precedence over imported file tags.

Reorganization

Remove special case handling of certain properties from TagLib code
Remove special case handling of certain properties from TrackInfo/AlbumInfo/TrackMetadata
Move tag migration code for total tracks from TrackDAO into TrackRecord
Move all code for merging imported file tags into TrackRecord

Import

SoundSourceProxy::updateTrackFromSource() Import and merge file tags when loading track objects. This will also populate all missing fields that are not (yet) stored in the database.

Export

Track::exportMetadata() Import and merge file tags again before exporting metadata. Tags are only written if differences are detected to avoid modifying the corresponding file if not needed.

uklotzde · 2019-11-05T20:07:43Z

Finally, after many unrelated changed in DlgTagFetcher the MSVC 2017 build seems to be successful (although it timed out). I have absolutely no idea why it only failed for this particular branch but succeeded on other branches like #2282 that include the exact same code!?!?

Holzhaus · 2019-11-05T23:01:33Z

Windows works in mysterious ways.

AppVeyor build on Windows with VS 2017 failed.

uklotzde · 2019-11-10T17:48:39Z

Rebased on #2344 to avoid merge conflicts.

daschuer

Thank you.
Is there. Something that especially requires a manual test.

daschuer · 2019-11-08T16:13:25Z

src/library/library.cpp

 void Library::saveEvictedTrack(Track* pTrack) noexcept {
-    // It can produce dangerous signal loops if the track is still
-    // sending signals while being saved!
-    // See: https://bugs.launchpad.net/mixxx/+bug/1365708


Why can this be removed?

The disconnecting code is part of GlobalTrackCache::saveEvictedTrack(). I have amended it with a more appropriate description regarding objects-under-destruction.

I don't experience any issues while saving intermediate modifications in the internal and external collections. Otherwise this would be a design or programming error that needs to be fixed.

daschuer · 2019-11-08T16:20:49Z

src/library/library.cpp

+            SoundSourceProxy::exportTrackMetadataBeforeSaving(pTrack);
+            break;
+        case TrackMetadataExportMode::Deferred:
+            pTrack->markForMetadataExport();


Do we need to set the flag unconditionally?

Yes. If the metadata is saved intermediately for keeping the databases up-to-date we must remember to save the file tags when the track objects goes out of scope. This is required even if the dirty flag is not set again, because no further changes happened after modifications have been saved to the database.

I will add a comment.

daschuer · 2019-11-10T12:30:47Z

src/sources/soundsourceproxy.cpp

+        // discard a users's custom choice! We are using a whitelisting
+        // filter here that explicitly checks all valid preconditions
+        // when it is permissible to update/replace existing cover art.
+        if (((coverInfoOld.source == CoverInfo::UNKNOWN) || (coverInfoOld.source == CoverInfo::GUESSED)) &&


if we invert the logic, we have only USER_SELECTED? I think this would be better to read.
Or a switch case with cases for all values and a debug assert in the default case.

I think switching from a white list to a black list is a fair compromise to reduce the overall complexity:

if (coverInfoOld.source == CoverInfo::USER_SELECTED && coverInfoOld.type == CoverInfo::FILE) { ...skip re-import } else { ...replace cover art }

daschuer · 2019-11-11T16:44:03Z

src/track/albuminfo.h

@@ -33,9 +33,6 @@ class AlbumInfo final {
    AlbumInfo& operator=(AlbumInfo&&) = default;
    AlbumInfo& operator=(const AlbumInfo&) = default;

-    // TODO(XXX): Remove after all new fields have been added to the library
-    void resetUnsupportedValues();


I do not understand the original code and the change. Can you give a hint?

The original implementation relied on null values as a marker. Fields with null values were not exported to prevent overwriting valid file tags with empty, missing fields.

daschuer · 2019-11-11T16:45:41Z

src/track/bpm.h

@@ -25,6 +25,12 @@ class Bpm final {

    // Adjusts floating-point values to match their string representation
    // in file tags to account for rounding errors.
+    // NOTE(2019-02-19, uklotzde): The pre-normalization cannot prevent


Is this a to-do?

No, this is a fact resulting from the ID3v2 format.

uklotzde · 2019-11-11T17:34:20Z

Thanks for taking a look, Daniel. Unfortunately I have to rebase this work on a library redesign that fixes a crash on shutdown and insufficient synchronization of external track collections caused by inapproiate dependencies. I'm only able to maintain a single branch of changes.

Be-ing · 2019-11-11T22:45:10Z

Maybe it would be easier to maintain one Aoide prerequisites branch than maintain several interdependent branches?

uklotzde · 2019-11-11T23:27:02Z

That's what I plan to prepare. But it becomes harder and harder to maintain and without any commitment from the team I feel unable to continue.

daschuer · 2019-11-11T23:55:56Z

Does the "Close and comment" button strikes in?

uklotzde · 2019-11-12T06:34:07Z

No. This has intentionally been closed. I'm trying to get rid of maintenance burden.

daschuer · 2019-11-12T07:40:07Z

That's s pity. You spend a significant time to work on this PR and I have spend significant time to review the PR. IMHO it is a good way forward to split you work into small PRs.

I can imagine that rebasing is a lot of risky work, because it creates many untested intermediate commits that likely all conflicting.

So why not just merge you development branch?

uklotzde · 2019-11-12T08:12:43Z

I've tried to split off some smaller PRs, but that just got not reviewed. I cannot keep track of multiple concurrent changes with conflicts due to our legacy architecture.

uklotzde · 2019-11-12T08:15:02Z

I have preserved the changes of your review by modifying or commenting the code accordingly. But without a clear commitment for progress I'm not able to continue.

daschuer · 2019-11-12T09:41:42Z

What kind of commitment you are looking for?

Holzhaus · 2019-11-12T15:46:33Z

Can you restore the branch? I'd review it ASAP ;-)

uklotzde added the library label Oct 29, 2019

uklotzde added this to the 2.3.0 milestone Oct 29, 2019

uklotzde added 10 commits November 10, 2019 14:18

Get or add single tracks via TrackCollection

05c46fa

Hide tracks via TrackCollection

3ceb8d2

Reduce visibility of member functions in TrackDAO

0955754

Reorder member functions to avoid merge conflicts

8c6b3d3

Import and merge missing track metadata from files

e469d78

Log old/new metadata when exporting file tags

49a66dd

Move export of track metadata from TrackCollection into Library

f1edc97

Fix naming conflict(?) and minor issues in DlgTagFetcher

88c8eb4

AppVeyor build on Windows with VS 2017 failed.

Delete obsolete member

0536a37

Delete undefined member function

3083206

uklotzde changed the title ~~Track metadata import/export~~ [WiP] Track metadata import/export Nov 10, 2019

daschuer mentioned this pull request Nov 11, 2019

Refactoring: Attach WTrackTableView to Library instead of TrackCollection #2343

Closed

daschuer reviewed Nov 11, 2019

View reviewed changes

uklotzde closed this Nov 11, 2019

uklotzde deleted the metadata_import_export branch November 12, 2019 06:29

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WiP] Track metadata import/export #2336

[WiP] Track metadata import/export #2336

uklotzde commented Oct 29, 2019 •

edited

uklotzde commented Nov 5, 2019

Holzhaus commented Nov 5, 2019

uklotzde commented Nov 10, 2019

daschuer left a comment

daschuer Nov 8, 2019

uklotzde Nov 11, 2019

daschuer Nov 8, 2019

uklotzde Nov 11, 2019

uklotzde Nov 11, 2019

daschuer Nov 10, 2019

uklotzde Nov 11, 2019

daschuer Nov 11, 2019

uklotzde Nov 11, 2019

daschuer Nov 11, 2019

uklotzde Nov 11, 2019

uklotzde commented Nov 11, 2019

Be-ing commented Nov 11, 2019

uklotzde commented Nov 11, 2019

daschuer commented Nov 11, 2019

uklotzde commented Nov 12, 2019

daschuer commented Nov 12, 2019

uklotzde commented Nov 12, 2019

uklotzde commented Nov 12, 2019

daschuer commented Nov 12, 2019

Holzhaus commented Nov 12, 2019

[WiP] Track metadata import/export #2336

[WiP] Track metadata import/export #2336

Conversation

uklotzde commented Oct 29, 2019 • edited

Reorganization

Import

Export

uklotzde commented Nov 5, 2019

Holzhaus commented Nov 5, 2019

uklotzde commented Nov 10, 2019

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

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

uklotzde commented Nov 11, 2019

Be-ing commented Nov 11, 2019

uklotzde commented Nov 11, 2019

daschuer commented Nov 11, 2019

uklotzde commented Nov 12, 2019

daschuer commented Nov 12, 2019

uklotzde commented Nov 12, 2019

uklotzde commented Nov 12, 2019

daschuer commented Nov 12, 2019

Holzhaus commented Nov 12, 2019

uklotzde commented Oct 29, 2019 •

edited