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

[Daniel van Vugt]

Ensure surface contents don't appear to stretch during resizing while
the client buffer size catches up to the surface size. (LP: #1259887)

The disadvantage to this approach is that edge pixels on the right and
bottom get clamped and repeated linearly. This is a restriction of OpenGL|ES
as it lacks GL_CLAMP_TO_BORDER support found in full OpenGL. Although
not having a separate border colour might actually be a good thing.

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:1482
http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-ci/1078/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-trusty-i386-build/1246
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-trusty-amd64-build/1244
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-trusty-touch/827
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-amd64-ci/810
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-amd64-ci/810/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-armhf-ci/815
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-armhf-ci/815/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-trusty-armhf/828
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-trusty-armhf/828/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/771
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/4823

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/mir-team-mir-development-branch-ci/1078/rebuild

review: Approve (continuous-integration)

[Alan Griffiths (alan-griffiths)]

Hmm. I guess this is as good a solution as any - I can't think of anything that has no downside. (I have to admit that my first reaction was "why not draw the same size that the client drew?" but that would not be responsive.)

review: Approve

[Alexandros Frantzis (afrantzis)]

> Ensure surface contents don't appear to stretch during rapid resizing while the client buffer size catches up to the surface size.

This fix assumes that the upper left corner of the surface is using the texel at (0,0) (i.e., the upper left corner is a stable point), but since we have an overridable tessellation method that may not be true (although it's usually the case).

I think that this should be handled in the tessellate() method, which knows all the details of UV mapping.

review: Needs Fixing

[Daniel van Vugt (vanvugt)]

Alexandros,

Initially I planned to do this in tessellate, certainly. And then I couldn't because it doesn't have a reference to the buffer, and it can't get one right now without also knowing the compositor frameno either. I hope to resolve that in future, but this approach is the one which can be made to work now without waiting for larger changes. You can block waiting for those larger changes, or just trust that "it will get better in future".

[Daniel van Vugt (vanvugt)]

Actually, I now think there's still a little too much jiggling. Possibly due to lack of floating point precision. That coupled with the visible texture clamping could actually cause more complaints.

review: Needs Fixing

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:1483
http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-ci/1105/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-trusty-i386-build/1288
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-trusty-amd64-build/1286
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-trusty-touch/868
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-amd64-ci/837
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-amd64-ci/837/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-armhf-ci/842
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-armhf-ci/842/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-trusty-armhf/869
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-trusty-armhf/869/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/811
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/4955

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/mir-team-mir-development-branch-ci/1105/rebuild

review: Approve (continuous-integration)

[Daniel van Vugt (vanvugt)]

Changed my mind. This is still a dramatic visual improvement on current resize behaviour. Particularly if you're looking at your window contents during the resize.

review: Abstain

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:1484
http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-ci/1135/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-trusty-i386-build/1328
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-trusty-amd64-build/1326
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-trusty-touch/908
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-amd64-ci/867/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-armhf-ci/872
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-armhf-ci/872/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-trusty-armhf/909
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-trusty-armhf/909/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/850
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/5069

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/mir-team-mir-development-branch-ci/1135/rebuild

review: Needs Fixing (continuous-integration)

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:1485
http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-ci/1146/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-trusty-i386-build/1345
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-trusty-amd64-build/1343
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-trusty-touch/925
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-amd64-ci/878
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-amd64-ci/878/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-armhf-ci/883
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-armhf-ci/883/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-trusty-armhf/926
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-trusty-armhf/926/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/865
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/5104

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/mir-team-mir-development-branch-ci/1146/rebuild

review: Approve (continuous-integration)

[Alexandros Frantzis (afrantzis)]

> I hope to resolve that in future, but this approach is the one which can be made to work now
> without waiting for larger changes. You can block waiting for those larger changes, or just
> trust that "it will get better in future".

I am OK with waiting until we get a proper fix, but it would be good to document this with a TODO and a description with the restriction that this puts on the tessellate function. For example, "For now, if you want sane visual behavior during resize, top left needs to use the surface texel (0pix,0pix)".

review: Needs Fixing

[Daniel van Vugt (vanvugt)]

OK, I've moved the texcoord fiddling back into tessellate() where it belongs. Unfortunately this does mean tessellate() gets another parameter, for now.

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:1487
http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-ci/1151/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-trusty-i386-build/1351
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-trusty-amd64-build/1349
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-trusty-touch/931/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-amd64-ci/883
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-amd64-ci/883/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-armhf-ci/888
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-armhf-ci/888/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-trusty-armhf/932
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-trusty-armhf/932/artifact/work/output/*zip*/output.zip
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/871/console
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/5116

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/mir-team-mir-development-branch-ci/1151/rebuild

review: Needs Fixing (continuous-integration)

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:1488
http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-ci/1164/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-trusty-i386-build/1369
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-trusty-amd64-build/1367
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-trusty-touch/949
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-amd64-ci/896
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-amd64-ci/896/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-armhf-ci/901
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-armhf-ci/901/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-trusty-armhf/950
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-trusty-armhf/950/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/885
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/5167

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/mir-team-mir-development-branch-ci/1164/rebuild

review: Approve (continuous-integration)

[Alberto Aguirre (albaguirre)]

LGTM

review: Approve

=== modified file 'examples/demo-shell/demo_renderer.cpp'
--- examples/demo-shell/demo_renderer.cpp	2014-03-21 02:49:38 +0000
+++ examples/demo-shell/demo_renderer.cpp	2014-03-25 03:43:02 +0000
@@ -173,9 +173,10 @@
 }
 
 void DemoRenderer::tessellate(std::vector<Primitive>& primitives,
-                              graphics::Renderable const& renderable) const
+                              graphics::Renderable const& renderable,
+                              geometry::Size const& buf_size) const
 {
-    GLRenderer::tessellate(primitives, renderable);
+    GLRenderer::tessellate(primitives, renderable, buf_size);
     tessellate_shadow(primitives, renderable, 80.0f);
     tessellate_frame(primitives, renderable, 30.0f);
 }

=== modified file 'examples/demo-shell/demo_renderer.h'
--- examples/demo-shell/demo_renderer.h	2014-03-21 02:49:38 +0000
+++ examples/demo-shell/demo_renderer.h	2014-03-25 03:43:02 +0000
@@ -34,7 +34,8 @@
 
     void begin() const override;
     void tessellate(std::vector<Primitive>& primitives,
-                    graphics::Renderable const& renderable) const override;
+                    graphics::Renderable const& renderable,
+                    geometry::Size const& buf_size) const override;
     void tessellate_shadow(std::vector<Primitive>& primitives,
                     graphics::Renderable const& renderable,
                     float radius) const;

=== modified file 'include/server/mir/compositor/gl_renderer.h'
--- include/server/mir/compositor/gl_renderer.h	2014-03-21 02:49:38 +0000
+++ include/server/mir/compositor/gl_renderer.h	2014-03-25 03:43:02 +0000
@@ -70,6 +70,11 @@
      * \param [in,out] primitives The list of rendering primitives to be
      *                            grown and/or modified.
      * \param [in]     renderable The renderable surface being tessellated.
+     * \param [in]     buf_size   The dimensions of the buffer being rendered,
+     *                            which can be particularly useful in
+     *                            calculating texcoords for a surface being
+     *                            actively resized (as the buf_size doesn't
+     *                            yet match renderable.size()).
      *
      * \note The cohesion of this function to GLRenderer is quite loose and it
      *       does not strictly need to reside here.
@@ -78,7 +83,8 @@
      *       tessellation is very much OpenGL-specific.
      */
     virtual void tessellate(std::vector<Primitive>& primitives,
-                            graphics::Renderable const& renderable) const;
+                            graphics::Renderable const& renderable,
+                            geometry::Size const& buf_size) const;
 
     /**
      * Load the texture for a surface any which way you like. The default

=== modified file 'src/server/compositor/gl_renderer.cpp'
--- src/server/compositor/gl_renderer.cpp	2014-03-21 02:49:38 +0000
+++ src/server/compositor/gl_renderer.cpp	2014-03-25 03:43:02 +0000
@@ -185,7 +185,8 @@
 }
 
 void mc::GLRenderer::tessellate(std::vector<Primitive>& primitives,
-                                graphics::Renderable const& renderable) const
+                                graphics::Renderable const& renderable,
+                                geometry::Size const& buf_size) const
 {
     auto const& rect = renderable.screen_position();
     GLfloat left = rect.top_left.x.as_int();
@@ -198,12 +199,17 @@
     client.tex_id = 0;
     client.type = GL_TRIANGLE_STRIP;
 
+    GLfloat tex_right = static_cast<GLfloat>(rect.size.width.as_int()) /
+                        buf_size.width.as_int();
+    GLfloat tex_bottom = static_cast<GLfloat>(rect.size.height.as_int()) /
+                         buf_size.height.as_int();
+
     auto& vertices = client.vertices;
     vertices.resize(4);
-    vertices[0] = {{left,  top,    0.0f}, {0.0f, 0.0f}};
-    vertices[1] = {{left,  bottom, 0.0f}, {0.0f, 1.0f}};
-    vertices[2] = {{right, top,    0.0f}, {1.0f, 0.0f}};
-    vertices[3] = {{right, bottom, 0.0f}, {1.0f, 1.0f}};
+    vertices[0] = {{left,  top,    0.0f}, {0.0f,      0.0f}};
+    vertices[1] = {{left,  bottom, 0.0f}, {0.0f,      tex_bottom}};
+    vertices[2] = {{right, top,    0.0f}, {tex_right, 0.0f}};
+    vertices[3] = {{right, bottom, 0.0f}, {tex_right, tex_bottom}};
 }
 
 void mc::GLRenderer::render(mg::Renderable const& renderable, mg::Buffer& buffer) const
@@ -239,7 +245,7 @@
     glEnableVertexAttribArray(texcoord_attr_loc);
 
     std::vector<Primitive> primitives;
-    tessellate(primitives, renderable);
+    tessellate(primitives, renderable, buffer.size());
    
     for (auto const& p : primitives)
     {

=== modified file 'tests/unit-tests/compositor/test_gl_renderer.cpp'
--- tests/unit-tests/compositor/test_gl_renderer.cpp	2014-03-21 02:49:38 +0000
+++ tests/unit-tests/compositor/test_gl_renderer.cpp	2014-03-25 03:43:02 +0000
@@ -329,6 +329,9 @@
     EXPECT_CALL(mock_gl, glEnableVertexAttribArray(position_attr_location));
     EXPECT_CALL(mock_gl, glEnableVertexAttribArray(texcoord_attr_location));
 
+    EXPECT_CALL(mock_buffer, size())
+        .WillOnce(Return(mir::geometry::Size{123, 456}));
+
     EXPECT_CALL(mock_gl, glBindTexture(GL_TEXTURE_2D, stub_texture));
     EXPECT_CALL(mock_gl, glVertexAttribPointer(position_attr_location, 3,
                                                GL_FLOAT, GL_FALSE, _, _));
@@ -372,6 +375,8 @@
     EXPECT_CALL(mock_gl, glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_MAG_FILTER, _));
 
     EXPECT_CALL(mock_buffer, bind_to_texture());
+    EXPECT_CALL(mock_buffer, size())
+        .WillOnce(Return(mir::geometry::Size{123, 456}));
 
     EXPECT_CALL(mock_gl, glBindTexture(GL_TEXTURE_2D, stub_texture));
     EXPECT_CALL(mock_gl, glDeleteTextures(1, Pointee(stub_texture)));
@@ -389,6 +394,9 @@
 {
     mtd::MockBuffer mock_buffer;
 
+    EXPECT_CALL(mock_buffer, size())
+        .WillRepeatedly(Return(mir::geometry::Size{123, 456}));
+
     InSequence seq;
 
     // First render() - texture generated and uploaded