Add option for clearing track's waveform

[ninomp]

This PR fixes https://bugs.launchpad.net/mixxx/+bug/1014449

However, I haven't managed to get this fully functional: clearing waveform doesn't work when a small number of tracks (< 5) are selected, but does work when more tracks are selected. By looking at Mixxx log, it seems to me that this is a caching issue and a small number of changes is not (immediately) written to database. Can someone of the more experienced Mixxx developers please help me with this? Thanks in advance.

[daschuer]

Why was that moved to here?
Can it happen that we have to clear a non dirty waveform with DataSize = 0?
When will the data size drop to 0?

[ninomp]

Well, the original idea was that a NULL pointer to waveform (or an empty waveform) mean that waveform should be cleared, but adding a flag to waveform seems to be a better/cleaner/more elegant solution.

[uklotzde]

The TrackDAO has an internal LRU cache of 5 tracks.

[ninomp]

@daschuer, @uklotzde thank you for your help! :-) Although the discussed idea was to add a flag to Waveform, I decided to add a flag to TrackInfoObject instead. Now, I would like to merge this and start working on a fix for https://bugs.launchpad.net/mixxx/+bug/1457746.

[uklotzde]

Same here, no need for locking.

[ninomp]

Thank you @uklotzde for your review. I have addressed your review comments.

[uklotzde]

The code in both AnalyzerWaveform and AnalysisDao for clearing the waveform if requested looks very similar. I would recommend to encapsulate the code from AnalysisDao in a function:

bool AnalysisDao::clearWaveformIfRequested(TrackInfoObject* pTrack)

Then from AnalyzerWaveform::loadStored() you could simply invoke pAnalysisDao->clearWaveformIfRequested(tio) before obtaining the waveform pointers from the TIO.

[ninomp]

Yes, you are right, but I'm planning to implement automatic clear of waveforms, beats and keys of track being loaded if Mixxx detects that audio of the track has changed, so I need a name that will cover both (automatic and manual) clearing. Do you mind if I do this then?

[uklotzde]

Sure :) Thanks for your work, Nino! It's a pleasure to review your commits,
well done.

On 01/11/2016 11:06 PM, Nino MP wrote:

In src/library/dao/analysisdao.cpp
#841 (comment):

@@ -323,14 +323,26 @@ void AnalysisDao::saveTrackAnalyses(TrackInfoObject* pTrack) {
ConstWaveformPointer pWaveform = pTrack->getWaveform();
ConstWaveformPointer pWaveSummary = pTrack->getWaveformSummary();

• TrackId trackId(pTrack->getId());

• // Delete waveform analysis if track was requested to have its waveform cleared.
• if (pTrack->isClearWaveformRequested()) {

Yes, you are right, but I'm planning to implement automatic clear of
waveforms, beats and keys of track being loaded if Mixxx detects that
audio of the track has changed, so I need a name that will cover both
(automatic and manual) clearing. Do you mind if I do this then?

—
Reply to this email directly or view it on GitHub
https://github.com/mixxxdj/mixxx/pull/841/files#r49386413.

[ninomp]

Thank you! 😄 I'm glad you like it.

[daschuer]

Is the waveform still gone if you:
Clear Waveform.
Close Mixxx
Restart Mixxx

[daschuer]

The check has to be done inside the lock.

[uklotzde]

The comparison of the pointers is thread-safe, but the whole set function is
not atomic. If we acquire the lock after the comparison in very rare cases
the member pointer might be modified by another thread in the meantime.

On 01/12/2016 09:50 AM, Daniel Schürmann wrote:

In src/trackinfoobject.cpp
#841 (comment):

}

void TrackInfoObject::setWaveform(ConstWaveformPointer pWaveform) {

• m_waveform = pWaveform;
• if (m_pWaveform == pWaveform) {

The check has to be done inside the lock.

—
Reply to this email directly or view it on GitHub
https://github.com/mixxxdj/mixxx/pull/841/files#r49427732.

[daschuer]

I am not sure if we have solved all theoretically race conditions with:

ConstWaveformPointer m_pWaveform;
ConstWaveformPointer m_pWaveformSummary;
bool m_bClearWaveformRequested;

I have not understand the code entirely, but I can think of a lot of artificial races between these values above. In the current solution you can write and read all these values from all threads at any time.
What should for instance happen if m_pWaveform is set while m_bClearWaveformRequested is already set as well?

What are the cases we have to consider?

By the way: A bool can be written in on CPU cycle, so there is no need for a lock. However in other places we have uses an QAtomicInt for this. This makes sure that all reviewers knows that the atomic nature is important for the code.

[uklotzde]

I wanted to propose a single function for setting all those members at once.
But I then thought this would be out of scope and cancelled my already
written comment.

The TIO has lots of issues when setting individual members concurrently from
different threads. Many interactions with the TIO are not atomic The design
shows a very naive approach on handling concurrency issues. Improving this
broken design will take some time ;)

On 01/12/2016 10:09 AM, Daniel Schürmann wrote:

I am not sure if we have solved all theoretically race conditions with:

ConstWaveformPointer m_pWaveform;
ConstWaveformPointer m_pWaveformSummary;
bool m_bClearWaveformRequested;

I have not understand the code entirely, but I can think of a lot of
artificial races between these values above. In the current solution you
can write and read all these values from all threads at any time.
What should for instance happen if m_pWaveform is set while
m_bClearWaveformRequested is already set as well?

What are the cases we have to consider?

By the way: A bool can be written in on CPU cycle, so there is no need for
a lock. However in other places we have uses an QAtomicInt for this. This
makes sure that all reviewers knows that the atomic nature is important
for the code.

—
Reply to this email directly or view it on GitHub
#841 (comment).

[daschuer]

Since most of these issues are probably only theoretically, It would work for me, if we just drop some TODO comments about the issues