GNOME Bugzilla – Bug 778517
thumbnailer: Simplify the code by not using our own MIME type to GdkPixbufLoader mapping
Last modified: 2018-12-11 08:45:08 UTC
The GdkPixbuf thumbnailer has its own MIME type to GdkPixbufLoader mapping in the create_loader function. That function also has this comment that was added due to bug 655406: /* need to specify the type here because the gdk_pixbuf_loader_write doesn't have access to the filename in order to correct detect the image type. */ Good news is that nowadays functions like gdk_pixbuf_new_from_file_at_size and gdk_pixbuf_get_file_info can factor in the name of the file while figuring out which GdkPixbufLoader to use. That is due to this commit: commit 496d8e39a6eec38ab0b6493736488418ec2d7000 Author: Matthias Clasen <mclasen@redhat.com> Date: Thu Dec 19 00:26:24 2013 -0500 Avoid a discrepancy in file recognition We are using a loader to implement some of the APIs that are taking a filename, such as gdk_pixbuf_new_from_file_at_scale. But the loader does not have the filename, so it can't use it when determining the image type. This makes a difference for types such as xbm, which have no good magic in the mime database. Fix this by introducing a private API that lets us pass the filename on to the loader. I'd suggest to simply use existing GdkPixbuf API to greatly simplify the thumbnailer. There are quite a few places in the GNOME platform that use g_content_type_guess in subtly different ways to guess the MIME type and how to decode a certain file. I have at least seen three examples: (a) get_content_type in gio/glocalfileinfo.c - that's used for g_file_query_info (b) _gdk_pixbuf_get_module in gdk-pixbuf/gdk-pixbuf-io.c - that's used when you use the gdk_pixbuf_new_* API (c) the GdkPixbuf thumbnailer, as already mentioned All of these can have different outcomes in various edge cases. For example, compressed SVGs (bug 655406); JPEGs saved with the '.png' extension (and vice versa); RAW images like image/x-canon-cr2 that can fallback to the TIFF loader. gdk_pixbuf_new_from_file_at_size performs better in those cases. It can handle compressed SVGs just fine; it can handle JPEGs with the '.png' extension and vice versa, which the current thumbnailer can't; and RAW images continue to fallback to the TIFF loader whenever possible. It also doesn't matter that gdk_pixbuf_new_from_file_at_size operates with file paths, not URIs. The thumbnailer already converts URIs to paths in main(). That it converts the path back to a URI is a distraction. I checked that it can handle an image on Google Drive using the FUSE path. One downside of using gdk_pixbuf_new_from_file_at_size is that it doesn't give us the original dimensions which we are currently getting from the loader. I think using gdk_pixbuf_get_file_info is an acceptable compromise. Yes, we have to do some extra I/O, but it is not a lot - we only read enough to guess the size. Plus, we avoid all the extra code, and get a thumbnailer than can handle some corner cases better.
Created attachment 345553 [details] [review] thumbnailer: Simplify the code by reusing existing GdkPixbuf API
Review of attachment 345553 [details] [review]: ::: thumbnailer/gdk-pixbuf-thumbnailer.c @@ +43,3 @@ pixbuf = tmp_pixbuf; + gdk_pixbuf_get_file_info (path, &original_width, &original_height); This creates a new loader, and will completely decode the image a 2nd time if the loader doesn't support decoding the header. Could we add those as "options" during the decoding instead? (See gdk_pixbuf_set_option())
Created attachment 371431 [details] [review] thumbnailer: Don't lose the dimensions of the original image
Created attachment 371432 [details] [review] loader: Expose the dimensions of the original image
Created attachment 371433 [details] [review] thumbnailer: Simplify the code by reusing existing GdkPixbuf API
Review of attachment 371431 [details] [review]: Looks good.
Review of attachment 371432 [details] [review]: Okay
Review of attachment 371433 [details] [review]: Okay
Pushed to master. Thanks for all the reviews!
"thumbnailer: Simplify the code by reusing existing GdkPixbuf API" broke thumbnailing GIF files, see https://gitlab.gnome.org/GNOME/gdk-pixbuf/issues/99