843895 - Use a wrapper class instead of ExtractFrame for imgRequestProxy::GetStaticRequest

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[Seth Fowler [:seth] [:s2h]]

imgIContainer::ExtractFrame doesn't map well onto the things that it's used for - it does clipping AND freezing of animation, but at every place it's used we only need one of those two things. In addition, it requires us to copy image frames in cases where we really don't need to copy anything. For those reasons, and because a different API would make it much easier and more performant to implement media fragments correctly, I want to replace ExtractFrame.

This bug is about replacing one of the uses of ExtractFrame: imgRequestProxy::GetStaticRequest. Here we only care about freezing the animation of the image, and do not need clipping, so we'll need a wrapper image class that freezes the image.

Comment 1

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 1) - Add an aWhichFrame parameter to imgIContainer::Draw.

Part 1. Allows us to draw an image at either the first frame or the current frame.

Comment 2

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 2) - Add ImageWrapper.

Part 2. This adds an abstract base class to make implementing Image wrapper classes as painless as possible.

Comment 3

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 3) - Add the FrozenImage wrapper class to stop image animation.

Part 3. Add FrozenImage, an Image wrapper class that replaces ExtractFrame's function of stopping the animation of an image. (In cases where we cared about this, it was always stopped at the first frame, so that's what FrozenImage does.)

Comment 4

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 4) - Use FrozenImage instead of ExtractFrame for imgRequestProxy::GetStaticRequest.

Part 4. The grand finale: we replace the usage of ExtractFrame in imgRequestProxy with FrozenImage.

Comment 5

[Seth Fowler [:seth] [:s2h]]

There's a try job for this patch stack here:

https://tbpl.mozilla.org/?tree=Try&rev=fc27992a188e

Comment 6

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 3) - Add the FrozenImage wrapper class to stop image animation.

Whoops. Forgot to override FrameRect in FrozenImage. Patch is identical except for that change.

Comment 7

[Seth Fowler [:seth] [:s2h]]

The try job revealed a bug that I'm pretty sure has a trivial fix. I'll get an updated patch / new try job cooking as soon as I can.

Comment 8

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 4) - Use FrozenImage instead of ExtractFrame for imgRequestProxy::GetStaticRequest.

The test_async_notification_404.js bug is now fixed. Hopefully everything should now be nice and green. Try job here: https://tbpl.mozilla.org/?tree=Try&rev=3f97cdc0c361

Comment 9

[Seth Fowler [:seth] [:s2h]]

This will need a rebase against the new version of the patches in bug 842850 but I'm holding off until all reviews there are done. It should be fine to review the patches here anyway as the differences won't be substantial.

Comment 10

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 1) - Add an aWhichFrame parameter to imgIContainer::Draw.

Went ahead and rebased so I can continue downstream work.

Comment 11

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 2) - Add ImageWrapper.

Rebase.

Comment 12

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 3) - Add the FrozenImage wrapper class to stop image animation.

Rebase.

Comment 13

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 4) - Use FrozenImage instead of ExtractFrame for imgRequestProxy::GetStaticRequest.

Rebase.

Comment 14

[Joe Drew (not getting mail)]

Comment on attachment 718234 [details] [diff] [review]
(Part 2) - Add ImageWrapper.

Review of attachment 718234 [details] [diff] [review]:
-----------------------------------------------------------------

::: image/src/ImageWrapper.cpp
@@ +33,5 @@
> +
> +uint32_t
> +ImageWrapper::SizeOfData()
> +{
> +  return mInnerImage->SizeOfData();

At least one of these should have + sizeof(mInnerImage), right?

Comment 15

[Joe Drew (not getting mail)]

Comment on attachment 718236 [details] [diff] [review]
(Part 3) - Add the FrozenImage wrapper class to stop image animation.

Review of attachment 718236 [details] [diff] [review]:
-----------------------------------------------------------------

::: image/src/FrozenImage.h
@@ +10,5 @@
> +
> +namespace mozilla {
> +namespace image {
> +
> +// PROBLEM: [noscript] ImageContainer getImageContainer(in LayerManager aManager);

maybe this should be removed?

@@ +24,5 @@
> + * XXX(seth): There is one known issue: GetImageContainer does not currently
> + * support anything but the current frame. We work around this by always
> + * returning null, but if it ever turns out that FrozenImage is widely used on
> + * codepaths that can actually benefit from GetImageContainer, it would be a
> + * good idea to fix that method.

I'm torn as to whether this belongs here or in GetImageContainer.

Comment 16

[Joe Drew (not getting mail)]

Comment on attachment 718237 [details] [diff] [review]
(Part 4) - Use FrozenImage instead of ExtractFrame for imgRequestProxy::GetStaticRequest.

Review of attachment 718237 [details] [diff] [review]:
-----------------------------------------------------------------

::: image/src/ImageFactory.h
@@ +51,5 @@
> +   * at the first frame.
> +   *
> +   * @param aImage         The existing image.
> +   */
> +  static already_AddRefed<Image> Freeze(Image* aImage);

I really like the name, but at the same time everything else is CreateFooImage. Torn.

::: image/src/imgRequestProxy.cpp
@@ +920,5 @@
>    // Create a static imgRequestProxy with our new extracted frame.
>    nsCOMPtr<nsIPrincipal> currentPrincipal;
>    GetImagePrincipal(getter_AddRefs(currentPrincipal));
> +  nsRefPtr<imgRequestProxy> req = new imgRequestProxyStatic(frozenImage,
> +                                                            currentPrincipal);

I wonder whether we need imgRequestProxyStatic any more.

Comment 17

[Seth Fowler [:seth] [:s2h]]

Thanks for the reviews, Joe! I'll make the changes you recommend.

> I'm torn as to whether this belongs here or in GetImageContainer.

I was too, as you may have later noticed, since I put a smaller version of that explanation in GetImageContainer too. Maybe the best thing is to put the full version in GetImageContainer and just point you there from the header file comment. It'd be bad if they got out of sync.

> I really like the name, but at the same time everything else is CreateFooImage.

For this and other reasons (namely that I'd like to be able to use this outside of imagelib) I'm actually thinking this belongs as a [noscript, notxpcom] method on imgITools. I'll move it over there. I'll hit you up for a review for that change once it gets made.

Comment 18

[Seth Fowler [:seth] [:s2h]]

(In reply to Joe Drew (:JOEDREW! \o/) from comment #14)
> At least one of these should have + sizeof(mInnerImage), right?

After looking at how RasterImage and VectorImage implement these calls I'm not so sure about this, actually. It doesn't seem like implementation-related small data members like these are counted. Also, this is information used for caching, but right now I see it as unlikely that ImageWrappers will be cached. For now I'll avoid making this change, but please let me know if you disagree with this analysis.

Comment 19

[Seth Fowler [:seth] [:s2h]]

(In reply to Seth Fowler [:seth] from comment #17)
> For this and other reasons (namely that I'd like to be able to use this
> outside of imagelib) I'm actually thinking this belongs as a [noscript,
> notxpcom] method on imgITools. I'll move it over there. I'll hit you up for
> a review for that change once it gets made.

Once I actually tried to do this I realized I was very, very wrong. Forget about this.

Comment 20

[Seth Fowler [:seth] [:s2h]]

(In reply to Joe Drew (:JOEDREW! \o/) from comment #16)
> I wonder whether we need imgRequestProxyStatic any more.

We're moving in the direction of not needing it really fast. We could have probably dropped it a while ago just by moving the principal stuff onto RequestBehaviour, I think.

Comment 21

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 3) - Add the FrozenImage wrapper class to stop image animation.

Applied changes from review.

Comment 22

[Seth Fowler [:seth] [:s2h]]

OK, just to be sure I'm running another try job through.

https://tbpl.mozilla.org/?tree=Try&rev=a8c6853914eb

Comment 23

[Seth Fowler [:seth] [:s2h]]

I'm pretty sure the oranges in that try job are from the patches in bug 846132. I'll post another try job once that bug is ship-shape.

Comment 24

[Seth Fowler [:seth] [:s2h]]

OK, I think we're ready for another try job here:

https://tbpl.mozilla.org/?tree=Try&rev=14be4e771bf4

Comment 25

[Seth Fowler [:seth] [:s2h]]

Try looks OK. Pushed in:

https://hg.mozilla.org/integration/mozilla-inbound/rev/ef39bbfd87e0
https://hg.mozilla.org/integration/mozilla-inbound/rev/6e2d12e2be75
https://hg.mozilla.org/integration/mozilla-inbound/rev/1b269c239a75
https://hg.mozilla.org/integration/mozilla-inbound/rev/f42a8ed7bf2f

Comment 26

[[e-mail]]

Comment on attachment 718237 [details] [diff] [review]
(Part 4) - Use FrozenImage instead of ExtractFrame for imgRequestProxy::GetStaticRequest.

>+  // Check for errors in the image. Callers code rely on GetStaticRequest
>+  // failing in this case
Bah, it turns out that I was relying on it succeeding in this case, I simply hadn't tested on a broken animated image. Where can I find one?

Comment 27

[Seth Fowler [:seth] [:s2h]]

(In reply to [e-mail] from comment #26)
> >+  // Check for errors in the image. Callers code rely on GetStaticRequest
> >+  // failing in this case
> Bah, it turns out that I was relying on it succeeding in this case, I simply
> hadn't tested on a broken animated image. Where can I find one?

We're not really checking for a broken animated image there. We're checking for _any_ case where GetImage() returns non-null, but GetImage()->HasError() returns true. If the image has an error, we won't exit early at the first check because NS_SUCCEEDED(GetAnimated()) will be false. The same scenario would've been caught in the original code (before this patch) by checking the return value of ExtractFrame.

If we don't do this, we break test_async_notification_404.js, FWIW. I'd prefer not to have this check either, honestly.

Comment 28

[[e-mail]]

I say this because this used to work in Gecko 18... I guess I need to start bisecting.

Comment 29

[[e-mail]]

I take that back, it works with some images but not others. Still testing...

Comment 30

[[e-mail]]

OK, so in Gecko 18, getting the static request succeeds for broken images but in Gecko 19 it fails...

Comment 31

[[e-mail]]

Ignore me, I wasn't calling getStaticRequest in Gecko 18.

Comment 32

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/ef39bbfd87e0
https://hg.mozilla.org/mozilla-central/rev/6e2d12e2be75
https://hg.mozilla.org/mozilla-central/rev/1b269c239a75
https://hg.mozilla.org/mozilla-central/rev/f42a8ed7bf2f

Comment 33

[Seth Fowler [:seth] [:s2h]]

Ms2ger pointed out on IRC that FrozenImage::GetAnimated was a bit unclear and the intention could be clarified through the use of a dummy variable. I pushed in this trivial change here:

https://hg.mozilla.org/integration/mozilla-inbound/rev/d64d4a7cf0ea

Comment 34

[Seth Fowler [:seth] [:s2h]]

Attachment: (Followup) - Use a dummy variable in FrozenImage::GetAnimated.

(Here's the patch itself.)

Comment 35

[Ed Morley [:emorley]]

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