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
=== modified file 'include/server/mir/compositor/recently_used_cache.h'
--- include/server/mir/compositor/recently_used_cache.h 2014-05-20 16:24:02 +0000
+++ include/server/mir/compositor/recently_used_cache.h 2014-08-29 13:25:39 +0000
@@ -45,6 +45,7 @@
45 std::shared_ptr<graphics::GLTexture> texture;45 std::shared_ptr<graphics::GLTexture> texture;
46 graphics::BufferID last_bound_buffer;46 graphics::BufferID last_bound_buffer;
47 bool used{true};47 bool used{true};
48 bool valid_binding{false};
48 std::shared_ptr<graphics::Buffer> resource;49 std::shared_ptr<graphics::Buffer> resource;
49 };50 };
5051
5152
=== modified file 'server-ABI-sha1sums'
--- server-ABI-sha1sums 2014-08-29 12:51:40 +0000
+++ server-ABI-sha1sums 2014-08-29 13:25:39 +0000
@@ -53,7 +53,7 @@
53561e7fdc22c48cf39a19e6f7c2db5e8c1feee772 include/server/mir/compositor/frame_dropping_policy_factory.h53561e7fdc22c48cf39a19e6f7c2db5e8c1feee772 include/server/mir/compositor/frame_dropping_policy_factory.h
54e787879afa46d9c8257ff1f4ae8c7900d4b5f1a4 include/server/mir/compositor/frame_dropping_policy.h54e787879afa46d9c8257ff1f4ae8c7900d4b5f1a4 include/server/mir/compositor/frame_dropping_policy.h
55e2e3e914a331a7611032860e3c64274aa42339aa include/server/mir/compositor/gl_renderer.h55e2e3e914a331a7611032860e3c64274aa42339aa include/server/mir/compositor/gl_renderer.h
564088fb037aa1bbae897145caca99314ea6cc2aa6 include/server/mir/compositor/recently_used_cache.h56ffb2f4f2b89412129cc5be42be3850ee2bfdcee0 include/server/mir/compositor/recently_used_cache.h
57e0860c43bf5196a792ffd7216826925e16275d07 include/server/mir/compositor/renderer_factory.h57e0860c43bf5196a792ffd7216826925e16275d07 include/server/mir/compositor/renderer_factory.h
5851391bd29f7499f8cbb89e4ba33acf2b72d93592 include/server/mir/compositor/renderer.h5851391bd29f7499f8cbb89e4ba33acf2b72d93592 include/server/mir/compositor/renderer.h
59686a839fd0cd5fa469344dc49190ff9d578efc25 include/server/mir/compositor/scene_element.h59686a839fd0cd5fa469344dc49190ff9d578efc25 include/server/mir/compositor/scene_element.h
6060
=== modified file 'src/server/compositor/recently_used_cache.cpp'
--- src/server/compositor/recently_used_cache.cpp 2014-08-06 03:10:56 +0000
+++ src/server/compositor/recently_used_cache.cpp 2014-08-29 13:25:39 +0000
@@ -32,12 +32,13 @@
32 auto& texture = textures[renderable.id()];32 auto& texture = textures[renderable.id()];
33 texture.texture->bind();33 texture.texture->bind();
3434
35 if (texture.last_bound_buffer != buffer_id)35 if ((texture.last_bound_buffer != buffer_id) || (!texture.valid_binding))
36 {36 {
37 buffer->gl_bind_to_texture();37 buffer->gl_bind_to_texture();
38 texture.resource = buffer;38 texture.resource = buffer;
39 texture.last_bound_buffer = buffer_id;39 texture.last_bound_buffer = buffer_id;
40 }40 }
41 texture.valid_binding = true;
41 texture.used = true;42 texture.used = true;
4243
43 return texture.texture;44 return texture.texture;
@@ -45,9 +46,8 @@
4546
46void mc::RecentlyUsedCache::invalidate()47void mc::RecentlyUsedCache::invalidate()
47{48{
48 mg::BufferID invalid_id;
49 for (auto &t : textures)49 for (auto &t : textures)
50 t.second.last_bound_buffer = invalid_id;50 t.second.valid_binding = false;
51}51}
5252
53void mc::RecentlyUsedCache::drop_unused()53void mc::RecentlyUsedCache::drop_unused()
5454
=== modified file 'tests/unit-tests/compositor/test_buffer_queue.cpp'
--- tests/unit-tests/compositor/test_buffer_queue.cpp 2014-08-29 12:51:40 +0000
+++ tests/unit-tests/compositor/test_buffer_queue.cpp 2014-08-29 13:25:39 +0000
@@ -581,7 +581,7 @@
581 {581 {
582 mc::BufferQueue q(nbuffers, allocator, basic_properties, policy_factory);582 mc::BufferQueue q(nbuffers, allocator, basic_properties, policy_factory);
583583
584 mg::BufferID client_id;584 mg::Buffer* last_client_acquired_buffer{nullptr};
585585
586 for (int i = 0; i < 20; i++)586 for (int i = 0; i < 20; i++)
587 {587 {
@@ -589,7 +589,7 @@
589 {589 {
590 auto handle = client_acquire_async(q);590 auto handle = client_acquire_async(q);
591 ASSERT_THAT(handle->has_acquired_buffer(), Eq(true));591 ASSERT_THAT(handle->has_acquired_buffer(), Eq(true));
592 client_id = handle->id();592 last_client_acquired_buffer = handle->buffer();
593 handle->release_buffer();593 handle->release_buffer();
594 }594 }
595595
@@ -597,7 +597,7 @@
597 {597 {
598 void const* user_id = reinterpret_cast<void const*>(monitor_id);598 void const* user_id = reinterpret_cast<void const*>(monitor_id);
599 auto buffer = q.compositor_acquire(user_id);599 auto buffer = q.compositor_acquire(user_id);
600 ASSERT_THAT(buffer->id(), Eq(client_id));600 ASSERT_THAT(buffer.get(), Eq(last_client_acquired_buffer));
601 q.compositor_release(buffer);601 q.compositor_release(buffer);
602 }602 }
603 }603 }
@@ -969,7 +969,7 @@
969 for (int nbuffers = 1; nbuffers <= max_nbuffers_to_test; ++nbuffers)969 for (int nbuffers = 1; nbuffers <= max_nbuffers_to_test; ++nbuffers)
970 {970 {
971 mc::BufferQueue q(nbuffers, allocator, basic_properties, policy_factory);971 mc::BufferQueue q(nbuffers, allocator, basic_properties, policy_factory);
972 mg::BufferID history[5];972 std::vector<mg::BufferID> history;
973973
974 const int width0 = 123;974 const int width0 = 123;
975 const int height0 = 456;975 const int height0 = 456;
@@ -980,7 +980,8 @@
980 int const nbuffers_to_use = q.buffers_free_for_client();980 int const nbuffers_to_use = q.buffers_free_for_client();
981 ASSERT_THAT(nbuffers_to_use, Gt(0));981 ASSERT_THAT(nbuffers_to_use, Gt(0));
982982
983 for (int produce = 0; produce < max_ownable_buffers(nbuffers); ++produce)983 int max_buffers{max_ownable_buffers(nbuffers)};
984 for (int produce = 0; produce < max_buffers; ++produce)
984 {985 {
985 geom::Size new_size{width, height};986 geom::Size new_size{width, height};
986 width += dx;987 width += dx;
@@ -989,7 +990,7 @@
989 q.resize(new_size);990 q.resize(new_size);
990 auto handle = client_acquire_async(q);991 auto handle = client_acquire_async(q);
991 ASSERT_THAT(handle->has_acquired_buffer(), Eq(true));992 ASSERT_THAT(handle->has_acquired_buffer(), Eq(true));
992 history[produce] = handle->id();993 history.emplace_back(handle->id());
993 auto buffer = handle->buffer();994 auto buffer = handle->buffer();
994 ASSERT_THAT(buffer->size(), Eq(new_size));995 ASSERT_THAT(buffer->size(), Eq(new_size));
995 handle->release_buffer();996 handle->release_buffer();
@@ -998,7 +999,8 @@
998 width = width0;999 width = width0;
999 height = height0;1000 height = height0;
10001001
1001 for (int consume = 0; consume < max_ownable_buffers(nbuffers); ++consume)1002 ASSERT_THAT(history.size(), Eq(max_buffers));
1003 for (int consume = 0; consume < max_buffers; ++consume)
1002 {1004 {
1003 geom::Size expect_size{width, height};1005 geom::Size expect_size{width, height};
1004 width += dx;1006 width += dx;
@@ -1464,7 +1466,7 @@
1464 q.drop_old_buffers();1466 q.drop_old_buffers();
14651467
1466 auto comp = q.compositor_acquire(this);1468 auto comp = q.compositor_acquire(this);
1467 ASSERT_THAT(comp->id(), Ne(mg::BufferID{}));1469 ASSERT_THAT(comp, Ne(nullptr));
1468}1470}
14691471
1470TEST_F(BufferQueueTest, gives_compositor_the_newest_buffer_after_dropping_old_buffers)1472TEST_F(BufferQueueTest, gives_compositor_the_newest_buffer_after_dropping_old_buffers)
14711473
=== modified file 'tests/unit-tests/compositor/test_gl_texture_cache.cpp'
--- tests/unit-tests/compositor/test_gl_texture_cache.cpp 2014-08-06 03:10:56 +0000
+++ tests/unit-tests/compositor/test_gl_texture_cache.cpp 2014-08-29 13:25:39 +0000
@@ -122,3 +122,18 @@
122 cache.drop_unused();122 cache.drop_unused();
123 EXPECT_EQ(old_use_count, mock_buffer.use_count());123 EXPECT_EQ(old_use_count, mock_buffer.use_count());
124}124}
125
126//LP: #1362444
127TEST_F(RecentlyUsedCache, invalidated_buffers_are_reloaded)
128{
129 ON_CALL(*mock_buffer, id())
130 .WillByDefault(testing::Return(mg::BufferID(0)));
131 EXPECT_CALL(*mock_buffer,gl_bind_to_texture())
132 .Times(2);
133
134 mc::RecentlyUsedCache cache;
135 cache.load(*renderable);
136 cache.load(*renderable);
137 cache.invalidate();
138 cache.load(*renderable);
139}
125140
=== modified file 'tests/unit-tests/frontend/test_client_buffer_tracker.cpp'
--- tests/unit-tests/frontend/test_client_buffer_tracker.cpp 2014-08-29 12:51:40 +0000
+++ tests/unit-tests/frontend/test_client_buffer_tracker.cpp 2014-08-29 13:25:39 +0000
@@ -91,9 +91,9 @@
9191
92TEST(ClientBufferTracker, tracks_correct_number_of_buffers)92TEST(ClientBufferTracker, tracks_correct_number_of_buffers)
93{93{
94 mg::BufferID ids[10];94 std::vector<mg::BufferID> ids;
95 for (unsigned int i = 0; i < 10; ++i)95 for (unsigned int i = 0; i < 10; ++i)
96 ids[i] = mg::BufferID{i};96 ids.emplace_back(mg::BufferID{i});
9797
98 for (unsigned int tracker_size = 2; tracker_size < 10; ++tracker_size)98 for (unsigned int tracker_size = 2; tracker_size < 10; ++tracker_size)
99 {99 {

Subscribers

People subscribed via source and target branches