1343499 - Need a way to query frame sizes in ico files

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, enhancement, P3)

Description

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

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.

Comment 1

[Timothy Nikkel (:tnikkel)]

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.

Comment 2

[Andrew Osmond [:aosmond] (he/him)]

Attachment: Expose native sizes to imagelib clients, v1

Comment 3

[Andrew Osmond [:aosmond] (he/him)]

Attachment: Expose native sizes to imagelib clients, v2

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

Comment 4

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

(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.

Comment 5

[Andrew Osmond [:aosmond] (he/him)]

Attachment: Expose native sizes to imagelib clients, v2

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

Comment 6

[Andrew Osmond [:aosmond] (he/him)]

Attachment: Expose native sizes to imagelib clients, v3

Rebased to build on top of bug 1315554.

Comment 7

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

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 8

[Andrew Osmond [:aosmond] (he/him)]

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 9

[Timothy Nikkel (:tnikkel)]

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?

Comment 10

[Timothy Nikkel (:tnikkel)]

(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.

Comment 11

[Andrew Osmond [:aosmond] (he/him)]

Attachment: Expose native sizes to imagelib clients, v4 [carries r=tnikkel]

(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.

Comment 12

[Andrew Osmond [:aosmond] (he/him)]

Attachment: Expose native sizes to imagelib clients, v5 [carries r=tnikkel]

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

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0b797601dc36
Expose native image sizes to imagelib users. r=tnikkel

Comment 14

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d9f0bdf14469
Backed out changeset 0b797601dc36 for build bustages. r=backout

Comment 15

[Andrew Osmond [:aosmond] (he/him)]

Attachment: Expose native sizes to imagelib clients, v6 [carries r=tnikkel]

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

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/619b5b27ce87
Expose native image sizes to imagelib users. r=tnikkel

Comment 17

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2bd7b9296a2e
Backed out changeset 619b5b27ce87 for CLOSED TREE build bustage r=backout

Comment 18

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or 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

Comment 19

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

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

Comment 20

[Andrew Osmond [:aosmond] (he/him)]

Attachment: Expose native sizes to imagelib clients, v7 [carries r=tnikkel]

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

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ad48fee41646
Expose native image sizes to imagelib users. r=tnikkel

Comment 22

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/ad48fee41646

Comment 23

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

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?

Comment 24

[Andrew Osmond [:aosmond] (he/him)]

(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.)

Comment 25

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

(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);

Comment 26

[Timothy Nikkel (:tnikkel)]

(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.

Comment 27

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Thank you, it works great!

Comment 28

[Andrew Osmond [:aosmond] (he/him)]

(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

Comment 29

[Brindusa Tot, Desktop QA]

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?

Comment 30

[Andrew Osmond [:aosmond] (he/him)]

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.

Comment 31

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

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