(fix) DlgTrackInfo: prevent wiping metadata when applying twice quickly #12965

ronso0 · 2024-03-13T23:14:20Z

Not sure what exactly is the root cause, but preventing what should be a no-op in Track already in DlgTrackInfo seems to suffice. Can't reproduce anymore.

With my sparse basic c++ knowledge, I suspected the cause is that the track record has been moved to Track (is null in DlgTrackRecord?) when applying again before the Track::changed signal triggered DlgTrackInfo to fetch the new track record.
But tracing that turned out this is not it since now the changed signal is received before trying to save again (maybe it's just delays caused by the new m_pLoadedTrack->getRecord(), idk 🤷‍♂️ )

With symptoms fixed this good enough for 2.4.1 I guess.

Update:

As clarified by @uklotzde the issue was indeed passing the record and the beats to track via std::move.

Fixes #12963

ronso0 · 2024-03-13T23:40:15Z

@zfhrp6 Please test this. Either build from source yoursel or pick a CI build (info is here https://github.com/mixxxdj/mixxx/wiki/Testing).

zfhrp6 · 2024-03-14T15:23:44Z

@ronso0 I've tested the CI build (actions/runs/8273014776). The steps to repro in issue can't reproduce, and repeating Alt+A also can't. 👍

uklotzde · 2024-03-14T21:14:18Z

src/library/dlgtrackinfo.cpp

    // The dialog is updated and repopulated by the Track::changed() signal.
    m_pLoadedTrack->replaceRecord(std::move(m_trackRecord), std::move(m_pBeatsClone));


Sorry, my bad.

Suggested change

// The dialog is updated and repopulated by the Track::changed() signal.

m_pLoadedTrack->replaceRecord(std::move(m_trackRecord), std::move(m_pBeatsClone));

// Update the cached track

//

// If replaceRecord() returns true then both m_trackRecord and m_pBeatsClone

// will be updated by the subsequent Track::changed() signal to keep them

// synchronized with the track. Otherwise the track has not been modified and

// both members must remain valid. Do not use std::move() for passing arguments!

m_pLoadedTrack->replaceRecord(m_trackRecord, m_pBeatsClone);

Without warranty. Consider refactoring the API to prevent such misunderstandings in the future.

Thanks for jumping in!

Okay, so this is the actual fix.
IIUC the no-op test I added above just hides the issue because it's unlikely the record (in DlgTrackInfo) has changed in between two quick save() calls. Correct?

Also, IIUC this

mixxx/src/track/track.cpp

Lines 308 to 310 in e2d4199

if (pOptionalBeats) {

bpmUpdatedFlag = trySetBeatsWhileLocked(std::move(pOptionalBeats));

if (recordUnchanged && !bpmUpdatedFlag) {

would already move the beats even if applying them fails and replaceRecord() returns false?

It only moves out of the argument which is passed by value. No side effects on the outer context.

ronso0 · 2024-03-15T10:25:08Z

src/library/dlgtrackinfo.cpp

+    if (m_trackRecord == m_pLoadedTrack->getRecord() &&
+            m_pBeatsClone == m_pLoadedTrack->getBeats()) {


In case metadata or beats have been changed elsewhere we'd already have updated here as well, so this scheck is safe IMO (i.e. does not undo those changes by re-applying this dialog's (unchanged) record and beats).

Admittedly, since the update is triggered by signals there is a slight chance users revert their own manual changes, but I'd consider that a "user error" because working on metadata in multiple places at once (at inhuman high rate) may inevitably create incosistencies.

Any thoughts?

Maybe an additional dirty flag is useful.
== + dirty -> reverted edit
!= + dirty -> actual edit
!= + !dirty -> outside edit
== + !dirty -> no edit

So we have a fast exit option in case !dirty

ronso0 · 2024-03-28T00:29:46Z

Has someone time to review?

daschuer · 2024-03-28T07:01:47Z

src/library/dlgtrackinfo.cpp

+    if (m_trackRecord == m_pLoadedTrack->getRecord() &&
+            m_pBeatsClone == m_pLoadedTrack->getBeats()) {


m_trackRecord == m_pLoadedTrack->getRecord() is a deep compare, which should be OK, but slow.
m_pBeatsClone == m_pLoadedTrack->getBeats() is a pointer compare, which will probably always false.

replaceRecord() has already a good equality check inside. In terms of avoiding deep copies and compares optimal, I think we need to split replaceRecord(). But I am not sure if it is worth the efforts.
At least is should be renamed to compareAndReplaceRecord()

I'll revert that commit. I don't want to complicate DlgTrackInfo with a dirty flag, this is just a compact, effecient bugfix after all.

The reason I considered this check in DlgTrackInfo is that Track::replaceRecord() does std::move(optionalBeats) too early (before it's clarified if beats can be applied), betas are moved later on in Track::setBeatsWhileLocked() only if applicable.
So beats might be moved even though they're not applied and Track::updated would not be emitted and we might get the same situation with the beats which this PR fixes for the other metadata.

daschuer · 2024-03-28T07:04:55Z

src/library/dlgtrackinfo.cpp

+    if (m_trackRecord == m_pLoadedTrack->getRecord() &&
+            m_pBeatsClone == m_pLoadedTrack->getBeats()) {


Maybe an additional dirty flag is useful.
== + dirty -> reverted edit
!= + dirty -> actual edit
!= + !dirty -> outside edit
== + !dirty -> no edit

So we have a fast exit option in case !dirty

daschuer · 2024-03-28T07:24:28Z

src/library/dlgtrackinfo.cpp

+    // both members must remain valid. Do not use std::move() for passing arguments!
+    // See https://github.com/mixxxdj/mixxx/issues/12963


Suggested change

// both members must remain valid. Do not use std::move() for passing arguments!

// See https://github.com/mixxxdj/mixxx/issues/12963

// both members must remain valid, so not use std::move() for passing arguments!

I don't think that the reference to the bug will help much later.

Well, it describes the symptoms, but I can do that here as well.

I am already fine without it. Then let's keep it.
No need for extra work.

Fixes mixxxdj#12963 Co-authored-by: Uwe Klotz <uwe.klotz@gmail.com>

daschuer

Thank you. LGTM

ronso0 mentioned this pull request Mar 13, 2024

applying track properties quickly twice will delete track metadata #12963

Closed

github-actions bot added the library label Mar 13, 2024

ronso0 force-pushed the dlgtrackinfo-prevent-wipe branch from 2b52841 to f74b726 Compare March 13, 2024 23:31

ronso0 changed the title ~~DlgTrackInfo: prevent wiping metadata when applying twice quickly~~ (fix) DlgTrackInfo: prevent wiping metadata when applying twice quickly Mar 14, 2024

uklotzde reviewed Mar 14, 2024

View reviewed changes

ronso0 marked this pull request as draft March 15, 2024 09:17

ronso0 force-pushed the dlgtrackinfo-prevent-wipe branch from d7de4c4 to c5f92d3 Compare March 15, 2024 09:41

ronso0 marked this pull request as ready for review March 15, 2024 09:55

ronso0 commented Mar 15, 2024

View reviewed changes

ronso0 added this to the 2.4.1 milestone Mar 15, 2024

ronso0 added metadata needs review labels Mar 15, 2024

ronso0 mentioned this pull request Mar 28, 2024

Importing musicbrainz data for ripped WAV files causes track title to disappear #13014

Open

daschuer requested changes Mar 28, 2024

View reviewed changes

ronso0 and others added 4 commits March 28, 2024 14:46

DlgTrackInfo: prevent wipe when quickly applying twice

0fce87a

Fixes mixxxdj#12963 Co-authored-by: Uwe Klotz <uwe.klotz@gmail.com>

Track: std::move beats only in setBeatsWhileLocked()

8278394

Track: remove redundant changed signals when replacing track record

605c118

DlgTrackInfo: fix slotSpinBpmValueChanged() for no track loaded

fec28c6

ronso0 force-pushed the dlgtrackinfo-prevent-wipe branch from c5f92d3 to fec28c6 Compare March 28, 2024 13:51

daschuer approved these changes Mar 28, 2024

View reviewed changes

daschuer merged commit fb16d84 into mixxxdj:2.4 Mar 28, 2024
14 checks passed

ronso0 deleted the dlgtrackinfo-prevent-wipe branch March 28, 2024 18:41

JoergAtGithub removed the needs review label Mar 28, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

(fix) DlgTrackInfo: prevent wiping metadata when applying twice quickly #12965

(fix) DlgTrackInfo: prevent wiping metadata when applying twice quickly #12965

ronso0 commented Mar 13, 2024 •

edited

Loading

ronso0 commented Mar 13, 2024

zfhrp6 commented Mar 14, 2024

uklotzde Mar 14, 2024

ronso0 Mar 15, 2024

ronso0 Mar 15, 2024 •

edited

Loading

uklotzde Mar 15, 2024

ronso0 Mar 15, 2024

daschuer Mar 28, 2024

ronso0 commented Mar 28, 2024

daschuer Mar 28, 2024

ronso0 Mar 28, 2024

daschuer Mar 28, 2024

daschuer Mar 28, 2024

ronso0 Mar 28, 2024

daschuer Mar 28, 2024

daschuer left a comment

		// The dialog is updated and repopulated by the Track::changed() signal.
		m_pLoadedTrack->replaceRecord(std::move(m_trackRecord), std::move(m_pBeatsClone));

-    // The dialog is updated and repopulated by the Track::changed() signal.
-    m_pLoadedTrack->replaceRecord(std::move(m_trackRecord), std::move(m_pBeatsClone));
+    // Update the cached track
+    //
+    // If replaceRecord() returns true then both m_trackRecord and m_pBeatsClone
+    // will be updated by the subsequent Track::changed() signal to keep them
+    // synchronized with the track. Otherwise the track has not been modified and
+    // both members must remain valid. Do not use std::move() for passing arguments!
+    m_pLoadedTrack->replaceRecord(m_trackRecord, m_pBeatsClone);

	if (pOptionalBeats) {
	bpmUpdatedFlag = trySetBeatsWhileLocked(std::move(pOptionalBeats));
	if (recordUnchanged && !bpmUpdatedFlag) {

		if (m_trackRecord == m_pLoadedTrack->getRecord() &&
		m_pBeatsClone == m_pLoadedTrack->getBeats()) {

		// both members must remain valid. Do not use std::move() for passing arguments!
		// See https://github.com/mixxxdj/mixxx/issues/12963

	// both members must remain valid. Do not use std::move() for passing arguments!
	// See https://github.com/mixxxdj/mixxx/issues/12963
	// both members must remain valid, so not use std::move() for passing arguments!

(fix) DlgTrackInfo: prevent wiping metadata when applying twice quickly #12965

(fix) DlgTrackInfo: prevent wiping metadata when applying twice quickly #12965

Conversation

ronso0 commented Mar 13, 2024 • edited Loading

Update:

ronso0 commented Mar 13, 2024

zfhrp6 commented Mar 14, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ronso0 Mar 15, 2024 • 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

ronso0 commented Mar 28, 2024

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 left a comment

Choose a reason for hiding this comment

ronso0 commented Mar 13, 2024 •

edited

Loading

ronso0 Mar 15, 2024 •

edited

Loading