GNOME Bugzilla – Bug 708942
Shows very small images
Last modified: 2021-06-09 16:32:21 UTC
At one point we excluded screenshots from the Pictures selection. That seems to have broken since I see them now.
/* Ignore screenshots */ software = gdk_pixbuf_get_option (pixbuf, "tEXt::Software"); if (software != NULL && g_str_equal (software, "gnome-screenshot")) { g_debug ("Ignored URL '%s' as it's a screenshot from gnome-screenshot", cc_background_item_get_uri (item)); g_object_unref (pixbuf); g_object_unref (item); return; }
Attach one of the screenshots, it seems to work fine here.
Created attachment 259307 [details] screenshot
For a screenshot I had saved: $ ./foo '/home/hadess/Pictures/Screenshot from 2013-10-09 20:08:03.png' Software used: gnome-screenshot For your screenshot: $ ./foo '/home/hadess/Pictures/foo.png' Software used: (null) The software I used (which mimics what the background panel does): // gcc -o foo foo.c `pkg-config --libs --cflags gdk-pixbuf-2.0` #include <gdk-pixbuf/gdk-pixbuf.h> int main (int argc, char **argv) { GdkPixbuf *pixbuf; pixbuf = gdk_pixbuf_new_from_file (argv[1], NULL); if (pixbuf == NULL) { g_print ("Invalid file %s\n", argv[1]); return 1; } g_print ("Software used: %s\n", gdk_pixbuf_get_option (pixbuf, "tEXt::Software")); return 0; } Seems that either whatever you're using to save screenshots is not setting the tEXt::Software bit, or something you used to manipulate the file isn't.
Yeah then I think we need to do more. If I use gimp to crop a screenshot it shows up in the background chooser. This isn't what we want. I'm also seeing 16px pngs in there too. I think we should have a minimum size - I thought we had that at one point.
(In reply to comment #5) > Yeah then I think we need to do more. If I use gimp to crop a screenshot it > shows up in the background chooser. This isn't what we want. It's hardly our fault if GIMP loses metadata when cropping the file though. > I'm also seeing 16px pngs in there too. I think we should have a minimum size - > I thought we had that at one point. We can certainly do that more easily.
(In reply to comment #6) > (In reply to comment #5) > > Yeah then I think we need to do more. If I use gimp to crop a screenshot it > > shows up in the background chooser. This isn't what we want. > > It's hardly our fault if GIMP loses metadata when cropping the file though. We may want to start using a subdirectory for screenshots then. Or go back to knowing that "Screenshot *" images are screenshots.
Created attachment 259796 [details] [review] background: Consolidate exit path
Created attachment 259797 [details] [review] background: Ignore images that are too small
(In reply to comment #5) > I'm also seeing 16px pngs in there too. I think we should have a minimum size - > I thought we had that at one point. git log -S gdk_pixbuf_get_width panels/background/ got me these commits and I could not locate anything like that in them: commit 1097d4ca754ec0a340bc26127808a901e6d78a94 Author: Bastien Nocera <hadess@hadess.net> Date: Mon Aug 19 23:40:26 2013 +0200 background: Fix memory corruption when creating preview When using a single screen, the captured area was too small, and we were copying data from out-of-bounds of the pixbuf area. https://bugzilla.gnome.org/show_bug.cgi?id=696166 commit 79ec684fa4912a1e5bff17ce11b036cef4a298aa Author: William Jon McCann <jmccann@redhat.com> Date: Tue May 22 11:34:20 2012 -0400 background: New background panel design Implement a new design for the wallpaper selection. https://bugzilla.gnome.org/show_bug.cgi?id=676539 commit 035126a970da54711ae0076ccae1e79f84c081f5 Author: Bastien Nocera <hadess@hadess.net> Date: Tue Dec 14 20:29:33 2010 +0000 background: Add emblem for slideshow previews Though for some reason the icon ends up being tiny... commit e721f417adfadbe6c7a913b9721fc860433ce165 Author: Thomas Wood <thomas.wood@intel.com> Date: Tue Aug 10 15:26:07 2010 +0100 Add initial implementation of "background" panel The background settings panel provides a way for users to change the desktop background by selecting an image and/or colour.
Review of attachment 259796 [details] [review]: ::: panels/background/bg-pictures-source.c @@ +155,2 @@ g_error_free (error); + goto out; That will warn if the image failed to load as you can't run g_object_unref() on NULL objects.
Review of attachment 259797 [details] [review]: ::: panels/background/bg-pictures-source.c @@ +271,2 @@ g_object_set_data (G_OBJECT (stream), "item", item); + gdk_pixbuf_new_from_stream_async (G_INPUT_STREAM (stream), We really don't want to do that. Loading huge JPEGs for example will skip over a lot of data and a lot of decoding when loaded at a specific size. This will make the chooser *much* slower. Hopefully gdk_pixbuf_get_file_info() doesn't decode the whole file, and an async variant could be used instead.
(In reply to comment #11) > Review of attachment 259796 [details] [review]: > > ::: panels/background/bg-pictures-source.c > @@ +155,2 @@ > g_error_free (error); > + goto out; > > That will warn if the image failed to load as you can't run g_object_unref() on > NULL objects. I am using g_clear_object, not g_object_unref. Or am I missing something?
Comment on attachment 259796 [details] [review] background: Consolidate exit path You're right, never mind.
Comment on attachment 259796 [details] [review] background: Consolidate exit path Thanks for the review.
Created attachment 260107 [details] [review] background: Consolidate exit path
Review of attachment 260107 [details] [review]: Looks fine.
Created attachment 260110 [details] [review] background: Ignore images that are too small This one uses a synchronous call to gdk_pixbuf_get_file_info. I will change it to an async call on a subsequent patch because an async variant of that API is not in gdk-pixbuf, yet.
Created attachment 260113 [details] [review] background: Fix a memory leak when add_single_file failed
Created attachment 260127 [details] [review] background: Use the asynchronous version of gdk_pixbuf_get_file_info
Comment on attachment 260107 [details] [review] background: Consolidate exit path Thanks for the review.
Review of attachment 260127 [details] [review]: Please merge that in the original patch that checks for small widths/heights, no need to have 2 separate patches for that.
Review of attachment 260113 [details] [review]: At first glance, looks fine.
Comment on attachment 260113 [details] [review] background: Fix a memory leak when add_single_file failed Thanks for the review.
Review of attachment 260110 [details] [review]: This patch does not apply against master anymore.
GNOME is going to shut down bugzilla.gnome.org in favor of gitlab.gnome.org. As part of that, we are mass-closing older open tickets in bugzilla.gnome.org which have not seen updates for a longer time (resources are unfortunately quite limited so not every ticket can get handled). If you can still reproduce the situation described in this ticket in a recent and supported software version, then please follow https://wiki.gnome.org/GettingInTouch/BugReportingGuidelines and create a new bug report at https://gitlab.gnome.org/GNOME/gnome-control-center/-/issues/ Thank you for your understanding and your help.