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

[Alan Griffiths]

examples: if we use GL to init each buffer, then we need a GL context

examples: if we use GL to init each buffer, then we need a GL context

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:1425
http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-ci/935/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-trusty-i386-build/1036
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-trusty-amd64-build/1034
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-trusty-touch/628
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-amd64-ci/667/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-armhf-ci/672
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-armhf-ci/672/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-trusty-armhf/629
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-trusty-armhf/629/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/603
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/4288

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

review: Needs Fixing (continuous-integration)

[Daniel van Vugt (vanvugt)]

I can't tell if this will render correctly, or if it's just providing a dummy context to avoid crashing. Obviously it would need to render correctly too, which means making use of "gl_context". Will have to test...

[Daniel van Vugt (vanvugt)]

Tested and works.

review: Approve (manual testing)

[Daniel van Vugt (vanvugt)]

Ping Jenkins again. CI problems are now fixed.

[Alexandros Frantzis (afrantzis)]

The proposed solution depends on knowledge of how things work internally (e.g. that buffers are allocated lazily). I think that something in the spirit of http://paste.ubuntu.com/6998792/ is cleaner (for a more robust solution we need to restore the previous context instead of just releasing the current one, or alternatively, not bind a new context at all if there is an active context already).

Note: I have tried it only on desktop, not N4, so there may be further complications.

A weak needs fixing... I prefer this solution over the proposed, and I think it's the Right Way (TM), but I can live with what is proposed (it's certainly more explicit than what we have now).

review: Needs Fixing

[Daniel van Vugt (vanvugt)]

I agree with the first sentence of above. Though I disagree about changing to a significantly more cryptic design as suggested. That's bad for maintenance in the long run. Probably worse than breaking the encapsulation. At least simple code is simple to understand and therefore to maintain.

[Alexandros Frantzis (afrantzis)]

> Though I disagree about changing to a significantly more cryptic design as suggested

Which part of the new design do you find cryptic?

[Daniel van Vugt (vanvugt)]

mir::raii::paired_calls is cryptic, but then one could argue RAII and C++ itself is equally so by design. So that's just an opinion.

Also, from memory mg::BufferInitializer only exists to support examples/*. Last time I looked into it, that class had no purpose in the rest of Mir. It's probably a bad idea to make extensive use of such classes which are only used in examples/* and will never be used in real servers/clients.

[Alan Griffiths (alan-griffiths)]

> The proposed solution depends on knowledge of how things work internally (e.g.
> that buffers are allocated lazily). I think that something in the spirit of
> http://paste.ubuntu.com/6998792/ is cleaner (for a more robust solution we
> need to restore the previous context instead of just releasing the current
> one, or alternatively, not bind a new context at all if there is an active
> context already).

Applied your patch - it is sufficient for the example.

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:1427
http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-ci/944/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-trusty-i386-build/1047
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-trusty-amd64-build/1045
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-trusty-touch/639
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-amd64-ci/676
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-amd64-ci/676/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-armhf-ci/681
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-armhf-ci/681/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-trusty-armhf/640
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-trusty-armhf/640/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/614
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/4313

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

review: Approve (continuous-integration)

[Alexandros Frantzis (afrantzis)]

Looks good (what else would I say?) :)

review: Approve

[Daniel van Vugt (vanvugt)]

I'm assuming the new version still works. :) Land it.

=== modified file 'examples/render_surfaces.cpp'
--- examples/render_surfaces.cpp	2014-02-21 11:19:46 +0000
+++ examples/render_surfaces.cpp	2014-02-26 10:14:52 +0000
@@ -28,10 +28,12 @@
 #include "mir/graphics/cursor.h"
 #include "mir/graphics/display.h"
 #include "mir/graphics/display_buffer.h"
+#include "mir/graphics/gl_context.h"
 #include "mir/shell/surface_factory.h"
 #include "mir/shell/surface.h"
 #include "mir/run_mir.h"
 #include "mir/report_exception.h"
+#include "mir/raii.h"
 
 #include "mir_image.h"
 #include "buffer_render_target.h"
@@ -296,12 +298,17 @@
         class RenderResourcesBufferInitializer : public mg::BufferInitializer
         {
         public:
-            RenderResourcesBufferInitializer()
+            RenderResourcesBufferInitializer(std::unique_ptr<mg::GLContext> gl_context)
+                : gl_context{std::move(gl_context)}
             {
             }
 
             void operator()(mg::Buffer& buffer)
             {
+                auto using_gl_context = mir::raii::paired_calls(
+                    [this] { gl_context->make_current(); },
+                    [this] { gl_context->release_current(); });
+
                 mt::ImageRenderer img_renderer{mir_image.pixel_data,
                                geom::Size{mir_image.width, mir_image.height},
                                mir_image.bytes_per_pixel};
@@ -309,9 +316,13 @@
                 brt.make_current();
                 img_renderer.render();
             }
+
+        private:
+            std::unique_ptr<mg::GLContext> const gl_context;
+
         };
 
-        return std::make_shared<RenderResourcesBufferInitializer>();
+        return std::make_shared<RenderResourcesBufferInitializer>(the_display()->create_gl_context());
     }
     ///\internal [RenderResourcesBufferInitializer_tag]