After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 778517 - thumbnailer: Simplify the code by not using our own MIME type to GdkPixbufLoader mapping
thumbnailer: Simplify the code by not using our own MIME type to GdkPixbufLoa...
Status: RESOLVED FIXED
Product: gdk-pixbuf
Classification: Platform
Component: general
git master
Other All
: Normal normal
: ---
Assigned To: gdk-pixbuf-maint
gdk-pixbuf-maint
Depends on:
Blocks:
 
 
Reported: 2017-02-12 01:33 UTC by Debarshi Ray
Modified: 2018-12-11 08:45 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
thumbnailer: Simplify the code by reusing existing GdkPixbuf API (6.86 KB, patch)
2017-02-12 01:44 UTC, Debarshi Ray
none Details | Review
thumbnailer: Don't lose the dimensions of the original image (1.91 KB, patch)
2018-04-26 12:49 UTC, Debarshi Ray
committed Details | Review
loader: Expose the dimensions of the original image (3.22 KB, patch)
2018-04-26 12:49 UTC, Debarshi Ray
committed Details | Review
thumbnailer: Simplify the code by reusing existing GdkPixbuf API (7.63 KB, patch)
2018-04-26 12:50 UTC, Debarshi Ray
committed Details | Review

Description Debarshi Ray 2017-02-12 01:33:04 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.
Comment 1 Debarshi Ray 2017-02-12 01:44:09 UTC
Created attachment 345553 [details] [review]
thumbnailer: Simplify the code by reusing existing GdkPixbuf API
Comment 2 Bastien Nocera 2017-02-15 11:07:51 UTC
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())
Comment 3 Debarshi Ray 2018-04-26 12:49:20 UTC
Created attachment 371431 [details] [review]
thumbnailer: Don't lose the dimensions of the original image
Comment 4 Debarshi Ray 2018-04-26 12:49:50 UTC
Created attachment 371432 [details] [review]
loader: Expose the dimensions of the original image
Comment 5 Debarshi Ray 2018-04-26 12:50:28 UTC
Created attachment 371433 [details] [review]
thumbnailer: Simplify the code by reusing existing GdkPixbuf API
Comment 6 Emmanuele Bassi (:ebassi) 2018-05-01 12:37:35 UTC
Review of attachment 371431 [details] [review]:

Looks good.
Comment 7 Emmanuele Bassi (:ebassi) 2018-05-01 12:38:43 UTC
Review of attachment 371432 [details] [review]:

Okay
Comment 8 Emmanuele Bassi (:ebassi) 2018-05-01 12:40:08 UTC
Review of attachment 371433 [details] [review]:

Okay
Comment 9 Debarshi Ray 2018-05-01 15:04:15 UTC
Pushed to master. Thanks for all the reviews!
Comment 10 Bastien Nocera 2018-12-11 08:45:08 UTC
"thumbnailer: Simplify the code by reusing existing GdkPixbuf API" broke thumbnailing GIF files, see https://gitlab.gnome.org/GNOME/gdk-pixbuf/issues/99