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 708942 - Shows very small images
Shows very small images
Status: RESOLVED OBSOLETE
Product: gnome-control-center
Classification: Core
Component: Background
unspecified
Other All
: Normal normal
: ---
Assigned To: Debarshi Ray
Control-Center Maintainers
Depends on: 712599
Blocks:
 
 
Reported: 2013-09-27 15:49 UTC by William Jon McCann
Modified: 2021-06-09 16:32 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
screenshot (18.68 KB, image/png)
2013-11-09 01:43 UTC, William Jon McCann
  Details
background: Consolidate exit path (1.74 KB, patch)
2013-11-14 11:41 UTC, Debarshi Ray
committed Details | Review
background: Ignore images that are too small (4.09 KB, patch)
2013-11-14 11:42 UTC, Debarshi Ray
rejected Details | Review
background: Consolidate exit path (3.08 KB, patch)
2013-11-18 12:45 UTC, Debarshi Ray
committed Details | Review
background: Ignore images that are too small (2.08 KB, patch)
2013-11-18 13:12 UTC, Debarshi Ray
needs-work Details | Review
background: Fix a memory leak when add_single_file failed (989 bytes, patch)
2013-11-18 13:16 UTC, Debarshi Ray
committed Details | Review
background: Use the asynchronous version of gdk_pixbuf_get_file_info (5.23 KB, patch)
2013-11-18 14:53 UTC, Debarshi Ray
reviewed Details | Review

Description William Jon McCann 2013-09-27 15:49:47 UTC
At one point we excluded screenshots from the Pictures selection. That seems to have broken since I see them now.
Comment 1 William Jon McCann 2013-09-27 15:53:13 UTC
  /* 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;
    }
Comment 2 Bastien Nocera 2013-09-28 12:16:43 UTC
Attach one of the screenshots, it seems to work fine here.
Comment 3 William Jon McCann 2013-11-09 01:43:34 UTC
Created attachment 259307 [details]
screenshot
Comment 4 Bastien Nocera 2013-11-09 02:35:25 UTC
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.
Comment 5 William Jon McCann 2013-11-13 18:50:01 UTC
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.
Comment 6 Bastien Nocera 2013-11-13 19:08:02 UTC
(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.
Comment 7 William Jon McCann 2013-11-13 19:10:56 UTC
(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.
Comment 8 Debarshi Ray 2013-11-14 11:41:47 UTC
Created attachment 259796 [details] [review]
background: Consolidate exit path
Comment 9 Debarshi Ray 2013-11-14 11:42:17 UTC
Created attachment 259797 [details] [review]
background: Ignore images that are too small
Comment 10 Debarshi Ray 2013-11-14 11:44:00 UTC
(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.
Comment 11 Bastien Nocera 2013-11-14 12:07:03 UTC
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.
Comment 12 Bastien Nocera 2013-11-14 12:11:57 UTC
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.
Comment 13 Debarshi Ray 2013-11-14 12:26:17 UTC
(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 14 Bastien Nocera 2013-11-14 12:42:03 UTC
Comment on attachment 259796 [details] [review]
background: Consolidate exit path

You're right, never mind.
Comment 15 Debarshi Ray 2013-11-14 14:37:21 UTC
Comment on attachment 259796 [details] [review]
background: Consolidate exit path

Thanks for the review.
Comment 16 Debarshi Ray 2013-11-18 12:45:17 UTC
Created attachment 260107 [details] [review]
background: Consolidate exit path
Comment 17 Bastien Nocera 2013-11-18 13:03:59 UTC
Review of attachment 260107 [details] [review]:

Looks fine.
Comment 18 Debarshi Ray 2013-11-18 13:12:16 UTC
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.
Comment 19 Debarshi Ray 2013-11-18 13:16:59 UTC
Created attachment 260113 [details] [review]
background: Fix a memory leak when add_single_file failed
Comment 20 Debarshi Ray 2013-11-18 14:53:58 UTC
Created attachment 260127 [details] [review]
background: Use the asynchronous version of gdk_pixbuf_get_file_info
Comment 21 Debarshi Ray 2013-11-18 14:55:55 UTC
Comment on attachment 260107 [details] [review]
background: Consolidate exit path

Thanks for the review.
Comment 22 Bastien Nocera 2013-11-18 14:57:20 UTC
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.
Comment 23 Bastien Nocera 2013-11-18 14:57:52 UTC
Review of attachment 260113 [details] [review]:

At first glance, looks fine.
Comment 24 Debarshi Ray 2013-11-20 16:45:17 UTC
Comment on attachment 260113 [details] [review]
background: Fix a memory leak when add_single_file failed

Thanks for the review.
Comment 25 Georges Basile Stavracas Neto 2018-01-18 13:14:02 UTC
Review of attachment 260110 [details] [review]:

This patch does not apply against master anymore.
Comment 26 André Klapper 2021-06-09 16:32:21 UTC
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.