Reorganize image animation starting

• Component: WebKit API
• Status: RESOLVED DUPLICATE

[Nandor Huszka]

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.

[Nandor Huszka]

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]

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]

(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]

(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]

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]

(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]

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]

(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]

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]

Please note r148300 just landed for bug 108864. It changes how gif animations are restarted after having been suspended.

[Nandor Huszka]

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)]

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 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 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 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]

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]

(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]

> 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]

(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]

(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]

(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)]

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]

(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]

This was fixed by https://bugs.webkit.org/show_bug.cgi?id=128632

*** This bug has been marked as a duplicate of bug 128632 ***