For hi-res favicons support, I need a way to ask to an ico file the size of the contained frames. Currently I need that on and off the main thread, in the future I'll try to refactor the code to always act off the main thread. On the main thread I have an imgIContainer generated by DecodeImageData. Off the main thread I have a SourceSurface obtained by DecodeToSurface.

For the off main thread case we could provide another ImageOps function that returned an array of available sizes. Then we would change DecodeToSurface to accept a Maybe<Size> argument to specify the size. So you'd make two calls, one to get the available sizes, one to get the SourceSurface. For the on the main thread case we could pipe the same info through to the imgIContainer interface, but it's grosser because it's an idl interface. So if we could avoid doing that it would be great. As for implementing this I imagine we would put an array on the Decoder and have the ico decoder fill in the array. The new ImageOps function would operate like DecodeToSurface except it would create an anonymous metadata decoder, and then pull this array from the decoder (or just use the regular image size if there is no array) instead of the decoded frame.

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=1365b6338083283d620aa17ab83144097bafe273

(In reply to Timothy Nikkel (:tnikkel) from comment #1) > For the on the main thread case we could pipe the same info through to the > imgIContainer interface, but it's grosser because it's an idl interface. So > if we could avoid doing that it would be great. It's in my follow-up plans to move all the decoding off the main thread, so if it's hard or ugly to add a method to imgIContainer I can re-prioritize things to do that first. Just let me know what you decide.

Add ImageOps::CreateImageBuffer to allow for more efficient execution if multiple DecodeMetadata/DecodeToSurface calls are made on the same input stream. try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=eb51cc98e787f4ddfee1f83e51c9432233a03f13

Rebased to build on top of bug 1315554.

I'm proceeding with my work, I'm almost at a point where I must decide whether I will land with or without proper ico files support (without it, for each ico I will just store the biggest frame). I'd like to understand the status of this feature, since I see the review request has been cleared.

Comment on attachment 8843281 [details] [diff] [review] Expose native sizes to imagelib clients, v2 (In reply to Marco Bonardo [::mak] from comment #7) > I'm proceeding with my work, I'm almost at a point where I must decide > whether I will land with or without proper ico files support (without it, > for each ico I will just store the biggest frame). > > I'd like to understand the status of this feature, since I see the review > request has been cleared. My bad, my original thinking was to land bug 1315554 first since they both touch the same code and this bug is less complex, hence the rebased patch. But since it is taking some time to get the reviews done (there are more than a few outstanding :)), I'll just land it in either order.

Comment on attachment 8843281 [details] [diff] [review] Expose native sizes to imagelib clients, v2 Do we need NS_IMETHOD/NS_IMETHODIMP when implementing GetNativeSizes? I think we can just do virtual nsresult since it's C++ and not xpcom. In OrientedImage::GetNativeSizes you need to swap the width and height only if mOrientation.SwapsWidthAndHeight(). And I think you're going to need to have a new member variable in OrientedImage to store the native sizes array with width and height swapped because otherwise it looks like you swap the width and height of the native sizes array in the inner image? >+/* static */ nsresult >+ImageOps::DecodeMetadata(ImageBuffer* aBuffer, >+ const nsACString& aMimeType, >+ ImageMetadata& aMetadata) >+{ >+ DecoderFactory::CreateAnonymousMetadataDecoder(decoderType, >+ WrapNotNull(sourceBuffer)); >+ if (!decoder) { >+ return NS_ERROR_FAILURE; >+ } >+ >+ // Run the decoder synchronously. >+ RefPtr<IDecodingTask> task = new AnonymousDecodingTask(WrapNotNull(decoder)); Doesn't this need to be an anoynmous metadata decoding task? I'm surprised this worked. Is it possible to add some asserts or something that we pair the right kind of decoding task with the right kind of decoder?

(In reply to Timothy Nikkel (:tnikkel) from comment #9) > Comment on attachment 8843281 [details] [diff] [review] > Expose native sizes to imagelib clients, v2 > > Do we need NS_IMETHOD/NS_IMETHODIMP when implementing GetNativeSizes? I > think we can just do virtual nsresult since it's C++ and not xpcom. > > In OrientedImage::GetNativeSizes you need to swap the width and height only > if mOrientation.SwapsWidthAndHeight(). And I think you're going to need to > have a new member variable in OrientedImage to store the native sizes array > with width and height swapped because otherwise it looks like you swap the > width and height of the native sizes array in the inner image? > > > >+/* static */ nsresult > >+ImageOps::DecodeMetadata(ImageBuffer* aBuffer, > >+ const nsACString& aMimeType, > >+ ImageMetadata& aMetadata) > >+{ > > >+ DecoderFactory::CreateAnonymousMetadataDecoder(decoderType, > >+ WrapNotNull(sourceBuffer)); > >+ if (!decoder) { > >+ return NS_ERROR_FAILURE; > >+ } > >+ > >+ // Run the decoder synchronously. > >+ RefPtr<IDecodingTask> task = new AnonymousDecodingTask(WrapNotNull(decoder)); > > Doesn't this need to be an anoynmous metadata decoding task? I'm surprised > this worked. Is it possible to add some asserts or something that we pair > the right kind of decoding task with the right kind of decoder? Oops, sorry, we don't have AnonymousMetadataDecodingTask, AnonymousDecodingTask is correct. Forget this comment.

(In reply to Timothy Nikkel (:tnikkel) from comment #9) > Comment on attachment 8843281 [details] [diff] [review] > Expose native sizes to imagelib clients, v2 > > Do we need NS_IMETHOD/NS_IMETHODIMP when implementing GetNativeSizes? I > think we can just do virtual nsresult since it's C++ and not xpcom. > Done. > In OrientedImage::GetNativeSizes you need to swap the width and height only > if mOrientation.SwapsWidthAndHeight(). And I think you're going to need to > have a new member variable in OrientedImage to store the native sizes array > with width and height swapped because otherwise it looks like you swap the > width and height of the native sizes array in the inner image? > Nice catch, I fixed this to check the orientation. I also amended the test case to add a new mismatched height/width resource to the ICO, and to verify GetNativeSizes after using ImageOps::Orient on 90 and 180. RasterImage::GetNativeSizes itself does a deep copy of RasterImage::mNativeSizes into the caller supplied array reference, and it is the caller array which we do the swap. So it should be safe.

Update DecodeToSurface test case for new sizes added to the ICO.

Pushed by aosmond@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/0b797601dc36 Expose native image sizes to imagelib users. r=tnikkel

Backout by aosmond@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/d9f0bdf14469 Backed out changeset 0b797601dc36 for build bustages. r=backout

Apparently gcc 5.4.0 (local) accepts things gcc 4.9.4 (treeherder) and clang do not. Confirmed build bustages fixed on Linux: try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e46a64568f630f014db4891c724ad1e4f442434c

Pushed by aosmond@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/619b5b27ce87 Expose native image sizes to imagelib users. r=tnikkel

Backout by aosmond@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/2bd7b9296a2e Backed out changeset 619b5b27ce87 for CLOSED TREE build bustage r=backout

Please check if this crash on Android is caused by this patch: https://treeherder.mozilla.org/logviewer.html#?job_id=85652574&repo=mozilla-inbound

More of these errors popped up, so these are indeed crashes from this bug.

Fix the remaining build issues, just a few nits (add explicit here, etc). Fix the mochitest crash -- turns out the C++ only methods need to be at the end of the IDL declarations. try (everything, let's avoid a repeat of this morning...): https://treeherder.mozilla.org/#/jobs?repo=try&revision=2f388c87ab9b0ccc71b3412845a091adbd9d047c

Pushed by aosmond@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/ad48fee41646 Expose native image sizes to imagelib users. r=tnikkel

Thank you for this, I'm starting using it. Sorry if these are dumb questions, but I need a couple additional details about this: 1. are the sizes returned by GetNativeSizes in appUnits or px? 2. provided I have a decoded ico, will EncodeScaledImage pick automatically the best frame out of the ico, to satisfy my size requirements, or should I decode a specific frame first?

(In reply to Marco Bonardo [::mak] from comment #23) > Thank you for this, I'm starting using it. > > Sorry if these are dumb questions, but I need a couple additional details > about this: > 1. are the sizes returned by GetNativeSizes in appUnits or px? Pixels. Same as imgIContainer::GetWidth and GetHeight. > 2. provided I have a decoded ico, will EncodeScaledImage pick automatically > the best frame out of the ico, to satisfy my size requirements, or should I > decode a specific frame first? Currently imgTools::EncodeScaledImage will prefer a best match to the requested size from the cache (even if it is the wrong size) over decoding for the specific size at its native resolution (because FLAG_HIGH_QUALITY_SCALING is used in the imgIContainer::GetFrameAtSize call in EncodeScaledImage). You should be able to force it to use the native resource for that size by calling GetFrameAtSize beforehand. (I'm happy to review any related patches for you.)

(In reply to Andrew Osmond [:aosmond] from comment #24) > You should be able to force it to use the native > resource for that size by calling GetFrameAtSize beforehand. (I'm happy to > review any related patches for you.) I tested with an icon made up of 3 completely different frames, and imgTools::EncodeScaledImage returned the exact frame I was expecting in every case, without the need for calling GetFrameAtSize first. So EncodeScaledImage with 32px, returned the 32x32 frame, and so on for 16 and 64. Was that by luck? I'm not sure whether I should still try to call GetFrameAtSize in my code, and I'm also not sure which arguments it'd need, it seems to allow selecting only FIRST or CURRENT frame (what is current? sounds like something for animated images?), and requires some flags (I suppose I only want FLAG_SYNC_DECODE?). So this is what I added before EncodeScaledImage, but doesn't seem to make any difference, it works even without it: RefPtr<gfx::SourceSurface> frame = container->GetFrameAtSize( size, imgIContainer::FRAME_FIRST, imgIContainer::FLAG_SYNC_DECODE);

(In reply to Marco Bonardo [::mak] from comment #25) > I tested with an icon made up of 3 completely different frames, and > imgTools::EncodeScaledImage returned the exact frame I was expecting in > every case, without the need for calling GetFrameAtSize first. So > EncodeScaledImage with 32px, returned the 32x32 frame, and so on for 16 and > 64. Was that by luck? EncodeScaledImage asks for a sync decode so you will get the frame at the size you ask for without having to call GetFrameAtSize. > I'm not sure whether I should still try to call GetFrameAtSize in my code, > and I'm also not sure which arguments it'd need, it seems to allow selecting > only FIRST or CURRENT frame (what is current? sounds like something for > animated images?), Yes, it's for frames of animated images. The things that ico files can hold more than one of are better called resources or images.

Thank you, it works great!

(In reply to Timothy Nikkel (:tnikkel) from comment #26) > (In reply to Marco Bonardo [::mak] from comment #25) > > I tested with an icon made up of 3 completely different frames, and > > imgTools::EncodeScaledImage returned the exact frame I was expecting in > > every case, without the need for calling GetFrameAtSize first. So > > EncodeScaledImage with 32px, returned the 32x32 frame, and so on for 16 and > > 64. Was that by luck? > > EncodeScaledImage asks for a sync decode so you will get the frame at the > size you ask for without having to call GetFrameAtSize. > Okay, yesterday was the day I went blind clearly, as I missed the || here: http://searchfox.org/mozilla-central/source/image/RasterImage.cpp#344

We saw in wiki page (https://wiki.mozilla.org/Features/Release_Tracking#Targeting_Firefox_55) that this is targeting Firefox 55. Does this need manual testing? If yes, could you provide some suggestions? Marco, is this related to implementation for bug 977177?

This particular bug does not require manual testing, because none of the changes are exposed outside of imagelib right now, so there isn't any way to trigger it. However the gtests I added verify the new functionality.

I am also adding a test in bug 977177 for this functionality, so I agree, no need for manual testing.