Merge lp:~alan-griffiths/mir/fix-1535894 into lp:mir
- fix-1535894
- Merge into development-branch
Status: | Rejected |
---|---|
Rejected by: | Alan Griffiths |
Proposed branch: | lp:~alan-griffiths/mir/fix-1535894 |
Merge into: | lp:mir |
Diff against target: |
112 lines (+38/-1) 6 files modified
include/server/mir/compositor/scene.h (+2/-0) src/server/compositor/multi_threaded_compositor.cpp (+2/-1) src/server/scene/surface_stack.cpp (+25/-0) src/server/scene/surface_stack.h (+1/-0) tests/include/mir/test/doubles/mock_scene.h (+4/-0) tests/include/mir/test/doubles/stub_scene.h (+4/-0) |
To merge this branch: | bzr merge lp:~alan-griffiths/mir/fix-1535894 |
Related bugs: |
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Kevin DuBois (community) | Approve | ||
Daniel van Vugt | Needs Fixing | ||
PS Jenkins bot (community) | continuous-integration | Approve | |
Mir CI Bot | continuous-integration | Needs Fixing | |
Review via email: mp+285058@code.launchpad.net |
Commit message
compositor, scene: Filter Scene::
Description of the change
compositor, scene: Filter Scene::
We recently landed a heuristic to not schedule composition for an output if a posted buffer doesn't intersect it. This was less effective than intended as these buffers were still counted at the end of a composition pass when deciding whether to do an additional composition pass.
Mir CI Bot (mir-ci-bot) wrote : | # |
Alan Griffiths (alan-griffiths) wrote : | # |
DEBUG: jenkins job parameter [use_descriptio
DEBUG: Unable to get "use_descriptio
DEBUG: Job is configured to fallback to description: False
/me dons SEP glasses
PS Jenkins bot (ps-jenkins) wrote : | # |
FAILED: Continuous integration, rev:3295
http://
Executed test runs:
SUCCESS: http://
FAILURE: http://
SUCCESS: http://
SUCCESS: http://
deb: http://
FAILURE: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
SUCCESS: http://
Click here to trigger a rebuild:
http://
Alexandros Frantzis (afrantzis) wrote : | # |
> /me dons SEP glasses
I will check whether that message should worry us, but the real problem seems to be:
21: [ RUN ] MultiThreadedCo
21: /��BUILDDIR�
21: Expected: (retry) < (max_retries), actual: 100 vs 100
21: [ FAILED ] MultiThreadedCo
For example in: https:/
- 3296. By Alan Griffiths
-
I wonder why that passes locally
Mir CI Bot (mir-ci-bot) wrote : | # |
FAILED: Continuous integration, rev:3296
https:/
Executed test runs:
None: https:/
Click here to trigger a rebuild:
https:/
Alexandros Frantzis (afrantzis) wrote : | # |
Not sure if this is related to changes in this MP:
11: [ RUN ] AndroidInputRec
11: /��BUILDDIR�
11: Expected: (duration) < (one_frame), actual: 8-byte object <05-B9 96-01 00-00 00-00> vs 8-byte object <2A-50 FE-00 00-00 00-00>
11: [ FAILED ] AndroidInputRec
Alan Griffiths (alan-griffiths) wrote : | # |
> Not sure if this is related to changes in this MP:
>
> https:/
> gcc,platform=
>
> 11: [ RUN ]
> AndroidInputRec
> 11: /��BUILDDIR�
> tests/client/
> 11: Expected: (duration) < (one_frame), actual: 8-byte object <05-B9 96-01
> 00-00 00-00> vs 8-byte object <2A-50 FE-00 00-00 00-00>
> 11: [ FAILED ]
> AndroidInputRec
I don't see how - AFAICS the test doesn't contain a MultiThreadedCo
Alan Griffiths (alan-griffiths) wrote : | # |
Ah. It's lp:1394369
PS Jenkins bot (ps-jenkins) wrote : | # |
PASSED: Continuous integration, rev:3296
http://
Executed test runs:
SUCCESS: http://
SUCCESS: http://
SUCCESS: http://
SUCCESS: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
SUCCESS: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
SUCCESS: http://
Click here to trigger a rebuild:
http://
Daniel van Vugt (vanvugt) wrote : | # |
Like the fix that came before it, this sounds like a workaround rather than a fix.
You are addressing the multi-monitor performance problem by only attempting to redraw one monitor per frame. That sounds like an admission of failure and that we can't render multi-monitor at full frame rate on Android. So I'd prefer to see that problem fixed and full frame rate on both displays simultaneously. Although we might be hitting the physical performance limits of the device, in which case changes like this is all we'd have left. Not yet proven though, and that would be a bit pessimistic.
I actually don't think we're hitting the device performance limitation as much as trying to run sub-optimal code on it:
https:/
Daniel van Vugt (vanvugt) wrote : | # |
Then again, if we don't intend to try and animate both screens at once then that's probably OK...
I'll stop short of approving though, because there's an additional problem with this branch that will break free-form rendering like Compiz or mir_proving_server -- Some pixels are painted outside of the surface itself (like shadows or other effects like minimizing animation). As such, your view area test will be wrong, and will fail to update the secondary screen when it needs updating.
Worse again, this change will fail completely under full screen transformations like desktop zoom. And again will fail to refresh displays correctly, appearing to freeze instead.
I actually disapprove, but will abstain because can see it will help pocket desktop as it stands right now.
Daniel van Vugt (vanvugt) wrote : | # |
Oh I forgot, we already have this feature... :)
void mc::DefaultDisp
{
report-
auto const& view_area = display_
auto const& occlusions = mc::filter_
Using this existing implementation would be preferable. It's just harder to modify now that we have display groups with the swap buffers call outside of the compositor. But not impossible. Plus doing the fix in the compositor would address my concerns for the future too.
All we need to do is return some result to the caller to tell them whether we have repainted (empty renderable_list), and so whether they should swap buffers.
Admittedly DefaultDisplayB
Fixing the problem in the compositor is ideal, because that's the only place where you might know about the additional rendering that's outside the expected confines of the surface rectangle.
It's not just other shells that don't exist yet which need this, but Unity8 needs it right now. Consider Unity8 on desktop with two monitors -- It does external rendering like minimize animations, shadows, and the whole launcher, that surface_stack.cpp doesn't know about and will get wrong using your proposed implementation.
Alan Griffiths (alan-griffiths) wrote : | # |
> Fixing the problem in the compositor is ideal, because that's the only place
> where you might know about the additional rendering that's outside the
> expected confines of the surface rectangle.
I assume you mean "Fixing the problem in the DisplayBufferCo
Shifting the logic from this MP and from -c 3274 to the DefaultDisplayB
I recall that we had similar logic once in an earlier version, with the DisplayBufferCo
Alan Griffiths (alan-griffiths) wrote : | # |
> Admittedly DefaultDisplayB
> in Unity8 any more. But we could do a similar fix in QtMir to address Unity8.
...
> It's not just other shells that don't exist yet which need this, but Unity8
> needs it right now. Consider Unity8 on desktop with two monitors -- It does
> external rendering like minimize animations, shadows, and the whole launcher,
> that surface_stack.cpp doesn't know about and will get wrong using your
> proposed implementation.
This highlights a serious failing in our customization points.
Unity8 uses QtCompositor from QtMir, not MultiThreadedCo
So the difference between this approach and your suggested alternative makes no difference to Unity8.
Alan Griffiths (alan-griffiths) wrote : | # |
> Oh I forgot, we already have this feature... :)
>
> void mc::DefaultDisp
> scene_elements)
> {
> report-
>
> auto const& view_area = display_
> auto const& occlusions = mc::filter_
> view_area);
>
> Using this existing implementation would be preferable. It's just harder to
> modify now that we have display groups with the swap buffers call outside of
> the compositor. But not impossible. Plus doing the fix in the compositor would
> address my concerns for the future too.
>
> All we need to do is return some result to the caller to tell them whether we
> have repainted (empty renderable_list), and so whether they should swap
> buffers.
Having thought about implementing this suggestion I don't think it works. (At least, not simply.)
The criteria for repainting is more complex that "!renderable_
Alan Griffiths (alan-griffiths) wrote : | # |
Another important case that isn't easily addressed by code in the context of DefaultDisplayB
Kevin DuBois (kdub) wrote : | # |
fix looks alright to me, so +1
Curious as to why "void frames_
Daniel van Vugt (vanvugt) wrote : | # |
> I recall that we had similar logic once in an earlier version, with the
> DisplayBufferCo
> recall why that was removed - we don't want to repeat the mistakes of the
> past.
Almost correct. What we had in an earlier version was a bool returned, which at the time indicated the DisplayBufferCo
But that just highlights a bool isn't really enough, and is now confusing. The DisplayBufferCo
Yes indeed r3274 was flawed for the same reason and needs to be moved into DisplayBufferCo
Also, here's a manual test case showing this branch causes visible regressions:
1. mir_proving_server --display-config sidebyside # needs 2 or more monitors
2. mir_demo_
3. Move the window to the right half of the left monitor near the edge, but not overlapping.
4. Zoom in with Super+mousewheel and move the mouse such that the zoomed window now appears on both monitors.
Using lp:mir - Triangle keeps animating smoothly.
Using this branch - Triangle freezes and stutters on the second monitor.
The same problem extends to regular viewing without any zoom. If you had a launcher notification animating on one screen, but no major window changes then the launcher animation would never be visible. Instead the screen with the launcher would appear to be frozen and idle, erroneously hiding the notification.
Someone might make the argument that in the Unity8 architecture this change only applies to system compositors and will work for Unity8. However that means we're now saying Mir multimonitor only works in a nested architectures where each system compositor surface fits one physical displays. That's not an acceptable limitation we should choose to introduce. Mir should still support non-nested configurations.
Unmerged revisions
- 3296. By Alan Griffiths
-
I wonder why that passes locally
- 3295. By Alan Griffiths
-
Filter Scene::
frames_ pending( ) by output area
Preview Diff
1 | === modified file 'include/server/mir/compositor/scene.h' | |||
2 | --- include/server/mir/compositor/scene.h 2015-02-22 07:46:25 +0000 | |||
3 | +++ include/server/mir/compositor/scene.h 2016-02-04 15:01:17 +0000 | |||
4 | @@ -26,6 +26,7 @@ | |||
5 | 26 | 26 | ||
6 | 27 | namespace mir | 27 | namespace mir |
7 | 28 | { | 28 | { |
8 | 29 | namespace geometry { class Rectangle; } | ||
9 | 29 | namespace scene | 30 | namespace scene |
10 | 30 | { | 31 | { |
11 | 31 | class Observer; | 32 | class Observer; |
12 | @@ -64,6 +65,7 @@ | |||
13 | 64 | * scene. | 65 | * scene. |
14 | 65 | */ | 66 | */ |
15 | 66 | virtual int frames_pending(CompositorID id) const = 0; | 67 | virtual int frames_pending(CompositorID id) const = 0; |
16 | 68 | virtual int frames_pending(CompositorID id, geometry::Rectangle const& view) const = 0; | ||
17 | 67 | 69 | ||
18 | 68 | virtual void register_compositor(CompositorID id) = 0; | 70 | virtual void register_compositor(CompositorID id) = 0; |
19 | 69 | virtual void unregister_compositor(CompositorID id) = 0; | 71 | virtual void unregister_compositor(CompositorID id) = 0; |
20 | 70 | 72 | ||
21 | === modified file 'src/server/compositor/multi_threaded_compositor.cpp' | |||
22 | --- src/server/compositor/multi_threaded_compositor.cpp 2016-02-01 09:42:10 +0000 | |||
23 | +++ src/server/compositor/multi_threaded_compositor.cpp 2016-02-04 15:01:17 +0000 | |||
24 | @@ -164,8 +164,9 @@ | |||
25 | 164 | int pending = 0; | 164 | int pending = 0; |
26 | 165 | for (auto& compositor : compositors) | 165 | for (auto& compositor : compositors) |
27 | 166 | { | 166 | { |
28 | 167 | auto const buffer = std::get<0>(compositor); | ||
29 | 167 | auto const comp_id = std::get<1>(compositor).get(); | 168 | auto const comp_id = std::get<1>(compositor).get(); |
31 | 168 | int pend = scene->frames_pending(comp_id); | 169 | int pend = scene->frames_pending(comp_id, buffer->view_area()); |
32 | 169 | if (pend > pending) | 170 | if (pend > pending) |
33 | 170 | pending = pend; | 171 | pending = pend; |
34 | 171 | } | 172 | } |
35 | 172 | 173 | ||
36 | === modified file 'src/server/scene/surface_stack.cpp' | |||
37 | --- src/server/scene/surface_stack.cpp 2016-01-29 08:18:22 +0000 | |||
38 | +++ src/server/scene/surface_stack.cpp 2016-02-04 15:01:17 +0000 | |||
39 | @@ -178,6 +178,31 @@ | |||
40 | 178 | return result; | 178 | return result; |
41 | 179 | } | 179 | } |
42 | 180 | 180 | ||
43 | 181 | int ms::SurfaceStack::frames_pending(mc::CompositorID id, geom::Rectangle const& view) const | ||
44 | 182 | { | ||
45 | 183 | RecursiveReadLock lg(guard); | ||
46 | 184 | |||
47 | 185 | int result = scene_changed ? 1 : 0; | ||
48 | 186 | for (auto const& surface : surfaces) | ||
49 | 187 | { | ||
50 | 188 | if (surface->visible() && view.overlaps({surface->top_left(), surface->size()})) | ||
51 | 189 | { | ||
52 | 190 | auto const tracker = rendering_trackers.find(surface.get()); | ||
53 | 191 | if (tracker != rendering_trackers.end() && tracker->second->is_exposed_in(id)) | ||
54 | 192 | { | ||
55 | 193 | // Note that we ask the surface and not a Renderable. | ||
56 | 194 | // This is because we don't want to waste time and resources | ||
57 | 195 | // on a snapshot till we're sure we need it... | ||
58 | 196 | int ready = surface->buffers_ready_for_compositor(id); | ||
59 | 197 | if (ready > result) | ||
60 | 198 | result = ready; | ||
61 | 199 | } | ||
62 | 200 | } | ||
63 | 201 | } | ||
64 | 202 | return result; | ||
65 | 203 | } | ||
66 | 204 | |||
67 | 205 | |||
68 | 181 | void ms::SurfaceStack::register_compositor(mc::CompositorID cid) | 206 | void ms::SurfaceStack::register_compositor(mc::CompositorID cid) |
69 | 182 | { | 207 | { |
70 | 183 | RecursiveWriteLock lg(guard); | 208 | RecursiveWriteLock lg(guard); |
71 | 184 | 209 | ||
72 | === modified file 'src/server/scene/surface_stack.h' | |||
73 | --- src/server/scene/surface_stack.h 2016-01-29 08:18:22 +0000 | |||
74 | +++ src/server/scene/surface_stack.h 2016-02-04 15:01:17 +0000 | |||
75 | @@ -75,6 +75,7 @@ | |||
76 | 75 | // From Scene | 75 | // From Scene |
77 | 76 | compositor::SceneElementSequence scene_elements_for(compositor::CompositorID id) override; | 76 | compositor::SceneElementSequence scene_elements_for(compositor::CompositorID id) override; |
78 | 77 | int frames_pending(compositor::CompositorID) const override; | 77 | int frames_pending(compositor::CompositorID) const override; |
79 | 78 | int frames_pending(compositor::CompositorID, geometry::Rectangle const&) const override; | ||
80 | 78 | void register_compositor(compositor::CompositorID id) override; | 79 | void register_compositor(compositor::CompositorID id) override; |
81 | 79 | void unregister_compositor(compositor::CompositorID id) override; | 80 | void unregister_compositor(compositor::CompositorID id) override; |
82 | 80 | 81 | ||
83 | 81 | 82 | ||
84 | === modified file 'tests/include/mir/test/doubles/mock_scene.h' | |||
85 | --- tests/include/mir/test/doubles/mock_scene.h 2015-02-22 07:46:25 +0000 | |||
86 | +++ tests/include/mir/test/doubles/mock_scene.h 2016-02-04 15:01:17 +0000 | |||
87 | @@ -42,6 +42,10 @@ | |||
88 | 42 | 42 | ||
89 | 43 | MOCK_METHOD1(scene_elements_for, compositor::SceneElementSequence(compositor::CompositorID)); | 43 | MOCK_METHOD1(scene_elements_for, compositor::SceneElementSequence(compositor::CompositorID)); |
90 | 44 | MOCK_CONST_METHOD1(frames_pending, int(compositor::CompositorID)); | 44 | MOCK_CONST_METHOD1(frames_pending, int(compositor::CompositorID)); |
91 | 45 | |||
92 | 46 | int frames_pending(compositor::CompositorID id, geometry::Rectangle const&) const override | ||
93 | 47 | { return frames_pending(id); } | ||
94 | 48 | |||
95 | 45 | MOCK_METHOD1(register_compositor, void(compositor::CompositorID)); | 49 | MOCK_METHOD1(register_compositor, void(compositor::CompositorID)); |
96 | 46 | MOCK_METHOD1(unregister_compositor, void(compositor::CompositorID)); | 50 | MOCK_METHOD1(unregister_compositor, void(compositor::CompositorID)); |
97 | 47 | 51 | ||
98 | 48 | 52 | ||
99 | === modified file 'tests/include/mir/test/doubles/stub_scene.h' | |||
100 | --- tests/include/mir/test/doubles/stub_scene.h 2015-02-22 07:46:25 +0000 | |||
101 | +++ tests/include/mir/test/doubles/stub_scene.h 2016-02-04 15:01:17 +0000 | |||
102 | @@ -40,6 +40,10 @@ | |||
103 | 40 | { | 40 | { |
104 | 41 | return 0; | 41 | return 0; |
105 | 42 | } | 42 | } |
106 | 43 | int frames_pending(compositor::CompositorID id, geometry::Rectangle const&) const override | ||
107 | 44 | { | ||
108 | 45 | return frames_pending(id); | ||
109 | 46 | } | ||
110 | 43 | void register_compositor(compositor::CompositorID) override | 47 | void register_compositor(compositor::CompositorID) override |
111 | 44 | { | 48 | { |
112 | 45 | } | 49 | } |
FAILED: Continuous integration, rev:3295 /mir-jenkins. ubuntu. com/job/ mir-ci/ 230/ /mir-jenkins. ubuntu. com/job/ generic- update- mp/230/ console
https:/
Executed test runs:
None: https:/
Click here to trigger a rebuild: /mir-jenkins. ubuntu. com/job/ mir-ci/ 230/rebuild
https:/