qtubuntu-media

Merge lp:~thomas-voss/qtubuntu-media/reuse-precreated-objects into lp:qtubuntu-media

  1. reuse-precreated-objects
  2. Merge into trunk
Proposed by Thomas Voß
Status: Merged
Approved by: Jim Hodapp
Approved revision: 65
Merged at revision: 55
Proposed branch: lp:~thomas-voss/qtubuntu-media/reuse-precreated-objects
Merge into: lp:qtubuntu-media
Diff against target: 222 lines (+36/-42)
4 files modified
src/aal/aalmediaplayerservice.cpp (+27/-22)
src/aal/aalmediaplayerservice.h (+0/-3)
src/aal/aalmediaplayerserviceplugin.cpp (+2/-5)
unittests/tst_mediaplayerplugin.cpp (+7/-12)
To merge this branch: bzr merge lp:~thomas-voss/qtubuntu-media/reuse-precreated-objects
Reviewer Review Type Date Requested Status
Alberto Aguirre (community) Approve
Jim Hodapp (community) code Approve
PS Jenkins bot continuous-integration Needs Fixing
Review via email: mp+240644@code.launchpad.net

Commit message

Get rid of custom ref-counting and reuse pre-created object.

Description of the change

Get rid of custom ref-counting and reuse pre-created object.

To post a comment you must log in.
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :

FAILED: Continuous integration, rev:61
http://jenkins.qa.ubuntu.com/job/qtubuntu-media-ci/71/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/qtubuntu-media-utopic-amd64-ci/22/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/qtubuntu-media-utopic-armhf-ci/22/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/qtubuntu-media-utopic-i386-ci/22/console

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/qtubuntu-media-ci/71/rebuild

review: Needs Fixing (continuous-integration)
lp:~thomas-voss/qtubuntu-media/reuse-precreated-objects updated
62. By Thomas Voß

Clean up patch.

Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :

FAILED: Continuous integration, rev:62
http://jenkins.qa.ubuntu.com/job/qtubuntu-media-ci/72/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/qtubuntu-media-utopic-amd64-ci/23/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/qtubuntu-media-utopic-armhf-ci/23/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/qtubuntu-media-utopic-i386-ci/23/console

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/qtubuntu-media-ci/72/rebuild

review: Needs Fixing (continuous-integration)
lp:~thomas-voss/qtubuntu-media/reuse-precreated-objects updated
63. By Thomas Voß

Revert change to dtor.

Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :

FAILED: Continuous integration, rev:63
http://jenkins.qa.ubuntu.com/job/qtubuntu-media-ci/73/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/qtubuntu-media-utopic-amd64-ci/24/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/qtubuntu-media-utopic-armhf-ci/24/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/qtubuntu-media-utopic-i386-ci/24/console

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/qtubuntu-media-ci/73/rebuild

review: Needs Fixing (continuous-integration)
lp:~thomas-voss/qtubuntu-media/reuse-precreated-objects updated
64. By Thomas Voß

Clean up test case and make sure that the fixture is setup cleanly for every test case.

Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :

FAILED: Continuous integration, rev:64
http://jenkins.qa.ubuntu.com/job/qtubuntu-media-ci/74/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/qtubuntu-media-utopic-amd64-ci/25/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/qtubuntu-media-utopic-armhf-ci/25/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/qtubuntu-media-utopic-i386-ci/25/console

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/qtubuntu-media-ci/74/rebuild

review: Needs Fixing (continuous-integration)
lp:~thomas-voss/qtubuntu-media/reuse-precreated-objects updated
65. By Thomas Voß

Guard setup of audio role for probably null object.

Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :

FAILED: Continuous integration, rev:65
http://jenkins.qa.ubuntu.com/job/qtubuntu-media-ci/75/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/qtubuntu-media-utopic-amd64-ci/26/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/qtubuntu-media-utopic-armhf-ci/26/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/qtubuntu-media-utopic-i386-ci/26/console

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/qtubuntu-media-ci/75/rebuild

review: Needs Fixing (continuous-integration)
Revision history for this message
Jim Hodapp (jhodapp) wrote :

Looks great, thanks!

review: Approve (code)
Revision history for this message
Alberto Aguirre (albaguirre) wrote :

I guess it's pre-existing but shouldn't AalMediaPlayerService constructor throw if it could not create a hubPlayerSession? I suppose this is because Qt wouldn't handle that very well?

 + if (m_hubPlayerSession == NULL)
78 + return QMediaPlayer::MultimediaRole;
79 +
80 return static_cast<QMediaPlayer::AudioRole>(m_hubPlayerSession->audio_stream_role().get());
81 }
82
83 void AalMediaPlayerService::setAudioRole(QMediaPlayer::AudioRole audioRole)
84 {
85 + if (m_hubPlayerSession == NULL)
86 + return;
87 +

I thought maybe that should probably be saved as a "pending to apply" setting but probably not wanted as we can have AalMediaPlayerService in a "bad" intermediate state anyway but that's all pre-existing so looks good to me.

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'src/aal/aalmediaplayerservice.cpp'
2--- src/aal/aalmediaplayerservice.cpp 2014-10-31 13:06:48 +0000
3+++ src/aal/aalmediaplayerservice.cpp 2014-11-05 12:09:31 +0000
4@@ -24,7 +24,6 @@
5
6 #include <QAbstractVideoSurface>
7 #include <QTimerEvent>
8-
9 #include <QThread>
10
11 #include <qtubuntu_media_signals.h>
12@@ -63,9 +62,9 @@
13 QMediaService(parent),
14 m_hubPlayerSession(NULL),
15 m_playbackStatusChangedConnection(the_void.connect([](){})),
16+ m_mediaPlayerControl(nullptr),
17+ m_videoOutput(nullptr),
18 m_videoOutputReady(false),
19- m_mediaPlayerControlRef(0),
20- m_videoOutputRef(0),
21 m_cachedDuration(0),
22 m_mediaPlaylist(NULL)
23 #ifdef MEASURE_PERFORMANCE
24@@ -102,19 +101,17 @@
25 {
26 if (qstrcmp(name, QMediaPlayerControl_iid) == 0)
27 {
28- if (m_mediaPlayerControlRef == 0 && m_mediaPlayerControl == NULL)
29+ if (not m_mediaPlayerControl)
30 createMediaPlayerControl();
31
32- ++m_mediaPlayerControlRef;
33 return m_mediaPlayerControl;
34 }
35
36- if (qstrcmp(name, QVideoRendererControl_iid) == 0)
37+ if (qstrcmp(name, QVideoRendererControl_iid) == 0)
38 {
39- if (m_videoOutputRef == 0 && m_videoOutput == NULL)
40+ if (not m_videoOutput)
41 createVideoRendererControl();
42
43- ++m_videoOutputRef;
44 return m_videoOutput;
45 }
46
47@@ -124,21 +121,11 @@
48 void AalMediaPlayerService::releaseControl(QMediaControl *control)
49 {
50 if (control == m_mediaPlayerControl)
51- {
52- if (m_mediaPlayerControlRef > 0)
53- --m_mediaPlayerControlRef;
54-
55- if (m_mediaPlayerControlRef == 0)
56- deleteMediaPlayerControl();
57- }
58+ deleteMediaPlayerControl();
59 else if (control == m_videoOutput)
60- {
61- if (m_videoOutputRef > 0)
62- --m_videoOutputRef;
63-
64- if (m_videoOutputRef == 0)
65- deleteVideoRendererControl();
66- }
67+ deleteVideoRendererControl();
68+ else
69+ delete control;
70 }
71
72 AalMediaPlayerService::GLConsumerWrapperHybris AalMediaPlayerService::glConsumer() const
73@@ -217,11 +204,17 @@
74
75 QMediaPlayer::AudioRole AalMediaPlayerService::audioRole() const
76 {
77+ if (m_hubPlayerSession == NULL)
78+ return QMediaPlayer::MultimediaRole;
79+
80 return static_cast<QMediaPlayer::AudioRole>(m_hubPlayerSession->audio_stream_role().get());
81 }
82
83 void AalMediaPlayerService::setAudioRole(QMediaPlayer::AudioRole audioRole)
84 {
85+ if (m_hubPlayerSession == NULL)
86+ return;
87+
88 qDebug() << __PRETTY_FUNCTION__;
89 m_hubPlayerSession->audio_stream_role().set(static_cast<media::Player::AudioStreamRole>(audioRole));
90 }
91@@ -452,6 +445,9 @@
92
93 void AalMediaPlayerService::createMediaPlayerControl()
94 {
95+ if (m_hubPlayerSession == NULL)
96+ return;
97+
98 m_mediaPlayerControl = new AalMediaPlayerControl(this);
99 m_hubPlayerSession->set_playback_complete_callback([](void *context)
100 {
101@@ -463,11 +459,17 @@
102
103 void AalMediaPlayerService::createVideoRendererControl()
104 {
105+ if (m_hubPlayerSession == NULL)
106+ return;
107+
108 m_videoOutput = new AalVideoRendererControl(this);
109 }
110
111 void AalMediaPlayerService::deleteMediaPlayerControl()
112 {
113+ if (m_hubPlayerSession == NULL)
114+ return;
115+
116 m_hubPlayerSession->set_playback_complete_callback(
117 empty_playback_complete_cb,
118 nullptr);
119@@ -478,6 +480,9 @@
120
121 void AalMediaPlayerService::deleteVideoRendererControl()
122 {
123+ if (m_hubPlayerSession == NULL)
124+ return;
125+
126 m_hubPlayerSession->set_frame_available_callback(
127 empty_frame_available_cb,
128 nullptr);
129
130=== modified file 'src/aal/aalmediaplayerservice.h'
131--- src/aal/aalmediaplayerservice.h 2014-10-31 13:06:48 +0000
132+++ src/aal/aalmediaplayerservice.h 2014-11-05 12:09:31 +0000
133@@ -117,9 +117,6 @@
134 AalVideoRendererControl *m_videoOutput;
135 bool m_videoOutputReady;
136
137- int m_mediaPlayerControlRef;
138- int m_videoOutputRef;
139-
140 int64_t m_cachedDuration;
141
142 const QMediaPlaylist* m_mediaPlaylist;
143
144=== modified file 'src/aal/aalmediaplayerserviceplugin.cpp'
145--- src/aal/aalmediaplayerserviceplugin.cpp 2014-10-31 11:27:34 +0000
146+++ src/aal/aalmediaplayerserviceplugin.cpp 2014-11-05 12:09:31 +0000
147@@ -27,18 +27,15 @@
148 QMediaService* AalServicePlugin::create(QString const& key)
149 {
150 qDebug() << Q_FUNC_INFO << key;
151+
152 if (key == QLatin1String(Q_MEDIASERVICE_MEDIAPLAYER))
153 return new AalMediaPlayerService();
154- else
155- qWarning() << "Key not supported:" << key;
156
157- return 0;
158+ return nullptr;
159 }
160
161 void AalServicePlugin::release(QMediaService *service)
162 {
163- qDebug() << Q_FUNC_INFO << service;
164- Q_UNUSED(service);
165 delete service;
166 }
167
168
169=== modified file 'unittests/tst_mediaplayerplugin.cpp'
170--- unittests/tst_mediaplayerplugin.cpp 2014-10-15 10:24:08 +0000
171+++ unittests/tst_mediaplayerplugin.cpp 2014-11-05 12:09:31 +0000
172@@ -38,7 +38,7 @@
173 {
174 Q_OBJECT
175
176- shared_ptr<AalMediaPlayerService> m_service;
177+ AalMediaPlayerService *m_service;
178 AalMediaPlayerControl *m_mediaPlayerControl;
179 QMediaControl *m_playerControl;
180 QMediaControl *m_rendererControl;
181@@ -46,8 +46,9 @@
182 shared_ptr<Service> m_hubService;
183
184 private Q_SLOTS:
185- void initTestCase();
186- void cleanupTestCase();
187+ // We want the setup to be run prior to every test case to
188+ // ensure correct test isolation, see http://qt-project.org/doc/qt-5/qtest-overview.html.
189+ void init();
190
191 void tst_requestRelease();
192 void tst_newMediaPlayer();
193@@ -63,26 +64,20 @@
194 void tst_volume();
195 };
196
197-void tst_MediaPlayerPlugin::initTestCase()
198+void tst_MediaPlayerPlugin::init()
199 {
200 m_hubService.reset(new TestService());
201- m_service.reset(new AalMediaPlayerService(this));
202+ m_service = new AalMediaPlayerService(this);
203 m_service->setService(m_hubService);
204 m_player.reset(new TestPlayer());
205 m_service->setPlayer(m_player);
206+ m_playerControl = m_service->requestControl(QMediaPlayerControl_iid);
207 m_mediaPlayerControl = m_service->mediaPlayerControl();
208- m_playerControl = m_service->requestControl(QMediaPlayerControl_iid);
209 QVERIFY(m_playerControl != NULL);
210 m_rendererControl = m_service->requestControl(QVideoRendererControl_iid);
211 QVERIFY(m_rendererControl != NULL);
212 }
213
214-void tst_MediaPlayerPlugin::cleanupTestCase()
215-{
216- m_service->releaseControl(m_playerControl);
217- m_service->releaseControl(m_rendererControl);
218-}
219-
220 void tst_MediaPlayerPlugin::tst_requestRelease()
221 {
222 QMediaControl *mpControl = NULL;

Subscribers

People subscribed via source and target branches