Create generic 8-bit alpha box blur in thebes

RESOLVED FIXED

Status

()

RESOLVED FIXED
11 years ago
10 years ago

People

(Reporter: ebutler, Assigned: ebutler)

Tracking

Trunk
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 4 obsolete attachments)

(Assignee)

Description

11 years ago
Created attachment 333303 [details] [diff] [review]
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.
Attachment #333303 - Flags: review?(vladimir)
(Assignee)

Comment 1

11 years ago
Created attachment 333322 [details] [diff] [review]
ver 2, sans compilation errors

Lets try that again, except adding the makefile changes so it actually compiles.
Attachment #333303 - Attachment is obsolete: true
Attachment #333322 - Flags: review?(vladimir)
Attachment #333303 - Flags: review?(vladimir)

Comment 2

11 years ago
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.
(Assignee)

Comment 3

11 years ago
> 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

11 years ago
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 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?)
Attachment #333322 - Flags: review?(vladimir) → review+
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

11 years ago
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.
(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.
(Assignee)

Comment 9

11 years ago
Created attachment 334007 [details] [diff] [review]
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.
Attachment #333322 - Attachment is obsolete: true
(Assignee)

Comment 10

11 years ago
Created attachment 334008 [details] [diff] [review]
ver 4

whoops, I had forgotten to fix the zero-size thing. Now fixed.
Attachment #334007 - Attachment is obsolete: true
(Assignee)

Comment 11

11 years ago
Created attachment 334474 [details] [diff] [review]
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.
Attachment #334008 - Attachment is obsolete: true
Checked in - http://hg.mozilla.org/mozilla-central/rev/b643a867e4d1
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.