rkflx added a comment.
First things first: The patch marks an impressive improvement in (perceived) drawing performance for slow-rendering PDFs, I'm glad your hard work payed off. See below for some minor comments regarding the code, but obviously I lack Okular/Poppler knowledge to review this in depth. I noticed https://phabricator.kde.org/D8379 added some autotests, but here we get nothing in that regard. Do you think there might be a way to autotest the intended behaviour of the rendering a bit, e.g. correct order of main canvas / thumbnail rendering, cancelling on zoom/resize/pan etc.? Just asking, not a requirement for acceptance of course. As for testing: - I'm using Qt 5.10.0, Okular master and Poppler master (Poppler patch did not apply cleanly, though) with `dublin-center-street.pdf` for now. - So far I could not yet perform very extensive test runs, but I thought I'd share the first problems I found so you can start looking into it. Will do more testing in a bit. Let me know if you think the problem is somewhere on my side. 1. Segfault when resizing window. Backtrace: #0 0x00007ffff2306893 in _int_free (av=0x7ffff2639c20 <main_arena>, p=0x7affb0, have_lock=0) at malloc.c:4184 #1 0x00007ffff233bba5 in tzset_internal (always=1) at tzset.c:403 #2 0x00007ffff233bd68 in tzset_internal (always=1) at tzset.c:553 #3 __tzset () at tzset.c:555 #4 0x00007ffff233ac29 in __GI_mktime (tp=0x7fffff7ff0e0) at mktime.c:588 #5 0x00007ffff3019291 in qt_mktime(QDate*, QTime*, QDateTimePrivate::DaylightStatus*, QString*, bool*) () from /usr/lib64/libQt5Core.so.5 #6 0x00007ffff301a109 in refreshDateTime(QDateTime::Data&) () from /usr/lib64/libQt5Core.so.5 #7 0x00007ffff301a3b0 in setDateTime(QDateTime::Data&, QDate const&, QTime const&) () from /usr/lib64/libQt5Core.so.5 #8 0x00007ffff301cfb9 in QDateTime::setMSecsSinceEpoch(long long) () from /usr/lib64/libQt5Core.so.5 #9 0x00007ffff301f5a7 in QDateTime::fromMSecsSinceEpoch(long long, Qt::TimeSpec, int) () from /usr/lib64/libQt5Core.so.5 #10 0x00007ffff301f8c8 in QDateTime::currentDateTime() () from /usr/lib64/libQt5Core.so.5 #11 0x00007ffff301f933 in QTime::currentTime() () from /usr/lib64/libQt5Core.so.5 #12 0x00007fffda79034b in Okular::DocumentPrivate::getFreeMemory (this=0x7c7d20, freeSwap=0x0) at okular/core/document.cpp:430 #13 0x00007fffda78f671 in Okular::DocumentPrivate::calculateMemoryToFree (this=0x7c7d20) at okular/core/document.cpp:229 #14 0x00007fffda7952ae in Okular::DocumentPrivate::sendGeneratorPixmapRequest (this=0x7c7d20) at okular/core/document.cpp:1248 #15 0x00007fffda7a0bc2 in Okular::Document::requestPixmaps (this=0x74ec70, requests=..., reqOptions=...) at okular/core/document.cpp:3325 #16 0x00007fffda79ff9d in Okular::Document::requestPixmaps (this=0x74ec70, requests=...) at okular/core/document.cpp:3177 #17 0x00007fffdac03e37 in ThumbnailListPrivate::slotRequestVisiblePixmaps (this=0x9d98d0) at okular/ui/thumbnaillist.cpp:634 #18 0x00007fffdac02e40 in ThumbnailList::notifyContentsCleared (this=0x96a7a0, changedFlags=1) at okular/ui/thumbnaillist.cpp:361 #19 0x00007fffda7a0c50 in Okular::Document::requestPixmaps (this=0x74ec70, requests=..., reqOptions=...) at okular/core/document.cpp:3328 #20 0x00007fffda79ff9d in Okular::Document::requestPixmaps (this=0x74ec70, requests=...) at okular/core/document.cpp:3177 #21 0x00007fffdac03e37 in ThumbnailListPrivate::slotRequestVisiblePixmaps (this=0x9d98d0) at okular/ui/thumbnaillist.cpp:634 #22 0x00007fffdac02e40 in ThumbnailList::notifyContentsCleared (this=0x96a7a0, changedFlags=1) at okular/ui/thumbnaillist.cpp:361 #23 0x00007fffda7a0c50 in Okular::Document::requestPixmaps (this=0x74ec70, requests=..., reqOptions=...) at okular/core/document.cpp:3328 #24 0x00007fffda79ff9d in Okular::Document::requestPixmaps (this=0x74ec70, requests=...) at okular/core/document.cpp:3177 #25 0x00007fffdac03e37 in ThumbnailListPrivate::slotRequestVisiblePixmaps (this=0x9d98d0) at okular/ui/thumbnaillist.cpp:634 #26 0x00007fffdac02e40 in ThumbnailList::notifyContentsCleared (this=0x96a7a0, changedFlags=1) ... #495 0x00007fffda7a0c50 in Okular::Document::requestPixmaps (this=0x74ec70, requests=..., reqOptions=...) at okular/core/document.cpp:3328 #496 0x00007fffda79ff9d in Okular::Document::requestPixmaps (this=0x74ec70, requests=...) at okular/core/document.cpp:3177 #497 0x00007fffdac03e37 in ThumbnailListPrivate::slotRequestVisiblePixmaps (this=0x9d98d0) at okular/ui/thumbnaillist.cpp:634 #498 0x00007fffdac02e40 in ThumbnailList::notifyContentsCleared (this=0x96a7a0, changedFlags=1) at okular/ui/thumbnaillist.cpp:361 #499 0x00007fffda7a0c50 in Okular::Document::requestPixmaps (this=0x74ec70, requests=..., reqOptions=...) at okular/core/document.cpp:3328 #500 0x00007fffda79ff9d in Okular::Document::requestPixmaps (this=0x74ec70, requests=...) at okular/core/document.cpp:3177 #501 0x00007fffdac03e37 in ThumbnailListPrivate::slotRequestVisiblePixmaps (this=0x9d98d0) at okular/ui/thumbnaillist.cpp:634 #502 0x00007fffdac02e40 in ThumbnailList::notifyContentsCleared (this=0x96a7a0, changedFlags=1) ... 2. OOM killed when changing sidebar size. 3. ASSERT when deselecting "parking (level 7)" in Layers sidebar: ASSERT: "(*rIt)->observer() == requesterObserver" in file okular/core/document.cpp, line 3204 Aborted (core dumped) 4. No text rendering in some situations (observed this multiple times when wildly zooming and jumping around via the thumbnail view, sadly don't have a concise video yet). INLINE COMMENTS > document.cpp:3109 > +{ > + // New request is more prioritary -> cancel > + if ( executingRequest.priority() > otherRequest.priority() ) `has higher priority` > document.cpp:3113 > + > + // New request is less prioritary -> don't cancel > + if ( executingRequest.priority() < otherRequest.priority() ) `has lower priority` > document.cpp:3117 > + > + // New request is as prioritary from a different observer -> don't cancel > + // AFAIK this never happens since all observers have different priorities `has same priority as` > document.cpp:3287 > + > + // If we were told to remove all the previous and the executing > request pagee is not part of the new requests, cancel it > + if ( !requestCancelled && removeAllPrevious && requesterObserver > == executingRequest->observer() && !newRequestsContainExecutingRequestPage ) - word missing after `previous`? - `pagee`? > document.cpp:3327 > + > + for ( DocumentObserver *o : qAsConst( observersPixmapCleared ) ) > + o->notifyContentsCleared( Okular::DocumentObserver::Pixmap ); Add `{ }`. > generator_chm.cpp:415 > > + Okular::Page *page = request->page(); > m_syncGen->view()->resize(page->width(), page->height()); `const` > generator_djvu.cpp:212 > userMutex()->lock(); > + Okular::Page *page = request->page(); > QList<KDjVu::TextEntity> te; `const` > generator_dvi.cpp:252 > { > + Okular::Page *page = request->page(); > + `const` > generator_pdf.cpp:1177 > +{ > + Okular::Page *page = request->page(); > #ifdef PDFGENERATOR_DEBUG `const` > generator_xps.cpp:2123 > QMutexLocker lock( userMutex() ); > - XpsPage * xpsPage = m_xpsFile->page( page->number() ); > + XpsPage * xpsPage = m_xpsFile->page( request->page()->number() ); > return xpsPage->textPage(); `const` (not really...) REPOSITORY R223 Okular BRANCH cancellable (branched from master) REVISION DETAIL https://phabricator.kde.org/D9328 To: aacid, ervin Cc: rkflx, ervin, michaelweghorn, ngraham, #okular, gassaf, aacid