MPRIS control: do not accumulate interface objects

Summary:
If MPRIS players were appearing and disappearing multiple times, the
OrgFreedesktopDBusPropertiesInterface & OrgMprisMediaPlayer2PlayerInterface
instances created for listening to the signals had been accumulating and
thus resulting in X signals per X restarted player, because the instances
were not deleted when a player disappeared.
Additionally were instances of them created on the fly on the stack in
some of the methods, instead of reusing the existing ones.

This patch changes that by introducing a class MprisPlayer which holds all
data & instances per player. This allows to look up the respective
interfaces instances to reuse them as well as properly controlling their
lifetime.

Test Plan:
Starting and restarting multiple MPRIS players (incl. multiple instances of
the same player app) works as expected as befire. They are listed on the
Android Media control as well as have proper states there when selected.
Additionally no longer multiple change signals are emitted if restarting a
player.

Reviewers: #kde_connect, mtijink

Reviewed By: #kde_connect, mtijink

Subscribers: mtijink

Differential Revision: https://phabricator.kde.org/D11389
parent 69ef7cfe
......@@ -38,6 +38,16 @@ K_PLUGIN_FACTORY_WITH_JSON( KdeConnectPluginFactory, "kdeconnect_mpriscontrol.js
Q_LOGGING_CATEGORY(KDECONNECT_PLUGIN_MPRIS, "kdeconnect.plugin.mpris")
MprisPlayer::MprisPlayer(const QString& serviceName, const QString& dbusObjectPath, const QDBusConnection& busConnection)
: m_serviceName(serviceName)
, m_propertiesInterface(new OrgFreedesktopDBusPropertiesInterface(serviceName, dbusObjectPath, busConnection))
, m_mediaPlayer2PlayerInterface(new OrgMprisMediaPlayer2PlayerInterface(serviceName, dbusObjectPath, busConnection))
{
m_mediaPlayer2PlayerInterface->setTimeout(500);
}
MprisControlPlugin::MprisControlPlugin(QObject* parent, const QVariantList& args)
: KdeConnectPlugin(parent, args)
, prevVolume(-1)
......@@ -75,7 +85,10 @@ void MprisControlPlugin::serviceOwnerChanged(const QString& serviceName, const Q
void MprisControlPlugin::addPlayer(const QString& service)
{
QDBusInterface mprisInterface(service, QStringLiteral("/org/mpris/MediaPlayer2"), QStringLiteral("org.mpris.MediaPlayer2"));
const QString mediaPlayerObjectPath = QStringLiteral("/org/mpris/MediaPlayer2");
// estimate identifier string
QDBusInterface mprisInterface(service, mediaPlayerObjectPath, QStringLiteral("org.mpris.MediaPlayer2"));
//FIXME: This call hangs and returns an empty string if KDED is still starting!
QString identity = mprisInterface.property("Identity").toString();
if (identity.isEmpty()) {
......@@ -83,30 +96,40 @@ void MprisControlPlugin::addPlayer(const QString& service)
}
QString uniqueName = identity;
for (int i = 1 ; !playerList[uniqueName].isEmpty() ; i++) {
for (int i = 1; playerList.contains(uniqueName); ++i) {
uniqueName = identity + " [" + i + "]";
}
playerList[uniqueName] = service;
qCDebug(KDECONNECT_PLUGIN_MPRIS) << "Mpris addPlayer" << service << "->" << uniqueName;
sendPlayerList();
MprisPlayer player(service, mediaPlayerObjectPath, QDBusConnection::sessionBus());
playerList.insert(uniqueName, player);
OrgFreedesktopDBusPropertiesInterface* freedesktopInterface = new OrgFreedesktopDBusPropertiesInterface(service, QStringLiteral("/org/mpris/MediaPlayer2"), QDBusConnection::sessionBus(), this);
connect(freedesktopInterface, &OrgFreedesktopDBusPropertiesInterface::PropertiesChanged, this, &MprisControlPlugin::propertiesChanged);
connect(player.propertiesInterface(), &OrgFreedesktopDBusPropertiesInterface::PropertiesChanged,
this, &MprisControlPlugin::propertiesChanged);
connect(player.mediaPlayer2PlayerInterface(), &OrgMprisMediaPlayer2PlayerInterface::Seeked,
this, &MprisControlPlugin::seeked);
OrgMprisMediaPlayer2PlayerInterface* mprisInterface0 = new OrgMprisMediaPlayer2PlayerInterface(service, QStringLiteral("/org/mpris/MediaPlayer2"), QDBusConnection::sessionBus());
connect(mprisInterface0, &OrgMprisMediaPlayer2PlayerInterface::Seeked, this, &MprisControlPlugin::seeked);
qCDebug(KDECONNECT_PLUGIN_MPRIS) << "Mpris addPlayer" << service << "->" << uniqueName;
sendPlayerList();
}
void MprisControlPlugin::seeked(qlonglong position){
//qCDebug(KDECONNECT_PLUGIN_MPRIS) << "Seeked in player";
OrgFreedesktopDBusPropertiesInterface* interface = (OrgFreedesktopDBusPropertiesInterface*)sender();
const QString& service = interface->service();
const QString& player = playerList.key(service);
OrgMprisMediaPlayer2PlayerInterface* mediaPlayer2PlayerInterface = (OrgMprisMediaPlayer2PlayerInterface*)sender();
const auto end = playerList.constEnd();
const auto it = std::find_if(playerList.constBegin(), end, [mediaPlayer2PlayerInterface](const MprisPlayer& player) {
return (player.mediaPlayer2PlayerInterface() == mediaPlayer2PlayerInterface);
});
if (it == end) {
qCWarning(KDECONNECT_PLUGIN_MPRIS) << "Seeked signal received for no longer tracked service" << mediaPlayer2PlayerInterface->service();
return;
}
const QString& playerName = it.key();
NetworkPacket np(PACKET_TYPE_MPRIS, {
{"pos", position/1000}, //Send milis instead of nanos
{"player", player}
{"player", playerName}
});
sendPacket(np);
}
......@@ -115,6 +138,19 @@ void MprisControlPlugin::propertiesChanged(const QString& propertyInterface, con
{
Q_UNUSED(propertyInterface);
OrgFreedesktopDBusPropertiesInterface* propertiesInterface = (OrgFreedesktopDBusPropertiesInterface*)sender();
const auto end = playerList.constEnd();
const auto it = std::find_if(playerList.constBegin(), end, [propertiesInterface](const MprisPlayer& player) {
return (player.propertiesInterface() == propertiesInterface);
});
if (it == end) {
qCWarning(KDECONNECT_PLUGIN_MPRIS) << "PropertiesChanged signal received for no longer tracked service" << propertiesInterface->service();
return;
}
OrgMprisMediaPlayer2PlayerInterface* const mediaPlayer2PlayerInterface = it.value().mediaPlayer2PlayerInterface();
const QString& playerName = it.key();
NetworkPacket np(PACKET_TYPE_MPRIS);
bool somethingToSend = false;
if (properties.contains(QStringLiteral("Volume"))) {
......@@ -160,25 +196,32 @@ void MprisControlPlugin::propertiesChanged(const QString& propertyInterface, con
}
if (somethingToSend) {
OrgFreedesktopDBusPropertiesInterface* interface = (OrgFreedesktopDBusPropertiesInterface*)sender();
const QString& service = interface->service();
const QString& player = playerList.key(service);
np.set(QStringLiteral("player"), player);
np.set(QStringLiteral("player"), playerName);
// Always also update the position
OrgMprisMediaPlayer2PlayerInterface mprisInterface(service, QStringLiteral("/org/mpris/MediaPlayer2"), QDBusConnection::sessionBus());
if (mprisInterface.canSeek()) {
long long pos = mprisInterface.position();
if (mediaPlayer2PlayerInterface->canSeek()) {
long long pos = mediaPlayer2PlayerInterface->position();
np.set(QStringLiteral("pos"), pos/1000); //Send milis instead of nanos
}
sendPacket(np);
}
}
void MprisControlPlugin::removePlayer(const QString& ifaceName)
void MprisControlPlugin::removePlayer(const QString& serviceName)
{
const QString identity = playerList.key(ifaceName);
qCDebug(KDECONNECT_PLUGIN_MPRIS) << "Mpris removePlayer" << ifaceName << "->" << identity;
playerList.remove(identity);
const auto end = playerList.end();
const auto it = std::find_if(playerList.begin(), end, [serviceName](const MprisPlayer& player) {
return (player.serviceName() == serviceName);
});
if (it == end) {
qCWarning(KDECONNECT_PLUGIN_MPRIS) << "Could not find player for serviceName" << serviceName;
return;
}
const QString& playerName = it.key();
qCDebug(KDECONNECT_PLUGIN_MPRIS) << "Mpris removePlayer" << serviceName << "->" << playerName;
playerList.erase(it);
sendPlayerList();
}
......@@ -190,7 +233,8 @@ bool MprisControlPlugin::receivePacket (const NetworkPacket& np)
//Send the player list
const QString player = np.get<QString>(QStringLiteral("player"));
bool valid_player = playerList.contains(player);
auto it = playerList.find(player);
bool valid_player = (it != playerList.end());
if (!valid_player || np.get<bool>(QStringLiteral("requestPlayerList"))) {
sendPlayerList();
if (!valid_player) {
......@@ -199,29 +243,31 @@ bool MprisControlPlugin::receivePacket (const NetworkPacket& np)
}
//Do something to the mpris interface
OrgMprisMediaPlayer2PlayerInterface mprisInterface(playerList[player], QStringLiteral("/org/mpris/MediaPlayer2"), QDBusConnection::sessionBus());
mprisInterface.setTimeout(500);
const QString& serviceName = it.value().serviceName();
// turn from pointer to reference to keep the patch diff small,
// actual patch would change all "mprisInterface." into "mprisInterface->"
auto& mprisInterface = *it.value().mediaPlayer2PlayerInterface();
if (np.has(QStringLiteral("action"))) {
const QString& action = np.get<QString>(QStringLiteral("action"));
//qCDebug(KDECONNECT_PLUGIN_MPRIS) << "Calling action" << action << "in" << playerList[player];
//qCDebug(KDECONNECT_PLUGIN_MPRIS) << "Calling action" << action << "in" << serviceName;
//TODO: Check for valid actions, currently we trust anything the other end sends us
mprisInterface.call(action);
}
if (np.has(QStringLiteral("setVolume"))) {
double volume = np.get<int>(QStringLiteral("setVolume"))/100.f;
qCDebug(KDECONNECT_PLUGIN_MPRIS) << "Setting volume" << volume << "to" << playerList[player];
qCDebug(KDECONNECT_PLUGIN_MPRIS) << "Setting volume" << volume << "to" << serviceName;
mprisInterface.setVolume(volume);
}
if (np.has(QStringLiteral("Seek"))) {
int offset = np.get<int>(QStringLiteral("Seek"));
//qCDebug(KDECONNECT_PLUGIN_MPRIS) << "Seeking" << offset << "to" << playerList[player];
//qCDebug(KDECONNECT_PLUGIN_MPRIS) << "Seeking" << offset << "to" << serviceName;
mprisInterface.Seek(offset);
}
if (np.has(QStringLiteral("SetPosition"))){
qlonglong position = np.get<qlonglong>(QStringLiteral("SetPosition"),0)*1000;
qlonglong seek = position - mprisInterface.position();
//qCDebug(KDECONNECT_PLUGIN_MPRIS) << "Setting position by seeking" << seek << "to" << playerList[player];
//qCDebug(KDECONNECT_PLUGIN_MPRIS) << "Setting position by seeking" << seek << "to" << serviceName;
mprisInterface.Seek(seek);
}
......@@ -251,6 +297,7 @@ bool MprisControlPlugin::receivePacket (const NetworkPacket& np)
answer.set(QStringLiteral("volume"),volume);
somethingToSend = true;
}
if (somethingToSend) {
answer.set(QStringLiteral("player"), player);
sendPacket(answer);
......
......@@ -25,9 +25,32 @@
#include <QHash>
#include <QLoggingCategory>
#include <QDBusServiceWatcher>
#include <QSharedPointer>
#include <core/kdeconnectplugin.h>
class OrgFreedesktopDBusPropertiesInterface;
class OrgMprisMediaPlayer2PlayerInterface;
class MprisPlayer
{
public:
MprisPlayer(const QString& serviceName, const QString& dbusObjectPath, const QDBusConnection& busConnection);
MprisPlayer() = delete;
public:
const QString& serviceName() const { return m_serviceName; }
OrgFreedesktopDBusPropertiesInterface* propertiesInterface() const { return m_propertiesInterface.data(); }
OrgMprisMediaPlayer2PlayerInterface* mediaPlayer2PlayerInterface() const { return m_mediaPlayer2PlayerInterface.data(); }
private:
QString m_serviceName;
QSharedPointer<OrgFreedesktopDBusPropertiesInterface> m_propertiesInterface;
QSharedPointer<OrgMprisMediaPlayer2PlayerInterface> m_mediaPlayer2PlayerInterface;
};
#define PACKET_TYPE_MPRIS QStringLiteral("kdeconnect.mpris")
Q_DECLARE_LOGGING_CATEGORY(KDECONNECT_PLUGIN_MPRIS)
......@@ -49,12 +72,12 @@ private Q_SLOTS:
private:
void serviceOwnerChanged(const QString& serviceName, const QString& oldOwner, const QString& newOwner);
void addPlayer(const QString& ifaceName);
void removePlayer(const QString& ifaceName);
void addPlayer(const QString& serviceName);
void removePlayer(const QString& serviceName);
void sendPlayerList();
void mprisPlayerMetadataToNetworkPacket(NetworkPacket& np, const QVariantMap& nowPlayingMap) const;
QHash<QString, QString> playerList;
QHash<QString, MprisPlayer> playerList;
int prevVolume;
QDBusServiceWatcher* m_watcher;
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment