Closed Bug 843895 Opened 7 years ago Closed 7 years ago

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

Categories

(Core :: ImageLib, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla22

People

(Reporter: seth, Assigned: seth)

References

Details

Attachments

(5 files, 7 obsolete files)

14.35 KB, patch
joe
: review+
Details | Diff | Splinter Review
10.34 KB, patch
joe
: review+
Details | Diff | Splinter Review
5.38 KB, patch
joe
: review+
Details | Diff | Splinter Review
7.18 KB, patch
Details | Diff | Splinter Review
810 bytes, patch
Details | Diff | Splinter Review
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.
Part 1. Allows us to draw an image at either the first frame or the current frame.
Attachment #716860 - Flags: review?(joe)
Attached patch (Part 2) - Add ImageWrapper. (obsolete) — Splinter Review
Part 2. This adds an abstract base class to make implementing Image wrapper classes as painless as possible.
Attachment #716862 - Flags: review?(joe)
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.)
Attachment #716864 - Flags: review?(joe)
Part 4. The grand finale: we replace the usage of ExtractFrame in imgRequestProxy with FrozenImage.
Attachment #716865 - Flags: review?(joe)
There's a try job for this patch stack here:

https://tbpl.mozilla.org/?tree=Try&rev=fc27992a188e
Blocks: 826093
Whoops. Forgot to override FrameRect in FrozenImage. Patch is identical except for that change.
Attachment #716898 - Flags: review?(joe)
Attachment #716864 - Attachment is obsolete: true
Attachment #716864 - Flags: review?(joe)
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.
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
Attachment #717402 - Flags: review?(joe)
Attachment #716865 - Attachment is obsolete: true
Attachment #716865 - Flags: review?(joe)
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.
Went ahead and rebased so I can continue downstream work.
Attachment #718232 - Flags: review?(joe)
Attachment #716860 - Attachment is obsolete: true
Attachment #716860 - Flags: review?(joe)
Rebase.
Attachment #718234 - Flags: review?(joe)
Attachment #716862 - Attachment is obsolete: true
Attachment #716862 - Flags: review?(joe)
Attachment #716898 - Attachment is obsolete: true
Attachment #716898 - Flags: review?(joe)
Attachment #717402 - Attachment is obsolete: true
Attachment #717402 - Flags: review?(joe)
Attachment #718232 - Flags: review?(joe) → review+
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?
Attachment #718234 - Flags: review?(joe) → review+
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.
Attachment #718236 - Flags: review?(joe) → review+
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.
Attachment #718237 - Flags: review?(joe) → review+
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.
(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.
(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.
(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.
Attachment #718236 - Attachment is obsolete: true
OK, just to be sure I'm running another try job through.

https://tbpl.mozilla.org/?tree=Try&rev=a8c6853914eb
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.
OK, I think we're ready for another try job here:

https://tbpl.mozilla.org/?tree=Try&rev=14be4e771bf4
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?
(In reply to neil@parkwaycc.co.uk 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.
I say this because this used to work in Gecko 18... I guess I need to start bisecting.
I take that back, it works with some images but not others. Still testing...
OK, so in Gecko 18, getting the static request succeeds for broken images but in Gecko 19 it fails...
Ignore me, I wasn't calling getStaticRequest in Gecko 18.
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
You need to log in before you can comment on or make changes to this bug.