890743 - GIF rendering: Firefox overrides frame delay values of 0ms of multi-frame GIFs even for safe cases (loop count == 1)

Status: VERIFIED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[yester129]

Attachment: save multiframe gif with delay 0ms between all frames and loop count 1.

User Agent: Mozilla/5.0 (Windows NT 5.1; rv:22.0) Gecko/20100101 Firefox/22.0 (Beta/Release)
Build ID: 20130618035212

Steps to reproduce:

Case 1: Rendering of this multi-frame GIF (tiled) http://phil.ipal.org/tc217.gif
Case 2: Rendering of this multi-frame GIF (overlayed) http://www.peda.com/iag/gif/hk.gif

Both in firefox 22.0 (windows XP)



Actual results:

Case 1: tc217.gif is unreasonable slow rendered while it should be rendered nearly instantly (all between frame delays are set to 0ms).
Case 2: Also hk.gif has a visible delay in rendering when it should be rendered near instantly.



Expected results:

Multi-frame gifs, which are specification conform (http://www.w3.org/Graphics/GIF/spec-gif87.txt), can be used for two use-cases: animated GIFs and tiled images (creating a image consisting out of several sub images). While first use case is far more often, second use case is completely valid and inside the GIF specification and should be supported when reasonable possible. 

I'm aware that for compatibility reasons between browsers (http://humpy77.deviantart.com/journal/Frame-Delay-Times-for-Animated-GIFs-240992090) and for plugging potential CPU cycle "leaks" workarounds and guesses were implemented for GIFs which have delay==0 and define no loop iteration count or have loop count==0 (infinite). (former discussions https://bugzilla.mozilla.org/show_bug.cgi?id=71829, https://bugzilla.mozilla.org/show_bug.cgi?id=149205 and more)

I suggest to implement this (https://bugzilla.mozilla.org/show_bug.cgi?id=149205#c14) reasonable suggestion from a former bug report to detect which GIF is a animation and which is a multi-frame GIF by checking for a explicitly set loop count == 1 and than respect delay==0ms (or whatever delay is set). Typically, misconfigured GIFs have neither loop count nor delay set, so this assumption should be reasonable. Even if wrong, at least no severe CPU ressource leak is opened as the loop count is only 1.

Topic solved: 
The attached multi-frame GIF, in which all frames are set to 0 ms delay, but loop count is set to 1, should be rendered near instantly. 

Also, this Wikipedia page trying to present multi-frame GIFs should be rendered correctly: https://de.wikipedia.org/wiki/Graphics_Interchange_Format 2 multi-frame images on the page.

Comment 1

[yester129]

another test case on wikipedia http://en.wikipedia.org/wiki/File:Juncker_2010-GIF-Truecolor.gif

Comment 2

[Joe Drew (not getting mail)]

Hm. I agree!

This should be reasonably simple for a new contributor to do. We actually store the correct timeout in imgFrame, but imgFrame::GetTimeout() overrides the timeout in the cases mentioned in comment 0.

Fixing this will probably mean putting the timeout-overriding code in somewhere other than imgFrame (I suggest FrameAnimator), because it'll need to know how many loops the image has. Be careful, though, because you'll probably need to change other users of GetTimeout() to call this new FrameAnimator::GetFrameTimeout()!

Comment 3

[Alexandre BM (:rednaks)]

So you propose to use FrameAnimator instead of imgFrame ? also moving mTimeout to FrameAnimator ?

Comment 4

[Joe Drew (not getting mail)]

I propose that RasterImage uses a method on FrameAnimator instead of imgFrame, yes, but mTimeout will stay in imgFrame, as that's the "storage" class for the frame.

Comment 5

[Alexandre BM (:rednaks)]

Attachment: bug890743.patch

Okey, I have another question, how can I get the imgFrame ? I see that there is mFrameBlender that can return a imgFrame but I have to provide and index. I passed mCurrentAnimationFrameIndex but there is something wrong, for example to compute the looptime we need to get all frames timeout ... Am I in the right direction or I'm wrong ?

Comment 6

[Josh Matthews [:jdm]]

Seth, is this something you could take over mentoring?

Comment 7

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

Yes, I'd be happy to. Alexandre, I apologize that it's taken so long to get feedback on this. I'll familiarize myself with the patch tomorrow and get back to you then.

Comment 8

[Alexandre BM (:rednaks)]

It's okey :), I heard about the news ...

Comment 9

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

(In reply to Alexandre BM (:rednaks) from comment #5)
> Created attachment 785540 [details] [diff] [review]
> bug890743.patch
> 
> Okey, I have another question, how can I get the imgFrame ? I see that there
> is mFrameBlender that can return a imgFrame but I have to provide and index.
> I passed mCurrentAnimationFrameIndex but there is something wrong, for
> example to compute the looptime we need to get all frames timeout ... Am I
> in the right direction or I'm wrong ?

You could iterate over the frames using a loop like the one in FrameAnimator::GetSingleLoopTime. That's not what you want here, though. FrameAnimator already has an mLoopCount field, but it's not what you want either, since it represents the number of frames remaining.

Your best bet is probably to add a new boolean field -- say "mIsSingleLoop"? -- and remember whether the image only loops once in FrameAnimator::SetLoopCount, where the initial loop count gets set.

Comment 10

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

(In reply to Seth Fowler [:seth] from comment #9)
> You could iterate over the frames using a loop like the one in
> FrameAnimator::GetSingleLoopTime. That's not what you want here, though.

To clarify, you don't want this because the individual frames don't have the information you need -- you need information about the overall loop count to know what to do. Once you have that information, you can apply it everywhere that imgFrame::GetTimeout gets called.

Comment 11

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

One other small piece of feedback: it's a good habit to get into to set a commit message on your patches, right from the beginning. Let me know if it's not clear what that means.

Also, please feel free to find me on IRC (I'm "seth") if there's anything I can help you with.

Comment 12

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

Bug 899861 might affect you, Alexandre, since you're using FrameAnimator here. If you haven't pulled already it might be easiest to just hold off doing so until you get the patch in shape. By then FrameAnimator may or may not be back, and we'll figure out what to do from there.

Comment 13

[Alexandre BM (:rednaks)]

Okey I'm waiting

Comment 14

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

To clarify, you don't need to wait to work on the patch. I was just warning that you might find it easier if you don't update to the latest revision of mozilla-central for now.

Comment 15

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

It looks like bug 899861 got backed out, so you no longer need to worry about FrameAnimator being removed. Pull freely.

Comment 16

[Alexandre BM (:rednaks)]

Hey Seth, I was looking for you on the IRC, could you please tell me what is your timezone ?

Comment 17

[Ali Ak]

Attachment: WIP fix for rendering single loop 0 frame delay images faster

The patch is just a test right now to see if I'm on the right track (GetRealTimeout is temporary just to see if this works). Obviously the frame delay should not be overridden at the imgFrame level, but should probably be at the FrameAnimator level (unless there are other users of GetTimeout, but I don't know the code at all yet). This patch makes the GIF render quicker

There is an issue though, the initial tiles in the GIF are still visibly rendered with a delay. Is this because the LoopCount is not set till all the frames are decoded or something?

Comment 18

[Ali Ak]

Attachment: Display 0-delay, single loop GIFs instantly

Do not aritifially create a frame delay for GIF files
that have a single loop and a frame delay with 0

Comment 19

[Alexandre BM (:rednaks)]

I'm freeing up the bug, would you like to take it ?

Comment 20

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

Comment on attachment 8340291 [details] [diff] [review]
Display 0-delay, single loop GIFs instantly

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

Very nice, Ali! This is a good starting point.

I'm reviewing this patch instead of the one you requested feedback for since this seems to be a newer version. If you weren't aware, you can mark a patch as obsolete by clicking on "Details" next to the patch name, then clicking "Edit Details" near the patch title and checking the box on the left labelled 'obsolete'. I'll leave you to do that in case I've made a mistake in guessing that this is the newer version of the previous patch.

::: image/src/FrameAnimator.h
@@ +125,5 @@
>     * Gets the length of a single loop of this image, in milliseconds.
>     *
>     * If this image is not finished decoding, is not animated, or it is animated
> +   * but does not loop, returns -1. Can return 0 in the case of an aniamted image
> +   * that has a 0ms delay between its frames.

Nit: should fix spelling of 'animated'.

I suspect this comment should be revised to say that we can only return 0 if we have both the 0ms delay and an animation which only loops once. That's not true now, but it should be true once the changes below are made.

::: image/src/imgFrame.cpp
@@ +746,5 @@
>  }
>  
>  int32_t imgFrame::GetTimeout() const
>  {
> +  return mTimeout;

The tricky thing about this bug is that before, an imgFrame's timeout was a local property that didn't require any information about the animation it was embedded in, but now to calculate it we need information that isn't available at the imgFrame level - the loop count of the animation.

One possibility is to move the calculation to a place where we have all the required information, (which is what you've done in this patch: move it into FrameAnimator) but we have to be pretty cautious because there are several places where we call imgFrame::GetTimeout outside of FrameAnimator. I'm not super comfortable about having different parts of the code perceive the timeout of frames differently.

What I would suggest is a bit more complicated but will be simpler and less bug prone in the end: let's move the responsibility for this calculation to FrameBlender, since FrameBlender is the class which is responsible for other global calculations involving all the frames in the animation.

The concrete steps I'd recommend are:

1. Rename imgFrame::GetTimeout() to imgFrame::GetRawTimeout(). Do not update callers; this will let you know which places need to be changed. Change it to just return mTimeout, as you have in this patch.

2. Add a method FrameBlender::GetTimeoutForFrame, which will be the new home of the full timeout calculation. You'll need the loop count to be available in FrameBlender, so also add a getter and setter for it: FrameBlender::LoopCount() and FrameBlender::SetLoopCount().

3. Update RasterImage to call mFrameBlender->SetLoopCount() where it currently calls mAnim->SetLoopCount(). Remove FrameAnimator::SetLoopCount() and the associated member variable; whenever FrameAnimator needs to know the loop count, it should call mFrameBlender->LoopCount().

4. Update FrameAnimator, FrameBlender, and RasterImage to call mFrameBlender->GetTimeoutForFrame() where they currently call imgFrame::GetTimeout(). The only thing that should call imgFrame::GetRawTimeout() (as it is now known) is FrameBlender::GetTimeoutForFrame().

It's entirely possible I've missed something, so let me know if you run into any issues. (If you're not aware, you can use Bugzilla's 'needinfo' mechanism for a quicker response: just check the "Need more information from:" box when you comment and enter ":seth" to select me.)

I'm looking forward to seeing the next version of the patch!

Comment 21

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

Comment on attachment 785540 [details] [diff] [review]
bug890743.patch

Marking Alexandre's patch obsolete and reassigning to Ali since it seems that Ali is taking this bug. Please feel free to change back if I'm wrong about this.

Comment 22

[Ali Ak]

Attachment: Display 0-delay, single loop GIFs instantly

Do not aritifially create a frame delay for GIF files
that have a single loop and a frame delay with 0. Move delay
calculation for FrameBlender and only access raw timeout of
imgFrame from within FrameBlender.

Comment 23

[Ali Ak]

Thanks for the feedback Seth! Very nice explanation of the steps required :)

Ok I re-did the patch the way you suggested, so that the raw timeout is only accessed form within FrameBlender. It does seem a little cleaner this way. The FrameBlender::DoBlend function has to set the timeout of the composite frame so I could've either create a SetTimeoutForFrame or just used the raw set/get on the imgFrame - chose to do the latter.

Is there a guide somewhere for adding a test for something like this? A unit test or a ref test maybe?

Comment 24

[Ali Ak]

One additional note: nsRefreshDriver::AddImageRequest checks if GetFirstFrameDelay returns 0 and creates an ImageStartData if it’s not 0. I don’t fully understand the consequences of having image frames now return 0.

Comment 25

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

(In reply to Ali Ak from comment #23)
> Thanks for the feedback Seth! Very nice explanation of the steps required :)
> 
> Ok I re-did the patch the way you suggested, so that the raw timeout is only
> accessed form within FrameBlender. It does seem a little cleaner this way.
> The FrameBlender::DoBlend function has to set the timeout of the composite
> frame so I could've either create a SetTimeoutForFrame or just used the raw
> set/get on the imgFrame - chose to do the latter.

Sounds great! Thanks for working on this!

> Is there a guide somewhere for adding a test for something like this? A unit
> test or a ref test maybe?

Well, we have a variety of guides for creating different types of tests. See for example this article on creating reftests:

https://developer.mozilla.org/en-US/docs/Creating_reftest-based_unit_tests

We should probably discuss your plans for testing this before you expend too much effort on it, though. Unfortunately it's difficult to do true unit testing in imagelib because many of the classes are very tightly coupled, as you've probably noticed. Generally most of our tests are instead integration tests of various kinds. This is usually workable, if not always convenient, but I'm sorry to say that this may be a case where our usual approach falls down.

The problem here is that it's quite hard to describe exactly what the test should test for in terms of properties observable from the public API of the system. We definitely can't test that the animation is complete in a certain amount of time. Such tests are generally not robust, and they are particularly bad for our testing infrastructure since we run some kinds of tests in VMs where timing can be quite wonky. We also can't test that we don't draw any intermediate frames since all sorts of things can cause delays in frame availability, including IO-related issues, concurrency between the handling of different events in Gecko and our test infrastructure, and parallelism between the decoding thread, the main thread, and the compositing thread.

In short, at the moment I have to admit I don't see a way of adding a test for this that doesn't involve more work than I think is appropriate to ask of you. =( We really need better support for unit testing in imagelib, because while we can test most things with integration tests, there are some edge cases like this that still bite us.

That said, I'd love to hear if you can come up with any ideas on how to test this in a robust manner! It's OK if not, though; the next best thing is to file a followup bug about creating a test for this feature (make it depend on this bug) and we can come back to it once imagelib has been refactored a bit to improve its testability.

Comment 26

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

(In reply to Ali Ak from comment #24)
> One additional note: nsRefreshDriver::AddImageRequest checks if
> GetFirstFrameDelay returns 0 and creates an ImageStartData if it’s not 0. I
> don’t fully understand the consequences of having image frames now return 0.

Thanks for drawing attention to this! I took a look, and it seems to me that this is fine. The code in nsRefreshDriver you're referring to tries to batch together images so when there are multiple images in the page, we refresh them together as much as possible. It does this by shifting their start time forward in some cases so that they start in phase with other images that the same inter-frame delay. (Of course, the delay can be different for different frames, but typically it's not. As a heuristic, we only check the first frame's delay.)

Your patch will mean that images which don't loop and have a zero inter-frame delay will no longer be delayed to start in phase with other images. However, precisely because they don't loop and we intend to only ever draw one frame if possible (the final one), this heuristic doesn't buy us anything for such images. This change is therefore not a problem at all.

Comment 27

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

Comment on attachment 8342899 [details] [diff] [review]
Display 0-delay, single loop GIFs instantly

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

Looks good, Ali! This patch is close to being ready to check in. At this moment I really just have style nits left.

One meta-nit about the patch itself:

When you check in a Mozilla patch, the commit message needs to include the name of the reviewer. There's a special syntax we use for this: we add "r=<reviewer>" to the end of the commit summary. In this case, the first line of your commit message should read "Bug 890743 - Display 0-delay, single loop GIFs instantly. r=seth".

Beyond these style nits, the main other thing we need to get this patch ready to go is a run on the try server. I'll go ahead and push a try job for your patch shortly.

::: image/src/FrameAnimator.h
@@ +119,5 @@
>     * Gets the length of a single loop of this image, in milliseconds.
>     *
>     * If this image is not finished decoding, is not animated, or it is animated
> +   * but does not loop, returns -1. Can return 0 in the case of an animated image
> +   * that has a 0ms delay between its frames and a single loop count.

I'd prefer to replace "a single loop count" with "does not loop". They mean the same thing, but the latter is a bit more high level.

(Note that this should be changed in the commit message as well.)

::: image/src/FrameBlender.h
@@ +58,5 @@
>  
>    /* The total number of frames in this image. */
>    uint32_t GetNumFrames() const;
>  
> +  /* Andjusted raw timeout of frame depending on loop count */

It'd be good to be a bit more clear about what "adjusted" means. This function returns the frame's timeout, adjusted if necessary so that it's never zero if the animation loops.

@@ +64,5 @@
> +
> +  /*
> +   * Set/Get the total number of loops for this image
> +   * -1 means loop forever
> +   */

Nit: Stick with Doxygen-style comments like in the original version of SetLoopCount in FrameAnimator. Periods, @note, that sort of thing. The original code in imagelib is quite ancient and wasn't written with Doxygen-style comments, but I'd prefer to move in that direction when we make changes.

Comment 28

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

Here's a try job for this patch:

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

Comment 29

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

Try looks good!

Comment 30

[Ali Ak]

Attachment: Display 0-delay, single loop GIFs instantly.

Do not aritifcially create a frame delay for GIF files
that do not loop and have an inter-frame delay with 0. Move timeout
calculation to FrameBlender and only access raw timeout of
imgFrame from within FrameBlender.

Comment 31

[Ali Ak]

(In reply to Seth Fowler [:seth] from comment #25)

> Sounds great! Thanks for working on this!

And thank you for taking the time out to provide guidance!

> In short, at the moment I have to admit I don't see a way of adding a test
> for this that doesn't involve more work than I think is appropriate to ask
> of you. =( We really need better support for unit testing in imagelib,
> because while we can test most things with integration tests, there are some
> edge cases like this that still bite us.

Yeah I see your points. Doesn't seem like there really is a way to test for correct gif rendering. But I was thinking more like unit testing for what functions should do in isolation. I haven't really thought it through, but I can look in to it eventually, when I'm perhaps more familiar with the code and whatever testing infrastructure is already in place. Though, as for time on this, I'm actually getting in to firefox fulltime. We have priorities towards fxos, but otherwise, my time is going to be inside images/ and gfx/ mostly.

> file a followup bug about creating a test for this feature (make it depend
> on this bug) and we can come back to it once imagelib has been refactored a
> bit to improve its testability.

Done - bug 947166

Comment 32

[Ali Ak]

(In reply to Seth Fowler [:seth] from comment #29)
> Try looks good!

Great. I guess now you need to r+ it or something and then you (or can I?) mark it as merge needed?

Also, feel free to edit/delete bug 947166 if I made it incorrectly :)

Comment 33

[Ali Ak]

(In reply to Seth Fowler [:seth] from comment #27)

> Looks good, Ali! This patch is close to being ready to check in. At this
> moment I really just have style nits left.
> 
> One meta-nit about the patch itself:

Sweet, fixed the nits. Sorry if there're more, I know how annoying it can be to have to comment on nits, but please do even if they are super nitty.

Comment 34

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

Comment on attachment 8343654 [details] [diff] [review]
Display 0-delay, single loop GIFs instantly.

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

Looks good!

I'll go ahead and push this for you shortly.

Comment 35

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

Pushed:

https://hg.mozilla.org/integration/mozilla-inbound/rev/263980931d1b

Comment 36

[Ali Ak]

Awesome. Thanks!

Comment 37

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/263980931d1b

Comment 38

[Joe Cheng [:jcheng] (please needinfo)]

hi Michael, can you provide the reason for 1.3T? nomination? Thanks

Comment 39

[Joe Cheng [:jcheng] (please needinfo)]

nevermind, found the reason
blocks Bug 962670
1.3T+

Comment 40

[Michael Wu [:mwu]]

http://hg.mozilla.org/releases/mozilla-b2g28_v1_3t/rev/2694572f8be1

Comment 41

[Manuela Muntean [Away]]

Verified as fixed with Firefox 29 beta 1 on Win XP x86.

Comment 42

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

In bug 1126330, we are removing the code that was added in this bug, because it's not web-compatible. Bad GIF encoders unfortunately produce GIFs with 0ms frame delay values. The result of this bug is that those GIFs play back in other browsers, but not in Firefox, which is unacceptable. (See bug 1105136 and its duplicates for other examples.)