[WIP] Livemetadata PR #1675

davidhm · 2018-05-20T19:03:23Z

This is only intended for code review atm, not to be merged.

daschuer · 2018-05-20T22:35:49Z

src/track/trackplaytimers.cpp

+#include "track/trackplaytimers.h"
+
+TrackTimers::TimerQt::TimerQt()
+{


Why do you wrap the QT timer here?

Because QTimer isn't a subclass of my TrackTimer. And I need TrackTimer to mock timers, otherwise I would have to actually wait until a track is played halfway to check if the signals are correctly emitted.

uklotzde · 2018-05-21T14:22:11Z

src/track/track.cpp

@@ -70,7 +71,11 @@ Track::Track(
          m_record(trackId),
          m_bDirty(false),
          m_bMarkedForMetadataExport(false),
-          m_analyzerProgress(-1) {
+          m_analyzerProgress(-1),          


I don't think that it is appropriate to store transient state of an application component directly inside the Track object. For example m_analyzerProgress does not belong in here and will be removed soon (#1624).

You should keep track of the tracks that are currently played in your component and manage this information there. Connect to the PlayerManager or individual decks for getting notified about which tracks you need to observe.

I don't get what you mean exactly? All info I've put in track is the number of ms it has been played along with some timers to update this number and whether if the track is scrobbable or not. Doesn't this information belong to the track? Should I keep a list of the tracks being currently played and update their played time in the player manager?

The Track object is already huge and becomes unmanageable if every aspect is thrown in there. It should contain only persistent state that needs to be synchronized with the database and the corresponding file, but not arbitrary transient application state.

Please try to keep the state that is needed for live metadata broadcasting private and local within the corresponding component or module. Use signals/slots for synchronization instead of distributing implementation details into public, shared global state.

Think in terms of modular microservices, even in the context of a desktop application. Consume signals/notifications from other components as input and produce output by emitting signals. All transient state should be isolated within the corresponding module.

Okey, I get it now. I will move my logic away from Track.

daschuer · 2018-05-21T21:16:07Z

src/track/trackplaytimers.cpp

+TrackTimers::TimerQt::TimerQt()
+{
+    connect(&m_Timer,SIGNAL(timeout()),
+            this,SIGNAL(timeout()));


Qt tries hard to emit the timeout() signal as precise as possible.
Unfortunately this interferes with the waveforms, also living in the main thread, which really need this exactness.
As workaround, we have a less exact GUI Tick timer that fires all 50 ms.
It is [Master]", "guiTick50ms"

Oh, this is why I used an elapsed timer with less granularity. I will use this one then.

daschuer · 2018-05-21T21:22:55Z

It is hard to check the code, without knowing what it should do.
Can you outline the requirements on your wiki page?

Some words or drawings about the data-flow and the related classes would also be nice. This should help to avoid later round-trips, because of some design issues.

davidhm · 2018-05-22T16:54:46Z

Alright, I'm gonna draw some diagrams.

daschuer · 2018-05-28T06:16:11Z

src/broadcast/scrobblingmanager.cpp

+m_CPXFaderCurve(ConfigKey(EngineXfader::kXfaderConfigKey, 
+                "xFaderCurve"),this),
+
+m_CPXFaderCalibration(ConfigKey(EngineXfader::kXfaderConfigKey, 


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

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

daschuer · 2018-05-28T06:18:28Z

src/broadcast/scrobblingmanager.cpp

+    }
+    QMutexLocker locker(&m_mutex);
+    QLinkedList<TrackInfo*>::Iterator it = m_trackList.begin();
+    while(it != m_trackList.end()) {


I think you can use here to the new
C++11 range based for loop.

Yeah, I forgot, thanks.

daschuer · 2018-05-28T06:19:35Z

src/broadcast/scrobblingmanager.cpp

+    QMutexLocker locker(&m_mutex);
+    QLinkedList<TrackInfo*>::Iterator it = m_trackList.begin();
+    while(it != m_trackList.end()) {
+        if ((*it)->m_pTrack == pPausedTrack and (*it)->m_pPlayer == sourceDeck) {


We use && instead of and

Why? Aren't and, not and or more explicative?

Probably yes, but we just don't use it. I am in doubt that many C++ developers even know that "and" exists.

...including me ;) Please don't use those operator synonyms. I've never seen anyone using them before. They seem to be supported for backward compatibility with C ISO/IEC 646 where they are defined as macros in a separate header file.

Okey guys, your style, your rules. But I have to say, every programming course I took first year in C++ told me to use "and", "or" and "not" because they are more expressive. And I have to agree ;)

daschuer · 2018-05-28T07:49:41Z

src/broadcast/scrobblingmanager.cpp

+        QMutexLocker locker(&m_mutex);
+        QLinkedList<TrackInfo*>::Iterator it = m_trackList.begin();
+        while(it != m_trackList.end()) {
+            if ((*it)->m_pTrack == pPausedTrack and (*it)->m_pPlayer == sourceDeck) {


Why is the deck important here?
Please add source code comments that explains this.

Because a track can be playing on a deck and be paused while the same track plays in another deck and not be paused. It's important that both the deck and the track match. I will add comments.

So if a track plays in two decks, with significant volume, the timer counts twice as fast?

I just realized there's several things wrong with this code. First, it uses pausePlayedTime in slotTrackResumed, which is clearly wrong. Two, a same track should have only one timer. Otherwise, if you have the same track in two decks and keep switching the cross fader back and forth the signal will be sent much later. Thanks for pointing it out.

daschuer · 2018-05-31T12:44:29Z

src/broadcast/scrobblingmanager.cpp

+            break;
+        }                   
+    }
+    if (allPaused)


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

daschuer · 2018-05-31T12:51:37Z

src/track/tracktiminginfo.cpp

+    if (m_pElapsedTimer->isValid()) {
+        m_playedMs += m_pElapsedTimer->elapsed();
+        m_pElapsedTimer->invalidate();
+        QObject::disconnect(m_pTimer.get(),SIGNAL(timeout()),


Disconnect locks a muted internally. You should not connect and disconnect signals after initialize.
Do you use Qt5? I am suffering from bufferunderflows. Maybe this is related.
Does it still deadlock if you remove this line?
Is there a way around using a timer signal here?

Unfortunately, there's a deadlock even if I remove every single connect and disconnect call inside that class. I tried using valgrind but it's giving me an illegal instruction error. I'm going to investigate further. Also, I don't use Qt5, I will check on that.

Qt5 will introduce new problems. I would stick with Qt4 in this case.
I will have a look into this soon today.

daschuer · 2018-06-01T06:38:51Z

src/broadcast/scrobblingmanager.cpp

+        }                   
+    }
+    if (allPaused)
+        pausedTrackInfo->m_trackInfo.pausePlayedTime();        


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 :-)

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

It actually kills the process here on my Ubuntu Trusty.

Mybe we have an additional deadlock?

daschuer · 2018-06-01T06:42:43Z

src/broadcast/scrobblingmanager.cpp

+            pausedTrackInfo = trackInfo;
+            for (BaseTrackPlayer *player : trackInfo->m_players) {
+                BaseTrackPlayerImpl *playerImpl = 
+                    qobject_cast<BaseTrackPlayerImpl*>(player);


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

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

The readability of the whole code base finally counts.

daschuer · 2018-06-01T06:44:19Z

src/broadcast/scrobblingmanager.cpp

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


if (!sourceDeck) { or == nullptr

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.

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.

Oh, ok, then I will use !

daschuer · 2018-06-01T06:47:38Z

src/broadcast/scrobblingmanager.cpp

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


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

daschuer · 2018-06-01T06:51:43Z

src/broadcast/scrobblingmanager.cpp

+    bool trackAlreadyAdded = false;
+    for (TrackInfo *trackInfo : m_trackList) {
+        if (trackInfo->m_pTrack == pNewTrack) {
+            trackInfo->m_players.append(newDeck);               


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.

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.

daschuer · 2018-06-01T06:54:36Z

src/broadcast/scrobblingmanager.cpp

+        }
+    }
+    if (!trackAlreadyAdded) {
+        TrackInfo *newTrackInfo = new TrackInfo(pNewTrack,newDeck);


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

daschuer · 2018-06-01T06:55:42Z

src/broadcast/scrobblingmanager.cpp

+                    break;                    
+                QLinkedList<BaseTrackPlayer*>::iterator it = 
+                    trackInfo->m_players.begin();
+                while (it != trackInfo->m_players.end()) {


Is this an endless loop?

Thanks for pointing it out

daschuer · 2018-06-03T13:32:28Z

src/broadcast/scrobblingmanager.h

+class BaseTrackPlayer;
+class PlayerManager;
+
+class ScrobblingManager : public QObject {


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?

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.

@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

daschuer · 2019-10-18T22:55:35Z

There is some work left to do.
Are you able to help? We need an independent code review.
And someone who is able to cut out merge able branches.

daschuer · 2019-10-18T22:59:03Z

https://github.com/daschuer/mixxx/tree/Livemetadata
Is a rebased Version. Unfortunately it has new merge conflicts now.
I think I will rebase it again.

nonchip · 2019-10-19T07:18:05Z

Are you able to help?

in the sense that i technically know C++, yeah, but haven't done anything with QT in ages and never even taken a look at the mixxx codebase, so not sure if i'd be able to offer much help, i'd probably be more of a hindrance :P

daschuer · 2019-10-19T10:42:46Z

One of the main reason Mixxx is so vivit, is the underestimated
second use case, that many developers including myself use it to learn coding and developing in an open source team. You are very welcome to do the same.
In this case here, you unbiased view will help us to optimize the code for easy understanding.
https://www.mixxx.org/wiki/doku.php/bugfix_workflow helps for a good start.

nonchip · 2019-10-20T07:58:52Z

good point, but given i know nothing about the codebase, very little about the used frameworks, and have never done whatever an "independent code review" entails i'm afraid i'm currently the wrong one for that task :/

honestly i'd probably do a better job trying to implement a new feature in a foreign codebase than to review someone else's code

davidhm · 2019-10-22T13:43:24Z

I'll try to continue this once I'm done with my master's. Right now I'm starting my Master's thesis and working at the same time, so it's gonna be quite hard for me to do extra work on the side.

ligi · 2020-06-21T20:00:30Z

Would love this feature! Hope it get's finished/merged soon

Be-ing · 2020-06-21T20:01:27Z

@ligi are you volunteering to finish it?

github-actions · 2020-10-20T00:22:07Z

This PR is marked as stale because it has been open 90 days with no activity.

poelzi · 2020-12-17T16:30:28Z

I will pick this one up and get it merged

daschuer · 2020-12-17T19:11:01Z

Great, thank you very much!

What is you approach?

poelzi · 2020-12-17T21:50:30Z

I just need to remote control mix a little bit. When leaving the room I want to pause it from home assitant and use my kdeconnect controls on the mobile etc.
For better control, we should finish the OSC api, but for simple scriping etc, the dbus api is actually the better one.

daschuer · 2020-12-17T22:43:51Z

That sound good.

Here are many conflicts. Do you plan to solve them or to start over?

uklotzde · 2020-12-17T23:01:14Z

That sound good.

Here are many conflicts. Do you plan to solve them or to start over?

I recommend to start over and only borrow what is needed, be it ideas or code.

uklotzde · 2020-12-17T23:05:26Z

I just need to remote control mix a little bit. When leaving the room I want to pause it from home assitant and use my kdeconnect controls on the mobile etc.
For better control, we should finish the OSC api, but for simple scriping etc, the dbus api is actually the better one.

But isn't D-Bus is only available on Linux? We should use cross-platform solutions for application features if ever possible.

daschuer · 2020-12-17T23:22:23Z

But isn't D-Bus is only available on Linux? We should use cross-platform solutions for application features if ever possible.

Yes it is Linux only. But it is THE interface to add info to the system tray. There is no other way than using platform dependent solutions for this.

uklotzde · 2020-12-17T23:41:55Z

But isn't D-Bus is only available on Linux? We should use cross-platform solutions for application features if ever possible.

Yes it is Linux only. But it is THE interface to add info to the system tray. There is no other way than using platform dependent solutions for this.

If it is only supposed to be used for the system tray ok. But if it is intended as a general purpose IPC interface than not.

daschuer · 2020-12-27T00:07:26Z

@poelzi: Please have a look at #3483 which is a working version.
Can you please do some brief tests with it?
Does it work for you?

Holzhaus · 2021-04-02T13:02:21Z

I guess we can close this in favor of #3483. If not, please reopen.

davidhm added 6 commits May 5, 2018 15:33

Added .vscode on gitignore

32cece7

First few files

ba829a2

So far manual tests are working

5169748

Pre automatic tests

3d8c501

Using github as a backup

523d09b

Error when compiling moc generated cpp

23ef862

daschuer reviewed May 20, 2018

View reviewed changes

uklotzde reviewed May 21, 2018

View reviewed changes

daschuer reviewed May 21, 2018

View reviewed changes

davidhm added 4 commits May 23, 2018 09:18

WIP: Automatic tests

0a4a4d6

Moved timers away from Track object

d203e85

Pre volume scrobbling

1a6bc0d

Pre tests, scrobbling manager

ff27795

daschuer reviewed May 28, 2018

View reviewed changes

Deadlock

db2db23

daschuer reviewed May 31, 2018

View reviewed changes

daschuer reviewed Jun 1, 2018

View reviewed changes

davidhm added 3 commits June 1, 2018 20:49

Solved the deadlock, simple tests pass

7f55150

WIP: Adding file listener

121443e

Fixed file buffer, it's automatically updated now

2cd5bbc

daschuer reviewed Jun 3, 2018

View reviewed changes

Be-ing modified the milestones: 2.4.0, stalled Apr 6, 2020

github-actions bot added the stale Stale issues that haven't been updated for a long time. label Oct 20, 2020

Be-ing changed the base branch from master to main October 23, 2020 23:50

github-actions bot removed the stale Stale issues that haven't been updated for a long time. label Dec 18, 2020

daschuer mentioned this pull request Dec 27, 2020

expose current track metadata to external APIs #3483

Draft

Holzhaus closed this Apr 2, 2021

This was referenced Aug 22, 2022

Report current track via MPRIS DBus interface #7493

Open

Qt5 affects abilty to find window in AltaCast #9327

Open

UDP message to the localhost port, for additional mixxx PLUGINS #9664

Open

Discord Rich Presence Integration #9961

Open

[WIP] Livemetadata PR #1675

[WIP] Livemetadata PR #1675

Conversation

davidhm commented May 20, 2018

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daschuer commented May 21, 2018

davidhm commented May 22, 2018

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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daschuer Jun 1, 2018 • 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

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

Choose a reason for hiding this comment

daschuer Jun 1, 2018 • 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

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 commented Oct 18, 2019

daschuer commented Oct 18, 2019

nonchip commented Oct 19, 2019

daschuer commented Oct 19, 2019

nonchip commented Oct 20, 2019

davidhm commented Oct 22, 2019

ligi commented Jun 21, 2020

Be-ing commented Jun 21, 2020

github-actions bot commented Oct 20, 2020

poelzi commented Dec 17, 2020

daschuer commented Dec 17, 2020

poelzi commented Dec 17, 2020

daschuer commented Dec 17, 2020

uklotzde commented Dec 17, 2020

uklotzde commented Dec 17, 2020

daschuer commented Dec 17, 2020

uklotzde commented Dec 17, 2020

daschuer commented Dec 27, 2020

Holzhaus commented Apr 2, 2021

daschuer Jun 1, 2018 •

edited

Loading

daschuer Jun 1, 2018 •

edited

Loading