ubuntu-touch-coreapps-reviewers team mailing list archive

[Stefano Verzegnassi <stefano92.100@xxxxxxxxx>]

Review: Needs Fixing

It is definitely more stable than before. Anyway I've been able to make it crash.

Here's the log: http://paste.ubuntu.com/12465531/
Steps to reproduce it:
1) Open Document Viewer
2) Open a LibreOffice text document (.odt, .doc, .docx). It needs to contain more than one page.
3) Scroll the flickable.
4) When the flickable is still scrolling, close the viewer and go back to the "browser" page.
5) LibreOffice crashes.

It does not seem to happen with all the documents (they need to be a lot complex - e.g. tables, images, etc.).
I'll provide you the document I tested privately (since it's supposed to be confidential).

Anyway, having a look at the log, it doesn't seem to be caused by the changes proposed in this MP, so it's not a valid reason to delay the merging of this branch.

We will probably need to do further investigation on LibreOfficeKit, in order to see if there's a way to prevent the crash.

----

I've left 7 inline comments, about things that may require a fix.
I'm sorry for being so pedantic, don't hate me for this. :P

That said, it looks already great and that RenderEngine class helps me a lot in implementing some change for the lok::part thumbnailer.



Diff comments:

> 
> === modified file 'src/plugin/libreofficetoolkit-qml-plugin/loview.cpp'
> --- src/plugin/libreofficetoolkit-qml-plugin/loview.cpp	2015-09-11 12:35:10 +0000
> +++ src/plugin/libreofficetoolkit-qml-plugin/loview.cpp	2015-09-19 08:53:46 +0000
> @@ -69,21 +70,28 @@
>      Q_EMIT parentFlickableChanged();
>  }
>  
> +void LOView::initializeDocument(const QString &path)
> +{
> +    m_document = QSharedPointer<LODocument>(new LODocument());
> +    m_document->setPath(path);
> +    Q_EMIT documentChanged();
> +}
> +
>  // Return the LODocument rendered by this class
>  LODocument* LOView::document() const
>  {
> -    return m_document;
> +    return m_document.data();
>  }
>  
>  // Set the LODocument
> -void LOView::setDocument(LODocument *doc)
> -{
> -    if (m_document == doc)
> -        return;
> +//void LOView::setDocument(LODocument *doc)

Can we remove the commented lines?

> +//{
> +//    if (m_document == doc)
> +//        return;
>  
> -    m_document = doc;
> -    Q_EMIT documentChanged();
> -}
> +//    m_document = QSharedPointer<LODocument>(doc);
> +//    Q_EMIT documentChanged();
> +//}
>  
>  // Not used yet.
>  qreal LOView::zoomFactor() const
> 
> === modified file 'src/plugin/libreofficetoolkit-qml-plugin/loview.h'
> --- src/plugin/libreofficetoolkit-qml-plugin/loview.h	2015-09-11 12:42:47 +0000
> +++ src/plugin/libreofficetoolkit-qml-plugin/loview.h	2015-09-19 08:53:46 +0000
> @@ -41,8 +44,11 @@
>      QQuickItem* parentFlickable() const;
>      void        setParentFlickable(QQuickItem* flickable);
>  
> +    Q_INVOKABLE void initializeDocument(const QString& path);
> +
> +    // TODO REWORK

Has it been done? If yes, please remove the TODO comment.

>      LODocument* document() const;
> -    void        setDocument(LODocument* doc);
> +//    void        setDocument(LODocument* doc);

Remove comment.

>  
>      qreal       zoomFactor() const;
>      void        setZoomFactor(qreal zoom);
> @@ -60,23 +66,24 @@
>      void updateViewSize();
>      void updateVisibleRect();
>      void scheduleVisibleRectUpdate();
> +    void renderResultReceived(int id, QImage img);
>  
>  private:
> -    QQuickItem*             m_parentFlickable;
> -    LODocument*             m_document;
> -
> -    qreal                   m_zoomFactor;
> -    int                     m_cacheBuffer;
> -
> -    QRect                   m_visibleArea;
> -    QRect                   m_bufferArea;
> -
> -    QTimer                  m_updateTimer;
> -
> -    QMap<int, SGTileItem*>    m_tiles;
> -
> -    void                    generateTiles(int x1, int y1, int x2, int y2, int tilesPerWidth);
> -    void                    createTile(int index, QRect rect);
> +    QQuickItem*                 m_parentFlickable;
> +    QSharedPointer<LODocument>  m_document;
> +
> +    qreal                       m_zoomFactor;
> +    int                         m_cacheBuffer;
> +
> +    QRect                       m_visibleArea;
> +    QRect                       m_bufferArea;
> +
> +    QTimer                      m_updateTimer;
> +
> +    QMap<int, SGTileItem*>      m_tiles;
> +
> +    void generateTiles(int x1, int y1, int x2, int y2, int tilesPerWidth);

Thank you for re-aligning this. Looks better now! :)

> +    void createTile(int index, QRect rect);
>  };
>  
>  #endif // LOVIEW_H
> 
> === modified file 'src/plugin/libreofficetoolkit-qml-plugin/qml/Viewer.qml'
> --- src/plugin/libreofficetoolkit-qml-plugin/qml/Viewer.qml	2015-09-10 12:22:49 +0000
> +++ src/plugin/libreofficetoolkit-qml-plugin/qml/Viewer.qml	2015-09-19 08:53:46 +0000
> @@ -24,6 +24,13 @@
>      property alias zoomFactor:  view.zoomFactor
>      property alias cacheBuffer: view.cacheBuffer
>  
> +    property string documentPath: ""

Could this be done from C++, or there's a specific reason?

In case, there's no urge to fix it now, since we have highest priorities.
But please add a TODO comment, so we have a reminder for the task.

> +
> +    onDocumentPathChanged: {
> +        if (documentPath)
> +            view.initializeDocument(documentPath)
> +    }
> +
>      contentHeight: view.height * view.zoomFactor
>      contentWidth: view.width * view.zoomFactor
>  
> 
> === added file 'src/plugin/libreofficetoolkit-qml-plugin/renderengine.cpp'
> --- src/plugin/libreofficetoolkit-qml-plugin/renderengine.cpp	1970-01-01 00:00:00 +0000
> +++ src/plugin/libreofficetoolkit-qml-plugin/renderengine.cpp	2015-09-19 08:53:46 +0000
> @@ -0,0 +1,56 @@
> +#include "renderengine.h"

I love it, well done! :D

> +#include <QtConcurrent/QtConcurrent>
> +#include <QThread>
> +
> +RenderEngine* RenderEngine::s_instance = nullptr;
> +
> +RenderEngine::RenderEngine():
> +    QObject(nullptr),
> +    m_activeTaskCount(0)
> +{
> +    int itc = QThread::idealThreadCount();
> +    m_idealThreadCount = itc == -1 ? DefaultIdealThreadCount : itc;
> +}
> +
> +void RenderEngine::enqueueTask(const QSharedPointer<LODocument>& doc, const QRect& area, int id)
> +{
> +    Q_ASSERT(doc != nullptr);
> +
> +    m_queue.enqueue(EngineTask(doc, area, id));
> +
> +    doNextTask();
> +}
> +
> +void RenderEngine::dequeueTask(int id)
> +{
> +    for (int i = 0; i < m_queue.size(); i++)
> +        if (m_queue.at(i).id == id) {
> +            m_queue.removeAt(i);
> +            break;
> +        }
> +}
> +
> +void RenderEngine::internalRenderCallback(int id, QImage img)
> +{
> +    m_activeTaskCount--;
> +    Q_EMIT renderFinished(id, img);
> +    doNextTask();
> +}
> +
> +void RenderEngine::doNextTask()
> +{
> +#ifdef DEBUG_VERBOSE
> +    qDebug() << " ---- doNextTask" << m_activeTaskCount << m_queue.count();
> +#endif
> +
> +    if (m_activeTaskCount >= m_idealThreadCount || !m_queue.count())
> +        return;
> +
> +    m_activeTaskCount++;
> +    auto task = m_queue.dequeue();
> +
> +    QtConcurrent::run( [=] {
> +        QImage img = task.document->paintTile(task.area.size(), task.area);
> +        QMetaObject::invokeMethod(this, "internalRenderCallback", Q_ARG(int, task.id), Q_ARG(QImage, img));
> +    });
> +}
> 
> === modified file 'src/plugin/libreofficetoolkit-qml-plugin/sgtileitem.cpp'
> --- src/plugin/libreofficetoolkit-qml-plugin/sgtileitem.cpp	2015-09-11 12:35:10 +0000
> +++ src/plugin/libreofficetoolkit-qml-plugin/sgtileitem.cpp	2015-09-19 08:53:46 +0000
> @@ -20,99 +21,135 @@
>  SGTileItem::~SGTileItem()
>  { }
>  
> -void SGTileItem::dispose()
> -{
> -    if (m_state.loadAcquire() != SgstRendering)
> -        deleteLater();
> -    m_state.storeRelease(SgstDisposed);
> -
> -#ifdef DEBUG_VERBOSE
> -    qDebug() << "---- dispose called: " << this << m_state;
> -#endif
> -}
> -
>  QSGNode *SGTileItem::updatePaintNode(QSGNode *oldNode, QQuickItem::UpdatePaintNodeData *)
>  {
>      QSGSimpleTextureNode* node = static_cast<QSGSimpleTextureNode*>(oldNode);
>      QQuickWindow* wnd = window();
>  
> -    if (!node && wnd) {
> -        if (this->m_state.loadAcquire() == SgstInitial) {
> -            m_state.storeRelease(SgstRendering);
> -
> -            QtConcurrent::run( [=] {
> -                if (!m_document)
> -                    return;
> -
> -                QImage img;
> -
> -                // By this time already can be disposed, so it's better to ckeck again.
> -                if (this->m_state.loadAcquire() == SgstRendering)
> -                    img = m_document->paintTile(this->area().size(), this->area());
> -
> -#ifdef DEBUG_VERBOSE
> -                else if (this->m_state.loadAcquire() == SgstDisposed)
> -                    qDebug() << "Already disposed:" << m_state.loadAcquire();
> -#endif
> -
> -                QMetaObject::invokeMethod(this, "renderCallback", Q_ARG(QImage, img));
> -            });
> -        } else if (m_state.loadAcquire() == SgstActive) {
> -            QImage image = m_data;
> -            auto texture = wnd->createTextureFromImage(image);
> -            node = new QSGSimpleTextureNode();
> -            node->setTexture(texture);
> -            node->setOwnsTexture(true);
> -            node->setRect(m_area);
> +    if (!node && wnd && !m_data.isNull()) {
> +        QImage image = m_data;
> +        auto texture = wnd->createTextureFromImage(image);
> +        node = new QSGSimpleTextureNode();
> +        node->setTexture(texture);
> +        node->setOwnsTexture(true);
> +        node->setRect(m_area);
>  
>  #ifdef DEBUG_SHOW_TILE_BORDER
> -            auto tileBorderGeometry = new QSGGeometry(QSGGeometry::defaultAttributes_Point2D(), 8);
> -            tileBorderGeometry->setDrawingMode(GL_LINES);
> -            tileBorderGeometry->setLineWidth(4);
> -
> -            QSGGeometry::Point2D* vertex = tileBorderGeometry->vertexDataAsPoint2D();
> -            vertex[0].set(node->rect().left(), node->rect().top());
> -            vertex[1].set(node->rect().left(), node->rect().bottom());
> -
> -            vertex[2].set(node->rect().right(), node->rect().top());
> -            vertex[3].set(node->rect().right(), node->rect().bottom());
> -
> -            vertex[4].set(vertex[0].x, vertex[0].y);
> -            vertex[5].set(vertex[2].x, vertex[2].y);
> -
> -            vertex[6].set(vertex[1].x, vertex[1].y);
> -            vertex[7].set(vertex[3].x, vertex[3].y);
> -
> -            auto tileBorderMaterial = new QSGFlatColorMaterial;
> -            tileBorderMaterial->setColor(Qt::red);
> -
> -            auto tileBorderNode = new QSGGeometryNode;
> -
> -            tileBorderNode->setGeometry(tileBorderGeometry);
> -            tileBorderNode->setFlag(QSGNode::OwnsGeometry);
> -
> -            tileBorderNode->setMaterial(tileBorderMaterial);
> -            tileBorderNode->setFlag(QSGNode::OwnsMaterial);
> -
> -            node->appendChildNode(tileBorderNode);
> +        drawTileBorders(node);
>  #endif
> -        }
>      }
>  
>      return node;
>  }
>  
> +//QSGNode *SGTileItem::updatePaintNode(QSGNode *oldNode, QQuickItem::UpdatePaintNodeData *)

These lines could be removed too.
In case, we still have this code in the earlier commits.

> +//{
> +//    QSGSimpleTextureNode* node = static_cast<QSGSimpleTextureNode*>(oldNode);
> +//    QQuickWindow* wnd = window();
> +
> +//    if (!node && wnd) {
> +//        if (this->m_state.loadAcquire() == SgstInitial) {
> +//            m_state.storeRelease(SgstRendering);
> +
> +//            QtConcurrent::run( [=] {
> +//                if (!m_document)
> +//                    return;
> +
> +//                QImage img;
> +
> +//                // By this time already can be disposed, so it's better to ckeck again.
> +//                if (this->m_state.loadAcquire() == SgstRendering)
> +//                    img = m_document->paintTile(this->area().size(), this->area());
> +
> +//#ifdef DEBUG_VERBOSE
> +//                else if (this->m_state.loadAcquire() == SgstDisposed)
> +//                    qDebug() << "Already disposed:" << m_state.loadAcquire();
> +//#endif
> +
> +//                QMetaObject::invokeMethod(this, "renderCallback", Q_ARG(QImage, img));
> +//            });
> +//        } else if (m_state.loadAcquire() == SgstActive) {
> +//            QImage image = m_data;
> +//            auto texture = wnd->createTextureFromImage(image);
> +//            node = new QSGSimpleTextureNode();
> +//            node->setTexture(texture);
> +//            node->setOwnsTexture(true);
> +//            node->setRect(m_area);
> +
> +//#ifdef DEBUG_SHOW_TILE_BORDER
> +//            auto tileBorderGeometry = new QSGGeometry(QSGGeometry::defaultAttributes_Point2D(), 8);
> +//            tileBorderGeometry->setDrawingMode(GL_LINES);
> +//            tileBorderGeometry->setLineWidth(4);
> +
> +//            QSGGeometry::Point2D* vertex = tileBorderGeometry->vertexDataAsPoint2D();
> +//            vertex[0].set(node->rect().left(), node->rect().top());
> +//            vertex[1].set(node->rect().left(), node->rect().bottom());
> +
> +//            vertex[2].set(node->rect().right(), node->rect().top());
> +//            vertex[3].set(node->rect().right(), node->rect().bottom());
> +
> +//            vertex[4].set(vertex[0].x, vertex[0].y);
> +//            vertex[5].set(vertex[2].x, vertex[2].y);
> +
> +//            vertex[6].set(vertex[1].x, vertex[1].y);
> +//            vertex[7].set(vertex[3].x, vertex[3].y);
> +
> +//            auto tileBorderMaterial = new QSGFlatColorMaterial;
> +//            tileBorderMaterial->setColor(Qt::red);
> +
> +//            auto tileBorderNode = new QSGGeometryNode;
> +
> +//            tileBorderNode->setGeometry(tileBorderGeometry);
> +//            tileBorderNode->setFlag(QSGNode::OwnsGeometry);
> +
> +//            tileBorderNode->setMaterial(tileBorderMaterial);
> +//            tileBorderNode->setFlag(QSGNode::OwnsMaterial);
> +
> +//            node->appendChildNode(tileBorderNode);
> +//#endif
> +//        }
> +//    }
> +
> +//    return node;
> +//}
> +
>  void SGTileItem::geometryChanged(const QRectF &newGeometry, const QRectF &oldGeometry)
>  {
>      QQuickItem::geometryChanged(newGeometry, oldGeometry);
>  }
>  
> -void SGTileItem::renderCallback(QImage image)
> +#ifdef DEBUG_SHOW_TILE_BORDER
> +void SGTileItem::drawTileBorders(QSGNode* basicNode)
>  {
> -    if (m_state.loadAcquire() == SgstRendering) {
> -        m_data = image;
> -        m_state.storeRelease(SgstActive);
> -        update();
> -    } else deleteLater();
> +    auto node = basicNode;
> +    auto tileBorderGeometry = new QSGGeometry(QSGGeometry::defaultAttributes_Point2D(), 8);
> +    tileBorderGeometry->setDrawingMode(GL_LINES);
> +    tileBorderGeometry->setLineWidth(4);
> +
> +    QSGGeometry::Point2D* vertex = tileBorderGeometry->vertexDataAsPoint2D();
> +    vertex[0].set(node->rect().left(), node->rect().top());
> +    vertex[1].set(node->rect().left(), node->rect().bottom());
> +
> +    vertex[2].set(node->rect().right(), node->rect().top());
> +    vertex[3].set(node->rect().right(), node->rect().bottom());
> +
> +    vertex[4].set(vertex[0].x, vertex[0].y);
> +    vertex[5].set(vertex[2].x, vertex[2].y);
> +
> +    vertex[6].set(vertex[1].x, vertex[1].y);
> +    vertex[7].set(vertex[3].x, vertex[3].y);
> +
> +    auto tileBorderMaterial = new QSGFlatColorMaterial;
> +    tileBorderMaterial->setColor(Qt::red);
> +
> +    auto tileBorderNode = new QSGGeometryNode;
> +
> +    tileBorderNode->setGeometry(tileBorderGeometry);
> +    tileBorderNode->setFlag(QSGNode::OwnsGeometry);
> +
> +    tileBorderNode->setMaterial(tileBorderMaterial);
> +    tileBorderNode->setFlag(QSGNode::OwnsMaterial);
> +
> +    node->appendChildNode(tileBorderNode);
>  }
> -
> +#endif


-- 
https://code.launchpad.net/~mrqtros/ubuntu-docviewer-app/ubuntu-docviewer-app-render-engine/+merge/271733
Your team Ubuntu Document Viewer Developers is subscribed to branch lp:ubuntu-docviewer-app/reboot.