expose current track metadata to external APIs #3483

daschuer · 2020-12-27T00:05:21Z

This is a rebase of @davidhm Livemetadata #1675 PR on current main.

It fetaures:

MPRIS D-Bus interface for Linux system tray remotes
Scrobbling to Listenbrains
Writing a NowPlaying.txt

daschuer · 2020-12-27T23:39:57Z

All green now. @poelzi Please give this a try.

poelzi

Smaller nitpicks and the DEBUG ASSERT needs to proper handle fallback.
Have not tested listenbrainz, but mrpis works nice. Also tested the dbus api.

poelzi · 2020-12-28T02:22:17Z

src/broadcast/filelistener/filelistener.cpp

+        m_fileContents.artist = pTrack->getArtist();
+        QString writtenString(m_latestSettings.fileFormatString);
+        writtenString.replace("$author", pTrack->getArtist())
+                .replace("$title", pTrack->getTitle()) += '\n';


Can we use '{{ track.title }}' and '{{ track.artist }}' here ?
I'm working on better track export functionality and have added the grantlee library for that, so you can define patterns that will become the path name for the exported files. With this syntax we will later be able to switch to a grantlee template and have the full template power later.

poelzi · 2020-12-28T03:08:30Z

src/broadcast/mpris/mediaplayer2.cpp

+}
+
+void MediaPlayer2::Quit() {
+    QApplication::quit();


We need to call the MixxxApplication to quit here otherwise the controlobjects are not destroyed in order, cause lots of Debug Asserts

poelzi · 2020-12-28T03:09:49Z

src/broadcast/mpris/mediaplayer2.cpp

+}
+
+void MediaPlayer2::Raise() {
+    m_pMixxx->raise();


This does not work when called over dbus.

poelzi · 2020-12-28T03:11:09Z

src/broadcast/mpris/mediaplayer2.cpp

+    QStringList ret;
+    ret << "audio/mpeg"
+        << "audio/ogg";
+    return ret;


Can we build something from the supported codecs ? At least wav, flac, mp3, aiff should be in the list

poelzi · 2020-12-28T03:21:31Z

src/broadcast/mpris/mprisplayer.cpp

+
+namespace {
+
+const QString kPlaybackStatusPlaying = "Playing";


Suggested change

const QString kPlaybackStatusPlaying = "Playing";

const QLatin1String kPlaybackStatusPlaying = "Playing";

poelzi · 2020-12-28T03:43:32Z

src/broadcast/mpris/mprisplayer.cpp

+    metadata.insert("mpris:length", m_currentMetadata.trackDuration);
+    metadata.insert("xesam:artist", m_currentMetadata.artists);
+    metadata.insert("xesam:title", m_currentMetadata.title);
+    metadata.insert("mpris:artUrl", m_currentMetadata.coverartUrl);


https://www.freedesktop.org/wiki/Specifications/mpris-spec/metadata/#index4h4
I think those would be interesting as well:

album -> xesam:album
rating -> xesam:userRating
play_counter -> xesam:useCount

poelzi · 2020-12-28T05:55:31Z

src/broadcast/mpris/mprisplayer.cpp

+}
+
+qlonglong MprisPlayer::seek(qlonglong offset, bool& success) {
+    if (autoDjIdle()) {


should use canSeek() here

poelzi · 2020-12-28T05:57:16Z

src/broadcast/mpris/mprisplayer.cpp

+}
+
+bool MprisPlayer::canSeek() const {
+    return autoDjIdle();


this check is wrong when the last track is playing and autodj is already off. you can still seek in the track running.

Yes, it is also wrong if auto DJ is off and user plays a track manually.
I think a fix will be part of general overhaul.

poelzi · 2020-12-28T06:02:11Z

src/broadcast/mpris/mprisplayer.cpp

+    if (playingDeck == nullptr) {
+        ControlProxy playing(ConfigKey(m_pausedDeck, "play"));
+        BaseTrackPlayer* player = m_pPlayerManager->getPlayer(m_pausedDeck);
+        DEBUG_ASSERT(player);


I had one crash here that I could not reproduce later:

3:41:31.777 [Main] debug TrackDAO::updateTrack TrackDAO: Updating track in database 7726 QFileInfo(/home/poelzi/Music/psyblast/[Omveda Records]/Omveda Records - XIII Monkeys - Psychedelic Renaissance (FREE)/XIII MONKEYS - XIII Monkeys - Psychedelic Renaissance (FREE) - 01 Alien Agenda.flac) 3:41:31.777 [Main] debug mixxx::Logger::debug SqlTransaction - Started new SQL database transaction on "MIXXX-1" 3:41:32.131 [Main] debug mixxx::Logger::debug SqlTransaction - Committed SQL database transaction on "MIXXX-1" 3:41:32.131 [Main] debug BaseTrackCache::updateIndexWithQuery BaseTrackCache(0xb16c2e0) updateIndexWithQuery took 0 ms 3:41:32.131 [Main] debug mixxx::Logger::debug GlobalTrackCache - Deleting Track(0x7f7ba83fb480) 3:41:32.134 [Main] debug AutoDJProcessor::calculateTransition player "[Channel2]" calculateTransition() 3:41:32.134 [Controller] debug unknown CDBG XONE:K2 MIDI 1: outgoing: status 0x9E (ch 15, opcode 0x9), ctrl 0x2D, val 0x7F 3:41:32.134 [Controller] debug unknown CDBG XONE:K2 MIDI 1: outgoing: status 0x9D (ch 14, opcode 0x9), ctrl 0x2C, val 0x7F 3:41:32.134 [Controller] debug unknown CDBG XONE:K2 MIDI 1: outgoing: status 0x9C (ch 13, opcode 0x9), ctrl 0x2C, val 0x7F 3:41:32.134 [Controller] debug unknown CDBG XONE:K2 MIDI 1: outgoing: status 0x9B (ch 12, opcode 0x9), ctrl 0x2E, val 0x7F 3:41:32.134 [Controller] debug unknown CDBG XONE:K2 MIDI 1: outgoing: status 0x9A (ch 11, opcode 0x9), ctrl 0x2D, val 0x7F 3:41:32.134 [Controller] debug unknown CDBG XONE:K2 MIDI 1: outgoing: status 0x99 (ch 10, opcode 0x9), ctrl 0x2C, val 0x7F 3:41:32.141 [Main] debug mixxx::Logger::debug AnalyzerThread - Enqueueing next track 7728 3:41:32.141 [AnalyzerThread 0 #1] debug mixxx::Logger::debug AnalyzerThread - Dequeued next track 7728 3:41:32.141 [AnalyzerThread 0 #1] debug mixxx::Logger::debug AnalyzerThread - Analyzing QFileInfo(/home/poelzi/Music/psyblast/[Omveda Records]/Omveda Records - XIII Monkeys - Psychedelic Renaissance (FREE)/XIII MONKEYS - XIII Monkeys - Psychedelic Renaissance (FREE) - 02 Psychedelic Renaissance.flac) 3:41:32.142 [AnalyzerThread 0 #1] debug mixxx::Logger::debug SoundSourceProxy - SoundSourceProvider "Xiph.org libFLAC" created a SoundSource for file "file:///home/poelzi/Music/psyblast/[Omveda Records]/Omveda Records - XIII Monkeys - Psychedelic Renaissance (FREE)/XIII MONKEYS - XIII Monkeys - Psychedelic Renaissance (FREE) - 02 Psychedelic Renaissance.flac" of type "flac" 3:41:32.142 [AnalyzerThread 0 #1] debug AnalysisDao::loadAnalysesFromQuery AnalysisDAO fetched 0 analyses, 0 bytes for track 7728 in 0 ms 3:41:32.143 [AnalyzerThread 0 #1] debug AnalyzerEbur128::initialize Skipping AnalyzerEbur128 3:41:32.143 [AnalyzerThread 0 #1] debug AnalyzerBeats::initialize AnalyzerBeats preference settings: Plugin: "qm-tempotracker:0" Min/Max BPM: 70 250 Fixed tempo assumption: true Offset correction: true Re-analyze when settings change: false Fast analysis: false 3:41:32.143 [AnalyzerThread 0 #1] debug AnalyzerKey::initialize AnalyzerKey preference settings: Plugin: "qm-keydetector:2" Re-analyze when settings change: false Fast analysis: false 3:41:32.143 [AnalyzerThread 0 #1] debug AnalyzerKey::shouldAnalyze Track has previous key detection result that is not up to date with latest settings but user preferences indicate we should not re-analyze it. 3:41:36.193 [AnalyzerThread 0 #1] debug Waveform::toByteArray Writing waveform from byte array: dataSize 371166 allSignalSize 371166 visualSampleRate 441 audioVisualRatio 100 3:41:36.729 [AnalyzerThread 0 #1] debug AnalysisDao::saveAnalysis AnalysisDAO saved analysis 7007 "3396322 (1601831 compressed)" bytes for track 7728 in 523 ms 3:41:36.729 [AnalyzerThread 0 #1] debug AnalysisDao::saveTrackAnalyses Saved waveform analysis for trackId 7728 analysisId 7007 3:41:36.729 [AnalyzerThread 0 #1] debug Waveform::toByteArray Writing waveform from byte array: dataSize 3840 allSignalSize 3840 visualSampleRate 4.5625 audioVisualRatio 9665.76 3:41:36.741 [AnalyzerThread 0 #1] debug AnalysisDao::saveAnalysis AnalysisDAO saved analysis 7008 "34941 (15475 compressed)" bytes for track 7728 in 11 ms 3:41:36.741 [AnalyzerThread 0 #1] debug AnalysisDao::saveTrackAnalyses Saved waveform summary analysis for trackId 7728 analysisId 7008 3:41:36.741 [AnalyzerThread 0 #1] debug mixxx::Logger::debug AnalyzerWaveform - Waveform generation for track 7728 done 4 s 3:41:37.269 [Main] critical DEBUG ASSERT: "key.isValid() || flags.testFlag(ControlFlag::AllowInvalidKey)" in function ControlProxy::ControlProxy(const ConfigKey &, QObject *, ControlFlags) at /home/poelzi/Projects/mixxx-test/src/control/controlproxy.cpp:19 3:41:37.270 [Main] critical DEBUG ASSERT: "player" in function void MprisPlayer::play() at /home/poelzi/Projects/mixxx-test/src/broadcast/mpris/mprisplayer.cpp:203 Segmentation fault (core dumped)

poelzi · 2020-12-28T06:06:13Z

@daschuer thanks for picking this one up, it would have taken quite some time for me. apart from smaller suggestions and one crash, this branch works nice.

daschuer · 2020-12-29T23:43:49Z

Now the crash is fixed.
Mpirs player should now work at any Auto-DJ state, fixing the reported issue.
I have also used [Master],gain for mpris volume, because it creates weird situations when messing with the deck volume.

Be-ing · 2020-12-30T00:36:04Z

Please add comments in the header of every new class explaining its purpose.

Be-ing · 2020-12-30T00:40:54Z

src/broadcast/mpris/mpris.cpp

+    // they are connected to is also deleted).
+    // http://doc.qt.io/qt-5/qdbusabstractadaptor.html
+    new MediaPlayer2(pMixxx, this);
+    m_busConnection.registerObject("/org/mpris/MediaPlayer2", this);


QLatin1String?

IIRC QLatin1String can be used when comparing a QString to a literal value. When constructing an actual QString, I think QStringLiteral should be used. Here we want the latter.

Be-ing · 2020-12-30T00:41:46Z

src/broadcast/mpris/mprisplayer.cpp

+// the playback will stop when there are no more tracks to play
+const QString kLoopStatusNone = QStringLiteral("None");
+// The current track will start again from the begining once it has finished playing
+const QString kLoopStatusTrack = QStringLiteral("Track");
+// The playback loops through a list of tracks
+const QString kLoopStatusPlaylist = QStringLiteral("Playlist");


Shouldn't these be translated?

no, those are status flags for the dbus api

Okay. It would help to add a comment to clarify.

Be-ing · 2020-12-30T00:56:14Z

src/broadcast/scrobblingmanager.cpp

+
+ScrobblingManager::ScrobblingManager(UserSettingsPointer pConfig,
+        std::shared_ptr<PlayerManager> pPlayerManager,
+        MixxxMainWindow* pWindow)


unused, please remove. Let's decouple application logic from QWidgets as much as possible.

It is used for the window control in mpris.

Be-ing · 2020-12-30T00:57:02Z

src/coreservices.cpp

@@ -114,7 +115,7 @@ void CoreServices::initializeSettings() {
    m_pSettingsManager = std::make_unique<SettingsManager>(settingsPath);
 }

-void CoreServices::initialize(QApplication* pApp) {
+void CoreServices::initialize(QApplication* pApp, MixxxMainWindow* pMixxx) {


Revert this! This defeats the purpose of the CoreServices class.

It is used for the window control in mpris.

Coupling CoreServices to QWidgets is a big step backwards. Please find another way to implement this. Using signals and slots instead could work.

That is not an issue, because it is only the forward declarated class. CoreServices passes only the pointer, without caring what it is.

It is an issue to knowingly create problems for other branches in progress.

Please explain the issue that it creates.

We only pass a pointer. This can be even a void pointer in CoreService and we can cast it MixxxMainWindow in MPRIS. But making use of the compilers type safety is better.

Be-ing · 2020-12-30T00:57:18Z

src/coreservices.h

@@ -22,6 +22,8 @@ class VinylControlManager;
 class TrackCollectionManager;
 class Library;
 class LV2Backend;
+class MixxxMainWindow;


It is used for the window control in mpris.

Be-ing · 2020-12-30T01:01:26Z

src/broadcast/mpris/mediaplayer2.cpp

+}
+
+void MediaPlayer2::setFullscreen(bool fullscreen) {
+    m_pMixxx->slotViewFullScreen(fullscreen);


The QWidget dependency can be removed from this class by emitting a signal here

Yes, we can emit a signal here, the issue is that someone needs to connect the MediaPlayer2 signal to the window.
This way we get mpris internals into the window class which is also a bad idea.

In addition we have also:
m_pMixxx->isFullScreen();
m_pMixxx->raise();

As info in the reverse direction.

Form this point of view the solution is not bad.
If we have one day another window technology we "just" need to implement a common interface for both technologies.
For now the current solution is just fine, because we don't know the requirements of the other window system.

I think we can do some proper common interface when the QMLApplication is in some proper state.

I disagree. We know this conflicts with #3638. That IMO must be resolved before merging this.

Be-ing · 2020-12-30T01:02:05Z

src/broadcast/mpris/mediaplayer2.cpp

+}
+
+bool MediaPlayer2::fullscreen() const {
+    return m_pMixxx->isFullScreen();


How can we get this information without m_pMixxx? Or maybe remove it? Is it really needed?

we could track the signals from the mainwindow whne switching from/to fullscreen

I like that idea.

Due to the connect call we get the coupling anyway. So there is not really a benefit using the complexity of signal and slots.

Be-ing · 2020-12-30T01:09:19Z

src/preferences/dialog/dlgprefmetadatadlg.ui

+   </rect>
+  </property>
+  <property name="windowTitle">
+   <string>Metadata Broadcast Preferences</string>


Suggested change

<string>Metadata Broadcast Preferences</string>

<string>Metadata Output Preferences</string>

I think the conflation of these new features with broadcasting would confuse a lot of users.

Be-ing · 2020-12-30T01:10:05Z

src/preferences/dialog/dlgprefmetadatadlg.ui

+      <item>
+       <widget class="QCheckBox" name="enableFileListener">
+        <property name="text">
+         <string>Enable metadata file</string>


Suggested change

<string>Enable metadata file</string>

<string>Write metadata of current track to a file</string>

Be-ing · 2020-12-30T01:11:08Z

src/preferences/dialog/dlgprefmetadatadlg.ui

+      <item>
+       <layout class="QHBoxLayout" name="horizontalLayout_4">
+        <item>
+         <widget class="QPushButton" name="filePathButton">


This feels odd on the left of the QLineEdit. It is conventional to put it on the right.

Be-ing · 2020-12-30T01:12:36Z

src/preferences/dialog/dlgprefmetadatadlg.ui

+      <item>
+       <widget class="QLabel" name="label_4">
+        <property name="text">
+         <string>User token. If you don't have one please register &lt;a href=&quot;https://listenbrainz.org/login/&quot;&gt;here&lt;/a&gt;</string>


Suggested change

<string>User token. If you don't have one please register <a href="https://listenbrainz.org/login/">here</a></string>

<string>User token. If you do not have one, <a href="https://listenbrainz.org/login/">register a ListenBrainz account</a>.</string>

src/preferences/metadatafilesettings.cpp

madorian · 2024-07-06T01:18:59Z

I'm trying to read this, but I'm not really understanding it;) Is this the only way to remotely control Mixxx? I want to load a file into one of its decks using an API.

daschuer · 2024-07-06T10:39:34Z

This PR needs some love to split it in review-able chunks to become part of the main branch. I would love to do it one day, but dragged away from it once again.

Do you have some spare time and programming skills?
Than we can implement your request, working together.

First I like to understand your use case? How do you plan to select the track? Do you use a different process that manages your tracks ? On which OS are you?

madorian · 2024-07-06T13:39:48Z

I feel you; too many balls in the air here, as well;)

I was just planning to escape from it all mixing some music;). I intend to find tracks using Emacs or CLI, then issue a command of some sort to load it into Mixxx. I'm on NixOS.

Swiftb0y · 2024-07-06T16:48:16Z

@madorian Have you read some of the more advanced techniques for finding tracks within the mixxx library? https://manual.mixxx.org/2.4/en/chapters/library#finding-tracks-search

daschuer · 2024-07-07T09:30:09Z

using Emacs or CLI

That could work and is kind of straight forward. So we need a mixxx_cli process that contacts the main mixxx process via a IPC interface.

The project can be split into two parts:

Design the CLI API
Design the IPC

For 1. Go ahead. Pandora's Box is open. What do you need? You may describe it as https://github.com/mixxxdj/proposals

2.: I would prefer to use a standard interface that can be used elsewhere. Like OSC or MPRiS.

Will this work for you?

madorian · 2024-07-07T21:34:59Z

@daschuer Yes, OSC is perfect and portable. I'm not familiar with MPRiS, but it seems very cool and I will read up on that.

acolombier · 2024-09-14T16:22:04Z

I just gave that a go and it work great! I haven't had a chance to look into the code much yet, but I have some early feedback on the feature

The setting panel could use a better design
1. The setting category could be perhaps more generic (e.g Integration). I'm thinking of building a custom D-Bus interface in Mixxx which would allow CO to be invoked by other apps, and this would be the kind of category to have both settings in.
  A dedicated icon would be great. Perhaps @ronso0 you have some suggestion here?
2. Doesn't look like the UI is scaling correctly
3. It would be great to grey out the label as if the setting is disabled. It might also be worth using group box to highlight best the root checkboxes
4. Description (if you don't... and The strings...) could look better as a separate QLabel with a smaller font, under the InputBox IMO
It would be great to get to support mpris:artUrl. (Looking at the doc, it looks like the covert art could be saved as a temp file)

Also, as an early feedback on the code

Please add comments in the header of every new class explaining its purpose.

This would be quite useful as this is adding a significant amount of file in the repo.

Appreciate you may not be interested in finishing this PR, let me know if so and I'd be interested to help!

Edit: Just noticed that mpris:arturl is supposed to already be implemented, but doesn't seem to work. I also think you will need individual URL for remote app to reload the new cover.

daschuer · 2024-10-15T21:18:59Z

I am very much interested to get this PR merged. My plan was to move the features into single PRs to make them review-able. But I got dragged away. Any help is welcome. My current priority is to release 2.4.2 and 2.5.0.

acolombier · 2024-10-27T13:26:20Z

Sounds good - glad to hear this is still alive! I'm removing needs review for now and making it a draft till this gets broken down into multiple PR.

github-actions bot added build code quality library ui labels Dec 27, 2020

daschuer mentioned this pull request Dec 27, 2020

[WIP] Livemetadata PR #1675

Closed

daschuer force-pushed the Livemetadata branch 5 times, most recently from c5f2ee0 to 982be0f Compare December 27, 2020 16:03

poelzi suggested changes Dec 28, 2020

View reviewed changes

Be-ing added this to the 2.4.0 milestone Dec 30, 2020

Be-ing changed the title ~~Rebase of Livemetadata #1675~~ expose current track metadata to external APIs Dec 30, 2020

Be-ing reviewed Dec 30, 2020

View reviewed changes

src/preferences/metadatafilesettings.cpp Outdated Show resolved Hide resolved

daschuer added 6 commits March 21, 2024 13:57

Rename Metadata Output Preferences

e42b7d8

Improve GUI

d25822f

latin1 with ISO-8859-1

c495c1a

Added comment to disabled code

ce6e9b8

Swap initalisation order.

4e710f5

Improve Metadata output preferences

2f7f002

daschuer force-pushed the Livemetadata branch from 50c981c to 2f7f002 Compare March 22, 2024 09:05

github-actions bot added autodj preferences broadcast Bugs pertaining to streaming radio broadcaster use-case soundio qml labels Mar 22, 2024

daschuer added 5 commits March 23, 2024 17:48

Add missing Q_UNUSED

d7931ea

Fix codespell issues.

c2ec4c1

Fix clang-format issues

709645c

Fix clazy complains

58591ce

Add a missing Q_UNUSED

fc9d92d

Eve00000 mentioned this pull request Apr 23, 2024

Mixxx status files + settings #13157

Open

acolombier mentioned this pull request Sep 14, 2024

Report current track via MPRIS DBus interface #7493

Open

acolombier removed the needs review label Oct 27, 2024

acolombier marked this pull request as draft October 27, 2024 13:26


		namespace {

		const QString kPlaybackStatusPlaying = "Playing";

	const QString kPlaybackStatusPlaying = "Playing";
	const QLatin1String kPlaybackStatusPlaying = "Playing";

	<string>Metadata Broadcast Preferences</string>
	<string>Metadata Output Preferences</string>

	<string>Enable metadata file</string>
	<string>Write metadata of current track to a file</string>

	<string>User token. If you don't have one please register <a href="https://listenbrainz.org/login/">here</a></string>
	<string>User token. If you do not have one, <a href="https://listenbrainz.org/login/">register a ListenBrainz account</a>.</string>

expose current track metadata to external APIs #3483

Are you sure you want to change the base?

expose current track metadata to external APIs #3483

Conversation

daschuer commented Dec 27, 2020

daschuer commented Dec 27, 2020

poelzi left a 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

poelzi commented Dec 28, 2020

daschuer commented Dec 29, 2020

Be-ing commented Dec 30, 2020

Choose a reason for hiding this comment

Holzhaus Dec 30, 2020 • 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

Be-ing Dec 30, 2020 • 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

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

madorian commented Jul 6, 2024

daschuer commented Jul 6, 2024

madorian commented Jul 6, 2024

Swiftb0y commented Jul 6, 2024

daschuer commented Jul 7, 2024

madorian commented Jul 7, 2024

acolombier commented Sep 14, 2024 • edited Loading

daschuer commented Oct 15, 2024

acolombier commented Oct 27, 2024

Holzhaus Dec 30, 2020 •

edited

Loading

Be-ing Dec 30, 2020 •

edited

Loading

acolombier commented Sep 14, 2024 •

edited

Loading