Merge lp:~3v1n0/unity/keep-texture-pixmap-friends-6.0 into lp:unity/6.0

[Marco Trevisan (Treviño)]

UnityWindow: add PixmapTexture struct: never separate a binded texture to its pixmap

Or it will lead to crashes. Now we cache both the GLTexture and the binded XPixmap, so
that we delete both together and there won't be crashes.

Texutures binded to Pixmaps should not be alive when the binded pixmap has been
destroyed. So, now we have a new PixmapTexture struct that keeps both the data
structures alive.
We now use a smart pointer of PixmapTexture to cache both the temporary decorations,
inside a CairoContext, and the ones at class level.

Texutures binded to Pixmaps should not be alive when the binded pixmap has been destroyed. So, now we have a new PixmapTexture struct that keeps both the data structures alive.
We now use a smart pointer of PixmapTexture to cache both the temporary decorations, inside a CairoContext, and the ones at class level.

This is safer and cleaner than the previously added workaround.

[Neil J. Patel (njpatel)]

review: Approve

=== modified file 'plugins/unityshell/src/unityshell.cpp'
--- plugins/unityshell/src/unityshell.cpp	2012-10-02 14:09:06 +0000
+++ plugins/unityshell/src/unityshell.cpp	2012-10-04 14:10:28 +0000
@@ -200,18 +200,18 @@
       failed = true;
     }
   }
-  
+
     //In case of software rendering then enable lowgfx mode.
   std::string renderer = ANSI_TO_TCHAR(NUX_REINTERPRET_CAST(const char *, glGetString(GL_RENDERER)));
-  
-  if (strstr(renderer.c_str(), "Software Rasterizer") ||
-      strstr(renderer.c_str(), "Mesa X11") ||
-      strstr(renderer.c_str(), "LLVM") ||
-      strstr(renderer.c_str(), "on softpipe") ||
+
+  if (renderer.find("Software Rasterizer") != std::string::npos ||
+      renderer.find("Mesa X11") != std::string::npos ||
+      renderer.find("LLVM") != std::string::npos ||
+      renderer.find("on softpipe") != std::string::npos ||
       (getenv("UNITY_LOW_GFX_MODE") != NULL && atoi(getenv("UNITY_LOW_GFX_MODE")) == 1))
-      {
-        Settings::Instance().SetLowGfxMode(true);
-      }
+    {
+      Settings::Instance().SetLowGfxMode(true);
+    }
 #endif
 
   if (!failed)
@@ -3406,32 +3406,51 @@
 }
 }
 
+struct UnityWindow::PixmapTexture
+{
+  PixmapTexture(unsigned width, unsigned height)
+    : w_(width)
+    , h_(height)
+    , pixmap_(XCreatePixmap(screen->dpy(), screen->root(), w_, h_, 32))
+    , texture_(GLTexture::bindPixmapToTexture(pixmap_, w_, h_, 32))
+  {}
+
+  ~PixmapTexture()
+  {
+    texture_.clear();
+
+    if (pixmap_)
+      XFreePixmap(screen->dpy(), pixmap_);
+  }
+
+  unsigned w_;
+  unsigned h_;
+  Pixmap pixmap_;
+  GLTexture::List texture_;
+};
+
 struct UnityWindow::CairoContext
 {
   CairoContext(unsigned width, unsigned height)
     : w_(width)
     , h_(height)
-    , pixmap_(XCreatePixmap(screen->dpy(), screen->root(), w_, h_, 32))
-    , texture_(GLTexture::bindPixmapToTexture(pixmap_, w_, h_, 32, compiz::opengl::ExternallyManaged)) //FIXME: This is a workaround, should create a new wrapper class which owns the texture and the cairo context.
+    , pixmap_texture_(std::make_shared<PixmapTexture>(w_, h_))
     , surface_(nullptr)
     , cr_(nullptr)
   {
-      Screen *xscreen = ScreenOfDisplay(screen->dpy(), screen->screenNum());
-      XRenderPictFormat* format = XRenderFindStandardFormat(screen->dpy(), PictStandardARGB32);
-
-      if (texture_.empty())
-        return;
-
-      surface_ = cairo_xlib_surface_create_with_xrender_format(screen->dpy(), pixmap_,
-                                                               xscreen, format,
-                                                               width, height);
-      cr_ = cairo_create(surface_);
-
-      // clear
-      cairo_save(cr_);
-      cairo_set_operator(cr_, CAIRO_OPERATOR_CLEAR);
-      cairo_paint(cr_);
-      cairo_restore(cr_);
+    Screen *xscreen = ScreenOfDisplay(screen->dpy(), screen->screenNum());
+    XRenderPictFormat* format = XRenderFindStandardFormat(screen->dpy(), PictStandardARGB32);
+    surface_ = cairo_xlib_surface_create_with_xrender_format(screen->dpy(),
+                                                             pixmap_texture_->pixmap_,
+                                                             xscreen, format,
+                                                             w_, h_);
+    cr_ = cairo_create(surface_);
+
+    // clear
+    cairo_save(cr_);
+    cairo_set_operator(cr_, CAIRO_OPERATOR_CLEAR);
+    cairo_paint(cr_);
+    cairo_restore(cr_);
   }
 
   ~CairoContext()
@@ -3441,15 +3460,11 @@
 
     if (surface_)
       cairo_surface_destroy(surface_);
-
-    if (pixmap_)
-      XFreePixmap(screen->dpy(), pixmap_);
   }
 
   unsigned w_;
   unsigned h_;
-  Pixmap pixmap_;
-  GLTexture::List texture_;
+  PixmapTexturePtr pixmap_texture_;
   cairo_surface_t* surface_;
   cairo_t *cr_;
 };
@@ -3665,16 +3680,16 @@
 
 void UnityWindow::BuildDecorationTexture()
 {
-  if (!decoration_tex_.empty())
+  if (decoration_tex_)
     return;
 
   auto const& border_extents = window->border();
 
   if (WindowManager::Default()->IsWindowDecorated(window->id()) && border_extents.top > 0)
   {
-    CairoContext context(window->borderRect().width(), border_extents.top);
+    CairoContext context(window->borderRect().width(), window->border().top);
     RenderDecoration(context);
-    decoration_tex_ = context.texture_;
+    decoration_tex_ = context.pixmap_texture_;
   }
 }
 
@@ -3726,8 +3741,8 @@
 
 void UnityWindow::CleanupCachedTextures()
 {
-  decoration_tex_.clear();
-  decoration_selected_tex_.clear();
+  decoration_tex_.reset();
+  decoration_selected_tex_.reset();
   decoration_title_.clear();
 }
 
@@ -3736,7 +3751,7 @@
                                        CompRegion const& region,
                                        unsigned int mask)
 {
-  ScaleWindow *scale_win = ScaleWindow::get(window);
+  ScaleWindow* scale_win = ScaleWindow::get(window);
   scale_win->scalePaintDecoration(attrib, transform, region, mask);
 
   if (!scale_win->hasSlot()) // animation not finished
@@ -3760,21 +3775,22 @@
   if (!highlighted)
   {
     BuildDecorationTexture();
-    DrawTexture(decoration_tex_, attrib, transform, mask, x, y, pos.scale);
+
+    if (decoration_tex_)
+      DrawTexture(decoration_tex_->texture_, attrib, transform, mask, x, y, pos.scale);
+
     close_button_geo_.Set(0, 0, 0, 0);
   }
   else
   {
     auto const& decoration_extents = window->border();
-    int width = scaled_geo.width;
-    int height = decoration_extents.top;
+    unsigned width = scaled_geo.width;
+    unsigned height = decoration_extents.top;
     bool redraw_decoration = true;
 
-    if (!decoration_selected_tex_.empty())
+    if (decoration_selected_tex_)
     {
-      GLTexture* texture = decoration_selected_tex_.front();
-
-      if (texture->width() == width && texture->height() == height)
+      if (decoration_selected_tex_->w_ == width && decoration_selected_tex_->h_ == height)
       {
         if (decoration_title_ == WindowManager::Default()->GetWindowName(window->id()))
           redraw_decoration = false;
@@ -3783,16 +3799,24 @@
 
     if (redraw_decoration)
     {
-      CairoContext context(width, height);
-      RenderDecoration(context, pos.scale);
+      if (width != 0 && height != 0)
+      {
+        CairoContext context(width, height);
+        RenderDecoration(context, pos.scale);
 
-      // Draw window title
-      int text_x = scale::decoration::ITEMS_PADDING * 2 + scale::decoration::CLOSE_SIZE;
-      RenderText(context, text_x, 0.0, width - scale::decoration::ITEMS_PADDING, height);
-      decoration_selected_tex_ = context.texture_;
+        // Draw window title
+        int text_x = scale::decoration::ITEMS_PADDING * 2 + scale::decoration::CLOSE_SIZE;
+        RenderText(context, text_x, 0.0, width - scale::decoration::ITEMS_PADDING, height);
+        decoration_selected_tex_ = context.pixmap_texture_;
+      }
+      else
+      {
+        decoration_selected_tex_.reset();
+      }
     }
 
-    DrawTexture(decoration_selected_tex_, attrib, transform, mask, x, y);
+    if (decoration_selected_tex_)
+      DrawTexture(decoration_selected_tex_->texture_, attrib, transform, mask, x, y);
 
     x += scale::decoration::ITEMS_PADDING;
     y += (height - scale::decoration::CLOSE_SIZE) / 2.0f;
@@ -3819,7 +3843,7 @@
 
 nux::Geometry UnityWindow::GetScaledGeometry()
 {
-  ScaleWindow *scale_win = ScaleWindow::get(window);
+  ScaleWindow* scale_win = ScaleWindow::get(window);
 
   ScalePosition const& pos = scale_win->getCurrentPosition();
   auto const& border_rect = window->borderRect();
 
=== modified file 'plugins/unityshell/src/unityshell.h'
--- plugins/unityshell/src/unityshell.h	2012-09-28 22:33:55 +0000
+++ plugins/unityshell/src/unityshell.h	2012-10-04 14:10:28 +0000
@@ -406,6 +406,8 @@
 private:
   typedef compiz::CompizMinimizedWindowHandler<UnityScreen, UnityWindow>
           UnityMinimizedHandler;
+  struct PixmapTexture;
+  typedef std::shared_ptr<PixmapTexture> PixmapTexturePtr;
   struct CairoContext;
 
   void DoEnableFocus ();
@@ -460,8 +462,8 @@
   static GLTexture::List close_normal_tex_;
   static GLTexture::List close_prelight_tex_;
   static GLTexture::List close_pressed_tex_;
-  GLTexture::List decoration_tex_;
-  GLTexture::List decoration_selected_tex_;
+  PixmapTexturePtr decoration_tex_;
+  PixmapTexturePtr decoration_selected_tex_;
   std::string decoration_title_;
   compiz::WindowInputRemoverLock::Weak input_remover_;
   panel::WindowState close_icon_state_;