Bug 119016 – REGRESSION (r151957): .gif images play at 10x speed for a few loops after loading

• Component: Images
• Status: RESOLVED FIXED

[jacoco]

Any time I load a .gif image in its own page, it loops through at 10x speed

[Zan Dobersek]

Can't reproduce on the GTK port, WebKit1 or WebKit2.

[Alexey Proskuryakov]

I can reproduce on Mac using <http://ru.fishki.net/picsw/072013/23/post/gif/05_dem.gif>.

Safari/WebKit 6.0.5: It plays very slowly for the first round while loading, then loops normally.
With nightly WebKit r152754: It doesn't play at all while loading, then plays crazy fast for some time, then loops normally.

This is not limited to standalone images - ones in a page like this one <http://fishki.net/comment.php?id=140726> also exhibit the same behavior.

[Alexey Proskuryakov]

<rdar://problem/14534370>

[Tim Horton]

Ossy, can you take a look? It looks like http://trac.webkit.org/changeset/151957 caused this.

[Csaba Osztrogonác]

(In reply to comment #4)
> Ossy, can you take a look? It looks like http://trac.webkit.org/changeset/151957 caused this.

Will check tomorrow.

[Csaba Osztrogonác]

I can't reproduce it on EFL WK1/WK2, Qt WK1/WK2 (r153370)
and Nix (WK2 only port) (392d108b1ae5d42be00b7d96c7bd47689e742789, r153083 - last trunk merge)

But http://fishki.net/comment.php?id=140726 crashes for me with
EFL/Qt/Nix WK2 browsers. I'll investigate it.

If you are sure that r151957 is the culprit, I'll find a Mac 
near here on monday and try to debug what happened.

[Csaba Osztrogonác]

Maybe there is some issue in platform/graphics/cg/ImageSourceCG.cpp.

After r151957 frameDurationAtIndex doesn't trigger decoding the frame,
maybe ImageSourceCG.cpp uses some invalid duration info.

[Csaba Osztrogonác]

I think this change doesn't fit somehow to the CG implementation:
http://trac.webkit.org/changeset/151957/trunk/Source/WebCore/platform/graphics/BitmapImage.cpp

Reverting the BitmapImage.cpp change with a PLATFORM(MAC) or something CG 
guard could be a workaround until we find the proper fix. Could you try
if it fixes the issue, please?

And I'll try to find the proper fix on monday.

[Csaba Osztrogonác]

I managed to reproduce this bug on Mac. For now I propose reverting
r151957 only on Mac platform as a workaround. And then fix ImageSourceCG.cpp
in a separated bug: https://bugs.webkit.org/show_bug.cgi?id=119218

[Csaba Osztrogonác]

Created attachment 207656 [details]
Patch

[Csaba Osztrogonác]

I started debugging this quick animation bug and got the root of the
problem, but I'm not sure how to fix it propely, but I'm on it.

The problem in nutshell:
The animation is started at the beginning and m_desiredFrameStartTime is initialized to the current time. But BitMapImage::frameIsCompleteAtIndex(index=1) is false until the last frame is loaded for some reason, which blocks the original very slow animation. I think it is _the_ bug somewhere. After the last frame is loaded, BitMapImage::frameIsCompleteAtIndex(index=1) becomes true and the animation is really started. Because the load time took ~5 seconds, m_desiredFrameStartTime is in the past, and the timer of the next frame immediately expired and so on.

...
bool BitmapImage::frameIsCompleteAtIndex(size_t index)
{
    if (index < m_frames.size() && m_frames[index].m_haveMetadata && m_frames[index].m_isComplete)
        return true;
    return m_source.frameIsCompleteAtIndex(index); <---- this one returns false!
}
...

[Csaba Osztrogonác]

strange, the status returned by CGImageSourceGetStatusAtIndex()
until the last frame is loaded is kCGImageStatusUnknownType = -3.

[Csaba Osztrogonác]

Created attachment 207713 [details]
Patch

Use USE(CG) guards instead of PLATFORM(MAC), because Windows port uses this code path too.

[Csaba Osztrogonác]

cc-ing Geoffrey and Mark as the authors of ImageSourceCG::frameIsCompleteAtIndex() - https://trac.webkit.org/changeset/55199

And I think https://trac.webkit.org/changeset/55199 is related to this bug too.

[Mark Rowe (bdash)]

Comment on attachment 207713 [details]
Patch

I really don't see the point in doing this rather than just fixing the problem.

[Csaba Osztrogonác]

(In reply to comment #15)
> (From update of attachment 207713 [details])
> I really don't see the point in doing this rather than just fixing the problem.

Because I have no idea how to fix it properly. 
So if you have any idea, it would be very welcome.

[Csaba Osztrogonác]

just a note: I can't fix CGImageSourceGetStatusAtIndex ( rdar://problem/7679174 ), because I have no access for Appple's internal rdar and code base. And I can't acces rdar://problem/14534370, so I don't know if anybody started working on it or if there is any discussion about it.

[Mark Rowe (bdash)]

It sounds like the changes made in <http://trac.webkit.org/changeset/151957> made assumptions about the behavior of ImageSource that aren't valid for all implementations. We should either understand what needs to be changed in the ImageSourceCG implementation to accommodate the changes made in r151957, or r151957 should be reverted. Adding #if's just heads down the path of divergent behavior which will make life difficult for everyone going forward.

<rdar://problem/7679174> is unlikely to be relevant to whatever is happening here since it was fixed in Lion. Fixing CGImageSourceGetStatusAtIndex isn't relevant here either since that's OS X API and not part of WebKit.

[Csaba Osztrogonác]

The root of the problem is that http://trac.webkit.org/changeset/113486 "made a bunch of incorrect changes to BitmapImage.cpp" - https://bugs.webkit.org/show_bug.cgi?id=116041#c12

After r113486 BitmapImage::frameIsCompleteAtIndex,  BitmapImage::frameHasAlphaAtIndex, ... trigger image decoding. Which
is incorrect and caused flakey crashes everywhere. Chrome fixed it
in Blink and r151957 fixed it in WebKit ... but unfortunately this
fix doesn't fit to the ImageSourceCG implementation.

Reverting r151957 is the worst choice, because it would paper over the
problem and only push the bug from CG platforms to other platforms. 

My proposed workaround would let the CG use the previous and working version
(which wastes resources with useless image decodings, but at least works) and
let all other ports keep the fix of the buggy r113486 and not to have flakey crashes.

If you want to revert r151957, we should revert r113486 and maybe
many other patches on the top of it to be consistent and fix all ports.

But let's try to collaborate a little bit instead of reverting everything.
r20070 added ImageSource::frameIsCompleteAtIndex():
+    bool frameIsCompleteAtIndex(size_t); // Whether or not the frame is completely decoded.
...
diff --git a/WebCore/platform/graphics/BitmapImage.cpp b/WebCore/platform/graphics/BitmapImage.cpp
index 136bc26..b4f66d2 100644
--- a/WebCore/platform/graphics/BitmapImage.cpp
+++ b/WebCore/platform/graphics/BitmapImage.cpp
@@ -209,6 +209,10 @@ void BitmapImage::startAnimation()
     if (m_frameTimer || !shouldAnimate() || frameCount() <= 1)
         return;

+    // Don't advance the animation until the current frame has completely loaded.
+    if (!m_source.frameIsCompleteAtIndex(m_currentFrame))
+        return;
+
...

These comments are obvious and expects that ImageSource::frameIsCompleteAtIndex() returns with true if the given frames is "completely loaded" "Whether or not the frame is completely decoded.". But unfortunately ImageSourceCG implementation of ImageSource::frameIsCompleteAtIndex() doesn't pass this requirement. r151957 only revealed this bug and didn't caused.

The problem is that we can't ask CGImageSource if the frame is 
loaded or not. And I can't fix CGImageSource, only Apple can do it.

[Geoffrey Garen]

Comment on attachment 207713 [details]
Patch

A random walk that inserts some bugs while fixing others, peppering the code with #ifdefs as we go, is not how WebKit development works.

[Geoffrey Garen]

Ossy, I'd like to give you a chance to fix r151957 -- but if you're still sure you can't fix it, I'll roll it out.

[Csaba Osztrogonác]

(In reply to comment #20)
> (From update of attachment 207713 [details])
> A random walk that inserts some bugs while fixing others, peppering the code with #ifdefs as we go, is not how WebKit development works.
I still think that r151957 was the correct fix and ImageSourceCG.cpp 
does the wrong thing. I'm sorry that I couldn't convince you about
it in https://bugs.webkit.org/show_bug.cgi?id=119016#c19 :-(

(In reply to comment #21)
> Ossy, I'd like to give you a chance to fix r151957 -- but if you're still sure you can't fix it, I'll roll it out.

Thanks for the chance, but as I said, I can't fix the 3rdparty CG library 
and have no idea how can we do a workaround for Mac inside WebKit.

You won, let's break animated gifs everywhere except Mac. I reverted
r151957 and its followup patch r152531 by http://trac.webkit.org/changeset/153475

[Mark Rowe (bdash)]

(In reply to comment #22)
> You won, let's break animated gifs everywhere except Mac. 

The change that you rolled out was an optimization. It broke Mac. Rolling it out undoes the optimization, it doesn't break anything.

[Csaba Osztrogonác]

(In reply to comment #23)
> (In reply to comment #22)
> > You won, let's break animated gifs everywhere except Mac. 
> 
> The change that you rolled out was an optimization. It broke Mac. Rolling it out undoes the optimization, it doesn't break anything.

No, it wasn't only an optimization, it was originally to fix flakey crashes
during animated gifs on all non-CG platform. Maybe the name of the original
bug was a little bit misleading. But the details can be found as comments.

Right now I got an idea in the original bug how to fix it properly and 
will try it tomorrow. I hope all platforms would be happy soon.