Ubuntu UI Toolkit

Merge lp:~mardy/ubuntu-ui-toolkit/lp1296728 into lp:ubuntu-ui-toolkit/staging

lp1296728
Merge into staging

Proposed by Alberto Mardegan on 2014-05-07

Status:

Merged

Approved by:

Loïc Molinari on 2014-05-20

Approved revision:

978

Merged at revision:

1063

Proposed branch:

lp:~mardy/ubuntu-ui-toolkit/lp1296728

Merge into:

lp:ubuntu-ui-toolkit/staging

Diff against target:

165 lines (+57/-16)

3 files modified

modules/Ubuntu/Components/plugin/shapeitem.cpp (+47/-13)
modules/Ubuntu/Components/plugin/shapeitem.h (+8/-0)
modules/Ubuntu/Components/plugin/shapeitemtexture.h (+2/-3)

To merge this branch:

bzr merge lp:~mardy/ubuntu-ui-toolkit/lp1296728

Undecided

Fix Released

Link a bug report

Reviewer	Review Type	Date Requested	Status
PS Jenkins bot	continuous-integration	2014-05-07	Approve on 2014-05-20
Loïc Molinari (community)		2014-05-07	Approve on 2014-05-20
Review via email: mp+218617@code.launchpad.net

This proposal supersedes a proposal from 2014-03-24.

Description of the change

Keep a map of windows and associated textures

It seems it's not possible to use a texture which was created for another
window; therefore, each window must keep record of its textures.

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2014-03-24: Posted in a previous version of this proposal

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/ubuntu-ui-toolkit-ci/1936/rebuild

review: Approve (continuous-integration)

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2014-05-07:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/ubuntu-sdk-team-ubuntu-ui-toolkit-staging-ci/181/rebuild

review: Approve (continuous-integration)

Revision history for this message

Loïc Molinari (loic.molinari) wrote on 2014-05-08:

Hi Alberto,

QtQuick used to create one OpenGL context for all the windows (QQuickViews), now it creates one context per window, which breaks our static allocation. I guess that's the main reason for that change.

1) I think we should handle that per context (window()->openglContext() with the right NULL checks) instead of per window and use the context pointer as the hash key to prevent further changes.

2) We should also clean up the created textures when the context is (about to be) destroyed so that there are no false positives when debugging with QML_LEAK_CHECK on, the only thing to do is to connect to the aboutToBeDestroyed() signal of the QOpenGLContext instance and delete "high" and "low" in the callback. Note that this signal is emitted from the rendering thread (with has the OpenGL context bound) and for delete not to crash, the connection must be direct (adding Qt:DirectConnection as last parameter of connect()). No need to connect to the destroyed() signal on the QQuickWindow instance anymore.

3) Can you please replace the comment from line 359 to something like "OpenGL allocates textures per context, so we store textures reused by all shape instances per context as well".

4) Now for the cosmetics, can you please use C++ style comments and "type* pointer" instead of "type *pointer" in the shape code (as it used to be when I first wrote that stuff).

Have fun ;)

review: Needs Fixing

Revision history for this message

Loïc Molinari (loic.molinari) wrote on 2014-05-08:

1) By further changes, I meant further changes in the QtQuick rendering engine.

lp:~mardy/ubuntu-ui-toolkit/lp1296728 updated on 2014-05-09

978. By Alberto Mardegan on 2014-05-09: Address review comments

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2014-05-09:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/ubuntu-sdk-team-ubuntu-ui-toolkit-staging-ci/193/rebuild

review: Approve (continuous-integration)

Revision history for this message

Loïc Molinari (loic.molinari) on 2014-05-20:

review: Approve

Revision history for this message

PS Jenkins bot (ps-jenkins) on 2014-05-20:

review: Approve (continuous-integration)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk

Subscribers

People subscribed via source and target branches

to all changes:

Alberto Mardegan

Dan Chapman 

Tom Mirmar

Ubuntu SDK team

 === modified file 'modules/Ubuntu/Components/plugin/shapeitem.cpp'
 --- modules/Ubuntu/Components/plugin/shapeitem.cpp	2014-04-08 18:41:31 +0000
 +++ modules/Ubuntu/Components/plugin/shapeitem.cpp	2014-05-09 08:10:26 +0000
@@ -32,6 +32,9 @@
  // Threshold in grid unit defining the texture quality to be used.
  const float lowHighTextureThreshold = 11.0f;
++// Map of windows and associated textures.
++QHash<QOpenGLContext*, ShapeItem::TextureHandles> ShapeItem::textures_;
++
  static const char* const shapeVertexShader =
      "uniform lowp mat4 matrix;                  \n"
      "attribute lowp vec4 positionAttrib;        \n"
@@ -338,6 +341,21 @@
      update();
+ }
++void ShapeItem::onOpenglContextDestroyed()
++{
++    QOpenGLContext* context = qobject_cast<QOpenGLContext*>(sender());
++    if (Q_UNLIKELY(!context)) return;
++
++    QHash<QOpenGLContext*, TextureHandles>::iterator it =
++        textures_.find(context);
++    if (it != textures_.end()) {
++        TextureHandles &textureHandles = it.value();
++        delete textureHandles.high;
++        delete textureHandles.low;
++        textures_.erase(it);
++    }
++}
++
  QSGNode* ShapeItem::updatePaintNode(QSGNode* old_node, UpdatePaintNodeData* data)
+ {
      Q_UNUSED(data);
@@ -345,19 +363,28 @@
      // FIXME(loicm) Shape textures are stored in the read-only data section of the plugin as it
      //     avoids having to deal with paths for now. It should preferably be loaded from a file.
--    /* Textures created with QWindow::createTextureFromImage() become invalid
--     * when the window is destroyed; therefore, we must keep track of which
--     * window was used to create them and be ready to re-create them if that
--     * window goes away. */
--    static QPointer<QWindow> textureOwner = 0;
--    if (!textureOwner) {
--        shapeTextureHigh.texture = window()->createTextureFromImage(
++    // OpenGL allocates textures per context, so we store textures reused by
++    // all shape instances per context as well
++    QOpenGLContext* openglContext = window() ? window()->openglContext() : NULL;
++    if (Q_UNLIKELY(!openglContext)) {
++        qCritical() << "Window has no GL context!";
++        delete old_node;
++        return NULL;
++    }
++
++    TextureHandles &textureHandles = textures_[openglContext];
++    // If the hash table didn't contain an entry for the current context, the
++    // line above has just caused the creation of a default-constructed value.
++    if (!textureHandles.high) {
++        textureHandles.high = window()->createTextureFromImage(
              QImage(shapeTextureHigh.data, shapeTextureHigh.width, shapeTextureHigh.height,
                     QImage::Format_ARGB32_Premultiplied));
--        shapeTextureLow.texture = window()->createTextureFromImage(
++        textureHandles.low = window()->createTextureFromImage(
              QImage(shapeTextureLow.data, shapeTextureLow.width, shapeTextureLow.height,
                     QImage::Format_ARGB32_Premultiplied));
--        textureOwner = window();
++        QObject::connect(openglContext, SIGNAL(aboutToBeDestroyed()),
++                         this, SLOT(onOpenglContextDestroyed()),
++                         Qt::DirectConnection);
+     }
      // The image item sets its texture in its updatePaintNode() method when QtQuick iterates through
@@ -387,8 +414,15 @@
      ShapeTexturedMaterial* texturedMaterial = node->texturedMaterial();
      ShapeColoredMaterial* coloredMaterial = node->coloredMaterial();
--    TextureData* textureData = (gridUnit_ > lowHighTextureThreshold) ?
--        &shapeTextureHigh : &shapeTextureLow;
++    TextureData* textureData;
++    QSGTexture* textureHandle;
++    if (gridUnit_ > lowHighTextureThreshold) {
++        textureData = &shapeTextureHigh;
++        textureHandle = textureHandles.high;
++    } else {
++        textureData = &shapeTextureLow;
++        textureHandle = textureHandles.low;
++    }
      // Set the shape texture to be used by the materials depending on current grid unit. The radius
      // is set considering the current grid unit and the texture raster grid unit. When the item size
@@ -406,8 +440,8 @@
          radius = halfMinWidthHeight;
          scaledDown |= 1;
+     }
--    coloredMaterial->setShapeTexture(textureData->texture, !!scaledDown);
--    texturedMaterial->setShapeTexture(textureData->texture, !!scaledDown);
++    coloredMaterial->setShapeTexture(textureHandle, !!scaledDown);
++    texturedMaterial->setShapeTexture(textureHandle, !!scaledDown);
      // Update the other material properties.
      coloredMaterial->setColor(color_);
 === modified file 'modules/Ubuntu/Components/plugin/shapeitem.h'
 --- modules/Ubuntu/Components/plugin/shapeitem.h	2013-10-18 08:56:39 +0000
 +++ modules/Ubuntu/Components/plugin/shapeitem.h	2014-05-09 08:10:26 +0000
@@ -91,6 +91,7 @@
  private Q_SLOTS:
      void onImagePropertiesChanged();
++    void onOpenglContextDestroyed();
  private:
      enum DirtyFlags {
@@ -110,6 +111,12 @@
                                | DirtyGridUnit | DirtyGeometry)
      };
++    struct TextureHandles {
++        TextureHandles(): high(0), low(0) {}
++        QSGTexture* high;
++        QSGTexture* low;
++    };
++
      QColor color_;
      QColor gradientColor_;
      bool gradientColorSet_;
@@ -124,6 +131,7 @@
      float gridUnit_;
      QRectF geometry_;
      QFlags<DirtyFlags> dirtyFlags_;
++    static QHash<QOpenGLContext*, TextureHandles> textures_;
      Q_DISABLE_COPY(ShapeItem)
  };
 === modified file 'modules/Ubuntu/Components/plugin/shapeitemtexture.h'
 --- modules/Ubuntu/Components/plugin/shapeitemtexture.h	2013-01-07 07:59:56 +0000
 +++ modules/Ubuntu/Components/plugin/shapeitemtexture.h	2014-05-09 08:10:26 +0000
@@ -20,7 +20,6 @@
  struct TextureData {
      const unsigned char* const data;
--    QSGTexture* texture;
      int width;
      int height;
      int bytesPerPixel;
@@ -5160,7 +5159,7 @@
  // High resolution shape texture.
  TextureData shapeTextureHigh __attribute__((aligned(16))) = {
--    shapeTextureHighData, NULL, 256, 128, 4, 32.0f, 64.0f, 18.0f,
++    shapeTextureHighData, 256, 128, 4, 32.0f, 64.0f, 18.0f,
+     {
          {  // Medium raw coords.
              { hh, 0.0f }, { 0.25f-hh, 0.0f }, { 0.25f-hh, 0.0f }, { hh, 0.0f },
@@ -6484,7 +6483,7 @@
  // Low resolution shape texture.
  TextureData shapeTextureLow __attribute__((aligned(16))) = {
--    shapeTextureLowData, NULL, 128, 64, 4, 16.0f, 32.0f, 9.0f,
++    shapeTextureLowData, 128, 64, 4, 16.0f, 32.0f, 9.0f,
+     {
          {  // Medium raw coords.
              { hl, 0.0f }, { 0.25f-hl, 0.0f }, { 0.25f-hl, 0.0f }, { hl, 0.0f },

Ubuntu UI Toolkit

Merge lp:~mardy/ubuntu-ui-toolkit/lp1296728 into lp:ubuntu-ui-toolkit/staging

Commit message

Description of the change

Preview Diff

Subscribers