WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED DUPLICATE of
bug 128632
112327
Reorganize image animation starting
https://bugs.webkit.org/show_bug.cgi?id=112327
Summary
Reorganize image animation starting
Nandor Huszka
Reported
2013-03-14 01:33:09 PDT
By using TiledBackingStore we prepaint things that aren't visible yet. So a gif is also repainted in every animation step regardless of its actual visibility. The main idea is add a new ability to TBS: it should know the animated gifs inside tiles. A concept how to achieve this: 1. don't paint an invisible gif * animation repainting stops (but the frame counter runs) * 2. ask TBS to remember the gif's bounding box, as "pseudo-dirty" area 3. TBS splits up this area between tiles, they store the appropriate rectangles * user moves the viewport onto the invisible gif * 4. TBS traverse tiles that are visible 5. every travered tile unites its dirty rect with stored "pseudo-dirty" areas that have intersection with viewport then forget it * gif is repainted (at the correct frame), because dirty rects are repainted, animation continues * A "pseudo-dirty" area won't cause painting, just if it becames visible. We could use this solution not only with gifs but with any animated elements. Any idea or comment to make it better is appreciated.
Attachments
Draft patch
(13.37 KB, patch)
2013-03-14 01:37 PDT
,
Nandor Huszka
no flags
Details
Formatted Diff
Diff
WIP patch - Approach: animations are started by FrameView
(10.08 KB, patch)
2013-04-03 01:33 PDT
,
Nandor Huszka
no flags
Details
Formatted Diff
Diff
WIP patch v2 - Approach: animations are started by FrameView
(13.50 KB, patch)
2013-04-10 02:53 PDT
,
Nandor Huszka
no flags
Details
Formatted Diff
Diff
WIP patch v3 - Approach: animations are started by FrameView
(14.36 KB, patch)
2013-04-17 01:26 PDT
,
Nandor Huszka
buildbot
: commit-queue-
Details
Formatted Diff
Diff
Archive of layout-test-results from webkit-ews-05 for mac-mountainlion
(805.54 KB, application/zip)
2013-04-17 15:19 PDT
,
Build Bot
no flags
Details
Show Obsolete
(3)
View All
Add attachment
proposed patch, testcase, etc.
Nandor Huszka
Comment 1
2013-03-14 01:37:37 PDT
Created
attachment 193086
[details]
Draft patch A draft patch with more comments. Planned improvements: access TiledBackingStore through Frame, smarter pseudo-dirty rect handler algorithm, dealing with image's bounding box instead of its rect.
Jocelyn Turcotte
Comment 2
2013-03-19 07:10:46 PDT
Is there a reason why this should apply to animated GIFs but not to general element animations? From my point of view a more general solution not only applying to images would be simpler and easier to maintain. For example we could take an approach where an invalidated rect won't trigger an update of an existing tile unless it intersect with the viewport visible rect (because we absolutely need all tiles to be in sync with the content currently visible to the user to prevent tearing between tiles). The accumulated dirty rect could be repainted the moment that the user pans the area into the viewport and resumes the page. This is just a quick idea though, this might lead to issues requiring something more complex.
Noam Rosenthal
Comment 3
2013-03-19 07:15:10 PDT
(In reply to
comment #2
)
> Is there a reason why this should apply to animated GIFs but not to general element animations? > > From my point of view a more general solution not only applying to images would be simpler and easier to maintain. > > For example we could take an approach where an invalidated rect won't trigger an update of an existing tile unless it intersect with the viewport visible rect (because we absolutely need all tiles to be in sync with the content currently visible to the user to prevent tearing between tiles). > The accumulated dirty rect could be repainted the moment that the user pans the area into the viewport and resumes the page. > > This is just a quick idea though, this might lead to issues requiring something more complex.
This problem is very specific to GIF animations. Since according to spec a GIF animation should animate on visibility, and a CSS/SVG animation starts programatically.
Allan Sandfeld Jensen
Comment 4
2013-03-19 07:19:39 PDT
(In reply to
comment #3
)
> (In reply to
comment #2
) > > Is there a reason why this should apply to animated GIFs but not to general element animations? > > > > From my point of view a more general solution not only applying to images would be simpler and easier to maintain. > > > > For example we could take an approach where an invalidated rect won't trigger an update of an existing tile unless it intersect with the viewport visible rect (because we absolutely need all tiles to be in sync with the content currently visible to the user to prevent tearing between tiles). > > The accumulated dirty rect could be repainted the moment that the user pans the area into the viewport and resumes the page. > > > > This is just a quick idea though, this might lead to issues requiring something more complex. > > This problem is very specific to GIF animations. Since according to spec a GIF animation should animate on visibility, and a CSS/SVG animation starts programatically.
There are other things that start on visibility (many flash plugins for instance). The GIF one is just a particular nasty hack, since it is started on paint, not on visibility. It would be nice to clean up that hack instead of working around it.
Zoltan Herczeg
Comment 5
2013-03-19 07:22:21 PDT
I don't see any problem to extend it to anything. The idea is designed that way. First we start with GIF animations, as they are straitforward. However, if we allow too many dirty regions, we end up having a fully dirty backbuffer, which defeats the purpose of the backing store. We could simply cache the visible rect and get the same effect, because any scroll operation would cause an invalidation.
Allan Sandfeld Jensen
Comment 6
2013-03-21 04:24:25 PDT
(In reply to
comment #5
)
> I don't see any problem to extend it to anything. The idea is designed that way. First we start with GIF animations, as they are straitforward. > > However, if we allow too many dirty regions, we end up having a fully dirty backbuffer, which defeats the purpose of the backing store. We could simply cache the visible rect and get the same effect, because any scroll operation would cause an invalidation.
My idea was more to do it using a different mechanic. We also have the problem with restarting the GIF animations after suspend. We currently solve that by doing a full repaint when resuming. We could save that full repaint if we had a better way of starting GIF animations. If something other than draw would start the animation when it enters the visible rect, then we could even pause the animation again when the users scrolls them out. Currently they are started when first visible, but never stopped. Stopping an animation is simple you just return false in RenderObject::willRenderImage. It could simply check if the render object is in the visible rect. The reason it isn't done is because they are not restarted automatically. If something (in frameview for instance), could track all animated images, and start them when they intersect the viewport, it would solve not only your specific issue here but also allow many other improvements. Starting gif animations is not just broken for you, but broken everywhere ;)
Nandor Huszka
Comment 7
2013-04-03 01:33:00 PDT
Created
attachment 196299
[details]
WIP patch - Approach: animations are started by FrameView I have implemented Allan's idea. The patch needs some fix in respect of animation resuming, but it works well in generally. Only BitmapImageCairo got rid of animation starting, but it seems we also need to remove every startAnimation call from other ports (because it is done by FrameView now). What do you think about it?
Allan Sandfeld Jensen
Comment 8
2013-04-04 03:42:19 PDT
(In reply to
comment #7
)
> Created an attachment (id=196299) [details] > WIP patch - Approach: animations are started by FrameView > > I have implemented Allan's idea. The patch needs some fix in respect of animation resuming, but it works well in generally. > Only BitmapImageCairo got rid of animation starting, but it seems we also need to remove every startAnimation call from other ports (because it is done by FrameView now). > What do you think about it?
Great. You need to make sure the lists are not linking to dead RenderObjects, so remove them when RenderObjects die. Second you need to remove the all the code starting animations in BitmapImage to be able to test if this really does start them correctly.
Nandor Huszka
Comment 9
2013-04-10 02:53:57 PDT
Created
attachment 197232
[details]
WIP patch v2 - Approach: animations are started by FrameView (In reply to
comment #8
)
> Great. You need to make sure the lists are not linking to dead RenderObjects, so remove them when RenderObjects die. Second you need to remove the all the code starting animations in BitmapImage to be able to test if this really does start them correctly.
Code is refactored and a newer patch is uploaded that fixes these things. Do you think it is good idea to rename Image's startAnimation to continueAnimation? In my opinion this name is more expressive considering that the method does (and this way we could check where are remaining startAnimation calls that should be removed from other ports because of build fails). As I have browsed the source, I found comments that often refer to draw that it starts animation. These comments also should be rewrited, or removed in the future, I think.
Allan Sandfeld Jensen
Comment 10
2013-04-12 14:02:30 PDT
Please note
r148300
just landed for
bug 108864
. It changes how gif animations are restarted after having been suspended.
Nandor Huszka
Comment 11
2013-04-17 01:26:30 PDT
Created
attachment 198478
[details]
WIP patch v3 - Approach: animations are started by FrameView (In reply to
comment #10
)
> Please note
r148300
just landed for
bug 108864
. It changes how gif animations are restarted after having been suspended.
I've checked this patch, and reused its solution, thanks. With the freshest WIP patch there are about 10 crashes on Qt because of ASSERT(frame()->view()); It seems in some cases there isn't a FrameView under this reference when a RenderObject is dying. Maybe AnimationController::cancelAnimations would be the correct place for deleting dying animations from FrameView? It is called also in RenderObject::willBeDestroyed. FYI: a mail will be sent to webkit-dev about it with subject "Reorganization of animation starting".
Simon Fraser (smfr)
Comment 12
2013-04-17 09:31:33 PDT
I think this bug's title and the patch need some renaming to clarify that they refer to image animations, not CSS animations or requestAnimationFrame.
Simon Fraser (smfr)
Comment 13
2013-04-17 09:39:52 PDT
Comment on
attachment 198478
[details]
WIP patch v3 - Approach: animations are started by FrameView View in context:
https://bugs.webkit.org/attachment.cgi?id=198478&action=review
I'm not sure I agree with this approach in general. It doesn't play nicely with threaded scrolling on Mac, for example; unless we test for restarting image animations while scrolling, we'll not see animated images when the threaded scrolling is happening.
> Source/WebCore/page/FrameView.cpp:4094 > + stoppedAnimationBoundingBox = stoppedAnimation->absoluteBoundingBoxRect();
absoluteBoundingBoxRect() can be a fairly expensive call. I'm a bit concerned about the performance implications.
> Source/WebCore/page/FrameView.cpp:4110 > + animationToResumeBoundingBox = animationToResume->absoluteBoundingBoxRect();
Why compute it twice?
> Source/WebCore/page/FrameView.cpp:4153 > +IntRect FrameView::computeScrolledViewPort(float deltaX, float deltaY) > +{ > + IntRect viewPort = visibleContentRect(); > + int newX = viewPort.x() - deltaX; > + int newY = viewPort.y() - deltaY; > + IntRect scrolledViewPort(IntPoint(newX, newY), viewPort.size()); > + > + return scrolledViewPort;
Isn't this the same as visibleContentRect() or similar?
Antti Koivisto
Comment 14
2013-04-17 10:39:06 PDT
Comment on
attachment 198478
[details]
WIP patch v3 - Approach: animations are started by FrameView View in context:
https://bugs.webkit.org/attachment.cgi?id=198478&action=review
The basic idea of not animating on speculative tiles is good. This just needs a good universal and well-performing mechanism for restarting them when they come to view. I wonder if it would be better at some point to make GIF animations more like other animations (driven by self-repeating timer) rather than doing the restart-timer-on-paint trick? The main benefit of that approach is the automatic stopping and restarting but if we need viewport checks and explicit restarting anyway I'm not sure if it is really helpful.
> Source/WebCore/page/FrameView.h:383 > + bool shouldRenderAnimation(RenderObject*); > + void animateLater(RenderObject*); > + void removeSavedAnimation(RenderObject*);
FrameView is bloated. It would be better to factor this functionality to a class (ImageAnimationController?)
Build Bot
Comment 15
2013-04-17 15:19:15 PDT
Comment on
attachment 198478
[details]
WIP patch v3 - Approach: animations are started by FrameView
Attachment 198478
[details]
did not pass mac-ews (mac): Output:
http://webkit-queues.appspot.com/results/71339
New failing tests: fast/loader/image-in-page-cache.html loader/go-back-to-different-window-size.html fast/backgrounds/animated-svg-as-mask.html fast/frames/frame-dead-region.html fast/harness/results.html fast/loader/stateobjects/pushstate-clears-forward-history.html fast/harness/user-preferred-language.html fast/dom/Geolocation/not-enough-arguments.html
Build Bot
Comment 16
2013-04-17 15:19:18 PDT
Created
attachment 198618
[details]
Archive of layout-test-results from webkit-ews-05 for mac-mountainlion The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: webkit-ews-05 Port: mac-mountainlion Platform: Mac OS X 10.8.2
Nandor Huszka
Comment 17
2013-04-18 09:01:54 PDT
(In reply to
comment #13
)
> I'm not sure I agree with this approach in general. It doesn't play nicely with threaded scrolling on Mac, for example; unless we test for restarting image animations while scrolling, we'll not see animated images when the threaded scrolling is happening.
I'm not familiar with threaded scrolling. Do you mean we should take care of image animations in layout tests after scrolling? Please describe this problem with more details. If this approach is completely no-go, what do you think about the one that Antti mentioned, would that cooperates well with threaded scrolling?
> > Source/WebCore/page/FrameView.cpp:4094 > > + stoppedAnimationBoundingBox = stoppedAnimation->absoluteBoundingBoxRect(); > > absoluteBoundingBoxRect() can be a fairly expensive call. I'm a bit concerned about the performance implications. > > > Source/WebCore/page/FrameView.cpp:4110 > > + animationToResumeBoundingBox = animationToResume->absoluteBoundingBoxRect(); > > Why compute it twice?
It's true, the rect of animations could be stored. I think animations' bounding boxes could be gathered in an other way in order to not to increase run time significantly.
> > Source/WebCore/page/FrameView.cpp:4153 > > +IntRect FrameView::computeScrolledViewPort(float deltaX, float deltaY) > Isn't this the same as visibleContentRect() or similar?
In FrameView::wheelEvent visibleContentRect gives back the previous viewport at this codepoint. However later (in RenderObject::willRenderImage) we should not only check whether animation is visible now, but whether it will become visible because of scrolling. (In reply to
comment #14
)
> The basic idea of not animating on speculative tiles is good. This just needs a good universal and well-performing mechanism for restarting them when they come to view.
The tiled backing store approach works well, the only difficulty with it is we can't access TBS elegantly, just by piercing a lot of layer or by using a hack. Do you think is it rewarding to improve this instead of the current approach?
> I wonder if it would be better at some point to make GIF animations more like other animations (driven by self-repeating timer) rather than doing the restart-timer-on-paint trick? The main benefit of that approach is the automatic stopping and restarting but if we need viewport checks and explicit restarting anyway I'm not sure if it is really helpful.
If we choose this, we could use image's timer to decide whether their animation should step a frame. There could be an AnimationController interface that is implemented by the ImageAnimationController and the old AnimationController (that should be renamed to CSSAnimationController, because it deals with CSS/SVG animation as Simon mentioned in
bug 108864
). Am I understand it correctly? A detailed description would give much help to me here.
> > Source/WebCore/page/FrameView.h:383 > > + bool shouldRenderAnimation(RenderObject*); > > + void animateLater(RenderObject*); > > + void removeSavedAnimation(RenderObject*); > > FrameView is bloated. It would be better to factor this functionality to a class (ImageAnimationController?)
Okay, I'll move them to a new class.
Arunprasad Rajkumar
Comment 18
2013-04-18 09:49:01 PDT
> Source/WebCore/page/FrameView.cpp:3560 > + // resume animations after painting
Start comments with caps & end with "." Follow this for all comments :)
> Source/WebCore/page/FrameView.cpp:4086 > + RenderObject* stoppedAnimation;
Scope of the stoppedAnimation can be reduced into loop
> Source/WebCore/page/FrameView.cpp:4088 > + bool intersectsArea;
Hope this variable can be removed, because after assignment it is only used for next conditional statement alone.
> Source/WebCore/page/FrameView.cpp:4105 > + RenderObject* animationToResume;
Ditto,(Scope can be reduced inside loop)
> Source/WebCore/page/FrameView.cpp:4106 > + IntRect animationToResumeBoundingBox;
Ditto,(Scope can be reduced inside loop)
Nandor Huszka
Comment 19
2013-04-22 09:52:08 PDT
(In reply to
comment #18
)
> Start comments with caps & end with "." > > Follow this for all comments :)
Okay, everybody likes full sencences. :)
> > Source/WebCore/page/FrameView.cpp:4086 > > + RenderObject* stoppedAnimation; > > Scope of the stoppedAnimation can be reduced into loop > > > Source/WebCore/page/FrameView.cpp:4088 > > + bool intersectsArea; > > Hope this variable can be removed, because after assignment it is only used for next conditional statement alone.
Of course, code will be more clear near to the final patch.
Nandor Huszka
Comment 20
2013-04-22 10:30:20 PDT
(In reply to
comment #17
)
> (In reply to
comment #13
) > > I'm not sure I agree with this approach in general. It doesn't play nicely with threaded scrolling on Mac, for example; unless we test for restarting image animations while scrolling, we'll not see animated images when the threaded scrolling is happening. > > I'm not familiar with threaded scrolling. Do you mean we should take care of image animations in layout tests after scrolling? Please describe this problem with more details. > > If this approach is completely no-go, what do you think about the one that Antti mentioned, would that cooperates well with threaded scrolling?
A new idea: I have relocated methods that deal with image animation restarting to ImageAnimationController as Antti suggested. What if we combine this new class with the mentioned - tiled backing store knows image animations - approach. This way BitmapImages won't animate themselves anymore, it will be TiledBackingStore's role. Besides TBS would cause the repaint of animated image if it is required (by invalidating their rect). Does it sound good (in terms of threaded scrolling)? If you have an other concrete notion about the implementation, please specify it. Unfortunately I don't have too much time for doing this, so I would accept any kind of solution that can be implemented at full blast, then r+-ed. :)
Allan Sandfeld Jensen
Comment 21
2013-04-22 11:13:02 PDT
(In reply to
comment #20
)
> A new idea: I have relocated methods that deal with image animation restarting to ImageAnimationController as Antti suggested. What if we combine this new class with the mentioned - tiled backing store knows image animations - approach. >
There is no guarantee that the frameview has a tile backing store. Also note that there are at least two major tile backing store implementations. TileBackingStore (used by TexMap and friends) and TileBacking (use by Mac).
Simon Fraser (smfr)
Comment 22
2013-04-24 13:26:35 PDT
To explain how tiled backing store works: we maintain a grid of tiles (each is its own bitmap, in a CoreAnimation layer). Scrolling involves moving these tiles around (often on another thread), so is very fast. At intervals, we recompute which tiles should be visible, adding some padding around the edges so that you have something to show on scrolling most of the time. Now, if we change gif animations to be started by some kind ImageAnimationController, independently of painting, we'll have to do work every time we bring in new tiles. We'll have to go through all the known animated GIFs (those for <img> and in the RenderStyle), compute which rect they get rendered to in absolute coords, and figure out if they should be started. Similarly with the area of removed tiles; we'll want to stop animations in those areas. I think the amount of work to do this is significant; it's a lot of rect computation and image management. The beauty of the current approach (just animate when painting) is its simplicity.
Nandor Huszka
Comment 23
2013-04-26 03:11:11 PDT
(In reply to
comment #22
)
> To explain how tiled backing store works: we maintain a grid of tiles (each is its own bitmap, in a CoreAnimation layer). Scrolling involves moving these tiles around (often on another thread), so is very fast. At intervals, we recompute which tiles should be visible, adding some padding around the edges so that you have something to show on scrolling most of the time. > > Now, if we change gif animations to be started by some kind ImageAnimationController, independently of painting, we'll have to do work every time we bring in new tiles. We'll have to go through all the known animated GIFs (those for <img> and in the RenderStyle), compute which rect they get rendered to in absolute coords, and figure out if they should be started. Similarly with the area of removed tiles; we'll want to stop animations in those areas. > > I think the amount of work to do this is significant; it's a lot of rect computation and image management. The beauty of the current approach (just animate when painting) is its simplicity.
Thank you for the explanation. It will be followed not immediately but in the future certainly.
Said Abou-Hallawa
Comment 24
2017-08-24 15:54:57 PDT
This was fixed by
https://bugs.webkit.org/show_bug.cgi?id=128632
*** This bug has been marked as a duplicate of
bug 128632
***
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug