QtMir

Merge lp:~unity-team/qtmir/surviveEmptyTexture into lp:qtmir

  1. surviveEmptyTexture
  2. Merge into trunk
Proposed by Michał Sawicz on 2015-10-15
Status: Merged
Approved by: Gerry Boland on 2015-10-15
Approved revision: 388
Merged at revision: 390
Proposed branch: lp:~unity-team/qtmir/surviveEmptyTexture
Merge into: lp:qtmir
Prerequisite: lp:~gerboland/qtmir/multimonitor
Diff against target: 125 lines (+22/-17)
6 files modified
src/modules/Unity/Application/mirbuffersgtexture.cpp (+7/-2)
src/modules/Unity/Application/mirsurface.cpp (+8/-6)
src/modules/Unity/Application/mirsurface.h (+1/-1)
src/modules/Unity/Application/mirsurfaceinterface.h (+1/-1)
src/modules/Unity/Application/mirsurfaceitem.cpp (+4/-6)
tests/modules/common/fake_mirsurface.h (+1/-1)
To merge this branch: bzr merge lp:~unity-team/qtmir/surviveEmptyTexture
Reviewer Review Type Date Requested Status
Gerry Boland 2015-10-15 Approve on 2015-10-15
PS Jenkins bot continuous-integration 2015-10-15 Needs Fixing on 2015-10-15
Review via email: mp+274510@code.launchpad.net

This proposal supersedes a proposal from 2015-10-02.

Commit Message

MirSurfaceItem: Survive holding a surface with an empty texture

Survive having a surface whose texture holds no mir buffer at all.
Instead of crashing in such situation we simply don't render it.

Description of the Change

To solve a reasonably common I crash I get when launching a desktop unity8 session where unity8-dash, for some reason, is messed up.

Now with this patch, when this happens, I just get an empty unity8-dash window instead and closing it (which makes it respawn) solves the problem.

* Are there any related MPs required for this MP to build/function as expected? Please list.
Just the prerequisite.

* Did you perform an exploratory manual test run of your code change and any related functionality?
Yes

* If you changed the packaging (debian), did you subscribe the ubuntu-unity team to this MP?
N/A

To post a comment you must log in.
PS Jenkins bot (ps-jenkins) wrote : Posted in a previous version of this proposal
review: Needs Fixing (continuous-integration)
Gerry Boland (gerboland) wrote : Posted in a previous version of this proposal

Thing is, how do we ever end up in a position that a Mir surface has no texture available?? That is an error in my opinion.

review: Needs Information
Daniel d'Andrada (dandrader) wrote : Posted in a previous version of this proposal

> Thing is, how do we ever end up in a position that a Mir surface has no
> texture available?? That is an error in my opinion.

It is. I just haven't spent time to find the root cause.

This patch makes MirSurface[Item] code more robust. So instead of crashing because of this issue it just won't try to render it. And adding robustness is never a bad thing even if this patch doesn't try to deal with the root cause (unity8-dash in a weird state, like it didn't start up correctly or is frozen during init, I don't know).

Gerry Boland (gerboland) wrote : Posted in a previous version of this proposal

It will cause a visual glitch though. If the buffer is missing, something has gone wrong. This is just masking the core problem. I don't like this at all.

review: Disapprove
Gerry Boland (gerboland) wrote : Posted in a previous version of this proposal

This will have to do until we figure out the root cause.

review: Abstain
PS Jenkins bot (ps-jenkins) wrote : Posted in a previous version of this proposal
review: Needs Fixing (continuous-integration)
PS Jenkins bot (ps-jenkins) wrote :
review: Needs Fixing (continuous-integration)
Gerry Boland (gerboland) wrote :

I still have't tracked down the proper cause, this will have to suffice as workaround until then

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'src/modules/Unity/Application/mirbuffersgtexture.cpp'
2--- src/modules/Unity/Application/mirbuffersgtexture.cpp 2015-10-15 06:47:18 +0000
3+++ src/modules/Unity/Application/mirbuffersgtexture.cpp 2015-10-15 06:47:18 +0000
4@@ -76,12 +76,17 @@
5
6 bool MirBufferSGTexture::hasAlphaChannel() const
7 {
8- return m_mirBuffer->pixel_format() == mir_pixel_format_abgr_8888
9- || m_mirBuffer->pixel_format() == mir_pixel_format_argb_8888;
10+ if (hasBuffer()) {
11+ return m_mirBuffer->pixel_format() == mir_pixel_format_abgr_8888
12+ || m_mirBuffer->pixel_format() == mir_pixel_format_argb_8888;
13+ } else {
14+ return false;
15+ }
16 }
17
18 void MirBufferSGTexture::bind()
19 {
20+ Q_ASSERT(hasBuffer());
21 glBindTexture(GL_TEXTURE_2D, m_textureId);
22 updateBindOptions(true/* force */);
23
24
25=== modified file 'src/modules/Unity/Application/mirsurface.cpp'
26--- src/modules/Unity/Application/mirsurface.cpp 2015-10-15 06:47:18 +0000
27+++ src/modules/Unity/Application/mirsurface.cpp 2015-10-15 06:47:18 +0000
28@@ -309,17 +309,17 @@
29 }
30 }
31
32-void MirSurface::updateTexture()
33+bool MirSurface::updateTexture()
34 {
35 QMutexLocker locker(&m_mutex);
36-
37- if (m_textureUpdated) {
38- return;
39- }
40-
41 Q_ASSERT(!m_texture.isNull());
42+
43 MirBufferSGTexture *texture = static_cast<MirBufferSGTexture*>(m_texture.data());
44
45+ if (m_textureUpdated) {
46+ return texture->hasBuffer();
47+ }
48+
49 const void* const userId = (void*)123;
50 auto renderables = m_surface->generate_renderables(userId);
51
52@@ -345,6 +345,8 @@
53 }
54
55 m_textureUpdated = true;
56+
57+ return texture->hasBuffer();
58 }
59
60 void MirSurface::onCompositorSwappedBuffers()
61
62=== modified file 'src/modules/Unity/Application/mirsurface.h'
63--- src/modules/Unity/Application/mirsurface.h 2015-09-25 17:30:26 +0000
64+++ src/modules/Unity/Application/mirsurface.h 2015-10-15 06:47:18 +0000
65@@ -87,7 +87,7 @@
66 // methods called from the rendering (scene graph) thread:
67 QSharedPointer<QSGTexture> texture() override;
68 QSGTexture *weakTexture() const override { return m_texture.data(); }
69- void updateTexture() override;
70+ bool updateTexture() override;
71 unsigned int currentFrameNumber() const override;
72 bool numBuffersReadyForCompositor() override;
73 // end of methods called from the rendering (scene graph) thread
74
75=== modified file 'src/modules/Unity/Application/mirsurfaceinterface.h'
76--- src/modules/Unity/Application/mirsurfaceinterface.h 2015-09-25 17:30:26 +0000
77+++ src/modules/Unity/Application/mirsurfaceinterface.h 2015-10-15 06:47:18 +0000
78@@ -54,7 +54,7 @@
79 // methods called from the rendering (scene graph) thread:
80 virtual QSharedPointer<QSGTexture> texture() = 0;
81 virtual QSGTexture *weakTexture() const = 0;
82- virtual void updateTexture() = 0;
83+ virtual bool updateTexture() = 0;
84 virtual unsigned int currentFrameNumber() const = 0;
85 virtual bool numBuffersReadyForCompositor() = 0;
86 // end of methods called from the rendering (scene graph) thread
87
88=== modified file 'src/modules/Unity/Application/mirsurfaceitem.cpp'
89--- src/modules/Unity/Application/mirsurfaceitem.cpp 2015-09-28 14:24:48 +0000
90+++ src/modules/Unity/Application/mirsurfaceitem.cpp 2015-10-15 06:47:18 +0000
91@@ -216,17 +216,15 @@
92
93 ensureTextureProvider();
94
95- m_surface->updateTexture();
96+ if (!m_textureProvider->texture() || !m_surface->updateTexture()) {
97+ delete oldNode;
98+ return 0;
99+ }
100
101 if (m_surface->numBuffersReadyForCompositor() > 0) {
102 QTimer::singleShot(0, this, SLOT(update()));
103 }
104
105- if (!m_textureProvider->texture()) {
106- delete oldNode;
107- return 0;
108- }
109-
110 m_textureProvider->smooth = smooth();
111
112 QSGDefaultImageNode *node = static_cast<QSGDefaultImageNode*>(oldNode);
113
114=== modified file 'tests/modules/common/fake_mirsurface.h'
115--- tests/modules/common/fake_mirsurface.h 2015-09-25 17:30:26 +0000
116+++ tests/modules/common/fake_mirsurface.h 2015-10-15 06:47:18 +0000
117@@ -133,7 +133,7 @@
118 // methods called from the rendering (scene graph) thread:
119 QSharedPointer<QSGTexture> texture() override { return QSharedPointer<QSGTexture>(); }
120 QSGTexture *weakTexture() const override { return nullptr; }
121- void updateTexture() override {}
122+ bool updateTexture() override { return true; }
123 unsigned int currentFrameNumber() const override { return 0; }
124 bool numBuffersReadyForCompositor() override { return 0; }
125 // end of methods called from the rendering (scene graph) thread

Subscribers

People subscribed via source and target branches