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.

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

[WIP] Livemetadata PR #1675

Closed
wants to merge 79 commits into from
Closed
Show file tree
Hide file tree
Changes from 11 commits
Commits
Show all changes
79 commits
Select commit Hold shift + click to select a range
32cece7
Added .vscode on gitignore
davidhm May 5, 2018
ba829a2
First few files
davidhm May 14, 2018
5169748
So far manual tests are working
davidhm May 16, 2018
3d8c501
Pre automatic tests
davidhm May 18, 2018
523d09b
Using github as a backup
davidhm May 18, 2018
23ef862
Error when compiling moc generated cpp
davidhm May 20, 2018
0a4a4d6
WIP: Automatic tests
davidhm May 23, 2018
d203e85
Moved timers away from Track object
davidhm May 24, 2018
1a6bc0d
Pre volume scrobbling
davidhm May 26, 2018
ff27795
Pre tests, scrobbling manager
davidhm May 27, 2018
db2db23
Deadlock
davidhm May 30, 2018
7f55150
Solved the deadlock, simple tests pass
davidhm Jun 1, 2018
121443e
WIP: Adding file listener
davidhm Jun 2, 2018
2cd5bbc
Fixed file buffer, it's automatically updated now
davidhm Jun 3, 2018
b9c4a6c
[Untested] Fixed file info, modified metadatabroadcast
davidhm Jun 5, 2018
e10df9e
[WIP] Adding tests for scrobbling manager
davidhm Jun 7, 2018
c1de7db
Added first test for scrobbling manager
davidhm Jun 9, 2018
50019b0
File listener template + factory
davidhm Jun 10, 2018
d805306
Scrobbling tests done
davidhm Jun 12, 2018
4e714d5
[WIP] Adding file preferences
davidhm Jun 12, 2018
b3f8ea8
Preferences mock-up
davidhm Jun 13, 2018
8cd9c16
Changed everything to a weak pointer
davidhm Jun 13, 2018
d4b9aa5
Added file listener path in options
davidhm Jun 14, 2018
ec98ef6
[WIP] Persistent config now playing file
davidhm Jun 14, 2018
0209b4a
[WIP] Requested changes
davidhm Jun 15, 2018
2d8f4a2
Adding new tab
davidhm Jun 17, 2018
88c4819
Added mock-up in preferences
davidhm Jun 17, 2018
ab8b524
Table view mockup
davidhm Jun 18, 2018
97daf77
Pre changing prefmetadata class
davidhm Jun 19, 2018
4bc2de3
Added file settings
davidhm Jun 20, 2018
49b8b93
[Untested] Added options to file listener
davidhm Jun 20, 2018
567e9b5
Modified settings, added concurrency
davidhm Jun 23, 2018
5748fa9
Added dedicated thread
davidhm Jun 26, 2018
3a677b6
Modified author and title string
davidhm Jun 26, 2018
9c43d09
Merge branch 'master' into Livemetadata
davidhm Jun 26, 2018
2c21183
Added connections to lambda expressions in player manager, file liste…
davidhm Jun 28, 2018
a6d67d7
Fixed tests failing
davidhm Jun 29, 2018
ba8a6fc
Added mock network manager, gotta test with mock server
davidhm Jul 1, 2018
6b7f207
Now listening scrobbles work with ListenBrainz
davidhm Jul 5, 2018
f7c6a83
Deleted log file
davidhm Jul 5, 2018
1057b06
Merge branch 'master' into Livemetadata
davidhm Jul 6, 2018
152925c
ListenBrainz full scrobbles work now too
davidhm Jul 6, 2018
ad43628
Fixed double delete in developer mode
davidhm Jul 7, 2018
751f567
Forgot to compile
davidhm Jul 7, 2018
64d35c5
Modified metadata file options
davidhm Jul 7, 2018
ddfb859
Editable combobox
davidhm Jul 9, 2018
e4dc8df
Added mpris stub, not working
davidhm Jul 11, 2018
a1ad4c6
MPRIS now reflects the playback state
davidhm Jul 12, 2018
ee4c5a5
Disabled failing test until a solution is found
davidhm Jul 13, 2018
4571d43
Fixed broken tests, continuing MPRIS implementation
davidhm Jul 18, 2018
185d2f2
Fixed non compiling build
davidhm Jul 18, 2018
0bc22be
Segfault on weak_ptr
davidhm Jul 19, 2018
0af2279
Fixed eject bug
davidhm Jul 19, 2018
6be0042
Mpris reflects AutoDJ enabled
davidhm Jul 19, 2018
62eae49
Pause, play and go next MPRIS functions work
davidhm Jul 21, 2018
c2317c7
Fixed correct fade-in in MPRIS
davidhm Jul 22, 2018
8b5e54a
Added volume, playback status and loop status as well as a LINUX define
davidhm Jul 23, 2018
cac8c1f
Moved MPRIS into a feature
davidhm Jul 24, 2018
6807838
Added MPRIS macro in includes too
davidhm Jul 24, 2018
d6880a1
MPRIS is seekable now
davidhm Jul 24, 2018
32e565a
MPRIS broadcasts current track and rate works now too
davidhm Jul 25, 2018
1b77c8b
Modified linked lists into hash maps
davidhm Aug 1, 2018
7ba10a7
Added space after commas
davidhm Aug 1, 2018
94ab525
Changed ref ampersand and disabled non working test
davidhm Aug 1, 2018
5e8fb60
Added more button in encoding combo box
davidhm Aug 3, 2018
41bdfb9
Resized combobox
davidhm Aug 6, 2018
0def3e3
Merge remote-tracking branch 'upstream/master' into Livemetadata
davidhm Aug 6, 2018
9fe5277
Added few UI suggestions
davidhm Aug 6, 2018
97271d8
Missing include
davidhm Aug 7, 2018
f4b9649
Added cover art to mpris
davidhm Aug 11, 2018
dde6c47
Added cover art to MPRIS player
davidhm Aug 12, 2018
e1cfd7a
Fixed few things
davidhm Aug 14, 2018
69a3020
Deleting all cover art files
davidhm Aug 17, 2018
7af4dd7
Single file image
davidhm Aug 17, 2018
170e6c9
Cover art file is now QTemporaryFile
davidhm Aug 18, 2018
8c6fa58
Revamped encoding combobox
davidhm Aug 18, 2018
899f207
Fixed some cover URL generation issues
daschuer Aug 19, 2018
4a13665
Small fixes
davidhm Aug 20, 2018
39dc901
Merge pull request #1 from daschuer/Livemetadata
davidhm Aug 20, 2018
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
2 changes: 2 additions & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -57,3 +57,5 @@ lib/*/*.lib
lib/*/lib/*.lib

lib/qtscript-bytearray/*.cc

*.vscode
Copy link
Member

@Holzhaus Holzhaus Aug 27, 2019

Choose a reason for hiding this comment

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

This file is not created by Mixxx or its build system. Adding patterns for every conceivable editor/IDE to this project's gitignore is neither desirable nor feasible. You should add it to your global gitignore instead.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree, we should stop adding these editor generated files to our shared .gitignore and remove the ones that are already there.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was already mentioned in that PR ages ago. I can't seem to find it.

6 changes: 6 additions & 0 deletions build/depends.py
Original file line number Diff line number Diff line change
Expand Up @@ -438,6 +438,7 @@ def configure(self, build, conf):
class TestHeaders(Dependence):
def configure(self, build, conf):
build.env.Append(CPPPATH="#lib/gtest-1.7.0/include")
build.env.Append(CPPPATH="#lib/gmock-1.7.0/include")

class FidLib(Dependence):
def sources(self, build):
Expand Down Expand Up @@ -663,6 +664,9 @@ def sources(self, build):
"control/controlttrotary.cpp",
"control/controlencoder.cpp",

"broadcast/metadatabroadcast.cpp",
"broadcast/scrobblingmanager.cpp",

"controllers/dlgcontrollerlearning.cpp",
"controllers/dlgprefcontroller.cpp",
"controllers/dlgprefcontrollers.cpp",
Expand Down Expand Up @@ -1093,6 +1097,8 @@ def sources(self, build):
"track/trackinfo.cpp",
"track/trackrecord.cpp",
"track/trackref.cpp",
"track/trackplaytimers.cpp",
"track/tracktiminginfo.cpp",

"mixer/auxiliary.cpp",
"mixer/baseplayer.cpp",
Expand Down
18 changes: 18 additions & 0 deletions src/broadcast/metadatabroadcast.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
#include "metadatabroadcast.h"
#include "mixer/playerinfo.h"

MetadataBroadcast::MetadataBroadcast() {

}

void MetadataBroadcast::slotReadyToBeScrobbled(Track *pTrack) {

}

void MetadataBroadcast::slotNowListening(Track *pTrack) {

}

QLinkedList<TrackPointer> MetadataBroadcast::getTrackedTracks() {
return m_trackedTracks;
}
17 changes: 17 additions & 0 deletions src/broadcast/metadatabroadcast.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
#pragma once

#include <QObject>
#include <QLinkedList>
#include "track/track.h"

class MetadataBroadcast : public QObject {
Q_OBJECT
public:
MetadataBroadcast();
QLinkedList<TrackPointer> getTrackedTracks();
public slots:
void slotReadyToBeScrobbled(Track *pTrack);
void slotNowListening(Track *pTrack);
private:
QLinkedList<TrackPointer> m_trackedTracks;
};
215 changes: 215 additions & 0 deletions src/broadcast/scrobblingmanager.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,215 @@
#include <QObject>

#include "broadcast/scrobblingmanager.h"
#include "control/controlproxy.h"
#include "engine/enginexfader.h"
#include "mixer/deck.h"

ScrobblingManager::ScrobblingManager() :
m_CPGuiTick("[Master]", "guiTick50ms",this),
Copy link
Member

Choose a reason for hiding this comment

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

Some style nits:
We use a new line and 4 or 8 space indent before the colon. All initializer are following in a column + 2 spaces.
Opening brace is always at the line end.

ScrobblingManager::ScrobblingManager(PlayerManager *manager) 
        : m_CPGuiTick("[Master]", "guiTick50ms",this),
          ...
          m_CPCrossfader("[Master]","crossfader", this) {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okey, thanks.

m_CPCrossfader("[Master]","crossfader", this),
m_CPXFaderCurve(ConfigKey(EngineXfader::kXfaderConfigKey,
"xFaderCurve"),this),

m_CPXFaderCalibration(ConfigKey(EngineXfader::kXfaderConfigKey,
Copy link
Member

Choose a reason for hiding this comment

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

Can't you call the function that calculates the xfader gain instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, but I need the correct parameters which are given by those Control objects, aren't they? I copied the code from EngineMaster.

"xFaderCalibration"),this),

m_CPXFaderMode(ConfigKey(EngineXfader::kXfaderConfigKey,
"xFaderMode"),this),

m_CPXFaderReverse(ConfigKey(EngineXfader::kXfaderConfigKey,
"xFaderReverse"),this)
{
m_CPGuiTick.connectValueChanged(SLOT(slotGuiTick(double)));
startTimer(1000);
}


void ScrobblingManager::slotTrackPaused(TrackPointer pPausedTrack) {
//Extra functional decision to only track main decks.
Deck *sourceDeck = qobject_cast<Deck*>(sender());
if (sourceDeck == 0) {
qDebug() << "Didn't load track in a deck yet scrobbling "
"received paused signal.";
return;
}
// QMutexLocker locker(&m_mutex);
bool allPaused = true;
TrackInfo *pausedTrackInfo;
for (TrackInfo *trackInfo : m_trackList) {
if (trackInfo->m_pTrack == pPausedTrack) {
pausedTrackInfo = trackInfo;
for (BaseTrackPlayer *player : trackInfo->m_players) {
BaseTrackPlayerImpl *playerImpl =
qobject_cast<BaseTrackPlayerImpl*>(player);
Copy link
Member

Choose a reason for hiding this comment

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

Avoid upcasting. here it is better to put the required functions as pure virtuals into the base class.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I didn't want to pollute the interface with functions I need in case someone complains. But I will if you're ok.

Copy link
Member

Choose a reason for hiding this comment

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

The readability of the whole code base finally counts.

if (playerImpl == 0) {
qDebug() << "Track player interface isn't a "
"BaseTrackPlayerImpl";
return;
}
if (!playerImpl->isTrackPaused())
allPaused = false;
}
break;
}
}
if (allPaused)
Copy link
Member

Choose a reason for hiding this comment

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

Is this one the line, that is related to the deadlock?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes

pausedTrackInfo->m_trackInfo.pausePlayedTime();
Copy link
Member

Choose a reason for hiding this comment

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

You crash is not a deadlock, it is a Segmentation fault, which happens when you access memory, not allocated by the process. In this case, you access memory via the uninitialized pointer pausedTrackInfo.

It is a good idea to always initialize empty pointers with "nullptr". Than you can decorate your code with
DEBUG_ASSERT(pausedTrackInfo) in various places to detect such design issues early.
If you start mixxx with --debugAssertBreak under GDB, it will hit a breakpoint, instead of just abort.
Very handy :-)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Wow, how can a seg fault keep all threads in a futex? Wouldn't it kill the process? Thanks for the help.

Copy link
Member

@daschuer daschuer Jun 1, 2018

Choose a reason for hiding this comment

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

It actually kills the process here on my Ubuntu Trusty.

Copy link
Member

Choose a reason for hiding this comment

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

Mybe we have an additional deadlock?

}

void ScrobblingManager::slotTrackResumed(TrackPointer pResumedTrack) {
//Extra functional decision to only track main decks.
Deck *sourceDeck = qobject_cast<Deck*>(sender());
Copy link
Member

Choose a reason for hiding this comment

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

Please avoid these kind of implication. It is better to maintain to send the desire informations explicit.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okey.

if (sourceDeck == 0) {
Copy link
Member

Choose a reason for hiding this comment

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

if (!sourceDeck) { or == nullptr

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I saw the specification of qobject_cast and it returns 0, I read the standard and nullptr doesn't have to be 0, same with NULL that is implementation defined. So I went with 0 because that's what the Qt doc says. Though I agree it's more readable.

Copy link
Member

Choose a reason for hiding this comment

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

They mean the actual register value. This is relevant if you reinterpret a nullptr as integer. It is not guarantied that this is 0. It is more about type safety here.
0 is an integer literal, that is casted implicit to the nullptr type. Using !.. or nullptr avoids this implicit cast.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, ok, then I will use !

qDebug() << "Didn't load track in a deck yet scrobbling "
"received resumed signal.";
return;
}
if (isTrackAudible(pResumedTrack,sourceDeck)) {
// QMutexLocker locker(&m_mutex);
for (TrackInfo *trackInfo : m_trackList) {
if (trackInfo->m_pTrack == pResumedTrack) {
trackInfo->m_trackInfo.resumePlayedTime();
break;
}
}
}
}

void ScrobblingManager::slotLoadingTrack(TrackPointer pNewTrack, TrackPointer pOldTrack) {
//Extra functional decision to only track main decks.
Deck *sourceDeck = qobject_cast<Deck*>(sender());
if (sourceDeck == 0) {
qDebug() << "Didn't load track in a deck yet scrobbling "
"received loading signal.";
return;
}
if (pOldTrack) {
m_tracksToBeReset.append(TrackToBeReset(pOldTrack,sourceDeck));
}
}

void ScrobblingManager::slotNewTrackLoaded(TrackPointer pNewTrack) {
//Empty deck gives a null pointer.
if (!pNewTrack)
return;
//Extra functional decision to only track main decks.
Deck *newDeck = qobject_cast<Deck*>(sender());
if (newDeck == 0) {
qDebug() << "Didn't load track in a deck yet scrobbling "
"received loaded signal.";
return;
}
// QMutexLocker locker(&m_mutex);
bool trackAlreadyAdded = false;
for (TrackInfo *trackInfo : m_trackList) {
if (trackInfo->m_pTrack == pNewTrack) {
trackInfo->m_players.append(newDeck);
Copy link
Member

@daschuer daschuer Jun 1, 2018

Choose a reason for hiding this comment

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

Storing a pointer you not own is always kind of dangerous, because you don't know the live time of it.
It is better to use a shared pointer, or the group string.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, the player manager doesn't destroy the decks until it itself gets destroyed, and this object gets destroyed too. But I will use a group string because shared pointer implies modifying player manager.

trackAlreadyAdded = true;
break;
}
}
if (!trackAlreadyAdded) {
TrackInfo *newTrackInfo = new TrackInfo(pNewTrack,newDeck);
Copy link
Member

Choose a reason for hiding this comment

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

you can consider to make it a unique pointer.
To make it work, m_trackList has to be a std:: container.

newTrackInfo->m_players.append(newDeck);
m_trackList.append(newTrackInfo);
connect(&m_trackList.last()->m_trackInfo,SIGNAL(readyToBeScrobbled(TrackPointer)),
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do you establish a signal/slot connection between all instances of the internal TrackTimingInfo class and the broadcaster instead of only using a single connection between the ScrobblingManager and the broadcaster? You send the affected track with as the payload data with every signal and the track is bound to each instance of TrackTimingInfo anyway. If you need more context than just the TrackPointer then you could always collect this in a dedicated signal data class.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't see how it is any different, you're just adding one level of indirection. In the end, the broadcaster manages the info on the grace period timer, the scrobbling manager doesn't care about it. Could you please explain why you think this is a bad idea?

Copy link
Contributor

Choose a reason for hiding this comment

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

Simple and robust design? It doesn't make sense to use n connections when 1 is sufficient and all the receiver needs to know about the context is already part of the message.

The track info objects are an implementation detail of your class that should not be visible from the outside. The connections make them visible, indirectly. You are also not allowed to delete those objects like you do now while some signals might still be pending. You have to delete them with QObject::deleteLater(). Otherwise the receiver will get a dangling sender pointer. But all this doesn't matter if you lift the connection up to the next level.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Correct me if I'm wrong but if they live on the same thread aren't they under a direct connection? Then there are no pending signals because the slot is called immediately. However, I didn't think about exposing implementation details so you're right, I will call them from the scrobbling manager.

Copy link
Member

Choose a reason for hiding this comment

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

A direct connection is finally a simple callback, Unfortunately, Qt has a lot of boilerplate code to detect that and to make connect and reconnect save in all circumstances.
Signal and slot connection suffer also a big issue regarding readability. You cannot instantly be sure who is connected where. So would should always prefer not to use signal and slots or similar messaging.

this,SLOT(slotReadyToBeScrobbled(TrackPointer)));
}
//A new track has been loaded so must unload old one.
resetTracks();
}

void ScrobblingManager::slotPlayerEmpty() {
// QMutexLocker locker(&m_mutex);
resetTracks();
}

void ScrobblingManager::resetTracks() {
for (TrackToBeReset candidateTrack : m_tracksToBeReset) {
for (TrackInfo *trackInfo : m_trackList) {
if (trackInfo->m_pTrack == candidateTrack.m_pTrack) {
if (!trackInfo->m_players.contains(candidateTrack.m_pPlayer)) {
qDebug() << "Track doesn't contain player"
"yet is requested for deletion.";
return;
}
//Load error, stray from engine buffer.
if (candidateTrack.m_pPlayer->getLoadedTrack() ==
candidateTrack.m_pTrack)
break;
QLinkedList<BaseTrackPlayer*>::iterator it =
trackInfo->m_players.begin();
while (it != trackInfo->m_players.end()) {
Copy link
Member

Choose a reason for hiding this comment

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

Is this an endless loop?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for pointing it out

if (*it == candidateTrack.m_pPlayer) {
trackInfo->m_players.erase(it);
}
}
if (trackInfo->m_players.empty()) {
trackInfo->m_trackInfo.pausePlayedTime();
trackInfo->m_trackInfo.resetPlayedTime();
delete trackInfo;
}
break;
}
}
}
}

bool ScrobblingManager::isTrackAudible(TrackPointer pTrack, BaseTrackPlayer * pPlayer) {
if (pPlayer->getLoadedTrack() != pTrack) {
qDebug() << "Track can't be audible because is not in player";
return false;
}
return getPlayerVolume(pPlayer) >= 0.20;
}

double ScrobblingManager::getPlayerVolume(BaseTrackPlayer *pPlayer) {
double finalVolume;
ControlProxy trackPreGain(pPlayer->getGroup(),"pregain",this);
double preGain = trackPreGain.get();
ControlProxy trackVolume(pPlayer->getGroup(),"volume",this);
double volume = trackVolume.get();
ControlProxy deckOrientation(pPlayer->getGroup(),"orientation",this);
int orientation = deckOrientation.get();

double xFaderLeft,xFaderRight;

EngineXfader::getXfadeGains(m_CPCrossfader.get(),
m_CPXFaderCurve.get(),
m_CPXFaderCalibration.get(),
m_CPXFaderMode.get(),
m_CPXFaderReverse.toBool(),
&xFaderLeft,&xFaderRight);
finalVolume = preGain * volume;
if (orientation == EngineChannel::LEFT)
finalVolume *= xFaderLeft;
else if (orientation == EngineChannel::RIGHT)
finalVolume *= xFaderRight;
return finalVolume;
}

void ScrobblingManager::slotGuiTick(double timeSinceLastTick) {
for (TrackInfo *trackInfo : m_trackList) {
trackInfo->m_trackInfo.slotGuiTick(timeSinceLastTick);
}
}

void ScrobblingManager::timerEvent(QTimerEvent *timerEvent) {
for (TrackInfo *trackInfo : m_trackList) {
bool inaudible = true;
Copy link
Contributor

Choose a reason for hiding this comment

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

Please try to avoid naming a boolean variables with a negation names. The name "audible" would work much better here. But the local variable is not needed anyway, the code can be simplified.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll change the name of the variable.

Copy link
Contributor

Choose a reason for hiding this comment

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

You don't need the variable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can you elaborate?

for (BaseTrackPlayer *player : trackInfo->m_players) {
if (isTrackAudible(trackInfo->m_pTrack,player)) {
inaudible = false;
break;
}
}
if (inaudible) {
trackInfo->m_trackInfo.pausePlayedTime();
}
}
}

void ScrobblingManager::slotReadyToBeScrobbled(TrackPointer pTrack) {
qDebug() << "Track ready to be scrobbled";
}
56 changes: 56 additions & 0 deletions src/broadcast/scrobblingmanager.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,56 @@
#pragma once

#include <QObject>
#include <QMutex>
#include <QLinkedList>

#include "mixer/basetrackplayer.h"
#include "track/track.h"
#include "track/tracktiminginfo.h"

class BaseTrackPlayer;

class ScrobblingManager : public QObject {
Copy link
Member

Choose a reason for hiding this comment

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

I think the name of this class is not self explaining.
How many "Current tracks" we will have finally?

  • The current scrobble track
  • The current Track for Metadata Streaming (RDS)
  • The current Track for history playlist
  • The current Track driving the master clock for beat matching (@kshitij98 project)

Will this become a CurrentTrackManager or CurrentTrackCalculator?

@kshitij98: could you reuse portions of this code? Or could you share your rules for a master clock track?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The current scrobble track, metadata streaming and history playlist are all the same, the one given by PlayerInfo.h. This manager takes into account when a track is loaded, paused, etc. and checks if they're audible every second to update the timers, and when they are it sends a scrobble signal.

Copy link
Contributor

Choose a reason for hiding this comment

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

@kshitij98: could you reuse portions of this code? Or could you share your rules for a master clock track?

PlayerInfo::getCurrentPlayingTrack returns the most audible track which should drive the master clock. This is not a part of my project so I will be able to continue working on it only once I'm done with my project, or I have sufficient time in between.

Related: https://bugs.launchpad.net/mixxx/+bug/1770239

Q_OBJECT
public:
ScrobblingManager();
private:
struct TrackInfo {
TrackPointer m_pTrack;
Copy link
Contributor

Choose a reason for hiding this comment

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

Storing shared track pointers in a class that exists for an unforeseeable time span, that is manually allocated and deleted and moreover stored as a plain pointer in a list managed by 2 elongate nested for loops concerns me. Remember, without releasing the track pointers you will not only introduce a memory leak, but also prevent that any modifications are written into the database or exported into files.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well the tracks are released as soon as they are ejected from all decks. I don't see how this is a problem since as long as there is a deck with a track the pointer is not released either.

Copy link
Contributor

Choose a reason for hiding this comment

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

Are you sure that they are deleted always and promptly? The code dealing with this lifecycle management is too complex and hard to reason about. That's my concern.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, I'm reasonably sure, unless there are unforeseen bugs. The problem is if I substitute it by a weak pointer then after the track has been dropped from the decks it will already be deleted, and then I can't query it further to mantain internal state.

Copy link
Member

Choose a reason for hiding this comment

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

IMHO the weak pointer idea is not too bad.
You normally scrobble a track only if it is playing, right? So other weak pointer should always point to a valid object if you needed.
If you hit a stalled week pointer, you can debug assert, because you code is faulty if it happens.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can add weak pointers and then pass all the info by value to the metadata broadcaster.

TrackTimingInfo m_trackInfo;
QLinkedList<BaseTrackPlayer*> m_players;
TrackInfo(TrackPointer pTrack, BaseTrackPlayer *player) :
m_pTrack(pTrack), m_trackInfo(pTrack)
{}
};
struct TrackToBeReset {
TrackPointer m_pTrack;
BaseTrackPlayer *m_pPlayer;
TrackToBeReset(TrackPointer pTrack, BaseTrackPlayer *player) :
m_pTrack(pTrack),m_pPlayer(player) {}
};
QMutex m_mutex;
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do you need this mutex? What exactly are the shared resources it protects and who are the contestants? I only see some slots at the interface, which is good decision.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As far as I know, each deck can request a track to the engine buffer independently with its own thread. And each thread signals independently. I'm not sure about this since I haven't explored in depth the engine. However, if that's the case, I need to protect the list of tracks from concurrent access, don't I?

Copy link
Contributor

Choose a reason for hiding this comment

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

Your slots will only be invoked by your object's event loop thread. It doesn't matter if the signal is sent from another thread.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm now that you mention it, if the decks are owned by the PlayerManager, and the manager owns the ScrobblingManager, they all live in the same thread don't they? I guess I don't need a mutex then. But I need to make absolutely sure because I don't want to find data race conditions further down the road which can be a pain in the ass to detect.

Copy link
Member

Choose a reason for hiding this comment

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

We will finally have threads, accessing this Infos. If they will be qt threads, receiving a singnal, we can get arround the issue.
But I think we need a qt free plain c interface for the consuming plug-ins, to get around the mess with crashing because of incompatible shared libraries...

QLinkedList<TrackInfo*> m_trackList;
QLinkedList<TrackToBeReset> m_tracksToBeReset;
ControlProxy m_CPGuiTick;
ControlProxy m_CPCrossfader;
ControlProxy m_CPXFaderCurve;
ControlProxy m_CPXFaderCalibration;
ControlProxy m_CPXFaderMode;
ControlProxy m_CPXFaderReverse;

void resetTracks();
bool isTrackAudible(TrackPointer pTrack, BaseTrackPlayer * pPlayer);
double getPlayerVolume(BaseTrackPlayer *pPlayer);
protected:
void timerEvent(QTimerEvent *timerEvent) override;
public slots:
void slotTrackPaused(TrackPointer pPausedTrack);
void slotTrackResumed(TrackPointer pResumedTrack);
void slotLoadingTrack(TrackPointer pNewTrack, TrackPointer pOldTrack);
void slotNewTrackLoaded(TrackPointer pNewTrack);
void slotPlayerEmpty();
private slots:
void slotGuiTick(double timeSinceLastTick);
void slotReadyToBeScrobbled(TrackPointer pTrack);
};
8 changes: 8 additions & 0 deletions src/mixer/basetrackplayer.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -387,6 +387,10 @@ void BaseTrackPlayerImpl::slotSetReplayGain(mixxx::ReplayGain replayGain) {
}

void BaseTrackPlayerImpl::slotPlayToggled(double v) {
if (v == 0)
emit(trackPaused(m_pLoadedTrack));
else if (v == 1)
emit(trackResumed(m_pLoadedTrack));
if (!v && m_replaygainPending) {
setReplayGain(m_pLoadedTrack->getReplayGain().getRatio());
}
Expand Down Expand Up @@ -472,3 +476,7 @@ void BaseTrackPlayerImpl::setReplayGain(double value) {
m_pReplayGain->set(value);
m_replaygainPending = false;
}

bool BaseTrackPlayerImpl::isTrackPaused() const {
return m_pPlay->toBool();
}
5 changes: 5 additions & 0 deletions src/mixer/basetrackplayer.h
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,9 @@ class BaseTrackPlayer : public BasePlayer {
void newTrackLoaded(TrackPointer pLoadedTrack);
void loadingTrack(TrackPointer pNewTrack, TrackPointer pOldTrack);
void playerEmpty();
void trackLoadFailed(TrackPointer pFailedTrack);
void trackPaused(TrackPointer pPausedTrack);
void trackResumed(TrackPointer pResumedTrack);
void noPassthroughInputConfigured();
void noVinylControlInputConfigured();
};
Expand Down Expand Up @@ -70,6 +73,8 @@ class BaseTrackPlayerImpl : public BaseTrackPlayer {
// For testing, loads a fake track.
TrackPointer loadFakeTrack(bool bPlay, double filebpm);

bool isTrackPaused() const;

public slots:
void slotLoadTrack(TrackPointer track, bool bPlay) final;
void slotTrackLoaded(TrackPointer pNewTrack, TrackPointer pOldTrack);
Expand Down
Loading