> -----Original Message----- > From: Kim Woelders [mailto:k...@woelders.dk] > Sent: 10. marts 2011 20:52 > To: Carsten Haitzler > Cc: enlightenment-devel@lists.sourceforge.net; Jesper Saxtorph > Subject: Re: [E-devel] imlib2 caching can fail > > On Thu, 10 Mar 2011 10:31:32 +0100, Carsten Haitzler > <ras...@rasterman.com> wrote: > > > On Sun, 06 Mar 2011 07:49:59 +0100 "Kim Woelders" <k...@woelders.dk> > said: > > > >> On Thu, 03 Mar 2011 15:46:15 +0100, Jesper Saxtorph > >> <jesper.saxto...@prevas.dk> wrote: > >> > >> > I have just submitted a bugraport + a patch on trac (bug #716). > >> > > >> > I just wanted to notifify it here also as I do not know the > >> > enlightenment community and how it works. > >> > > >> Either is fine. > >> > >> > To repeat my bug report: > >> > "I use imlib2 as a image library in a project. However the use we > have > >> > sometimes tricker a situation where imlib2 uses an invalid cache. > >> > > >> > imlib2 uses timestamps to test if a image cache i valid. If a files > >> > modification time is in the future it is not possible to use > >> validation > >> > scheme. Further if the timestamp is equal to now, we do not know if > >> the > >> > modification time is in the future or not. The result is that the > >> cache > >> > should be invalidated for file timestamps >= now. > >> > An example of a problematic situation: timestamps are in whole > seconds > >> > times given as here as seconds: > >> > time=32.1 : image.png is written by someone > >> > time=32.4 : image.png is loaded by imlib2 > >> > time=32.5 : image.png is written with new data > >> > The situation is now that the cache has the same timestamp as the > >> file, > >> > but the content is not the same. > >> > > >> > A possible fix is a 3 line patch, which I have attached. It > invalidate > >> > the cache if the files timestamp is >= now. > >> > The patch is made agains head, however, the image.c file (where the > >> > patch is applied) has changed very little in it lifetime, so it works > >> > fine with earlier versions of imlib2 (I use it against 1.4.2)." > >> > > >> > I attached an incorrect patch at first to the ticket, but have > >> submittet > >> > the correct afterwards. Sorry for that. > >> > > >> > My suggestion to a patch (I have made the long comment as I think it > >> is > >> > not obvious why it is needed): > >> > --- imlib2/src/lib/image.c.orig 2011-03-03 14:23:49.000000000 +0100 > >> > +++ imlib2/src/lib/image.c 2011-03-03 14:45:19.000000000 +0100 > >> > @@ -1017,6 +1017,18 @@ > >> > im->key = __imlib_FileKey(file); > >> > } > >> > im->moddate = __imlib_FileModDate(file); > >> > + /* If the file modify time is now or in the future, we can not > >> make > >> > a */ > >> > + /* cache. */ > >> > + /* One of several possible scenarios: */ > >> > + /* time=now: file is written by someone */ > >> > + /* time=now: file is loaded here */ > >> > + /* time=now: file is written again by someone */ > >> > + /* Now we have a file a timestamp equal to our cache, but with a > >> */ > >> > + /* different content. */ > >> > + if (im->moddate >= time(NULL)) > >> > + { > >> > + dont_cache = 1; > >> > + } > >> > /* ok - just check all our loaders are up to date */ > >> > __imlib_RescanLoaders(); > >> > /* take a guess by extension on the best loader to use */ > >> > > >> > Hope it make sense, otherwise feel free to ask and/or discuss it > >> > > >> I see there is a problem, but I don't think it is the proper solution. > >> If you have a file with a "now"/future time stamp it would never be > >> cached, which is wrong. > >> > >> How about in stead changing line 986 (svn) to check whether the time > >> stamp > >> has changed in stead of checking if it is newer? > > > > while in general you are also right.. he's also trying to fix the > > problem of > > "write file at 34.1 secs, then write AGAIN at 34.5 secs" - they both > > have the > > SAME timestamp but the 2nd is newer. imlib2 will not load the newer file > > as the > > timestamps match due to timestamp resolution (34 vs 34 secs). but while > > solving > > this problem it breaks caching for timestamp == now. you'd need much > > higher > > resolution timestamps (and even then you still just have a smaller race > > condition) or .. you'd have to have another way to differentiate the > > files. > > file size + timestamp + inode + .... even then you just lower the > > probability > > not eliminate it. > > > Yeah, I know. > > The important thing here, IMO, is that images which never change, should > always be cached (unless explicitly not caching), whichever > past/now/future time stamp they may have. > > How about this: > > diff --git src/lib/image.c src/lib/image.c > index d404961..e8610b2 100644 > --- src/lib/image.c > +++ src/lib/image.c > @@ -982,8 +982,10 @@ __imlib_LoadImage(const char *file, > ImlibProgressFunction progres > time_t current_modified_time; > > current_modified_time = __imlib_FileModDate(file); > - /* if the file on disk is newer than the cached one */ > - if (current_modified_time > im->moddate) > + /* if the disk file time stamp is different from the */ > + /* cached one or appears to have just been set */ > + if ((current_modified_time != im->moddate) || > + (current_modified_time == time(NULL))) > { > /* invalidate image */ > SET_FLAG(im->flags, F_INVALID); > > This of course requires using imlib_image_set_changes_on_disk() in the > application.
If I remember right, setting the F_INVALID flag will invalidate the current (old) cach and not prevent caching what we are about to load. The problem I describe is that when we load an image with a timestamp now or in the future, we will never be able to see if there is a never image on the disk afterwards. Therefore you can not check and invalidate the cach later on, as you try to do. If you take my sequence of write image -> imlib load it -> write it again within the same timestamp, you will not be able to detect that it has happened afterwards. Your suggestion will simple invalidate any old cach sitting around, then load the image and cach it and never see it has changed again within the same timestamp. However, as I have been looking at the code again, I found that you are right that dont_cache will make it never cache the image, as it test dont_cache in the end of the function and set F_UNCACHEABLE. I still think the idea of my code is the best way to go, however it need to be adjusted so it is only the current caching which is avoided. I wondered why the code was made like that and began to look further. I found that the comments in the code do not match the behaviour. imlib_load_image_without_cache and imlib_load_image_immediately_without_cache in api.c has comments stating they will load the image "without looking in the cache first", but do not state it will not be cached for further use. As far as I can see, these functions WILL use the cache, if the cache is valid. However, they will not cache what they load. Also they will set the F_UNCACHEABLE flag. As I have not had time to look further into it, I am not sure if the intention here is what the comments states or what the code do. -Jesper ------------------------------------------------------------------------------ Colocation vs. Managed Hosting A question and answer guide to determining the best fit for your organization - today and in the future. http://p.sf.net/sfu/internap-sfd2d _______________________________________________ enlightenment-devel mailing list enlightenment-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/enlightenment-devel
