Merge lp:~vanvugt/mir/fix-1672269 into lp:mir

[Daniel van Vugt]

Remember the glViewport calculation can change as a result of a
new output transformation (rotation) and not just a change in the
viewport rectangle. (LP: #1672269)

Technically the rotation does change the rectangle by transposing its
dimenions, however in LP: #1672269 this fact doesn't help because the
mode change has reconstructed the renderer with the newly transposed
dimensions before ever notifying there is an output transformation. So
the second set_viewport call that should have triggered glViewport
bailed out early thinking nothing had changed. But now we do it as
early (and seldom) as possible in both functions to cover all bases.

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

PASSED: Continuous integration, rev:4100
https://mir-jenkins.ubuntu.com/job/mir-ci/3203/
Executed test runs:
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-mir/4307
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-0-fetch/4394
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=vivid+overlay/4384
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=xenial+overlay/4384
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=zesty/4384
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=zesty/4339
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=zesty/4339/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/4339
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=xenial+overlay/4339/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=zesty/4339
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=zesty/4339/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/4339
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=cross-armhf,compiler=gcc,platform=android,release=vivid+overlay/4339/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/4339
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=android,release=vivid+overlay/4339/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/4339
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=mesa,release=xenial+overlay/4339/artifact/output/*zip*/output.zip

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

review: Approve (continuous-integration)

[Alexandros Frantzis (afrantzis)]

OK.

review: Approve

[Alan Griffiths (alan-griffiths)]

OK

review: Approve

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

review: Approve (continuous-integration)

=== modified file 'src/renderers/gl/renderer.cpp'
--- src/renderers/gl/renderer.cpp	2017-03-16 04:08:46 +0000
+++ src/renderers/gl/renderer.cpp	2017-03-21 09:09:36 +0000
@@ -372,12 +372,18 @@
                       0.0f});
 
     viewport = rect;
+    update_gl_viewport();
+}
 
+void mrg::Renderer::update_gl_viewport()
+{
     /*
      * Letterboxing: Move the glViewport to add black bars in the case that
      * the logical viewport aspect ratio doesn't match the display aspect.
      * This keeps pixels square. Note "black"-bars are really glClearColor.
      */
+    render_target.ensure_current();
+
     auto transformed_viewport = display_transform *
                                 glm::vec4(viewport.size.width.as_int(),
                                           viewport.size.height.as_int(), 0, 1);
@@ -407,7 +413,12 @@
 
 void mrg::Renderer::set_output_transform(glm::mat2 const& t)
 {
-    display_transform = glm::mat4(t);
+    auto const new_display_transform = glm::mat4(t);
+    if (new_display_transform != display_transform)
+    {
+        display_transform = new_display_transform;
+        update_gl_viewport();
+    }
 }
 
 void mrg::Renderer::suspend()

=== modified file 'src/renderers/gl/renderer.h'
--- src/renderers/gl/renderer.h	2017-01-27 08:13:36 +0000
+++ src/renderers/gl/renderer.h	2017-03-21 09:09:36 +0000
@@ -123,6 +123,8 @@
                       Renderer::Program const& prog) const;
 
 private:
+    void update_gl_viewport();
+
     std::unique_ptr<mir::gl::TextureCache> const texture_cache;
     geometry::Rectangle viewport;
     glm::mat4 screen_to_gl_coords;

=== modified file 'tests/unit-tests/renderers/gl/test_gl_renderer.cpp'
--- tests/unit-tests/renderers/gl/test_gl_renderer.cpp	2017-02-15 10:17:21 +0000
+++ tests/unit-tests/renderers/gl/test_gl_renderer.cpp	2017-03-21 09:09:36 +0000
@@ -316,6 +316,57 @@
     renderer.render(renderable_list);
 }
 
+TEST_F(GLRenderer, unchanged_viewport_avoids_gl_calls)
+{
+    int const screen_width = 1920;
+    int const screen_height = 1080;
+    mir::geometry::Rectangle const view_area{{0,0}, {1920,1080}};
+
+    ON_CALL(mock_egl, eglQuerySurface(_,_,EGL_WIDTH,_))
+        .WillByDefault(DoAll(SetArgPointee<3>(screen_width),
+                             Return(EGL_TRUE)));
+    ON_CALL(mock_egl, eglQuerySurface(_,_,EGL_HEIGHT,_))
+        .WillByDefault(DoAll(SetArgPointee<3>(screen_height),
+                             Return(EGL_TRUE)));
+    ON_CALL(mock_display_buffer, view_area())
+        .WillByDefault(Return(view_area));
+
+    mrg::Renderer renderer(mock_display_buffer);
+
+    renderer.set_viewport(view_area);
+
+    EXPECT_CALL(mock_gl, glViewport(_,_,_,_))
+        .Times(0);
+    renderer.set_viewport(view_area);
+}
+
+TEST_F(GLRenderer, unchanged_viewport_updates_gl_if_rotated)
+{   // Regression test for LP: #1672269
+    int const screen_width = 1920;
+    int const screen_height = 1080;
+    mir::geometry::Rectangle const view_area{{0,0}, {1920,1080}};
+
+    ON_CALL(mock_egl, eglQuerySurface(_,_,EGL_WIDTH,_))
+        .WillByDefault(DoAll(SetArgPointee<3>(screen_width),
+                             Return(EGL_TRUE)));
+    ON_CALL(mock_egl, eglQuerySurface(_,_,EGL_HEIGHT,_))
+        .WillByDefault(DoAll(SetArgPointee<3>(screen_height),
+                             Return(EGL_TRUE)));
+    ON_CALL(mock_display_buffer, view_area())
+        .WillByDefault(Return(view_area));
+
+    mrg::Renderer renderer(mock_display_buffer);
+
+    renderer.set_viewport(view_area);
+
+    EXPECT_CALL(mock_gl, glViewport(_,_,_,_))
+        .Times(1);
+    glm::mat2 const something_different{0,-1,
+                                        1, 0};
+    renderer.set_output_transform(something_different);
+    renderer.set_viewport(view_area);
+}
+
 TEST_F(GLRenderer, sets_viewport_unscaled_exact)
 {
     int const screen_width = 1920;