Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix waveform concurrency issues. Possible fix for Bug #1383404 #420

Merged
merged 3 commits into from
Dec 4, 2014
Merged

Fix waveform concurrency issues. Possible fix for Bug #1383404 #420

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
4ca69a4
Fix waveform concurrency issues. Potentially fixes Bug #1383404.
rryan Dec 3, 2014
a61a72b
Review comments.
rryan Dec 4, 2014
bad8917
More review comments.
rryan Dec 4, 2014
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
161 changes: 86 additions & 75 deletions src/analyserwaveform.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
#include <QImage>
#include <QtDebug>
#include <QTime>
#include <QMutexLocker>
#include <QtDebug>

#include "analyserwaveform.h"
Expand All @@ -15,10 +14,6 @@

AnalyserWaveform::AnalyserWaveform(ConfigObject<ConfigValue>* pConfig) :
m_skipProcessing(false),
m_waveform(NULL),
m_waveformSummary(NULL),
m_waveformDataSize(0),
m_waveformSummaryDataSize(0),
m_waveformData(NULL),
m_waveformSummaryData(NULL),
m_currentStride(0),
Expand Down Expand Up @@ -60,10 +55,8 @@ bool AnalyserWaveform::initialise(TrackPointer tio, int sampleRate, int totalSam
m_skipProcessing = false;

m_timer->start();
m_waveform = tio->getWaveform();
m_waveformSummary = tio->getWaveformSummary();

if (!m_waveform || !m_waveformSummary || totalSamples == 0) {
if (totalSamples == 0) {
qWarning() << "AnalyserWaveform::initialise - no waveform/waveform summary";
return false;
}
Expand All @@ -72,28 +65,32 @@ bool AnalyserWaveform::initialise(TrackPointer tio, int sampleRate, int totalSam
if (loadStored(tio)) {
m_skipProcessing = true;
} else {
// Now actually initalise the AnalyserWaveform:
QMutexLocker waveformLocker(m_waveform->getMutex());
QMutexLocker waveformSummaryLocker(m_waveformSummary->getMutex());

// Now actually initialize the AnalyserWaveform:
destroyFilters();
resetFilters(tio, sampleRate);

//TODO (vrince) Do we want to expose this as settings or whatever ?
const int mainWaveformSampleRate = 441;
//two visual sample per pixel in full width overview in full hd
const int summaryWaveformSamples = 2*1920;
m_waveform->initalise(sampleRate, totalSamples, mainWaveformSampleRate);
m_waveformSummary->initalise(sampleRate, totalSamples,
mainWaveformSampleRate, summaryWaveformSamples);

m_waveformDataSize = m_waveform->getDataSize();
m_waveformSummaryDataSize = m_waveformSummary->getDataSize();
m_waveform = WaveformPointer(new Waveform(
sampleRate, totalSamples, mainWaveformSampleRate, -1));
m_waveformSummary = WaveformPointer(new Waveform(
sampleRate, totalSamples, mainWaveformSampleRate,
summaryWaveformSamples));

// Now, that the Waveform memory is initialized, we can set set them to
// the TIO. Be aware that other threads of Mixxx can touch them from
// now.
tio->setWaveform(m_waveform);
tio->setWaveformSummary(m_waveformSummary);

m_waveformData = &m_waveform->at(0);
m_waveformSummaryData = &m_waveformSummary->at(0);
m_waveformData = m_waveform->data();
m_waveformSummaryData = m_waveformSummary->data();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should get rid of this by moving the m_waveformSummary->data() Into WaveformStride::store()


m_stride.init(m_waveform->getAudioVisualRatio(), m_waveformSummary->getAudioVisualRatio());
m_stride.init(m_waveform->getAudioVisualRatio(),
m_waveformSummary->getAudioVisualRatio());

m_currentStride = 0;
m_currentSummaryStride = 0;
Expand All @@ -111,17 +108,14 @@ bool AnalyserWaveform::initialise(TrackPointer tio, int sampleRate, int totalSam
}

bool AnalyserWaveform::loadStored(TrackPointer tio) const {
Waveform* waveform = tio->getWaveform();
Waveform* waveformSummary = tio->getWaveformSummary();

if (!waveform || !waveformSummary) {
qWarning() << "AnalyserWaveform::loadStored - no waveform/waveform summary";
return false;
}
ConstWaveformPointer pTrackWaveform = tio->getWaveform();
ConstWaveformPointer pTrackWaveformSummary = tio->getWaveformSummary();
ConstWaveformPointer pLoadedTrackWaveform;
ConstWaveformPointer pLoadedTrackWaveformSummary;

int trackId = tio->getId();
bool missingWaveform = waveform->getDataSize() == 0;
bool missingWavesummary = waveformSummary->getDataSize() == 0;
bool missingWaveform = pTrackWaveform.isNull();
bool missingWavesummary = pTrackWaveformSummary.isNull();

if (trackId != -1 && (missingWaveform || missingWavesummary)) {
QList<AnalysisDao::AnalysisInfo> analyses =
Expand All @@ -135,20 +129,19 @@ bool AnalyserWaveform::loadStored(TrackPointer tio) const {
if (analysis.type == AnalysisDao::TYPE_WAVEFORM) {
vc = WaveformFactory::waveformVersionToVersionClass(analysis.version);
if (missingWaveform && vc == WaveformFactory::VC_USE) {
if (WaveformFactory::updateWaveformFromAnalysis(waveform, analysis)) {
missingWaveform = false;
}
pLoadedTrackWaveform = ConstWaveformPointer(
WaveformFactory::loadWaveformFromAnalysis(analysis));
missingWaveform = false;
} else if (vc != WaveformFactory::VC_KEEP) {
// remove all other Analysis except that one we should keep
m_analysisDao->deleteAnalysis(analysis.analysisId);
}
} if (analysis.type == AnalysisDao::TYPE_WAVESUMMARY) {
vc = WaveformFactory::waveformSummaryVersionToVersionClass(analysis.version);
if (missingWavesummary && vc == WaveformFactory::VC_USE) {
if (WaveformFactory::updateWaveformFromAnalysis(waveformSummary, analysis)) {
tio->waveformSummaryNew();
missingWavesummary = false;
}
pLoadedTrackWaveformSummary = ConstWaveformPointer(
WaveformFactory::loadWaveformFromAnalysis(analysis));
missingWavesummary = false;
} else if (vc != WaveformFactory::VC_KEEP) {
// remove all other Analysis except that one we should keep
m_analysisDao->deleteAnalysis(analysis.analysisId);
Expand All @@ -160,6 +153,12 @@ bool AnalyserWaveform::loadStored(TrackPointer tio) const {
// If we don't need to calculate the waveform/wavesummary, skip.
if (!missingWaveform && !missingWavesummary) {
qDebug() << "AnalyserWaveform::loadStored - Stored waveform loaded";
if (pLoadedTrackWaveform) {
tio->setWaveform(pLoadedTrackWaveform);
}
if (pLoadedTrackWaveformSummary) {
tio->setWaveformSummary(pLoadedTrackWaveformSummary);
}
return true;
}
return false;
Expand Down Expand Up @@ -204,36 +203,36 @@ void AnalyserWaveform::process(const CSAMPLE* buffer, const int bufferLength) {

for (int i = 0; i < bufferLength; i+=2) {
// Take max value, not average of data
CSAMPLE cover[2] = { fabs(buffer[i]), fabs(buffer[i+1]) };
CSAMPLE clow[2] = { fabs(m_buffers[ Low][i]), fabs(m_buffers[ Low][i+1]) };
CSAMPLE cmid[2] = { fabs(m_buffers[ Mid][i]), fabs(m_buffers[ Mid][i+1]) };
CSAMPLE chigh[2] = { fabs(m_buffers[High][i]), fabs(m_buffers[High][i+1]) };
CSAMPLE cover[2] = { fabs(buffer[i]), fabs(buffer[i + 1]) };
CSAMPLE clow[2] = { fabs(m_buffers[Low][i]), fabs(m_buffers[Low][i + 1]) };
CSAMPLE cmid[2] = { fabs(m_buffers[Mid][i]), fabs(m_buffers[Mid][i + 1]) };
CSAMPLE chigh[2] = { fabs(m_buffers[High][i]), fabs(m_buffers[High][i + 1]) };

// This is for if you want to experiment with averaging instead of
// maxing.
// m_stride.m_overallData[Right] += buffer[i]*buffer[i];
// m_stride.m_overallData[ Left] += buffer[i+1]*buffer[i+1];
// m_stride.m_filteredData[Right][ Low] += m_buffers[ Low][i ]*m_buffers[ Low][i];
// m_stride.m_filteredData[ Left][ Low] += m_buffers[ Low][i+1]*m_buffers[ Low][i+1];
// m_stride.m_filteredData[Right][ Mid] += m_buffers[ Mid][i ]*m_buffers[ Mid][i];
// m_stride.m_filteredData[ Left][ Mid] += m_buffers[ Mid][i+1]*m_buffers[ Mid][i+1];
// m_stride.m_filteredData[Right][High] += m_buffers[High][i ]*m_buffers[High][i];
// m_stride.m_filteredData[ Left][High] += m_buffers[High][i+1]*m_buffers[High][i+1];
// m_stride.m_overallData[Left] += buffer[i + 1]*buffer[i + 1];
// m_stride.m_filteredData[Right][Low] += m_buffers[Low][i]*m_buffers[Low][i];
// m_stride.m_filteredData[Left][Low] += m_buffers[Low][i + 1]*m_buffers[Low][i + 1];
// m_stride.m_filteredData[Right][Mid] += m_buffers[Mid][i]*m_buffers[Mid][i];
// m_stride.m_filteredData[Left][Mid] += m_buffers[Mid][i + 1]*m_buffers[Mid][i + 1];
// m_stride.m_filteredData[Right][High] += m_buffers[High][i]*m_buffers[High][i];
// m_stride.m_filteredData[Left][High] += m_buffers[High][i + 1]*m_buffers[High][i + 1];

// Record the max across this stride.
storeIfGreater(&m_stride.m_overallData[Left], cover[Left]);
storeIfGreater(&m_stride.m_overallData[Right], cover[Right]);
storeIfGreater(&m_stride.m_filteredData[ Left][ Low], clow[Left]);
storeIfGreater(&m_stride.m_filteredData[Right][ Low], clow[Right]);
storeIfGreater(&m_stride.m_filteredData[ Left][ Mid], cmid[Left]);
storeIfGreater(&m_stride.m_filteredData[Right][ Mid], cmid[Right]);
storeIfGreater(&m_stride.m_filteredData[ Left][High], chigh[Left]);
storeIfGreater(&m_stride.m_filteredData[Left][Low], clow[Left]);
storeIfGreater(&m_stride.m_filteredData[Right][Low], clow[Right]);
storeIfGreater(&m_stride.m_filteredData[Left][Mid], cmid[Left]);
storeIfGreater(&m_stride.m_filteredData[Right][Mid], cmid[Right]);
storeIfGreater(&m_stride.m_filteredData[Left][High], chigh[Left]);
storeIfGreater(&m_stride.m_filteredData[Right][High], chigh[Right]);

m_stride.m_position++;

if (fmod(m_stride.m_position, m_stride.m_length) < 1) {
if (m_currentStride + ChannelCount > m_waveformDataSize) {
if (m_currentStride + ChannelCount > m_waveform->getDataSize()) {
qWarning() << "AnalyserWaveform::process - currentStride >= waveform size";
return;
}
Expand All @@ -243,7 +242,7 @@ void AnalyserWaveform::process(const CSAMPLE* buffer, const int bufferLength) {
}

if (fmod(m_stride.m_position, m_stride.m_averageLength) < 1) {
if (m_currentSummaryStride + ChannelCount > m_waveformSummaryDataSize) {
if (m_currentSummaryStride + ChannelCount > m_waveformSummary->getDataSize()) {
qWarning() << "AnalyserWaveform::process - current summary stride >= waveform summary size";
return;
}
Expand All @@ -252,13 +251,13 @@ void AnalyserWaveform::process(const CSAMPLE* buffer, const int bufferLength) {
m_waveformSummary->setCompletion(m_currentSummaryStride);

#ifdef TEST_HEAT_MAP
QPointF point(float(m_strideSummary.m_filteredData[Right][High]),
float(m_strideSummary.m_filteredData[Right][ Mid]));
QPointF point(m_stride.m_filteredData[Right][High],
m_stride.m_filteredData[Right][Mid]);

float norm = sqrt(point.x()*point.x() + point.y()*point.y());
point /= norm;

point *= m_strideSummary.m_filteredData[Right][ Low];
point *= m_stride.m_filteredData[Right][Low];
test_heatMap->setPixel(point.toPoint(),0xFF0000FF);
#endif
}
Expand All @@ -274,33 +273,45 @@ void AnalyserWaveform::cleanup(TrackPointer tio) {
return;
}

if (m_waveform) {
m_waveform->reset();
}

if (m_waveformSummary) {
m_waveformSummary->reset();
}
tio->setWaveform(ConstWaveformPointer());
// Since clear() could delete the waveform, clear our pointer to the
// waveform's vector data first.
m_waveformData = NULL;
m_waveform.clear();

tio->setWaveformSummary(ConstWaveformPointer());
// Since clear() could delete the waveform, clear our pointer to the
// waveform's vector data first.
m_waveformSummaryData = NULL;
m_waveformSummary.clear();
}

void AnalyserWaveform::finalise(TrackPointer tio) {
if (m_skipProcessing || m_waveform == NULL || m_waveformSummary == NULL) {
if (m_skipProcessing) {
return;
}

QMutexLocker waveformLocker(m_waveform->getMutex());
// Force completion to waveform size
m_waveform->setCompletion(m_waveform->getDataSize());
m_waveform->setVersion(WaveformFactory::currentWaveformVersion());
m_waveform->setDescription(WaveformFactory::currentWaveformDescription());
waveformLocker.unlock();
if (m_waveform) {
m_waveform->setCompletion(m_waveform->getDataSize());
m_waveform->setVersion(WaveformFactory::currentWaveformVersion());
m_waveform->setDescription(WaveformFactory::currentWaveformDescription());
// Since clear() could delete the waveform, clear our pointer to the
// waveform's vector data first.
m_waveformData = NULL;
m_waveform.clear();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

First set members and then destroy, this deserves a comment.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

will do

}

QMutexLocker waveformSummaryLocker(m_waveformSummary->getMutex());
// Force completion to waveform size
m_waveformSummary->setCompletion(m_waveformSummary->getDataSize());
m_waveformSummary->setVersion(WaveformFactory::currentWaveformSummaryVersion());
m_waveformSummary->setDescription(WaveformFactory::currentWaveformSummaryDescription());
waveformSummaryLocker.unlock();
if (m_waveformSummary) {
m_waveformSummary->setCompletion(m_waveformSummary->getDataSize());
m_waveformSummary->setVersion(WaveformFactory::currentWaveformSummaryVersion());
m_waveformSummary->setDescription(WaveformFactory::currentWaveformSummaryDescription());
// Since clear() could delete the waveform, clear our pointer to the
// waveform's vector data first.
m_waveformSummaryData = NULL;
m_waveformSummary.clear();
}

#ifdef TEST_HEAT_MAP
test_heatMap->save("heatMap.png");
Expand Down
12 changes: 3 additions & 9 deletions src/analyserwaveform.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ inline CSAMPLE scaleSignal(CSAMPLE invalue, FilterIndex index = FilterCount) {
}
}

class WaveformStride {
struct WaveformStride {
inline void init(double samples, double averageSamples) {
m_length = samples;
m_averageLength = averageSamples;
Expand Down Expand Up @@ -122,7 +122,6 @@ class WaveformStride {
}
}

private:
int m_position;
double m_length;
double m_averageLength;
Expand All @@ -136,9 +135,6 @@ class WaveformStride {
float m_averageFilteredData[ChannelCount][FilterCount];

float m_postScaleConversion;

private:
friend class AnalyserWaveform;
};

class AnalyserWaveform : public Analyser {
Expand All @@ -163,10 +159,8 @@ class AnalyserWaveform : public Analyser {
private:
bool m_skipProcessing;

Waveform* m_waveform;
Waveform* m_waveformSummary;
int m_waveformDataSize;
int m_waveformSummaryDataSize;
WaveformPointer m_waveform;
WaveformPointer m_waveformSummary;
WaveformData* m_waveformData;
WaveformData* m_waveformSummaryData;

Expand Down
4 changes: 2 additions & 2 deletions src/library/dao/analysisdao.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -320,8 +320,8 @@ void AnalysisDao::saveTrackAnalyses(TrackInfoObject* pTrack) {
return;
}
const int trackId = pTrack->getId();
Waveform* pWaveform = pTrack->getWaveform();
Waveform* pWaveSummary = pTrack->getWaveformSummary();
ConstWaveformPointer pWaveform = pTrack->getWaveform();
ConstWaveformPointer pWaveSummary = pTrack->getWaveformSummary();

// Don't try to save invalid or non-dirty waveforms.
if (!pWaveform || pWaveform->getDataSize() == 0 || !pWaveform->isDirty() ||
Expand Down
22 changes: 9 additions & 13 deletions src/trackinfoobject.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,8 +48,6 @@ TrackInfoObject::TrackInfoObject(const QString& file,
m_pSecurityToken(pToken.isNull() ? Sandbox::openSecurityToken(
m_fileInfo, true) : pToken),
m_qMutex(QMutex::Recursive),
m_waveform(new Waveform()),
m_waveformSummary(new Waveform()),
m_analyserProgress(-1) {
initialize(parseHeader, parseCoverArt);
}
Expand All @@ -61,16 +59,12 @@ TrackInfoObject::TrackInfoObject(const QFileInfo& fileInfo,
m_pSecurityToken(pToken.isNull() ? Sandbox::openSecurityToken(
m_fileInfo, true) : pToken),
m_qMutex(QMutex::Recursive),
m_waveform(new Waveform()),
m_waveformSummary(new Waveform()),
m_analyserProgress(-1) {
initialize(parseHeader, parseCoverArt);
}

TrackInfoObject::TrackInfoObject(const QDomNode &nodeHeader)
: m_qMutex(QMutex::Recursive),
m_waveform(new Waveform()),
m_waveformSummary(new Waveform()),
m_analyserProgress(-1) {
QString filename = XmlParse::selectNodeQString(nodeHeader, "Filename");
QString location = XmlParse::selectNodeQString(nodeHeader, "Filepath") + "/" + filename;
Expand Down Expand Up @@ -145,9 +139,6 @@ TrackInfoObject::~TrackInfoObject() {
// Notifies TrackDAO and other listeners that this track is about to be
// deleted and should be saved to the database, removed from caches, etc.
emit(deleted(this));

delete m_waveform;
delete m_waveformSummary;
}

void TrackInfoObject::parse(bool parseCoverArt) {
Expand Down Expand Up @@ -788,16 +779,21 @@ QString TrackInfoObject::getURL() {
return m_sURL;
}

Waveform* TrackInfoObject::getWaveform() {
ConstWaveformPointer TrackInfoObject::getWaveform() {
return m_waveform;
}

Waveform* TrackInfoObject::getWaveformSummary() {
void TrackInfoObject::setWaveform(ConstWaveformPointer pWaveform) {
m_waveform = pWaveform;
emit(waveformUpdated());
}

ConstWaveformPointer TrackInfoObject::getWaveformSummary() const {
return m_waveformSummary;
}

// called from the AnalyserQueue Thread
void TrackInfoObject::waveformSummaryNew() {
void TrackInfoObject::setWaveformSummary(ConstWaveformPointer pWaveform) {
m_waveformSummary = pWaveform;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is there any reason why we might want to make sure the waveform and it's summary are both updated atomically (together)?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The waveform widgets use the waveform and the overviews us the summary. I don't think it matters too much.

emit(waveformSummaryUpdated());
}

Expand Down
Loading