Mir

Merge lp:~alan-griffiths/mir/fix-1516670 into lp:mir

fix-1516670
Merge into development-branch

Proposed by Alan Griffiths on 2015-11-20

Status:

Merged

Approved by:

Alan Griffiths on 2015-11-26

Approved revision:

no longer in the source branch.

Merged at revision:

3131

Proposed branch:

lp:~alan-griffiths/mir/fix-1516670

Merge into:

lp:mir

Diff against target:

192 lines (+78/-37)

3 files modified

src/server/graphics/nested/display.cpp (+67/-30)
src/server/graphics/nested/display.h (+2/-0)
tests/acceptance-tests/test_nested_mir.cpp (+9/-7)

To merge this branch:

bzr merge lp:~alan-griffiths/mir/fix-1516670

Medium

Fix Released

Link a bug report

Reviewer	Review Type	Date Requested	Status
PS Jenkins bot (community)	continuous-integration	2015-11-20	Approve on 2015-11-26
Alexandros Frantzis (community)			Needs Information on 2015-11-26
Andreas Pokorny (community)			Approve on 2015-11-26
Alberto Aguirre		2015-11-20	Pending
Review via email: mp+278184@code.launchpad.net

This proposal supersedes a proposal from 2015-11-17.

Commit message

nested: For clone mode use a fullscreen surface for the output with the largest intersection with the bounding rectangle

Description of the change

nested: For clone mode use a fullscreen surface for the output with the largest intersection with the bounding rectangle

The previous implementation attempted to cover the bounding rectangle of all outputs with a surface that was fullscreen on the first - which led to inconsistent states.

The surface creation logic has also been reworked to reuse existing surfaces where possible, which makes the server interaction less racy (I suspect there are some races still, but this makes plugging and replugging a lot more stable on the phone).

I also added some information to the surface name (which can be helpful when debugging window management in the host server).

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2015-11-17: Posted in a previous version of this proposal

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/mir-ci/5567/rebuild

review: Needs Fixing (continuous-integration)

Revision history for this message

Alan Griffiths (alan-griffiths) wrote on 2015-11-18: Posted in a previous version of this proposal

Having a stack of overlapping fullscreen surfaces in the host server introduces some visual artifacts (and inefficiencies).

So this is clearly not the ideal solution - just less broken than what we currently have.

There are a number of approaches for a better solution:

1. use non-fullscreen surfaces in nested mode and either:
1.1. display only the intersection of all outputs
1.2. display the bounding rectangle of all outputs

2. use only fullscreen surfaces and either:
2.1. select the "smallest" display
2.2. select the "largest" display

I'm not yet convinced by any of the above options, but favour of sticking with "fullscreen only" when nesting.

Revision history for this message

Alberto Aguirre (albaguirre) wrote on 2015-11-18: Posted in a previous version of this proposal

Are the visual artifacts on phone/desktop/both?

And what are the cases where outputs overlap?

We could also potentially run into max texture size issues for high res displays if we attempt to do 1.2.

In any case, a fullscreen surface per output makes sense to me.

review: Needs Information

Revision history for this message

Alan Griffiths (alan-griffiths) wrote on 2015-11-19: Posted in a previous version of this proposal

> Are the visual artifacts on phone/desktop/both?

Both, but easier to see on phone.

> And what are the cases where outputs overlap?

When displays of different sizes are "cloned".

So, with an external monitor attached to a mako:

$ sudo stop lightdm
$ bin/mir_demo_server --window-manager system-compositor&
$ bin/mir_demo_standalone_render_surfaces --no-file --host $XDG_RUNTIME_DIR/mir_socket --display-config clone

On the external monitor the fullscreen for the phone output is overlayed[1] on top of the fullscreen for the external monitor and the edge is visible as the flying surfaces it (as the rendering is not quite in sync.)

> We could also potentially run into max texture size issues for high res
> displays if we attempt to do 1.2.

Yep.

> In any case, a fullscreen surface per output makes sense to me.

[1] I'm not sure if this ordering is guaranteed, but was consistent in my limited testing.

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2015-11-19: Posted in a previous version of this proposal

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/mir-ci/5591/rebuild

review: Approve (continuous-integration)

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2015-11-20:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/mir-ci/5613/rebuild

review: Needs Fixing (continuous-integration)

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2015-11-23:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/mir-ci/5631/rebuild

review: Needs Fixing (continuous-integration)

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2015-11-23:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/mir-ci/5634/rebuild

review: Needs Fixing (continuous-integration)

Revision history for this message

Andreas Pokorny (andreas-pokorny) wrote on 2015-11-25:

hm I think this is an improvement but in clone mode we should use the smallest screen size for each surface and have the output scale.

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2015-11-26:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/mir-ci/5673/rebuild

review: Approve (continuous-integration)

Revision history for this message

Andreas Pokorny (andreas-pokorny) wrote on 2015-11-26:

Ok overall after looking and testing (on kms).. our default display configuration for cloned seems to be wrong. It should figure out a suitable intersection and try to select suitable modes on each output, instead of just aligning top_left. If that would be in place it would not matter if we pick the maximum overlap or intersection. (The former might be a better solution then).

review: Approve

Revision history for this message

Alexandros Frantzis (afrantzis) wrote on 2015-11-26:

Note that following scenario is still not working for me (but I am not sure if this MP was supposed to fix this):

Internal monitor 1280x800
External monitor 1920x1080

In different shells:
$ sudo bin/mir_demo_server --arw-file -f /tmp/mir_host
$ bin/mir_demo_server --host-socket /tmp/mir_host -f /tmp/mir_nested --vt 1
$ bin/mir_demo_client_display_config -m /tmp/mir_nested

Should start in clone mode, press 'h', press 'c' again. After last 'c' we should expect to go back to the original setup but we get into a strange setup instead. There seem to be two "framebuffer" surfaces, the first sized for the internal screen and the second for the external. On the external screen the two FB surfaces overlap, with the internal one being in front and hiding the top left part of the external. The client surface is only drawn on the external FB surface.

Also note that both the old and the new code don't handle properly cases where one output doesn't fully contain the other (e.g. 100x100+0+0 100x100+50+50). It's not a common case but we should (eventually) either support it or explicitly prohibit it.

> area.intersection_with(output.extents()))

Since area is the bounding rectangle of all outputs, isn't this expression equivalent to just output.extents()?

@Andreas
> hm I think this is an improvement but in clone mode we should use the smallest screen size
> for each surface and have the output scale.
...
> It should figure out a suitable intersection and try to select suitable modes on each output,
> instead of just aligning top_left.

Fair enough, we should probably rename the current clone policy to '(max)overlap' and implement a new 'clone' one. In any case there are both valid policies and should be properly supported. Plus there are cases where we can't 'clone' because there aren't any matching modes, so we need to fall back to 'overlap' anyway.

review: Needs Information

Revision history for this message

PS Jenkins bot (ps-jenkins) on 2015-11-26:

review: Approve (continuous-integration)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk

Subscribers

People subscribed via source and target branches

to all changes:

Alan Griffiths

Emanuele Antonio Faraone

Gerry Boland

Mir development team

 === modified file 'src/server/graphics/nested/display.cpp'
 --- src/server/graphics/nested/display.cpp	2015-11-18 12:38:30 +0000
 +++ src/server/graphics/nested/display.cpp	2015-11-25 23:12:50 +0000
@@ -34,6 +34,7 @@
  #include <boost/throw_exception.hpp>
  #include <stdexcept>
++#include <sstream>
  namespace mi = mir::input;
  namespace mg = mir::graphics;
@@ -147,6 +148,11 @@
      return std::chrono::milliseconds::zero();
+ }
++geom::Rectangle mgn::detail::DisplaySyncGroup::view_area() const
++{
++    return output->view_area();
++}
++
  mgn::Display::Display(
      std::shared_ptr<mg::Platform> const& platform,
      std::shared_ptr<HostConnection> const& connection,
@@ -203,11 +209,16 @@
  void mgn::Display::configure(mg::DisplayConfiguration const& configuration)
+ {
      std::lock_guard<std::mutex> lock(configuration_mutex);
--    if (*current_configuration != configuration)
--    {
--        apply_to_connection(configuration);
--        create_surfaces(configuration);
--    }
++    apply_to_connection(configuration);
++    create_surfaces(configuration);
++}
++
++namespace
++{
++long area_of(geom::Rectangle const& rect)
++{
++    return static_cast<long>(rect.size.width.as_int())*rect.size.height.as_int();
++}
+ }
  void mgn::Display::create_surfaces(mg::DisplayConfiguration const& configuration)
@@ -223,35 +234,61 @@
      unique_outputs.for_each_group(
          [&](mg::OverlappingOutputGroup const& group)
+         {
--            bool have_output_for_group = false;
--            geometry::Rectangle const& area = group.bounding_rectangle();
--            group.for_each_output([&](mg::DisplayConfigurationOutput output)
++            geometry::Rectangle const area = group.bounding_rectangle();
++
++            long max_overlap_area = 0;
++            mg::DisplayConfigurationOutput best_output;
++
++            group.for_each_output([&](mg::DisplayConfigurationOutput const& output)
+                 {
--                    if (!have_output_for_group)
++                    if (area_of(area.intersection_with(output.extents())) > max_overlap_area)
+                     {
--                        auto const& egl_config_format = output.current_format;
--
--                        complete_display_initialization(egl_config_format);
--
--                        auto const host_surface = connection->create_surface(
--                            area.size.width.as_int(),
--                            area.size.height.as_int(),
--                            egl_config_format,
--                            "Mir nested display",
--                            mir_buffer_usage_hardware,
--                            static_cast<uint32_t>(output.id.as_value()));
--
--                        result[output.id] = std::make_shared<mgn::detail::DisplaySyncGroup>(
--                            std::make_shared<mgn::detail::DisplayBuffer>(
--                                egl_display,
--                                host_surface,
--                                area,
--                                dispatcher,
--                                cursor_listener,
--                                output.current_format));
--                        have_output_for_group = true;
++                        max_overlap_area = area_of(area.intersection_with(output.extents()));
++                        best_output = output;
+                     }
                  });
++
++            auto const& egl_config_format = best_output.current_format;
++            geometry::Rectangle const& extents = best_output.extents();
++
++            auto& display_buffer = result[best_output.id];
++
++            {
++                std::unique_lock<std::mutex> lock(outputs_mutex);
++                display_buffer = outputs[best_output.id];
++            }
++
++            if (display_buffer)
++            {
++                if (display_buffer->view_area() != extents)
++                    display_buffer.reset();
++            }
++
++            if (!display_buffer)
++            {
++                complete_display_initialization(egl_config_format);
++
++                std::ostringstream surface_title;
++
++                surface_title << "Mir nested display for output #" << best_output.id.as_value();
++
++                auto const host_surface = connection->create_surface(
++                    extents.size.width.as_int(),
++                    extents.size.height.as_int(),
++                    egl_config_format,
++                    surface_title.str().c_str(),
++                    mir_buffer_usage_hardware,
++                    static_cast<uint32_t>(best_output.id.as_value()));
++
++                display_buffer = std::make_shared<mgn::detail::DisplaySyncGroup>(
++                    std::make_shared<mgn::detail::DisplayBuffer>(
++                        egl_display,
++                        host_surface,
++                        extents,
++                        dispatcher,
++                        cursor_listener,
++                        best_output.current_format));
++            }
          });
+     {
 === modified file 'src/server/graphics/nested/display.h'
 --- src/server/graphics/nested/display.h	2015-10-29 02:15:05 +0000
 +++ src/server/graphics/nested/display.h	2015-11-25 23:12:50 +0000
@@ -100,6 +100,8 @@
      void for_each_display_buffer(std::function<void(graphics::DisplayBuffer&)> const&) override;
      void post() override;
      std::chrono::milliseconds recommended_sleep() const override;
++
++    geometry::Rectangle view_area() const;
  private:
      std::shared_ptr<detail::DisplayBuffer> const output;
  };
 === modified file 'tests/acceptance-tests/test_nested_mir.cpp'
 --- tests/acceptance-tests/test_nested_mir.cpp	2015-11-20 10:46:36 +0000
 +++ tests/acceptance-tests/test_nested_mir.cpp	2015-11-25 23:12:50 +0000
@@ -111,7 +111,7 @@
  std::vector<geom::Rectangle> const display_geometry
+ {
      {{  0, 0}, { 640,  480}},
--    {{480, 0}, {1920, 1080}}
++    {{640, 0}, {1920, 1080}}
  };
  std::chrono::seconds const timeout{10};
@@ -410,12 +410,13 @@
      // would be included in one of the later counts and cause a test failure.
      Mock::VerifyAndClearExpectations(mock_session_mediator_report.get());
--    // One post when surface drawn
++    // One post on each output when surface drawn
+     {
          mt::WaitCondition wait;
--        EXPECT_CALL(*mock_session_mediator_report, session_submit_buffer_called(_)).Times(1)
--                .WillOnce(InvokeWithoutArgs([&] { wait.wake_up_everyone(); }));
++        EXPECT_CALL(*mock_session_mediator_report, session_submit_buffer_called(_)).Times(2)
++            .WillOnce(InvokeWithoutArgs([]{}))
++            .WillOnce(InvokeWithoutArgs([&] { wait.wake_up_everyone(); }));
          mir_buffer_stream_swap_buffers_sync(mir_surface_get_buffer_stream(surface));
@@ -423,12 +424,13 @@
          Mock::VerifyAndClearExpectations(mock_session_mediator_report.get());
+     }
--    // One post when surface released
++    // One post on each output when surface released
+     {
          mt::WaitCondition wait;
--        EXPECT_CALL(*mock_session_mediator_report, session_submit_buffer_called(_)).Times(1)
--                .WillOnce(InvokeWithoutArgs([&] { wait.wake_up_everyone(); }));
++        EXPECT_CALL(*mock_session_mediator_report, session_submit_buffer_called(_)).Times(2)
++            .WillOnce(InvokeWithoutArgs([]{}))
++            .WillOnce(InvokeWithoutArgs([&] { wait.wake_up_everyone(); }));
          mir_surface_release_sync(surface);
          mir_connection_release(connection);