507817 - BorderImage should not call ExtractCurrentFrame each time it draws

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Bobby Holley (:bholley)]

In DrawBorderImageComponent, we call ExtractCurrentFrame each time we draw, which kills performance for borderimage.

http://mxr.mozilla.org/mozilla-central/source/layout/base/nsCSSRendering.cpp#2225

Excerpt from an email with joe:
Zack Weinberg would know why we call it every time. Jeff, Mark Finkle and I were talking about this the other day; it should be easy to fix as long as layout/stylesheets have some sort of object we can attach the border-image extracted images to. I suggest filing a bug on Core: Style System, CC zwol and dbaron, and ask what the right way to go about doing this is.

Comment 1

[Zack Weinberg (:zwol)]

The original implementation of this feature was not by me; I no longer remember which bug it was wound up with me rewriting it.  However, the old code did the moral equivalent of ExtractCurrentFrame on every draw, too.

I don't think all the information required is available much earlier than this point -- I think we can't do it in the rule nodes, for instance.  dbaron would be a better person to ask about that.  We might could cache the extracted images in the style system somewhere, but we would need to take care not to break animated images.

Comment 2

[Markus Stange [:mstange]]

See also http://groups.google.co.nz/group/mozilla.dev.tech.layout/browse_thread/thread/77ec6a7142845e77/5b856404146c0de4?hl=en%05b856404146c0de4 :

> We shouldn't really have to extract an image at all. We only do it to
> get the right repeat/pad behaviour. At some point that will be fixed in
> the graphics layer; we'll still extract an image, but it will be a
> "virtual" image and in some/most cases we won't need to actually copy
> the data.

Comment 3

[Zack Weinberg (:zwol)]

> We shouldn't really have to extract an image at all. We only do it to
> get the right repeat/pad behaviour.

True.  See bug 468496 among others.

Comment 4

[Joe Drew (not getting mail)]

I don't really care about animated images; before bug 753 landed, we always asked for frame 0 anyways, and now we just ask for the "Current" frame because that was the most straightforward thing to do.

Comment 5

[Mark Finkle (:mfinkle) (use needinfo?)]

Attachment: patch

Simple patch (with some timing code in it) that skips the ExtractCurrentFrame call if possible.

DrawSingleImage has a param for a source rect, so in cases where we are calling DrawSingleImage, the patch uses the full image and the source rect.

If we are using the DrawImage call, we still do the extract because DrawImage doesn't support a source rect yet.

Tested on an n810. The ExtractCurrentFrame call takes about 4-5ms. The DrawSingleImage call takes around 3-4ms, so we save nearly half of the time.

The rendering result looks ok on the n810

Comment 6

[Zack Weinberg (:zwol)]

There's a subtlety about ExtractCurrentFrame: it always drops the result into an image surface, which means we avoid the lack of support for EXTEND_PAD in x11 and quartz surfaces.  See bug 468496.  Thus, going straight to DrawSingleImage + source rect may not be safe.

Please try the border-image reftests on Linux and Mac with your patch.  Also, load each of the border-image reftest files in a regular browser (on Linux and Mac, with your patch) and examine the rendering at all zoom levels.

Comment 7

[Mark Finkle (:mfinkle) (use needinfo?)]

Had a reftest failure on Linux:
http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1249410355.1249417835.3529.gz

and Mac:
http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1249410356.1249418816.14342.gz

As Zack predicted :(

Comment 8

[Markus Stange [:mstange]]

In which cases does the patch result in wrong behaviour? Does it have to do with non-pixel-aligned / scaled images? Can we detect these cases and special-case them to take the slow path?

Comment 9

[Zack Weinberg (:zwol)]

Markus: would that it were that simple.  See bug 468496 and the many discussions and duplicates linked from there.

Comment 10

[Joe Drew (not getting mail)]

Actually the fact that DrawSingleImage causes reftest fails is sort of surprising, given that it is supposed to work around these problems. But looking at the results shows that it does seem to be the lack of EXTEND_PAD that's causing this. This probably shouldn't be the case, so we should maybe try to isolate the bug in DrawSingleImage.

Comment 11

[Robert O'Callahan (:roc) (email my personal email if necessary)]

DrawSingleImage depends on nsThebesImage::Draw working properly, and in the absence of EXTEND_PAD, nsThebesImage::Draw fails in some cases. So it's not surprising that we have a problem here.

Comment 12

[Mark Finkle (:mfinkle) (use needinfo?)]

I'm starting to read through bug 468496 to learn more about the EXTEND_PAD issues. I wonder if there are know cases, determined by available data, where skipping the ExtractCurrentFrame would work ok, even with the EXTEND_PAD problem.

Another approach I am considering is to cache the sub-images, created by ExtractCurrentFrame, in nsStyleBorder and re-use them the next time the border is rendered. I'm assuming that we could cache the images until a property of nsStyleBorder was changed, at which point we'd need to clear the cache.

The obvious drawback to caching is the increase in memory usage.

Comment 13

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to comment #12)
> I'm starting to read through bug 468496 to learn more about the EXTEND_PAD
> issues. I wonder if there are know cases, determined by available data, where
> skipping the ExtractCurrentFrame would work ok, even with the EXTEND_PAD
> problem.

If the CTM is identity and we're not doing any scaling for the border-image, then you don't need EXTEND_PAD.

Comment 14

[Mark Finkle (:mfinkle) (use needinfo?)]

Attachment: patch 2 (cache sub frames)

This patch caches the temporary subimages created for each border image style. The subimages are held in the nsStyleBorder object. The cache is cleared when a border image is set.

The patch also has some commented out timing code.

Using the patch on a N900 running Fennec I saw individual border image paint time drop from an avg of 20ms (max 45ms) to an avg of 2ms (max 5ms) when the cache path was hit.

To give an idea of how that affects Fennec, I timed the border image paint time required to open the Awesomebar and open the Preferences view. Both use border-image, but Preferences uses it quite a bit more:

Open awesomebar:  (before) 279ms, (after) 129ms
Open preferences: (before) 591ms, (after) 187ms

Obviously the 10x cache path improvement does not translate to real usage, but the improvement is still significant.

Comment 15

[Mark Finkle (:mfinkle) (use needinfo?)]

Attachment: patch 3

Same as the last patch but with all "SubFrame" references renamed to "SubImage"

Comment 16

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

How does this deal with the border image animating?

Comment 17

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to comment #16)
> How does this deal with the border image animating?

That is something I did not plan on dealing with (or even know about). I need to test it, but I assume it won't work correctly.

Also, Try Server shows a crash in the new nsStyleBorder::GetSubImage method for the side-scaling-1h.html test. I am looking into that too.

Comment 18

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

We have a bit of code to handle animated border images right now, so it should work currently.

Does the stuff you're caching on nsStyleBorder depend in any way on the size of the element on which you're drawing the border image?  A single nsStyleBorder can be shared between multiple elements, so we'd have problems if it did.

(Also, there are relatively straightforward ways to improve the sharing further, but I don't know if that would help anything.)

Comment 19

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to comment #18)
> We have a bit of code to handle animated border images right now, so it should
> work currently.

OK, I'll test the patch with animated border images and report back

> Does the stuff you're caching on nsStyleBorder depend in any way on the size of
> the element on which you're drawing the border image?  A single nsStyleBorder
> can be shared between multiple elements, so we'd have problems if it did.

The patch should not depend on the element or its size. The patch should only be dealing with the slices created from the main border image. I don't think those slices depend on the element size.

> (Also, there are relatively straightforward ways to improve the sharing
> further, but I don't know if that would help anything.)

I'll give most anything a shot :)

Comment 20

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to comment #19)
> (In reply to comment #18)
> > (Also, there are relatively straightforward ways to improve the sharing
> > further, but I don't know if that would help anything.)
> 
> I'll give most anything a shot :)

They'd only help if it's common for multiple elements to share the same border-image because they got it from the same CSS style rule, and have all CSS rules less specific than that CSS style rule in common.  (In other words, quite common for a sequence of similarly styled siblings, and sometimes for things like multiple occurrences of the "button" element in different parts of a document.)  Is that often the case with what you're dealing with?

Comment 21

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to comment #20)
> (In reply to comment #19)

> They'd only help if it's common for multiple elements to share the same
> border-image because they got it from the same CSS style rule, and have all CSS
> rules less specific than that CSS style rule in common.  (In other words, quite
> common for a sequence of similarly styled siblings, and sometimes for things
> like multiple occurrences of the "button" element in different parts of a
> document.)  Is that often the case with what you're dealing with?

That is exactly the Fennec UI use case. We have CSS styles for normal buttons, toolbarbuttons, checkbox toggels and radiobuttons

Comment 22

[Mark Finkle (:mfinkle) (use needinfo?)]

Attachment: patch 4 (no crash on try server)

This patch fixes a problem in GetSubImage that resulted in crashes on try server unit tests

Comment 23

[Mark Finkle (:mfinkle) (use needinfo?)]

Attachment: patch 5 (no cache for animated)

I don't think there is a need to thrash the cache for animated border-images. This patch skips trying to cache animated borders.

pushed to try server...

Comment 24

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 412009 [details] [diff] [review]
patch 5 (no cache for animated)

Seems fine to me, except I'd rather have GetSubImage return imgIContainer* (not addrefed).  And you should document in nsStyleStruct.h that SetSubImage/GetSubImage are just a cache to be used by the caller to cache pieces of mBorderImage (and don't do anything magic internally other than caching).

>diff --git a/layout/base/nsCSSRendering.cpp b/layout/base/nsCSSRendering.cpp
>+                                     const nsStyleBorder& aBorderStyle,
>+                                     PRUint8 aIndex);

I'd call this aStyleBorder rather than aBorderStyle for consistency with other code.

>   if (aBorderStyle.IsBorderImageLoaded()) {
>+//    PRTime startTime = PR_Now();
>     DrawBorderImage(aPresContext, aRenderingContext, aForFrame,
>                     aBorderArea, aBorderStyle, aDirtyRect);
>+//    PRTime endTime = PR_Now();
>+//    PRInt32 delta = (PRInt32)(endTime/1000UL - startTime/1000UL);
>+//    printf("DrawBorderImage: %d ms\n", delta);
>     return;

Probably better to remove this from the patch.

r=dbaron with that

Comment 25

[Joe Drew (not getting mail)]

Comment on attachment 412009 [details] [diff] [review]
patch 5 (no cache for animated)

>diff --git a/layout/base/nsCSSRendering.cpp b/layout/base/nsCSSRendering.cpp
       DrawBorderImageComponent(aRenderingContext, aForFrame,
>                                imgContainer, aDirtyRect,
>                                destArea, subArea,
>-                               fillStyleH, fillStyleV, unitSize);
>+                               fillStyleH, fillStyleV,
>+                               unitSize, aBorderStyle, i * (RIGHT + 1) + j);

That index calculation is really ugly, but I'm not 100% clear on what I'd do to make it better.

> DrawBorderImageComponent(nsIRenderingContext& aRenderingContext,
>+  // Don't bother trying to cache sub images if the border image is animated
>+  PRBool animated;
>+  aImage->GetAnimated(&animated);

Check the rv here. If we haven't finished decoding the image, we won't know whether it's animated. (If it fails, we shouldn't cache anything.)

>+    if (!animated)
>+      const_cast<nsStyleBorder&>(aBorderStyle).SetSubImage(aIndex, subImage);

I am not in love with this const_cast, but if dbaron's okay with it, so am I.

Comment 26

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Er, I missed that const_cast.  It might be better if you made the method on nsStyleBorder a const method and put the const_cast inside it so it's clear on the style system end what's going on, but if that's hard it's also ok as-is.

Comment 27

[Mark Finkle (:mfinkle) (use needinfo?)]

* Made GetSubImage return an unaddrefed imgIContainer*
* Added comments to nsStyleStruct.h about the reason for cache support
* Changed aBorderStyle to aStyleBorder in the nsCSSRendering.cpp
* Changed the animated check to default to PR_TRUE in case GetAnimated() returns with an error - image not decoded yet
* Moved the const_cast into the SetSubImage method

Comment 28

[Mark Finkle (:mfinkle) (use needinfo?)]

* Removed timing code

Comment 29

[Mark Finkle (:mfinkle) (use needinfo?)]

pushed to m-c:
http://hg.mozilla.org/mozilla-central/rev/63bd5af8386b

Comment 30

[Mark Finkle (:mfinkle) (use needinfo?)]

pushed to m-192:
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/f2cf439098e4

Comment 31

[Boris Zbarsky [:bzbarsky]]

This bug added this line:

+  if (0 <= aIndex && mSubImages.Count() > aIndex)

which warns in gcc because aIndex is unsigned (hence always >= 0, so the first test is pointless).

Comment 32

[Boris Zbarsky [:bzbarsky]]

I pushed http://hg.mozilla.org/mozilla-central/rev/db9dd27917cd to fix that warning.