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 776105 - Regression failing to load pixdata from GResource
Regression failing to load pixdata from GResource
Status: RESOLVED FIXED
Product: gdk-pixbuf
Classification: Platform
Component: loaders
git master
Other Linux
: Normal normal
: ---
Assigned To: gdk-pixbuf-maint
gdk-pixbuf-maint
Depends on:
Blocks:
 
 
Reported: 2016-12-14 17:26 UTC by Patrick Griffis (tingping)
Modified: 2016-12-19 16:28 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
tests: Add test for bug 776105 (1.47 KB, patch)
2016-12-18 00:27 UTC, Bastien Nocera
committed Details | Review
io: Also handle compressed GdkPixdata when loading from GResources (1.82 KB, patch)
2016-12-19 12:32 UTC, Bastien Nocera
committed Details | Review

Description Patrick Griffis (tingping) 2016-12-14 17:26:23 UTC
bug 776004 removed pixdata support and in [Arch package] version 2.36.1+9+gb1fbea9 HexChat now fails to load its icons.

As mentioned in my other comment:

> It uses GResources with the to-pixdata option[1] and loads them with
> gdk_pixbuf_new_from_resource()[2]
> 
> Anything i should change or is that a scenario you didn't intend to break?
> 
> [1] -
> https://github.com/hexchat/hexchat/blob/master/data/hexchat.gresource.xml
> [2] -
> https://github.com/hexchat/hexchat/blob/master/src/fe-gtk/pixmaps.c#L100

(In reply to Bastien Nocera)
> Please file a new bug about this. You'll want to check whether
> _gdk_pixbuf_new_from_resource_try_mmap() is called and succeeds. It's
> currently tested and working in pixbuf-resource test in the test suite. Make
> sure that also passes.

Test seems to pass:

PASS: pixbuf-resource 1 /pixbuf/resource
PASS: pixbuf-resource 2 /pixbuf/resource/at-scale

_gdk_pixbuf_new_from_resource_try_mmap() seems to fail:

Run till exit from #0  _gdk_pixbuf_new_from_resource_try_mmap (resource_path=0x5555558e0190 "/icons/ulist_voice.png")
    at /home/tingping/jhbuild/checkout/gdk-pixbuf/gdk-pixbuf/gdk-pixbuf-io.c:1667
0x00007ffff5f01094 in gdk_pixbuf_new_from_resource (resource_path=0x5555558e0190 "/icons/ulist_voice.png", error=0x0)
    at /home/tingping/jhbuild/checkout/gdk-pixbuf/gdk-pixbuf/gdk-pixbuf-io.c:1719
1719	        pixbuf = _gdk_pixbuf_new_from_resource_try_mmap (resource_path);
Value returned is $1 = (GdkPixbuf *) 0x0
Comment 1 Bastien Nocera 2016-12-16 15:41:23 UTC
Most likely the regression from bug 775693. Can you please check again with master?
Comment 2 Patrick Griffis (tingping) 2016-12-16 21:40:34 UTC
Master still fails.
Comment 3 Daniel Boles 2016-12-17 14:24:18 UTC
This appears to be caused by HexChat's usage of both the "to-pixdata" and "compressed" attributes.

I encountered this problem in my app... having said 'I'm not sure using both makes much sense, but it works for HexChat, so let's do it!' ;-) - ...and removing either fixed it. I chose to remove "to-pixdata", since "compressed" produced a considerably smaller binary.

https://bugzilla.gnome.org/show_bug.cgi?id=776004#c20
Comment 4 Bastien Nocera 2016-12-18 00:27:55 UTC
Created attachment 342133 [details] [review]
tests: Add test for bug 776105
Comment 5 Bastien Nocera 2016-12-18 00:44:44 UTC
(In reply to Daniel Boles from comment #3)
> This appears to be caused by HexChat's usage of both the "to-pixdata" and
> "compressed" attributes.
> 
> I encountered this problem in my app... having said 'I'm not sure using both
> makes much sense, but it works for HexChat, so let's do it!' ;-) - ...and
> removing either fixed it. I chose to remove "to-pixdata", since "compressed"
> produced a considerably smaller binary.

Using to-pixdata is supposed to be faster as you're not going to have to parse the PNG/JPEG/whatever, and will save RAM, as the pixbuf is already almost constructed, but it will be bigger. Compressing it means that instead of unpacking the PNG/JPEG/whatever, you're going to unpack the compressed pixdata and keep it in RAM.

Unless the data you include is in a format that's going to be available on the build machine, but not on the system where it runs (say, a JPEG-2000 image), or the file is huge, then you're better off leaving the file as uncompressed and not as pixdata.

Or use one, and not both.

(In reply to Bastien Nocera from comment #4)
> Created attachment 342133 [details] [review] [review]
> tests: Add test for bug 776105

In gdk_pixbuf_new_from_resource(), if we have compressed data, we'll need to sniff whether we have pixdata after having opened the stream, and deserialize it and load it by hand.

It's going to be slower. I wonder if we should throw a warning if "to-pixdata" and "compressed" are used together, and mention this in the GResources docs.
Comment 6 Patrick Griffis (tingping) 2016-12-18 01:04:20 UTC
It would be nice to get some real numbers next to these options. HexChat certainly isn't a noteworthy example of anything since it has a few small icons. Compression here saved me 100KB of space, pixdata saved me like 200KB of memory in my very basic testing.
Comment 7 Daniel Boles 2016-12-18 09:02:51 UTC
Thanks for the explanation.

In my case, RAM shouldn't be an issue, as I construct objects from my GResource and then unregister it. So I'll probably stick with what produces the smallest binary. That's an arbitrary criterion, but better than nothing. Using both gave the smallest results - slightly smaller than just "compressed", as using now.
Comment 8 Bastien Nocera 2016-12-19 12:32:57 UTC
Created attachment 342212 [details] [review]
io: Also handle compressed GdkPixdata when loading from GResources
Comment 9 Matthias Clasen 2016-12-19 12:39:02 UTC
Review of attachment 342212 [details] [review]:

ok
Comment 10 Bastien Nocera 2016-12-19 13:22:17 UTC
Attachment 342133 [details] pushed as a700ecc - tests: Add test for bug 776105
Attachment 342212 [details] pushed as 0af5d60 - io: Also handle compressed GdkPixdata when loading from GResources
Comment 11 Hussam Al-Tayeb 2016-12-19 16:25:44 UTC
I get a build failure:
./.libs/libgdk_pixbuf-2.0.so: undefined reference to `_gdk_pixbuf_new_from_resource_try_mmap'
collect2: error: ld returned 1 exit status
make[4]: *** [Makefile:1860: gdk-pixbuf-csource] Error 1
make[4]: Leaving directory '/home/hussam/cache/gnome/gdk-pixbuf2/src/gdk-pixbuf/gdk-pixbuf'
make[3]: *** [Makefile:2096: all-recursive] Error 1
make[3]: Leaving directory '/home/hussam/cache/gnome/gdk-pixbuf2/src/gdk-pixbuf/gdk-pixbuf'
make[2]: *** [Makefile:1551: all] Error 2
make[2]: Leaving directory '/home/hussam/cache/gnome/gdk-pixbuf2/src/gdk-pixbuf/gdk-pixbuf'
make[1]: *** [Makefile:579: all-recursive] Error 1
make[1]: Leaving directory '/home/hussam/cache/gnome/gdk-pixbuf2/src/gdk-pixbuf'
make: *** [Makefile:484: all] Error 2
Comment 12 Bastien Nocera 2016-12-19 16:28:44 UTC
(In reply to Hussam Al-Tayeb from comment #11)
> I get a build failure:
> ./.libs/libgdk_pixbuf-2.0.so: undefined reference to
> `_gdk_pixbuf_new_from_resource_try_mmap'

Fixed already.