We need to fix playback of imgur .gifv videos. They play super slowly. E.g. http://i.imgur.com/YSxwPUC.gifv
I can't reproduce it
(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.
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.
(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.
(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. :/
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.
(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.
(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.
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
(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.
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.
(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-
(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.
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
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.
> 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.
(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.
(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.
(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.
Created attachment 322524 [details] Patch
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.
Comment on attachment 322524 [details] Patch Clearing flags on attachment: 322524 Committed r222836: <http://trac.webkit.org/changeset/222836>
All reviewed patches have been landed. Closing bug.
<rdar://problem/34808431>