Created attachment 66749 [details] [review]
adds a cache loader to the PhotoImageView class

This patch adds a cache loader to the PhotoImageView class.   It also touches a couple of other classes:

BrowseablePointerChangedArgs:
   - adds a 'ExpectedNextItem and ExpectedNextIndex

BrowseablePointer:
   - modified MovePrevious and MoveNext to set the expected next item and index  values in the BrowseablePointerChangedArgs

AsynchPixBufLoader:
  - added a Uri property to get the current Uri being loaded (needed to record the Uri of the file loaded through the cache loader)
  - Added an 'Abort' method that turns off the loader's timer.  if we're loading a cached image, we need to abort the normal load, or it will over-write our pixbuf


The primary change is in the PhotoImageView class.   we add three new data members - an AsyncPixbufLoader to handle loading the cached images, and a pixbuf and uri to store the info on the cached image.    A new "HandleDoneCache" method updates the stored pixbuf and uri when the cache loader finishes loading. 

The bulk of the changes are in the PhotoItemChanged method - code was added to check if there is a cached image, and if there is, if it's uri matches the uri we're trying to switch to, the cached image is used.

Code there also triggers the loading of the expected next image by the cache loader if there is an expected next image in the changed args we get.

Finally, I made a somewhat unrelated change that selects the image we change to in the toplevel view --- so when you exit full-screen mode, the last image you were viewing in full-screen mode is selected.   I'd submit this as a separate patch, but it was rather entangled in the cached loading image code.

Created attachment 66751 [details] [review]
Updated version of patch - compiles clean against head

Whoops - the selectId method I was using came out of a patch I had applied that wasn't in head yet.   Commented out the selection code --- the patch otherwise is identical to what was described above...

Just had a quick peek at this patch (viewed it in FireFox and VI), and it seems to me that there is a slight problem with identations. spaces versus tabs, and that one tab is not equal to 8 spaces. or?
Perhaps you have one tab equal 4 spaces?
Larry have stated he wants tabs, and if not possible, then equal one tab as 8 spaces.

Tried it a bit, and it do improve the responsiveness quite a bit.
Some weird behaviour though.

1) Go to last picture, and try to go to the next picture. 
   --> This patch will progressive redraw the current (last) picture.

2) Go to first picture, and try to go to the previous picture.
   --> This patch will progressive redraw the current (first) picture.

3) Go to first picture, step forward (with 4 seconds delay) to the last picture, and experience the direct switch to the next photo. The directly press Page up (for previous) and see the redraw. (probably ok, since F-Spot do not know which direction you want to go in), and continue stepping to the second photo with 4 seconds delay. When you come to the second photo, wait the 4 seconds and then go to the first photo, and see F-Spot do the progressive redraw. 

4) In browse mode, select the first photo, and go to full screen. Wait 5 seconds, and the go to next photo. F-Spot will do the progressive redraw. Should have anticipated the direction, since it was the first photo.

Just my small 2 cents..

Created attachment 66803 [details] [review]
Yet another updated patch

This fixes the tab issue (I forgot to turn on indent-tabs-mode in my new emacs), and the first two issues Bengt reported.   

The extra redraws were being caused an an inconsistency between doing "Index=i" and "SetIndex(i)" in BrowseblePointer --- "Index=i" would do nothing if i was already selected, but SetIndex(i) would trigger a ItemChanged event even if i was already selected.   I don't know if this was intentional or not - but I've modified the code to be consistent, so both Index=i and SetIndex(i) do nothing if i is already selected.

I don't really consider Bengt's 3rd point to be a bug --- that's the way I intended it to work.  :-)

I'm not sure how to fix Bengt's 4th point - I'll take another look tonight and see if there's an easy way...

Created attachment 66805 [details] [review]
Fix that also trigger cache load on starting of fullscreen mode

found a fairly easy way to fix Bengt's fourth point - if PhotoImageChanged isn't given an expected next item by the change handler (which should only happen on initial startup) - it now assumes that it should pre-load the next image, unless you are on the last image, in which case it will pre-load the previous image

I can easily lock the display using this latest version by switching images quickly.

Hmm.  Looks like I've got a memory leak.  I had tested switching images very quickly, and didn't notice a problem --- but I tried it again now, and I noticed that my memory usage was climbing quite rapidly while I did it.  I suspect the cached pixbufs aren't being cleaned up properly.

I'll try to get an updated patch tonight...

Created attachment 67010 [details] [review]
Updated patch with fix for memory leak

Fixed a memory leak - the Abort method for AsycnPixbufLoader wasn't calling the Close method, so it was leaking a fair bit of memory each time it was called.

This patch is identical to the previous one, except that I added code to call Close to the Abort message.   From my testing it looks like the memory leak is gone.

Created attachment 67011 [details] [review]
Updated patch with fix for memory leak

Fixed a memory leak - the Abort method for AsycnPixbufLoader wasn't calling the Close method, so it was leaking a fair bit of memory each time it was called.

This patch is identical to the previous one, except that I added code to call Close to the Abort message.   From my testing it looks like the memory leak is gone.

Quick patch review: patch 67011 for bug 343856

some pros:
- apply against CVS and compile cleanly
- it works and guess the way you're browsing

some cons:
- when I see the very last item, the counter say sg like 4717 of 4722. But it is the *last* one. It has probably something to do with some (maybe 5) FileNotFoundException (but still shows the thumbnail)... But I do not have the same issue with unpatched f-spot...
- why not trying to load (if the system is idle) more than one picture ?
- why not loading/keeping the previous image when browsing forward. Sometime, you skipped one image too fast, and want to see it back. (obviously, the same when browsing the other way)

in short:
- I like it !

Updating patch statuses (obsolete because it's a duplicate, needs-work because of the issue raised by Stephane).

Good work otherwise!

I will look into the counter issue Stephane brought up...  I do *not* plan on working on the caching multiple images at the moment.   I agree that it would be great - what I'd like to see is a proper least recently used caching scheme where the user can select the maximum number of images that get cached - but that's quite a bit more effort, and the implementation there now is good enough for me...

can we accept this patch as is (once I sort out the counter issue) and open a new bug for caching multiple images?

Yeah, that sounds best, one feature per bug. Sounds like a finished patch when that counter issue is ironed out, unless new things pop up.

Created attachment 67314 [details] [review]
Update to patch with call to PhotoChanged callback added

The counter problem was caused by the fact that I didn't have a call to "PhotoChanged" if it exists.   I was focusing mostly on full-screen mode in my tests, so I didn't notice that the counter in "Edit Photo" mode wasn't getting updated when the photo was being loaded from the cache.

Good catch, Stephane!

This patch is identical to the previous patch, except that I duplicated the 
if (PhotoChanged != null)
    PhotoChanged (this);

in the code that's invoked when loading an image from the cache.

This seems to solve the counter problem for me.

> This seems to solve the counter problem for me.

For me too !!!

One last suggestion that you can drop if you don't like it: Pre-caching is quite hungry in cpu ressources (sometime, it loads 2 images at (almost) the same time). Could it be possible to enable pre-caching at will ? e.g. with a preference in Gconf ?

Only my €.O2, and you know, it's almost nothing!

Excellent, this is coming along very well.  I really appreciate that you went after the the problem this way.  I'm looking over the patch now and I'll add some comments shortly.

GQView has image preloading and it looks much nicer than F-Spot.  It is an optional preference, and it appears to cache both the previous and next images.

To save memory I suggest that the direction of viewing (forwards or backwards) is tracked, and the next image preloaded assuming that the direction of viewing will remain the same.  This way the ugly progressive load will only be seen when the direction changes.

An option to preload both previous and next image or turn off preloading should be available in gconf.

I've just tried the patch, which still applies cleanly on cvs, and it works as advertised.

I agree with Mike in that gqview is very comfortable to use in that regard. But I think this patch is going in the right direction.

Created attachment 94461 [details] [review]
Updated patch

I updated the patch to have it apply cleanly to trunk and to fix some whitespace.  The patch works great for me.  This has been open long enough, I think it's time to commit it and go from there in terms of adding more options/features.

Noticed a couple bugs with this patch:  1) If try to view/edit an image that isn't accessible you get an exception instead of the current behavior of displaying the question mark image, and 2) if you change to another version of an image, the new version won't display until you view another image then come back.

I like this patch very much! Especially for quickly showing my pictures to friends (I can move to the next picture at any time).

Though, I noticed two more bugs: 
1) If you delete the current picture, the view does not get updated (View mode)
2) Not searching for any tag, display the first photo, go into browse mode again. Search for one tag (or import roll), click on the first photo (which should be different). => The view shows the first

I think they could be solved by saving the filename of the cached pictures and looking it up if the image is changed. This would also fix comment #21, bug 2.

Maybe a dumb question, but will the patch be commited?

Many people use cameras that produce 8 or 10MP images and the problem increases (the bug has been opened for almost 2 years).

Mateusz: There are not too many active F-Spot developers AFAIK, but indeed this is a problem that hinders the usability of F-Spot to a quite great extent for anyone dealing with newer compact digital cameras. The patch works quite well within some limits, though.

Not only cache should be implemented, but it should be found out what on earth (Mono?) is causing the image loading in general to be so slow if compared with eg. gthumb.

I aggree with timo jyrinki.
Image loading should become faster AND preloading should be implemented.
At the moment, F-Spot takes more than twice the time to display the next image compared to gtumb.

More discussion at https://bugzilla.novell.com/show_bug.cgi?id=173529

IMHO, at the moment one of F-Spot's major drawbacks is its lack of image-preloading.
The result is: With GQview offering great performance on this topic (concerning both preloading and image quality), when presenting my pictures to someone else, I most always export them from F-Spot first and then use GQview to actually present the photos.
Besides being cumbersome work, this automatically also means less "marketing" in F-Spot's favour.

Any idea when this patch can be included in the mainline?
I would by far prefer seeing this feature included in the next F-Spot release, compared to other user-interface related enhancements or new features ... even if the functionality was not perfect yet, the patch definitely seems to be a leap forward.

There are not many (or any) active developers working on F-Spot, so it's not of use to ask when the patch will be included. Obviously the patch should still see more work by someone, so until someone starts working on the code, it will not be committed.

That said, I'm personally finding F-Spot now finally barely usable (but: usable) when disabling the progressive drawing of images:

--- f-spot-0.5.0.3.orig/src/PhotoImageView.cs
+++ f-spot-0.5.0.3/src/PhotoImageView.cs
@@ -16,7 +16,7 @@
                protected BrowsablePointer item;
                protected FSpot.Loupe loupe;
                protected FSpot.Loupe sharpener;
-               ProgressType load_async = ProgressType.Full;
+               ProgressType load_async = ProgressType.None;
                bool progressive_display;
                public GdkGlx.Context Glx;
                private OldEditor editor;

There are a few major annoyances still for F-Spot being actually enjoyable to use, but for now the "calendar view" is so good a feature that I'm using it anyhow. But out-of-the-box I couldn't stand the 1-2s fuzziness per picture when showing photos out of my camera (12Mpix photos). I'm using Core 2 Duo E6420.

This is just FYI while you're waiting for this precaching thing to be properly implemented or some other software arrives that has the features you like in F-Spot. This still means F-Spot is 2-4x slower than other viewing programs when showing the photos, but it's not as annoying as with progressive loading enabled in my humble opinion.

(In reply to comment #28)
> There are not many (or any) active developers working on F-Spot

You obviously haven't been following planet gnome lately:
http://www.maxxer.it/f-blog/2009/06/moving-to-git/
http://weblog.savanne.be/172-f-spot-alive-and-kicking

This is something that needs to be fixed (and will be fixed), as obviously, we hate it too.

Comment #28 : "This still means F-Spot is 2-4x slower than other viewing programs when showing the photos"

C++ ?  ;)

/* ducks, runs */

(In reply to comment #30)
> C++ ?  ;)

Most of our performance critical code is written in C and based on eye of gnome, so: no. In fact, tests where we ported this to C# (for maintainability) did not give measurable performance loss, so it's not a matter of using a different language. It's about using a smarter algorithm :-)

F-Spot has moved to https://github.com/f-spot/f-spot/issues

If this Bugzilla ticket is still valid in a recent version of F-Spot, please feel free to post this topic as a ticket in the F-Spot project on GitHub.

Closing this report as WONTFIX as part of Bugzilla Housekeeping as we are planning to shut down GNOME Bugzilla in favor of GNOME Gitlab.