Am Montag, den 03.09.2007, 10:57 +0200 schrieb Alexander Larsson: > On Sun, 2007-09-02 at 11:59 +0200, Christian Neumair wrote: > > I've attached a patch to bug 104224 [1] that implements multithreaded > > thumbnail loading. I'd appreciate some comments. > > > > Do not confuse this with the thumbnail cache issue discusses in recent > > blog entries, we'll implement another solution to that. > > > > [1] http://bugzilla.gnome.org/show_bug.cgi?id=104224 > > It seems a bit late in the game for this release, but seems like a good > idea. > > Some quick comments from just glancing over the patch:
Thanks for your incredibly fast reponse! > I don't think the immediate_thumb_loading argument is needed. We should > just always read it async. I thought it might be useful for bookmark lists, the spatial window's button, the file properties window etc, but you may well be right here. > Why are you implementing thumbnail reading as a thread? Why not use the > existing async i/o machinery in gnome-vfs. We can entirely reuse the existing loading code, and that IMO makes it way more traceable, although multithreading is a can of worms. We just have to protect the cache against parallel access. Of course your scepticism towards multithreaded programming is well-reasoned, but it just seemed to be more straightforward since it allowed to completely reuse the synchronous and well-tested core loader code (nautilus_thumbnail_load_image). We're really doing lots of I/O, I'm not sure how well the async machine deals with a few hundred tiny requests per second. > thumbnail_loading_thread_func() uses a lot of nautilus stuff in a > thread, which is not possible. Nautilus just is not threadsafe in this > fashion. Threads can only do really low-level stuff. "a lot of nautilus stuff" is not very precise. We use nautilus_icon_factory_get_pixbuf_for_icon which in fact doesn't do anything VFS- or NautilusFile-related, but just does the cache handling and filling (i.e. does actual image loading). I forgot to lock the mutex around the g_hash_table_foreach_remove call in nautilus_icon_factory_sweep, though. We may also want to protect the recently used list as it is updated through mark_recently_used in get_icon_from_cache. We also use NautilusFile, for passing around URIs, and - in the timeout - for emitting the file_changed() signal. At least the possibly-harmful passing-around could be removed. thumbnail_loading_thread_func() will just pass a URI to thumbnail_loading_notify_file_changed anyway, so passing the thread an URI as well may be fine. Regarding the file_changed() NautilusFile usage in thumbnail_loading_notify_file_changed: We also use similar code for NautilusThumbnail, in thumbnail_thread_notify_file_changed(), so I thought it would be safe. > Does is_loading_thumbnail really need to be its own thing? Can't we just > re-use is_thumbnailing (that is after all what we're doing from a > highlevel perspective). I'm not sure. One day we'll hopefully implement proper thumbnailing cancellation (through the stop button in the toolbar), I think it may come in handy to distinguish it from the NautilusIconFactory-related code, where cancellation isn't neccessary - or, if we decide to also allow thumbnail loading cancellation, it will have an entirely different code path. > Maybe break out the is_loading pixbuf generation code to a helper > function. Pardon? > Maybe the nautilus_thumbnail_prioritize() stuff can be used or extended > to prioritize loading of thumbnails for currently visible icons. I'm not sure. Actually, the NautilusThumbnail code has a lot less in common with thumbnail loading than the NautilusIconFactory code, and that's why I put it there. Why would we want to prioritize thumbnail loading, it's supposed to happen instantaneous (well almost), or at least within 2-3 seconds for virtually arbitrarily large folders. Are you talking about the situation where one runs out of main memory, and just wants to show thumbnails for visible files? -- Christian Neumair <[EMAIL PROTECTED]> -- nautilus-list mailing list [email protected] http://mail.gnome.org/mailman/listinfo/nautilus-list
