GNOME Bugzilla – Bug 551026
Handle PNG payload in ICO files
Last modified: 2018-05-22 13:09:00 UTC
The ICO loader can't open this crummy file, but GIMP can.
Created attachment 118112 [details] ICN_OPENDISC_250.ICO File from "The Chemical Brothers - We Are The Night"'s data track.
Looks like a vista ico with the payload encoded as a png. We don't support anything like that atm.
*** Bug 581838 has been marked as a duplicate of this bug. ***
I have written a patch that implements PNG loader support in the ico loader. It's a clean implementation that uses the gdk pixbuf png loader, so the patch is actually fairly short. I have tested this against many icons I've needed to work with in the last little while, and it does the right thing. I'm also attacking a test icon that has many individual icons in it, to show that the patch chooses the correct one.
Created attachment 328096 [details] [review] Add PNG support to the ICO loader.
Created attachment 328097 [details] Test icon with several images. This icon contains the following images in the following order: - 16x16 4bit, 1bit mask BMP - 32x32 4bit, 1bit mask BMP - 16x16 32bit, 8bit alpha BMP - 32x32 32bit, 8bit alpha BMP - 48x48 32bit, 8bit alpha PNG - 128x128 32bit, 8bit alpha PNG - 256x256 32bit, 8bit alpha PNG
Review of attachment 328096 [details] [review]: Looks to me like you are leaking your pngloader, possibly. context_free() should clear it if it is set. It would be great to have representative test images for a couple of ico variants, to test this with.
Created attachment 328101 [details] [review] Add PNG support to the ICO loader, take two. unref the laoder in context_free().
(In reply to Matthias Clasen from comment #7) > Review of attachment 328096 [details] [review] [review]: > > Looks to me like you are leaking your pngloader, possibly. context_free() > should clear it if it is set. > > It would be great to have representative test images for a couple of ico > variants, to test this with. Good find. I added g_object_unref() in the context_free() function. Some of the other loaders also use gdk_pixbuf_loader_close(), but this seems to break eog for me. I'm not sure if that's required. As to a collection of test icons, should I seek out a bunch of them? Attachment 328097 [details] is meant to test the PNG support when mixed in with BMP images. That file is the same recipe as all the icons I've been dealing with. The icon that Bastien attached also has a bunch of images in no specific order, and that's a file that (at least according to icotool) isn't quite kosher.
Created attachment 328110 [details] Test Icons Took some time, but I created production-quality icons of the GTK logo. The tarball contains the icon in 256, 128, 48, 32, 16 px size icons, with 32.8bpp, 24.1bpp, 8.1bpp, 4.1bpp, and 1.1bpp depth.alpha values. Most icons are in BMP format, the 32.8bpp also appear in PNG format. The 1-bit icons are only in 16 and 32 px sizes, as would have been common in the Windows 2.0 days. There is a single "combined.ico", which includes all of the icons combined, *except* the largest depth (32.8bpp), which is only PNG. After applying the PNG patch, this icon loads more nicely, since it will go from loading a 24.1bpp BMP with a hard shadow to a 32.8bpp PNG with a soft shadow at 256x256.
Thanks for all the test images! I do see some unexpected results with the patch applied. First of all, I needed to shuffle around the loader_close and unref calls a bit, since GdkPixbufLoader requires that close is always called before finalizing a loader. I got things working by removing the close and unref calls from the error path in png_pixbuf_write, and instead adding a close call in stop_load. With those fixes, some of your test images appear wrong when loaded; they get the top half of the image fully transparent. I'll attach some representative examples.
Created attachment 328166 [details] 128-24.1bpp-bmp.ico
Created attachment 328167 [details] 48-8.1bpp-bmp.ico
Review of attachment 328101 [details] [review]: see comments
Hi Matthias, I need more information. The patch, as I supplied it, worked perfectly with the icons I submitted. The unloading and signal handlers were based closely on the same functions in the io-ani.c, which also can chain a loader. So, I'm unclear why you needed to shuffle things around, which then break the loading of the images. To me, they worked well the way they were. Unless something's changed in the git code that would necessitate it? I'm testing against 2.32.2. I'm just trying to figure out where I went wrong.
(In reply to Pat Suwalski from comment #15) > Hi Matthias, > > I need more information. The patch, as I supplied it, worked perfectly with > the icons I submitted. > > The unloading and signal handlers were based closely on the same functions > in the io-ani.c, which also can chain a loader. > > So, I'm unclear why you needed to shuffle things around, which then break > the loading of the images. To me, they worked well the way they were. Unless > something's changed in the git code that would necessitate it? I'm testing > against 2.32.2. > > I'm just trying to figure out where I went wrong. The easiest would be for you to add a few tests. A separate commit is fine for those. In particular, you'll want to add more files in tests/test-images/reftests/ and launch the pixbuf-reftest to make sure they all come out alright.
Created attachment 338211 [details] [review] Add PNG support to the ICO loader, based on 2.34.0. This is the patch regenerated against 2.34.0, which needed a bit of work because the ImageScore code had changed quite a bit. It works for me as is, but creating the test cases is still on the to-do list.
Created attachment 338212 [details] reftests directory for the ico format. This archive contains reftest images with the previously submitted test icons (.ico) and their PNG equivalents (.ico.ref.png). tests/test-images/reftests/ico/ tests/test-images/reftests/ico/128-24.1bpp-bmp.ico tests/test-images/reftests/ico/128-24.1bpp-bmp.ico.ref.png tests/test-images/reftests/ico/128-32.8bpp-bmp.ico tests/test-images/reftests/ico/128-32.8bpp-bmp.ico.ref.png tests/test-images/reftests/ico/128-32.8bpp-png.ico tests/test-images/reftests/ico/128-32.8bpp-png.ico.ref.png tests/test-images/reftests/ico/128-8.1bpp-bmp.ico tests/test-images/reftests/ico/128-8.1bpp-bmp.ico.ref.png tests/test-images/reftests/ico/16-1.1bpp-bmp.ico tests/test-images/reftests/ico/16-1.1bpp-bmp.ico.ref.png tests/test-images/reftests/ico/16-24.1bpp-bmp.ico tests/test-images/reftests/ico/16-24.1bpp-bmp.ico.ref.png tests/test-images/reftests/ico/16-32.8bpp-bmp.ico tests/test-images/reftests/ico/16-32.8bpp-bmp.ico.ref.png tests/test-images/reftests/ico/16-32.8bpp-png.ico tests/test-images/reftests/ico/16-32.8bpp-png.ico.ref.png tests/test-images/reftests/ico/16-4.1bpp-bmp.ico tests/test-images/reftests/ico/16-4.1bpp-bmp.ico.ref.png tests/test-images/reftests/ico/16-8.1bpp-bmp.ico tests/test-images/reftests/ico/16-8.1bpp-bmp.ico.ref.png tests/test-images/reftests/ico/256-24.1bpp-bmp.ico tests/test-images/reftests/ico/256-24.1bpp-bmp.ico.ref.png tests/test-images/reftests/ico/256-32.8bpp-bmp.ico tests/test-images/reftests/ico/256-32.8bpp-bmp.ico.ref.png tests/test-images/reftests/ico/256-32.8bpp-png.ico tests/test-images/reftests/ico/256-32.8bpp-png.ico.ref.png tests/test-images/reftests/ico/256-8.1bpp-bmp.ico tests/test-images/reftests/ico/256-8.1bpp-bmp.ico.ref.png tests/test-images/reftests/ico/32-1.1bpp-bmp.ico tests/test-images/reftests/ico/32-1.1bpp-bmp.ico.ref.png tests/test-images/reftests/ico/32-24.1bpp-bmp.ico tests/test-images/reftests/ico/32-24.1bpp-bmp.ico.ref.png tests/test-images/reftests/ico/32-32.8bpp-bmp.ico tests/test-images/reftests/ico/32-32.8bpp-bmp.ico.ref.png tests/test-images/reftests/ico/32-32.8bpp-png.ico tests/test-images/reftests/ico/32-32.8bpp-png.ico.ref.png tests/test-images/reftests/ico/32-4.1bpp-bmp.ico tests/test-images/reftests/ico/32-4.1bpp-bmp.ico.ref.png tests/test-images/reftests/ico/32-8.1bpp-bmp.ico tests/test-images/reftests/ico/32-8.1bpp-bmp.ico.ref.png tests/test-images/reftests/ico/48-24.1bpp-bmp.ico tests/test-images/reftests/ico/48-24.1bpp-bmp.ico.ref.png tests/test-images/reftests/ico/48-32.8bpp-bmp.ico tests/test-images/reftests/ico/48-32.8bpp-bmp.ico.ref.png tests/test-images/reftests/ico/48-32.8bpp-png.ico tests/test-images/reftests/ico/48-32.8bpp-png.ico.ref.png tests/test-images/reftests/ico/48-4.1bpp-bmp.ico tests/test-images/reftests/ico/48-4.1bpp-bmp.ico.ref.png tests/test-images/reftests/ico/48-8.1bpp-bmp.ico tests/test-images/reftests/ico/48-8.1bpp-bmp.ico.ref.png tests/test-images/reftests/ico/combined.ico tests/test-images/reftests/ico/combined.ico.ref.png
Created attachment 338213 [details] reftest screenshot. So, here's a screen capture of my reftest directory, showing gdk-pixbuf working via the shell thumbnailer. However, when I run the suggested "pixbuf-reftest" tool, I get a nonsensical error: $ ./pixbuf-reftest /pixbuf/reftest/colormap-too-small.tga: OK /pixbuf/reftest/rle-too-many-pixels-2.tga: OK /pixbuf/reftest/rle-too-many-pixels.tga: OK /pixbuf/reftest/ico/128-24.1bpp-bmp.ico: ** ERROR:pixbuf-reftest.c:164:test_reftest: assertion failed (error == NULL): Image data at 69x1 is #00000000, but should be #00000000 (gdk-pixbuf-error-quark, 0) Aborted (core dumped) Looking at the source image, pixel 69x1 the first non-transparent pixel, and obviously #00000000===#00000000. Any advice would be appreciated.
(In reply to Pat Suwalski from comment #19) > Created attachment 338213 [details] > reftest screenshot. > > So, here's a screen capture of my reftest directory, showing gdk-pixbuf > working via the shell thumbnailer. > > However, when I run the suggested "pixbuf-reftest" tool, I get a nonsensical > error: > > $ ./pixbuf-reftest > /pixbuf/reftest/colormap-too-small.tga: OK > /pixbuf/reftest/rle-too-many-pixels-2.tga: OK > /pixbuf/reftest/rle-too-many-pixels.tga: OK > /pixbuf/reftest/ico/128-24.1bpp-bmp.ico: ** > ERROR:pixbuf-reftest.c:164:test_reftest: assertion failed (error == NULL): > Image data at 69x1 is #00000000, but should be #00000000 > (gdk-pixbuf-error-quark, 0) > Aborted (core dumped) > > Looking at the source image, pixel 69x1 the first non-transparent pixel, and > obviously #00000000===#00000000. > > Any advice would be appreciated. I bet it's bug 743862. You can check this by enabling the debug at the top of the ico loader and checking the value of context->HeaderDone in gdk_pixbuf__ico_image_load_increment().
(In reply to Bastien Nocera from comment #20) <snip> > I bet it's bug 743862. You can check this by enabling the debug at the top > of the ico loader and checking the value of context->HeaderDone in > gdk_pixbuf__ico_image_load_increment(). Ignore me. I spent the afternoon root-causing the other bug, and all the bugs look like this one now ;)
(In reply to Bastien Nocera from comment #21) > Ignore me. I spent the afternoon root-causing the other bug, and all the > bugs look like this one now ;) Yep, I was just looking at that other bug when I got the message, thinking "I think I replaced all that code?!" In fact, this patch probably fixes that issue.
Review of attachment 338211 [details] [review]: This should have been 3 separate patches: - one to do a selection of icons based on the dimensions and depth (I prepared one like that for bug 743862) - one to start using a constant for the info header size (I made one, and replaced more users, including 40/41/41 in the line parsing code) - the PNG support ::: gdk-pixbuf-2.34.0.orig/gdk-pixbuf/io-ico.c @@ +49,3 @@ +#include "gdk-pixbuf-loader.h" + +#define INFOHEADER_SIZE 40 This is now on master. @@ +135,3 @@ gint height; gint DIBoffset; + gint length; This is unused. @@ +295,3 @@ GList *l; + gint width, height, depth; Something similar is now on master. @@ +413,2 @@ /* We know how many bytes are in the "header" part. */ + State->HeaderSize = entry->DIBoffset + INFOHEADER_SIZE; /* 40 = sizeof(InfoHeader) */ Already on master. @@ +650,2 @@ context->HeaderSize = 54; + context->HeaderBuf = g_try_malloc(14 + INFOHEADER_SIZE + 4*256 + 512); Also on master. @@ +659,3 @@ } /* 4*256 for the colormap */ + context->BytesInHeaderBuf = 14 + INFOHEADER_SIZE + 4*256 + 512 ; On master. @@ +1259,3 @@ for (entry = entries; entry; entry = entry->next) { icon = (IconEntry *)entry->data; + size = INFOHEADER_SIZE + icon->height * (icon->and_rowstride + icon->xor_rowstride); On master. @@ +1294,2 @@ /* bitmap header */ + dwords[0] = INFOHEADER_SIZE; On master.
(In reply to Pat Suwalski from comment #22) > (In reply to Bastien Nocera from comment #21) > > Ignore me. I spent the afternoon root-causing the other bug, and all the > > bugs look like this one now ;) > > Yep, I was just looking at that other bug when I got the message, thinking > "I think I replaced all that code?!" In fact, this patch probably fixes that > issue. No, it works around it...
I think the purpose of comment #23 is to generate a fourth version of the patch against master?
(In reply to Pat Suwalski from comment #25) > I think the purpose of comment #23 is to generate a fourth version of the > patch against master? Yes, though I'd like to fix bug 743862 first, and maybe bug 65902, before looking at this one again.
-- GitLab Migration Automatic Message -- This bug has been migrated to GNOME's GitLab instance and has been closed from further activity. You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.gnome.org/GNOME/gdk-pixbuf/issues/16.