Bug 139599 - fast/images/animated-gif-body-outside-viewport.html is flaky
Summary: fast/images/animated-gif-body-outside-viewport.html is flaky
Status: RESOLVED WONTFIX
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Chris Dumez
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-12-12 15:01 PST by Alexey Proskuryakov
Modified: 2015-01-06 14:37 PST (History)
6 users (show)

See Also:


Attachments
WIP Patch (2.94 KB, patch)
2014-12-12 19:30 PST, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (4.04 KB, patch)
2014-12-12 22:54 PST, Chris Dumez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alexey Proskuryakov 2014-12-12 15:01:18 PST
This test randomly fails on several Mac WK1 bots:

http://webkit-test-results.appspot.com/dashboards/flakiness_dashboard.html#tests=fast%2Fimages%2Fanimated-gif-body-outside-viewport.html

-PASS isBackgroundAnimated is false
+FAIL isBackgroundAnimated should be false. Was true.
Comment 1 Alexey Proskuryakov 2014-12-12 15:03:38 PST
Marked as flaky in r177239.
Comment 2 Chris Dumez 2014-12-12 19:30:46 PST
Created attachment 243241 [details]
WIP Patch
Comment 3 Chris Dumez 2014-12-12 22:54:12 PST
Created attachment 243247 [details]
Patch
Comment 4 WebKit Commit Bot 2014-12-14 11:01:58 PST
Comment on attachment 243247 [details]
Patch

Clearing flags on attachment: 243247

Committed r177265: <http://trac.webkit.org/changeset/177265>
Comment 5 WebKit Commit Bot 2014-12-14 11:02:04 PST
All reviewed patches have been landed.  Closing bug.
Comment 7 Chris Dumez 2014-12-16 20:38:29 PST
Hmm, it seems that this is an actual failure on WK1. On WebKit1, it seems FrameView::paintsEntireContents() return true. As a result, FrameView::windowClipRect() returns [0, 0, 1600, 1200] (this is the size of the content, *including* what is not visible) instead of the expected [0, 0, 800, 600]. This means we don't currently don't always properly detect if animated gifs are outside the viewport on WK1.
Comment 8 Chris Dumez 2014-12-16 20:41:21 PST
(In reply to comment #7)
> Hmm, it seems that this is an actual failure on WK1. On WebKit1, it seems
> FrameView::paintsEntireContents() return true. As a result,
> FrameView::windowClipRect() returns [0, 0, 1600, 1200] (this is the size of
> the content, *including* what is not visible) instead of the expected [0, 0,
> 800, 600]. This means we don't currently don't always properly detect if
> animated gifs are outside the viewport on WK1.

Looks like this was done by Andreas a long time ago:
https://bugs.webkit.org/show_bug.cgi?id=49375
Comment 9 Simon Fraser (smfr) 2014-12-16 20:56:55 PST
(In reply to comment #7)
> Hmm, it seems that this is an actual failure on WK1. On WebKit1, it seems
> FrameView::paintsEntireContents() return true. As a result,

I think paintsEntireContents() should only be true in a layer-backed WK1 view (if the WebHTMLView has its own layer).
Comment 10 Chris Dumez 2014-12-16 21:12:50 PST
(In reply to comment #9)
> (In reply to comment #7)
> > Hmm, it seems that this is an actual failure on WK1. On WebKit1, it seems
> > FrameView::paintsEntireContents() return true. As a result,
> 
> I think paintsEntireContents() should only be true in a layer-backed WK1
> view (if the WebHTMLView has its own layer).

Hmm, it seems to be true when I run MiniBrowser:
Tools/Scripts/run-minibrowser --debug

However, it seems to be false when running the layout tests so this is not the source of the failure.
Comment 11 Chris Dumez 2014-12-16 21:26:15 PST
Hmm, what seems to happen locally (with WK1 DumpRenderTree) is that RenderElement::newImageAnimationFrameAvailable() never gets called. As a result, RenderView::addRendererWithPausedImageAnimations() is never called and we never set the 'hasPausedImageAnimations' flag on the renderer.

Does anyone have an idea why newImageAnimationFrameAvailable() wouldn't get called with WK1/DRT? The function gets called on WK2/WKTR and the test passes just fine there.
Comment 12 Chris Dumez 2014-12-16 21:58:30 PST
I found an interesting comment in RenderLayerBacking::updateImageContents():
// Image animation is "lazy", in that it automatically stops unless someone is drawing
// the image. So we have to kick the animation each time; this has the downside that the
// image will keep animating, even if its layer is not visible.
image->startAnimation();


Is WK1 using RenderLayerBacking? If not, then it is likely the image is not being animated outside the viewport simply because it is not being drawn.

In this case, this would mean that Antti's optimization only makes sense on WK2. That would make my animated GIF layout tests wk2-specific.
Comment 13 Simon Fraser (smfr) 2014-12-17 09:24:59 PST
MiniBrowser does layer-backing on Yosemite, because it has a blurred toolbar.
Comment 14 Chris Dumez 2014-12-17 10:01:20 PST
(In reply to comment #12)
> I found an interesting comment in RenderLayerBacking::updateImageContents():
> // Image animation is "lazy", in that it automatically stops unless someone
> is drawing
> // the image. So we have to kick the animation each time; this has the
> downside that the
> // image will keep animating, even if its layer is not visible.
> image->startAnimation();
> 
> 
> Is WK1 using RenderLayerBacking? If not, then it is likely the image is not
> being animated outside the viewport simply because it is not being drawn.
> 
> In this case, this would mean that Antti's optimization only makes sense on
> WK2. That would make my animated GIF layout tests wk2-specific.

+dino in cc as it seems he added this comment.
Comment 15 Chris Dumez 2015-01-06 14:37:27 PST
Animated image throttling behaves differently on WebKit1 than on WebKit2 and our layout test infrastructure does not work for WebKit1. This test is WK2 specific. I recently skipped it on WK1.