1
=== modified file 'include/server/mir/frontend/client_buffers.h'
2
--- include/server/mir/frontend/client_buffers.h	2017-01-30 08:13:20 +0000
3
+++ include/server/mir/frontend/client_buffers.h	2017-03-02 12:43:44 +0000
4
@@ -32,7 +32,7 @@
5
32
public:
32
public:
6
33
    virtual graphics::BufferID add_buffer(std::shared_ptr<graphics::Buffer> const& properties) = 0;
33
    virtual graphics::BufferID add_buffer(std::shared_ptr<graphics::Buffer> const& properties) = 0;
7
34
    virtual void remove_buffer(graphics::BufferID id) = 0;
34
    virtual void remove_buffer(graphics::BufferID id) = 0;
9
35
    virtual std::shared_ptr<graphics::Buffer>& operator[](graphics::BufferID) = 0;
35
    virtual std::shared_ptr<graphics::Buffer> get(graphics::BufferID) const = 0;
10
36
    virtual void send_buffer(graphics::BufferID id) = 0;
36
    virtual void send_buffer(graphics::BufferID id) = 0;
11
37
    virtual void receive_buffer(graphics::BufferID id) = 0;
37
    virtual void receive_buffer(graphics::BufferID id) = 0;
12
38
38
13
39
39
14
=== modified file 'src/server/compositor/buffer_map.cpp'
15
--- src/server/compositor/buffer_map.cpp	2017-01-30 08:13:20 +0000
16
+++ src/server/compositor/buffer_map.cpp	2017-03-02 12:43:44 +0000
17
@@ -91,7 +91,7 @@
18
91
        it->second.owner = Owner::server;
91
        it->second.owner = Owner::server;
19
92
}
92
}
20
93
93
22
94
std::shared_ptr<mg::Buffer>& mc::BufferMap::operator[](mg::BufferID id)
94
std::shared_ptr<mg::Buffer> mc::BufferMap::get(mg::BufferID id) const
23
95
{
95
{
24
96
    std::unique_lock<decltype(mutex)> lk(mutex);
96
    std::unique_lock<decltype(mutex)> lk(mutex);
25
97
    return checked_buffers_find(id, lk)->second.buffer;
97
    return checked_buffers_find(id, lk)->second.buffer;
26
@@ -105,3 +105,12 @@
27
105
        BOOST_THROW_EXCEPTION(std::logic_error("cannot find buffer by id"));
105
        BOOST_THROW_EXCEPTION(std::logic_error("cannot find buffer by id"));
28
106
    return it;
106
    return it;
29
107
}
107
}
30
108
31
109
mc::BufferMap::Map::const_iterator mc::BufferMap::checked_buffers_find(
32
110
    mg::BufferID id, std::unique_lock<std::mutex> const&) const
33
111
{
34
112
    auto it = buffers.find(id);
35
113
    if (it == buffers.end())
36
114
        BOOST_THROW_EXCEPTION(std::logic_error("cannot find buffer by id"));
37
115
    return it;
38
116
}
39
108
117
40
=== modified file 'src/server/compositor/buffer_map.h'
41
--- src/server/compositor/buffer_map.h	2017-01-30 08:13:20 +0000
42
+++ src/server/compositor/buffer_map.h	2017-03-02 12:43:44 +0000
43
@@ -39,7 +39,7 @@
44
39
    void receive_buffer(graphics::BufferID id) override;
39
    void receive_buffer(graphics::BufferID id) override;
45
40
    void send_buffer(graphics::BufferID id) override;
40
    void send_buffer(graphics::BufferID id) override;
46
41
41
48
42
    std::shared_ptr<graphics::Buffer>& operator[](graphics::BufferID) override;
42
    std::shared_ptr<graphics::Buffer> get(graphics::BufferID) const override;
49
43
    
43
    
50
44
private:
44
private:
51
45
    std::mutex mutable mutex;
45
    std::mutex mutable mutex;
52
@@ -54,6 +54,7 @@
53
54
    //used to keep strong reference
54
    //used to keep strong reference
54
55
    Map buffers;
55
    Map buffers;
55
56
    Map::iterator checked_buffers_find(graphics::BufferID, std::unique_lock<std::mutex> const&);
56
    Map::iterator checked_buffers_find(graphics::BufferID, std::unique_lock<std::mutex> const&);
56
57
    Map::const_iterator checked_buffers_find(graphics::BufferID, std::unique_lock<std::mutex> const&) const;
57
57
58
58
58
    //would be better to schedule the async buffer callbacks in the ipc subsystem,
59
    //would be better to schedule the async buffer callbacks in the ipc subsystem,
59
59
    //instead of driving from within the compositor threads (LP: #1395421)
60
    //instead of driving from within the compositor threads (LP: #1395421)
60
60
61
61
=== modified file 'src/server/compositor/stream.cpp'
62
--- src/server/compositor/stream.cpp	2017-02-21 21:43:12 +0000
63
+++ src/server/compositor/stream.cpp	2017-03-02 12:43:44 +0000
64
@@ -97,7 +97,7 @@
65
97
        std::lock_guard<decltype(mutex)> lk(mutex); 
97
        std::lock_guard<decltype(mutex)> lk(mutex); 
66
98
        first_frame_posted = true;
98
        first_frame_posted = true;
67
99
        buffers->receive_buffer(buffer->id());
99
        buffers->receive_buffer(buffer->id());
69
100
        schedule->schedule((*buffers)[buffer->id()]);
100
        schedule->schedule(buffers->get(buffer->id()));
70
101
        if (!associated_buffers.empty() && (client_owned_buffer_count(lk) == 0))
101
        if (!associated_buffers.empty() && (client_owned_buffer_count(lk) == 0))
71
102
            drop_policy->swap_now_blocking();
102
            drop_policy->swap_now_blocking();
72
103
    }
103
    }
73
@@ -247,7 +247,7 @@
74
247
    else
247
    else
75
248
    {
248
    {
76
249
        for (auto it : associated_buffers)
249
        for (auto it : associated_buffers)
78
250
            if ((*buffers)[it]->pixel_format() != mir_pixel_format_argb_8888)
250
            if (buffers->get(it)->pixel_format() != mir_pixel_format_argb_8888)
79
251
                return false;
251
                return false;
80
252
    }
252
    }
81
253
    return true;
253
    return true;
82
254
254
83
=== modified file 'src/server/scene/application_session.cpp'
84
--- src/server/scene/application_session.cpp	2017-02-24 12:21:55 +0000
85
+++ src/server/scene/application_session.cpp	2017-03-02 12:43:44 +0000
86
@@ -476,7 +476,7 @@
87
476
476
88
477
std::shared_ptr<mg::Buffer> ms::ApplicationSession::get_buffer(mg::BufferID id)
477
std::shared_ptr<mg::Buffer> ms::ApplicationSession::get_buffer(mg::BufferID id)
89
478
{
478
{
91
479
    return (*buffers)[id];
479
    return buffers->get(id);
92
480
}
480
}
93
481
481
94
482
void ms::ApplicationSession::send_error(mir::ClientVisibleError const& error)
482
void ms::ApplicationSession::send_error(mir::ClientVisibleError const& error)
95
483
483
96
=== modified file 'tests/include/mir/test/doubles/stub_buffer_stream_factory.h'
97
--- tests/include/mir/test/doubles/stub_buffer_stream_factory.h	2017-01-18 02:29:37 +0000
98
+++ tests/include/mir/test/doubles/stub_buffer_stream_factory.h	2017-03-02 12:43:44 +0000
99
@@ -39,7 +39,7 @@
100
39
    void remove_buffer(graphics::BufferID) override
39
    void remove_buffer(graphics::BufferID) override
101
40
    {
40
    {
102
41
    }
41
    }
104
42
    std::shared_ptr<graphics::Buffer>& operator[](graphics::BufferID) override
42
    std::shared_ptr<graphics::Buffer> get(graphics::BufferID) const override
105
43
    {
43
    {
106
44
        return buffer;
44
        return buffer;
107
45
    }
45
    }
108
46
46
109
=== modified file 'tests/integration-tests/test_buffer_scheduling.cpp'
110
--- tests/integration-tests/test_buffer_scheduling.cpp	2017-01-30 05:18:36 +0000
111
+++ tests/integration-tests/test_buffer_scheduling.cpp	2017-03-02 12:43:44 +0000
112
@@ -485,7 +485,7 @@
113
485
                if (!submit_stream)
485
                if (!submit_stream)
114
486
                    return;
486
                    return;
115
487
                mg::BufferID id{static_cast<unsigned int>(buffer.buffer_id())};
487
                mg::BufferID id{static_cast<unsigned int>(buffer.buffer_id())};
117
488
                submit_stream->submit_buffer((*map)[id]);
488
                submit_stream->submit_buffer(map->get(id));
118
489
            });
489
            });
119
490
        ipc->on_allocate(
490
        ipc->on_allocate(
120
491
            [this](geom::Size sz)
491
            [this](geom::Size sz)
121
492
492
122
=== modified file 'tests/integration-tests/test_submit_buffer.cpp'
123
--- tests/integration-tests/test_submit_buffer.cpp	2017-01-30 08:13:20 +0000
124
+++ tests/integration-tests/test_submit_buffer.cpp	2017-03-02 12:43:44 +0000
125
@@ -142,7 +142,7 @@
126
142
            {
142
            {
127
143
            } 
143
            } 
128
144
144
130
145
            std::shared_ptr<mg::Buffer>& operator[](mg::BufferID) override
145
            std::shared_ptr<mg::Buffer> get(mg::BufferID) const override
131
146
            {
146
            {
132
147
                return b;
147
                return b;
133
148
            }
148
            }
134
149
149
135
=== modified file 'tests/unit-tests/compositor/test_client_buffers.cpp'
136
--- tests/unit-tests/compositor/test_client_buffers.cpp	2017-01-30 08:13:20 +0000
137
+++ tests/unit-tests/compositor/test_client_buffers.cpp	2017-03-02 12:43:44 +0000
138
@@ -49,7 +49,7 @@
139
49
TEST_F(ClientBuffers, access_of_nonexistent_buffer_throws)
49
TEST_F(ClientBuffers, access_of_nonexistent_buffer_throws)
140
50
{
50
{
141
51
    EXPECT_THROW({
51
    EXPECT_THROW({
143
52
        auto buffer = map[stub_buffer.id()];
52
        auto buffer = map.get(stub_buffer.id());
144
53
    }, std::logic_error);
53
    }, std::logic_error);
145
54
}
54
}
146
55
55
147
@@ -63,13 +63,13 @@
148
63
TEST_F(ClientBuffers, can_access_once_added)
63
TEST_F(ClientBuffers, can_access_once_added)
149
64
{
64
{
150
65
    auto id = map.add_buffer(mt::fake_shared(stub_buffer));
65
    auto id = map.add_buffer(mt::fake_shared(stub_buffer));
152
66
    EXPECT_THAT(map[id].get(), Eq(&stub_buffer));
66
    EXPECT_THAT(map.get(id).get(), Eq(&stub_buffer));
153
67
}
67
}
154
68
68
155
69
TEST_F(ClientBuffers, sends_update_msg_to_send_buffer)
69
TEST_F(ClientBuffers, sends_update_msg_to_send_buffer)
156
70
{
70
{
157
71
    auto id = map.add_buffer(mt::fake_shared(stub_buffer));
71
    auto id = map.add_buffer(mt::fake_shared(stub_buffer));
159
72
    auto buffer = map[id];
72
    auto buffer = map.get(id);
160
73
    EXPECT_CALL(*mock_sink, update_buffer(Ref(*buffer)));
73
    EXPECT_CALL(*mock_sink, update_buffer(Ref(*buffer)));
161
74
    map.send_buffer(id);
74
    map.send_buffer(id);
162
75
}
75
}
163
@@ -77,7 +77,7 @@
164
77
TEST_F(ClientBuffers, sends_no_update_msg_if_buffer_is_not_around)
77
TEST_F(ClientBuffers, sends_no_update_msg_if_buffer_is_not_around)
165
78
{
78
{
166
79
    auto id = map.add_buffer(mt::fake_shared(stub_buffer));
79
    auto id = map.add_buffer(mt::fake_shared(stub_buffer));
168
80
    auto buffer = map[id];
80
    auto buffer = map.get(id);
169
81
81
170
82
    EXPECT_CALL(*mock_sink, remove_buffer(Ref(*buffer)));
82
    EXPECT_CALL(*mock_sink, remove_buffer(Ref(*buffer)));
171
83
    map.remove_buffer(id);
83
    map.remove_buffer(id);
172
84
84
173
=== modified file 'tests/unit-tests/compositor/test_dropping_schedule.cpp'
174
--- tests/unit-tests/compositor/test_dropping_schedule.cpp	2017-01-18 02:29:37 +0000
175
+++ tests/unit-tests/compositor/test_dropping_schedule.cpp	2017-03-02 12:43:44 +0000
176
@@ -38,9 +38,8 @@
177
38
    MOCK_METHOD1(remove_buffer, void(mg::BufferID id));
38
    MOCK_METHOD1(remove_buffer, void(mg::BufferID id));
178
39
    MOCK_METHOD1(send_buffer, void(mg::BufferID id));
39
    MOCK_METHOD1(send_buffer, void(mg::BufferID id));
179
40
    MOCK_METHOD1(receive_buffer, void(mg::BufferID id));
40
    MOCK_METHOD1(receive_buffer, void(mg::BufferID id));
180
41
    MOCK_METHOD1(at, std::shared_ptr<mg::Buffer>&(mg::BufferID));
181
42
    MOCK_CONST_METHOD0(client_owned_buffer_count, size_t());
41
    MOCK_CONST_METHOD0(client_owned_buffer_count, size_t());
183
43
    std::shared_ptr<mg::Buffer>& operator[](mg::BufferID id) { return at(id); }
42
    MOCK_CONST_METHOD1(get, std::shared_ptr<mg::Buffer>(mg::BufferID));
184
44
};
43
};
185
45
44
186
46
struct DroppingSchedule : Test
45
struct DroppingSchedule : Test
187
47
46
188
=== modified file 'tests/unit-tests/compositor/test_multi_monitor_arbiter.cpp'
189
--- tests/unit-tests/compositor/test_multi_monitor_arbiter.cpp	2017-02-15 19:14:27 +0000
190
+++ tests/unit-tests/compositor/test_multi_monitor_arbiter.cpp	2017-03-02 12:43:44 +0000
191
@@ -39,9 +39,8 @@
192
39
    MOCK_METHOD1(remove_buffer, void(mg::BufferID id));
39
    MOCK_METHOD1(remove_buffer, void(mg::BufferID id));
193
40
    MOCK_METHOD1(receive_buffer, void(mg::BufferID id));
40
    MOCK_METHOD1(receive_buffer, void(mg::BufferID id));
194
41
    MOCK_METHOD1(send_buffer, void(mg::BufferID id));
41
    MOCK_METHOD1(send_buffer, void(mg::BufferID id));
195
42
    MOCK_METHOD1(at, std::shared_ptr<mg::Buffer>&(mg::BufferID));
196
43
    MOCK_CONST_METHOD0(client_owned_buffer_count, size_t());
42
    MOCK_CONST_METHOD0(client_owned_buffer_count, size_t());
198
44
    std::shared_ptr<mg::Buffer>& operator[](mg::BufferID id) { return at(id); }
43
    MOCK_CONST_METHOD1(get, std::shared_ptr<mg::Buffer>(mg::BufferID));
199
45
};
44
};
200
46
45
201
47
struct FixedSchedule : mc::Schedule
46
struct FixedSchedule : mc::Schedule
202
48
47
203
=== modified file 'tests/unit-tests/compositor/test_stream.cpp'
204
--- tests/unit-tests/compositor/test_stream.cpp	2017-01-18 02:29:37 +0000
205
+++ tests/unit-tests/compositor/test_stream.cpp	2017-03-02 12:43:44 +0000
206
@@ -64,9 +64,9 @@
207
64
    }
64
    }
208
65
    void send_buffer(mg::BufferID id)
65
    void send_buffer(mg::BufferID id)
209
66
    {
66
    {
211
67
        sink.send_buffer(mf::BufferStreamId{33}, *operator[](id), mg::BufferIpcMsgType::update_msg);
67
        sink.send_buffer(mf::BufferStreamId{33}, *get(id), mg::BufferIpcMsgType::update_msg);
212
68
    }
68
    }
214
69
    std::shared_ptr<mg::Buffer>& operator[](mg::BufferID id)
69
    std::shared_ptr<mg::Buffer> get(mg::BufferID id) const
215
70
    {
70
    {
216
71
        auto it = std::find_if(buffers.begin(), buffers.end(),
71
        auto it = std::find_if(buffers.begin(), buffers.end(),
217
72
            [id](std::shared_ptr<mg::Buffer> const& b)
72
            [id](std::shared_ptr<mg::Buffer> const& b)
218
73
73
219
=== modified file 'tests/unit-tests/scene/test_surface_stack.cpp'
220
--- tests/unit-tests/scene/test_surface_stack.cpp	2017-02-15 14:45:41 +0000
221
+++ tests/unit-tests/scene/test_surface_stack.cpp	2017-03-02 12:43:44 +0000
222
@@ -292,7 +292,7 @@
223
292
292
224
293
    struct StubBuffers : mtd::StubClientBuffers
293
    struct StubBuffers : mtd::StubClientBuffers
225
294
    {
294
    {
227
295
        std::shared_ptr<mg::Buffer>& operator[](mg::BufferID) override
295
        std::shared_ptr<mg::Buffer> get(mg::BufferID) const override
228
296
        {
296
        {
229
297
            return buffer;
297
            return buffer;
230
298
        }
298
        }
Status:	Merged
Approved by:	Daniel van Vugt on 2017-03-03
Approved revision:	no longer in the source branch.
Merged at revision:	4064
Proposed branch:	lp:~vanvugt/mir/fix-racy-BufferMap-operator
Merge into:	lp:mir
Diff against target:	230 lines (+28/-20) 13 files modified include/server/mir/frontend/client_buffers.h (+1/-1) src/server/compositor/buffer_map.cpp (+10/-1) src/server/compositor/buffer_map.h (+2/-1) src/server/compositor/stream.cpp (+2/-2) src/server/scene/application_session.cpp (+1/-1) tests/include/mir/test/doubles/stub_buffer_stream_factory.h (+1/-1) tests/integration-tests/test_buffer_scheduling.cpp (+1/-1) tests/integration-tests/test_submit_buffer.cpp (+1/-1) tests/unit-tests/compositor/test_client_buffers.cpp (+4/-4) tests/unit-tests/compositor/test_dropping_schedule.cpp (+1/-2) tests/unit-tests/compositor/test_multi_monitor_arbiter.cpp (+1/-2) tests/unit-tests/compositor/test_stream.cpp (+2/-2) tests/unit-tests/scene/test_surface_stack.cpp (+1/-1)
To merge this branch:	bzr merge lp:~vanvugt/mir/fix-racy-BufferMap-operator
Related bugs:	Link a bug report
Reviewer	Review Type	Date Requested	Status
Mir CI Bot	continuous-integration	2017-03-02	Approve on 2017-03-03
Alan Griffiths		2017-03-02	Approve on 2017-03-02
Review via email: mp+318751@code.launchpad.net