Improve synchronization of track metadata and file tags #2406
Conversation
uklotzde
changed the title
|
I would like to get this into 2.3 to avoid maintaining 2 largely different versions. |
|
Could you share a simple routine to test this? |
|
Even if EXTRA_METADATA is enabled (a prerequisite) the extended properties are not visible in Mixxx. You should not notice any changes, unless you send data to aoide. The improvements are mainly behind the scenes and ensure that newly added fields in the Mixxx database get populated lazily from file tags as expected. I will think about enabling some trace logs to report all properties when loading a track. |
|
Btw we should really add a flag to activate EXTRA_METADATA and enable it for the CI builds so that the code is built and tested on Travis and Appveyor. |
|
@daschuer I agree that the changes compared to the current state are hard to track. It is a major redesig. Currently all the code for synchronizing metadata from the database with file tags is scattered all around. We have special case handling on different levels in TrackDAO and the low-level TagLib code, where it does not belong. |
|
@uklotzde Have a look at this branch: https://github.com/Holzhaus/mixxx/tree/extra-metadata-flag It's untested though, since I'm currently travelling and had to use my phone. Using vim on a touchscreen ain't easy btw ;-) |
|
@Holzhaus I hesitated to add a new feature flag without an actual use case apart from aoide. But we could do this. |
|
The flag would obviously be disabled for release builds, but it would make it easier to test this. How do you test it currently? Do you put a |
- Preserve existing properties - Replace missing extra properties
...instead of with an empty string. Both work, but NULL is more explicit.
|
I found some issues in the way cover art is guessed. Files in the folder were not considered if reloading metadata from source, only embedded covers. The CoverInfo source of all files without embedded cover is reset to UNKNOWN and the LibraryScanner will revisit them on the next run. Not a big deal but annoying. Due to merge conflicts I have not fixed it on master and only in this branch. |
...to consider not only embedded cover art but also image files in the folder of the track's file. Otherwise the source of the cover info would be reset to UNKNOWN.
Removes redundant code out of TrackDAO that doesn't belong there!
|
src/track/bpm.h
Outdated
| // or not. | ||
| // TL;DR: If metadata export is enabled ID3 tags will be rewritten | ||
| // for all files with a fractional bpm values even if their metadata | ||
| // has not been modified. |
There was a problem hiding this comment.
I think this comment need to be updated.
|
Thank you LGTM. I have also done some brief testing, so we can risk to merge this 1k PR |
| @@ -47,4 +49,88 @@ bool TrackRecord::updateGlobalKeyText( | |||
| return false; | |||
| } | |||
|
|
|||
| namespace { | |||
|
|
|||
| void mergeReplayGainMetadataProperty( | |||
There was a problem hiding this comment.
I'm getting a warning now:
/home/jan/Projects/mixxx/src/track/trackrecord.cpp:54:6: Warnung: »void mixxx::{anonymous}::mergeReplayGainMetadataProperty(mixxx::ReplayGain&, const mixxx::ReplayGain&)« definiert, aber nicht verwendet [-Wunused-function]
54 | void mergeReplayGainMetadataProperty(
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
There was a problem hiding this comment.
Oh, damn. An #ifdef is missing. Fix will follow.
This PR introduces a simpler, more robust, and extensible synchronization between track metadata that is stored in the database and file tags. We currently only store a subset of properties in the database.
I have used and tested this code for many weeks and didn't notice any issues or lost file tags. It is also needed for synchronization of tracks with external libraries like #2282 that store an extended set of metadata.
Reorganization
TrackInfo/AlbumInfo/TrackMetadataTrackDAOintoTrackRecordTrackRecordImport
SoundSourceProxy::updateTrackFromSource(): Import and merge file tags when loading track objects on the fly. This will populate all missing fields that are not (yet) stored in the database.Export
Track::exportMetadata(): Import and merge file tags once again before exporting metadata into file tags. We have to reload the file tags anyway to check for differences as we already do now, because files should only be modified if differences are detected. Additional debug logs show both the existing and the modified metadata before writing file tags.