[KPhotoAlbum] Import crash

Thu May 18 05:10:39 BST 2017

Just a couple of nits.

On 05/17/2017 09:59 PM, Robert Krawitz wrote:
> On Wed, 17 May 2017 21:07:25 +0200, Johannes Zarl-Zierl wrote:
>> On Dienstag, 16. Mai 2017 21:54:01 CEST Robert Krawitz wrote:
>>> On Mon, 15 May 2017 22:46:55 +0200, Johannes Zarl-Zierl wrote:
>>>> So I guess many of the imported images are already in the database? This
>>>> would lead to a direct recursion. As a quick fix I guess we could bypass
>>>> the direct call to aCopyJobCompleted(0) in copyNextFromExternal() in the
>>>> default case...
>>> I did it by having aCopyJobCompleted() set (clear) a flag to indicate
>>> whether to continue looping.  But there's a new problem: as the import
>>> progresses, I get messages like this:
>>>
>>> kf5.kio.core: Invalid URL: QUrl("img_6004_c2.jpg")
>>> kf5.kio.core: Invalid URL: QUrl("img_6005-5.jpg")
>>> kf5.kio.core: Invalid URL: QUrl("img_6006-5.jpg")
>>> kf5.kio.core: Invalid URL: QUrl("img_6007-6.jpg")
>>> kf5.kio.core: Invalid URL: QUrl("img_6008-5.jpg")
>>> kf5.kio.core: Invalid URL: QUrl("img_6009-6.jpg")
>>>
>>> That's because the export file creates unique filenames even if the
>>> files live in different directories (I presume because if you actually
>>> exported the images, rather than the file, they'd live in one
>>> directory so would need unique filenames):
>> [snip]
>>
>>> So this probably isn't going to do what I want after all.  Rats.
>> Just to be clear about the problem: You end up with a bunch of non-existing images in the database, in addition to the original that is already in the database?
> I didn't let it run that far.  It was prompting me for the location of
> each file, and I punted at that point.
>
>> If so, maybe you can help things along by using the "Find duplicates" dialog.
>>
>> A proper fix needs to change the way we create export files. In the
>> past, I was always hesitant to touch the .kim file format in order
>> to avoid compatibility problems. Maybe it's time to revisit this
>> decision...
> I agree it needs to be backward compatible, so a newer kpa can import
> an older .kim file.  I don't think the other direction is necessary;
> if an old kpa can't import a new import file, you upgrade.
While I totally agree with this in principle, sometimes you can't 
upgrade because of dependencies.
I had an install of something from a tarball fail today because of that.

The question then becomes: Are there any plausible scenarios where you 
would have a newer version the .kim file (which had to be generated on a 
newer version of KPA to start with) and still need to use it with an 
older version of KPA. I don't know the answer. E.g., Do all users have 
QT5 installed?
>
>>>>> I might also note that during a (much smaller) import the
>>>>> timeline bar constantly flashes, seemingly as each image is
>>>>> added to the database; this resulted in my window manager
>>>>> crashing.
>>>> Interesting. Report a bug with the window manager ;-)
>>>> Honestly though, I guess we should do that more efficiently. It seems that
>>>> ImportHandler::addNewRecord() calls DB::addImages for each image instead
>>>> of assembling a list and adding the whole list at once...
>>> I was looking at that, but I don't know whether it's safe to call
>>> updateCategories() before addImages().  If not, this will have to be
>>> done in two passes.
>> Just looking at the code I would think that it is safe to change
>> updateCategories() so that it accepts an ImageInfoList and call both
>> addImages() and updateCategories() once after assembling the image
>> list.
> I'll put that on my list.
>
>>>>> Finally, the process of comparing MD5 checksums against the database
>>>>> appears to be extremely slow; it took hours to go through 90,000
>>>>> images.  Perhaps the existing MD5 checksums need to be stored in a
>>>>> hash, rather than a sequential comparison or something?
>>>> Looking at DB::MD5Map, this is already a map. I'd have to look more
>>>> closely to see what's eating cpu in there...
>>> One of the problems is with displaying the images page.
>>> createImagesPage() is fast enough, but when it Qt then goes to display
>>> it, it's deathly slow if there are a lot of images (presumably there's
>>> some inefficiency in Qt with creating a dialog with thousands of
>>> items in it -- or the way KPA is doing it, since file browsers have to
>>> be able to display lots of items).  Even if I have it not try to
>>> display a pixmap and/or label, it's very slow.
>> I would assume that file managers don't need run into this issue
>> because most file systems perform poorly when you stash thousands of
>> files into a single directory.  For widgets that really have a huge
>> number of items, I guess you could do progessive loading? I haven't
>> looked into this, though...
> I tried creating 100,000 files in one directory (touch $(seq 1
> 100000)).  I opened it with Dolphin; the browser loaded in something
> under 1 second.
>
> I suspect QLayout isn't really designed for that many items; it's very
> flexible and tries hard to achieve the best layout for the widgets you
> put into it.  If I run it under gdb, and break into it, it looks like
> it's spending a huge amount of time doing stuff like this (all inside
> that ImportExport::ImportDialog::exec() call):
>
> (gdb) where
> #0  0x00007ffff284768f in QWidgetPrivate::subtractOpaqueSiblings(QRegion&, bool*, bool) const () at /usr/lib64/libQt5Widgets.so.5
> #1  0x00007ffff2821312 in  () at /usr/lib64/libQt5Widgets.so.5
> #2  0x00007ffff2822e99 in  () at /usr/lib64/libQt5Widgets.so.5
> #3  0x00007ffff2841f1f in QWidgetPrivate::syncBackingStore() ()
>      at /usr/lib64/libQt5Widgets.so.5
> #4  0x00007ffff2856a0d in QWidget::event(QEvent*) ()
>      at /usr/lib64/libQt5Widgets.so.5
> #5  0x00007ffff28131bc in QApplicationPrivate::notify_helper(QObject*, QEvent*) () at /usr/lib64/libQt5Widgets.so.5
> #6  0x00007ffff281a0f0 in QApplication::notify(QObject*, QEvent*) ()
>      at /usr/lib64/libQt5Widgets.so.5
> #7  0x00007fffef2da245 in QCoreApplication::notifyInternal2(QObject*, QEvent*) () at /usr/lib64/libQt5Core.so.5
> #8  0x00007fffef2dc2a3 in QCoreApplicationPrivate::sendPostedEvents(QObject*, int, QThreadData*) () at /usr/lib64/libQt5Core.so.5
> #9  0x00007fffef329043 in  () at /usr/lib64/libQt5Core.so.5
> #10 0x00007fffe9494134 in g_main_context_dispatch ()
>      at /usr/lib64/libglib-2.0.so.0
> #11 0x00007fffe9494388 in  () at /usr/lib64/libglib-2.0.so.0
> #12 0x00007fffe949442c in g_main_context_iteration ()
>      at /usr/lib64/libglib-2.0.so.0
> #13 0x00007fffef32888c in QEventDispatcherGlib::processEvents(QFlags<QEventLoop::ProcessEventsFlag>) () at /usr/lib64/libQt5Core.so.5
> #14 0x00007fffef2d86ab in QEventLoop::exec(QFlags<QEventLoop::ProcessEventsFlag>) () at /usr/lib64/libQt5Core.so.5
> #15 0x00007ffff29eb68f in QDialog::exec() () at /usr/lib64/libQt5Widgets.so.5
> #16 0x00000000005eb0d0 in ImportExport::ImportDialog::exec(ImportExport::KimFileReader*, QUrl const&) ()
> #17 0x00000000005d3dbf in ImportExport::Import::exec(QString const&) ()
> #18 0x00000000005d40f9 in ImportExport::Import::imageImport(QUrl const&) ()
> #19 0x00000000005d4335 in ImportExport::Import::imageImport() ()
>
> For that matter, a lot of times when I'm doing an import I'm really
> not interested in selecting which files I want to import; just grab
> the whole thing and go.
I'm  not sure if the following is relevant or not. I'm used to thinking 
about files and directories, not the content of databases.

I would suspect that this is the most common case. I always have my 
photos batched into directories and operate on whole directories. In any 
case, that's what file managers and shell scripts are for.

KPA should be able to do such things (especially if it already does), 
but it probably doesn't need to be very efficient at it because it may 
not be used that frequently.