Description
Fujii Hironori
2017-06-07 22:15:23 PDT
I saw this again by reloading http://www.nicovideo.jp/ again and again. http://ads.nicovideo.jp/assets/images/06/065300e6d96bcb502826d5b4c3f72f70.gif is the image url. Created attachment 312284 [details] big size animation gif (1000x1000, loop count:1) This happens always when painting a big finished animation GIF image. > bool ImageSource::shouldUseAsyncDecoding() > { > if (!isDecoderAvailable()) > return false; > // FIXME: figure out the best heuristic for enabling async image decoding. > return size().area() * sizeof(RGBA32) >= (frameCount() > 1 ? 100 * KB : 500 * KB); > } Created attachment 312286 [details]
WIP patch
Created attachment 312639 [details]
Patch
Comment on attachment 312639 [details] Patch Attachment 312639 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/3916346 New failing tests: fast/images/animated-gif-paint-after-animation.html Created attachment 312643 [details]
Archive of layout-test-results from ews100 for mac-elcapitan
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Comment on attachment 312639 [details] Patch Attachment 312639 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/3916358 New failing tests: fast/images/animated-gif-paint-after-animation.html Created attachment 312644 [details]
Archive of layout-test-results from ews114 for mac-elcapitan
The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews114 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Created attachment 312648 [details]
Patch
Comment on attachment 312648 [details] Patch Attachment 312648 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/3916694 New failing tests: fast/images/animated-gif-paint-after-animation.html Created attachment 312650 [details]
Archive of layout-test-results from ews101 for mac-elcapitan
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Comment on attachment 312648 [details] Patch Attachment 312648 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/3916752 New failing tests: fast/images/animated-gif-paint-after-animation.html Created attachment 312652 [details]
Archive of layout-test-results from ews114 for mac-elcapitan
The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews114 Port: mac-elcapitan Platform: Mac OS X 10.11.6
What's happening in mac and mac-debug EWS? There is a comment in TestExpectations of mac-wk1: https://trac.webkit.org/browser/webkit/trunk/LayoutTests/platform/mac-wk1/TestExpectations?rev=217880#L123 > # Animated image throttling behaves differently on WK1. > fast/images/animated-gif-body-outside-viewport.html [ Skip ] > fast/images/animated-gif-window-resizing.html [ Skip ] > fast/images/animated-gif-zooming.html [ Skip ] I need to do the following trick as other test cases do.
> // Force layout and display so the image frame starts decoding.
> document.body.offsetHeight;
> testRunner.display();
Created attachment 312753 [details]
Patch
Created attachment 312847 [details]
Patch
Could you review, Said? Could anyone review my patch? Comment on attachment 312847 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=312847&action=review > Source/WebCore/ChangeLog:8 > + Test: fast/images/animated-gif-paint-after-animation.html Please move this line after the comment below. > Source/WebCore/platform/graphics/BitmapImage.cpp:-191 > - ASSERT(!canAnimate() && !m_currentFrame); You can use: ASSERT(!canAnimate() && (!m_currentFrame || m_animationFinished)); > LayoutTests/fast/images/animated-gif-paint-after-animation.html:12 > + window.onload = function() { Instead of listening to the onload event you can move all the script at the end of <body> tag. > LayoutTests/fast/images/animated-gif-paint-after-animation.html:15 > + img.src = 'resources/animated-red-green-blue-1000x1000-repeat-1.gif'; Usually setting the image source comes after defining the onload function: img.onload = function() { .... } img.src = "...". > LayoutTests/fast/images/animated-gif-paint-after-animation.html:25 > + window.setTimeout(function() { > + testRunner.notifyDone(); > + }, 500); Instead of using the css animation and the setTimeout(), I think this can be replaced by the following: // Force the page to be displayed. The image element is already invalidated when the frame finished decoding. testRunner.display(); // Change the width of the img element img.width = "100" // Force a final layout and display so the image last frame is drawn. document.body.offsetHeight; testRunner.display(); testRunner.notifyDone(); > LayoutTests/fast/images/animated-gif-paint-after-animation.html:34 > + @keyframes n { > + from { width : 10px; } > + to { width: 20px; } > + } Why do you need to add this animation? I think you want to force a last redraw after the last frame is drawn. You can change the width of the element manually after receiving the webkitImageFrameReady event; see above. > LayoutTests/fast/images/animated-gif-paint-after-animation.html:36 > + display: block; Is setting the display to "block" really needed for the test? Comment on attachment 312847 [details]
Patch
Thank you very much, Said. I'll revise the patch.
Created attachment 313992 [details]
Patch
Comment on attachment 313992 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=313992&action=review > LayoutTests/fast/images/animated-gif-paint-after-animation.html:31 > + window.setTimeout(function() { > + // Force a final layout and display so the image last frame is drawn. > + img.width = "100"; > + if (window.testRunner) > + testRunner.notifyDone(); > + }, 500); Using the setTimeout() makes the test non deterministic. There is no guarantee here that the image will draw its last frame twice in 500ms. Also the 500ms is way too long period to wait for. As I said earlier, you can control the layout and display yourself so you can know exactly when the test should end: // Force the page to be displayed. The image element is already invalidated when the frame finished decoding. testRunner.display(); // Change the width of the img element img.width = "100"; // notifyDone() will force the last layout and display. testRunner.notifyDone(); Thank you for the review. It needs to wait at least 30ms for finishing the animation. I've found the way without setTimeout by counting decoded frames. Created attachment 314113 [details]
Patch
Comment on attachment 314113 [details] Patch Attachment 314113 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/4018364 New failing tests: fast/images/animated-gif-paint-after-animation.html Created attachment 314119 [details]
Archive of layout-test-results from ews117 for mac-elcapitan
The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews117 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Comment on attachment 314113 [details] Patch Attachment 314113 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/4020855 New failing tests: fast/images/animated-gif-paint-after-animation.html Created attachment 314150 [details]
Archive of layout-test-results from ews103 for mac-elcapitan
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103 Port: mac-elcapitan Platform: Mac OS X 10.11.6
(In reply to Fujii Hironori from comment #24) > Thank you for the review. > It needs to wait at least 30ms for finishing the animation. > I've found the way without setTimeout by counting decoded frames. I was assuming your animated image has only two frames. Just delete the last frame of the image and have it only contain two frames: red and green. Then your code should be simpler img.addEventListener("webkitImageFrameReady", function() { // Force the page to be displayed. The image element is already invalidated when the frame finished decoding. if (window.testRunner) testRunner.display(); // Change the width of the img element img.width = "100"; // notifyDone() will force the last layout and display. if (window.testRunner) testRunner.notifyDone(); }); img.src = 'resources/animated-red-green-1000x1000-repeat-1.gif'; Also make sure the test asserts without your patch. (In reply to Build Bot from comment #29) > Created attachment 314150 [details] > Archive of layout-test-results from ews103 for mac-elcapitan > > The attached test failures were seen while running run-webkit-tests on the > mac-ews. > Bot: ews103 Port: mac-elcapitan Platform: Mac OS X 10.11.6 The test is timing out because DRT does not force drawing when a repaint happens. Instead DRT waits for notifyDone() to do the last draw. Your code relies on having the event webkitImageFrameReady is called two times and the image is drawn four times: 1st time: draws frame 0 and request frame 1 for decoding. 2nd time: draws frame 1 and request frame 2 for decoding. 3nd time: draws frame 2. 4th time: draws frame 2. Only the first draw happens because onload calls testRunner.display(). The second and the third draws do not happen because webkitImageFrameReady callback returns when count != 2. And webkitImageFrameReady can't fire unless a draw happens. The fourth time does not happen because testRunner.notifyDone() is not called. Changing the image to have two frames only and changing the webkitImageFrameReady callback as I mentioned above should draw the image three times and testRunner.notifyDone() is guaranteed to be called. How to create an animation gif with ImageMagick convert -size 1000x1000 xc:red red.png convert -size 1000x1000 xc:green green.png convert -delay 1 -loop 1 red.png green.png animated-red-green-1000x1000-repeat-1.gif Created attachment 314235 [details]
Patch
I hit this assertion on macOS today. Comment on attachment 314235 [details] Patch Attachment 314235 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/4024594 New failing tests: fast/workers/worker-exception-during-navigation.html Created attachment 314239 [details]
Archive of layout-test-results from ews107 for mac-elcapitan-wk2
The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107 Port: mac-elcapitan-wk2 Platform: Mac OS X 10.11.6
Comment on attachment 314239 [details] Archive of layout-test-results from ews107 for mac-elcapitan-wk2 (In reply to Build Bot from comment #35) > Attachment 314235 [details] did not pass mac-wk2-ews (mac-wk2): > Output: http://webkit-queues.webkit.org/results/4024594 > New failing tests: > fast/workers/worker-exception-during-navigation.html This EWS failure seems Bug 102131. Comment on attachment 314235 [details] Patch Clearing flags on attachment: 314235 Committed r219003: <http://trac.webkit.org/changeset/219003> All reviewed patches have been landed. Closing bug. |