Import metadata and cover image without temporary track object

[uklotzde]

The responsibilities of and interactions between TrackPointer/SoundSourceProxy/GlobalTrackCache are still confusing. This PR tries to improve the situation slightly by aligning the member functions in SoundSourceProxy that serve similar purposes.

Added:

• SoundSourceProxy::importTrackMetadataAndCoverImageFromFile() (as a static companion of importTrackMetadataAndCoverImage())

Removed:

• SoundSourceProxy::importTemporaryTrack()
• SoundSourceProxy::importTemporaryCoverImage()

This helps to separate responsibilities and is another prerequisite for #2656.

[uklotzde]

Ok, ready. Only a simple refactoring for the next steps.

[Holzhaus]

LGTM, thank you.

[JoergAtGithub]

@m0dB found out, that the section
https://github.com/mixxxdj/mixxx/blame/dafa98eb4298a902bc94e29f1ec0ad391488def9/src/sources/soundsourceproxy.cpp#L557-L569
seems to the orgin for bug #11131.
When this section is replaced by just

pTrack = Track::newTemporary(std::move(trackFileAccess));
GlobalTrackCacheLocker locker; 

the CoverArt in the spinnies loads reliable as on 2.3.

@uklotzde Since neither me nor m0dB are familar with this code, could you as original author have a look on the proposed fix.

[uklotzde]

The code comments are about writing metadata. Simply deleting this code without being able to explain why it is not needed might have unintended side effects. Like removing a Mutex from multi-threaded code will resolve a deadlock.

Sorry, but I am no longer working on Mixxx and don't have plans to continue. If you think removing this section is a good idea, please do so. I could keep it in my fork to ensure my metadata is not messed up in unexpected ways.

[uklotzde]

Please also consider that this code is used by many other components, but only the spinnies don't seem to work as expected. That's suspicious.

[m0dB]

Hi @uklotzde , sorry to hear you will not be working on mixxx anymore.

I agree with your assessment about the comment on writing metadata, but I believe the problem is actually the unlocking when it is not safe yet:

    if (pTrack) {
        // We can safely unlock the cache if the track object is already cached.
        locker.unlockCache();

So if any, removing this unlock will make the code safer, not less safe.

I have been comparing the 2.3 and 2.4 code of the code path involved with the cover art and this was the only thing that stood out.

It would be great if @JoergAtGithub could compare builds before and after your change but I don't know how easy it is to create these builds.

[uklotzde]

But unconditionally using a temporary instead of the shared, mutable data might lead to unexpected inconsistencies. The problem is that this function seems to be used in different contexts. Importing a cover art image is only one of them.

This is what the proposed change would do. The code for keeping the cache locked would look different. I have no clue about the implications.

[daschuer]

The Lock ensures that only a single track object for each lives in Mixxx. This is to avoid concurrent changes and a shift of audio data when metadata is inserted at the beginning.

If we use a temporary track object, we may read the data that is written by an already cached track object.
That's why we need to keep the lock. The original comment can be moved and rephrased to be more clear.

When we hold the shared track pointer from the cache, the track is also not written. This happens only after the last instance goes out of scope.

In both cases a new Soundsource is created form the file path. So I have no idea why using a temporary track fixes the bug.

[m0dB]

So you are suggesting that only removing the early explicit unlock() would be sufficient? Can you try that, @JoergAtGithub ?

[daschuer]

So you are suggesting that only removing the early explicit unlock() would be sufficient?

This should not change anything. My guess is that somewhere else is a race condition and this change here changes only the timing that it is less likely.

[uklotzde]

So you are suggesting that only removing the early explicit unlock() would be sufficient?

This should not change anything. My guess is that somewhere else is a race condition and this change here changes only the timing that it is less likely.

...So everyone else will be suffering by a workaround for a bug in the spinny code. Unlocking the cache once you get hold of the pointer should be safe. It has nothing to do with the actual import of the metadata. By holding the lock during the file I/O the whole import process will stall the UI elsewhere.

[uklotzde]

The synchronization of the cache is not supposed to control synchronization in different contexts. I am not supporting such a change. But I am not supposed to make decisions.

[m0dB]

But if you look at Joerg's bugreport, everything points at something going wrong at this level, not at the spinny level:

info [Thread (pooled)] MetadataSourceTagLib - No track metadata or cover art found in file "C:/Users/Joerg.WORLDWARTWEB/Music/Der Dritte Raum - Hale Bopp (original mix).mp3" with type 3
debug [Thread (pooled)] SoundSourceProxy - SoundSourceProvider "MAD: MPEG Audio Decoder" created a SoundSource for file "file:///C:/Users/Joerg.WORLDWARTWEB/Music/Der Dritte Raum - Hale Bopp (original mix).mp3" of type "mp3"
debug [AnalyzerThread 0 #1] AnalyzerThread - Dequeued next track 140
debug [Thread (pooled)] SoundSourceProxy - SoundSourceProvider "MAD: MPEG Audio Decoder" created a SoundSource for file "file:///C:/Users/Joerg.WORLDWARTWEB/Music/Der Dritte Raum - Hale Bopp (original mix).mp3" of type "mp3"
warning [Thread (pooled)] TagLib - Cannot read audio properties from inaccessible/unreadable/invalid file:
debug [AnalyzerThread 0 #1] AnalyzerThread - Analyzing QFileInfo(C:\Users\Joerg.WORLDWARTWEB\Music\Der Dritte Raum - Hale Bopp (original mix).mp3)
info [Thread (pooled)] MetadataSourceTagLib - No track metadata or cover art found in file "C:/Users/Joerg.WORLDWARTWEB/Music/Der Dritte Raum - Hale Bopp (original mix).mp3" with type 3
warning [Thread (pooled)] TagLib - Cannot read audio properties from inaccessible/unreadable/invalid file:
info [Thread (pooled)] MetadataSourceTagLib

And it interesting at least that not unlocking the cache solves the issue. Of course it can be a timing issue and a bug in the spinny code that only got exposed after this change. In that case, I hope that having found this can help in tracing that down. But I think we are a bit at a loss.

[uklotzde]

I only see logs from the TagLib code complaining about being unable to access the file itself. Why and how is the cache involved here?

[uklotzde]

Changing code by trial and error without understanding the root cause is harmful.

[m0dB]

Yes, but unfortunately I am not able to reproduce the issue myself so I can't debug this and the only thing I could think of to be helpful was comparing the code that did and didn't work.