Closed Bug 549468 Opened 15 years ago Closed 13 years ago

Add basic support for .ico icon encoder

Categories

(Core :: Graphics: ImageLib, defect)

Product:
Core
Component:
Graphics: ImageLib
Platform:
x86
Windows 7
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla9

People

(Reporter: jimm, Assigned: bbondy)

References

Details

(Whiteboard: [sec-assigned:dveditz])

Attachments

(2 files, 12 obsolete files)

reftests for BMP and ICO encoders
(deleted), patch
bbondy
: review+
Details | Diff | Splinter Review
Patch for working ICO encoder + review comments
(deleted), patch
joe
: review+
Details | Diff | Splinter Review
We'll need this for win7 jump list fave icon support. Icons on uri displayed in the lists need to point to local ico files, which we'll have to cache for the list under the app data dir or similar. For this we'll need the ability to write fave icons out as ico files.
Blocks: 549472
Assignee: nobody → jmathies
One way to simplify this would be to save PNG-encoded ICO files, which are supported by Vista and later. I think this would let "outsource" most of the work to the PNG encoder. And would result in smaller (but incompatible with XP) .ico files. I've filed bug 600556 asking for support of rendering PNG-encoded ICO files a few weeks ago.
Depends on: 600556
Assignee: jmathies → netzen
Depends on: 670466
I added a new task which should be implemented before implementing the .ico encoder. Bug 670466 - Add basic support for .bmp encoder I'll start on Bug 670466 Monday and then when that is done will do this task after that.
Just to recap the task: Favicons usually have ICO format because it is the only supported format for IE, but sometimes websites will use other formats. Favicons for Firefox are supported with: ICO, PNG, GIF, JPEG, PNG, or SVG (possibly more). Jumplist entries must be in ICO format. We therefore need to be able to write out ICO files from images which are not in ICO format (in the cases where different favicon formats were used by sites). To do this we need to implement an ICO encoder modules/libpr0n/encoders. (Currently we only have encoders for jpeg and png, but not ICO.) Here is an example with a PNG favicon: <html> <head> <link rel="icon" type="image/png" href="http://example.com/image.png" /> </head> <body></body> </html> I am implementing the ICOs with a contained BMP since if we need shell integration in older Windows OS that will be supported and there is no benefit to using 16x16 PNGs in size.
(In reply to comment #3) > I am implementing the ICOs with a contained BMP since if we need shell > integration in older Windows OS that will be supported and there is no > benefit to using 16x16 PNGs in size. I think it may be better to go with PNG's though, because: * the PNG encoder already exists in the code. You could plug it in, right? * jumplists are only supported in Windows 7, while PNG icons are supported since Vista * even though PNG icons are not supported in XP, Microsoft does its best to phase out XP soon anyway. And even considering its current popularity, the fact, that we have been living without an ICO encoder all the time, indicates that there hasn't been much demand for it in older versions of Windows anyway. Or am I wrong? :)
I'm not sure if we plan to do any kind of shell integration with FF pinned tabs or not. In any case there is the ability to use encoder options so I think I'll support both ways. Also data can be exported to the OS with <canvas> now so people could export a file that doesn't work for PNG ICO, then they'd probably file a bug report.
You mean someone may want to right-click on a canvas element and save it as an ico file? I personally would not expect Firefox to support exporting as ICO at all. It might be a cool feature, but I don't think it would be used often enough to justify its existence.
With the introduction of an ICO encoder you get support for <canvas>.toDataURL automatically. With that function anyone can specify a mime type. For unsupported mime types it just returns PNG as per spec of the data URI scheme. With the ICO encoder it will now actually export ICOs. It might not be used often but its possible to do. Also it is with that function that I'm creating ref tests to ensure we don't have a buggy encoder. As mentioned though I'll support both PNG and BMP ICOs, so it's nothing to worry about. The user of the encoder can pick what to use.
Attached file Intermediate patch for .ico icon encoder (obsolete) (deleted) —
Attached is an intermediate patch for an ICO encoder that supports PNG ICOs, 24BPP, and 32BPP. Tomorrow I will finish up and also create ref tests.
Attached patch Intermediate patch for .ico icon encoder (obsolete) (deleted) — — Splinter Review
(Forgot to check on patch in the last message)
Attachment #545308 - Attachment is obsolete: true
I added a patch to Bug 670466 which contains the reftests for the ICO encoder. Please do not run them though until I submit a new code patch which fixes some of the tests that currently fail the reftests.
Attached patch Finalized patch for ico encoder (obsolete) (deleted) — — Splinter Review
Implemented the encoder which supports PNG, BMP32BPP, and BMP24BPP. All reftests I made currently pass for it and the BMP encoder. I will wait to implement the review comments on the decoder before asking for a review here.
Attachment #545310 - Attachment is obsolete: true
Attached patch Finalized patch for ico encoder (obsolete) (deleted) — — Splinter Review
minor update
Attachment #545583 - Attachment is obsolete: true
Attached patch Patch for working ICO encoder (obsolete) (deleted) — — Splinter Review
Minor fixes so that the patch works with its dependant Bug 600556 I will wait for r? until I do some extra refactoring/commenting on the patch.
Attachment #545981 - Attachment is obsolete: true
Attached patch Patch for working ICO encoder (obsolete) (deleted) — — Splinter Review
Some code refactoring and commenting, ready for review.
Attachment #546444 - Attachment is obsolete: true
Attachment #546587 - Flags: review?(joe)
Blocks: 668068
Comment on attachment 546587 [details] [diff] [review] Patch for working ICO encoder Review of attachment 546587 [details] [diff] [review]: ----------------------------------------------------------------- My major objection is ContainableEncoder, which requires multiple inheritance and (right now) defines a symbol that's already defined in a separate interface, which is a no-no. I think your best bet might be to extend the mozilla::imagelib::Decoder interface instead. Also, I think you'll want to reimport your license boilerplate for all the new files from http://www.mozilla.org/MPL/boilerplate-1.1/ - just in case. I found some errors from copy and pasting old headers that makes me worry that I missed other license errors. Note also that the initial developer is the Mozilla Foundation. http://weblogs.mozillazine.org/gerv/archives/2010/02/mpl_initial_developer_for_mozilla_employ.html ::: modules/libpr0n/build/nsImageModule.cpp @@ +101,5 @@ > { "@mozilla.org/image/loader;1", &kNS_IMGLOADER_CID }, > { "@mozilla.org/image/request;1", &kNS_IMGREQUESTPROXY_CID }, > { "@mozilla.org/image/tools;1", &kNS_IMGTOOLS_CID }, > { "@mozilla.org/image/rasterimage;1", &kNS_RASTERIMAGE_CID }, > + { "@mozilla.org/image/encoder;2?type=image/ico", &kNS_ICOENCODER_CID }, This isn't actually the right MIME type for icons; the officially registered MIME type is image/vnd.microsoft.icon, though image/x-icon is also pretty common. It doesn't actually matter that much, especially not for encoders, but I wanted to make sure this was officially noted. ::: modules/libpr0n/encoders/ico/Makefile.in @@ +13,5 @@ > +# > +# The Original Code is mozilla.org code. > +# > +# The Initial Developer of the Original Code is Mozilla Foundation > +# Portions created by the Initial Developer are Copyright (C) 2005 2011 :) ::: modules/libpr0n/encoders/ico/nsICOEncoder.cpp @@ +14,5 @@ > + * > + * The Original Code is ICO Encoding code > + * > + * The Initial Developer of the Original Code is > + * Google Inc. No sir! @@ +55,5 @@ > + mImageBufferSize(0), mImageBufferUsed(0), > + mImageBufferReadPoint(0), mCallback(nsnull), > + mCallbackTarget(nsnull), mNotifyThreshold(0), > + mUsePNG(PR_TRUE), > + mReentrantMonitor("nsICOEncoder.mReentrantMonitor") Quibble - I often prefer Foo::Foo() : mFoo(0), mBar(1), (or alternately : mFoo(0) , mBar(1) ) @@ +69,5 @@ > + } > +} > + > +// nsICOEncoder::InitFromData > +// Two output options are supported: bpp=<bpp_value>;png=<yes|no> I'd rather format=<png|bmp> @@ +90,5 @@ > + // Stride is the padded width of each row, so it better be longer > + if ((aInputFormat == INPUT_FORMAT_RGB && > + aStride < aWidth * 3) || > + ((aInputFormat == INPUT_FORMAT_RGBA || aInputFormat == INPUT_FORMAT_HOSTARGB) && > + aStride < aWidth * 4)) { Here, you should vertically align aStride such that it's right below aInputFormat, because it's part of the same bracketed clause. Similarly, make sure the first ( in ((aInputFormat is vertically aligned directly below the second ( in if ((aInputFormat. Basically, make sure indentation follows bracket depth. Do this throughout, please. @@ +114,5 @@ > +NS_IMETHODIMP > +nsICOEncoder::AddImageFrame(const PRUint8* aData, > + PRUint32 aLength, // (unused, > + // req'd by JS) > + PRUint32 aWidth, Can you fix the formatting of this function's parameters? @@ +126,5 @@ > + mContainedImage = new nsPNGEncoder(); > + nsresult rv; > + nsAutoString noParams; > + rv = mContainedImage->InitFromData(aData, aLength, aWidth, aHeight, > + aStride, aInputFormat, noParams); Can you vertically align aStride here with aData? vim has a nice configuration setting to let you do this: set cino=(0,w1 (Do this throughout everything) @@ +133,5 @@ > + return rv; > + > + mImageBufferSize = ICONFILEHEADERSIZE + ICODIRENTRYSIZE + > + mContainedImage->GetImageBufferSize(); > + mImageBufferStart = (PRUint8*)PR_Malloc(mImageBufferSize); Why PR_Malloc? @@ +229,5 @@ > + return NS_ERROR_INVALID_ARG; > + > + // parse and check any provided output options > + PRUint32 bpp; > + PRBool usePNG; You should probably initialize these in this function instead of relying on ParseOptions to initialize them. @@ +232,5 @@ > + PRUint32 bpp; > + PRBool usePNG; > + nsresult rv = ParseOptions(aOutputOptions, &bpp, &usePNG); > + if (rv != NS_OK) > + return rv; NS_SUCCEEDED(rv) instead of != NS_OK. @@ +278,5 @@ > + nsCAutoString optionsCopy; > + optionsCopy.Assign(NS_ConvertUTF16toUTF8(aOptions)); > + char* options = optionsCopy.BeginWriting(); > + > + while (char* token = nsCRT::strtok(options, ";", &options)) { I think it might be a bit cleaner if you used ParseString or FindInReadable (see nsReadableUtils.h) rather than strtok. See also https://developer.mozilla.org/en/XPCOM_string_guide At a bare minimum, though, you should switch to NS_strtok. @@ +363,5 @@ > + void *aClosure, PRUint32 aCount, > + PRUint32 *_retval) > +{ > + // Avoid another thread reallocing the buffer underneath us > + ReentrantMonitorAutoEnter autoEnter(mReentrantMonitor); Did this happen to you? Imagelib is generally pretty aggressively single-threaded. @@ +477,5 @@ > + mICODirEntry.mBitCount = aBPP; > + mICODirEntry.mBytesInRes = 0; > + mICODirEntry.mColorCount = 0; > + mICODirEntry.mWidth = aWidth; > + mICODirEntry.mHeight = aHeight; aWidth and aHeight are PRUint32s, but mWidth and mHeight are PRUint8s. Give careful thought to what happens here if the PRUint32s are too big for the PRUint8s! ::: modules/libpr0n/encoders/ico/nsICOEncoder.h @@ +13,5 @@ > + * License. > + * > + * The Original Code is Oracle Corporation code. > + * > + * The Initial Developer of the Original Code is Oracle Corporation. Hmm.... @@ +85,5 @@ > + void EncodeInfoHeader(); > + > + // Holds either a PNG or a BMP depending on the encoding options specified > + // or if no encoding options specified will use the default (PNG) > + nsAutoPtr<mozilla::imagelib::ContainableEncoder> mContainedImage; This should probably be called mContainedEncoder. Also, you could add "using mozilla::imagelib" and s/mozilla::imagelib::// throughout. ::: modules/libpr0n/src/ContainableEncoder.h @@ +15,5 @@ > + * The Original Code is mozilla.org code. > + * > + * The Initial Developer of the Original Code is > + * the Mozilla Foundation. > + * Portions created by the Initial Developer are Copyright (C) 2010. 2011 @@ +46,5 @@ > +{ > +public: > + virtual PRUint32 GetImageBufferSize() const = 0; > + virtual PRUint8* GetImageBuffer() const = 0; > + NS_IMETHOD InitFromData(const PRUint8* aData, It's quite weird to have an NS_IMETHOD (which means that it's an IDL-defined interface method) defined in a regular class. You might be a bit happier if you create a separate method that just calls the imgIEncoder version (implemented either in ContainableEncoder or in the concrete classes, it doesn't matter which).
Attachment #546587 - Flags: review?(joe) → review-
Thanks for the review, I'll: - Extend mozilla::imagelib::Decoder interface instead of having the multiple inheritance and nuke ContainableEncoder. - Use image/vnd.microsoft.icon - Fixup licensing at top of files and pay attention to that next time I create a new file. - Change to format=<png|bmp> - Do some kind of error checking when width and height are out of bounds. - Fixup formatting and other comments
> > ReentrantMonitorAutoEnter autoEnter(mReentrantMonitor); > Did this happen to you? Imagelib is generally pretty aggressively single-threaded. It did not happen to me but I put it the same as the PNG and JPEG encoders. Maybe I can add another task to remove from all encoders separately so it stays consistent with the other encoders?
Attached patch Patch for working ICO encoder + review comments (obsolete) (deleted) — — Splinter Review
All comments implemented and formatting fixed.
Attachment #546587 - Attachment is obsolete: true
Attachment #548648 - Flags: review?(joe)
Attached patch reftests for BMP and ICO encoders (obsolete) (deleted) — — Splinter Review
This reftest patch was previously r+'d in Bug 670466. - I changed the formatting options to format=png|bmp to match the code - I changed the mime type to the proper ico encoding to match the code - I moved the reftest patch to this bug because it contains both ICO and BMP tests, and this bug (ico encoder) comes after Bug 670466 (bmp encoder)
Attachment #548650 - Flags: review?(joe)
Attached patch Patch for working ICO encoder + review comments (obsolete) (deleted) — — Splinter Review
Implemented Review comments from Bug 670466 but applied to this bug.
Attachment #548648 - Attachment is obsolete: true
Attachment #550723 - Flags: review?(joe)
Attachment #548648 - Flags: review?(joe)
Comment on attachment 548650 [details] [diff] [review] reftests for BMP and ICO encoders Review of attachment 548650 [details] [diff] [review]: ----------------------------------------------------------------- Needs a couple of minor updates, but overall looks good. I don't need to review this again. ::: modules/libpr0n/test/reftest/encoders-lossless/encoder.html @@ +13,5 @@ > + // - options=<canvas toDataURL encoder options> > + // Example: > + // encoder.html?img=escape(reference_image.png) > + // &mime=escape(image/ico) > + // &options=escape(-moz-parse-options:bpp=24;png=no) Should probably update this example of -moz-parse-options for format=<png|bmp> if that's what we're going with. (Not that it's that important, as it's just an example.) @@ +28,5 @@ > + } > + if (isFinite(Date.parse(sValue))) { > + return(new Date(sValue)); > + } > + return(sValue); Return isn't a function, so you don't need to put brackets around the things you're returning. @@ +63,5 @@ > + > + // Once the image loads async, we then draw the image onto the canvas, > + // and call canvas.toDataURL to invoke the encoder, and then set a new > + // image to the encoded data URL > + function onReferenceImageLoad() { function openbrace goes on its own line. @@ +73,5 @@ > + // Obtain the canvas and draw the reference image > + canvas.width = img.width; > + canvas.height = img.height; > + var ctx = canvas.getContext('2d') > + ctx.drawImage(img, 0, 0, img.width, img.height); You don't need to specify img.width and img.height here, since drawImage defaults to that. @@ +77,5 @@ > + ctx.drawImage(img, 0, 0, img.width, img.height); > + > + // Obtain the data URL with parsing options if specified > + var dataURL; > + if(parseOptions && parseOptions.length > 0) I checked, and the empty string is interpreted as false, so you don't need parseOptions.length > 0 here. Also, you can put a space between if and ( throughout.
Attachment #548650 - Flags: review?(joe) → review+
> Return isn't a function... Yes I know :) It was leftover from the previous reference you gave me for getting URL parameters: https://developer.mozilla.org/en/window.location
Attached patch reftests for BMP and ICO encoders (obsolete) (deleted) — — Splinter Review
Implemented review comments on reftest patch
Attachment #548650 - Attachment is obsolete: true
Attachment #550925 - Flags: review+
Fixed an additional comment in reftest.list which had the old example png=yes instead of format=png.
Attachment #550925 - Attachment is obsolete: true
Attachment #550927 - Flags: review+
Comment on attachment 550723 [details] [diff] [review] Patch for working ICO encoder + review comments Review of attachment 550723 [details] [diff] [review]: ----------------------------------------------------------------- Looks good! Some small nits below. ::: modules/libpr0n/encoders/bmp/nsBMPEncoder.cpp @@ +184,5 @@ > +// Returns a pointer to the start of the image buffer > +NS_IMETHODIMP nsBMPEncoder::GetImageBuffer(char **aOutputBuffer) > +{ > + NS_ENSURE_ARG_POINTER(aOutputBuffer); > + *aOutputBuffer = (char*)mImageBufferStart; reinterpret_cast maybe? (For all encoders, do the same thing, anyways; right now we use reinterpret_cast in the ICO encoder and C-style casts everywhere else.) ::: modules/libpr0n/encoders/ico/nsICOEncoder.cpp @@ +150,5 @@ > + PRUint32 imageBufferSize; > + mContainedEncoder->GetImageBufferSize(&imageBufferSize); > + mImageBufferSize = ICONFILEHEADERSIZE + ICODIRENTRYSIZE + > + imageBufferSize; > + mImageBufferStart = static_cast<PRUint8*>(moz_malloc(mImageBufferSize)); new (fallible_t()) PRUint8[mImageBufferSize] might be prettier? I don't know really. @@ +178,5 @@ > + aStride, aInputFormat, params); > + NS_ENSURE_SUCCESS(rv, rv); > + > + PRUint32 andMaskSize = ((mICODirEntry.mWidth + 31) / 32) * 4 * // row AND mask > + mICODirEntry.mHeight; //num rows space between // and num here @@ +205,5 @@ > + imageBufferSize - BFH_LENGTH); > + // We need to fix the BMP height to be *2 for the AND mask > + PRUint32 fixedHeight = mICODirEntry.mHeight * 2; > + fixedHeight = NATIVE32_TO_LITTLE(fixedHeight); > + memcpy(mImageBufferCurr + 8, &fixedHeight, sizeof(fixedHeight)); This bare "+ 8" makes me a little uneasy - what's its derivation? @@ +536,5 @@ > + > + memcpy(mImageBufferCurr, &littleEndianmIDE.mWidth, sizeof(littleEndianmIDE.mWidth)); > + memcpy(mImageBufferCurr + 1, &littleEndianmIDE.mHeight, sizeof(littleEndianmIDE.mHeight)); > + memcpy(mImageBufferCurr + 2, &littleEndianmIDE.mColorCount, sizeof(littleEndianmIDE.mColorCount)); > + memcpy(mImageBufferCurr + 3, &littleEndianmIDE.mReserved, sizeof(littleEndianmIDE.mReserved)); It might be better to have a separate pointer that gets incremented by sizeof(...) every time, so we don't have all these bare numbers inside. (Same goes for EncodeFileHeader.)
Attachment #550723 - Flags: review?(joe) → review+
> It might be better to have a separate pointer that gets incremented by sizeof I had a different one originally but you made me remove it :) I think you mean a local one though and not a member like I had before right?
ah never mind I was thinking of a different spot in code. My bad! :)
Right - a local one. :)
> new (fallible_t()) PRUint8[mImageBufferSize] might be prettier? I tried like that originally but it won't compile that way. The other references do it the same way as me as well in the code base of mozilla-central. (Function cast definitions illegal)
Attached patch Patch for working ICO encoder + review comments (obsolete) (deleted) — — Splinter Review
Implemented latest review comments.
Attachment #550723 - Attachment is obsolete: true
Attachment #553576 - Flags: review?(joe)
(In reply to Brian R. Bondy [:bbondy] from comment #29) > > new (fallible_t()) PRUint8[mImageBufferSize] might be prettier? > > I tried like that originally but it won't compile that way. The other > references do it the same way as me as well in the code base of > mozilla-central. (Function cast definitions illegal) Yeah, it turns out that MSVC has a bug that makes that not work. (Bug 679503 is going to fix that.) In the mean time, you can say something along the lines of static fallible_t fallible = fallible_t(); Block* b = new (fallible) Block(); like in bug 613766 to work around the problem.
The fallible change from static to const is going to be in Bug 670466, so the attached patch is ready for review whenever you get a chance.
Attached patch Patch for working ICO encoder + review comments (obsolete) (deleted) — — Splinter Review
Attachment #553576 - Attachment is obsolete: true
Attachment #553628 - Flags: review?(joe)
Attachment #553576 - Flags: review?(joe)
hold on this one I uploaded wrong patch :)
Attachment #553628 - Attachment is obsolete: true
Attachment #553632 - Flags: review?(joe)
Attachment #553628 - Flags: review?(joe)
Good to go.
Attachment #553632 - Flags: review?(joe) → review+
Depends on: 682201
Keywords: sec-review-needed
Whiteboard: [secr:bsterne]
Whiteboard: [secr:bsterne] → [secr:dveditz]
Whiteboard: [secr:dveditz] → [sec-assigned:dveditz]
Flags: sec-review?(dveditz)
Keywords: sec-review-needed
Flags: sec-review?(dveditz)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: