[Merge] lp:~phablet-team/qtubuntu-media/audio_role into lp:qtubuntu-media
Alfonso Sanchez-Beato
alfonso.sanchez-beato at canonical.com
Tue Mar 22 08:40:10 UTC 2016
Review: Needs Fixing
See comments.
Diff comments:
>
> === added file 'src/aal/aalaudiorolecontrol.cpp'
> --- src/aal/aalaudiorolecontrol.cpp 1970-01-01 00:00:00 +0000
> +++ src/aal/aalaudiorolecontrol.cpp 2016-03-21 21:02:25 +0000
> @@ -0,0 +1,110 @@
> +/*
> + * Copyright (C) 2016 Canonical, Ltd.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU Lesser General Public License as published by
> + * the Free Software Foundation; version 3.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public License
> + * along with this program. If not, see <http://www.gnu.org/licenses/>.
> + */
> +
> +#include "aalaudiorolecontrol.h"
> +
> +#include <QDebug>
> +
> +namespace media = core::ubuntu::media;
> +
> +AalAudioRoleControl::AalAudioRoleControl()
> + : QAudioRoleControl()
> + , m_audioRole(QAudio::UnknownRole)
> +{
> +}
> +
> +QAudio::Role AalAudioRoleControl::audioRole() const
> +{
> + return m_audioRole;
> +}
> +
> +void AalAudioRoleControl::setAudioRole(QAudio::Role role)
> +{
> + if (m_hubPlayerSession == nullptr)
> + {
> + qWarning() << "Failed to setAudioRole since m_hubPlayerSession is NULL";
It is better to make the program crash if this is not something that should happen. Even more, I think this class only makes sense if you have a m_hubPlayerSession, so probably it will be better to add it as argument to the constructor.
> + return;
> + }
> +
> + try {
> + m_hubPlayerSession->audio_stream_role().set(fromQAudioRole(role));
> + }
> + catch (const std::runtime_error &e) {
Should not we just let the exception propagate and let the library user choose what to do in this case?
> + qWarning() << "Failed to set audio stream role: " << e.what();
> + return;
> + }
> +
> + if (role != m_audioRole)
> + Q_EMIT audioRoleChanged(m_audioRole = role);
> +}
> +
> +QList<QAudio::Role> AalAudioRoleControl::supportedAudioRoles() const
> +{
> + return QList<QAudio::Role>() << QAudio::MusicRole
> + << QAudio::VideoRole
> + << QAudio::AlarmRole
> + << QAudio::NotificationRole
> + << QAudio::RingtoneRole
> + << QAudio::VoiceCommunicationRole;
> +}
> +
> +void AalAudioRoleControl::setPlayerSession
> + (const std::shared_ptr<core::ubuntu::media::Player>& playerSession)
> +{
> + m_hubPlayerSession = playerSession;
> +}
> +
> +QAudio::Role AalAudioRoleControl::toQAudioRole(const media::Player::AudioStreamRole &role)
> +{
> + switch (role)
> + {
> + case media::Player::AudioStreamRole::multimedia:
> + return QAudio::MusicRole;
> + case media::Player::AudioStreamRole::alarm:
> + return QAudio::AlarmRole;
> + case media::Player::AudioStreamRole::alert:
> + return QAudio::NotificationRole;
> + case media::Player::AudioStreamRole::phone:
> + return QAudio::VoiceCommunicationRole;
> + default:
> + qWarning() << "Unhandled or invalid core::ubuntu::media::AudioStreamRole: " << role;
> + return QAudio::MusicRole;
> + }
> +}
> +
> +media::Player::AudioStreamRole AalAudioRoleControl::fromQAudioRole(const QAudio::Role &role)
> +{
> + // If we don't have a valid role, this should be the default translation
> + if (role == QAudio::Role::UnknownRole)
> + return media::Player::AudioStreamRole::multimedia;
> +
> + switch (role)
> + {
> + case QAudio::MusicRole:
> + case QAudio::VideoRole:
> + return media::Player::AudioStreamRole::multimedia;
> + case QAudio::AlarmRole:
> + return media::Player::AudioStreamRole::alarm;
> + case QAudio::NotificationRole:
> + case QAudio::RingtoneRole:
> + return media::Player::AudioStreamRole::alert;
> + case QAudio::VoiceCommunicationRole:
> + return media::Player::AudioStreamRole::phone;
> + default:
> + qWarning() << "Unhandled or invalid QAudio::Role:" << role;
> + return media::Player::AudioStreamRole::multimedia;
> + }
> +}
>
> === added file 'src/aal/aalaudiorolecontrol.h'
> --- src/aal/aalaudiorolecontrol.h 1970-01-01 00:00:00 +0000
> +++ src/aal/aalaudiorolecontrol.h 2016-03-21 21:02:25 +0000
> @@ -0,0 +1,45 @@
> +/*
> + * Copyright (C) 2016 Canonical, Ltd.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU Lesser General Public License as published by
> + * the Free Software Foundation; version 3.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public License
> + * along with this program. If not, see <http://www.gnu.org/licenses/>.
> + */
> +
> +#ifndef AALAUDIOROLECONTROL_H
> +#define AALAUDIOROLECONTROL_H
> +
> +#include <core/media/player.h>
> +
> +#include <qaudiorolecontrol.h>
> +
> +class AalAudioRoleControl : public QAudioRoleControl
> +{
> +public:
> + AalAudioRoleControl();
> +
> + QAudio::Role audioRole() const;
> + void setAudioRole(QAudio::Role role);
> + QList<QAudio::Role> supportedAudioRoles() const;
> +
> + static QAudio::Role toQAudioRole
> + (const core::ubuntu::media::Player::AudioStreamRole &role);
> + static core::ubuntu::media::Player::AudioStreamRole fromQAudioRole
> + (const QAudio::Role &role);
I would prefer names similar to audioStream2qAudio() and qAudio2audioStream()
> +
> + void setPlayerSession(const std::shared_ptr<core::ubuntu::media::Player>& playerSession);
> +
> +private:
> + QAudio::Role m_audioRole;
> + std::shared_ptr<core::ubuntu::media::Player> m_hubPlayerSession;
> +};
> +
> +#endif // AALAUDIOROLECONTROL_H
>
> === modified file 'src/aal/aalmediaplayerservice.cpp'
> --- src/aal/aalmediaplayerservice.cpp 2016-02-19 16:38:09 +0000
> +++ src/aal/aalmediaplayerservice.cpp 2016-03-21 21:02:25 +0000
> @@ -741,6 +741,42 @@
> }
> }
>
> +void AalMediaPlayerService::constructNewPlayerService()
This function should be defined just below the constructors, as it is used only there.
> +{
> + if (not m_hubService.get())
> + m_hubService = media::Service::Client::instance();
> +
> + // As core::Connection doesn't allow us to start with a disconnected connection
> + // instance we have to connect it first with a dummy signal and then disconnect
> + // it again. If we don't do this connectSignals() will never be able to attach
> + // to the relevant signals.
> + m_endOfStreamConnection.disconnect();
> +
> + if (!newMediaPlayer())
> + qWarning() << "Failed to create a new media player backend. Video playback will not function." << endl;
> +
> + if (m_hubPlayerSession == NULL)
> + {
> + qWarning() << "Could not finish contructing new AalMediaPlayerService instance since m_hubPlayerSession is NULL";
> + return;
> + }
> +
> + createMediaPlayerControl();
> + createVideoRendererControl();
> + createAudioRoleControl();
> +
> + m_playbackStatusChangedConnection = m_hubPlayerSession->playback_status_changed().connect(
> + [this](const media::Player::PlaybackStatus &status) {
> + m_newStatus = status;
> + QMetaObject::invokeMethod(this, "onPlaybackStatusChanged", Qt::QueuedConnection);
> + });
> +
> + m_errorConnection = m_hubPlayerSession->error().connect(
> + std::bind(&AalMediaPlayerService::onError, this, _1));
> +
> + connect(qGuiApp, &QGuiApplication::applicationStateChanged, this, &AalMediaPlayerService::onApplicationStateChanged);
> +}
> +
> void AalMediaPlayerService::updateClientSignals()
> {
> qDebug() << Q_FUNC_INFO;
> === added file 'tests/integration/audiorole/tst_audiorole.cpp'
> --- tests/integration/audiorole/tst_audiorole.cpp 1970-01-01 00:00:00 +0000
> +++ tests/integration/audiorole/tst_audiorole.cpp 2016-03-21 21:02:25 +0000
> @@ -0,0 +1,142 @@
> +/*
> + * Copyright (C) 2016 Canonical, Ltd.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU Lesser General Public License as published by
> + * the Free Software Foundation; version 3.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public License
> + * along with this program. If not, see <http://www.gnu.org/licenses/>.
> + */
> +
> +#include "tst_audiorole.h"
> +
> +#include <unistd.h>
> +
> +#include <QMediaPlayer>
> +#include <QMediaPlaylist>
> +
> +#include <QtTest/QtTest>
> +
> +void tst_AudioRole::initTestCase()
> +{
> +}
> +
> +void tst_AudioRole::cleanupTestCase()
> +{
> +}
> +
> +void tst_AudioRole::init()
> +{
> + // NOTE: This sleep is currently needed in order to give media-hub a bit of time
> + // between our different tests to cleanup and come back in a state where it can
> + // respond to our requests.
> + sleep(1);
Is this not a bug on media-hub side? It should be able to handle the calls in any case.
> +}
> +
> +void tst_AudioRole::verifyAudioRolePlays()
> +{
> + QMediaPlayer *player = new QMediaPlayer;
> + const QString fullFileUri {"file://" + QFINDTESTDATA("testdata/Ubuntu.ogg")};
> + const QMediaContent content {QUrl{fullFileUri}};
> + qDebug() << "Trying to play file:" << fullFileUri;
> + player->setMedia(content);
> + QSignalSpy spy(player, SIGNAL(audioRoleChanged(QAudio::Role)));
> + player->setAudioRole(QAudio::MusicRole);
> + QCOMPARE(spy.count(), 1);
> +
> + player->play();
> + sleep(1);
> + qDebug() << "player->error():" << player->error();
> + qDebug() << "player->mediaStatus():" << player->mediaStatus();
> + QCOMPARE(player->audioRole(), QAudio::MusicRole);
> + QVERIFY(player->mediaStatus() != QMediaPlayer::InvalidMedia);
> + QCOMPARE(player->state(), QMediaPlayer::State::PlayingState);
> +}
> +
> +void tst_AudioRole::verifyVideoRolePlays()
> +{
> + QMediaPlayer *player = new QMediaPlayer;
> + const QString fullFileUri {"file://" + QFINDTESTDATA("testdata/Ubuntu.ogg")};
> + const QMediaContent content {QUrl{fullFileUri}};
> + qDebug() << "Trying to play file:" << fullFileUri;
> + player->setMedia(content);
> + QSignalSpy spy(player, SIGNAL(audioRoleChanged(QAudio::Role)));
> + player->setAudioRole(QAudio::VideoRole);
> + QCOMPARE(spy.count(), 1);
> +
> + player->play();
> + sleep(1);
> + qDebug() << "player->error():" << player->error();
> + qDebug() << "player->mediaStatus():" << player->mediaStatus();
> + QCOMPARE(player->audioRole(), QAudio::VideoRole);
> + QVERIFY(player->mediaStatus() != QMediaPlayer::InvalidMedia);
> + QCOMPARE(player->state(), QMediaPlayer::State::PlayingState);
> +}
> +
> +void tst_AudioRole::verifyAlarmRolePlays()
> +{
> + QMediaPlayer *player = new QMediaPlayer;
> + const QString fullFileUri {"file://" + QFINDTESTDATA("testdata/Ubuntu.ogg")};
> + const QMediaContent content {QUrl{fullFileUri}};
> + qDebug() << "Trying to play file:" << fullFileUri;
> + player->setMedia(content);
> + QSignalSpy spy(player, SIGNAL(audioRoleChanged(QAudio::Role)));
> + player->setAudioRole(QAudio::AlarmRole);
> + QCOMPARE(spy.count(), 1);
> +
> + player->play();
> + sleep(1);
> + qDebug() << "player->error():" << player->error();
> + qDebug() << "player->mediaStatus():" << player->mediaStatus();
> + QCOMPARE(player->audioRole(), QAudio::AlarmRole);
> + QVERIFY(player->mediaStatus() != QMediaPlayer::InvalidMedia);
> + QCOMPARE(player->state(), QMediaPlayer::State::PlayingState);
> +}
> +
> +void tst_AudioRole::verifyNotificationRolePlays()
> +{
> + QMediaPlayer *player = new QMediaPlayer;
> + const QString fullFileUri {"file://" + QFINDTESTDATA("testdata/Ubuntu.ogg")};
> + const QMediaContent content {QUrl{fullFileUri}};
> + qDebug() << "Trying to play file:" << fullFileUri;
> + player->setMedia(content);
> + QSignalSpy spy(player, SIGNAL(audioRoleChanged(QAudio::Role)));
> + player->setAudioRole(QAudio::NotificationRole);
> + QCOMPARE(spy.count(), 1);
> +
> + player->play();
> + sleep(1);
> + qDebug() << "player->error():" << player->error();
> + qDebug() << "player->mediaStatus():" << player->mediaStatus();
> + QCOMPARE(player->audioRole(), QAudio::NotificationRole);
> + QVERIFY(player->mediaStatus() != QMediaPlayer::InvalidMedia);
> + QCOMPARE(player->state(), QMediaPlayer::State::PlayingState);
> +}
> +
> +void tst_AudioRole::verifyVoiceCommunicationRolePlays()
> +{
> + QMediaPlayer *player = new QMediaPlayer;
> + const QString fullFileUri {"file://" + QFINDTESTDATA("testdata/Ubuntu.ogg")};
> + const QMediaContent content {QUrl{fullFileUri}};
> + qDebug() << "Trying to play file:" << fullFileUri;
> + player->setMedia(content);
> + QSignalSpy spy(player, SIGNAL(audioRoleChanged(QAudio::Role)));
> + player->setAudioRole(QAudio::VoiceCommunicationRole);
> + QCOMPARE(spy.count(), 1);
> +
> + player->play();
> + sleep(1);
> + qDebug() << "player->error():" << player->error();
> + qDebug() << "player->mediaStatus():" << player->mediaStatus();
> + QCOMPARE(player->audioRole(), QAudio::VoiceCommunicationRole);
> + QVERIFY(player->mediaStatus() != QMediaPlayer::InvalidMedia);
> + QCOMPARE(player->state(), QMediaPlayer::State::PlayingState);
> +}
> +
> +QTEST_GUILESS_MAIN(tst_AudioRole)
--
https://code.launchpad.net/~phablet-team/qtubuntu-media/audio_role/+merge/287368
Your team Ubuntu Phablet Team is subscribed to branch lp:qtubuntu-media.
More information about the Ubuntu-reviews
mailing list