BorderImage should not call ExtractCurrentFrame each time it draws

RESOLVED FIXED

Status

()

Core
CSS Parsing and Computation
RESOLVED FIXED
8 years ago
4 years ago

People

(Reporter: Bobby Holley (parental leave - send mail for anything urgent), Assigned: mfinkle)

Tracking

Trunk
x86
Mac OS X
Points:
---

Firefox Tracking Flags

(status1.9.2 beta4-fixed)

Details

Attachments

(1 attachment, 4 obsolete attachments)

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.
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.
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.
> 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.
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.
Created attachment 392521 [details] [diff] [review]
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
Assignee: nobody → mark.finkle
Attachment #392521 - Flags: review?(joe)
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.
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 :(
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?
Markus: would that it were that simple.  See bug 468496 and the many discussions and duplicates linked from there.
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.
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.
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.
(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.
Attachment #392521 - Flags: review?(joe) → review-
Created attachment 411682 [details] [diff] [review]
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.
Attachment #392521 - Attachment is obsolete: true
Attachment #411682 - Flags: review?(joe)
Created attachment 411683 [details] [diff] [review]
patch 3

Same as the last patch but with all "SubFrame" references renamed to "SubImage"
Attachment #411682 - Attachment is obsolete: true
Attachment #411683 - Flags: review?(joe)
Attachment #411682 - Flags: review?(joe)
How does this deal with the border image animating?
Attachment #411683 - Flags: review?(dbaron)
(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.
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.)
(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 :)
(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?
(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
Created attachment 411887 [details] [diff] [review]
patch 4 (no crash on try server)

This patch fixes a problem in GetSubImage that resulted in crashes on try server unit tests
Attachment #411683 - Attachment is obsolete: true
Attachment #411887 - Flags: review?(joe)
Attachment #411683 - Flags: review?(joe)
Attachment #411683 - Flags: review?(dbaron)
(Assignee)

Updated

8 years ago
Attachment #411887 - Flags: review?(dbaron)
Created attachment 412009 [details] [diff] [review]
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...
Attachment #411887 - Attachment is obsolete: true
Attachment #412009 - Flags: review?(joe)
Attachment #411887 - Flags: review?(joe)
Attachment #411887 - Flags: review?(dbaron)
(Assignee)

Updated

8 years ago
Attachment #412009 - Flags: review?(dbaron)
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
Attachment #412009 - Flags: review?(dbaron) → review+
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.
Attachment #412009 - Flags: review?(joe) → review+
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.
* 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
* Removed timing code
pushed to m-c:
http://hg.mozilla.org/mozilla-central/rev/63bd5af8386b
Status: NEW → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
(Assignee)

Updated

8 years ago
tracking-fennec: --- → ?
(Assignee)

Updated

8 years ago
Attachment #412009 - Flags: approval1.9.2?

Updated

8 years ago
Attachment #412009 - Flags: approval1.9.2? → approval1.9.2+
pushed to m-192:
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/f2cf439098e4
status1.9.2: --- → final-fixed
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).
I pushed http://hg.mozilla.org/mozilla-central/rev/db9dd27917cd to fix that warning.
tracking-fennec: ? → ---
You need to log in before you can comment on or make changes to this bug.