large favicons (>32 KB) won't show up in url bar autocomplete sites like http://www.howstuffworks.com (which as a 50 KB + favicon, see http://static.howstuffworks.com/en-us/www/misc/favicon.ico) will not show a favicon in the url bar search results. see bug #332770 for some details. looking closer at howstuffworks' large favicon, it has multiple resolutions with various color depths. on the fly, for favicons over our limit, could we pull out the "best" resolution / depth / size (16 x 16 px) for us, convert it to a data url, and store that as the favicon?

http://litmus.mozilla.org/show_test.cgi?id=4522 in-litmus+ Let me know if you find anymore good testcases; thanks!

adding where else the favicon won't show. but note, you will see it in the location bar (but not autocomplete) and in tabs. those are using favicon directly, and not the favicon service. for future reference: in FaviconLoadListener::OnDataAvailable(), if our data is more than MAX_FAVICON_SIZE, we bail out.

Data point: I looked at a couple of sites that had favicon collections. One had ~300 icons, of which ~5% were larger than 1024 bytes. All icons were under 1100 bytes, and all were png/gif/jpg. Compressing those "large" icons as PNGs got them to ~700 bytes [One needs to be careful about saving them as 32bpp PNGs, as indexed color PNGs are inefficient at small sizes -- the color table chunk is fat.] Another site with ~400 icons had a similar proportion of image sizes. I didn't try converting the larger ones. So, 1024 seems like a reasonable threshold to trigger a scaling and/or reencoding. IE, sensible 16x16 icons should be under that threshold so we won't waste time reencoding them. Pathological 16x16 icons that don't compress well should be rare.

This is still rather rough. It's mostly functional, but kind of crashy.

This is some debug data, where I enabled recompression for all favicons (regardless of size) and visited 91 sites from Digg, Fark, Slashdot, and the top 25 Alexa sites. Some remarks: * 21 sites had original icons that were 318 bytes (2 of these sites were actually a bit smaller). 9 of those icons increased in size by recompressing them. The other 12 saved an average of 108 bytes. * 8 sites had icons > 318 bytes, but smaller than 1024. All compressed smaller, an average of 24%. * The icons for the other 70 sites totaled 164,434 bytes. Recompressed, they were 125,094, a 24% reduction in size. The largest reduction was 9062-->678 (-8384). * 5 of the 91 icons were GIFs, 1 was PNG, and the rest were image/x-icon. I'm not sure how to compare the tradeoff of spending X cpu cycles to save Y bytes. 1024 still seems like a reasonable threshold to trigger recompression, although with more analysis lowering it might make sense. I think our biggest bang-per-buck comes from reducing the larger images anyway, so I'm inclined not to worry about saving a few bytes on smaller images. Looking at my own places.sqlite, it looks like 2/3 to 3/4 of the icons are larger than 1024.

Seems to be functional and not crashy now. :-) My C++ is a little rusty, though, so might want to pay special attention for pointer/XPCOM abuse.

One more data point: The places.sqlite in my daily-usage profile is 45.4MB, of which 8.4MB (18.5%) is favicon BLOB data (3317 icons).

Bug 400682 isn't blocked by this, for the same reason bug 402703 wasn't... From that bug, I wrote: "I don't think 389273 is a dependency here; it will avoid the problem in this bug by reencoding large (in bytes) images to 16x16, but a small (in bytes) image that's bigger than 16x16 would still exhibit this problem."

Comment on attachment 287367 [details] [diff] [review] Patch for review, v.1 Sorry for the delay. For the places parts, r=sspitzer I'm leaving most of the image stuff for Stuart to review, but I did have a few comments/questions/nits. 1) + * @param aContainer + * An imgIContainer holding the decoded image. Specify |null| when + * calling to have one created, otherwise specify a container to + * reuse. + */ + imgIContainer decodeImageData ([array, size_is(aLength), const] in PRUint8 aData, + in unsigned long aLength, + in ACString aMimeType, + in imgIContainer aExistingContainer); a) Shouldn't aExistingContainer be an "inout" parameter? b) Shouldn't you be using "octet" instead of "PRUint8" 2) + /** + * encodeScaledImage + * Scales the provided image to the specified dimensions, encodes the + * result as a PNG, and returns a stream to the caller. + * + * @param aContainer + * A buffer holding an image file. + * @param aWidth, aHeight + * The size desired for the resulting image. + */ + nsIInputStream encodeScaledImage (in imgIContainer aContainer, + in long aWidth, + in long aHeight); +}; a) Why not have an additional parameter to encodeScaledImage for the image type (so the caller could pass in an ACString for the mime type, "image/png"), like you do for decodeImageData()? b) Also, I think your comment should indicate that aWidth and aLength are in pixels 3) + // Reencode the image + nsCOMPtr<imgIEncoder> encoder = do_CreateInstance("@mozilla.org/image/encoder;2?type=image/png"); + if (!encoder) + return NS_ERROR_FAILURE; + + // XXX if source image had no transparency, provide "transparency=no" option? + long strideSize = dest->Stride(); + rv = encoder->InitFromData(dest->Data(), aHeight * strideSize, aWidth, aHeight, strideSize, + imgIEncoder::INPUT_FORMAT_HOSTARGB, EmptyString()); + NS_ENSURE_SUCCESS(rv, rv); Is that XXX comment about transparency why you want to keep encodeScaledImage() as a PNG, and not pass in the mime type? Perhaps this XXX comment could be conditionally, based on the passed in mimeType? 4) + //nsCAutoString spec; + //aFavicon->GetSpec(spec); + //printf("Optimizing favicon %s (%s)...\n", spec.get(), PromiseFlatCString(aMimeType).get()); nit, please remove the commented out code. 5) + //printf("Original favicon size = %d, new size = %d\n", aDataLen, dataLen); nit, please remove the commented out code. 6) + rv = imgtool->EncodeScaledImage(container, 16, 16, getter_AddRefs(iconStream)); + NS_ENSURE_SUCCESS(rv, rv); ... + aNewMimeType.AssignLiteral("image/png"); As mentioned previously, I think we should pass in "image/png" here, and use that for aNewMimeType as well. 7) { - if (aOffset + aCount > MAX_FAVICON_SIZE) - return NS_ERROR_FAILURE; // too big Do we want any sort of cut-off? (What should we do if someone specifies an 1MB .ico file?) 8) Do we preserve transparency, no matter what the original format of the ico image? (does the png decoder do that for us?)

(In reply to comment #10) > b) Shouldn't you be using "octet" instead of "PRUint8" This seems to be used inconsistently in the tree. :-( The similar imgIEncoder/Decoder use PRUint8, so I'll stick with that. > + // XXX if source image had no transparency, provide "transparency=no" > option? > Is that XXX comment about transparency why you want to keep > encodeScaledImage() as a PNG, and not pass in the mime type? No, it's actually a question for Stuart. :-) We're encoding transparency into the PNG as-is, but if we can determine that there's no transparency used in the source image (eg, because it's unsupported, ala JPEG), then we can skip encoding the alpha channel (RGB pixels, instead of RGBA), and presumably get a smaller PNG. Not a big deal, but seems like a simple optimization. > - if (aOffset + aCount > MAX_FAVICON_SIZE) > - return NS_ERROR_FAILURE; // too big > > Do we want any sort of cut-off? (What should we do if someone specifies an > 1MB .ico file?) I don't think so... It's no different than someone including <img src="ginormous-image.jpg"> in the HTML. > Do we preserve transparency, no matter what the original format of the ico > image? (does the png decoder do that for us?) Yes, see above.

This fixes some problems in the imgTools.cpp, which caused it to scale and paint improperly. I also added a testcase.

Here's the full patch with a small proposed change. I think it would make sense to use an nsIInputStream as input for encodeScaledImage instead of a byte array. This will enable easy piping from output streams, direct access to various data sources that are exposed using an input stream (e.g. files) and also makes it possible (or at least much easier) to use this great utility class from JavaScript. (I need this for bug 400179.) A couple of other suggestions I'd be interested in your opinion on: (1) Shouldn't scaling and encoding be separated into two methods? In the case of bug 400179, for example, I just need to reencode, so having the scaling take place is inefficient. (2) imgIContainer decodeImageData (in nsIInputStream aInStr, in ACString aMimeType, in imgIContainer aContainer); I guess this will make it a bit fiddlier to use from C++ if you have an existing container, since you'd have to use a temporary variable. On the other hand, the JS syntax for inout parameters is a bit confusing so this would make use from JS a lot easier assuming that in the normal scenario the caller doesn't have a container yet. Anyway, I wouldn't go to the mat for this one. If you like either of these ideas, I'd be happy to implement and submit a new patch.

Comment on attachment 291906 [details] [diff] [review] Use stream as input for mozITools.encodeScaledImage sorry this has taken so long to get to... + frame->LockImageData(); I don't understand why you're locking this -- I don't see an unlock. This is bad. aside from that this looks ok.

(In reply to comment #17) > (From update of attachment 291906 [details] [diff] [review]) > sorry this has taken so long to get to... > > + frame->LockImageData(); > > I don't understand why you're locking this -- I don't see an unlock. This is > bad. > > aside from that this looks ok. Sorry, I put that in there for debugging purposes. I'll post a new patch.

(In reply to comment #16) > Here's the full patch with a small proposed change. I think it would make sense > to use an nsIInputStream as input for encodeScaledImage instead of a byte > array. Since my version was rolling the byte array into a stream anyway, that seems sensible, especially for callers who might already have a stream (to avoid stream -> buffer -> stream crazyness). Can there be an issue, though, if a caller provides a stream that's fed by a network connection that could block? Or can that not happen? > Shouldn't scaling and encoding be separated into two methods? In the case of > bug 400179, for example, I just need to reencode, so having the scaling take > place is inefficient. Maybe? I briefly considered it at first, but seemed like it would be a pain to get at the gfxContext/Surface again. Maybe just shortcut the scaling when aWidth == aHeight == 0? > imgIContainer decodeImageData (in nsIInputStream aInStr, > in ACString aMimeType, > in imgIContainer aContainer); > > I guess this will make it a bit fiddlier to use from C++ if you have an > existing container, since you'd have to use a temporary variable. I left this as an inout (for aContainer), since the code this was based on (bug 296818) reuses image containers. That code should probably be changed to use imgITools to avoid the duplication, but since it's still kind of new I didn't want to get entangled in that right now (ie, followup bug!). Stuart might have opinions on that too.

(In reply to comment #20) > > Shouldn't scaling and encoding be separated into two methods? In the case of > > bug 400179, for example, I just need to reencode, so having the scaling take > > place is inefficient. > > Maybe? I briefly considered it at first, but seemed like it would be a pain to > get at the gfxContext/Surface again. Maybe just shortcut the scaling when > aWidth == aHeight == 0? Yeah, I thought about a shortcut too, but I don't think gfxContext/Surface are needed anyway for reencoding. I'll give this some thought and propose a patch when I get a chance. Is this bug supposed to land for 1.9?

do you want me to wait for review until you post a new patch?

Justin, do you mind if I take a crack at splitting up the scaling and reencoding methods or do you want to get this checked in first?

This includes: * Change interface to take a stream instead of a byte array (from Matt) * New interface to allow encoding without scaling * Unit test specifically for imgITools

These go in /modules/libpr0n/test/unit/. The icons from attachment 288531 [details] are also still needed, in /toolkit/components/places/tests/unit/.

Comment on attachment 294518 [details] [diff] [review] Patch for review, v.4 You don't want to do this inside the else clause in imgTools::EncodeScaledImage() (line 324 in the patched file): nsRefPtr<gfxImageSurface> dest = new gfxImageSurface( gfxIntSize(aScaledWidth, aScaledHeight), gfxASurface::ImageFormatARGB32); This causes dest to be destroyed before the data is passed to the encoder's InitFromData() method. On Windows, in debug mode, the result is a pleasingly minimalistic gray rectangle, but that's small comfort. One solution would be to move the declaration of dest outside of the if/else construct entirely and just do the assignment at the aforementioned location.

Comment on attachment 294518 [details] [diff] [review] Patch for review, v.4 Moving the declaration out of the "else" block fixed the problem.

Oh, oops, I added the if/else scope in the last patch and didn't change the nsRefPtr scope. This can and should just be moved out of the block as Mark/Matt suggest. Stuart, is this still on your review radar?

Checked in, with fix from comment 26/27.

Ah, nuts. Test failures. While I look in this, I've backed out the changes to: toolkit/components/places/src/nsFaviconService.cpp toolkit/components/places/src/nsFaviconService.h and disabled the tests: toolkit/components/places/tests/unit/test_favicons.js modules/libpr0n/test/unit/test_imgtools.js Windows failed with: *** CHECK FAILED: 905 == 948 [...] test_libpr0n/unit/test_imgtools.js :: compareArrays :: line 87 [...] test_libpr0n/unit/test_imgtools.js :: run_test :: line 208 Looks like it working, but getting a different encoded result for one test. Linux failed by crashing while running test_imgtools.js. :-( Some Gtk errors logged before (?) the crash may or may not be relevant: (process:29021): GLib-GObject-CRITICAL **: gtype.c:2240: initialization assertion failed, use IA__g_type_init() prior to this function (process:29021): Gdk-CRITICAL **: gdk_pango_context_get_for_screen: assertion `GDK_IS_SCREEN (screen)' failed (process:29021): GLib-GObject-CRITICAL **: g_object_get_qdata: assertion `G_IS_OBJECT (object)' failed (process:29021): GLib-GObject-CRITICAL **: g_object_unref: assertion `G_IS_OBJECT (object)' failed I'll look into this.

After some cajoling, I managed to get a stack from the core file when test_imgtools.js crashes on Linux. It's triggered by the first test in the file, inside imgTools::DecodeImageData()... nsPNGDecoder::WriteFrom() has read all the data in the provided stream and finished decoding it. Then things look a little questionable: it tries to optimize to a native surface, starts to call out XRender, and crashes because there no display. I'm guessing there's something about the xpcshell environment that causes this, since it doesn't have a window around (and invoking this code from Firefox while browsing works fine). Maybe the tests need to explicitly do some extra work to initialize something? Or imgTools needs to do some extra initialization to allow image processing in a CLI-only environment?

On Windows, the test failures are in tests 5 and 6. The encoded images are valid, but some pixels differ by 1 or 2 values (hilighted in this attachment for test5). Test 1 decodes a 64x64 PNG. Test 2 encodes it as a 16x16 JPEG, test 3 as a 64x64 JPEG. Test 4 decodes a 32x32 JPEG. Test 5 encodes it as a 16x16 PNG, test 6 as a 32x32 PNG. Test 7 decodes a 16x16 ICO. Test 8 encodes it as a 32x32 PNG, test 7 as a 16x16 PNG. It looks like either the JPEG decoder is giving slightly different output, or the scaling gives slightly different results (which might be hidden by the lossy JPEG encoding in test 3). It might be easiest to have Windows-specific reference files for the test.

This disables animation of favicons like here? http://www.elektronotdienst-nuernberg.de

Animations are still shown in the tab and urlbar favicon. The reencoded version is used in the history/bookmarks menus and places organizer, and is not animated. IMO, that's a feature not a bug!

Relanded. - Fixed the Linux crash with bug 412378. - The imgITools tests (test_imgtools.js) have been partially disabled on Windows due to some pixels varying slightly from the reference output [eg, a pixel value being rgb(100,101,100) instead of rgb(100,100,100)]. I tried adding Windows-specific reference images, but they passed locally and failed in the Windows test box. I've had this same problem with other tests, and have filed bug 413092 to try and debug the problem locally. - The nsFaviconService tests (test_favicons.js) were also partially disabled on Windows, for the same reason.

Verified FIXED using: Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.4; en-US; rv:1.9b3pre) Gecko/2008020419 Minefield/3.0b3pre Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b3pre) Gecko/2008020404 Minefield/3.0b3pre Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9b3pre) Gecko/2008020419 Minefield/3.0b3pre