Merge lp:~vanvugt/mir/output-event-refreshrate into lp:mir

[Daniel van Vugt]

Add refresh rate to MirSurfaceOutputEvent.

FAQ:

  * Why?
    Two reasons at least. First and foremost, in future when libmirclient
    is doing its own vsync throttling, just knowing the timestamp of the
    last vsync isn't enough. That only gives you the correct phase
    information. Even if you observe multiple prior vsyncs that still
    won't tell you the maximum refresh rate of the display if your
    previous frames were not fast, and we need to know the maximum rate to
    calculate the correct minimal vsync interval so the client can catch
    up to the full rate of the display.
      Secondly, this gives libmirclient the correct frequency information
    it needs to do accurate touch resampling and remove the 59Hz hack.

  * So clients can do their own vsync now?
    Yes and no. You could use the refresh rate right now in a sleep and
    implement your own vsync that is immune to nesting lag.
    That's great, but it would be without the display vsync timing
    (not wired up with IPC yet) to provide the correct phase. So yes,
    you can use this refresh rate value for your vsync and solve most of
    the lag problem now, but no it's not yet the optimal solution (up to
    one frame extra lag) until we also have the vsync phase information
    from the server.

  * Why not just query the display config?
    That would be more heavy-weight than this approach, and less
    consistent with the current API. But more importantly we need these
    summary mir_surface_output_event_ functions as future proofing to
    support the possibility that the "output" being described is actually
    just a summary of multiple different physical outputs' attributes.

  * Is a double precise enough to do our own frame timing?
    Well, it's all the accuracy we can get right now. But later when
    we've got the vsync phase information too, the precision of the
    refresh_rate value won't matter as much because it will get
    autocorrected with the phase timestamp on each frame. So then we won't
    need much precision from the refresh_rate value in the end.

  * The MSC used in conjuction with UST surely provides this info?
    Indeed that used to be true for fixed framerate displays, but the old
    equation of (delta(UST)/delta(MSC)) doesn't work in the new world of
    GSync and FreeSync. You need to know the best case refresh rate,
    which that equation does not tell you any more.

  * What's MSC and UST?
    See the 'physical-frame' branch.

  * Why add a public client API function?
    Strictly speaking it won't be necessary as the refresh rate
    information will only be required internally by libmirclient. But
    right now it's proving handy for testing to add this one new public
    function.

[Mir CI Bot (mir-ci-bot)]

PASSED: Continuous integration, rev:3737
https://mir-jenkins.ubuntu.com/job/mir-ci/1859/
Executed test runs:
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-mir/2354
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-0-fetch/2417
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=vivid+overlay/2409
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=xenial+overlay/2409
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=yakkety/2409
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=yakkety/2383
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=yakkety/2383/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=xenial+overlay/2383
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=xenial+overlay/2383/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=yakkety/2383
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=yakkety/2383/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=cross-armhf,compiler=gcc,platform=android,release=vivid+overlay/2383
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=cross-armhf,compiler=gcc,platform=android,release=vivid+overlay/2383/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=android,release=vivid+overlay/2383
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=android,release=vivid+overlay/2383/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=mesa,release=xenial+overlay/2383
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=mesa,release=xenial+overlay/2383/artifact/output/*zip*/output.zip

Click here to trigger a rebuild:
https://mir-jenkins.ubuntu.com/job/mir-ci/1859/rebuild

review: Approve (continuous-integration)

[Kevin DuBois (kdub)]

  * Why add a public client API function?
I could see nested passthrough being interested in seeing the value, so +1 to a public function.

  * Why not just query the display config?
Seems appropriate to have it be a field of the output event.

lgtm

review: Approve

[Cemil Azizoglu (cemil-azizoglu)]

LGTM

review: Approve

[Chris Halse Rogers (raof)]

So, I think that this *should* be implemented as getting the MirOutput* associated with the event, but that ship may have sailed.

review: Disapprove

[Daniel van Vugt (vanvugt)]

I half agree (depending on the day of the week), and may yet do just that.

However the alternative of landing this branch which introduces just one function is not bad. Certainly not worth disapproving.

[Mir CI Bot (mir-ci-bot)]

FAILED: Autolanding.
More details in the following jenkins job:
https://mir-jenkins.ubuntu.com/job/mir-autolanding/652/
Executed test runs:
    FAILURE: https://mir-jenkins.ubuntu.com/job/build-mir/2382/console
    None: https://mir-jenkins.ubuntu.com/job/generic-land-mp/693/console
    FAILURE: https://mir-jenkins.ubuntu.com/job/build-0-fetch/2445/console
    FAILURE: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=vivid+overlay/2437/console
    FAILURE: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=xenial+overlay/2437/console
    FAILURE: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=yakkety/2437/console
    FAILURE: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=yakkety/2411/console
    FAILURE: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=xenial+overlay/2411/console
    FAILURE: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=yakkety/2411/console
    FAILURE: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=cross-armhf,compiler=gcc,platform=android,release=vivid+overlay/2411/console
    FAILURE: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=android,release=vivid+overlay/2411/console
    FAILURE: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=mesa,release=xenial+overlay/2411/console

review: Needs Fixing (continuous-integration)

[Mir CI Bot (mir-ci-bot)]

PASSED: Continuous integration, rev:3739
https://mir-jenkins.ubuntu.com/job/mir-ci/1879/
Executed test runs:
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-mir/2383
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-0-fetch/2446
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=vivid+overlay/2438
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=xenial+overlay/2438
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=yakkety/2438
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=yakkety/2412
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=yakkety/2412/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=xenial+overlay/2412
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=xenial+overlay/2412/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=yakkety/2412
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=yakkety/2412/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=cross-armhf,compiler=gcc,platform=android,release=vivid+overlay/2412
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=cross-armhf,compiler=gcc,platform=android,release=vivid+overlay/2412/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=android,release=vivid+overlay/2412
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=android,release=vivid+overlay/2412/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=mesa,release=xenial+overlay/2412
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=mesa,release=xenial+overlay/2412/artifact/output/*zip*/output.zip

Click here to trigger a rebuild:
https://mir-jenkins.ubuntu.com/job/mir-ci/1879/rebuild

review: Approve (continuous-integration)

=== modified file 'examples/eglapp.c'
--- examples/eglapp.c	2016-10-03 04:00:30 +0000
+++ examples/eglapp.c	2016-10-04 08:31:47 +0000
@@ -130,11 +130,12 @@
     unsigned ff = mir_surface_output_event_get_form_factor(out);
     char const* form_factor = (ff < 6) ? form_factor_name[ff] : "out-of-range";
 
-    printf("Surface is on output %u: %d DPI, scale %.1fx, %s form factor\n",
+    printf("Surface is on output %u: %d DPI, scale %.1fx, %s form factor, %.2fHz\n",
            mir_surface_output_event_get_output_id(out),
            mir_surface_output_event_get_dpi(out),
            mir_surface_output_event_get_scale(out),
-           form_factor);
+           form_factor,
+           mir_surface_output_event_get_refresh_rate(out));
 }
 
 static void mir_eglapp_handle_event(MirSurface* surface, MirEvent const* ev, void* context)

=== modified file 'include/client/mir/events/event_builders.h'
--- include/client/mir/events/event_builders.h	2016-09-19 04:16:15 +0000
+++ include/client/mir/events/event_builders.h	2016-10-04 08:31:47 +0000
@@ -55,6 +55,7 @@
     frontend::SurfaceId const& surface_id,
     int dpi,
     float scale,
+    double refresh_rate,
     MirFormFactor form_factor,
     uint32_t id);
 

=== modified file 'include/client/mir_toolkit/events/surface_output_event.h'
--- include/client/mir_toolkit/events/surface_output_event.h	2016-06-23 06:46:20 +0000
+++ include/client/mir_toolkit/events/surface_output_event.h	2016-10-04 08:31:47 +0000
@@ -55,6 +55,17 @@
 float mir_surface_output_event_get_scale(MirSurfaceOutputEvent const* ev);
 
 /**
+ * Retrieve the maximum refresh rate of the output(s) associated with a
+ * MirSurfaceOutputEvent. For variable refresh rate displays this represents
+ * the maximum refresh rate of the display to aim for, rather than a measurement
+ * of recent performance.
+ *
+ * \param [in] ev   The event
+ * \return          The refresh rate in Hz
+ */
+double mir_surface_output_event_get_refresh_rate(MirSurfaceOutputEvent const* ev);
+
+/**
  * Retrieve the ID of the output this surface is on from a MirSurfaceOutputEvent
  *
  * \param [in] ev   The event

=== modified file 'src/client/event.cpp'
--- src/client/event.cpp	2016-09-19 04:16:15 +0000
+++ src/client/event.cpp	2016-10-04 08:31:47 +0000
@@ -278,6 +278,12 @@
     return ev->scale();
 }
 
+double mir_surface_output_event_get_refresh_rate(MirSurfaceOutputEvent const* ev)
+{
+    expect_event_type(ev, mir_event_type_surface_output);
+    return ev->refresh_rate();
+}
+
 uint32_t mir_surface_output_event_get_output_id(MirSurfaceOutputEvent const *ev)
 {
     expect_event_type(ev, mir_event_type_surface_output);

=== modified file 'src/client/events/event_builders.cpp'
--- src/client/events/event_builders.cpp	2016-09-19 04:16:15 +0000
+++ src/client/events/event_builders.cpp	2016-10-04 08:31:47 +0000
@@ -127,6 +127,7 @@
     mf::SurfaceId const& surface_id,
     int dpi,
     float scale,
+    double refresh_rate,
     MirFormFactor form_factor,
     uint32_t output_id)
 {
@@ -135,6 +136,7 @@
     e->set_surface_id(surface_id.as_value());
     e->set_dpi(dpi);
     e->set_scale(scale);
+    e->set_refresh_rate(refresh_rate);
     e->set_form_factor(form_factor);
     e->set_output_id(output_id);
 

=== modified file 'src/client/symbols.map'
--- src/client/symbols.map	2016-10-04 07:43:10 +0000
+++ src/client/symbols.map	2016-10-04 08:31:47 +0000
@@ -433,6 +433,7 @@
     mir_surface_placement_get_relative_position;
     mir_display_output_type_name;
     mir_output_type_name;
+    mir_surface_output_event_get_refresh_rate;
 } MIR_CLIENT_0.24;
 
 MIR_CLIENT_DETAIL_0.25 {

=== modified file 'src/common/events/surface_output_event.cpp'
--- src/common/events/surface_output_event.cpp	2016-03-29 03:41:45 +0000
+++ src/common/events/surface_output_event.cpp	2016-10-04 08:31:47 +0000
@@ -53,6 +53,16 @@
     scale_ = scale;
 }
 
+double MirSurfaceOutputEvent::refresh_rate() const
+{
+    return refresh_rate_;
+}
+
+void MirSurfaceOutputEvent::set_refresh_rate(double rate)
+{
+    refresh_rate_ = rate;
+}
+
 MirFormFactor MirSurfaceOutputEvent::form_factor() const
 {
     return form_factor_;

=== modified file 'src/common/symbols.map'
--- src/common/symbols.map	2016-09-27 08:57:34 +0000
+++ src/common/symbols.map	2016-10-04 08:31:47 +0000
@@ -402,5 +402,7 @@
       MirSurfacePlacementEvent::placement*;
       MirSurfacePlacementEvent::set_placement*;
       mir::output_type_name*;
+      MirSurfaceOutputEvent::refresh_rate*;
+      MirSurfaceOutputEvent::set_refresh_rate*;
     };
 } MIR_COMMON_0.24;

=== modified file 'src/include/common/mir/events/surface_output_event.h'
--- src/include/common/mir/events/surface_output_event.h	2016-03-30 19:01:10 +0000
+++ src/include/common/mir/events/surface_output_event.h	2016-10-04 08:31:47 +0000
@@ -24,6 +24,8 @@
 
 #include "mir/events/event.h"
 
+// XXX This is a private structure so why are its members kept private?
+//     What's the point in having all the getters and setters?
 struct MirSurfaceOutputEvent : MirEvent
 {
     MirSurfaceOutputEvent();
@@ -37,6 +39,9 @@
     float scale() const;
     void set_scale(float scale);
 
+    double refresh_rate() const;
+    void set_refresh_rate(double);
+
     MirFormFactor form_factor() const;
     void set_form_factor(MirFormFactor factor);
 
@@ -49,6 +54,7 @@
     float scale_{0.0f};
     MirFormFactor form_factor_;
     uint32_t output_id_{std::numeric_limits<uint32_t>::max()};
+    double refresh_rate_{0.0};
 };
 
 #endif /* MIR_COMMON_SURFACE_OUTPUT_EVENT_H_ */

=== modified file 'src/server/scene/application_session.cpp'
--- src/server/scene/application_session.cpp	2016-09-28 00:41:49 +0000
+++ src/server/scene/application_session.cpp	2016-10-04 08:31:47 +0000
@@ -328,6 +328,7 @@
                     surface.first,
                     output_properties->dpi,
                     output_properties->scale,
+                    output_properties->refresh_rate,
                     output_properties->form_factor,
                     static_cast<uint32_t>(output_properties->id.as_value())
                     ));

=== modified file 'src/server/scene/output_properties_cache.cpp'
--- src/server/scene/output_properties_cache.cpp	2016-01-29 08:18:22 +0000
+++ src/server/scene/output_properties_cache.cpp	2016-10-04 08:31:47 +0000
@@ -57,6 +57,7 @@
                     output.extents(),
                     calculate_dpi(mode.size, output.physical_size_mm),
                     output.scale,
+                    mode.vrefresh_hz,
                     output.form_factor,
                     output.id});
             }

=== modified file 'src/server/scene/output_properties_cache.h'
--- src/server/scene/output_properties_cache.h	2016-01-29 08:18:22 +0000
+++ src/server/scene/output_properties_cache.h	2016-10-04 08:31:47 +0000
@@ -37,6 +37,7 @@
     geometry::Rectangle extents;
     int dpi;
     float scale;
+    double refresh_rate;
     MirFormFactor form_factor;
     graphics::DisplayConfigurationOutputId id;
 };

=== modified file 'src/server/scene/surface_event_source.cpp'
--- src/server/scene/surface_event_source.cpp	2016-09-19 04:16:15 +0000
+++ src/server/scene/surface_event_source.cpp	2016-10-04 08:31:47 +0000
@@ -57,6 +57,7 @@
             id,
             new_output_properties->dpi,
             new_output_properties->scale,
+            new_output_properties->refresh_rate,
             new_output_properties->form_factor,
             static_cast<uint32_t>(new_output_properties->id.as_value())
         ));

=== modified file 'tests/acceptance-tests/test_client_surface_events.cpp'
--- tests/acceptance-tests/test_client_surface_events.cpp	2016-06-27 10:58:58 +0000
+++ tests/acceptance-tests/test_client_surface_events.cpp	2016-10-04 08:31:47 +0000
@@ -37,6 +37,7 @@
 #include <condition_variable>
 #include <chrono>
 #include <mutex>
+#include <unordered_map>
 
 namespace mf = mir::frontend;
 namespace ms = mir::scene;
@@ -265,16 +266,21 @@
 
     auto constexpr form_factor = mir_form_factor_tablet;
     float constexpr scale = 2.15f;
+    std::unordered_map<unsigned,mg::DisplayConfigurationMode> current_mode;
 
     auto display_configuration = server.the_display()->configuration();
 
     display_configuration->for_each_output(
-        [](mg::UserDisplayConfigurationOutput& output_config)
+        [&current_mode](mg::UserDisplayConfigurationOutput& output_config)
         {
             output_config.scale = scale;
             output_config.form_factor = form_factor;
+            current_mode[output_config.id.as_value()] =
+                output_config.modes[output_config.current_mode_index];
         });
 
+    ASSERT_FALSE(current_mode.empty());
+
     set_event_filter(mir_event_type_surface_output);
     reset_last_event();
 
@@ -290,6 +296,10 @@
 
     EXPECT_THAT(mir_surface_output_event_get_form_factor(output_event), Eq(form_factor));
     EXPECT_THAT(mir_surface_output_event_get_scale(output_event), FloatEq(scale));
+
+    auto id = mir_surface_output_event_get_output_id(output_event);
+    ASSERT_THAT(current_mode.find(id), Ne(current_mode.end()));
+    EXPECT_THAT(mir_surface_output_event_get_refresh_rate(output_event), Eq(current_mode[id].vrefresh_hz));
 }
 
 TEST_F(ClientSurfaceEvents, can_unset_surface_event_handler)
@@ -526,6 +536,7 @@
     float constexpr scale = 2.15f;
 
     std::vector<uint32_t> display_ids;
+    std::unordered_map<unsigned,mg::DisplayConfigurationMode> current_mode;
 
     {
         mt::Signal display_config_changed;
@@ -535,13 +546,17 @@
         std::shared_ptr<mg::DisplayConfiguration> const display_configuration{server.the_display()->configuration()};
 
         display_configuration->for_each_output(
-            [&display_ids](mg::UserDisplayConfigurationOutput& output_config)
+            [&display_ids,&current_mode](mg::UserDisplayConfigurationOutput& output_config)
             {
                 output_config.scale = scale;
                 output_config.form_factor = form_factor;
                 display_ids.push_back(static_cast<uint32_t>(output_config.id.as_value()));
+                current_mode[output_config.id.as_value()] =
+                    output_config.modes[output_config.current_mode_index];
             });
 
+        ASSERT_FALSE(current_mode.empty());
+
         set_event_filter(mir_event_type_surface_output);
         reset_last_event();
 
@@ -568,7 +583,11 @@
     auto surface_event = mir_event_get_surface_output_event(context.event);
     EXPECT_THAT(mir_surface_output_event_get_form_factor(surface_event), Eq(form_factor));
     EXPECT_THAT(mir_surface_output_event_get_scale(surface_event), Eq(scale));
-    EXPECT_THAT(display_ids, Contains(Eq(mir_surface_output_event_get_output_id(surface_event))));
+    auto id = mir_surface_output_event_get_output_id(surface_event);
+    EXPECT_THAT(display_ids, Contains(Eq(id)));
+    ASSERT_THAT(current_mode.find(id), Ne(current_mode.end()));
+    EXPECT_THAT(mir_surface_output_event_get_refresh_rate(surface_event),
+                Eq(current_mode[id].vrefresh_hz));
 
     mir_surface_release_sync(surface);
 }

=== modified file 'tests/unit-tests/scene/test_application_session.cpp'
--- tests/unit-tests/scene/test_application_session.cpp	2016-09-28 00:41:48 +0000
+++ tests/unit-tests/scene/test_application_session.cpp	2016-10-04 08:31:47 +0000
@@ -836,8 +836,8 @@
 struct ApplicationSessionSurfaceOutput : public ApplicationSession
 {
     ApplicationSessionSurfaceOutput() :
-        high_dpi(static_cast<mg::DisplayConfigurationOutputId>(5), {3840, 2160}, {509, 286}, 2.5f, mir_form_factor_monitor),
-        projector(static_cast<mg::DisplayConfigurationOutputId>(2), {1280, 1024}, {800, 600}, 0.5f, mir_form_factor_projector),
+        high_dpi(static_cast<mg::DisplayConfigurationOutputId>(5), {3840, 2160}, {509, 286}, 2.5f, 60.0, mir_form_factor_monitor),
+        projector(static_cast<mg::DisplayConfigurationOutputId>(2), {1280, 1024}, {800, 600}, 0.5f, 50.0, mir_form_factor_projector),
         stub_surface_factory{std::make_shared<ObserverPreservingSurfaceFactory>()},
         sender{std::make_shared<testing::NiceMock<mtd::MockEventSink>>()},
         app_session(
@@ -860,8 +860,9 @@
             geom::Size const& resolution,
             geom::Size const& physical_size,
             float scale,
+            double hz,
             MirFormFactor form_factor) :
-            output{id, resolution, physical_size, mir_pixel_format_argb_8888, 60.0, true},
+            output{id, resolution, physical_size, mir_pixel_format_argb_8888, hz, true},
             form_factor{form_factor},
             scale{scale},
             dpi{calculate_dpi(resolution, physical_size)},
@@ -902,6 +903,11 @@
 
     auto const event = mir_event_get_surface_output_event(arg);
 
+    if (output.output.current_mode_index >= output.output.modes.size())
+        return false;
+
+    auto const& mode = output.output.modes[output.output.current_mode_index];
+
     return
         ExplainMatchResult(
             Eq(output.dpi),
@@ -916,6 +922,10 @@
             mir_surface_output_event_get_scale(event),
             result_listener) &&
         ExplainMatchResult(
+            Eq(mode.vrefresh_hz),
+            mir_surface_output_event_get_refresh_rate(event),
+            result_listener) &&
+        ExplainMatchResult(
             Eq(output.id),
             mir_surface_output_event_get_output_id(event),
             result_listener);