Merge lp:~kaihengfeng/unity/lp1292830 into lp:unity

Proposed by Kai-Heng Feng on 2017-03-09
Status: Needs review
Proposed branch: lp:~kaihengfeng/unity/lp1292830
Merge into: lp:unity
Diff against target: 200 lines (+57/-7)
8 files modified
decorations/DecoratedWindow.cpp (+25/-2)
decorations/DecorationsDataPool.h (+2/-2)
decorations/DecorationsManager.cpp (+18/-0)
decorations/DecorationsManager.h (+2/-0)
decorations/DecorationsPriv.h (+4/-0)
decorations/DecorationsTitle.h (+2/-1)
decorations/DecorationsWindowButton.h (+2/-2)
plugins/unityshell/src/unityshell.cpp (+2/-0)
To merge this branch: bzr merge lp:~kaihengfeng/unity/lp1292830
Reviewer Review Type Date Requested Status
Andrea Azzarone 2017-03-09 Needs Fixing on 2017-03-28
Review via email: mp+319387@code.launchpad.net
To post a comment you must log in.
Andrea Azzarone (azzar1) wrote :

The fix looks good to be but I would prefer to not use friends class and just expose a public method, e.g. just make WindowButton::UpdateTexture public.

review: Needs Fixing
lp:~kaihengfeng/unity/lp1292830 updated on 2017-03-29
4227. By Kai-Heng Feng on 2017-03-29

Updates textures after resume from suspend. (LP: #1292830)

The Nvidia driver does not make resources in video memory persistent across S3
[1], hence applications need to update texture in vram itself.

Currently, these three decoration components require texture update:
1. Shadow.
2. Title.
3. Button.

Fortunately, Nvidia intends to fix it in the future [1]. Once the issue is
addressed, this commit can be fully reverted.

Kai-Heng Feng (kaihengfeng) wrote :

Force pushed a new version to address the issue.

Marco Trevisan (Treviño) (3v1n0) wrote :

I'm, I see the approach here, but I'm not much a fan of some things...

You told me that instead of recalling set-texture on any element, just damaging its area isn't enough? What's the problem here?
Is this due to the fact that since the texture is saved as xrandr format, then the driver just deletes that on S3?

In any case I'd prefer that this thing would be done at DecorationsWidgets level so that it can be esily applied to all elements, and not just per item.

Let me know what's feasible, and I'll try to figure out the best approach, avoiding to be too much specific.

Kai-Heng Feng (kaihengfeng) wrote :

> I'm, I see the approach here, but I'm not much a fan of some things...
>
> You told me that instead of recalling set-texture on any element, just
> damaging its area isn't enough? What's the problem here?

https://www.khronos.org/registry/OpenGL/extensions/NV/NV_robustness_video_memory_purge.txt

Looks like the link in the commit log was mistakenly deleted when I did force push, here's the excerpt:

    The NVIDIA OpenGL driver architecture on Linux has a limitation:
    resources located in video memory are not persistent across certain
    events. VT switches, suspend/resume events, and mode switching
    events may erase the contents of video memory. Any resource that
    is located exclusively in video memory, such as framebuffer objects
    (FBOs), will be lost. As the OpenGL specification makes no mention
    of events where the video memory is allowed to be cleared, the
    driver attempts to hide this fact from the application, but cannot
    do it for all resources.

> Is this due to the fact that since the texture is saved as xrandr format, then
> the driver just deletes that on S3?

This I can't answer, but I think the excerpt above should've explained it.

> In any case I'd prefer that this thing would be done at DecorationsWidgets
> level so that it can be esily applied to all elements, and not just per item.
>
> Let me know what's feasible, and I'll try to figure out the best approach,
> avoiding to be too much specific.

The link gives a super simple example:

    create_gl_context();
    create_resources();
    while (1) {
        update_world();
        if (GetGraphicsResetStatusARB() == PURGED_CONTEXT_RESET_NV) {
            delete_resources();
            create_resources();
        }
        render_frame();
    }

I am not sure if it's feasible in current architecture though - maybe logind signal is the only way to go?

Andrea Azzarone (azzar1) wrote :

> > I'm, I see the approach here, but I'm not much a fan of some things...
> >
> > You told me that instead of recalling set-texture on any element, just
> > damaging its area isn't enough? What's the problem here?
>
> https://www.khronos.org/registry/OpenGL/extensions/NV/NV_robustness_video_memo
> ry_purge.txt
>
> Looks like the link in the commit log was mistakenly deleted when I did force
> push, here's the excerpt:
>
> The NVIDIA OpenGL driver architecture on Linux has a limitation:
> resources located in video memory are not persistent across certain
> events. VT switches, suspend/resume events, and mode switching
> events may erase the contents of video memory. Any resource that
> is located exclusively in video memory, such as framebuffer objects
> (FBOs), will be lost. As the OpenGL specification makes no mention
> of events where the video memory is allowed to be cleared, the
> driver attempts to hide this fact from the application, but cannot
> do it for all resources.
>
> > Is this due to the fact that since the texture is saved as xrandr format,
> then
> > the driver just deletes that on S3?
>
> This I can't answer, but I think the excerpt above should've explained it.
>
> > In any case I'd prefer that this thing would be done at DecorationsWidgets
> > level so that it can be esily applied to all elements, and not just per
> item.
> >
> > Let me know what's feasible, and I'll try to figure out the best approach,
> > avoiding to be too much specific.
>
> The link gives a super simple example:
>
> create_gl_context();
> create_resources();
> while (1) {
> update_world();
> if (GetGraphicsResetStatusARB() == PURGED_CONTEXT_RESET_NV) {
> delete_resources();
> create_resources();
> }
> render_frame();
> }
>
> I am not sure if it's feasible in current architecture though - maybe logind
> signal is the only way to go?

I would not rely on an extension that might not be available. Btw I guess Marco he's suggesting to apply the fix inside TexturedWindow, so removiong the logic from the *Manager class. You just need a way to get the "onResume" signal there.

Kai-Heng Feng (kaihengfeng) wrote :

Nvidia put the fixes back to 375 series [1], so we no longer need this merge proposal:

"Fixed a regression that caused corruption in certain applications, such as window border shadows in Unity, after resuming from suspend."

[1] https://devtalk.nvidia.com/default/topic/1007268/b/t/post/5141478

Unmerged revisions

4227. By Kai-Heng Feng on 2017-03-29

Updates textures after resume from suspend. (LP: #1292830)

The Nvidia driver does not make resources in video memory persistent across S3
[1], hence applications need to update texture in vram itself.

Currently, these three decoration components require texture update:
1. Shadow.
2. Title.
3. Button.

Fortunately, Nvidia intends to fix it in the future [1]. Once the issue is
addressed, this commit can be fully reverted.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'decorations/DecoratedWindow.cpp'
2--- decorations/DecoratedWindow.cpp 2016-08-29 17:43:07 +0000
3+++ decorations/DecoratedWindow.cpp 2017-03-29 06:53:09 +0000
4@@ -419,10 +419,18 @@
5 top_layout_->scale = cv_->DPIScale();
6
7 if (win_->actions() & CompWindowActionCloseMask)
8- top_layout_->Append(std::make_shared<WindowButton>(win_, WindowButtonType::CLOSE));
9+ {
10+ auto close_button = std::make_shared<WindowButton>(win_, WindowButtonType::CLOSE);
11+ top_layout_->Append(close_button);
12+ close_button_ = close_button;
13+ }
14
15 if (win_->actions() & CompWindowActionMinimizeMask)
16- top_layout_->Append(std::make_shared<WindowButton>(win_, WindowButtonType::MINIMIZE));
17+ {
18+ auto minimize_button = std::make_shared<WindowButton>(win_, WindowButtonType::MINIMIZE);
19+ top_layout_->Append(minimize_button);
20+ minimize_button_ = minimize_button;
21+ }
22
23 if (win_->actions() & (CompWindowActionMaximizeHorzMask|CompWindowActionMaximizeVertMask))
24 {
25@@ -598,6 +606,21 @@
26 SyncMenusGeometries();
27 }
28
29+void Window::Impl::UpdateTopLayoutTextures()
30+{
31+ if (auto title_ptr = title_.lock())
32+ title_ptr->RenderTexture();
33+
34+ if (auto close_button_ptr = close_button_.lock())
35+ close_button_ptr->UpdateTexture();
36+
37+ if (auto minimize_button_ptr = minimize_button_.lock())
38+ minimize_button_ptr->UpdateTexture();
39+
40+ if (auto state_change_button_ptr = state_change_button_.lock())
41+ state_change_button_ptr->UpdateTexture();
42+}
43+
44 void Window::Impl::ComputeShadowQuads()
45 {
46 if (!(deco_elements_ & cu::DecorationElement::SHADOW))
47
48=== modified file 'decorations/DecorationsDataPool.h'
49--- decorations/DecorationsDataPool.h 2015-10-21 14:14:50 +0000
50+++ decorations/DecorationsDataPool.h 2017-03-29 06:53:09 +0000
51@@ -43,13 +43,13 @@
52 cu::SimpleTexture::Ptr const& ButtonTexture(WindowButtonType, WidgetState) const;
53 cu::SimpleTexture::Ptr const& ButtonTexture(double scale, WindowButtonType, WidgetState) const;
54
55+ void SetupTextures();
56+
57 private:
58 DataPool();
59 DataPool(DataPool const&) = delete;
60 DataPool& operator=(DataPool const&) = delete;
61
62- void SetupTextures();
63-
64 cu::SimpleTexture::Ptr glow_texture_;
65
66 typedef std::array<std::array<cu::SimpleTexture::Ptr, size_t(WidgetState::Size)>, size_t(WindowButtonType::Size)> WindowButtonsArray;
67
68=== modified file 'decorations/DecorationsManager.cpp'
69--- decorations/DecorationsManager.cpp 2016-07-26 11:33:50 +0000
70+++ decorations/DecorationsManager.cpp 2017-03-29 06:53:09 +0000
71@@ -90,6 +90,15 @@
72 inactive_shadow_pixmap_ = BuildShadowTexture(manager_->inactive_shadow_radius(), manager_->inactive_shadow_color());
73 }
74
75+void Manager::Impl::UpdateWindowsTextures()
76+{
77+ for (auto const& win : windows_) {
78+ win.second->impl_->bg_textures_.clear();
79+ win.second->impl_->UpdateTopLayoutTextures();
80+ win.second->impl_->UpdateDecorationTextures();
81+ }
82+}
83+
84 void Manager::Impl::OnShadowOptionsChanged(bool active)
85 {
86 if (active)
87@@ -440,6 +449,15 @@
88 .add("active_window", screen->activeWindow());
89 }
90
91+void Manager::UpdateTextures()
92+{
93+ impl_->data_pool_->SetupTextures();
94+ impl_->BuildInactiveShadowTexture();
95+ impl_->BuildActiveShadowTexture();
96+ impl_->UpdateWindowsTextures();
97+ impl_->UpdateWindowsExtents();
98+}
99+
100 debug::Introspectable::IntrospectableList Manager::GetIntrospectableChildren()
101 {
102 IntrospectableList children;
103
104=== modified file 'decorations/DecorationsManager.h'
105--- decorations/DecorationsManager.h 2014-02-13 00:51:59 +0000
106+++ decorations/DecorationsManager.h 2017-03-29 06:53:09 +0000
107@@ -54,6 +54,8 @@
108
109 Window::Ptr GetWindowByXid(::Window);
110
111+ void UpdateTextures();
112+
113 protected:
114 std::string GetName() const;
115 void AddProperties(debug::IntrospectionData&);
116
117=== modified file 'decorations/DecorationsPriv.h'
118--- decorations/DecorationsPriv.h 2016-08-29 17:43:07 +0000
119+++ decorations/DecorationsPriv.h 2017-03-29 06:53:09 +0000
120@@ -130,6 +130,7 @@
121 void ComputeGenericShadowQuads();
122 void ComputeShapedShadowQuad();
123 void UpdateDecorationTextures();
124+ void UpdateTopLayoutTextures();
125 void UpdateWindowEdgesGeo();
126 void UpdateForceQuitDialogPosition();
127 void RenderDecorationTexture(Side, nux::Geometry const&);
128@@ -167,6 +168,8 @@
129 std::shared_ptr<ForceQuitDialog> force_quit_;
130 InputMixer::Ptr input_mixer_;
131 Layout::Ptr top_layout_;
132+ uweak_ptr<WindowButton> close_button_;
133+ uweak_ptr<WindowButton> minimize_button_;
134 uweak_ptr<WindowButton> state_change_button_;
135 uweak_ptr<MenuLayout> menus_;
136 uweak_ptr<Title> title_;
137@@ -198,6 +201,7 @@
138 void UnsetAppMenu();
139 void BuildActiveShadowTexture();
140 void BuildInactiveShadowTexture();
141+ void UpdateWindowsTextures();
142 cu::PixmapTexture::Ptr BuildShadowTexture(unsigned radius, nux::Color const&);
143 void OnShadowOptionsChanged(bool active);
144 void OnWindowFrameChanged(bool, ::Window, std::weak_ptr<decoration::Window> const&);
145
146=== modified file 'decorations/DecorationsTitle.h'
147--- decorations/DecorationsTitle.h 2014-02-27 07:10:31 +0000
148+++ decorations/DecorationsTitle.h 2017-03-29 06:53:09 +0000
149@@ -38,6 +38,8 @@
150 int GetNaturalWidth() const;
151 int GetNaturalHeight() const;
152
153+ void RenderTexture();
154+
155 void Draw(GLWindow*, GLMatrix const&, GLWindowPaintAttrib const&, CompRegion const&, unsigned mask);
156
157 protected:
158@@ -47,7 +49,6 @@
159 private:
160 void OnFontChanged(std::string const&);
161 void OnTextChanged(std::string const& new_text);
162- void RenderTexture();
163
164 bool render_texture_;
165 nux::Size texture_size_;
166
167=== modified file 'decorations/DecorationsWindowButton.h'
168--- decorations/DecorationsWindowButton.h 2016-08-02 11:03:26 +0000
169+++ decorations/DecorationsWindowButton.h 2017-03-29 06:53:09 +0000
170@@ -37,6 +37,8 @@
171
172 WidgetState GetCurrentState() const;
173
174+ void UpdateTexture();
175+
176 protected:
177 void ButtonDownEvent(CompPoint const&, unsigned button, Time) override;
178 void ButtonUpEvent(CompPoint const&, unsigned button, Time) override;
179@@ -46,8 +48,6 @@
180 void AddProperties(debug::IntrospectionData&);
181
182 private:
183- void UpdateTexture();
184-
185 bool pressed_;
186 bool was_pressed_;
187 CompWindow* win_;
188
189=== modified file 'plugins/unityshell/src/unityshell.cpp'
190--- plugins/unityshell/src/unityshell.cpp 2017-02-27 13:47:32 +0000
191+++ plugins/unityshell/src/unityshell.cpp 2017-03-29 06:53:09 +0000
192@@ -472,6 +472,8 @@
193 LOG_INFO(logger) << "UnityScreen constructed: " << timer.ElapsedSeconds() << "s";
194
195 UScreen::GetDefault()->resuming.connect([this] {
196+ deco_manager_->UpdateTextures();
197+
198 /* Force paint local::FRAMES_TO_REDRAW_ON_RESUME frames on resume */
199 force_draw_countdown_ += local::FRAMES_TO_REDRAW_ON_RESUME;
200 });