Bug 55830 – REGRESSION: Animated gifs blink and cause high CPU usage

• Status: RESOLVED
• Resolution: INVALID
• Component: Images

[David]

http://www.openbsd.org/art/banners/banner1.gif

With webkit 1.2.7, that image blinks instead of animating.

Backing out change listed at http://gitorious.org/webkitgtk/stable/commit/d842bb085aceec4fcfc392a7b76c92c258cd5151 fixes the issue.

[Alexey Proskuryakov]

This is extremely strange. How can a change in SMIL code possibly affect animated GIFs?

[David]

Are you able to confirm?

[Marco Peereboom]

I see this bug too and reverting that code works for me as well.

[Nikolas Zimmermann]

This sounds impossible. SMIL is not involved when displaying gifs.

[Marco Peereboom]

"Impossible" or "extremely strange" doesn't change the fact that webkit regressed with this change.

[Nikolas Zimmermann]

(In reply to comment #5)
> "Impossible" or "extremely strange" doesn't change the fact that webkit regressed with this change.

Nope, there must be another reason, either corrupted builds, whatever, there is no SMIL involved, believe me. This code path is never executed when displaying gifs. It's about animating SVG documents.

What's the testcase you're using? Just viewing the gif, embedding the gif in a SVG document, or a HTML document? We need more input to judge.

[David]

in my original report, i pasted a direct link to a gif.

Do you show it as blinking or animated?

[Dirk Schulze]

Which svn revision is d842bb085aceec4fcfc392a7b76c92c258cd5151? Btw. I tried a chromium snapshot from March 3rd, but didn't see a problem with the gif. The image looks the same like in Safari 5, animated.

[Tobias Ulmer]

I can confirm that the gif just flickers on Archlinux i686 with libwekkit-1.2.7-1 and midori-0.3.2-1.

[David]

oops.

I had two patches applied.

It seems the following change is the culprit.  Backing this one out makes animations work again.   Does this one make more sense? :)

http://trac.webkit.org/changeset/68446/trunk/WebCore/platform/image-deco
ders/gif/GIFImageDecoder.cpp

[David]

http://trac.webkit.org/changeset/68446/trunk/WebCore/platform/image-decoders/gif/GIFImageDecoder.cpp

[Peter Kasting]

(In reply to comment #11)
> http://trac.webkit.org/changeset/68446/trunk/WebCore/platform/image-decoders/gif/GIFImageDecoder.cpp

What port is broken here?  If you look at the full patch there, you'll see that it only changed behavior on Skia.  AFAIK that's only used for Chromium, which displays this GIF correctly in my tests.

I just checked the current ImageDecoder*.cpp files and no one but Skia ever returns false from this function, so unless you're using a Skia-based port this change can't have affected you.

[Marco Peereboom]

Per Comment #9 From Tobias Ulmer 2011-03-07 06:46:28 PST

I can confirm that the gif just flickers on Archlinux i686 with libwekkit-1.2.7-1 and midori-0.3.2-1.

So that is not just chromium

[Peter Kasting]

(In reply to comment #13)
> Per Comment #9 From Tobias Ulmer 2011-03-07 06:46:28 PST
> 
> I can confirm that the gif just flickers on Archlinux i686 with libwekkit-1.2.7-1 and midori-0.3.2-1.
> 
> So that is not just chromium

Yes, I saw that comment.  I have no idea what port that is or what code it uses.  So it doesn't answer my question.

[David]

Here is a diff that causes "a1 = 1" to flood my xterm.  So, it does indeed hit the code path.



--- GIFImageDecoder.cpp.orig    Mon Mar  7 16:59:34 2011
+++ GIFImageDecoder.cpp Mon Mar  7 16:49:46 2011
@@ -342,10 +342,11 @@
         ASSERT(prevBuffer->status() == RGBA32Buffer::FrameComplete);
 
         if ((prevMethod == RGBA32Buffer::DisposeNotSpecified) || (prevMethod == RGBA32Buffer::DisposeKeep)) {
-            // Preserve the last frame as the starting state for this frame.
-            if (!buffer->copyBitmapData(*prevBuffer));
-                return setFailed();
-        } else {
+            int a1;
+               // Preserve the last frame as the starting state for this frame.
+            a1 = buffer->copyBitmapData(*prevBuffer);
+               printf("a1 = %d\n", a1);  
+      } else {
             // We want to clear the previous frame to transparent, without
             // affecting pixels in the image outside of the frame.
             const IntRect& prevRect = prevBuffer->rect();
@@ -356,10 +357,11 @@
                 if (!buffer->setSize(bufferSize.width(), bufferSize.height()))
                     return setFailed();
             } else {
+               int a2;
               // Copy the whole previous buffer, then clear just its frame.
-              if (!buffer->copyBitmapData(*prevBuffer));
-                  return setFailed();
-              for (int y = prevRect.y(); y < prevRect.bottom(); ++y) {
+              a2 = buffer->copyBitmapData(*prevBuffer);
+              printf("a2 = %d\n", a2);
+               for (int y = prevRect.y(); y < prevRect.bottom(); ++y) {
                   for (int x = prevRect.x(); x < prevRect.right(); ++x)
                       buffer->setRGBA(x, y, 0, 0, 0, 0);
               }

[David]

here is your patch to fix it.  


--- GIFImageDecoder.cpp.orig    Mon Mar  7 16:59:34 2011
+++ GIFImageDecoder.cpp Mon Mar  7 17:14:09 2011
@@ -343,7 +343,7 @@
 
         if ((prevMethod == RGBA32Buffer::DisposeNotSpecified) || (prevMethod == RGBA32Buffer::DisposeKeep)) {
             // Preserve the last frame as the starting state for this frame.
-            if (!buffer->copyBitmapData(*prevBuffer));
+            if (!buffer->copyBitmapData(*prevBuffer))
                 return setFailed();
         } else {
             // We want to clear the previous frame to transparent, without
@@ -357,7 +357,7 @@
                     return setFailed();
             } else {
               // Copy the whole previous buffer, then clear just its frame.
-              if (!buffer->copyBitmapData(*prevBuffer));
+              if (!buffer->copyBitmapData(*prevBuffer))
                   return setFailed();
               for (int y = prevRect.y(); y < prevRect.bottom(); ++y) {
                   for (int x = prevRect.x(); x < prevRect.right(); ++x)

[Peter Kasting]

Your fork/branch/checkout/what-have-you is broken.  You have a trailing semicolon after the calls to copyBitmapData() that does not exist in the changeset you cited or on the trunk.  The "patch to fix" is a patch that makes the code look like what trunk already looks like (and always has looked like).

To put this another way, whoever constructed the particular libraries in Archlinux did it wrong, and the fix should go into whatever location those are built out of.

[David]

ahh, the webkitgtk branch is where it exists.

[Marco Peereboom]

You are right.  That file seems correct in the trunk however it is busted in the tarball that is provided by http://webkitgtk.org/?page=download

The contribute link on the webkitgtk to report bugs goes to bugs.webkit.org.

So whomever runs the show on the webkitgtk checkout made a merging mistake (or something).  How does one get fixes into webkitgtk?  Is it not the same bug tracker?

[Alexey Proskuryakov]

Followup in bug 55937.