Bug 119016

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

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.

<rdar://problem/14534370>

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

(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.

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.

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.

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.

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

Created attachment 207656 [details] Patch

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! } ...

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

Created attachment 207713 [details] Patch Use USE(CG) guards instead of PLATFORM(MAC), because Windows port uses this code path too.

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.

Comment on attachment 207713 [details] Patch I really don't see the point in doing this rather than just fixing the problem.

(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.

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.

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.

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.

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.

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.

(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

(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.

(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.