Currently the ICO and ICNS decoders and encoders only support a single icon size. We should add support so that more than one size can be handle (perhaps using multiple frames in an imgContainer?).

I'm going to take this bug for later if there are no objections.

This is needed to support the sizes attribute in icon link elements: http://www.whatwg.org/specs/web-apps/current-work/multipage/links.html#rel-icon If websites start providing these larger icons, we can start using them in different parts of the browser front-end, so back-end support for this is definitely wanted!

There are a couple of ways to do this, but the most straightforward is probably to have a separate image cache entry per size. The tricky part will be specifying that size. I think the least-breaking way will be to have a separate URI scheme: moz-image-size:width=80;height=80;uri=http://blah (and then just decode the URI & width/height hints and load as normal, and in the ICO decoder ask the image what size it wants.) The other option is to redecode the size you want at draw time, but I don't recommend that because it can ping-pong decoding. Boris, do you have suggestions on this?

There has been talk in the W3C of using URI fragments to select a particular image out of a multi-resolution image like this. Worth looking whether that's at all stable. If it's not and we're just doing this internally for now, a moz-image-size scheme or whatever should be fine as a stopgap, esp. if we make it not loadable from untrusted content so web sites can't start depending on it.

> There has been talk in the W3C of using URI fragments to select a particular image out of a multi-resolution image I think this? http://www.w3.org/TR/media-frags/ http://www.w3.org/2008/WebVideo/Fragments/ > There are a couple of ways to do this... Wouldn't we store an array of each contained image internally? And add each of its images we would call mImage->EnsureFrame(...)? > the most straightforward is probably to have a separate image cache entry per size I think it would be common to have for example 2 16x16 BMPs inside of an ICO one at 24BPP and another at 4BPP. So maybe the color depth and if each is a PNG or BMP should be additional options in that URI? If not specified use best for size? Possibly using frame index into the ICO instead? That's what a lot of the Win32 APIs that use icons do. (I think Named Dimension in the working draft Media Fragments URI http://__invalidlink__.com/icon.ico#id= encode(moz-icon-image:width=80;height=80;...)).

> I think this? Yes.

(In reply to Brian R. Bondy [:bbondy] from comment #5) > Wouldn't we store an array of each contained image internally? And add each > of its images we would call mImage->EnsureFrame(...)? The problem is that all of RasterImage is built around the assumption that once you have multiple frames, you've got an animated image. > > the most straightforward is probably to have a separate image cache entry per size > > I think it would be common to have for example 2 16x16 BMPs inside of an ICO > one at 24BPP and another at 4BPP. So maybe the color depth and if each is a > PNG or BMP should be additional options in that URI? If not specified use > best for size? Possibly using frame index into the ICO instead? That's > what a lot of the Win32 APIs that use icons do. (I think Named Dimension in > the working draft Media Fragments URI > http://__invalidlink__.com/icon.ico#id= > encode(moz-icon-image:width=80;height=80;...)). Having extra options in the URI sounds great to me.

unassigned myself in case someone else has a chance to jump in

I'm not familiar with this code at all, but can we just do something simple like this? This will just decode the first image it finds that is >= "size" from a url like: file:///path/to/file.ico?size=32 I don't see anything in the media fragments stuff dealing with multi-resolution images other than the "ignore this for multi-resolution images" part. Do we need to propose something there? I can look at the frames stuff, but for most of the current use cases, it seems unlikely that we'll need to decode more than one of these?

Comment on attachment 682389 [details] [diff] [review] WIP Hmm. This would cause a web-visible difference in behavior, right? I'd rather use media fragments instead of query params for that sort of thing, since query params are for server-side processing.

Yes, media fragments make a lot of sense for this. The spec currently doesn't handle this case, though - there's no media fragment dimension to select a resolution. (Adding that support would make it possible to support pixel-based spatial fragments on multiresolution images, which would be nice.) If resolving that would take too long we could certainly add a custom "-moz-resolution" dimension or something along those lines. Unfortunately the approach that is being taken for the "existing" spatial media fragments support (not yet landed; bug 790640) involves a wrapper image class that applies cropping to the actual image at draw time, but right now there's no cropping-like operation that will work to extract a particular size out of an ICO file. If we did things that way, the primary downside would be that it'd involve decoding all of the available sizes. (This could be solved by performing the decoding lazily at draw time, but if we're moving decoding off the main thread this might be a net loss as we'd be putting it back into the critical path by making it lazy.) We could also use an approach where the RasterImage only holds one size, selected at the time it is created. This is similar to the approach taken in the patch, but involving imgRequest is IMO not the right way to go. If we'd like to do things this way, the RasterImage code should be entirely responsible for examining the media fragment portion of the URI and deciding what to do. (In either RasterImage::Init(), or in a factory function like we'll most likely end up with once bug 811129 is finished.)

Thanks! Looks like the parser bits in bug 790640 will make this easy enough, and they're close to ready to land. Personally, I'd rather go with the single image approach rather than holding a bunch of images in memory that we may not need, but I can vaguely see some strange use cases where you might want to flip resolution on hover or something....?

Can't we just parse them lazily instead of keeping them in memory?

@Florian: Assuming you are referring to decoding the images, we certainly can, but as I mentioned in comment 11 that may be a pessimization once we are decoding off the main thread. @Wes: I concur about decoding only a single image being preferable. For favicons I don't know that we'd ever have a need to decode multiple sizes unless something in the UI changes. On the other hand, having all the sizes around makes a bit more sense for images embedded in the page, since at least on mobile people are zooming in and out with some frequency and we might want to use different versions of the image at different zoom levels. Is there any multiresolution image format that people actually use on the page itself, though? Seems like people use media queries or image-set for this right now, so I guess there's no point thinking about doing anything clever here.

(In reply to Seth Fowler [:seth] from comment #14) > @Wes: I concur about decoding only a single image being preferable. For > favicons I don't know that we'd ever have a need to decode multiple sizes > unless something in the UI changes. On desktop we already plan to use 32px icons in awesomebar results with 16px favicons in the addressbar itself. (bug 768703)

Hmm, that's a good point. (And presumably on "retina" displays we may want 32px for the address bar and 64px for the results?) Sounds like however we structure the rest of the system, we at least need support for RasterImages that hold more than one resolution. One thing I forgot to mention regarding lazy image decoding is that I think once drawing and decoding are happening on different threads it's possible that our hands will be tied, because currently we don't build e.g. Cairo in a threadsafe manner, and so we couldn't safely decode on draw, pessimization or not.

If you have an ICO with a 16px, 32px and 64px size (as per Comment 16), then you'll need to keep 5376px of image data (plus meta info) in memory, even if you'll ever need 1280px (sometimes you only ever need 256px) of image data (non-retina case). That's more than four times the image data, and only for *one* ICO file (plus most ICOs have 256px or 512px sizes). Now multiply that with every ICO or even other multi-resolution image containers … what about memshrink? There needs be a way to determine which part/size of the image we need before we actually decode it. E. g. make the CSS parser report which resolutions it encounters in any case and only keep those resolutions. Is that a viable way?

Florian, I think we're in violent agreement. =) To make things maximally flexible for later optimizations, I think we need to determine the resolution of the image we need / would like at the time that we request it, which is a stricter condition than determining it before decoding. There should be no problem doing this; we're implicitly assuming that we can do it when we discuss solutions involving media fragments or custom URI schemes that select a resolution. At this point the only thing that isn't clear is how we'll set things up at a low level in imagelib. After giving it some thought I'm not sure that the simplest approach is to store all of the resolutions in the same RasterImage; it's probably better to just have multiple RasterImages which share the same (immutable) undecoded data. This means that we can keep RasterImages largely or entirely the same, instead of adding an additional layer of complexity around the management of multiple resolutions.

Alright, good to hear :)

This is just the same as my first patch, but using media fragments stuff instead. This requires the first r+ patch in bug 790640. I'm still getting familiar with this I'm not sure this does what we want and shares the downloaded media, but it works fine for the use cases I've seen put forward.

Feedback ping? I can rebase this to not need 790640 if it helps?

Sorry for taking so long Wes. I'm straightening out some issues with the new version of the code in bug 790640 right now. I haven't offered feedback yet because I need to see how your code will fit in when it's ported over to the changes that are happening there. You're welcome to rebase your changes if you'd prefer to get them in more quickly, though I don't think the delay is likely to be that long now.

Since these refactor bits were taken out of bug 790640, I figured I'd resurface them here.

This implements the moz-resolution media fragment stuff. I need to write some reftests for this.

I think we also probably want to do something to pick an appropriate resolution from images like these when none is specified. i.e. <img width=32 height=32 src=myico.cio> should pull out a 32x32 icon (or something close). I've tried a few different pieces to see if I ever have this info. This doesn't work, but I was wondering if you could give me any pointers on a better place to look seth?

Comment on attachment 721514 [details] [diff] [review] Patch 1/2 Review of attachment 721514 [details] [diff] [review]: ----------------------------------------------------------------- This needs to be in a separate bug. I've created a new bug 849114 to take care of that. (It includes a slightly different, more up-to-date version of this patch.) We'll get that in ASAP. You should rebase your second patch on top of that one. (But hold off until I give you my review comments.)

Comment on attachment 721516 [details] [diff] [review] Bonus patch Review of attachment 721516 [details] [diff] [review]: ----------------------------------------------------------------- ::: image/src/RasterImage.cpp @@ +3131,5 @@ > > mFrameDecodeFlags = DECODE_FLAGS_DEFAULT; > } > > + mFill = aFill; This approach won't work, as you mention. By the time you get to the point where you're drawing, decoding will already be done. The right starting point is nsImageLoadingContent, I think. (There may be other places to consider - for example, CSS background images use mozilla::css::ImageLoader IIRC.) You'd need to determine when the image is sitting in a container that has a fixed size in terms of CSS. To be honest, though, CSS already has support for this kind of thing via other mechanisms (image-set), and there are very few image formats that support multiple resolutions in a single file. It's almost certainly not worth the effort.

Comment on attachment 721515 [details] [diff] [review] Patch 2/2 Review of attachment 721515 [details] [diff] [review]: ----------------------------------------------------------------- This is a good start, Wes! I'd like to see it again after it's rebased and the comments below have been addressed. ::: content/html/content/src/nsMediaFragmentURIParser.cpp @@ +382,5 @@ > if (!gotTemporal && fragments[i - 1].first.EqualsLiteral("t")) { > gotTemporal = ParseNPT(utf16Value); > } else if (!gotSpatial && fragments[i - 1].first.EqualsLiteral("xywh")) { > gotSpatial = ParseXYWH(utf16Value); > + } else if (!gotResolution && fragments[i - 1].first.EqualsLiteral("moz-resolution")) { If we're doing this like CSS does this should probably actually be "-moz-resolution". ::: content/html/content/src/nsMediaFragmentURIParser.h @@ +58,5 @@ > // returns the unit used - either pixels or percents. > ClipUnit GetClipUnit() const { return mClipUnit; } > > + nsIntSize GetResolution() const { return mResolution; } > + bool HasResolution() const { return mHasResolution; } Comment these like the other methods on this class. ::: image/decoders/nsICODecoder.cpp @@ +257,5 @@ > uint16_t colorDepth = 0; > + nsIntSize prefSize; > + mImage.GetRequestedResolution(&prefSize); > + if (prefSize.width == 0 && prefSize.height == 0) > + { Place this brace on the same line as the if statement; that's the imagelib style. (I know some code doesn't follow it, but we shouldn't add new code like that.) @@ +259,5 @@ > + mImage.GetRequestedResolution(&prefSize); > + if (prefSize.width == 0 && prefSize.height == 0) > + { > + prefSize.SizeTo(PREFICONSIZE,PREFICONSIZE); > + } Add a blank line here to offset this from the large block of code that follows. @@ +292,5 @@ > + int32_t deltaX = e.mWidth - prefSize.width; > + int32_t deltaY = e.mHeight - prefSize.height; > + if ((e.mBitCount >= colorDepth && > + deltaX >= 0 && deltaX < diffX && > + deltaY >= 0 && deltaY < diffY) || You dropped the mapping from |e.mWidth = 0| to |e.mWidth = 256| (and similar for |e.mHeight|). That is part of the definition of the .ICO format. You can't remove that. Also, I'd prefer that you use only one |delta| term here, and abstract the calculation into a function. Defining it in terms of the |deltaX| and |deltaY| values you have here, I'm thinking something along the lines that |delta| is INT_MAX if |deltaX < 0 || deltaY < 0|, and otherwise |delta| is |deltaX + deltaY|. With only one value you can make this code cleaner and match the requested resolution more closely to boot. Make sure you update the calculation to include the mapping from |e.mWidth = 0| to |e.mWidth = 256|, though, or it won't work right! ::: image/src/Makefile.in @@ +50,5 @@ > # Because SVGDocumentWrapper.cpp includes "mozilla/dom/SVGSVGElement.h" > LOCAL_INCLUDES += \ > -I$(topsrcdir)/content/svg/content/src \ > -I$(topsrcdir)/content/base/src \ > + -I$(topsrcdir)/content/html/content/src \ This is moved in my updated version of the media fragments parsing patch, so update this. But also, don't add this in this block of LOCAL_INCLUDES. It makes the comment above the block false. Move it to its own line and add a comment explaining the purpose of the include. (similar to what's happening on line 57 and 58 of this file) ::: image/src/RasterImage.cpp @@ +516,5 @@ > + if (mDecoder && parser.HasResolution()) { > + nsIntSize resolution = parser.GetResolution(); > + SetRequestedResolution(resolution); > + } > + } Move this to ImageFactory::CreateRasterImage. You'll have an nsIURI there, so you won't need this stuff involving the IO service, which will make the code a lot cleaner. Also, just write "newImage->SetRequestedResolution(parser.GetResolution())". ::: image/src/RasterImage.h @@ +269,5 @@ > nsresult SetSourceSizeHint(uint32_t sizeHint); > > + nsresult SetRequestedResolution(nsIntSize requestedResolution); > + > + nsresult GetRequestedResolution(nsIntSize *requestedResolution); Get rid of the blank line between these two methods, and document them with a comment as a group. SetRequestedResolution should be declared as "void SetRequestedResolution(const nsIntSize aRequestedResolution)". GetRequestedResolution should be declared as "nsIntSize GetRequestedResolution()"; after all, these methods can't fail, so there's no need for a result code. Both of their definitions are trivial so just define them here in the header and remove their definitions from the .cpp file. @@ +619,5 @@ > }; > NS_IMETHOD RequestDecodeCore(RequestDecodeType aDecodeType); > > private: // data > + nsIntSize mRequestedResolution; I think this is more logically grouped with the decoder-related fields, since the decoder is what actually consumes this value. Place it below the declaration of mDecodeCount and add a comment explaining what this field means.

Note that in bug 849114 some changes ended up getting made to the the parser patch, so be sure you have the latest version when rebasing.

Updated to trunk. I also modified this to return the image whose resolution is closest to the requested one as well. Since consumers (mainly favicons in our chrome) may not know exactly what resolutions are in the container, they can say "I'd really like 64x64" and if there's something 60x60, we'll give them that. Added some simple reftests as well.

Wouldn't it be better to always pick the next size up? Slight downscaling tends to look better than slight upscaling.

(In reply to Greg Edwards from comment #31) > Wouldn't it be better to always pick the next size up? Slight downscaling > tends to look better than slight upscaling. FWIW I am inclined to agree.

Comment on attachment 727462 [details] [diff] [review] Patch Review of attachment 727462 [details] [diff] [review]: ----------------------------------------------------------------- Looks good Wes! ::: netwerk/base/src/nsMediaFragmentURIParser.h @@ +53,5 @@ > + // True if a valid spatial media fragment indicated a resolution. > + bool HasResolution() const { return !mResolution.empty(); } > + > + // True if a valid spatial media fragment indicated a resolution. > + nsIntSize GetResolution() const { return mResolution.ref(); } Should phrase this to describe what GetResolution returns. Looks like this accidentally got the same comment as HasResolution.

Try did not like me. Mostly because of this color depth thing. With the new patch if there were two images in a container with the same size but different color depth's, we took whichever we found first. This modifies us to favor the better depth matching our old behavior. That also means that for one test, where there are two icons in the file that are 4px away from the requested size, we now pick whichever is later in the container. I also started to update the code to choose downscaling over upscaling, but decided it was more complex than I wanted to write here. (i.e. I think we'd want to favor slight upscales over big downscales).

This looked good on try: https://tbpl.mozilla.org/?tree=Try&rev=a863f2626d8d

Slight upscale vs. big downscale depends a lot on how the icon was authored. If the small image is already just a downscale of the big one, picking the big one should unquestionably be better. In other situations where the small icon has been grid fitted and stroke widths adjusted, picking the small one could be better. Colour depth should only be looked at if the resolution is a tie. (I think this is what you're saying anyway?)

I really don't think we should try to do anything complicated without clear evidence of problems from a simpler approach. Unless we find actual cases in the wild where it produces bad results, IMO the best bet is just to favor downscales.

Minor followup patch for nsMediaFragmentURIParser::Parse. It looks like the original patch here forgot to add gotResolution to the test for whether all possible types have been found. And while we're here, I'd suggest making the if() statements independent, each ending with a continue, instead of the else-if chaining; it seems cleaner and simpler to read that way.

Comment on attachment 728978 [details] [diff] [review] followup - clean up the media fragment parsing loop Review of attachment 728978 [details] [diff] [review]: ----------------------------------------------------------------- r+ on the fix for the check at the top of the loop only. I don't agree that the switch to using continue is an improvement. However, I overall don't like the structure of this loop code, so if you can refactor it _more_ and make it nicer and less prones to mistakes like the one you're fixing in this patch, I'd totally r+ that in a hurry. If you want to, though, do that in a separate patch and/or bug so we can land this fix.

OK, as you're not keen on the "continue" version, I pushed this with only the fix to the loop-termination check. Any more radical restructuring should be left for a separate bug, I think, rather than added to this already-resolved one. https://hg.mozilla.org/integration/mozilla-inbound/rev/ce60918f4e48

Why was this exposed to the web at large?

We originally thought it might be useful to webdevs and were going to push for it. Without it, Gecko doesn't provide a way to get anything but the first image from multi-resolution images (in chrome or content). Ideally I think we'd look at where an image was being drawn and decode the right one for that size, but we don't have any information like this in the decoder right now. If we had that, its debatable whether its useful for a developer to be able to explicitly select a certain resolution from a file. If you want to block it from content, I don't think anyone will put up a fight.

(In reply to Wesley Johnston (:wesj) from comment #45) > We originally thought it might be useful to webdevs and were going to push > for it. Without it, Gecko doesn't provide a way to get anything but the > first image from multi-resolution images (in chrome or content). That's fair. But as you know, .ico files are not very commonly used on the web, so perhaps we should wait for authors to request this feature. > Ideally I think we'd look at where an image was being drawn and decode the > right one for that size, but we don't have any information like this in the > decoder right now. If we had that, its debatable whether its useful for a > developer to be able to explicitly select a certain resolution from a file. Agreed. > If you want to block it from content, I don't think anyone will put up a > fight. OK. :-) It's not so much that I have anything in particular against this feature, but I think if we think it's worth solving then we should go through the standardization process and get feedback from other browser vendors to come up with a solution that is usable across different engines. Until we do that, it's harmful to expose a feature to the web. I filed bug 972134 to make this only available for chrome. Hope this makes sense.

Web devs should never have to deal with resolution fragments ever. The only use I could see exposing this to the web would be to support a JS shim library to emulate Bug 888823.

(In reply to :Ehsan Akhgari (needinfo? me!) (slow responsiveness, emailapocalypse) from comment #46) > (In reply to Wesley Johnston (:wesj) from comment #45) > > We originally thought it might be useful to webdevs and were going to push > > for it. Without it, Gecko doesn't provide a way to get anything but the > > first image from multi-resolution images (in chrome or content). > > That's fair. But as you know, .ico files are not very commonly used on the > web, so perhaps we should wait for authors to request this feature. I believe they are used quite a lot these days to allow for the delivery of the correct resolution favicon to retina displays and others. A prominent example of a site using .ico files: http://www.apple.com/favicon.ico

(In reply to comment #48) > (In reply to :Ehsan Akhgari (needinfo? me!) (slow responsiveness, > emailapocalypse) from comment #46) > > (In reply to Wesley Johnston (:wesj) from comment #45) > > > We originally thought it might be useful to webdevs and were going to push > > > for it. Without it, Gecko doesn't provide a way to get anything but the > > > first image from multi-resolution images (in chrome or content). > > > > That's fair. But as you know, .ico files are not very commonly used on the > > web, so perhaps we should wait for authors to request this feature. > > I believe they are used quite a lot these days to allow for the delivery of the > correct resolution favicon to retina displays and others. A prominent example > of a site using .ico files: > > http://www.apple.com/favicon.ico Favicon handling is quite different to what we're talking about here.

(In reply to :Ehsan Akhgari (needinfo? me!) (slow responsiveness, emailapocalypse) from comment #49) > Favicon handling is quite different to what we're talking about here. :( Thats not entirely true. We use this in our XUL UI for Favicons. That was the main impetus for adding it. i.e. Favicon handling is the exact reason for this feature. They aren't often seen in content but it does happen. For instance DuckDuckGo has a handy feature to show favicon next to search results (making their listings generally easier to parse than Google's), and they do use (cached versions of) the ico files from the pages themselves.

(In reply to comment #50) > (In reply to :Ehsan Akhgari (needinfo? me!) (slow responsiveness, > emailapocalypse) from comment #49) > > Favicon handling is quite different to what we're talking about here. > > :( Thats not entirely true. We use this in our XUL UI for Favicons. That was > the main impetus for adding it. i.e. Favicon handling is the exact reason for > this feature. > > They aren't often seen in content but it does happen. For instance DuckDuckGo > has a handy feature to show favicon next to search results (making their > listings generally easier to parse than Google's), and they do use (cached > versions of) the ico files from the pages themselves. Oh :/ Well, that's sad. But good to know. I stand corrected.