20745 2008-09-09 07:33:53 -0700 REGRESSION: Animated GIF not animated 2008-09-22 15:02:49 -0700 1 1 1 Unclassified WebKit Layout and Rendering 528+ (Nightly build) Mac (Intel) OS X 10.5 RESOLVED FIXED http://castle104.com/images/spinner.gif InRadar P1 Normal --- 1 naofumi pkasting ap ftuvok pkasting oldest_to_newest 90652 0 naofumi 2008-09-09 07:33:53 -0700 Confirmed on Webkit SVN r38283, MacOS X 10.5.4, MacBook (Intel Core Duo 2GHz). This bug was not present on the shipping version of Safari 3.1.2(5525.20.1). On Webkit SVN r38283, the animated GIF in the URL is not animated immediately after being loaded. It stays frozen at one frame. If you resize the window, the GIF starts. You can also get it to start by keeping the mouse button pressed on the Webkit window (this is however unreliable and doesn't always work). If you hit the reload button (Command + R), then the animation stops again. This problem does not occur on the current shipping version of Safari 3.1.2. 90653 1 naofumi 2008-09-09 07:37:54 -0700 MIght be related to bug #16177 90775 2 ftuvok 2008-09-10 11:33:54 -0700 This is not a problem created when submitting a form via javascript. I'll give you an example http://cache.hyvz.com/images/smilies/default/smiley_byebye.gif That is an animated gif that doesn't move in webkit. This problem started with a build less than 2 weeks ago from today… I update weekly and this appeared on my last update. 90823 3 naofumi 2008-09-10 23:15:53 -0700 Did some detective work to find out when this bug appeared on the nightlies. Before WebKit-SVN-r36053 was OK. After WebKit-SVN-r36078 has this bug. 90824 4 gsherloc 2008-09-10 23:37:18 -0700 Possibly: http://trac.webkit.org/changeset/36069 is the culprit 91072 5 pkasting 2008-09-13 09:27:19 -0700 This is probably my fault. 91147 6 mitz 2008-09-14 16:15:04 -0700 <rdar://problem/6218856> 91405 7 pkasting 2008-09-16 11:13:19 -0700 OK, so far it looks like my patch is tickling a probably-previously-unnoticed bug in the CG decoder (which I can't debug). When the image is loaded, setData(..., ..., true) is called, meaning all the data is available; but when startAnimation() asks if the first frame is complete, the CG decoder says "no". So the animation code doesn't begin the animation. bdash mentioned on IRC that he'd glance at this. Either the CG code needs to be fixed to properly report frame completion status, or I'm going to have to figure out how to work around this in the animation code, which I'm not sure how to do. Unassigning for now as I can't proceed further until I hear more about the CG side of things. 91411 8 hyatt 2008-09-16 12:04:18 -0700 You need to cache the frame before you can ask if it's complete (at least for CG). We should just add a frameIsCompleteAtIndex to BitmapImage (that is like the other methods) that caches the frame and then asks the source if the frame is complete after doing the caching. 91413 9 pkasting 2008-09-16 12:46:07 -0700 Ah, I see what type of thing you're suggesting. Let me try and whip that up. I wonder if this will be a perf hit. There's some code in the animation functions that tries to aggressively throw away decoded data for large images, and we may be fighting that, especially when startAnimation() goes into a loop looking through the frames to see how far we need to go to catch up. Please review this carefully when I get it done. 91436 10 23486 pkasting 2008-09-16 14:00:34 -0700 Created attachment 23486 Partial patch v1 This patch makes life better, but does not fix all the problems. I elected to cache "m_isComplete" on the FrameData to keep things in parallel with how the duration and alpha work. Someone should yell if this is invalid, say because the completion status of a frame can change without cacheFrame() being called a second time. With this patch, the image on the URL animates on load. However, some fiddling with the resize corner can eventually make it stop animating. What I think is happening is that we're falling into the "too far behind, skip the current frame" case in startAnimation(). In this case, internalAdvanceAnimation() changes the image and notifies observers, expecting this to eventually result in another call to BitmapImage::draw(), which in turn will call back to startAnimation() and result in a timer getting set for the next frame. But draw() is never reached after observer notification. I don't know why. We mark parts of the backing store dirty but beyond that my ability to trace through this code diminishes. Help wanted :( 91532 11 23486 hyatt 2008-09-17 14:48:53 -0700 Comment on attachment 23486 Partial patch v1 r=me 91694 12 23486 eric 2008-09-18 16:08:10 -0700 Comment on attachment 23486 Partial patch v1 Landed as r36628 Clearing review flag, since this is only part 1 of 2 for the fix. 91871 13 hyatt 2008-09-19 21:41:37 -0700 This fix I suggested is not right. Fully decoding every frame just to catch up is leading to even basic animated GIFs consuming 100% CPU. 91873 14 hyatt 2008-09-19 21:51:06 -0700 This patch caused https://bugs.webkit.org/show_bug.cgi?id=20954 And was rolled out in r36702. 91878 15 hyatt 2008-09-19 22:43:20 -0700 So here are the interesting issues that I see: (1) cacheFrame forces a decode of an image frame. This can potentially be very expensive. (2) CG won't report a frame is complete unless you have in fact tried to decode an image frame. (3) Resizing on OS X (mousing down in the resizer) pauses all animations. That means when you lift the mouse up the "catch up" code is presumably going to run. (4) startAnimation() got moved to the top of the draw function in ImageCG.cpp. This means the frame you are trying to draw may be falsely reported as incomplete. (5) (Cross-platform issue) We are willing to draw incomplete frames. An image like: http://img2.abload.de/img/almost_failedovs.gif flashes to white a bunch as incomplete frames render. (When the image is uncached.) 91879 16 hyatt 2008-09-19 22:45:34 -0700 There's an additional issue of multi-frame image formats other than GIF being falsely considered to be "animatable." That needs to be fixed. 91899 17 pkasting 2008-09-20 09:39:55 -0700 If you've rolled back this patch, please also roll back r36069 and reopen bug 19663. Until these issues are addressed, having that checked in puts the tree in a broken state; animated images just don't work right. The patch on bug 20945 addresses the problem where images are falsely reported as animatable. As for the numeric list you give, my thoughts: (2) seems like something that could be improved in CG, honestly. For example, if m_allDataReceived is true, you know all frames are complete. I think the open-source decoders (which Chromium is using) may do better here. Fixing this would probably allow me to rewrite the code in a way that doesn't cacheFrame() so aggressively (i.e. by not making the fix on this bug). Still, offhand it doesn't seem like we should really be eating tons of extra CPU here. I think I need to discuss this more. (3) seems like desired behavior. (I would like even more for resizing not to pause animations, but I assume that's platform-standard or something.) Unless resizing also halts videos, plugins, timers, sound, etc., animated images _should_ catch up. (Of course, if it halts all those things too, I retract my statement.) (4) shouldn't be an issue, I don't think (?), unless (2) remains unfixed and something like this patch doesn't go in. As-is, you have to decode the frame to draw anyway, so all this is doing is making us decode it at a slightly earlier point in draw() in order to have correct frame completion info. As for (5), I'd need to take a look at my code to ensure it's not trying to skip through incomplete frames; that's the first possibility that comes to mind. 92209 18 pkasting 2008-09-22 14:49:18 -0700 Closing, since the original patch that caused this regression has now been backed out. 92214 19 ftuvok 2008-09-22 14:57:51 -0700 This is still an issue in the latest nightly build (r36766) 92218 20 pkasting 2008-09-22 15:02:49 -0700 The revert didn't happen until r36781, have patience :) 23486 2008-09-16 14:00:34 -0700 2008-09-18 16:08:10 -0700 Partial patch v1 patch text/plain 5618 pkasting SW5kZXg6IENoYW5nZUxvZw0KPT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PQ0KLS0tIENoYW5nZUxvZwkocmV2aXNpb24gMzY0 NjEpCisrKyBDaGFuZ2VMb2cJKHdvcmtpbmcgY29weSkKQEAgLTEsMyArMSwxNyBAQAorMjAwOC0w OS0xNiAgUGV0ZXIgS2FzdGluZyAgPHBrYXN0aW5nQGdvb2dsZS5jb20+CisKKyAgICAgICAgUmV2 aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCisKKyAgICAgICAgaHR0cHM6Ly9idWdzLndlYmtpdC5v cmcvc2hvd19idWcuY2dpP2lkPTIwNzQ1CisgICAgICAgIEFuaW1hdGVkIEdJRnMgZG8gbm90IGFu aW1hdGUgcHJvcGVybHkgd2l0aCAoYXQgbGVhc3QpIENHLgorCisgICAgICAgICogV2ViQ29yZVxw bGF0Zm9ybVxncmFwaGljc1xCaXRtYXBJbWFnZS5jcHA6CisgICAgICAgICogV2ViQ29yZVxwbGF0 Zm9ybVxncmFwaGljc1xCaXRtYXBJbWFnZS5oOgorICAgICAgICAqIFdlYkNvcmVccGxhdGZvcm1c Z3JhcGhpY3NcY2Fpcm9cSW1hZ2VDYWlyby5jcHA6CisgICAgICAgICogV2ViQ29yZVxwbGF0Zm9y bVxncmFwaGljc1xjZ1xJbWFnZUNHLmNwcDoKKyAgICAgICAgKiBXZWJDb3JlXHBsYXRmb3JtXGdy YXBoaWNzXHF0XEltYWdlUXQuY3BwOgorICAgICAgICAqIFdlYkNvcmVccGxhdGZvcm1cZ3JhcGhp Y3Ncd3hcSW1hZ2VXeC5jcHA6CisKIDIwMDgtMDktMTUgIERhdmlkIFNtaXRoICA8Y2F0ZmlzaC5t YW5AZ21haWwuY29tPgogCiAgICAgICAgICJKdXN0IGRvIGl0IidkIGJ5IE1hcmsgUm93ZQpAQCAt Mjc0NSw3ICsyNzU5LDcgQEAKICAgICAgICAgKiBwbGF0Zm9ybS9ncmFwaGljcy9jYWlyby9Gb250 Q2Fpcm8uY3BwOgogICAgICAgICAoV2ViQ29yZTo6Rm9udDo6ZHJhd0dseXBocyk6CiAKLTIwMDgt MDktMDMgIFBldGVyIEthc3RpbmcgIDx6ZXJvZHB4QGdtYWlsLmNvbT4KKzIwMDgtMDktMDMgIFBl dGVyIEthc3RpbmcgIDxwa2FzdGluZ0Bnb29nbGUuY29tPgogCiAgICAgICAgIFJldmlld2VkIGJ5 IERhdmUgSHlhdHQuCiAKSW5kZXg6IHBsYXRmb3JtL2dyYXBoaWNzL0JpdG1hcEltYWdlLmNwcA0K PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PQ0KLS0tIHBsYXRmb3JtL2dyYXBoaWNzL0JpdG1hcEltYWdlLmNwcAkocmV2aXNp b24gMzY0NjEpCisrKyBwbGF0Zm9ybS9ncmFwaGljcy9CaXRtYXBJbWFnZS5jcHAJKHdvcmtpbmcg Y29weSkKQEAgLTEzMCw2ICsxMzAsNyBAQCB2b2lkIEJpdG1hcEltYWdlOjpjYWNoZUZyYW1lKHNp emVfdCBpbmRlCiAgICAgaWYgKG51bUZyYW1lcyA9PSAxICYmIG1fZnJhbWVzW2luZGV4XS5tX2Zy YW1lKQogICAgICAgICBjaGVja0ZvclNvbGlkQ29sb3IoKTsKIAorICAgIG1fZnJhbWVzW2luZGV4 XS5tX2lzQ29tcGxldGUgPSBtX3NvdXJjZS5mcmFtZUlzQ29tcGxldGVBdEluZGV4KGluZGV4KTsK ICAgICBpZiAoc2hvdWxkQW5pbWF0ZSgpKQogICAgICAgICBtX2ZyYW1lc1tpbmRleF0ubV9kdXJh dGlvbiA9IG1fc291cmNlLmZyYW1lRHVyYXRpb25BdEluZGV4KGluZGV4KTsKICAgICBtX2ZyYW1l c1tpbmRleF0ubV9oYXNBbHBoYSA9IG1fc291cmNlLmZyYW1lSGFzQWxwaGFBdEluZGV4KGluZGV4 KTsKQEAgLTIxNCw2ICsyMTUsMTcgQEAgTmF0aXZlSW1hZ2VQdHIgQml0bWFwSW1hZ2U6OmZyYW1l QXRJbmRleAogICAgIHJldHVybiBtX2ZyYW1lc1tpbmRleF0ubV9mcmFtZTsKIH0KIAorYm9vbCBC aXRtYXBJbWFnZTo6ZnJhbWVJc0NvbXBsZXRlQXRJbmRleChzaXplX3QgaW5kZXgpCit7CisgICAg aWYgKGluZGV4ID49IGZyYW1lQ291bnQoKSkKKyAgICAgICAgcmV0dXJuIHRydWU7CisKKyAgICBp ZiAoaW5kZXggPj0gbV9mcmFtZXMuc2l6ZSgpIHx8ICFtX2ZyYW1lc1tpbmRleF0ubV9mcmFtZSkK KyAgICAgICAgY2FjaGVGcmFtZShpbmRleCk7CisKKyAgICByZXR1cm4gbV9mcmFtZXNbaW5kZXhd Lm1faXNDb21wbGV0ZTsKK30KKwogZmxvYXQgQml0bWFwSW1hZ2U6OmZyYW1lRHVyYXRpb25BdElu ZGV4KHNpemVfdCBpbmRleCkKIHsKICAgICBpZiAoaW5kZXggPj0gZnJhbWVDb3VudCgpKQpAQCAt MjQ3LDcgKzI1OSw3IEBAIHZvaWQgQml0bWFwSW1hZ2U6OnN0YXJ0QW5pbWF0aW9uKCkKICAgICAg ICAgcmV0dXJuOwogCiAgICAgLy8gRG9uJ3QgYWR2YW5jZSB0aGUgYW5pbWF0aW9uIHVudGlsIHRo ZSBjdXJyZW50IGZyYW1lIGhhcyBjb21wbGV0ZWx5IGxvYWRlZC4KLSAgICBpZiAoIW1fc291cmNl LmZyYW1lSXNDb21wbGV0ZUF0SW5kZXgobV9jdXJyZW50RnJhbWUpKQorICAgIGlmICghZnJhbWVJ c0NvbXBsZXRlQXRJbmRleChtX2N1cnJlbnRGcmFtZSkpCiAgICAgICAgIHJldHVybjsKIAogICAg IC8vIERvbid0IGFkdmFuY2UgcGFzdCB0aGUgbGFzdCBmcmFtZSBpZiB3ZSBoYXZlbid0IGRlY29k ZWQgdGhlIHdob2xlIGltYWdlCkBAIC0yODIsNyArMjk0LDcgQEAgdm9pZCBCaXRtYXBJbWFnZTo6 c3RhcnRBbmltYXRpb24oKQogICAgICAgICAvLyBTZWUgaWYgd2UndmUgYWxzbyBwYXNzZWQgdGhl IHRpbWUgZm9yIGZyYW1lcyBhZnRlciB0aGF0IHRvIHN0YXJ0LCBpbgogICAgICAgICAvLyBjYXNl IHdlIG5lZWQgdG8gc2tpcCBzb21lIGZyYW1lcyBlbnRpcmVseS4KICAgICAgICAgc2l6ZV90IG5l eHRGcmFtZSA9IChtX2N1cnJlbnRGcmFtZSArIDEpICUgZnJhbWVDb3VudCgpOwotICAgICAgICB3 aGlsZSAobV9zb3VyY2UuZnJhbWVJc0NvbXBsZXRlQXRJbmRleChuZXh0RnJhbWUpKSB7CisgICAg ICAgIHdoaWxlIChmcmFtZUlzQ29tcGxldGVBdEluZGV4KG5leHRGcmFtZSkpIHsKICAgICAgICAg ICAgIC8vIFNob3VsZCB3ZSBza2lwIHRoZSBjdXJyZW50IGZyYW1lPwogICAgICAgICAgICAgZG91 YmxlIG5leHRGcmFtZVN0YXJ0VGltZSA9IG1fZGVzaXJlZEZyYW1lU3RhcnRUaW1lICsgZnJhbWVE dXJhdGlvbkF0SW5kZXgobmV4dEZyYW1lKTsKICAgICAgICAgICAgIGlmICh0aW1lIDwgbmV4dEZy YW1lU3RhcnRUaW1lKQpJbmRleDogcGxhdGZvcm0vZ3JhcGhpY3MvQml0bWFwSW1hZ2UuaA0KPT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PQ0KLS0tIHBsYXRmb3JtL2dyYXBoaWNzL0JpdG1hcEltYWdlLmgJKHJldmlzaW9uIDM2 NDYxKQorKysgcGxhdGZvcm0vZ3JhcGhpY3MvQml0bWFwSW1hZ2UuaAkod29ya2luZyBjb3B5KQpA QCAtNjYsNiArNjYsNyBAQCB0ZW1wbGF0ZSA8dHlwZW5hbWUgVD4gY2xhc3MgVGltZXI7CiBzdHJ1 Y3QgRnJhbWVEYXRhIDogTm9uY29weWFibGUgewogICAgIEZyYW1lRGF0YSgpCiAgICAgICAgIDog bV9mcmFtZSgwKQorICAgICAgICAsIG1faXNDb21wbGV0ZShmYWxzZSkKICAgICAgICAgLCBtX2R1 cmF0aW9uKDApCiAgICAgICAgICwgbV9oYXNBbHBoYSh0cnVlKSAKICAgICB7CkBAIC03OSw2ICs4 MCw3IEBAIHN0cnVjdCBGcmFtZURhdGEgOiBOb25jb3B5YWJsZSB7CiAgICAgdm9pZCBjbGVhcigp OwogCiAgICAgTmF0aXZlSW1hZ2VQdHIgbV9mcmFtZTsKKyAgICBib29sIG1faXNDb21wbGV0ZTsK ICAgICBmbG9hdCBtX2R1cmF0aW9uOwogICAgIGJvb2wgbV9oYXNBbHBoYTsKIH07CkBAIC0xNTAs NiArMTUyLDcgQEAgcHJvdGVjdGVkOgogICAgIHNpemVfdCBjdXJyZW50RnJhbWUoKSBjb25zdCB7 IHJldHVybiBtX2N1cnJlbnRGcmFtZTsgfQogICAgIHNpemVfdCBmcmFtZUNvdW50KCk7CiAgICAg TmF0aXZlSW1hZ2VQdHIgZnJhbWVBdEluZGV4KHNpemVfdCk7CisgICAgYm9vbCBmcmFtZUlzQ29t cGxldGVBdEluZGV4KHNpemVfdCk7CiAgICAgZmxvYXQgZnJhbWVEdXJhdGlvbkF0SW5kZXgoc2l6 ZV90KTsKICAgICBib29sIGZyYW1lSGFzQWxwaGFBdEluZGV4KHNpemVfdCk7IAogCkluZGV4OiBw bGF0Zm9ybS9ncmFwaGljcy9jYWlyby9JbWFnZUNhaXJvLmNwcA0KPT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PQ0KLS0tIHBs YXRmb3JtL2dyYXBoaWNzL2NhaXJvL0ltYWdlQ2Fpcm8uY3BwCShyZXZpc2lvbiAzNjQ2MSkKKysr IHBsYXRmb3JtL2dyYXBoaWNzL2NhaXJvL0ltYWdlQ2Fpcm8uY3BwCSh3b3JraW5nIGNvcHkpCkBA IC00Myw2ICs0Myw3IEBAIHZvaWQgRnJhbWVEYXRhOjpjbGVhcigpCiAgICAgaWYgKG1fZnJhbWUp IHsKICAgICAgICAgY2Fpcm9fc3VyZmFjZV9kZXN0cm95KG1fZnJhbWUpOwogICAgICAgICBtX2Zy YW1lID0gMDsKKyAgICAgICAgbV9pc0NvbXBsZXRlID0gZmFsc2U7CiAgICAgICAgIG1fZHVyYXRp b24gPSAwLjsKICAgICAgICAgbV9oYXNBbHBoYSA9IHRydWU7CiAgICAgfQpJbmRleDogcGxhdGZv cm0vZ3JhcGhpY3MvY2cvSW1hZ2VDRy5jcHANCj09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0NCi0tLSBwbGF0Zm9ybS9ncmFw aGljcy9jZy9JbWFnZUNHLmNwcAkocmV2aXNpb24gMzY0NjEpCisrKyBwbGF0Zm9ybS9ncmFwaGlj cy9jZy9JbWFnZUNHLmNwcAkod29ya2luZyBjb3B5KQpAQCAtNTIsNiArNTIsNyBAQCB2b2lkIEZy YW1lRGF0YTo6Y2xlYXIoKQogICAgIGlmIChtX2ZyYW1lKSB7CiAgICAgICAgIENHSW1hZ2VSZWxl YXNlKG1fZnJhbWUpOwogICAgICAgICBtX2ZyYW1lID0gMDsKKyAgICAgICAgbV9pc0NvbXBsZXRl ID0gZmFsc2U7CiAgICAgICAgIG1fZHVyYXRpb24gPSAwLjBmOwogICAgICAgICBtX2hhc0FscGhh ID0gdHJ1ZTsKICAgICB9CkluZGV4OiBwbGF0Zm9ybS9ncmFwaGljcy9xdC9JbWFnZVF0LmNwcA0K PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PQ0KLS0tIHBsYXRmb3JtL2dyYXBoaWNzL3F0L0ltYWdlUXQuY3BwCShyZXZpc2lv biAzNjQ2MSkKKysrIHBsYXRmb3JtL2dyYXBoaWNzL3F0L0ltYWdlUXQuY3BwCSh3b3JraW5nIGNv cHkpCkBAIC03Myw2ICs3Myw3IEBAIHZvaWQgRnJhbWVEYXRhOjpjbGVhcigpCiB7CiAgICAgaWYg KG1fZnJhbWUpIHsKICAgICAgICAgbV9mcmFtZSA9IDA7CisgICAgICAgIG1faXNDb21wbGV0ZSA9 IGZhbHNlOwogICAgICAgICBtX2R1cmF0aW9uID0gMC4wZjsKICAgICAgICAgbV9oYXNBbHBoYSA9 IHRydWU7CiAgICAgfQpJbmRleDogcGxhdGZvcm0vZ3JhcGhpY3Mvd3gvSW1hZ2VXeC5jcHANCj09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT0NCi0tLSBwbGF0Zm9ybS9ncmFwaGljcy93eC9JbWFnZVd4LmNwcAkocmV2aXNpb24g MzY0NjEpCisrKyBwbGF0Zm9ybS9ncmFwaGljcy93eC9JbWFnZVd4LmNwcAkod29ya2luZyBjb3B5 KQpAQCAtNTcsNiArNTcsNyBAQCB2b2lkIEZyYW1lRGF0YTo6Y2xlYXIoKQogICAgIGlmIChtX2Zy YW1lKSB7CiAgICAgICAgIGRlbGV0ZSBtX2ZyYW1lOwogICAgICAgICBtX2ZyYW1lID0gMDsKKyAg ICAgICAgbV9pc0NvbXBsZXRlID0gZmFsc2U7CiAgICAgICAgIG1fZHVyYXRpb24gPSAwLjsKICAg ICAgICAgbV9oYXNBbHBoYSA9IHRydWU7CiAgICAgfQo=