Mir

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

Proposed by Kevin DuBois on 2014-01-08
Status: Merged
Approved by: Kevin DuBois on 2014-01-08
Approved revision: 1318
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 2014-01-08 Approve on 2014-01-08
Gerry Boland (community) functional Approve on 2014-01-08
Andreas Pokorny (community) Approve on 2014-01-08
Alan Griffiths Needs Fixing on 2014-01-08
Alexandros Frantzis (community) Approve on 2014-01-08
Daniel van Vugt Approve on 2014-01-08
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.
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)
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
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
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
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);

Andreas Pokorny (andreas-pokorny) wrote :

LGTM

review: Approve
Gerry Boland (gerboland) wrote :

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

review: Approve (functional)
Kevin DuBois (kdub) wrote :

flaky test

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