Media Hub

Merge lp:~albaguirre/media-hub/fix-1368786 into lp:media-hub

  1. fix-1368786
  2. Merge into trunk
Proposed by Alberto Aguirre
Status: Merged
Approved by: Jim Hodapp
Approved revision: 68
Merged at revision: 66
Proposed branch: lp:~albaguirre/media-hub/fix-1368786
Merge into: lp:media-hub
Diff against target: 164 lines (+20/-52)
1 file modified
src/core/media/player_implementation.cpp (+20/-52)
To merge this branch: bzr merge lp:~albaguirre/media-hub/fix-1368786
Reviewer Review Type Date Requested Status
Jim Hodapp (community) code Approve
PS Jenkins bot continuous-integration Approve
Review via email: mp+234900@code.launchpad.net

Commit message

Fix wake lock state machine and subtle race conditions on wake lock refcounts.

Description of the change

Playing->stopped did not release the display wakelock which kept the screen on for gallery-app.

Also repeated press of play/pause of a short clip in either media-player app or gallery-app resulted in the following state transitions:

ready->pause which acquired a display wakelock
and pause->playing which acquired another display wakelock
playing->pause which releases only one count

To post a comment you must log in.
Revision history for this message
Alberto Aguirre (albaguirre) wrote :

Note: I tested for regressions of https://launchpad.net/bugs/1342351 - it still works ok, but I'd like somebody else to confirm.

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

FAILED: Continuous integration, rev:67
http://jenkins.qa.ubuntu.com/job/media-hub-ci/130/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/media-hub-utopic-amd64-ci/72
    SUCCESS: http://jenkins.qa.ubuntu.com/job/media-hub-utopic-armhf-ci/71
        deb: http://jenkins.qa.ubuntu.com/job/media-hub-utopic-armhf-ci/71/artifact/work/output/*zip*/output.zip
    FAILURE: http://jenkins.qa.ubuntu.com/job/media-hub-utopic-i386-ci/70/console

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

review: Needs Fixing (continuous-integration)
Revision history for this message
Alberto Aguirre (albaguirre) wrote :

^^-- "[ FAILED ] GStreamerEngine.get_position_duration_work (25 ms)"

Ummm it doesn't fail locally and looks unrelated to this MP.

lp:~albaguirre/media-hub/fix-1368786 updated
68. By Alberto Aguirre

empty commit to kick CI

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

PASSED: Continuous integration, rev:68
http://jenkins.qa.ubuntu.com/job/media-hub-ci/131/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/media-hub-utopic-amd64-ci/73
    SUCCESS: http://jenkins.qa.ubuntu.com/job/media-hub-utopic-armhf-ci/72
        deb: http://jenkins.qa.ubuntu.com/job/media-hub-utopic-armhf-ci/72/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/media-hub-utopic-i386-ci/71

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

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

Looks great, thanks.

review: Approve (code)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'src/core/media/player_implementation.cpp'
2--- src/core/media/player_implementation.cpp 2014-09-09 21:27:29 +0000
3+++ src/core/media/player_implementation.cpp 2014-09-17 14:45:41 +0000
4@@ -47,7 +47,7 @@
5 WAKELOCK_CLEAR_DISPLAY,
6 WAKELOCK_CLEAR_SYSTEM,
7 WAKELOCK_CLEAR_INVALID
8- };
9+ };
10
11 Private(PlayerImplementation* parent,
12 const dbus::types::ObjectPath& session_path,
13@@ -79,11 +79,8 @@
14
15 /*
16 * Wakelock state logic:
17- *
18- * PLAYING->READY: delay 4 seconds and try to clear current wakelock type
19- * PLAYING->PAUSED or PLAYING->STOPPED: delay 4 seconds and try to clear current wakelock type
20- * READY->PAUSED: request a new wakelock (system or display)
21- * PLAYING->PAUSED: delay 4 seconds and try to clear current wakelock type
22+ * PLAYING->READY or PLAYING->PAUSED or PLAYING->STOPPED: delay 4 seconds and try to clear current wakelock type
23+ * ANY STATE->PLAYING: request a new wakelock (system or display)
24 */
25 engine->state().changed().connect(
26 [parent, this](const Engine::State& state)
27@@ -107,25 +104,23 @@
28 parent->meta_data_for_current_track().set(std::get<1>(engine->track_meta_data().get()));
29 // And update our playback status.
30 parent->playback_status().set(media::Player::playing);
31- if (previous_state == Engine::State::stopped || previous_state == Engine::State::paused)
32- {
33- request_power_state();
34- }
35+ request_power_state();
36 break;
37 }
38 case Engine::State::stopped:
39 {
40 parent->playback_status().set(media::Player::stopped);
41+ if (previous_state == Engine::State::playing)
42+ {
43+ wakelock_timeout.reset(new timeout(4000, true, std::bind(&Private::clear_wakelock,
44+ this, std::placeholders::_1), current_wakelock_type()));
45+ }
46 break;
47 }
48 case Engine::State::paused:
49 {
50 parent->playback_status().set(media::Player::paused);
51- if (previous_state == Engine::State::ready)
52- {
53- request_power_state();
54- }
55- else if (previous_state == Engine::State::playing)
56+ if (previous_state == Engine::State::playing)
57 {
58 wakelock_timeout.reset(new timeout(4000, true, std::bind(&Private::clear_wakelock,
59 this, std::placeholders::_1), current_wakelock_type()));
60@@ -154,7 +149,7 @@
61 {
62 if (parent->is_video_source())
63 {
64- if (display_wakelock_count == 0)
65+ if (++display_wakelock_count == 1)
66 {
67 auto result = uscreen_session->invoke_method_synchronously<core::UScreen::keepDisplayOn, int>();
68 if (result.is_error())
69@@ -162,16 +157,10 @@
70 disp_cookie = result.value();
71 cout << "Requested new display wakelock" << endl;
72 }
73-
74- {
75- // Keep track of how many display wakelocks have been requested
76- std::lock_guard<std::mutex> lock(wakelock_mutex);
77- ++display_wakelock_count;
78- }
79 }
80 else
81 {
82- if (system_wakelock_count == 0)
83+ if (++system_wakelock_count == 1)
84 {
85 auto result = powerd_session->invoke_method_synchronously<core::Powerd::requestSysState, std::string>(sys_lock_name, static_cast<int>(1));
86 if (result.is_error())
87@@ -179,12 +168,6 @@
88 sys_cookie = result.value();
89 cout << "Requested new system wakelock" << endl;
90 }
91-
92- {
93- // Keep track of how many system wakelocks have been requested
94- std::lock_guard<std::mutex> lock(wakelock_mutex);
95- ++system_wakelock_count;
96- }
97 }
98 }
99 catch(const std::exception& e)
100@@ -204,12 +187,8 @@
101 case wakelock_clear_t::WAKELOCK_CLEAR_INACTIVE:
102 break;
103 case wakelock_clear_t::WAKELOCK_CLEAR_SYSTEM:
104- {
105- std::lock_guard<std::mutex> lock(wakelock_mutex);
106- --system_wakelock_count;
107- }
108 // Only actually clear the system wakelock once the count reaches zero
109- if (system_wakelock_count == 0)
110+ if (--system_wakelock_count == 0)
111 {
112 cout << "Clearing system wakelock" << endl;
113 powerd_session->invoke_method_synchronously<core::Powerd::clearSysState, void>(sys_cookie);
114@@ -217,12 +196,8 @@
115 }
116 break;
117 case wakelock_clear_t::WAKELOCK_CLEAR_DISPLAY:
118- {
119- std::lock_guard<std::mutex> lock(wakelock_mutex);
120- --display_wakelock_count;
121- }
122 // Only actually clear the display wakelock once the count reaches zero
123- if (display_wakelock_count == 0)
124+ if (--display_wakelock_count == 0)
125 {
126 cout << "Clearing display wakelock" << endl;
127 uscreen_session->invoke_method_synchronously<core::UScreen::removeDisplayOnRequest, void>(disp_cookie);
128@@ -250,20 +225,14 @@
129 void clear_wakelocks()
130 {
131 // Clear both types of wakelocks (display and system)
132- if (system_wakelock_count > 0)
133+ if (system_wakelock_count.load() > 0)
134 {
135- {
136- std::lock_guard<std::mutex> lock(wakelock_mutex);
137- system_wakelock_count = 1;
138- }
139+ system_wakelock_count = 1;
140 clear_wakelock(wakelock_clear_t::WAKELOCK_CLEAR_SYSTEM);
141 }
142- if (display_wakelock_count > 0)
143+ if (display_wakelock_count.load() > 0)
144 {
145- {
146- std::lock_guard<std::mutex> lock(wakelock_mutex);
147- display_wakelock_count = 1;
148- }
149+ display_wakelock_count = 1;
150 clear_wakelock(wakelock_clear_t::WAKELOCK_CLEAR_DISPLAY);
151 }
152 }
153@@ -278,9 +247,8 @@
154 std::string sys_lock_name;
155 int disp_cookie;
156 std::string sys_cookie;
157- std::mutex wakelock_mutex;
158- uint8_t system_wakelock_count;
159- uint8_t display_wakelock_count;
160+ std::atomic<int> system_wakelock_count;
161+ std::atomic<int> display_wakelock_count;
162 std::unique_ptr<timeout> wakelock_timeout;
163 Engine::State previous_state;
164 PlayerImplementation::PlayerKey key;

Subscribers

People subscribed via source and target branches