Mir

Merge lp:~albaguirre/mir/fix-1501927 into lp:mir

  1. fix-1501927
  2. Merge into development-branch
Proposed by Alberto Aguirre
Status: Merged
Approved by: Alexandros Frantzis
Approved revision: no longer in the source branch.
Merged at revision: 3001
Proposed branch: lp:~albaguirre/mir/fix-1501927
Merge into: lp:mir
Diff against target: 171 lines (+85/-22)
2 files modified
src/platforms/android/server/display.cpp (+14/-15)
tests/unit-tests/graphics/android/test_display.cpp (+71/-7)
To merge this branch: bzr merge lp:~albaguirre/mir/fix-1501927
Reviewer Review Type Date Requested Status
Alexandros Frantzis (community) Approve
Daniel van Vugt Approve
PS Jenkins bot (community) continuous-integration Approve
Review via email: mp+273146@code.launchpad.net

Commit message

[android] Update display sync group during configure where it's safe to delete display buffers. (LP: #1501927)

Fix removing display buffers during for_each_display_sync_group, as that can be called from
threads other than the compositor thread, which may still be using the display buffers. Instead,
remove unused display buffers during the configure phase where it's safe to do so as the compositor
thread is stopped before applying a new configuration.

Description of the change

[android] Update display sync group during configure where it's safe to delete display buffers.

Fix removing display buffers during for_each_display_sync_group, as that can be called from
threads other than the compositor thread, which may still be using the display buffers. Instead,
remove unused display buffers during the configure phase where it's safe to do so as the compositor
thread is stopped before applying a new configuration.

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

FAILED: Continuous integration, rev:2983
http://jenkins.qa.ubuntu.com/job/mir-ci/5104/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/4230
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-vivid-amd64-build/3137
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/4172/console
    None: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-wily-touch/130/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-wily-amd64-ci/1258
        deb: http://jenkins.qa.ubuntu.com/job/mir-wily-amd64-ci/1258/artifact/work/output/*zip*/output.zip
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-wily-i386-ci/130/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/4173/console
    None: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-wily-armhf/131/console

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/mir-ci/5104/rebuild

review: Needs Fixing (continuous-integration)
Revision history for this message
Andreas Pokorny (andreas-pokorny) wrote :

no test?

Revision history for this message
Daniel van Vugt (vanvugt) wrote :

I sometimes wonder about tests for crash fixes. Sometimes they're not feasible because crashes occur in the implementation which is stubbed or mocked. It's not impossible to write tests, but sometimes it's not feasible to add all the testing infrastructure required to emulate the root cause of the problem, in a short time.

Similarly: https://code.launchpad.net/~vanvugt/mir/fix-1493721/+merge/273021

I'd probably approve this one if Jenkins was happier with it.

review: Abstain
Revision history for this message
Chris Halse Rogers (raof) wrote :

Code change seems sensible, but yeah, no test :)

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

FAILED: Continuous integration, rev:2984
http://jenkins.qa.ubuntu.com/job/mir-ci/5172/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/4316
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-vivid-amd64-build/3223
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/4258/console
    None: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-wily-touch/216/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-wily-amd64-ci/1326
        deb: http://jenkins.qa.ubuntu.com/job/mir-wily-amd64-ci/1326/artifact/work/output/*zip*/output.zip
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-wily-i386-ci/216/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/4259/console
    None: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-wily-armhf/217/console

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/mir-ci/5172/rebuild

review: Needs Fixing (continuous-integration)
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :

PASSED: Continuous integration, rev:2985
http://jenkins.qa.ubuntu.com/job/mir-ci/5179/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/4323
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-vivid-amd64-build/3230
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/4265
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-wily-touch/223/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-wily-amd64-ci/1333
        deb: http://jenkins.qa.ubuntu.com/job/mir-wily-amd64-ci/1333/artifact/work/output/*zip*/output.zip
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-wily-i386-ci/223/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/4266
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/4266/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-touch/6925
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/24008
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-wily-armhf/224/console

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/mir-ci/5179/rebuild

review: Approve (continuous-integration)
Revision history for this message
Daniel van Vugt (vanvugt) wrote :

Cool

review: Approve
Revision history for this message
Alexandros Frantzis (afrantzis) wrote :

Looks good.

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'src/platforms/android/server/display.cpp'
2--- src/platforms/android/server/display.cpp 2015-10-05 08:19:39 +0000
3+++ src/platforms/android/server/display.cpp 2015-10-06 18:42:44 +0000
4@@ -221,21 +221,6 @@
5 void mga::Display::for_each_display_sync_group(std::function<void(mg::DisplaySyncGroup&)> const& f)
6 {
7 std::lock_guard<decltype(configuration_mutex)> lock{configuration_mutex};
8- update_configuration(lock);
9- if ((config.external().connected) && !displays.display_present(mga::DisplayName::external))
10- displays.add(mga::DisplayName::external,
11- create_display_buffer(
12- display_device,
13- mga::DisplayName::external,
14- *display_buffer_builder,
15- config.external(),
16- gl_program_factory,
17- gl_context,
18- config.external().top_left - origin,
19- overlay_option));
20- if ((!config.external().connected) && displays.display_present(mga::DisplayName::external))
21- displays.remove(mga::DisplayName::external);
22-
23 f(displays);
24 }
25
26@@ -254,6 +239,20 @@
27
28 std::lock_guard<decltype(configuration_mutex)> lock{configuration_mutex};
29
30+ if ((config.external().connected) && !displays.display_present(mga::DisplayName::external))
31+ displays.add(mga::DisplayName::external,
32+ create_display_buffer(
33+ display_device,
34+ mga::DisplayName::external,
35+ *display_buffer_builder,
36+ config.external(),
37+ gl_program_factory,
38+ gl_context,
39+ config.external().top_left - origin,
40+ overlay_option));
41+ if ((!config.external().connected) && displays.display_present(mga::DisplayName::external))
42+ displays.remove(mga::DisplayName::external);
43+
44 new_configuration.for_each_output([this](mg::DisplayConfigurationOutput const& output)
45 {
46 if (output.current_format != config[output.id].current_format)
47
48=== modified file 'tests/unit-tests/graphics/android/test_display.cpp'
49--- tests/unit-tests/graphics/android/test_display.cpp 2015-10-05 17:36:04 +0000
50+++ tests/unit-tests/graphics/android/test_display.cpp 2015-10-06 18:42:44 +0000
51@@ -665,6 +665,8 @@
52 group_count++;
53 group.for_each_display_buffer([&](mg::DisplayBuffer&) {db_count++;});
54 };
55+ auto conf = display.configuration();
56+ display.configure(*conf);
57 display.for_each_display_sync_group(db_group_counter);
58 EXPECT_THAT(group_count, Eq(1));
59 EXPECT_THAT(db_count, Eq(2));
60@@ -672,6 +674,8 @@
61 //hotplug external away
62 external_connected = false;
63 hotplug_fn();
64+ conf = display.configuration();
65+ display.configure(*conf);
66
67 group_count = 0;
68 db_count = 0;
69@@ -682,6 +686,8 @@
70 //hotplug external back
71 external_connected = true;
72 hotplug_fn();
73+ conf = display.configuration();
74+ display.configure(*conf);
75
76 group_count = 0;
77 db_count = 0;
78@@ -709,11 +715,20 @@
79 {20,20}, {4,4}, mir_pixel_format_abgr_8888, 50.0f, external_connected};
80 }));
81
82-
83- testing::InSequence seq;
84- EXPECT_CALL(mock_config, power_mode(mga::DisplayName::primary, _));
85- EXPECT_CALL(mock_config, power_mode(mga::DisplayName::external, mir_power_mode_on));
86- EXPECT_CALL(mock_config, power_mode(mga::DisplayName::external, mir_power_mode_on));
87+ InSequence seq;
88+
89+ //At construction, we expect external display to be connected
90+ EXPECT_CALL(mock_config, power_mode(mga::DisplayName::primary, _));
91+ EXPECT_CALL(mock_config, power_mode(mga::DisplayName::external, mir_power_mode_on));
92+
93+ //Unplugging
94+ EXPECT_CALL(mock_config, power_mode(mga::DisplayName::primary, _));
95+
96+ //Hotplugging external back again
97+ EXPECT_CALL(mock_config, power_mode(mga::DisplayName::external, mir_power_mode_on));
98+ EXPECT_CALL(mock_config, power_mode(mga::DisplayName::primary, _));
99+
100+ //Expected after destructor
101 EXPECT_CALL(mock_config, power_mode(mga::DisplayName::primary, _));
102 EXPECT_CALL(mock_config, power_mode(mga::DisplayName::external, mir_power_mode_off));
103 });
104@@ -728,12 +743,15 @@
105 //hotplug external away
106 external_connected = false;
107 hotplug_fn();
108- display.for_each_display_sync_group([](mg::DisplaySyncGroup&){});
109+ auto conf = display.configuration();
110+ display.configure(*conf);
111
112 //hotplug external back
113 external_connected = true;
114 hotplug_fn();
115- display.for_each_display_sync_group([](mg::DisplaySyncGroup&){});
116+ conf = display.configuration();
117+ display.configure(*conf);
118+
119 }
120
121 TEST_F(Display, configures_external_display)
122@@ -925,3 +943,49 @@
123 });
124 });
125 }
126+
127+TEST_F(Display, does_not_remove_dbs_when_enumerating_display_groups)
128+{
129+ using namespace testing;
130+ std::function<void()> hotplug_fn = []{};
131+ bool external_connected = true;
132+ stub_db_factory->with_next_config([&](mtd::MockHwcConfiguration& mock_config)
133+ {
134+ ON_CALL(mock_config, active_config_for(mga::DisplayName::primary))
135+ .WillByDefault(Return(mtd::StubDisplayConfigurationOutput{
136+ mg::DisplayConfigurationOutputId{0}, {20,20}, {4,4}, mir_pixel_format_abgr_8888, 50.0f, true}));
137+
138+ ON_CALL(mock_config, active_config_for(mga::DisplayName::external))
139+ .WillByDefault(Invoke([&](mga::DisplayName)
140+ {
141+ return mtd::StubDisplayConfigurationOutput{mg::DisplayConfigurationOutputId{1},
142+ {20,20}, {4,4}, mir_pixel_format_abgr_8888, 50.0f, external_connected};
143+ }));
144+ EXPECT_CALL(mock_config, subscribe_to_config_changes(_,_))
145+ .WillOnce(DoAll(SaveArg<0>(&hotplug_fn), Return(std::make_shared<char>('2'))));
146+ });
147+
148+ mga::Display display(
149+ stub_db_factory,
150+ stub_gl_program_factory,
151+ stub_gl_config,
152+ null_display_report,
153+ mga::OverlayOptimization::enabled);
154+
155+ auto db_count = 0;
156+ auto db_group_counter = [&](mg::DisplaySyncGroup& group) {
157+ group.for_each_display_buffer([&](mg::DisplayBuffer&) {db_count++;});
158+ };
159+
160+ display.for_each_display_sync_group(db_group_counter);
161+ auto const expected_buffer_count = 2;
162+ EXPECT_THAT(db_count, Eq(expected_buffer_count));
163+
164+ //hotplug external away
165+ external_connected = false;
166+ hotplug_fn();
167+
168+ db_count = 0;
169+ display.for_each_display_sync_group(db_group_counter);
170+ EXPECT_THAT(db_count, Eq(expected_buffer_count));
171+}

Subscribers

People subscribed via source and target branches