1270999 - Fix setting the animation start time of animated images so that it is not before the first frame of the image is available

Status: NEW

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

Description

[Timothy Nikkel (:tnikkel)]

Explanation in the commit message of patch.

Comment 1

[Timothy Nikkel (:tnikkel)]

Attachment: patch

Comment 2

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

Comment on attachment 8749884 [details] [diff] [review]
patch

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

This is horrifying; I'm sure you agree. I'm going to r+, but I did want to ask: have you considered modifying nsImageLoadingContent / ImageLoader to wait until FRAME_COMPLETE before registering the image with the refresh driver? That seems considerably less terrifying to me.

::: image/RasterImage.cpp
@@ +1022,5 @@
>    if (mError || mAnimationMode == kDontAnimMode || mAnimating || !mAnim) {
>      return;
>    }
>  
> +  NS_ASSERTION(GetNumFrames() > 0,

Any reason to prefer NS_ASSERTION over MOZ_ASSERT?

::: layout/base/nsRefreshDriver.cpp
@@ +1223,5 @@
>  
> +bool
> +nsRefreshDriver::AddImageRequest(imgIRequest* aRequest)
> +{
> +  uint32_t status;

Please initialize to 0.

@@ +1231,5 @@
> +  } else {
> +    return AddImageToStartTable(aRequest);
> +  }
> +
> +  return true;

This line is unreachable; I'm surprised you aren't getting try failures due to this. Get rid of this and just remove the |else| above.

::: layout/base/nsRefreshDriver.h
@@ +447,5 @@
> +  CloneWaitingTable mCloneWaitingTable;
> +  // We need to make a clone of the imgIRequest in order to observe
> +  // notifications from it, hence the need for both tables.
> +
> +  bool AddImageToWaitingTable(imgIRequest *aRequest);

imgIRequest*. Same comment for elsewhere in the patch.

Comment 3

[Timothy Nikkel (:tnikkel)]

Yes, I agree it is a gross patch. But it is also a pretty accurate representation of how to use the poor imgIRequest interface that all users of imagelib have to deal with.

I considered just writing code for all the users of animated images, but I initially didn't want to go that route because it meant writing similar code in several places, and it meant more load on any new user of this code to write similar code again. This was before I realized doing it in the refresh driver would be such a pain though. We would need to code in five places I think, see http://mxr.mozilla.org/mozilla-central/search?string=egisterimage . nsImageLoadingContent, nsBulletFrame, ImageLoader, nsTreeBodyFrame, nsImageBoxFrame.

Perhaps I can create an imgIRequestTee class that wraps an imgIRequest and delivers the notifications to a new observer (and the old observer), but doesn't take a stake in the original imgIRequest. It would wrap all this grossness inside a class that lives inside imagelib, and provide a sane interface.

Comment 4

[Timothy Nikkel (:tnikkel)]

Attachment: imgrequesttee

I implemented the imgIRequestTee idea. It does improve the refresh driver code, and keeps the main badness contained in imagelib. Overall it requires more code changes though.

Comment 5

[Timothy Nikkel (:tnikkel)]

Attachment: dontstartanimbeforefirstframebetter

Comment 6

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

Comment on attachment 8751666 [details] [diff] [review]
imgrequesttee

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

I wish we didn't need this. =( We need to rethink this whole design at some point. But I agree, this is easier for client code than the things you need to do now.

::: image/imgIRequestTee.h
@@ +3,5 @@
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +/**
> + * A 'tee' for imgIRequests so that another object can observe it's

Nit: "its"

@@ +18,5 @@
> +  NS_INLINE_DECL_REFCOUNTING(imgIRequestTee)
> +
> +  imgIRequestTee(imgINotificationObserver* aObserver, imgIRequest* aRequest)
> +  {
> +  	aRequest->Clone(aObserver, /* aAsyncNotify */ true, getter_AddRefs(mRequest));

Nit: I'm not sure if splinter is just rendering this badly or what, but it looks like you're using four space tabs. If so, please switch to two space tabs.

::: image/imgRequestProxy.cpp
@@ +670,5 @@
>  
> +void
> +imgRequestProxy::SetRequestTee(imgIRequestTee* aRequestTee)
> +{
> +  mRequestTee = aRequestTee;

We should probably assert that we don't already have one, right?

Comment 7

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

Comment on attachment 8751668 [details] [diff] [review]
dontstartanimbeforefirstframebetter

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

This is indeed nicer with imgIRequestTree. Still gross though. =) But I'll r+.

You never answered me above, so while you have the r+ and I won't hold up the patch over this, I'd just like to hear your thoughts on fixing this by not registering the image with the refresh driver until FRAME_COMPLETE gets delivered. That *seems* to me (having not debugged this problem at all) to be a simpler solution. Would that work, or are there problems there?

::: image/RasterImage.cpp
@@ +1028,5 @@
>    if (mError || mAnimationMode == kDontAnimMode || mAnimating || !mAnim) {
>      return;
>    }
>  
> +  NS_ASSERTION(GetNumFrames() > 0,

MOZ_ASSERT, please.

::: layout/base/nsRefreshDriver.cpp
@@ +2011,5 @@
>      if (NS_SUCCEEDED(req->GetImage(getter_AddRefs(image)))) {
> +#ifdef DEBUG
> +      uint32_t status;
> +      nsresult res = req->GetImageStatus(&status);
> +      NS_ASSERTION(!NS_SUCCEEDED(res) || (status & imgIRequest::STATUS_FRAME_COMPLETE),

MOZ_ASSERT here too.

Comment 8

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

Comment on attachment 8749884 [details] [diff] [review]
patch

Marking the previous patch as obsolete since it seems to be superseded by the new patches.

Comment 9

[Milan Sreckovic [:milan] (needinfo for best results)]

More work before being able to land this?

Comment 10

[Timothy Nikkel (:tnikkel)]

The current patch is still pretty gross. I want to try the other approach mentioned to see if it's better.

Comment 11

[Kenan]

Since here is no progress the last 5 years, could we use the patch provided, even if it is "pretty gross" ?