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 2 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.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
148 changes: 80 additions & 68 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,8 +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),
Expand Down Expand Up @@ -60,10 +57,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 +67,35 @@ 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_waveform = WaveformPointer(new Waveform(
sampleRate, totalSamples, mainWaveformSampleRate, -1));
m_waveformSummary = WaveformPointer(new Waveform(
sampleRate, totalSamples, mainWaveformSampleRate,
summaryWaveformSamples));

// We must not allow other parts of Mixxx to touch the waveforms while
// we are initializing them. Don't set them on the TIO until they are
// ready.
Copy link
Member

Choose a reason for hiding this comment

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

What does the comment mean? Can we change it to:

// 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.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yea, that's a better way to put it.

tio->setWaveform(m_waveform);
tio->setWaveformSummary(m_waveformSummary);

m_waveformDataSize = m_waveform->getDataSize();
m_waveformSummaryDataSize = m_waveformSummary->getDataSize();
Copy link
Member

Choose a reason for hiding this comment

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

It there a need for m_waveformDataSize? If we inline m_waveform->getDataSize(); we can use it directly.

Copy link
Member Author

Choose a reason for hiding this comment

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

Nope -- I'll remove.


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 +113,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 +134,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 +158,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,30 +208,30 @@ 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++;
Expand All @@ -252,13 +256,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 +278,41 @@ void AnalyserWaveform::cleanup(TrackPointer tio) {
return;
}

if (m_waveform) {
m_waveform->reset();
}
tio->setWaveform(ConstWaveformPointer());
m_waveformData = NULL;
m_waveformDataSize = 0;
m_waveform.clear();

if (m_waveformSummary) {
m_waveformSummary->reset();
}
tio->setWaveformSummary(ConstWaveformPointer());
m_waveformSummaryData = NULL;
m_waveformSummaryDataSize = 0;
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());
m_waveformData = NULL;
m_waveformDataSize = 0;
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());
m_waveformSummaryData = NULL;
m_waveformSummaryDataSize = 0;
m_waveformSummary.clear();
}

#ifdef TEST_HEAT_MAP
test_heatMap->save("heatMap.png");
Expand Down
4 changes: 2 additions & 2 deletions src/analyserwaveform.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -163,8 +163,8 @@ class AnalyserWaveform : public Analyser {
private:
bool m_skipProcessing;

Waveform* m_waveform;
Waveform* m_waveformSummary;
WaveformPointer m_waveform;
WaveformPointer m_waveformSummary;
int m_waveformDataSize;
int m_waveformSummaryDataSize;
WaveformData* m_waveformData;
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
15 changes: 7 additions & 8 deletions src/trackinfoobject.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,9 +36,9 @@
#include "track/beats.h"
#include "track/keys.h"
#include "util/sandbox.h"
#include "waveform/waveform.h"

class Cue;
class Waveform;

class TrackInfoObject;
typedef QSharedPointer<TrackInfoObject> TrackPointer;
Expand Down Expand Up @@ -223,12 +223,11 @@ class TrackInfoObject : public QObject {
// Set URL for track
void setURL(const QString& url);

Waveform* getWaveform();
void waveformNew();
ConstWaveformPointer getWaveform();
void setWaveform(ConstWaveformPointer pWaveform);

Waveform* getWaveformSummary();
const Waveform* getWaveformSummary() const;
void waveformSummaryNew();
ConstWaveformPointer getWaveformSummary() const;
void setWaveformSummary(ConstWaveformPointer pWaveform);

void setAnalyserProgress(int progress);
int getAnalyserProgress() const;
Expand Down Expand Up @@ -396,8 +395,8 @@ class TrackInfoObject : public QObject {
BeatsPointer m_pBeats;

//Visual waveform data
Waveform* const m_waveform;
Waveform* const m_waveformSummary;
ConstWaveformPointer m_waveform;
ConstWaveformPointer m_waveformSummary;

QAtomicInt m_analyserProgress; // in 0.1%

Expand Down