Re: patch nag: Multithreaded thumbnail loading

[Alexander Larsson <alexl redhat com>]

On Mon, 2007-09-03 at 12:36 +0200, Christian Neumair wrote:
> > 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.

If it is slow in one place its likely to be slow in another. I see no
reason for not doing it async everywhere. Thats what we do with (almost)
all other i/o in nautilus.

> > 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.

Its not just a can of worms. More like a gigantic dumpster of worms. You
can't even use NautilusFile in a thread that is not the main thread. And
to change this (make nautilus fully threadsafe) is a large piece of
work.

> 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.

nautilus_thumbnail_load_image() doesn't look all that hard to turn into
an async function. You just have to use eel_read_entire_file_async()
instead of g_file_get_contents().

Plus the async loading code is already well tested with lots of tiny
requests (its used to sniff the mimetype for files after all).

> > 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.

It just seems like a lot of complexity, slowdown and risk for deadlocks
to the nautilus core to add this threading protection, in order to avoid
what seems to me like a small bit of code. All the icon factory has to
do is check for an availible thumbnail and if one exists schedule an
async read of it. When that read finishes we insert the result into the
cache and emit changed on the file.

> > Maybe break out the is_loading pixbuf generation code to a helper
> > function.
> 
> Pardon?

This part:
+	if (nautilus_file_is_thumbnailing ((NautilusFile *) icon->data) ||
+	    nautilus_file_is_loading_thumbnail ((NautilusFile *) icon->data)) {
+		/* resize the ICON_NAME_THUMBNAIL_LOADING icon to the expected thumbnail size. */
+
+		double pixels_per_unit = (double) nautilus_get_icon_size_for_zoom_level (container->details->zoom_level)
+			/ NAUTILUS_ICON_SIZE_STANDARD;
+
+		if (gdk_pixbuf_get_width (pixbuf) < NAUTILUS_ICON_SIZE_THUMBNAIL * pixels_per_unit &&
+		    gdk_pixbuf_get_height (pixbuf) < NAUTILUS_ICON_SIZE_THUMBNAIL * pixels_per_unit) {
+			/* TODO? this only handles icons smaller than the expected thumbnail size ATM.
+			 * Should not be a common problem, though */
+			GdkPixbuf *new_pixbuf;
+			double x_size;
+			double y_size;
+			double x_offset;
+			double y_offset;
+			char *mime_type;
+			int i;
+
+			mime_type = nautilus_file_get_mime_type ((NautilusFile *)icon->data);
+			if (g_str_has_prefix (mime_type, "video/")) {
+				/* assume 4:3 aspect ratio, i.e. we'll always occupy the full width. */
+				x_size = NAUTILUS_ICON_SIZE_THUMBNAIL * pixels_per_unit;
+				y_size = 3./4 * x_size;
+			} else {
+				/* scale up to the max. thumbnail size.
+				 * This is correct at least in one dimension, and prevents the icons from jumping
+				 * around as the thumbnail is created, if it is tall for text below icon, and if it
+				 * is wide for text beside icon.
+				 */
+				x_size = NAUTILUS_ICON_SIZE_THUMBNAIL * pixels_per_unit;
+				y_size = NAUTILUS_ICON_SIZE_THUMBNAIL * pixels_per_unit;
+			}
+			g_free (mime_type);
+
+			/* maybe the size was smaller than the input pixbuf, so size it up.
+ 			 * This only seems to be relevant in the 4:3 case, for y_size.
+ 			 */
+			x_size = MAX (x_size, gdk_pixbuf_get_width (pixbuf));
+			y_size = MAX (y_size, gdk_pixbuf_get_width (pixbuf));
+
+			x_offset = x_size - gdk_pixbuf_get_width (pixbuf);
+			y_offset = y_size - gdk_pixbuf_get_height (pixbuf);
+
+			/* center wrt "minor" dimension, i.e. horizontally for text below
+ 			 * and vertically for text besides icon */
+			if (details->label_position == NAUTILUS_ICON_LABEL_POSITION_BESIDE)
+				y_offset /= 2;
+			else 
+				x_offset /= 2;
+
+			new_pixbuf = gdk_pixbuf_new (GDK_COLORSPACE_RGB, TRUE,
+						     gdk_pixbuf_get_bits_per_sample (pixbuf),
+						     x_size, y_size);
+
+			gdk_pixbuf_fill (new_pixbuf, 0x00000000);
+			gdk_pixbuf_copy_area (pixbuf,
+					      0, 0,
+					      gdk_pixbuf_get_width (pixbuf),
+					      gdk_pixbuf_get_height (pixbuf),
+					      new_pixbuf,
+					      x_offset, y_offset);
+			g_object_unref (pixbuf);
+			pixbuf = new_pixbuf;
+
+			for (i = 0; i < attach_points.num_points; i++) {
+				attach_points.points[i].x += x_offset;
+				attach_points.points[i].y += y_offset;
+			}
+
+			embedded_text_rect.x += x_offset;
+			embedded_text_rect.y += y_offset;
+		}
+	}

Is perhaps a bit long to have inline in the function. At least parts of
it could be put in a helper function.

> > 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?

There is a limited amount of threads loading the thumbnails, therefore
it makes sense to load the ones that are actually visible first. We
might not actually use the precise code in
nautilus_thumbnail_prioritize(), but at least it would hooks into the
same places.