Mir

Merge lp:~kdub/mir/fix-1238695 into lp:mir

  1. fix-1238695
  2. Merge into development-branch
Proposed by Kevin DuBois
Status: Merged
Approved by: Kevin DuBois
Approved revision: no longer in the source branch.
Merged at revision: 1320
Proposed branch: lp:~kdub/mir/fix-1238695
Merge into: lp:mir
Diff against target: 124 lines (+56/-13)
3 files modified
src/platform/graphics/android/buffer.cpp (+19/-10)
src/platform/graphics/android/buffer.h (+3/-3)
tests/unit-tests/graphics/android/test_buffer_tex_bind.cpp (+34/-0)
To merge this branch: bzr merge lp:~kdub/mir/fix-1238695
Reviewer Review Type Date Requested Status
PS Jenkins bot (community) continuous-integration Approve
Gerry Boland (community) functional Approve
Andreas Pokorny (community) Approve
Alan Griffiths Needs Fixing
Alexandros Frantzis (community) Approve
Daniel van Vugt Approve
Kevin DuBois Pending
Review via email: mp+200754@code.launchpad.net

Commit message

Fix: unity8 display flickers and stops responding on Nexus 7 grouper (LP: #1238695)

It seems the issue with the nexus 7 was that its driver had a tough time binding one EGLImage object to two different gl contexts at the same time. This is why the screenshotting thread could mess up the compositing thread and cause the flickering in the bug. If we have two sibling EGLImage objects pointing to the same underlying buffer, we can bind them as textures in both the compositing and screenshotting thread without problems.

Description of the change

It seems the issue with the nexus 7 was that its driver had a tough time binding one EGLImage object to two different gl contexts at the same time. This is why the screenshotting thread could mess up the compositing thread and cause the flickering in the bug. If we have two sibling EGLImage objects pointing to the same underlying buffer, we can bind them as textures in both the compositing and screenshotting thread without problems.

this is indeed a workaround, but its pretty palatable.

tested on nexus 4, galaxy nexus, nexus 7, and nexus 10

fixes: LP: #1238695 LP: #1263741 LP: #1265787

To post a comment you must log in.
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :

FAILED: Continuous integration, rev:1316
No commit message was specified in the merge proposal. Click on the following link and set the commit message (if you want a jenkins rebuild you need to trigger it yourself):
https://code.launchpad.net/~kdub/mir/fix-1238695/+merge/200754/+edit-commit-message

http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-ci/630/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-android-trusty-i386-build/583/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-trusty-amd64-build/579
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-trusty-touch/187
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-amd64-ci/356
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-amd64-ci/356/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-armhf-ci/359
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-armhf-ci/359/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-trusty-armhf/187
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-trusty-armhf/187/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/209
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/2804

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

review: Needs Fixing (continuous-integration)
Revision history for this message
Daniel van Vugt (vanvugt) wrote :

(1) for(auto it = egl_image_map.begin(); it != egl_image_map.end(); it++)
could be:
    for (auto it& : egl_image_map)
But that's not important.

Looks OK and tested on N7 too.

review: Approve
Revision history for this message
Alexandros Frantzis (afrantzis) wrote :

Looks good. Also fixes N10 snapshotting issues.

+1 for "for (auto& it : egl_image_map)" but not critical.

Retriggering build.

review: Approve
Revision history for this message
Alan Griffiths (alan-griffiths) wrote :

95 + EGLDisplay disp1 = reinterpret_cast<EGLDisplay>(0x43);
96 + EGLDisplay disp2 = reinterpret_cast<EGLDisplay>(0x88);
97 + EGLContext ctxt1 = reinterpret_cast<EGLContext>(0x11);
98 + EGLContext ctxt2 = reinterpret_cast<EGLContext>(0x12);

Casting arbitrary integers to pointers can lead to undefined behavior. I know we're unlikely to be targeting a platform that, for example, faults on loading an invalid pointer into a register but why not just use the addresses of local variables and avoid the risk?

PS +1 for "for (auto& it : egl_image_map)"

review: Needs Fixing
Revision history for this message
Alan Griffiths (alan-griffiths) wrote :

> 95 + EGLDisplay disp1 = reinterpret_cast<EGLDisplay>(0x43);
> 96 + EGLDisplay disp2 = reinterpret_cast<EGLDisplay>(0x88);
> 97 + EGLContext ctxt1 = reinterpret_cast<EGLContext>(0x11);
> 98 + EGLContext ctxt2 = reinterpret_cast<EGLContext>(0x12);
>
> Casting arbitrary integers to pointers can lead to undefined behavior. I know
> we're unlikely to be targeting a platform that, for example, faults on loading
> an invalid pointer into a register but why not just use the addresses of local
> variables and avoid the risk?

e.g.

auto const disp1 = static_cast<EGLDisplay>(&disp1);

Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :

PASSED: Continuous integration, rev:1316
http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-ci/635/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-trusty-i386-build/592
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-trusty-amd64-build/588
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-trusty-touch/196
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-amd64-ci/361
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-amd64-ci/361/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-armhf-ci/364
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-armhf-ci/364/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-trusty-armhf/196
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-trusty-armhf/196/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/217
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/2822

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

review: Approve (continuous-integration)
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :

PASSED: Continuous integration, rev:1318
http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-ci/640/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-trusty-i386-build/598
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-trusty-amd64-build/594
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-trusty-touch/202
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-amd64-ci/366
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-amd64-ci/366/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-armhf-ci/369
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-armhf-ci/369/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-trusty-armhf/202
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-trusty-armhf/202/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/223
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/2856

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

review: Approve (continuous-integration)
Revision history for this message
Andreas Pokorny (andreas-pokorny) wrote :

LGTM

review: Approve
Revision history for this message
Gerry Boland (gerboland) wrote :

Functional testing this on Nexus10 shows it working well, the bug is fixed. Nice one!

review: Approve (functional)
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :

FAILED: Autolanding.
More details in the following jenkins job:
http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-autolanding/283/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-trusty-i386-build/601
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-trusty-amd64-build/597
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-trusty-touch/205/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-amd64-autolanding/185
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-amd64-autolanding/185/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-armhf-autolanding/186
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-armhf-autolanding/186/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-trusty-armhf/205
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-trusty-armhf/205/artifact/work/output/*zip*/output.zip
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/226/console
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/2861

review: Needs Fixing (continuous-integration)
Revision history for this message
Kevin DuBois (kdub) wrote :

flaky test

Revision history for this message
PS Jenkins bot (ps-jenkins) :
review: Approve (continuous-integration)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'src/platform/graphics/android/buffer.cpp'
2--- src/platform/graphics/android/buffer.cpp 2013-12-18 02:19:19 +0000
3+++ src/platform/graphics/android/buffer.cpp 2014-01-08 16:28:01 +0000
4@@ -42,14 +42,13 @@
5
6 mga::Buffer::~Buffer()
7 {
8- std::map<EGLDisplay,EGLImageKHR>::iterator it;
9- for(it = egl_image_map.begin(); it != egl_image_map.end(); it++)
10+ for(auto& it : egl_image_map)
11 {
12- egl_extensions->eglDestroyImageKHR(it->first, it->second);
13+ EGLDisplay disp = it.first.first;
14+ egl_extensions->eglDestroyImageKHR(disp, it.second);
15 }
16 }
17
18-
19 geom::Size mga::Buffer::size() const
20 {
21 ANativeWindowBuffer *anwb = native_buffer->anwb();
22@@ -79,26 +78,36 @@
23 std::unique_lock<std::mutex> lk(content_lock);
24 native_buffer->wait_for_content();
25
26- EGLDisplay disp = eglGetCurrentDisplay();
27- if (disp == EGL_NO_DISPLAY) {
28+ DispContextPair current
29+ {
30+ eglGetCurrentDisplay(),
31+ eglGetCurrentContext()
32+ };
33+
34+ if (current.first == EGL_NO_DISPLAY)
35+ {
36 BOOST_THROW_EXCEPTION(std::runtime_error("cannot bind buffer to texture without EGL context\n"));
37 }
38+
39 static const EGLint image_attrs[] =
40 {
41 EGL_IMAGE_PRESERVED_KHR, EGL_TRUE,
42 EGL_NONE
43 };
44+
45 EGLImageKHR image;
46- auto it = egl_image_map.find(disp);
47+ auto it = egl_image_map.find(current);
48 if (it == egl_image_map.end())
49 {
50- image = egl_extensions->eglCreateImageKHR(disp, EGL_NO_CONTEXT, EGL_NATIVE_BUFFER_ANDROID,
51- native_buffer->anwb(), image_attrs);
52+ image = egl_extensions->eglCreateImageKHR(
53+ current.first, EGL_NO_CONTEXT, EGL_NATIVE_BUFFER_ANDROID,
54+ native_buffer->anwb(), image_attrs);
55+
56 if (image == EGL_NO_IMAGE_KHR)
57 {
58 BOOST_THROW_EXCEPTION(std::runtime_error("error binding buffer to texture\n"));
59 }
60- egl_image_map[disp] = image;
61+ egl_image_map[current] = image;
62 }
63 else /* already had it in map */
64 {
65
66=== modified file 'src/platform/graphics/android/buffer.h'
67--- src/platform/graphics/android/buffer.h 2013-12-18 02:19:19 +0000
68+++ src/platform/graphics/android/buffer.h 2014-01-08 16:28:01 +0000
69@@ -58,10 +58,10 @@
70 std::shared_ptr<NativeBuffer> native_buffer_handle() const;
71
72 private:
73+ typedef std::pair<EGLDisplay, EGLContext> DispContextPair;
74+ std::map<DispContextPair,EGLImageKHR> egl_image_map;
75+
76 std::mutex mutable content_lock;
77-
78- std::map<EGLDisplay,EGLImageKHR> egl_image_map;
79-
80 std::shared_ptr<NativeBuffer> native_buffer;
81 std::shared_ptr<EGLExtensions> egl_extensions;
82 };
83
84=== modified file 'tests/unit-tests/graphics/android/test_buffer_tex_bind.cpp'
85--- tests/unit-tests/graphics/android/test_buffer_tex_bind.cpp 2013-12-18 02:19:19 +0000
86+++ tests/unit-tests/graphics/android/test_buffer_tex_bind.cpp 2014-01-08 16:28:01 +0000
87@@ -349,3 +349,37 @@
88 mga::Buffer buffer(mock_native_buffer, extensions);
89 buffer.bind_to_texture();
90 }
91+
92+TEST_F(AndroidBufferBinding, different_egl_contexts_displays_generate_new_eglimages)
93+{
94+ using namespace testing;
95+
96+ int d1 = 0, d2 = 0, c1 = 0, c2 = 0;
97+ EGLDisplay disp1 = reinterpret_cast<EGLDisplay>(&d1);
98+ EGLDisplay disp2 = reinterpret_cast<EGLDisplay>(&d2);
99+ EGLContext ctxt1 = reinterpret_cast<EGLContext>(&c1);
100+ EGLContext ctxt2 = reinterpret_cast<EGLContext>(&c2);
101+
102+ EXPECT_CALL(mock_egl, eglGetCurrentDisplay())
103+ .Times(3)
104+ .WillOnce(Return(disp1))
105+ .WillOnce(Return(disp1))
106+ .WillOnce(Return(disp2));
107+
108+ EXPECT_CALL(mock_egl, eglGetCurrentContext())
109+ .Times(3)
110+ .WillOnce(Return(ctxt1))
111+ .WillRepeatedly(Return(ctxt2));
112+
113+ EXPECT_CALL(mock_egl, eglCreateImageKHR(disp1,_,_,_,_))
114+ .Times(2);
115+ EXPECT_CALL(mock_egl, eglCreateImageKHR(disp2,_,_,_,_))
116+ .Times(1);
117+ EXPECT_CALL(mock_egl, eglDestroyImageKHR(_,_))
118+ .Times(Exactly(3));
119+
120+ mga::Buffer buffer(mock_native_buffer, extensions);
121+ buffer.bind_to_texture();
122+ buffer.bind_to_texture();
123+ buffer.bind_to_texture();
124+}

Subscribers

People subscribed via source and target branches