Mir

Merge lp:~kdub/mir/fix-1362444 into lp:mir

  1. fix-1362444
  2. Merge into development-branch
Proposed by Kevin DuBois
Status: Merged
Approved by: Daniel van Vugt
Approved revision: no longer in the source branch.
Merged at revision: 1882
Proposed branch: lp:~kdub/mir/fix-1362444
Merge into: lp:mir
Diff against target: 171 lines (+32/-14)
6 files modified
include/server/mir/compositor/recently_used_cache.h (+1/-0)
server-ABI-sha1sums (+1/-1)
src/server/compositor/recently_used_cache.cpp (+3/-3)
tests/unit-tests/compositor/test_buffer_queue.cpp (+10/-8)
tests/unit-tests/compositor/test_gl_texture_cache.cpp (+15/-0)
tests/unit-tests/frontend/test_client_buffer_tracker.cpp (+2/-2)
To merge this branch: bzr merge lp:~kdub/mir/fix-1362444
Reviewer Review Type Date Requested Status
Daniel van Vugt Approve
PS Jenkins bot (community) continuous-integration Approve
Alan Griffiths Approve
Review via email: mp+232565@code.launchpad.net

Commit message

correct the texture cache to not rely on 'invalid' buffer id's.

fixes: lp: #1362444

Description of the change

correct the texture cache to not rely on 'invalid' buffer id's.

fixes: lp: #1362444

note: this bug should only affect the 1st frame of the 1st client on the servers using our GL rendering implementation on the nested or mesa platforms. (so u8 and usc on ubuntu touch shouldn't be affected)

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

FAILED: Continuous integration, rev:1871
http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-ci/2538/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-utopic-i386-build/1506
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-clang-utopic-amd64-build/1512/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-utopic-touch/1489/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-utopic-amd64-ci/1060/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-utopic-armhf/426/console

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/mir-team-mir-development-branch-ci/2538/rebuild

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

FAILED: Continuous integration, rev:1872
http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-ci/2543/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-utopic-i386-build/1512
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-utopic-amd64-build/1518
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-utopic-touch/1495/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-utopic-amd64-ci/1065
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-utopic-amd64-ci/1065/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-utopic-armhf/432
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-utopic-armhf/432/artifact/work/output/*zip*/output.zip
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/2573/console
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/12297

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/mir-team-mir-development-branch-ci/2543/rebuild

review: Needs Fixing (continuous-integration)
Revision history for this message
Kevin DuBois (kdub) wrote :

I hadn't seen this test failure before, but I couldn't reproduce and its in an unrelated part of the system. Filing a bug seemed prudent though, so filed lp: #1362762 and retriggered.

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

PASSED: Continuous integration, rev:1872
http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-ci/2544/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-utopic-i386-build/1514
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-utopic-amd64-build/1520
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-utopic-touch/1497
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-utopic-amd64-ci/1066
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-utopic-amd64-ci/1066/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-utopic-armhf/434
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-utopic-armhf/434/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/2578
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/12321

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/mir-team-mir-development-branch-ci/2544/rebuild

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

Verified the fix works.

I'm just wondering if we could do a better fix... There might be other code (other regressions) depending on BufferID to be initialized to invalid. So if we don't fix BufferID itself, then we can't be sure we've caught all the regressions of this kind.

review: Needs Information
Revision history for this message
Kevin DuBois (kdub) wrote :

So I audited the code by compiling by using
class BufferID : public IntWrapper<BufferIDTag, uint32_t>
{
BufferID(uint32_t val) : IntWrapper(val) {}
BufferID() = delete;
}

to catch anywhere that we were using the default constructor. It only being used in the RecentlyUsedCache and in some places in the tests. I improved these to not rely on the invalid concept.

I didn't see a good reason to change BufferID further, so kept the current use:
typedef IntWrapper<BufferIDTag, uint32_t> BufferID;

Revision history for this message
Alan Griffiths (alan-griffiths) wrote :

It is really confusing for those reviewing if a revision labelled "audit for where the default constructor for BufferID is used" is mostly a merge from devel

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

PASSED: Continuous integration, rev:1873
http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-ci/2557/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-utopic-i386-build/1534
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-utopic-amd64-build/1540
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-utopic-touch/1517
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-utopic-amd64-ci/1079
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-utopic-amd64-ci/1079/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-utopic-armhf/454
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-utopic-armhf/454/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/2595
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/12366

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/mir-team-mir-development-branch-ci/2557/rebuild

review: Approve (continuous-integration)
Revision history for this message
Kevin DuBois (kdub) wrote :

> It is really confusing for those reviewing if a revision labelled "audit for
> where the default constructor for BufferID is used" is mostly a merge from
> devel

yeah, will redouble effort not to mix merges with fixes/changes

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

OK, if you've done the audit and Jenkins is also happy then I'm happy.

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'include/server/mir/compositor/recently_used_cache.h'
2--- include/server/mir/compositor/recently_used_cache.h 2014-05-20 16:24:02 +0000
3+++ include/server/mir/compositor/recently_used_cache.h 2014-08-29 13:25:39 +0000
4@@ -45,6 +45,7 @@
5 std::shared_ptr<graphics::GLTexture> texture;
6 graphics::BufferID last_bound_buffer;
7 bool used{true};
8+ bool valid_binding{false};
9 std::shared_ptr<graphics::Buffer> resource;
10 };
11
12
13=== modified file 'server-ABI-sha1sums'
14--- server-ABI-sha1sums 2014-08-29 12:51:40 +0000
15+++ server-ABI-sha1sums 2014-08-29 13:25:39 +0000
16@@ -53,7 +53,7 @@
17 561e7fdc22c48cf39a19e6f7c2db5e8c1feee772 include/server/mir/compositor/frame_dropping_policy_factory.h
18 e787879afa46d9c8257ff1f4ae8c7900d4b5f1a4 include/server/mir/compositor/frame_dropping_policy.h
19 e2e3e914a331a7611032860e3c64274aa42339aa include/server/mir/compositor/gl_renderer.h
20-4088fb037aa1bbae897145caca99314ea6cc2aa6 include/server/mir/compositor/recently_used_cache.h
21+ffb2f4f2b89412129cc5be42be3850ee2bfdcee0 include/server/mir/compositor/recently_used_cache.h
22 e0860c43bf5196a792ffd7216826925e16275d07 include/server/mir/compositor/renderer_factory.h
23 51391bd29f7499f8cbb89e4ba33acf2b72d93592 include/server/mir/compositor/renderer.h
24 686a839fd0cd5fa469344dc49190ff9d578efc25 include/server/mir/compositor/scene_element.h
25
26=== modified file 'src/server/compositor/recently_used_cache.cpp'
27--- src/server/compositor/recently_used_cache.cpp 2014-08-06 03:10:56 +0000
28+++ src/server/compositor/recently_used_cache.cpp 2014-08-29 13:25:39 +0000
29@@ -32,12 +32,13 @@
30 auto& texture = textures[renderable.id()];
31 texture.texture->bind();
32
33- if (texture.last_bound_buffer != buffer_id)
34+ if ((texture.last_bound_buffer != buffer_id) || (!texture.valid_binding))
35 {
36 buffer->gl_bind_to_texture();
37 texture.resource = buffer;
38 texture.last_bound_buffer = buffer_id;
39 }
40+ texture.valid_binding = true;
41 texture.used = true;
42
43 return texture.texture;
44@@ -45,9 +46,8 @@
45
46 void mc::RecentlyUsedCache::invalidate()
47 {
48- mg::BufferID invalid_id;
49 for (auto &t : textures)
50- t.second.last_bound_buffer = invalid_id;
51+ t.second.valid_binding = false;
52 }
53
54 void mc::RecentlyUsedCache::drop_unused()
55
56=== modified file 'tests/unit-tests/compositor/test_buffer_queue.cpp'
57--- tests/unit-tests/compositor/test_buffer_queue.cpp 2014-08-29 12:51:40 +0000
58+++ tests/unit-tests/compositor/test_buffer_queue.cpp 2014-08-29 13:25:39 +0000
59@@ -581,7 +581,7 @@
60 {
61 mc::BufferQueue q(nbuffers, allocator, basic_properties, policy_factory);
62
63- mg::BufferID client_id;
64+ mg::Buffer* last_client_acquired_buffer{nullptr};
65
66 for (int i = 0; i < 20; i++)
67 {
68@@ -589,7 +589,7 @@
69 {
70 auto handle = client_acquire_async(q);
71 ASSERT_THAT(handle->has_acquired_buffer(), Eq(true));
72- client_id = handle->id();
73+ last_client_acquired_buffer = handle->buffer();
74 handle->release_buffer();
75 }
76
77@@ -597,7 +597,7 @@
78 {
79 void const* user_id = reinterpret_cast<void const*>(monitor_id);
80 auto buffer = q.compositor_acquire(user_id);
81- ASSERT_THAT(buffer->id(), Eq(client_id));
82+ ASSERT_THAT(buffer.get(), Eq(last_client_acquired_buffer));
83 q.compositor_release(buffer);
84 }
85 }
86@@ -969,7 +969,7 @@
87 for (int nbuffers = 1; nbuffers <= max_nbuffers_to_test; ++nbuffers)
88 {
89 mc::BufferQueue q(nbuffers, allocator, basic_properties, policy_factory);
90- mg::BufferID history[5];
91+ std::vector<mg::BufferID> history;
92
93 const int width0 = 123;
94 const int height0 = 456;
95@@ -980,7 +980,8 @@
96 int const nbuffers_to_use = q.buffers_free_for_client();
97 ASSERT_THAT(nbuffers_to_use, Gt(0));
98
99- for (int produce = 0; produce < max_ownable_buffers(nbuffers); ++produce)
100+ int max_buffers{max_ownable_buffers(nbuffers)};
101+ for (int produce = 0; produce < max_buffers; ++produce)
102 {
103 geom::Size new_size{width, height};
104 width += dx;
105@@ -989,7 +990,7 @@
106 q.resize(new_size);
107 auto handle = client_acquire_async(q);
108 ASSERT_THAT(handle->has_acquired_buffer(), Eq(true));
109- history[produce] = handle->id();
110+ history.emplace_back(handle->id());
111 auto buffer = handle->buffer();
112 ASSERT_THAT(buffer->size(), Eq(new_size));
113 handle->release_buffer();
114@@ -998,7 +999,8 @@
115 width = width0;
116 height = height0;
117
118- for (int consume = 0; consume < max_ownable_buffers(nbuffers); ++consume)
119+ ASSERT_THAT(history.size(), Eq(max_buffers));
120+ for (int consume = 0; consume < max_buffers; ++consume)
121 {
122 geom::Size expect_size{width, height};
123 width += dx;
124@@ -1464,7 +1466,7 @@
125 q.drop_old_buffers();
126
127 auto comp = q.compositor_acquire(this);
128- ASSERT_THAT(comp->id(), Ne(mg::BufferID{}));
129+ ASSERT_THAT(comp, Ne(nullptr));
130 }
131
132 TEST_F(BufferQueueTest, gives_compositor_the_newest_buffer_after_dropping_old_buffers)
133
134=== modified file 'tests/unit-tests/compositor/test_gl_texture_cache.cpp'
135--- tests/unit-tests/compositor/test_gl_texture_cache.cpp 2014-08-06 03:10:56 +0000
136+++ tests/unit-tests/compositor/test_gl_texture_cache.cpp 2014-08-29 13:25:39 +0000
137@@ -122,3 +122,18 @@
138 cache.drop_unused();
139 EXPECT_EQ(old_use_count, mock_buffer.use_count());
140 }
141+
142+//LP: #1362444
143+TEST_F(RecentlyUsedCache, invalidated_buffers_are_reloaded)
144+{
145+ ON_CALL(*mock_buffer, id())
146+ .WillByDefault(testing::Return(mg::BufferID(0)));
147+ EXPECT_CALL(*mock_buffer,gl_bind_to_texture())
148+ .Times(2);
149+
150+ mc::RecentlyUsedCache cache;
151+ cache.load(*renderable);
152+ cache.load(*renderable);
153+ cache.invalidate();
154+ cache.load(*renderable);
155+}
156
157=== modified file 'tests/unit-tests/frontend/test_client_buffer_tracker.cpp'
158--- tests/unit-tests/frontend/test_client_buffer_tracker.cpp 2014-08-29 12:51:40 +0000
159+++ tests/unit-tests/frontend/test_client_buffer_tracker.cpp 2014-08-29 13:25:39 +0000
160@@ -91,9 +91,9 @@
161
162 TEST(ClientBufferTracker, tracks_correct_number_of_buffers)
163 {
164- mg::BufferID ids[10];
165+ std::vector<mg::BufferID> ids;
166 for (unsigned int i = 0; i < 10; ++i)
167- ids[i] = mg::BufferID{i};
168+ ids.emplace_back(mg::BufferID{i});
169
170 for (unsigned int tracker_size = 2; tracker_size < 10; ++tracker_size)
171 {

Subscribers

People subscribed via source and target branches