Bug 22981 – SVG-as-image redraw broken: svg-as-background-5.html LayoutTest fails

• Component: SVG
• Status: RESOLVED FIXED

[Simon Fraser (smfr)]

Following on from bug 21910, this layout test continues to fail:
LayoutTests/fast/backgrounds/svg-as-background-5.html

The reason is that the SVG content loads slowly, and has one resource load failure (for the JS file), so the first time SVGImage draws, the SVG has not finished loading yet. After that, layout within the SVG fails to cause repaints, because the SVGImage's FrameView has no associated platform widget, and the EmptyChromeClient does nothing with repaints. Thus the repaints get thrown on the floor.

[Simon Fraser (smfr)]

Created attachment 26235 [details]
Strawman patch

Maybe something like this. We could enhance ImageObserver to have a new method like imageChangedInRect(const IntRect&) if we want to restrict redraws to a subset of the image.

[Simon Fraser (smfr)]

Created attachment 26257 [details]
Patch, testcases, changelog

        Reviewed by NOBODY (OOPS!).

        https://bugs.webkit.org/show_bug.cgi?id=22981

        Repaints inside of an SVGImage were thrown on the floor,
        which broke incremental painting due to loading, or SVG animation.
        Fix this by assigning the SVGImage a ChromeClient subclass that passes
        along repaints via a new method on ImageObserver, which also takes
        a rect parameter for the changed rect, allowing incremental repaints.
        Fix RenderImage::imageChanged to take advantage of this changedRect to
        only repaint the changed parts of the image.
        
        Tests: fast/backgrounds/animated-svg-as-background.html
               fast/backgrounds/animated-svg-as-mask.html
               fast/images/animated-svg-as-image.html

[Darin Adler]

Comment on attachment 26257 [details]
Patch, testcases, changelog

> -        curr->first->imageChanged(static_cast<WrappedImagePtr>(this));
> +        // FIXME: what rect to use?
> +        curr->first->imageChanged(static_cast<WrappedImagePtr>(this), IntRect(0, 0, -1, -1));

Seems clear that passing no specific rect is the right thing to do. Why is there any doubt?

> +    virtual void changedInRect(const Image* image, const IntRect& rect);

Please omit the names "image" and "rect" here.

> +// Returns a rect produced by mapping and scaling r from the bounds of srcRect to the bounds of destRect
> +static IntRect mapRect(const IntRect& r, const IntRect& srcRect, const IntRect& destRect)
> +{
> +    float widthMult = (float)destRect.width() / (float)srcRect.width();
> +    float heightMult = (float)destRect.height() / (float)srcRect.height();
> +
> +    FloatRect mappedRect(destRect.x() + (r.x() - srcRect.x()) * widthMult,
> +                         destRect.y() + (r.y() - srcRect.y()) * heightMult,
> +                         r.width() * widthMult,
> +                         r.height() * heightMult);
> +    
> +    return enclosingIntRect(mappedRect);
> +}

Seems weak to be converting back and forth to int like this. Formatting here is also a bit strange. Use of float vs. double seems a bit arbitrary, perhaps driven by the type FloatRect. I'm not fond of the "mult" abbreviation here, nor of the use of C-style casts to convert an int to a float.

> +    virtual void chromeDestroyed()
> +    {
> +        // There is no setClient() on Chrome, so we can't clear its client explicitly. We have to wait for
> +        // this callback.
> +        delete this;
> +    }

We could add a setClient() on Chrome if we like. We could also consider making the SVGImage itself be the ChromeClient, using private inheritance perhaps?

I'm going to say r=me because I had enough comments that it seems worth revising another round.

[Simon Fraser (smfr)]

Created attachment 26262 [details]
Revised patch

(In reply to comment #3)
> (From update of attachment 26257 [details] [review])
> > -        curr->first->imageChanged(static_cast<WrappedImagePtr>(this));
> > +        // FIXME: what rect to use?
> > +        curr->first->imageChanged(static_cast<WrappedImagePtr>(this), IntRect(0, 0, -1, -1));
> 
> Seems clear that passing no specific rect is the right thing to do. Why is
> there any doubt?

Removed.

> > +    virtual void changedInRect(const Image* image, const IntRect& rect);

Done.

> > +// Returns a rect produced by mapping and scaling r from the bounds of srcRect to the bounds of destRect
> > +static IntRect mapRect(const IntRect& r, const IntRect& srcRect, const IntRect& destRect)
> > +{
> > +    float widthMult = (float)destRect.width() / (float)srcRect.width();
> > +    float heightMult = (float)destRect.height() / (float)srcRect.height();
> > +
> > +    FloatRect mappedRect(destRect.x() + (r.x() - srcRect.x()) * widthMult,
> > +                         destRect.y() + (r.y() - srcRect.y()) * heightMult,
> > +                         r.width() * widthMult,
> > +                         r.height() * heightMult);
> > +    
> > +    return enclosingIntRect(mappedRect);
> > +}
> 
> Seems weak to be converting back and forth to int like this. Formatting here is
> also a bit strange. Use of float vs. double seems a bit arbitrary, perhaps
> driven by the type FloatRect. I'm not fond of the "mult" abbreviation here, nor
> of the use of C-style casts to convert an int to a float.

I tidied this up a bit, but I think at least the width and height computations need
to do float-int conversions. As for double vs. float, all of the coordinate math I could
find uses floats. Much of it uses C-style cases too.

> > +    virtual void chromeDestroyed()
> > +    {
> > +        // There is no setClient() on Chrome, so we can't clear its client explicitly. We have to wait for
> > +        // this callback.
> > +        delete this;
> > +    }
> 
> We could add a setClient() on Chrome if we like.

That would require adding null checks on m_client in lots of places (the contract, for
now, seems to be that Chrome::m_client is never null).

> We could also consider making the SVGImage itself be the ChromeClient, using
> private inheritance perhaps?

I think that will result in lifetime issues. ChromeClient is not refcounted,
but SVGImage is, and the image gets destroyed before the Chrome.

So without fairly significant Chrome work, this seems the simplest solution for now.

[Simon Fraser (smfr)]

> I think that will result in lifetime issues. ChromeClient is not refcounted,
> but SVGImage is, and the image gets destroyed before the Chrome.

I guess I could manually refcount the SVGImage on behalf of Chrome.

[Darin Adler]

(In reply to comment #4)
> As for double vs. float, all of the coordinate math I could
> find uses floats. Much of it uses C-style cases too.

Switching from float to double and figuring out when to use which is going to be an important future topic.

But the C-style casts are not in doubt. They are deprecated and should never be used anywhere.

[Darin Adler]

Comment on attachment 26262 [details]
Revised patch

> -        curr->first->imageChanged(static_cast<WrappedImagePtr>(this));
> +        curr->first->imageChanged(static_cast<WrappedImagePtr>(this), IntRect(0, 0, -1, -1));

I think using this special value for IntRect as a signal is ugly. At the call site programmers have no way to know what the -1/-1 size means. Even if I look in the the header file, I have to follow the thread of calls all the way to CachedResourceClient::imageChanged; in many cases the caller is calling another function that forwards a rectangle a couple of levels, and so won't see the comment.

A named constant would be better, unknownChangedRect perhaps or entireImageChangedRect. Even better would be a design that didn't use a magic value. We typically refer to that as "in-band signaling" and sometimes go out of our way to avoid it. For example, you could have two separate imageChanged functions, one with and one without a rect. But given all the affected classes, I guess you don't want that.

If you do want to preserve the in-band signaling but don't want to use the named constant, an economical way to do it would be to make the argument a const IntRect* = 0 and use the null pointer to mean unknown/entire-image. This means that you can't make the mistake of using the special rectangle as a rectangle, because it will do a null dereference. I also think that the value 0 is a more clear way to say "I don't have a rectangle".

And you could have a default argument so we wouldn't ever have to specify IntRect(0, 0, -1, -1) explicitly. Perhaps you omitted it to help you remember to specify the rectangle whenever possible. That's an argument against a default argument.

> +        if (rect.width() != -1 && rect.height() != -1) {

I find it a little strange that this checks both the width and height, but neither x nor y. Why not just width, or all four? Maybe if you had entireImageChangedRect, you could also have isEntireImageChangedRect function or use if (rect == entireImageChangedRect). Either would be clearer than the check for -1.

> +    virtual void chromeDestroyed()
> +    {
> +        // There is no setClient() on Chrome, so we can't clear its client explicitly. We have to wait for
> +        // this callback.
> +        delete this;
> +    }

Can the image outlast the chrome client? If not, why not? If so, then you need to set m_image->m_chromeClient to 0 here to make sure the image doesn't access a deleted chrome client object.

Can the chrome client outlast the image? If not, why not? If so, then I think you need some null checks on m_image and you need ~SVGImage to clear out m_chromeClient->m_image to make sure the chrome client object doesn't access the deleted image.

I'm going to say r=me, but I'm worried about those lifetime issues, and I'd like to hear what you have to say about them.

[Simon Fraser (smfr)]

(In reply to comment #7)
> (From update of attachment 26262 [details] [review])
> > -        curr->first->imageChanged(static_cast<WrappedImagePtr>(this));
> > +        curr->first->imageChanged(static_cast<WrappedImagePtr>(this), IntRect(0, 0, -1, -1));
> 
> I think using this special value for IntRect as a signal is ugly. At the call
> site programmers have no way to know what the -1/-1 size means. Even if I look
> in the the header file, I have to follow the thread of calls all the way to
> CachedResourceClient::imageChanged; in many cases the caller is calling another
> function that forwards a rectangle a couple of levels, and so won't see the
> comment.
> 
> A named constant would be better, unknownChangedRect perhaps or
> entireImageChangedRect. Even better would be a design that didn't use a magic
> value. We typically refer to that as "in-band signaling" and sometimes go out
> of our way to avoid it. For example, you could have two separate imageChanged
> functions, one with and one without a rect. But given all the affected classes,
> I guess you don't want that.
> 
> If you do want to preserve the in-band signaling but don't want to use the
> named constant, an economical way to do it would be to make the argument a
> const IntRect* = 0 and use the null pointer to mean unknown/entire-image. This
> means that you can't make the mistake of using the special rectangle as a
> rectangle, because it will do a null dereference. I also think that the value 0
> is a more clear way to say "I don't have a rectangle".

I like the IntRect* approach, and the ability to provide a default argument.

> > +    virtual void chromeDestroyed()
> > +    {
> > +        // There is no setClient() on Chrome, so we can't clear its client explicitly. We have to wait for
> > +        // this callback.
> > +        delete this;
> > +    }
> 
> Can the image outlast the chrome client? If not, why not? If so, then you need
> to set m_image->m_chromeClient to 0 here to make sure the image doesn't access
> a deleted chrome client object.

The image cannot outlast the chrome client; the image owns the Page, which owns
the Chrome (neither are ref-counted). So in ~SVGImage, the Page is destroyed,
which destroys the Chrome, which calls chromeDestroyed() on its client.

> Can the chrome client outlast the image? If not, why not?

Same as above. The lifetimes of the Page, and the Chrome are all controlled by
SVGImage, which destroys the Page in its destructor. So I think we're guaranteed
to have deterministic teardown (unless future changes make Page or Chrome
ref-counted).

> you need some null checks on m_image and you need ~SVGImage to clear out
> m_chromeClient->m_image to make sure the chrome client object doesn't access
> the deleted image.

I can manually clear the FrameView, Frame and Page own/refptrs in ~SVGImage,
have chromeDestroyed() clear the SVGImageChromeClient m_image, and then assert in ~SVGImage
that the chromeClient's image was cleared, before deleting m_chromeClient there.
That's a little cleaner (no "delete this"), and add some future-proofing.

[Simon Fraser (smfr)]

Committing to http://svn.webkit.org/repository/webkit/trunk ...
	M	LayoutTests/ChangeLog
	A	LayoutTests/fast/backgrounds/animated-svg-as-background.html
	A	LayoutTests/fast/backgrounds/animated-svg-as-mask.html
	A	LayoutTests/fast/backgrounds/resources/animated-rect-fixed-size.svg
	A	LayoutTests/fast/backgrounds/resources/animated-rect-relative-size.svg
	M	LayoutTests/fast/backgrounds/svg-as-background-5.html
	A	LayoutTests/fast/canvas/canvas-as-image-incremental-repaint.html
	A	LayoutTests/fast/canvas/canvas-as-image.html
	A	LayoutTests/fast/images/animated-svg-as-image.html
	A	LayoutTests/fast/images/resources/animated-rect-fixed-size.svg
	A	LayoutTests/fast/images/resources/animated-rect-relative-size.svg
	A	LayoutTests/platform/mac/fast/backgrounds/animated-svg-as-background-expected.checksum
	A	LayoutTests/platform/mac/fast/backgrounds/animated-svg-as-background-expected.png
	A	LayoutTests/platform/mac/fast/backgrounds/animated-svg-as-background-expected.txt
	A	LayoutTests/platform/mac/fast/backgrounds/animated-svg-as-mask-expected.checksum
	A	LayoutTests/platform/mac/fast/backgrounds/animated-svg-as-mask-expected.png
	A	LayoutTests/platform/mac/fast/backgrounds/animated-svg-as-mask-expected.txt
	M	LayoutTests/platform/mac/fast/backgrounds/svg-as-background-5-expected.checksum
	M	LayoutTests/platform/mac/fast/backgrounds/svg-as-background-5-expected.png
	A	LayoutTests/platform/mac/fast/canvas/canvas-as-image-expected.checksum
	A	LayoutTests/platform/mac/fast/canvas/canvas-as-image-expected.png
	A	LayoutTests/platform/mac/fast/canvas/canvas-as-image-expected.txt
	A	LayoutTests/platform/mac/fast/canvas/canvas-as-image-incremental-repaint-expected.checksum
	A	LayoutTests/platform/mac/fast/canvas/canvas-as-image-incremental-repaint-expected.png
	A	LayoutTests/platform/mac/fast/canvas/canvas-as-image-incremental-repaint-expected.txt
	A	LayoutTests/platform/mac/fast/images/animated-svg-as-image-expected.checksum
	A	LayoutTests/platform/mac/fast/images/animated-svg-as-image-expected.png
	A	LayoutTests/platform/mac/fast/images/animated-svg-as-image-expected.txt
	M	WebCore/ChangeLog
	M	WebCore/css/CSSCanvasValue.cpp
	M	WebCore/loader/CachedImage.cpp
	M	WebCore/loader/CachedImage.h
	M	WebCore/loader/CachedResourceClient.h
	M	WebCore/platform/graphics/ImageObserver.h
	M	WebCore/rendering/RenderBox.cpp
	M	WebCore/rendering/RenderBox.h
	M	WebCore/rendering/RenderImage.cpp
	M	WebCore/rendering/RenderImage.h
	M	WebCore/rendering/RenderListMarker.cpp
	M	WebCore/rendering/RenderListMarker.h
	M	WebCore/rendering/RenderObject.cpp
	M	WebCore/rendering/RenderObject.h
	M	WebCore/rendering/RenderSVGImage.cpp
	M	WebCore/rendering/RenderSVGImage.h
	M	WebCore/rendering/RenderScrollbarPart.cpp
	M	WebCore/rendering/RenderScrollbarPart.h
	M	WebCore/rendering/RenderTableCol.cpp
	M	WebCore/rendering/RenderTableCol.h
	M	WebCore/rendering/RenderTableRow.cpp
	M	WebCore/rendering/RenderTableRow.h
	M	WebCore/rendering/RenderTableSection.cpp
	M	WebCore/rendering/RenderTableSection.h
	M	WebCore/svg/graphics/SVGImage.cpp
	M	WebCore/svg/graphics/SVGImage.h
Committed r39555

[Eric Seidel (no email)]

Looks like this caused bug 57052.