In the most common cases of both types of box-shadow, much of the shadow is not shown to the user. It would be useful to optimise these cases by telling the blurrer about these areas, so that it can instantly skip over them while blurring. Planet Opera in debug builds scrolls really great with this patch.

skipRect should be stored as an nsIntRect to avoid int/float conversions during blurring. BoxBlurHorizontal/Vertical could take an nsIntRect* which is just null if there's no skip rect. for (PRInt32 y = 0; y < aRows; y++) { + if (aHasSkipRect && aSkipRect.Contains(gfxPoint(x, y))) { I think you can rewrite this code to avoid so many conditional branches. For one thing, the coordinate of the outer loop can be checked in the outer loop. + skipGfxRect.Inset(PR_MAX(borderRadii[C_TL].height, borderRadii[C_TR].height), 0, + PR_MAX(borderRadii[C_BL].height, borderRadii[C_BR].height), 0); Don't you need to look at the widths too? This is a different optimization to the one I was thinking of. It's probably better because you avoid touching pixels in the skip rect.

Now with more death-defying optimisations...and a reftest! Those review comments did actually inspire some more speedups in my head. Sorry about the IRL misunderstanding earlier :-) > + skipGfxRect.Inset(PR_MAX(borderRadii[C_TL].height, > borderRadii[C_TR].height), 0, > + PR_MAX(borderRadii[C_BL].height, > borderRadii[C_BR].height), 0); > > Don't you need to look at the widths too? No, because with that alone the border radii are out of the picture now, so no need to consider the width of the radii.

+ skipGfxRect.Round(); This should be RoundIn. Also, I think you should do it in Init() instead of the caller. + // nsIntRect doesn't have Deflate() + mSkipRect.Inflate(-aBlurRadius.width, -aBlurRadius.height); Just add Deflate to nsIntRect. + PRBool nearSkipRect = aSkipRect && y >= aSkipRect->y && + y <= aSkipRect->YMost(); Shouldn't this be y < aSkipRect->YMost()? And shouldn't this be called inSkipRectY or something like that? + x <= aSkipRect->XMost()) { < here too + x += aSkipRect->width + 1; Shouldn't this be x = aSkipRect->XMost()? If so, we need tests that cover the cases that fail with this patch.

Fix comments

+ if (inSkipRectY && skipRectSpans) { + y = aSkipRect->YMost(); + continue; + } This isn't actually what you want. We'll increment y so the next value of y will be aSkipRect->YMost() + 1.

Whoops, didn't refresh the patch

Comment on attachment 425146 [details] [diff] [review] Patch 3.1 [Backout: Comment 8] http://hg.mozilla.org/mozilla-central/rev/b3d6a8724029

Comment on attachment 425146 [details] [diff] [review] Patch 3.1 [Backout: Comment 8] http://hg.mozilla.org/mozilla-central/rev/ca2c610c1ea1 http://hg.mozilla.org/mozilla-central/rev/46ae6ac7433b Merge for "Backed out changeset: b3d6a8724029" of Bug 544099 - Allow shadow blurring to skip areas where blurring is unnecessary; Patch 3.1. which breaks comm-central. Examples: http://tinderbox.mozilla.org/showlog.cgi?log=SeaMonkey/1265307743.1265308771.22473.gz WINNT 5.2 comm-central-trunk build on 2010/02/04 10:22:23 and http://tinderbox.mozilla.org/showlog.cgi?log=Thunderbird/1265310781.1265311012.16088.gz Linux comm-central bloat on 2010/02/04 11:13:01

I need to fix things for non-libxul builds, but by God was it hard. gfx has a dependency on thebes, so making thebes depend on gfx makes a circular dependency. I had to make a new library (poor non-libxul builds), in old-gfx src folders because that is compiled after thebes.

Use hg rename to move the file. I think this approach of creating a new gfx library so it can link in nsIntRect etc as well as thebes is OK, but the gfx guys should sign off on it. One question is the name ... 'blur' is probably too specific. Maybe "gfxutils"?

Fix comments; I used gfxutils and added a comment to its makefile explaining why this library is necessary.

Comment on attachment 425733 [details] [diff] [review] Patch 5 Looks good to me. Asking joedrew to sign off on it as a graphics peer.

Joe, how soon can you review this? It's getting quite important since we're using shadows more and more in the UI, in ways that this patch will make a big speed difference.

If someone currently has this patch applied to one of their builds, could they please check the following website and see if the animations are choppy? http://idiotbox07.awardspace.biz/testing/ If they are not, then I think Bug 546362 can be marked as a duplicate of this bug. Thanks

(In reply to comment #12) > (From update of attachment 425733 [details] [diff] [review]) > Looks good to me. Asking joedrew to sign off on it as a graphics peer. Is http://www.mozilla.org/about/owners.html#graphics outdated?

Comment on attachment 425733 [details] [diff] [review] Patch 5 In general, I'm not in love with this patch. Mostly, I dislike the idea of a "skip rect" -- I'd much rather have a mask which defaults to the full rect. For now, though, I'm going to review this as if I was OK with skip rects. I also don't really like the mHasSkipRect stuff, but I understand you were just hewing to the prevailing style there. (I'd rather just do a check like mSkipRect.IsEmpty() rather than having a separate piece of state you have to keep up-to-date.) >diff --git a/gfx/thebes/src/gfxBlur.cpp b/gfx/src/thebes/utils/gfxBlur.cpp >+// Copied from nsSVGUtils >+// Converts a gfxRect to an nsIntRect for speed >+static nsIntRect >+GfxRectToIntRect(const gfxRect& aIn) >+{ >+ nsIntRect result(PRInt32(aIn.X()), PRInt32(aIn.Y()), >+ PRInt32(aIn.Width()), PRInt32(aIn.Height())); >+ NS_ASSERTION(gfxRect(result.x, result.y, result.width, result.height) == aIn, >+ "The given gfxRect isn't rounded properly!"); >+ return result; >+} Let's put this somewhere public so we can reuse the code. It can even go in utils because it uses both old-style and Thebes code. > gfxContext* > gfxAlphaBoxBlur::Init(const gfxRect& aRect, > const gfxIntSize& aBlurRadius, >- const gfxRect* aDirtyRect) >+ const gfxRect* aDirtyRect, >+ gfxRect* aSkipRect) [...] >+ if (aSkipRect) { >+ // If we get passed a skip rect, we can lower the amount of >+ // blurring we need to do. We convert it to nsIntRect to avoid >+ // expensive int<->float conversions if we were to use gfxRect instead. >+ aSkipRect->RoundIn(); >+ mSkipRect = GfxRectToIntRect(*aSkipRect); >+ nsIntRect shadowIntRect = GfxRectToIntRect(rect); >+ >+ mSkipRect.Deflate(aBlurRadius.width, aBlurRadius.height); >+ mSkipRect.IntersectRect(mSkipRect, shadowIntRect); >+ if (mSkipRect == shadowIntRect) >+ return nsnull; >+ >+ mSkipRect -= shadowIntRect.TopLeft(); What's the purpose of offsetting the skip rect by the shadow rect? It especially doesn't make sense given that it's possible that they're the same. > static void > BoxBlurHorizontal(unsigned char* aInput, > unsigned char* aOutput, > PRInt32 aLeftLobe, > PRInt32 aRightLobe, > PRInt32 aStride, >- PRInt32 aRows) >+ PRInt32 aRows, >+ const nsIntRect* aSkipRect) This is an internal function - can't we make this not-a-pointer and skip all the if (aSkipRect) checks? > { > PRInt32 boxSize = aLeftLobe + aRightLobe + 1; >+ PRBool skipRectSpans = aSkipRect && aSkipRect->x <= 0 && >+ aSkipRect->XMost() >= aStride - 1; You compare XMost and stride here, and they're different logical types. Probably, aStride should be renamed aWidth. The name of this variable is also weird; maybe skipRectApplies? Finally, inSkipRectY's condition is a lot more understandable, at least by me. If you integrated those two checks, I'd be a lot happier. > > for (PRInt32 y = 0; y < aRows; y++) { >+ // Check whether the skip rect intersects this row. If the skip >+ // rect covers the whole surface in this row, we can avoid >+ // this row entirely (and any others along the skip rect). >+ PRBool inSkipRectY = aSkipRect && y >= aSkipRect->y && >+ y < aSkipRect->YMost(); >+ if (inSkipRectY && skipRectSpans) { >+ y = aSkipRect->YMost() - 1; >+ continue; >+ } >+ > PRInt32 alphaSum = 0; > for (PRInt32 i = 0; i < boxSize; i++) { > PRInt32 pos = i - aLeftLobe; > pos = PR_MAX(pos, 0); > pos = PR_MIN(pos, aStride - 1); > alphaSum += aInput[aStride * y + pos]; > } > for (PRInt32 x = 0; x < aStride; x++) { >+ // Check whether we are within the skip rect. If so, go >+ // to the next point outside the skip rect. >+ if (inSkipRectY && x >= aSkipRect->x && >+ x < aSkipRect->XMost()) { >+ x = aSkipRect->XMost(); Do you have numbers that show that skipping by changing the loop variable in this way is helpful? I'd much prefer a simple condition along the lines of "if (skipping) continue;". >+ // Recalculate the neighbouring alpha values for >+ // our new point on the surface. >+ alphaSum = 0; >+ for (PRInt32 i = 0; i < boxSize; i++) { >+ PRInt32 pos = x + i - aLeftLobe; >+ pos = PR_MAX(pos, 0); >+ pos = PR_MIN(pos, aStride - 1); >+ alphaSum += aInput[aStride * y + pos]; >+ } >+ } Also, is this required, or will the code below cover this case? > PRInt32 tmp = x - aLeftLobe; > PRInt32 last = PR_MAX(tmp, 0); > PRInt32 next = PR_MIN(tmp + boxSize, aStride - 1); > > aOutput[aStride * y + x] = alphaSum/boxSize; > > alphaSum += aInput[aStride * y + next] - > aInput[aStride * y + last]; > static void > BoxBlurVertical(unsigned char* aInput, > unsigned char* aOutput, > PRInt32 aTopLobe, > PRInt32 aBottomLobe, > PRInt32 aStride, >- PRInt32 aRows) >+ PRInt32 aRows, >+ const nsIntRect* aSkipRect) Same general comments/questions as BoxBlurHorizontal. >@@ -240,31 +328,32 @@ gfxAlphaBoxBlur::Paint(gfxContext* aDest >+ const nsIntRect* skipRect = (mHasSkipRect ? &mSkipRect : nsnull); I reiterate my objection to conditional skip rects. :) >diff --git a/layout/base/nsCSSRendering.h b/layout/base/nsCSSRendering.h >--- a/layout/base/nsCSSRendering.h >+++ b/layout/base/nsCSSRendering.h >@@ -412,32 +412,35 @@ public: > * DoPaint(). To set the color of the > * resulting blurred graphic mask, you must > * set the color on this context before > * calling Init(). > * > * @param aDirtyRect The absolute dirty rect in app units. Used to > * optimize the temporary surface size and speed up blur. > * >+ * @param aSkipRect An area in device pixels (NOT app units!) to avoid >+ * blurring over, to prevent unnecessary work. >+ * Is there a way we can keep the dirty rect and skip rect in the same units?

Also, roc asked in comment 3 for some tests that I don't think were included.

(In reply to comment #16) > (From update of attachment 425733 [details] [diff] [review]) > In general, I'm not in love with this patch. Mostly, I dislike the idea of a > "skip rect" -- I'd much rather have a mask which defaults to the full rect. > For now, though, I'm going to review this as if I was OK with skip rects. How would you represent such a mask? > I also don't really like the mHasSkipRect stuff, but I understand you were > just hewing to the prevailing style there. (I'd rather just do a check like > mSkipRect.IsEmpty() rather than having a separate piece of state you have to > keep up-to-date.) Yeah, we should get rid of mHasSkipRect. > > gfxContext* > > gfxAlphaBoxBlur::Init(const gfxRect& aRect, > > const gfxIntSize& aBlurRadius, > >- const gfxRect* aDirtyRect) > >+ const gfxRect* aDirtyRect, > >+ gfxRect* aSkipRect) > [...] > >+ if (aSkipRect) { > >+ // If we get passed a skip rect, we can lower the amount of > >+ // blurring we need to do. We convert it to nsIntRect to avoid > >+ // expensive int<->float conversions if we were to use gfxRect instead. > >+ aSkipRect->RoundIn(); > >+ mSkipRect = GfxRectToIntRect(*aSkipRect); > >+ nsIntRect shadowIntRect = GfxRectToIntRect(rect); > >+ > >+ mSkipRect.Deflate(aBlurRadius.width, aBlurRadius.height); > >+ mSkipRect.IntersectRect(mSkipRect, shadowIntRect); > >+ if (mSkipRect == shadowIntRect) > >+ return nsnull; > >+ > >+ mSkipRect -= shadowIntRect.TopLeft(); > > What's the purpose of offsetting the skip rect by the shadow rect? It > especially doesn't make sense given that it's possible that they're the same. It's making the skip rect relative to the shadow rect. > This is an internal function - can't we make this not-a-pointer and skip all > the if (aSkipRect) checks? The idea is to short-circuit various tests if there is no skip rect. But you're right, we might as well just get rid of it, since those short-circuits aren't in the inner loop anyway. > > { > > PRInt32 boxSize = aLeftLobe + aRightLobe + 1; > >+ PRBool skipRectSpans = aSkipRect && aSkipRect->x <= 0 && > >+ aSkipRect->XMost() >= aStride - 1; > > You compare XMost and stride here, and they're different logical types. > Probably, aStride should be renamed aWidth. OK. > The name of this variable is also weird; maybe skipRectApplies? > Finally, inSkipRectY's condition is a lot more understandable, at least by me. > If you integrated those two checks, I'd be a lot happier. Fair enough, that's in the outer loop anyway. > > for (PRInt32 y = 0; y < aRows; y++) { > >+ // Check whether the skip rect intersects this row. If the skip > >+ // rect covers the whole surface in this row, we can avoid > >+ // this row entirely (and any others along the skip rect). > >+ PRBool inSkipRectY = aSkipRect && y >= aSkipRect->y && > >+ y < aSkipRect->YMost(); > >+ if (inSkipRectY && skipRectSpans) { > >+ y = aSkipRect->YMost() - 1; > >+ continue; > >+ } > >+ > > PRInt32 alphaSum = 0; > > for (PRInt32 i = 0; i < boxSize; i++) { > > PRInt32 pos = i - aLeftLobe; > > pos = PR_MAX(pos, 0); > > pos = PR_MIN(pos, aStride - 1); > > alphaSum += aInput[aStride * y + pos]; > > } > > for (PRInt32 x = 0; x < aStride; x++) { > >+ // Check whether we are within the skip rect. If so, go > >+ // to the next point outside the skip rect. > >+ if (inSkipRectY && x >= aSkipRect->x && > >+ x < aSkipRect->XMost()) { > >+ x = aSkipRect->XMost(); > > Do you have numbers that show that skipping by changing the loop variable in > this way is helpful? I'd much prefer a simple condition along the lines of "if > (skipping) continue;". I don't think we need numbers. For the common case where the skip rect is nearly the size of the shadow rect, and treating the blur radius as a constant, the complexity of BoxBlurHorizontal/Vertical is O(W + H) if we set x to aSkipRect->XMost(), and O(W*H) if we don't. > >+ // Recalculate the neighbouring alpha values for > >+ // our new point on the surface. > >+ alphaSum = 0; > >+ for (PRInt32 i = 0; i < boxSize; i++) { > >+ PRInt32 pos = x + i - aLeftLobe; > >+ pos = PR_MAX(pos, 0); > >+ pos = PR_MIN(pos, aStride - 1); > >+ alphaSum += aInput[aStride * y + pos]; > >+ } > >+ } > > Also, is this required, or will the code below cover this case? It's required. The code below only accumulates one value into alphaSum. We need to recompute alphaSum over the entire box at the new x position. > >diff --git a/layout/base/nsCSSRendering.h b/layout/base/nsCSSRendering.h > >--- a/layout/base/nsCSSRendering.h > >+++ b/layout/base/nsCSSRendering.h > >@@ -412,32 +412,35 @@ public: > > * DoPaint(). To set the color of the > > * resulting blurred graphic mask, you must > > * set the color on this context before > > * calling Init(). > > * > > * @param aDirtyRect The absolute dirty rect in app units. Used to > > * optimize the temporary surface size and speed up blur. > > * > >+ * @param aSkipRect An area in device pixels (NOT app units!) to avoid > >+ * blurring over, to prevent unnecessary work. > >+ * > > Is there a way we can keep the dirty rect and skip rect in the same units? I think it's more convenient for the callers for them to be in the units they're currently in.

Hm, something that just occurred to me: we only have one build tier now, so is libgfxutil still required?

(In reply to comment #18) > (In reply to comment #16) > > (From update of attachment 425733 [details] [diff] [review] [details]) > > In general, I'm not in love with this patch. Mostly, I dislike the idea of a > > "skip rect" -- I'd much rather have a mask which defaults to the full rect. > > For now, though, I'm going to review this as if I was OK with skip rects. > > How would you represent such a mask? Probably as a 2D bit array. > > What's the purpose of offsetting the skip rect by the shadow rect? It > > especially doesn't make sense given that it's possible that they're the same. > > It's making the skip rect relative to the shadow rect. But why? It's easier to read when they're both in the same coordinate space, don't you find?

(In reply to comment #20) > > > What's the purpose of offsetting the skip rect by the shadow rect? It > > > especially doesn't make sense given that it's possible that they're the same. > > > > It's making the skip rect relative to the shadow rect. > > But why? It's easier to read when they're both in the same coordinate space, > don't you find? As we go through each pixel in the image surface, we can directly compare the X and Y values we have in the for loop with the coordinates of the skip rect.

(In reply to comment #19) > Hm, something that just occurred to me: we only have one build tier now, so is > libgfxutil still required? I'm not sure.

(In reply to comment #20) > (In reply to comment #18) > > (In reply to comment #16) > > > (From update of attachment 425733 [details] [diff] [review] [details] [details]) > > > In general, I'm not in love with this patch. Mostly, I dislike the idea of a > > > "skip rect" -- I'd much rather have a mask which defaults to the full rect. > > > For now, though, I'm going to review this as if I was OK with skip rects. > > > > How would you represent such a mask? > > Probably as a 2D bit array. That would be a lot more complex, and it's complexity we wouldn't use in any common case. It also would also prevent the reduction from O(W*H) to O(W+H) in the box-blur subroutines.

(In reply to comment #22) > (In reply to comment #19) > > Hm, something that just occurred to me: we only have one build tier now, so is > > libgfxutil still required? > > I'm not sure. (Pardon my ignorance) What do you mean by "one build tier" and why does that mean we don't need a new library?

I don't think the tier change changes the linkage decisions at all. However, I would probably just make this always static library unless you have some compelling reason to do otherwise.

(In reply to comment #16) > > for (PRInt32 y = 0; y < aRows; y++) { > >+ // Check whether the skip rect intersects this row. If the skip > >+ // rect covers the whole surface in this row, we can avoid > >+ // this row entirely (and any others along the skip rect). > >+ PRBool inSkipRectY = aSkipRect && y >= aSkipRect->y && > >+ y < aSkipRect->YMost(); > >+ if (inSkipRectY && skipRectSpans) { > >+ y = aSkipRect->YMost() - 1; > >+ continue; > >+ } > >+ > > PRInt32 alphaSum = 0; > > for (PRInt32 i = 0; i < boxSize; i++) { > > PRInt32 pos = i - aLeftLobe; > > pos = PR_MAX(pos, 0); > > pos = PR_MIN(pos, aStride - 1); > > alphaSum += aInput[aStride * y + pos]; > > } > > for (PRInt32 x = 0; x < aStride; x++) { > >+ // Check whether we are within the skip rect. If so, go > >+ // to the next point outside the skip rect. > >+ if (inSkipRectY && x >= aSkipRect->x && > >+ x < aSkipRect->XMost()) { > >+ x = aSkipRect->XMost(); > > Do you have numbers that show that skipping by changing the loop variable in > this way is helpful? I'd much prefer a simple condition along the lines of "if > (skipping) continue;". It means we don't have to do constant continue'ing for every pixel, we just do the jump. I don't have numbers, but I don't see how it is harder to understand. > >diff --git a/layout/base/nsCSSRendering.h b/layout/base/nsCSSRendering.h > >--- a/layout/base/nsCSSRendering.h > >+++ b/layout/base/nsCSSRendering.h > >@@ -412,32 +412,35 @@ public: > > * DoPaint(). To set the color of the > > * resulting blurred graphic mask, you must > > * set the color on this context before > > * calling Init(). > > * > > * @param aDirtyRect The absolute dirty rect in app units. Used to > > * optimize the temporary surface size and speed up blur. > > * > >+ * @param aSkipRect An area in device pixels (NOT app units!) to avoid > >+ * blurring over, to prevent unnecessary work. > >+ * > > Is there a way we can keep the dirty rect and skip rect in the same units? The problem is, that would result in a lot more code.

(In reply to comment #16) > Finally, inSkipRectY's condition is a lot more understandable, at least by me. > If you integrated those two checks, I'd be a lot happier. What do you mean by this? If you mean fold more conditions into inSkipRectY, this variable is read twice in the function and the second time, it requires that only the y-coordinates have been checked.

I think Joe misunderstood the meaning of skipRectSpans, which is understandable since it's a poor name. Let's call it skipRectCoversEntireRow (and later, skipRectCoversEntireColumn)?

I tried to address all the review comments I could understand or reasonably do. If I missed anything that roc or I didn't address in previous comments, please elaborate. Ted, I tried making it a static library but it wouldn't compile, because I suspect of the dependencies on shared libraries. I just set FORCE_STATIC_LIB, if there was another method I'm supposed to do, please say so. Thanks.

Comment on attachment 437773 [details] [diff] [review] Patch 6 General comment: use NS_MIN/NS_MAX instead of PR_MIN/PR_MAX, since the latter evaluates (one of) its arguments twice. >diff --git a/gfx/thebes/src/gfxBlur.cpp b/gfx/src/thebes/utils/gfxBlur.cpp >+// Copied from nsSVGUtils >+// Converts a gfxRect to an nsIntRect for speed >+nsIntRect >+gfxAlphaBoxBlur::GfxRectToIntRect(const gfxRect& aIn) >+{ >+ nsIntRect result(PRInt32(aIn.X()), PRInt32(aIn.Y()), >+ PRInt32(aIn.Width()), PRInt32(aIn.Height())); >+ NS_ASSERTION(gfxRect(result.x, result.y, result.width, result.height) == aIn, >+ "The given gfxRect isn't rounded properly!"); >+ return result; >+} This isn't logically part of gfxAlphaBoxBlur, so it should be put into something external inside utils. Make a followup bug to do that (and to remove nsSVGUtils' duplicate version too). > gfxContext* > gfxAlphaBoxBlur::Init(const gfxRect& aRect, > const gfxIntSize& aBlurRadius, >- const gfxRect* aDirtyRect) >+ const gfxRect* aDirtyRect, >+ gfxRect* aSkipRect) aSkipRect should be const (see below). >+ if (aSkipRect) { >+ // If we get passed a skip rect, we can lower the amount of >+ // blurring we need to do. We convert it to nsIntRect to avoid >+ // expensive int<->float conversions if we were to use gfxRect instead. >+ aSkipRect->RoundIn(); Please don't modify your parameters! Make a copy and modify that instead. > BoxBlurHorizontal(unsigned char* aInput, > { > PRInt32 boxSize = aLeftLobe + aRightLobe + 1; >+ PRBool skipRectCoversWholeRow = aSkipRect.x <= 0 && >+ aSkipRect.XMost() >= aWidth - 1; This still breaks my brain. I really don't like the skip rect being relative to the actual rect, particularly given that the calculation of inSkipRectY is so much nicer. It might help a little if you just reorder it so that it's in the same order as in the calculation of inSkipRectY. > BoxBlurVertical(unsigned char* aInput, > { > PRInt32 boxSize = aTopLobe + aBottomLobe + 1; >+ PRBool skipRectCoversWholeColumn = aSkipRect.y <= 0 && >+ aSkipRect.YMost() >= aRows - 1; Same comments as BoxBlurHorizontal.

Thanks very much. I decided to move the function out in this patch anyway, but getting nsSVGUtils to use it will be a followup.

The patch seems to have stuck. http://hg.mozilla.org/mozilla-central/rev/8bf90a5aba8d

Do I read correctly that this creates a new library in shared builds? If so, it's clear to me why it broke all packaged tests on SeaMonkey (we need to do shared builds because we can't do libxul yet and static breaks with compiling some tests). In that case, I'll file a followup to package that lib there.

Actually, I just filed bug 560096.

Yes, apologies. I had to make a new library to get this to build in Thunderbird.