Bug 176089 – [GTK][WPE] Fix playback of GIFs

• Component: Images
• Status: RESOLVED FIXED

[Michael Catanzaro]

We need to fix playback of imgur .gifv videos. They play super slowly.

E.g. http://i.imgur.com/YSxwPUC.gifv

[Carlos Garcia Campos]

I can't reproduce it

[Michael Catanzaro]

(In reply to Carlos Garcia Campos from comment #1)
> I can't reproduce it

Seems .givf can be a wrapper around either GIF or MP4. You're getting the MP4 so it plays fine. I'm getting the GIF, which is broken. Here is a direct link to the GIF:

http://i.imgur.com/YSxwPUC.gif

I think GIFs on many websites have been broken for a long time (about one year now). The first iteration is extremely slow and choppy. After the first iteration, it plays properly.

[Jer Noble]

In bug #176118, I think I discovered a clue about what might be causing the choppy decode during the first play through. The image-decoders/ decoders used on GTK+ will attempt to decode the entire image frame when asked whether the frame is "complete" (i.e., has all its data available) or what the frame's duration is. These operations happen on the main thread, so the decode is synchronous, and depending on the amount of time required to decode the frame, will cause playback to miss one-or-more subsequent frames, or for those frames to be delivered late.

An interesting area of investigation would be to see if "parsing" of image data can be separated from "decoding" of frames, so that answers to the questions of "is this frame complete" and "what is this frame's duration" can be answered synchronously and cheaply without requiring a full decode of the frame.

[Said Abou-Hallawa]

(In reply to Michael Catanzaro from comment #2)
> (In reply to Carlos Garcia Campos from comment #1)
> > I can't reproduce it
> 
> Seems .givf can be a wrapper around either GIF or MP4. You're getting the
> MP4 so it plays fine. I'm getting the GIF, which is broken. Here is a direct
> link to the GIF:
> 
> http://i.imgur.com/YSxwPUC.gif
> 
> I think GIFs on many websites have been broken for a long time (about one
> year now). The first iteration is extremely slow and choppy. After the first
> iteration, it plays properly.

Is the problem fixed if you disable async image decoding for animated images? You can disable it in mini browser from Settings/Enable Animated Image Async Decoding.

If the problem is fixed when async image decoding is disabled, you can disable async image decoding by making ImageSource::canUseAsyncDecoding() returns false for GTK always. This will restore the old behavior till you have a proper fix.

[Michael Catanzaro]

(In reply to Said Abou-Hallawa from comment #4)
> Is the problem fixed if you disable async image decoding for animated
> images? You can disable it in mini browser from Settings/Enable Animated
> Image Async Decoding.

GTK and WPE do not expose that setting.

> If the problem is fixed when async image decoding is disabled, you can
> disable async image decoding by making ImageSource::canUseAsyncDecoding()
> returns false for GTK always. This will restore the old behavior till you
> have a proper fix.

No difference. :/

[Carlos Alberto Lopez Perez]

I have uploaded here some examples of large animations where the problem is much easier to reproduce:


https://people.igalia.com/clopez/wkbug/image_animations_big/

With the 5second version of the gif the first time it plays it takes a lot to load and then it plays really slowly  on the first play.. after it finishes the first play it plays at an ok speed (I guess this is because its slow decoding the image frames into memory or something like that?)


With the 5second version of the apng it plays just fine from the first run.


The 20second version of the gif makes thing much worse and the 20second of the apng causes a crash on the webprocess :(


Feel free to download the images to your disk and you will see it makes no difference to load from local disk, the network speed is not to blame here.

I also uploaded mp4 and webm versions of the animations so you can check how they should play.

[Zan Dobersek]

(In reply to Michael Catanzaro from comment #2)
> http://i.imgur.com/YSxwPUC.gif
> 

As the GIF is being played, newer and newer frames have to be decoded, originating from GIFImageDecoder::frameBufferAtIndex():
http://trac.webkit.org/browser/webkit/trunk/Source/WebCore/platform/image-decoders/gif/GIFImageDecoder.cpp#L104

BitmapImage independently enforces a 30MB limit for decoded image data in destroyDecodedDataIfNecessary(). Once that is hit, the frame buffer cache is wiped out for the BitmapImage's ImageSource and the associated GIFImageDecoder:
http://trac.webkit.org/browser/webkit/trunk/Source/WebCore/platform/graphics/BitmapImage.cpp#L72
http://trac.webkit.org/browser/webkit/trunk/Source/WebCore/platform/graphics/ImageSource.cpp#L66

In GIFImageDecoder::clearFrameBufferCache(), this also wipes out the associated GIFImageReader object:
http://trac.webkit.org/browser/webkit/trunk/Source/WebCore/platform/image-decoders/gif/GIFImageDecoder.cpp#L121

The problem manifests when subsequent GIFImageDecoder::decode() calls, now with a newly-created GIFImageReader, want to decode for GIFFullQuery, and it worsens when doing so for higher-index frames. This specific GIFImageReader::decode() call becomes expensive:
http://trac.webkit.org/browser/webkit/trunk/Source/WebCore/platform/image-decoders/gif/GIFImageDecoder.cpp#L327

After each GIFImageReader re-creation, the decoding in that object has to start over, which is then becoming increasingly expensive the further we go into the GIF playback.

The lightning GIF has 390 frames, and this destroy-and-recreate-GIFImageReader occurrence kicks in approximately every 25-26th frame. For last 5 occurrences (frames 262, 288, 314, 340, 366), this decoding blocks the main thread from ~900 up to ~1100 milliseconds every time, so yeah.

[Carlos Alberto Lopez Perez]

(In reply to Zan Dobersek from comment #7)
> (In reply to Michael Catanzaro from comment #2)
> > http://i.imgur.com/YSxwPUC.gif
> > 
> 
> As the GIF is being played, newer and newer frames have to be decoded,
> originating from GIFImageDecoder::frameBufferAtIndex():
> http://trac.webkit.org/browser/webkit/trunk/Source/WebCore/platform/image-
> decoders/gif/GIFImageDecoder.cpp#L104
> 
> BitmapImage independently enforces a 30MB limit for decoded image data in
> destroyDecodedDataIfNecessary(). Once that is hit, the frame buffer cache is
> wiped out for the BitmapImage's ImageSource and the associated
> GIFImageDecoder:
> http://trac.webkit.org/browser/webkit/trunk/Source/WebCore/platform/graphics/
> BitmapImage.cpp#L72
> http://trac.webkit.org/browser/webkit/trunk/Source/WebCore/platform/graphics/
> ImageSource.cpp#L66
> 
> In GIFImageDecoder::clearFrameBufferCache(), this also wipes out the
> associated GIFImageReader object:
> http://trac.webkit.org/browser/webkit/trunk/Source/WebCore/platform/image-
> decoders/gif/GIFImageDecoder.cpp#L121
> 
> The problem manifests when subsequent GIFImageDecoder::decode() calls, now
> with a newly-created GIFImageReader, want to decode for GIFFullQuery, and it
> worsens when doing so for higher-index frames. This specific
> GIFImageReader::decode() call becomes expensive:
> http://trac.webkit.org/browser/webkit/trunk/Source/WebCore/platform/image-
> decoders/gif/GIFImageDecoder.cpp#L327
> 
> After each GIFImageReader re-creation, the decoding in that object has to
> start over, which is then becoming increasingly expensive the further we go
> into the GIF playback.
> 
> The lightning GIF has 390 frames, and this
> destroy-and-recreate-GIFImageReader occurrence kicks in approximately every
> 25-26th frame. For last 5 occurrences (frames 262, 288, 314, 340, 366), this
> decoding blocks the main thread from ~900 up to ~1100 milliseconds every
> time, so yeah.

I think there are more issues here.

As I only can see an slow play the first time the GIF plays (for any of the examples: lightning or bigbuck5s.gif).

Once the first iteration loop is played, then the second (and posterior) iteration loops play perfectly fine.

I also tested to remove the LargeAnimationCutoff limit and there is no difference. Same behaviour than before.

[Carlos Alberto Lopez Perez]

Another example of a GIF that will be really really slow loading (even from local disk) and slow playing the first loop. Will play fine second and posterior loops: 

http://i.imgur.com/qvKlqC8.gif

For the related fix on Chromium, check https://bugs.chromium.org/p/chromium/issues/detail?id=462733

[Zan Dobersek]

(In reply to Carlos Alberto Lopez Perez from comment #8)
> I think there are more issues here.
> 
> As I only can see an slow play the first time the GIF plays (for any of the
> examples: lightning or bigbuck5s.gif).
> 
> Once the first iteration loop is played, then the second (and posterior)
> iteration loops play perfectly fine.
> 

That's because at this point all the decoding is already done, and there's image data accessible without the need for decoding.

> I also tested to remove the LargeAnimationCutoff limit and there is no
> difference. Same behaviour than before.

Don't remove it. Increase it to a gigabyte and observe there's no stuttering.

[Zan Dobersek]

The m_reader deletion in GIFImageDecoder::clearFrameBufferCache() was introduced in bug #159089:
http://trac.webkit.org/browser/webkit/trunk/Source/WebCore/platform/image-decoders/gif/GIFImageDecoder.cpp#L174

If commented out, stuttering playback of GIFs is fixed. There's also no flickering reintroduced that was described and fixed in bug #159089, but I'd need someone else to double-check that.

[Carlos Alberto Lopez Perez]

(In reply to Michael Catanzaro from comment #0)
> We need to fix playback of imgur .gifv videos. They play super slowly.
> 
> E.g. http://i.imgur.com/YSxwPUC.gifv

Mmmm... is the above resource actually a GIF? ^^ 

At least to me it loads an HTML resource that ends loading a video: http://i.imgur.com/YSxwPUC.mp4 :\


https://help.imgur.com/hc/en-us/articles/208606616-What-is-GIFV-

[Carlos Alberto Lopez Perez]

(In reply to Zan Dobersek from comment #11)
> The m_reader deletion in GIFImageDecoder::clearFrameBufferCache() was
> introduced in bug #159089:
> http://trac.webkit.org/browser/webkit/trunk/Source/WebCore/platform/image-
> decoders/gif/GIFImageDecoder.cpp#L174
> 
> If commented out, stuttering playback of GIFs is fixed. There's also no
> flickering reintroduced that was described and fixed in bug #159089, but I'd
> need someone else to double-check that.

I tested this and I can confirm it fixes the slow playback on the first loop.
This is what I tested:

$ cd /tmp
$ mkdir giftests
$ wget http://i.imgur.com/qvKlqC8.gif -O kangaroos.gif
$ wget https://people.igalia.com/clopez/wkbug/image_animations_big/bigbuck5s.gif
$ wget https://people.igalia.com/clopez/wkbug/image_animations_big/bigbuck20s.gif
$ ls -sh1
total 187M
88M bigbuck20s.gif
18M bigbuck5s.gif
82M kangaroos.gif


Then load GTK MiniBrowser with url file:///tmp/giftests and start measuring time and memory once clicked on the gif

Currently:

kangaroos:  7350 MB WebProcess memory usage --  41sec to load from disk -- 25 sec first loop --- 15 sec second lop
bigbuck5s:   515 MB WebProcess memory usage --  2.5sec to load from disk -- 7 sec first loop --- 5 sec second lop
bigbuck20s: 8500 MB WebProcess memory usage --  48sec to load from disk -- 54 sec first loop --- 20 sec second lop


After commenting this out:

kangaroos:  similar memory usage, same time to load from disk, *but* it plays fine on the first loop (15 seconds) and without glitch
bigbuck5s:  similar memory usage, same time to load from disk, *but* it plays fine on the first loop (5 seconds) and without glitch
bigbuck20s: similar memory usage, same time to load from disk, *but* it plays fine on the first loop (20 seconds) and without glitch
 


So I think we have at least 3 bugs:

 - 1) Slow playback of GIFs on first loop (that seems to be fixed by your suggested fix of commenting out the deletion of the m_reader)
 - 2) Slow decoding of the GIFs (even when loading from disk)
 - 3) Huge memory usage of long GIFs.

[Carlos Alberto Lopez Perez]

Created attachment 321810 [details]
PoC patch based on https://crbug.com/462733

I'm uploading (not for final review, still comments about it very much welcomed), a PoC patch based on https://crbug.com/462733

With this (and plus the fix of commenting the reader deletion ttps://bugs.webkit.org/show_bug.cgi?id=159089 ) I get this results:

kangaroos:   690 MB WebProcess memory usage --  <1sec to load from disk -- 15 sec first loop --- 15 sec second lop
bigbuck5s:   300 MB WebProcess memory usage --  <1sec to load from disk -- 5 sec first loop --- 5 sec second lop
bigbuck20s: 1030 MB WebProcess memory usage --  <1sec to load from disk -- 20 sec first loop --- 20 sec second lop


So looks like almost fixed.

But there is like some bugs on the current patch, as the GIF sometimes (1 every 5 tries) doesn't play on the first load and has to be re-tried until it plays

[Build Bot]

Attachment 321810 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/image-decoders/FastSharedBufferReader.h:82:  Could not find a newline character at the end of the file.  [whitespace/ending_newline] [5]
ERROR: Tools/TestWebKitAPI/Tests/WebCore/FastSharedBufferReaderTest.cpp:34:  Found header this file implements after other header. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
ERROR: Tools/TestWebKitAPI/Tests/WebCore/FastSharedBufferReaderTest.cpp:36:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Tools/TestWebKitAPI/Tests/WebCore/FastSharedBufferReaderTest.cpp:101:  One line control clauses should not use braces.  [whitespace/braces] [4]
ERROR: Source/WebCore/platform/image-decoders/gif/GIFImageReader.h:0:  No copyright message found.  You should have a line: "Copyright [year] <Copyright Owner>"  [legal/copyright] [5]
ERROR: Source/WebCore/platform/image-decoders/gif/GIFImageReader.h:44:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/WebCore/platform/image-decoders/FastSharedBufferReader.cpp:31:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
ERROR: Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:0:  No copyright message found.  You should have a line: "Copyright [year] <Copyright Owner>"  [legal/copyright] [5]
ERROR: Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:506:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:511:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Source/WebCore/ChangeLog:10:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
Total errors found: 11 in 10 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Miguel Gomez]

> So I think we have at least 3 bugs:
> 
>  - 1) Slow playback of GIFs on first loop (that seems to be fixed by your
> suggested fix of commenting out the deletion of the m_reader)
>  - 2) Slow decoding of the GIFs (even when loading from disk)
>  - 3) Huge memory usage of long GIFs.

Without checking this concrete issue carefully (maybe this changed lately), some comments.

- The image decoding is not something that happens once. When enough decoded images are stored and they reach a concrete size, stored frames will be deleted to keep the size under control. Due to this it can happen that frames are not requested in the common order. This is the reason behind the destruction of m_reader, which would expect to be requested the next frame in the animation but it can actually be requested a former frame, and that causes that wrong frames are being drawn. I'm not sure whether we can somehow reposition the reader instead of destroying it, we need to check that.

- Regarding the memory usage, there should be this limit I mentioned to avoid it (the LargeAnimationCutoff metioned by Zan). If you remove it, then all the decoded frames are stored and that makes the second play smoother, but consumes a lot of memory on large animations. Also, during the first play, there won't be requests to delete decoded frames, which means that the m_reader won't be destroyed and the decoding will be faster.

[Carlos Alberto Lopez Perez]

(In reply to Miguel Gomez from comment #16)

> 
> - Regarding the memory usage, there should be this limit I mentioned to
> avoid it (the LargeAnimationCutoff metioned by Zan). If you remove it, then
> all the decoded frames are stored and that makes the second play smoother,
> but consumes a lot of memory on large animations. Also, during the first
> play, there won't be requests to delete decoded frames, which means that the
> m_reader won't be destroyed and the decoding will be faster.

I didn't removed the LargeAnimationCutoff for any of the tests I posted above.

Current WebKitGTK+ trunk is using 8GB of RAM to play the images of kangaroos or bigbuck20s on my laptop.

[Michael Catanzaro]

(In reply to Carlos Alberto Lopez Perez from comment #12)
> (In reply to Michael Catanzaro from comment #0)
> > We need to fix playback of imgur .gifv videos. They play super slowly.
> > 
> > E.g. http://i.imgur.com/YSxwPUC.gifv
> 
> Mmmm... is the above resource actually a GIF? ^^ 
> 
> At least to me it loads an HTML resource that ends loading a video:
> http://i.imgur.com/YSxwPUC.mp4 :\
> 
> 
> https://help.imgur.com/hc/en-us/articles/208606616-What-is-GIFV-

Use the .gif URL in comment #2.

[Miguel Gomez]

(In reply to Zan Dobersek from comment #7)
> (In reply to Michael Catanzaro from comment #2)
> > http://i.imgur.com/YSxwPUC.gif
> > 
> 
> As the GIF is being played, newer and newer frames have to be decoded,
> originating from GIFImageDecoder::frameBufferAtIndex():
> http://trac.webkit.org/browser/webkit/trunk/Source/WebCore/platform/image-
> decoders/gif/GIFImageDecoder.cpp#L104
> 
> BitmapImage independently enforces a 30MB limit for decoded image data in
> destroyDecodedDataIfNecessary(). Once that is hit, the frame buffer cache is
> wiped out for the BitmapImage's ImageSource and the associated
> GIFImageDecoder:
> http://trac.webkit.org/browser/webkit/trunk/Source/WebCore/platform/graphics/
> BitmapImage.cpp#L72
> http://trac.webkit.org/browser/webkit/trunk/Source/WebCore/platform/graphics/
> ImageSource.cpp#L66
> 
> In GIFImageDecoder::clearFrameBufferCache(), this also wipes out the
> associated GIFImageReader object:
> http://trac.webkit.org/browser/webkit/trunk/Source/WebCore/platform/image-
> decoders/gif/GIFImageDecoder.cpp#L121
> 
> The problem manifests when subsequent GIFImageDecoder::decode() calls, now
> with a newly-created GIFImageReader, want to decode for GIFFullQuery, and it
> worsens when doing so for higher-index frames. This specific
> GIFImageReader::decode() call becomes expensive:
> http://trac.webkit.org/browser/webkit/trunk/Source/WebCore/platform/image-
> decoders/gif/GIFImageDecoder.cpp#L327
> 
> After each GIFImageReader re-creation, the decoding in that object has to
> start over, which is then becoming increasingly expensive the further we go
> into the GIF playback.
> 
> The lightning GIF has 390 frames, and this
> destroy-and-recreate-GIFImageReader occurrence kicks in approximately every
> 25-26th frame. For last 5 occurrences (frames 262, 288, 314, 340, 366), this
> decoding blocks the main thread from ~900 up to ~1100 milliseconds every
> time, so yeah.

Perfectly explained Zan.

Currently the GIFImageReader cannot decode a frame that was already decoded. So when older frames get deleted from the cache and they have to be decoded again, the reader won't decode anything. This is why when clearing the cache the reader is being deleted as well, so it's able to re-decode already decoded frames. Without the reader deletion, if older frames are removed from the cache, when they are requested again to the reader nothing is returned from the decoder.

But as Zan explains, everytime the reader is re-created, it has to parse the whole animation data, and then it decodes all the frames until the requested one, and this gets more and more expensive as the animation grows.

I think I found a way to rewind the reader to be able to decode older frames without having to delete it. Let me test it carefully and I'll upload the patch.

[Miguel Gomez]

Created attachment 322524 [details]
Patch

[Build Bot]

Attachment 322524 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:0:  No copyright message found.  You should have a line: "Copyright [year] <Copyright Owner>"  [legal/copyright] [5]
Total errors found: 1 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Commit Bot]

Comment on attachment 322524 [details]
Patch

Clearing flags on attachment: 322524

Committed r222836: <http://trac.webkit.org/changeset/222836>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/34808431>