Mir

Merge lp:~kdub/mir/android-physical-size into lp:mir

android-physical-size
Merge into development-branch

Proposed by Kevin DuBois on 2015-03-09

Status:

Merged

Approved by:

Kevin DuBois on 2015-03-11

Approved revision:

no longer in the source branch.

Merged at revision:

2386

Proposed branch:

lp:~kdub/mir/android-physical-size

Merge into:

lp:mir

Diff against target:

131 lines (+43/-14)

2 files modified

src/platforms/android/server/hwc_blanking_control.cpp (+14/-1)
tests/unit-tests/graphics/android/test_hwc_configuration.cpp (+29/-13)

To merge this branch:

bzr merge lp:~kdub/mir/android-physical-size

Medium

Fix Released

Link a bug report

Reviewer	Review Type	Date Requested	Status
PS Jenkins bot (community)	continuous-integration		Approve on 2015-03-11
Chris Halse Rogers			Approve on 2015-03-11
Daniel van Vugt			Approve on 2015-03-11
Alan Griffiths		2015-03-09	Approve on 2015-03-10
Robert Carr (community)			Approve on 2015-03-09
Review via email: mp+252338@code.launchpad.net

Commit message

android: pass the reported physical size of the display from the driver to the rest of the mir system.

fixes: lp: #1430315

Description of the change

android: pass the reported physical size of the display from the driver to the rest of the mir system.

Revision history for this message

Alan Griffiths (alan-griffiths) wrote on 2015-03-09:

Mostly OK. But...

72 -TEST_F(HwcConfiguration, no_fpe_from_malformed_refresh)
73 +TEST_F(HwcConfiguration, no_fpe_from_malformed_refresh_or_dpi)

"no_fpe_from_malformed_refresh_or_dpi" is a bit hard to parse (especially as what the test does).

"no_fpe_or_dpi_from_malformed_refresh" is probably better, but breaks the "one test per condition" rule.

review: Needs Fixing

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2015-03-09:

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

review: Needs Fixing (continuous-integration)

Revision history for this message

Robert Carr (robertcarr) wrote on 2015-03-09:

LGTM

review: Approve

Revision history for this message

Kevin DuBois (kdub) wrote on 2015-03-09:

failure looks unrelated, filed: https://bugs.launchpad.net/mir/+bug/1430000

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2015-03-09:

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

review: Approve (continuous-integration)

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2015-03-09:

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

review: Approve (continuous-integration)

Revision history for this message

Daniel van Vugt (vanvugt) wrote on 2015-03-10:

Cool. If you can however, try using or extending "mir/geometry/length.h" instead of:

10 +int dpi_to_mm(uint32_t dpi, int pixel_num)

Suggest fixing, not needs fixing.

review: Needs Fixing

Revision history for this message

Alan Griffiths (alan-griffiths) wrote on 2015-03-10:

review: Approve

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2015-03-10:

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

review: Needs Fixing (continuous-integration)

Revision history for this message

Francis Ginther (fginther) wrote on 2015-03-10:

The above http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/4560/console build was aborted due to an unbootable image making it into the pipeline. The build has been restarted.

Revision history for this message

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

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

review: Approve (continuous-integration)

Revision history for this message

Daniel van Vugt (vanvugt) on 2015-03-11:

review: Approve

Revision history for this message

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

review: Needs Fixing (continuous-integration)

Revision history for this message

Chris Halse Rogers (raof) wrote on 2015-03-11:

Looks sensible, to the extent that Android allows you to be. Do you really have to calculate display size from DPI?

review: Approve

Revision history for this message

Daniel van Vugt (vanvugt) wrote on 2015-03-11:

Judging by the HWC API, yes it probably does need to be done this way.

Although in hindsight, this particular case allows for very little utilization of mir::geometry::Length.

Revision history for this message

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

review: Needs Fixing (continuous-integration)

Revision history for this message

Kevin DuBois (kdub) wrote on 2015-03-11:

^^ ci bug: https://bugs.launchpad.net/mir/+bug/1430000

Revision history for this message

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

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:

Emanuele Antonio Faraone

Gerry Boland

Kevin DuBois

Mir development team

 === modified file 'src/platforms/android/server/hwc_blanking_control.cpp'
 --- src/platforms/android/server/hwc_blanking_control.cpp	2015-03-05 05:47:28 +0000
 +++ src/platforms/android/server/hwc_blanking_control.cpp	2015-03-10 19:31:31 +0000
@@ -20,6 +20,7 @@
  #include "hwc_wrapper.h"
  #include "mir/raii.h"
  #include "android_format_conversion-inl.h"
++#include "mir/geometry/length.h"
  #include <EGL/egl.h>
  #include <EGL/eglext.h>
  #include <boost/throw_exception.hpp>
@@ -27,6 +28,7 @@
  #include <chrono>
  namespace mga = mir::graphics::android;
++namespace geom = mir::geometry;
  namespace
+ {
@@ -100,6 +102,17 @@
+     }
+ }
++namespace
++{
++int dpi_to_mm(uint32_t dpi, int pixel_num)
++{
++    if (dpi == 0) return 0;
++    float dpi_inches = dpi / 1000.0f; //android multiplies by 1000
++    geom::Length length(pixel_num / dpi_inches, geom::Length::Units::inches);
++    return length.as(geom::Length::Units::millimetres);
++}
++}
++
  mga::DisplayAttribs mga::HwcBlankingControl::active_attribs_for(DisplayName display_name)
+ {
      auto configs = hwc_device->display_configs(display_name);
@@ -137,7 +150,7 @@
      return {
          {values[0], values[1]},
--        {0, 0}, //TODO: convert DPI to MM and return
++        {dpi_to_mm(values[3], values[0]), dpi_to_mm(values[4], values[1])},
          period_to_hz(std::chrono::nanoseconds{values[2]}),
          true,
          format,
 === modified file 'tests/unit-tests/graphics/android/test_hwc_configuration.cpp'
 --- tests/unit-tests/graphics/android/test_hwc_configuration.cpp	2015-03-05 05:47:28 +0000
 +++ tests/unit-tests/graphics/android/test_hwc_configuration.cpp	2015-03-10 19:31:31 +0000
@@ -128,7 +128,10 @@
  TEST_F(HwcConfiguration, queries_connected_primary_display_properties)
+ {
      using namespace testing;
--    geom::Size px_size {343, 254};
++    auto android_reported_dpi_x = 390000u;
++    auto android_reported_dpi_y = 400000u;
++    geom::Size px_size {768, 1280};
++    geom::Size mm_size {50, 81};
      std::vector<mga::ConfigId> hwc_config {mga::ConfigId{0xA1}, mga::ConfigId{0xBEE}};
      std::chrono::milliseconds vrefresh_period {16};
@@ -153,6 +156,12 @@
                          case HWC_DISPLAY_VSYNC_PERIOD:
                              values[i] = std::chrono::duration_cast<std::chrono::nanoseconds>(vrefresh_period).count();
                              break;
++                        case HWC_DISPLAY_DPI_X:
++                            values[i] = android_reported_dpi_x;
++                            break;
++                        case HWC_DISPLAY_DPI_Y:
++                            values[i] = android_reported_dpi_y;
++                            break;
                          default:
                              break;
+                     }
@@ -166,6 +175,7 @@
      auto vrefresh_hz = 1000.0 / vrefresh_period.count();
      auto attribs = config.active_attribs_for(display);
      EXPECT_THAT(attribs.pixel_size, Eq(px_size));
++    EXPECT_THAT(attribs.mm_size, Eq(mm_size));
      EXPECT_THAT(attribs.vrefresh_hz, Eq(vrefresh_hz));
      EXPECT_TRUE(attribs.connected);
+ }
@@ -205,29 +215,35 @@
  TEST_F(HwcConfiguration, no_fpe_from_malformed_refresh)
+ {
      using namespace testing;
--    EXPECT_CALL(*mock_hwc_wrapper, display_attributes( _, _, _, _))
++    EXPECT_CALL(*mock_hwc_wrapper, display_attributes(_,_,_,_))
              .WillOnce(Invoke([]
              (mga::DisplayName, mga::ConfigId, uint32_t const* attribute_list, int32_t* values)
+             {
                  int i = 0;
                  while(attribute_list[i] != HWC_DISPLAY_NO_ATTRIBUTE)
--                {
--                    switch(attribute_list[i])
--                    {
--                        case HWC_DISPLAY_VSYNC_PERIOD:
--                            values[i] = 0;
--                            break;
--                        default:
--                            break;
--                    }
--                    i++;
--                }
++                    values[i++] = 0;
                  return 0;
              }));
      auto attribs = config.active_attribs_for(mga::DisplayName::external);
      EXPECT_THAT(attribs.vrefresh_hz, Eq(0.0f));
+ }
++TEST_F(HwcConfiguration, no_fpe_from_malformed_dpi)
++{
++    using namespace testing;
++    EXPECT_CALL(*mock_hwc_wrapper, display_attributes(_,_,_,_))
++            .WillOnce(Invoke([]
++            (mga::DisplayName, mga::ConfigId, uint32_t const* attribute_list, int32_t* values)
++            {
++                int i = 0;
++                while(attribute_list[i] != HWC_DISPLAY_NO_ATTRIBUTE)
++                    values[i++] = 0;
++                return 0;
++            }));
++    auto attribs = config.active_attribs_for(mga::DisplayName::external);
++    EXPECT_THAT(attribs.mm_size, Eq(geom::Size{0,0}));
++}
++
  TEST_F(HwcConfiguration, subscribes_to_hotplug_and_vsync)
+ {
      using namespace testing;