GNOME Bugzilla – Bug 768062
Add an external thumbnailer for images
Last modified: 2018-04-26 12:57:11 UTC
.
Created attachment 330393 [details] [review] thumbnailer: Add an external thumbnailer for images So that broken images, or images that use too much RAM can get killed without prejudice. _gdk_pixbuf_new_from_uri_at_scale() and gnome_desktop_thumbnail_scale_down_pixbuf () are directly from gnome-desktop.
Created attachment 330394 [details] [review] thumbnailer: Helper cleanups Clean up _gdk_pixbuf_new_from_uri_at_scale() by: - Always preserving the aspect ratio - Using a single value for the target with/height - Returning errors
Created attachment 330401 [details] [review] thumbnail: Let the caller handle preview icons As we only get called for images, and that preview icons can also be used for other data types handled natively by external devices, let the caller handle those.
Review of attachment 330393 [details] [review]: ::: thumbnailer/gdk-pixbuf-thumbnailer.c @@ +33,3 @@ +} SizePrepareContext; + +#define LOAD_BUFFER_SIZE 4096 Isn't this a bit tiny ? I know we load files in 64k chunks in other places @@ +190,3 @@ + } + + if (loader == NULL) { Why not just create the loader outside the loop ? @@ +192,3 @@ + if (loader == NULL) { + loader = create_loader (file, buffer, bytes_read); + if (1 <= width || 1 <= height) { Could just put g_return_if_fail (width >=1 && height >= 1) up top ? ::: thumbnailer/gnome-thumbnailer-skeleton.c @@ +32,3 @@ +#ifndef THUMBNAILER_USAGE +#error "THUMBNAILER_USAGE must be set" +#endif Is this useful ? Could just ditch it and put the usage string below, where it is used ? @@ +269,3 @@ + output = filenames[1]; + +#ifdef THUMBNAILER_RETURNS_PIXBUF Are these ifdefs worth keeping ? Could just toss the ones we don't use ?
Review of attachment 330393 [details] [review]: ::: thumbnailer/gdk-pixbuf-thumbnailer.c @@ +33,3 @@ +} SizePrepareContext; + +#define LOAD_BUFFER_SIZE 4096 Bumped in a separate commit. The function is lifted directly from gnome-desktop. @@ +190,3 @@ + } + + if (loader == NULL) { Because the loader is created based on the data as well as the filename. We might not have data or enough data yet. @@ +192,3 @@ + if (loader == NULL) { + loader = create_loader (file, buffer, bytes_read); + if (1 <= width || 1 <= height) { Again, this is copy/pasted. A size < 1 (meaning 0, as it's a guint) in file_to_pixbuf() means that we don't resize the image. ::: thumbnailer/gnome-thumbnailer-skeleton.c @@ +32,3 @@ +#ifndef THUMBNAILER_USAGE +#error "THUMBNAILER_USAGE must be set" +#endif This is a copy/paste library from the gnome-thumbnailer-skeleton git repo, which is used for more than just the gdk-pixbuf thumbnailer: https://github.com/hadess/gnome-thumbnailer-skeleton Linked from the docs: https://developer.gnome.org/platform-overview/stable/dev-thumbnailer.html.en and Philip's blog post: https://tecnocode.co.uk/2013/10/21/writing-a-gnome-thumbnailer/
Attachment 330393 [details] pushed as 06cf4c7 - thumbnailer: Add an external thumbnailer for images Attachment 330394 [details] pushed as 6afda9c - thumbnailer: Helper cleanups Attachment 330401 [details] pushed as 42d7620 - thumbnail: Let the caller handle preview icons
Review of attachment 330394 [details] [review]: Sorry for reopening this one 6 months after the fact, but this definitely looks wrong, and Coverity is (rightfully) pointing it out (CID 1388524). ::: thumbnailer/gdk-pixbuf-thumbnailer.c @@ +50,2 @@ + if ((info->size > 0 || info->size > 0)) { + if (info->size < 0) These two lines make no sense at all, and I can’t reliably work out what the intention was. @@ +55,2 @@ } + else if (info->size < 0) This will never be reached because it’s the same as the first case. @@ +71,3 @@ + width = info->size; + if (info->size > 0) + height = info->size; Both of these `if` conditions are the same, and can never be reached because they’re in the else of `if ((info->size > 0 || info->size > 0))`.
(In reply to Philip Withnall from comment #7) > Review of attachment 330394 [details] [review] [review]: > > Sorry for reopening this one 6 months after the fact, but this definitely > looks wrong, and Coverity is (rightfully) pointing it out (CID 1388524). > > ::: thumbnailer/gdk-pixbuf-thumbnailer.c > @@ +50,2 @@ > + if ((info->size > 0 || info->size > 0)) { > + if (info->size < 0) > > These two lines make no sense at all, and I can’t reliably work out what the > intention was. It was a 0-effort attempt at removing having 2 separate height and width. No functional changes at all. Looks like I didn't finish it. > @@ +55,2 @@ > } > + else if (info->size < 0) > > This will never be reached because it’s the same as the first case. > > @@ +71,3 @@ > + width = info->size; > + if (info->size > 0) > + height = info->size; > > Both of these `if` conditions are the same, and can never be reached because > they’re in the else of `if ((info->size > 0 || info->size > 0))`. Ditto.
Created attachment 343162 [details] [review] thumbnailer: Cleanup the cleanups Commit 6afda9c removed the separate height and width arguments, as we always want to keep the original image's aspect ratio. The cleanup wasn't quite finished though, and ended up with slightly non-sensical checks (like doing comparisons twice). Coverity CID 1388524
Review of attachment 343162 [details] [review]: A lot simpler! \o/ Accepted with the simplification below, or a reason why I’m wrong and the simplification shouldn’t be done. ::: thumbnailer/gdk-pixbuf-thumbnailer.c @@ +51,1 @@ + if ((double)height * (double)info->size > (double)width * (double)info->size) { Is it necessary to multiply both sides of the comparison by `info->size`? Since we’ve guaranteed `info->size` is positive, this can’t affect the comparison unless we are expecting overflow.
Created attachment 343174 [details] [review] thumbnailer: Cleanup the cleanups Commit 6afda9c removed the separate height and width arguments, as we always want to keep the original image's aspect ratio. The cleanup wasn't quite finished though, and ended up with slightly non-sensical checks (like doing comparisons twice). Coverity CID 1388524
Attachment 343174 [details] pushed as 3aa1366 - thumbnailer: Cleanup the cleanups
Review of attachment 330393 [details] [review]: ::: thumbnailer/gdk-pixbuf-thumbnailer.c @@ +286,3 @@ + "gnome-original-width")); + original_height = GPOINTER_TO_INT (g_object_get_data (G_OBJECT (pixbuf), + "gnome-original-height")); We are losing the gnome-original-* values because they are not GdkPixbuf options, not under the "gdk_pixbuf_options" key, and hence not copied by gdk_pixbuf_copy_options. We should read them off the unoriented GdkPixbuf object. See attachment 371431 [details] [review] for a fix.