450178 - Create generic 8-bit alpha box blur in thebes

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Eric Butler [:ebutler]]

Attachment: ver 1

Layout has a box blur class for use with shadows. Canvas also has use for such a class (for shadows), so the box blur should be generalized and placed in a location more easily accessible. This is primarily a graphics operation, and placing it in thebes puts it in a better location to take advantage of any future graphics optimizations.

This patch creates a general box blur class gfxAlphaBoxBlur, and then implements nsContextBoxBlur using gfxAlphaBoxBlur as a back end. nsContextBoxBlur cannot be eliminated entirely since it makes a few more optimizations to ensure correctness that canvas cannot make. Canvas will use gfxAlphaBoxBlur directly.

Comment 1

[Eric Butler [:ebutler]]

Attachment: ver 2, sans compilation errors

Lets try that again, except adding the makefile changes so it actually compiles.

Comment 2

[Michael Ventnor]

Have you updated the class for recent stability fixes?

+static void
+BoxBlurVertical(unsigned char* aInput,
+                unsigned char* aOutput,
+                PRInt32 aTopLobe,
+                PRInt32 aBottomLobe,
+                PRInt32 aStride,
+                PRInt32 aRows)
+{
+    PRInt32 boxSize = aTopLobe + aBottomLobe + 1;
+
+    for (PRInt32 x = 0; x < aStride; x++) {
+        PRInt32 alphaSum = 0;
+        for (PRInt32 i = 0; i < boxSize; i++) {
+            PRInt32 pos = i - aTopLobe;
+            pos = PR_MAX(pos, 0);
+            pos = PR_MIN(pos, aStride - 1);

That needs to be PR_MIN(pos, aRows - 1) IIRC. You'd better check what nsContextBoxBlur is like now.

Comment 3

[Eric Butler [:ebutler]]

> That needs to be PR_MIN(pos, aRows - 1) IIRC.

So it does. I'll include that fix after vlad reviews the patch.

> Have you updated the class for recent stability fixes?

It looks to me as though it mimics the current nsContextBoxBlur class correctly, unless I missed something. Everything else in layout in untouched and so the existing code should be unaffected.

Comment 4

[Michael Ventnor]

You may also want to have a rect.IsEmpty() check somehow because if you make a 0x0 surface you will crash in the blur code when it tries to access aInput[-1].

Comment 5

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 333322 [details] [diff] [review]
ver 2, sans compilation errors

Looks great; do add the rect IsEmpty() check though, and probably just return nsnull from Init.

Also, is it worth optimizing aBlurRadius = (0,0) by just returning the actual destination context and doing PushGroup/PopGroup under the hood?  Probably not, but something to keep in mind.  (Maybe add a NS_WARNING in case aBlurRadius == 0,0 for debug builds, saying that the caller should avoid using this in that case?)

Comment 6

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 333322 [details] [diff] [review]
ver 2, sans compilation errors

Oop, one thing:

>+gfxContext*
>+gfxAlphaBoxBlur::Init(const gfxRect& aRect,
>+                      const gfxIntSize& aBlurRadius)
>+{
>+    mBlurRadius = aBlurRadius;
>+
>+    gfxRect rect(aRect);
>+
>+    rect.Outset(aBlurRadius.height, aBlurRadius.width,
>+                aBlurRadius.height, aBlurRadius.width);
>+    rect.RoundOut();
>+
>+    // Make an alpha-only surface to draw on. We will play with the data after
>+    // everything is drawn to create a blur effect.
>+    mImageSurface = new gfxImageSurface(gfxIntSize(static_cast<PRInt32>(rect.Width()), static_cast<PRInt32>(rect.Height())),
>+                                        gfxASurface::ImageFormatA8);
>+    if (!mImageSurface || mImageSurface->CairoStatus())
>+        return nsnull;
>+
>+    // Use a device offset so callers don't need to worry about translating
>+    // coordinates, they can draw as if this was part of the destination context
>+    // at the coordinates of mRect.
>+    mImageSurface->SetDeviceOffset(-rect.TopLeft());

-rect.TopLeft() is wrong -- the rect was outset by the blur radius.  The correct device offset should be - rect.TopLeft() + gfxSize(aBlurRadius.width, aBlurRadius.height), no?

Comment 7

[Michael Ventnor]

I don't think so, since the device offset is meant to relate to the surface (if that even makes sense). As it stands that code looks exactly like what was copied previously and changing it might break CSS shadows.

Comment 8

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

(In reply to comment #7)
> I don't think so, since the device offset is meant to relate to the surface (if
> that even makes sense). As it stands that code looks exactly like what was
> copied previously and changing it might break CSS shadows.

The device offset is supposed to specify where the origin is in relation to the surface.  For shadows, we're drawing identical content into a temporary buffer -- so we should be drawing at an identical location (which is the topleft of aRect, not of rect).  I'll take a look at the CSS shadows code to see what's going on there, but I suspect it's overcompensating for shadow offsets like the canvas code was.  We shouldn't need to change the underlying matrix; in theory it shouldn't have any effect on the rendering if we translate, but it might, especially if scaling is also involved.

Comment 9

[Eric Butler [:ebutler]]

Attachment: ver 3

Fixes the problems addressed above, as well as one other typo.

>Also, is it worth optimizing aBlurRadius = (0,0) by just returning the actual
>destination context and doing PushGroup/PopGroup under the hood?  Probably not,
>but something to keep in mind.  (Maybe add a NS_WARNING in case aBlurRadius ==
>0,0 for debug builds, saying that the caller should avoid using this in that
>case?)


For now, it just silently skips all the blur code if the blur radius is zero. As canvas shadows stand now, an intermediate surface is needed anyway, and this reduces the number of special cases canvas would have to handle. Since we may be changing how canvas shadows are drawn, that could no longer be the case, but I haven't really evaluated how that will work yet. We should probably hold off landing this until I figure out what's going to happen with canvas shadows.

Comment 10

[Eric Butler [:ebutler]]

Attachment: ver 4

whoops, I had forgotten to fix the zero-size thing. Now fixed.

Comment 11

[Eric Butler [:ebutler]]

Attachment: ver 5

>-rect.TopLeft() is wrong -- the rect was outset by the blur radius.  The
>correct device offset should be - rect.TopLeft() + gfxSize(aBlurRadius.width,
>aBlurRadius.height), no?

No, I think Michael is right. If the caller draws something at aRect.TopLeft(), we don't want that to be flush up against the top left corner of the temporary surface. We want the offset such that there is room to blur. The original way was correct.

Comment 12

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Checked in - http://hg.mozilla.org/mozilla-central/rev/b643a867e4d1