D9328: Add support for cancellable image rendering and text extraction
Henrik Fehlauer
noreply at phabricator.kde.org
Fri Jan 19 00:23:27 UTC 2018
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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/okular-devel/attachments/20180119/438c8f9c/attachment-0001.html>
More information about the Okular-devel
mailing list