Mir

Merge lp:~vanvugt/mir/resize-BufferStream into lp:mir

  1. resize-BufferStream
  2. Merge into development-branch
Proposed by Daniel van Vugt
Status: Merged
Approved by: Daniel van Vugt
Approved revision: no longer in the source branch.
Merged at revision: 1200
Proposed branch: lp:~vanvugt/mir/resize-BufferStream
Merge into: lp:mir
Diff against target: 182 lines (+98/-0)
8 files modified
include/server/mir/compositor/buffer_stream.h (+1/-0)
include/test/mir_test_doubles/mock_buffer_stream.h (+1/-0)
include/test/mir_test_doubles/stub_buffer_stream.h (+4/-0)
src/server/compositor/buffer_stream_surfaces.cpp (+5/-0)
src/server/compositor/buffer_stream_surfaces.h (+1/-0)
tests/integration-tests/compositor/test_buffer_stream.cpp (+73/-0)
tests/integration-tests/test_swapinterval.cpp (+1/-0)
tests/unit-tests/compositor/test_buffer_stream.cpp (+12/-0)
To merge this branch: bzr merge lp:~vanvugt/mir/resize-BufferStream
Reviewer Review Type Date Requested Status
Kevin DuBois (community) Approve
PS Jenkins bot (community) continuous-integration Approve
Alexandros Frantzis (community) Approve
Alan Griffiths Needs Fixing
Review via email: mp+193555@code.launchpad.net

Commit message

Add resize() support to BufferStream, in preparation for resizable surfaces.

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

PASSED: Continuous integration, rev:1192
http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-ci/313/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-trusty-i386-build/90
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-trusty-amd64-build/88
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-amd64-ci/42
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-amd64-ci/42/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-armhf-ci/42
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-armhf-ci/42/artifact/work/output/*zip*/output.zip

Click here to trigger a rebuild:
http://10.97.0.26:8080/job/mir-team-mir-development-branch-ci/313/rebuild

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

Looks good functionality-wise.

8 + virtual void resize(const geometry::Size &size)

According to our style guide, the preferred position for placing 'const' is after the type, although we don't require it. However, we do require the '&'/'*' to be adjacent to the type.

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

I know the style guide and have raised the issue several times since April:
https://lists.ubuntu.com/archives/mir-devel/2013-April/000098.html

I will, as always, use the more sensible style where it makes sense in isolation. When modifying code adjacent to the older style however, I will keep the older style.

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

PASSED: Continuous integration, rev:1193
http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-ci/324/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-trusty-i386-build/106
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-trusty-amd64-build/103
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-amd64-ci/53
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-amd64-ci/53/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-armhf-ci/53
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-armhf-ci/53/artifact/work/output/*zip*/output.zip

Click here to trigger a rebuild:
http://10.97.0.26:8080/job/mir-team-mir-development-branch-ci/324/rebuild

review: Approve (continuous-integration)
Revision history for this message
Alan Griffiths (alan-griffiths) wrote :

> I know the style guide and have raised the issue several times since April:
> https://lists.ubuntu.com/archives/mir-devel/2013-April/000098.html
>
> I will, as always, use the more sensible style where it makes sense in
> isolation. When modifying code adjacent to the older style however, I will
> keep the older style.

The adjacent code does not follow this style, so that rationale doesn't apply.

Please use the more sensible style described in the style guideline.

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

Alan, I think you're in the minority industry-wise if you describe the style guide's approach as sensible. But I know I'm the minority within mir-team, so will compromise.

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

PASSED: Continuous integration, rev:1196
http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-ci/334/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-trusty-i386-build/119
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-trusty-amd64-build/116
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-amd64-ci/63
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-amd64-ci/63/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-armhf-ci/63
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-armhf-ci/63/artifact/work/output/*zip*/output.zip

Click here to trigger a rebuild:
http://10.97.0.26:8080/job/mir-team-mir-development-branch-ci/334/rebuild

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

Looks good.

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

PASSED: Continuous integration, rev:1197
http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-ci/337/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-trusty-i386-build/124
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-trusty-amd64-build/121
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-amd64-ci/66
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-amd64-ci/66/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-armhf-ci/66
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-armhf-ci/66/artifact/work/output/*zip*/output.zip

Click here to trigger a rebuild:
http://10.97.0.26:8080/job/mir-team-mir-development-branch-ci/337/rebuild

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

looks good to me

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

The last remaining "Needs Fixing" was fixed yesterday. And there are two other human approvals aside from that, so landing...

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'include/server/mir/compositor/buffer_stream.h'
2--- include/server/mir/compositor/buffer_stream.h 2013-10-23 15:35:18 +0000
3+++ include/server/mir/compositor/buffer_stream.h 2013-11-05 06:53:08 +0000
4@@ -47,6 +47,7 @@
5 virtual std::shared_ptr<graphics::Buffer> lock_snapshot_buffer() = 0;
6 virtual geometry::PixelFormat get_stream_pixel_format() = 0;
7 virtual geometry::Size stream_size() = 0;
8+ virtual void resize(geometry::Size const& size) = 0;
9 virtual void allow_framedropping(bool) = 0;
10 virtual void force_requests_to_complete() = 0;
11 };
12
13=== modified file 'include/test/mir_test_doubles/mock_buffer_stream.h'
14--- include/test/mir_test_doubles/mock_buffer_stream.h 2013-10-23 15:07:55 +0000
15+++ include/test/mir_test_doubles/mock_buffer_stream.h 2013-11-05 06:53:08 +0000
16@@ -38,6 +38,7 @@
17
18 MOCK_METHOD0(get_stream_pixel_format, geometry::PixelFormat());
19 MOCK_METHOD0(stream_size, geometry::Size());
20+ MOCK_METHOD1(resize, void(geometry::Size const&));
21 MOCK_METHOD0(force_client_completion, void());
22 MOCK_METHOD1(allow_framedropping, void(bool));
23 MOCK_METHOD0(force_requests_to_complete, void());
24
25=== modified file 'include/test/mir_test_doubles/stub_buffer_stream.h'
26--- include/test/mir_test_doubles/stub_buffer_stream.h 2013-10-23 15:07:55 +0000
27+++ include/test/mir_test_doubles/stub_buffer_stream.h 2013-11-05 06:53:08 +0000
28@@ -62,6 +62,10 @@
29 return geometry::Size();
30 }
31
32+ void resize(geometry::Size const&) override
33+ {
34+ }
35+
36 void force_requests_to_complete()
37 {
38 }
39
40=== modified file 'src/server/compositor/buffer_stream_surfaces.cpp'
41--- src/server/compositor/buffer_stream_surfaces.cpp 2013-10-24 13:41:51 +0000
42+++ src/server/compositor/buffer_stream_surfaces.cpp 2013-11-05 06:53:08 +0000
43@@ -64,6 +64,11 @@
44 return buffer_bundle->properties().size;
45 }
46
47+void mc::BufferStreamSurfaces::resize(geom::Size const& size)
48+{
49+ buffer_bundle->resize(size);
50+}
51+
52 void mc::BufferStreamSurfaces::force_requests_to_complete()
53 {
54 buffer_bundle->force_requests_to_complete();
55
56=== modified file 'src/server/compositor/buffer_stream_surfaces.h'
57--- src/server/compositor/buffer_stream_surfaces.h 2013-10-25 09:01:53 +0000
58+++ src/server/compositor/buffer_stream_surfaces.h 2013-11-05 06:53:08 +0000
59@@ -47,6 +47,7 @@
60
61 geometry::PixelFormat get_stream_pixel_format();
62 geometry::Size stream_size();
63+ void resize(geometry::Size const& size) override;
64 void allow_framedropping(bool);
65 void force_requests_to_complete();
66
67
68=== modified file 'tests/integration-tests/compositor/test_buffer_stream.cpp'
69--- tests/integration-tests/compositor/test_buffer_stream.cpp 2013-10-25 09:01:53 +0000
70+++ tests/integration-tests/compositor/test_buffer_stream.cpp 2013-11-05 06:53:08 +0000
71@@ -125,6 +125,79 @@
72 }
73 }
74
75+TEST_F(BufferStreamTest, resize_affects_client_buffers_immediately)
76+{
77+ auto old_size = buffer_stream.stream_size();
78+
79+ auto client1 = buffer_stream.secure_client_buffer();
80+ EXPECT_EQ(old_size, client1->size());
81+ client1.reset();
82+
83+ geom::Size const new_size
84+ {
85+ old_size.width.as_int() * 2,
86+ old_size.height.as_int() * 3
87+ };
88+ buffer_stream.resize(new_size);
89+ EXPECT_EQ(new_size, buffer_stream.stream_size());
90+
91+ auto client2 = buffer_stream.secure_client_buffer();
92+ EXPECT_EQ(new_size, client2->size());
93+ client2.reset();
94+
95+ buffer_stream.resize(old_size);
96+ EXPECT_EQ(old_size, buffer_stream.stream_size());
97+
98+ auto client3 = buffer_stream.secure_client_buffer();
99+ EXPECT_EQ(old_size, client3->size());
100+ client3.reset();
101+}
102+
103+TEST_F(BufferStreamTest, compositor_gets_resized_buffers)
104+{
105+ auto old_size = buffer_stream.stream_size();
106+
107+ buffer_stream.secure_client_buffer().reset();
108+
109+ geom::Size const new_size
110+ {
111+ old_size.width.as_int() * 2,
112+ old_size.height.as_int() * 3
113+ };
114+ buffer_stream.resize(new_size);
115+ EXPECT_EQ(new_size, buffer_stream.stream_size());
116+
117+ buffer_stream.secure_client_buffer().reset();
118+
119+ auto comp1 = buffer_stream.lock_compositor_buffer(1);
120+ EXPECT_EQ(old_size, comp1->size());
121+ comp1.reset();
122+
123+ auto comp2 = buffer_stream.lock_compositor_buffer(2);
124+ EXPECT_EQ(new_size, comp2->size());
125+ comp2.reset();
126+
127+ buffer_stream.secure_client_buffer().reset();
128+
129+ auto comp3 = buffer_stream.lock_compositor_buffer(3);
130+ EXPECT_EQ(new_size, comp3->size());
131+ comp3.reset();
132+
133+ buffer_stream.resize(old_size);
134+ EXPECT_EQ(old_size, buffer_stream.stream_size());
135+
136+ // No new client frames since resize(old_size), so compositor gets new_size
137+ auto comp4 = buffer_stream.lock_compositor_buffer(4);
138+ EXPECT_EQ(new_size, comp4->size());
139+ comp4.reset();
140+
141+ // Generate a new frame, which should be back to old_size now
142+ buffer_stream.secure_client_buffer().reset();
143+ auto comp5 = buffer_stream.lock_compositor_buffer(5);
144+ EXPECT_EQ(old_size, comp5->size());
145+ comp5.reset();
146+}
147+
148 TEST_F(BufferStreamTest, can_get_partly_released_back_buffer)
149 {
150 buffer_stream.secure_client_buffer().reset();
151
152=== modified file 'tests/integration-tests/test_swapinterval.cpp'
153--- tests/integration-tests/test_swapinterval.cpp 2013-10-29 11:39:28 +0000
154+++ tests/integration-tests/test_swapinterval.cpp 2013-11-05 06:53:08 +0000
155@@ -61,6 +61,7 @@
156 std::shared_ptr<mg::Buffer> lock_snapshot_buffer() { return std::make_shared<mtd::StubBuffer>(); }
157 geom::PixelFormat get_stream_pixel_format() { return geom::PixelFormat::abgr_8888; }
158 geom::Size stream_size() { return geom::Size{}; }
159+ void resize(geom::Size const&) override {}
160 void force_requests_to_complete() {}
161 void allow_framedropping(bool)
162 {
163
164=== modified file 'tests/unit-tests/compositor/test_buffer_stream.cpp'
165--- tests/unit-tests/compositor/test_buffer_stream.cpp 2013-10-29 11:39:28 +0000
166+++ tests/unit-tests/compositor/test_buffer_stream.cpp 2013-11-05 06:53:08 +0000
167@@ -142,3 +142,15 @@
168 mc::BufferStreamSurfaces buffer_stream(mock_bundle);
169 buffer_stream.allow_framedropping(true);
170 }
171+
172+TEST_F(BufferStreamTest, resizes_bundle)
173+{
174+ geom::Size const new_size{66, 77};
175+
176+ EXPECT_CALL(*mock_bundle, resize(new_size))
177+ .Times(1);
178+
179+ mc::BufferStreamSurfaces buffer_stream(mock_bundle);
180+ buffer_stream.resize(new_size);
181+}
182+

Subscribers

People subscribed via source and target branches