PageView: Fixed crash in preloading within single page mode. Lowered number

of wasted PixmapRequests (regressions introduced by DocumentViewport).
  Optimizations.
Document: Fixed oscillations in memory deallocator. Merged 'cleanupMemory'
  and 'freeMemory' -> 'cleanupPixmapMemory'.
  Delayed initialization of QTimers.
GeneratorPDF: Added a comment on threading and asyncronous pixmap loading.
TODO: updated adding items by Mikolaj Machowski and Grzegorz Ja?kiewicz

svn path=/trunk/kdegraphics/kpdf/; revision=380913

TODO

@@ -11,27 +11,35 @@ Status:
    next steps: empty the in-progress list and keep it empty until release.
 
 In progress:
--> memory: check for alloc/dealloc dynamic loops
--> new word highlighting for searches / other highlights (on paper - enrico)
+-> new word highlighting for searches / other highlights
 
-More items (first items will enter 'In progress list' first):
+High priority 3.4 features (deadline is 2k5-Feb-2):
 -> display current page / total pages (with analog indicator too (progressbar/...))
    maybe this can be done on a small widget at the top of the toolbox, displaying
    'document' informations (pages, current pg, some metadata, etc..)
    Tested a 16px ktoolbar in the left-bottom corner.. looks goos and can be used to
    insert some actions that aren't so useful in the main (and bigger) toolbar
--> c00l: add scrollbar marks for bookmarks (like kate)
+-> implement history (for actionNamed and restoring previous viewports on navigation)
+-> rmb popup for bookmarking pages on thumbnailslist, showing bkmarked pages and
+   go to next/previous bookmark actions (support for showing bookmarked pages
+   is already in, but disabled)
+-> evaluate wether to change F9 as the default presentation shortcut
+
+More items (first items will enter 'In progress list' first):
+-> presentation: provide a pageX/totalPages indicator in addition to the circle one
+-> add scrollbar marks for bookmarks (like kate)
 -> google search in the page
 -> cleanup code and update README.png
 -> find-as-you-type: use shortcut for 'find next' action (not the default one)
 -> show Viewport in ThumbnailsList (blended/contour)
+-> history as a toolbox child (collecting Doc's viewport changes notifications)
 -> Delay TOC (DocumentSynapsis) generation (and move it on thread)
 -> refactor ThumbnailsList to do internal rendering as pageview does (way
    faster than using QScrollView + inserted Widgets and saves 8% on document
    loading)
 -> move toolbar view actions in the PageView instead of the part. maybe.. or not...
--> usability: layout 2PPV [1 2,3 4,5 6] -> [1,2 3,4 5]. add option for 'ebook' style alignment. (by Mikolaj)
--> usability: trigger redraw on 'filter text' on current page (by Mikolaj)
+-> usability: layout 2PPV [1 2,3 4,5 6] -> [1,2 3,4 5]. add option for 'ebook' style alignment
+-> usability: trigger redraw on 'filter text' on current page
 -> abstract TextPage generation (the last xpdf dependant class!). then go dancing in the
    streets.
 -> better boomark rendering (tested a 'clip overlay' but looks bad actually)
@@ -59,8 +67,6 @@ More items (first items will enter 'In progress list' first):
 -> export all text in plain_text/html
 -> extract(export?) images (have a look at ImageOutputDev.cc and pdfimages.cc from xpdf (not in our xpdf sources))
 -> text selection in wordprocessor style (very hard/impossible)
--> implement history (mainly for actionNamed)
--> history as a toolbox child (collecting Doc's viewport changes notifications)
 -> zoom: fit text (with configurable margin)
 -> open gzipped (.pdf.gz?) files
 -> kttsd output with menu entries. speech{document/page/selection}. (patch available - enrico)
@@ -85,6 +91,8 @@ More items (first items will enter 'In progress list' first):
 -> export: export to other formats keeping formatting (a dream.. except for PNG :-)
 
 Done (newest features come first):
+-> FIX: various in memory unallocator, preload with single pages, pageview
+-> FIX: optimized pageView (removed 1 waster req on start, lowered reqs)
 -> FIX: memory unloading order and hard swap avoiding
 -> CHG: open and open-recent buttons unified in Shell
 -> CHG: lens icon for the find-ahead messages

core/document.cpp

@@ -81,10 +81,8 @@ KPDFDocument::KPDFDocument()
 {
     d->searchPage = -1;
     d->allocatedPixmapsTotalMemory = 0;
-    d->memCheckTimer = new QTimer( this );
-    connect( d->memCheckTimer, SIGNAL( timeout() ), this, SLOT( slotCheckMemory() ) );
-    d->saveBookmarksTimer = new QTimer( this );
-    connect( d->saveBookmarksTimer, SIGNAL( timeout() ), this, SLOT( saveDocumentInfo() ) );
+    d->memCheckTimer = 0;
+    d->saveBookmarksTimer = 0;
 }
 
 KPDFDocument::~KPDFDocument()
@@ -152,9 +150,19 @@ bool KPDFDocument::openDocument( const QString & docFile )
     setViewport( loadedViewport );
 
     // start bookmark saver timer
+    if ( !d->saveBookmarksTimer )
+    {
+        d->saveBookmarksTimer = new QTimer( this );
+        connect( d->saveBookmarksTimer, SIGNAL( timeout() ), this, SLOT( saveDocumentInfo() ) );
+    }
     d->saveBookmarksTimer->start( 5 * 60 * 1000 );
 
     // start memory check timer
+    if ( !d->memCheckTimer )
+    {
+        d->memCheckTimer = new QTimer( this );
+        connect( d->memCheckTimer, SIGNAL( timeout() ), this, SLOT( slotTimedMemoryCheck() ) );
+    }
     d->memCheckTimer->start( 2000 );
 
     return true;
@@ -166,8 +174,11 @@ void KPDFDocument::closeDocument()
     if ( generator && pages_vector.size() > 0 )
         saveDocumentInfo();
 
-    // stop memory check timer
-    d->memCheckTimer->stop();
+    // stop timers
+    if ( d->memCheckTimer )
+        d->memCheckTimer->stop();
+    if ( d->saveBookmarksTimer )
+        d->saveBookmarksTimer->stop();
 
     // delete contents generator
     delete generator;
@@ -190,7 +201,7 @@ void KPDFDocument::closeDocument()
         delete *pIt;
     pages_vector.clear();
 
-    // clear memory management data
+    // clear memory allocation descriptors
     QValueList< AllocatedPixmap * >::iterator aIt = d->allocatedPixmapsFifo.begin();
     QValueList< AllocatedPixmap * >::iterator aEnd = d->allocatedPixmapsFifo.end();
     for ( ; aIt != aEnd; ++aIt )
@@ -235,12 +246,27 @@ void KPDFDocument::reparseConfig()
     // reparse generator config and if something changed clear KPDFPages
     if ( generator && generator->reparseConfig() )
     {
-        // invalidate pixmaps and send reload signals to observers
+        // invalidate pixmaps
         QValueVector<KPDFPage*>::iterator it = pages_vector.begin(), end = pages_vector.end();
         for ( ; it != end; ++it )
             (*it)->deletePixmapsAndRects();
+
+        // [MEM] remove allocation descriptors
+        QValueList< AllocatedPixmap * >::iterator aIt = d->allocatedPixmapsFifo.begin();
+        QValueList< AllocatedPixmap * >::iterator aEnd = d->allocatedPixmapsFifo.end();
+        for ( ; aIt != aEnd; ++aIt )
+            delete *aIt;
+        d->allocatedPixmapsFifo.clear();
+        d->allocatedPixmapsTotalMemory = 0;
+
+        // send reload signals to observers
         foreachObserver( notifyContentsCleared( DocumentObserver::Pixmap) );
     }
+
+    // free memory if in 'low' profile
+    if ( Settings::memoryLevel() == Settings::EnumMemoryLevel::Low &&
+         !d->allocatedPixmapsFifo.isEmpty() && !pages_vector.isEmpty() )
+        cleanupPixmapMemory();
 }
 
 
@@ -707,15 +733,15 @@ void KPDFDocument::sendGeneratorRequest()
     // [MEM] preventive memory freeing
     int pixmapBytes = 4 * request->width * request->height;
     if ( pixmapBytes > (1024 * 1024) )
-        cleanupMemory( pixmapBytes );
+        cleanupPixmapMemory( pixmapBytes );
 
     // submit the request to the generator
     generator->generatePixmap( request );
 }
 
-void KPDFDocument::cleanupMemory( int /*freeOffset*/ )
+void KPDFDocument::cleanupPixmapMemory( int /*bytesOffset*/ )
 {
-    // choose memory parameters based on configuration profile
+    // [MEM] choose memory parameters based on configuration profile
     int clipValue = -1;
     int memoryToFree = -1;
     switch ( Settings::memoryLevel() )
@@ -725,50 +751,43 @@ void KPDFDocument::cleanupMemory( int /*freeOffset*/ )
             break;
 
         case Settings::EnumMemoryLevel::Normal:
-            memoryToFree = d->allocatedPixmapsTotalMemory - getTotalMemory() / 5;
-            clipValue = d->allocatedPixmapsTotalMemory - getFreeMemory() / 2;
+            memoryToFree = d->allocatedPixmapsTotalMemory - getTotalMemory() / 3;
+            clipValue = (d->allocatedPixmapsTotalMemory - getFreeMemory()) / 2;
             break;
 
         case Settings::EnumMemoryLevel::Aggressive:
-            clipValue = d->allocatedPixmapsTotalMemory - getFreeMemory() / 2;
+            clipValue = (d->allocatedPixmapsTotalMemory - getFreeMemory()) / 2;
             break;
     }
 
-    // p--rintf("T:%d TT:%d FF:%d  - c:%d m:%d\n", d->allocatedPixmapsTotalMemory, getTotalMemory(), getFreeMemory(), clipValue, memoryToFree);
-
     if ( clipValue > memoryToFree )
         memoryToFree = clipValue;
 
     if ( memoryToFree > 0 )
-        freeMemory( memoryToFree );
-}
-
-void KPDFDocument::freeMemory( int bytesToFree )
-{
-    //kdDebug() << "Freeing: " << bytesToFree << " bytes" << endl;
-    // [MEM] free memory starting from older pixmaps
-    int pagesFreed = 0;
-    QValueList< AllocatedPixmap * >::iterator pIt = d->allocatedPixmapsFifo.begin();
-    QValueList< AllocatedPixmap * >::iterator pEnd = d->allocatedPixmapsFifo.end();
-    while ( (pIt != pEnd) && (bytesToFree > 0) )
     {
-        AllocatedPixmap * p = *pIt;
-        if ( d->observers[ p->id ]->canUnloadPixmap( p->page ) )
+        // [MEM] free memory starting from older pixmaps
+        int pagesFreed = 0;
+        QValueList< AllocatedPixmap * >::iterator pIt = d->allocatedPixmapsFifo.begin();
+        QValueList< AllocatedPixmap * >::iterator pEnd = d->allocatedPixmapsFifo.end();
+        while ( (pIt != pEnd) && (memoryToFree > 0) )
         {
-            // update internal variables
-            pIt = d->allocatedPixmapsFifo.remove( pIt );
-            d->allocatedPixmapsTotalMemory -= p->memory;
-            bytesToFree -= p->memory;
-            pagesFreed++;
-            // delete pixmap
-            pages_vector[ p->page ]->deletePixmap( p->id );
-            // delete allocation descriptor
-            delete p;
-        } else
-            ++pIt;
+            AllocatedPixmap * p = *pIt;
+            if ( d->observers[ p->id ]->canUnloadPixmap( p->page ) )
+            {
+                // update internal variables
+                pIt = d->allocatedPixmapsFifo.remove( pIt );
+                d->allocatedPixmapsTotalMemory -= p->memory;
+                memoryToFree -= p->memory;
+                pagesFreed++;
+                // delete pixmap
+                pages_vector[ p->page ]->deletePixmap( p->id );
+                // delete allocation descriptor
+                delete p;
+            } else
+                ++pIt;
+        }
+        //p--rintf("freeMemory A:[%d -%d = %d] \n", d->allocatedPixmapsFifo.count() + pagesFreed, pagesFreed, d->allocatedPixmapsFifo.count() );
     }
-
-    //kdDebug() << "items: " << d->allocatedPixmapsFifo.count() << " [" << (d->allocatedPixmapsTotalMemory/1000) << "] Removed " << pagesFreed << " pages. gap: " << bytesToFree << endl;
 }
 
 int KPDFDocument::getTotalMemory()
@@ -1006,12 +1025,12 @@ void KPDFDocument::saveDocumentInfo() const
     infoFile.close();
 }
 
-void KPDFDocument::slotCheckMemory()
+void KPDFDocument::slotTimedMemoryCheck()
 {
     // [MEM] clean memory (for 'free mem dependant' profiles only)
     if ( Settings::memoryLevel() != Settings::EnumMemoryLevel::Low &&
          d->allocatedPixmapsTotalMemory > 1024*1024 )
-        cleanupMemory();
+        cleanupPixmapMemory();
 }
 
 

core/document.h

@@ -92,8 +92,7 @@ class KPDFDocument : public QObject // only for a private slot..
     private:
         void sendGeneratorRequest();
         // memory management related functions
-        void cleanupMemory( int bytesOffset = 0 );
-        void freeMemory( int bytes );
+        void cleanupPixmapMemory( int bytesOffset = 0 );
         int getTotalMemory();
         int getFreeMemory();
         // more private functions
@@ -108,7 +107,7 @@ class KPDFDocument : public QObject // only for a private slot..
 
     private slots:
         void saveDocumentInfo() const;
-        void slotCheckMemory();
+        void slotTimedMemoryCheck();
 };
 
 

core/generator_pdf/generator_pdf.cpp

@@ -39,6 +39,21 @@
 // id for DATA_READY PDFPixmapGeneratorThread Event
 #define TGE_DATAREADY_ID 6969
 
+/** NOTES on threading:
+ * internal: thread race prevention is done via the 'docLock' mutex. the
+ *           mutex is needed only because we have the asyncronous thread; else
+ *           the operations are all within the 'gui' thread, scheduled by the
+ *           Qt scheduler and no mutex is needed.
+ * external: dangerous operations are all locked via mutex internally, and the
+ *           only needed external thing is the 'canGeneratePixmap' method
+ *           that tells if the generator is free (since we don't want an
+ *           internal queue to store PixmapRequests). A generatedPixmap call
+ *           without the 'ready' flag set, results in undefined behavior.
+ * So, as example, printing while generating a pixmap asyncronously is safe,
+ * it might only block the gui thread by 1) waiting for the mutex to unlock
+ * in async thread and 2) doing the 'heavy' print operation.
+ */
+
 PDFGenerator::PDFGenerator( KPDFDocument * doc )
     : Generator( doc ), pdfdoc( 0 ), kpdfOutputDev( 0 ), ready( true ),
     pixmapRequest( 0 ), docInfoDirty( true ), docSynopsisDirty( true )
@@ -208,7 +223,7 @@ void PDFGenerator::generatePixmap( PixmapRequest * request )
     // be set only to prevent asking a pixmap while the thread is running)
     ready = false;
 
-    // debug message
+    // debug requests to this (xpdf) generator
     //kdDebug() << "id: " << request->id << " is requesting " << (request->async ? "ASYNC" : "sync") <<  " pixmap for page " << request->page->number() << " [" << request->width << " x " << request->height << "]." << endl;
 
     /** asyncronous requests (generation in PDFPixmapGeneratorThread::run() **/

ui/pageview.cpp

@@ -78,7 +78,8 @@ public:
     bool typeAheadActivated;
     QString findString;
     bool blockViewport;
-    PageViewMessage * messageWindow;    //in pageviewutils.h
+    bool blockPixmapsRequest;           // prevent pixmap requests
+    PageViewMessage * messageWindow;    // in pageviewutils.h
 
     // actions
     KToggleAction * aMouseEdit;
@@ -120,6 +121,7 @@ PageView::PageView( QWidget *parent, KPDFDocument *document )
     d->scrollIncrement = 0;
     d->dirtyLayout = false;
     d->blockViewport = false;
+    d->blockPixmapsRequest = false;
     d->messageWindow = new PageViewMessage(this);
 
     // widget setup: setup focus, accept drops and track mouse
@@ -273,11 +275,11 @@ void PageView::notifyViewportChanged()
     d->blockViewport = true;
 
     // find PageViewItem matching the viewport description
-    const DocumentViewport & viewport = d->document->viewport();
+    const DocumentViewport & vp = d->document->viewport();
     PageViewItem * item = 0;
     QValueVector< PageViewItem * >::iterator iIt = d->items.begin(), iEnd = d->items.end();
     for ( ; iIt != iEnd; ++iIt )
-        if ( (*iIt)->pageNumber() == viewport.pageNumber )
+        if ( (*iIt)->pageNumber() == vp.pageNumber )
         {
             item = *iIt;
             break;
@@ -289,19 +291,21 @@ void PageView::notifyViewportChanged()
     }
 
     // relayout in "Single Pages" mode or if a relayout is pending
+    d->blockPixmapsRequest = true;
     if ( !Settings::viewContinous() || d->dirtyLayout )
         slotRelayoutPages();
 
     // restore viewport or use default x-center, v-top alignment
     const QRect & r = item->geometry();
-    if ( viewport.reCenter.enabled )
+    if ( vp.reCenter.enabled )
     {
-        int xCenter = (int)( viewport.reCenter.normalizedCenterX * (float)r.width() );
-        int yCenter = (int)( viewport.reCenter.normalizedCenterY * (float)r.height() );
+        int xCenter = (int)( vp.reCenter.normalizedCenterX * (float)r.width() );
+        int yCenter = (int)( vp.reCenter.normalizedCenterY * (float)r.height() );
         center( r.left() + xCenter, r.top() + yCenter );
     }
     else
         center( r.left() + r.width() / 2, r.top() + visibleHeight() / 2 - 10 );
+    d->blockPixmapsRequest = false;
 
     // request visible pixmaps in the current viewport and recompute it
     slotRequestVisiblePixmaps();
@@ -1339,7 +1343,7 @@ void PageView::slotRelayoutPages()
         {
             PageViewItem * item = *iIt;
             // update internal page size (leaving a little margin in case of Fit* modes)
-            updateItemSize( item, colWidth[ cIdx ] - 6, viewportHeight - 10 );
+            updateItemSize( item, colWidth[ cIdx ] - 6, viewportHeight - 8 );
             // find row's maximum height and column's max width
             if ( item->width() + 6 > colWidth[ cIdx ] )
                 colWidth[ cIdx ] = item->width() + 6;
@@ -1355,7 +1359,7 @@ void PageView::slotRelayoutPages()
 
         // 2) arrange widgets inside cells
         int insertX = 0,
-            insertY = 10; //TODO take d->zoomFactor into account (2+4*x)
+            insertY = 4; //TODO take d->zoomFactor into account (2+4*x)
         cIdx = 0;
         rIdx = 0;
         for ( iIt = d->items.begin(); iIt != iEnd; ++iIt )
@@ -1403,12 +1407,12 @@ void PageView::slotRelayoutPages()
             if ( item == currentItem || (cIdx > 0 && cIdx < nCols) )
             {
                 // update internal page size (leaving a little margin in case of Fit* modes)
-                updateItemSize( item, colWidth[ cIdx ] - 4, viewportHeight - 6 );
+                updateItemSize( item, colWidth[ cIdx ] - 6, viewportHeight - 8 );
                 // find row's maximum height and column's max width
-                if ( item->width() + 4 > colWidth[ cIdx ] )
-                    colWidth[ cIdx ] = item->width() + 4;
-                if ( item->height() + 6 > fullHeight )
-                    fullHeight = item->height() + 6;
+                if ( item->width() + 6 > colWidth[ cIdx ] )
+                    colWidth[ cIdx ] = item->width() + 6;
+                if ( item->height() + 8 > fullHeight )
+                    fullHeight = item->height() + 8;
                 cIdx++;
             }
         }
@@ -1435,7 +1439,9 @@ void PageView::slotRelayoutPages()
             fullWidth += colWidth[ i ];
 
         delete [] colWidth;
-	slotRequestVisiblePixmaps();
+
+        // this can cause a little overhead
+        slotRequestVisiblePixmaps();
     }
 
     // 3) reset dirty state
@@ -1475,6 +1481,10 @@ void PageView::slotRelayoutPages()
 
 void PageView::slotRequestVisiblePixmaps( int newLeft, int newTop )
 {
+    // if requests are blocked (because raised by an unwanted event), exit
+    if ( d->blockPixmapsRequest )
+        return;
+
     // precalc view limits for intersecting with page coords inside the lOOp
     bool isEvent = newLeft != -1 && newTop != -1 && !d->blockViewport;
     QRect viewportRect( isEvent ? newLeft : contentsX(),
@@ -1543,7 +1553,7 @@ void PageView::slotRequestVisiblePixmaps( int newLeft, int newTop )
         {
             PageViewItem * i = d->items[ headRequest ];
             // request the pixmap if not already present
-            if ( !i->page()->hasPixmap( PAGEVIEW_ID, i->width(), i->height() ) )
+            if ( !i->page()->hasPixmap( PAGEVIEW_ID, i->width(), i->height() ) && i->width() > 0 )
                 requestedPixmaps.push_back( new PixmapRequest(
                         PAGEVIEW_ID, i->pageNumber(), i->width(), i->height(), PAGEVIEW_PRELOAD_PRIO, true ) );
         }
@@ -1554,7 +1564,7 @@ void PageView::slotRequestVisiblePixmaps( int newLeft, int newTop )
         {
             PageViewItem * i = d->items[ tailRequest ];
             // request the pixmap if not already present
-            if ( !i->page()->hasPixmap( PAGEVIEW_ID, i->width(), i->height() ) )
+            if ( !i->page()->hasPixmap( PAGEVIEW_ID, i->width(), i->height() ) && i->width() > 0 )
                 requestedPixmaps.push_back( new PixmapRequest(
                         PAGEVIEW_ID, i->pageNumber(), i->width(), i->height(), PAGEVIEW_PRELOAD_PRIO, true ) );
         }