Closed Bug 368247 Opened 17 years ago Closed 17 years ago

Rewrite border rendering for Thebes

Categories

(Core :: Layout, defect)

x86
All
defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: vlad, Assigned: vlad)

References

(Depends on 3 open bugs)

Details

Attachments

(7 files, 6 obsolete files)

119.18 KB, image/png
Details
5.25 KB, text/html
Details
2.94 KB, text/html
Details
130.21 KB, patch
Details | Diff | Splinter Review
901 bytes, text/html
Details
91.06 KB, patch
roc
: review+
Details | Diff | Splinter Review
81.81 KB, image/png
Details
There are a pile of bugs in border rendering that can be fixed by greatly simplifying the border rendering code by using the Thebes APIs directly.
dbaron had some issues that he wanted fixed as well; opening this bug to track that work.  I'll probably take a stab at this starting in the next one to two weeks.
Fixing issues that show up with non-opaque rgba() colors is pretty near the top of the list.

In particular, this means rewriting dashed and dotted borders to share more of the existing code with other border types so that they get border beveling.  Dashed and dotted borders should always begin having gaps in them past the bevel.  So that they look nice, the distance from the outside edge of the bevel to the first gap in dashed/dotted borders should be a function of the two border widths at the bevel and the dash ratio (1 for dotted, the ratio of desired dash length/width for dashed), for the left end of a top border (and the equivalent for the other 7) probably something like max(border-left-width, dash-ratio * border-top-width).  (So the length of the dashes/dots or the size of the gaps between them has to vary a little bit with the length of the borders so that you always get an integral number of dashes in the right space -- I think we do that part of it right already.)  A bunch of this is covered in bug 19963.

It would also be nice to fix dotted border drawing to use circles (bug 1515), but that can be done in a separate patch later; getting the corners right in that case is nontrivial.
(Though (dash-ratio + 1) / 2 may look better than dash-ratio as a multiplication factor there.  It probably requires looking at some real examples.)
(And simplifying a lot of the border-drawing code by doing transformations on the rendering context to do the different sides would be lovely. :-)
Doing circles for dots will be harder, since we can't use the existing cairo dashed mechanics yet.  There is some stuff in the pipeline for letting you do strokes with arbitrary paths along the stroke that we'll be able to use once that happens, but I don't know how far off that is.

My plan is to combine all the dotted/dashed/solid code into one, because we -should- be able to render everything using the same path.  We have full control over cairo's dashing (dash start, dash pattern, etc.), so if we have a dotted/dashed border, we just need to calculate those bits correctly and then just draw lines as normal.

I talked about some ideas in bug 328241, but I'll post a more detailed plan on fixing the non-opaque border sides issue.
So here's what I'm thinking:

(Note: "style" here means a single border-style/border-color/border-width combination)

• If all borders are the same style (whether with or without opacity), draw a single stroked path of the appropriate color/width.
• If borders are different styles, but the colors involved are fully opaque, draw each part separately.
• If borders are different styles and/or different opacities, then:
	○ Start drawing to a temporary surface
	○ Draw the borders using SATURATE
		§ This will not be hardware accelerated, but shouldn't be hit all that often
	○ Composite the temporary surface back to the rendered area

That's the basic rendering path for dealing with opacity.  Each side of that will be rendered using thebes directly, since we can do all the curves/dotting/dashing/etc. all at once.

We can also clip the background to rounded borders, but that would need to be done in a different place than this.
Blocks: 369647
Attached file some testcases (obsolete) —
Just some testcases where the current border-code does strange things.
Status: NEW → ASSIGNED
Status: ASSIGNED → NEW
Blocks: 367498
Thanks for the testcase!  Here's what it looks like with what's in my tree right now.  Note that the black backgrounds don't line up in most cases because the background rendering code is generating that rounded polygon using a totally different mechanism.
bug 141715 is probably also related, and could be fixed with this one.
Depends on: 141715
Attached file updated testcase
Updated testcase. Added some tests with -moz-border-xxx-colors, fixed some minor errors and typos.
Attachment #254422 - Attachment is obsolete: true
Attached patch work-in-progress, v0.1 (obsolete) — Splinter Review
Ok, here's a first patch in progress.  I'm posting this to solicit feedback particularily in code readability, ideas for areas for refactoring/cleanup, areas that need more documentation, etc.  Obviously some stuff in the patch won't remain (e.g. the debug printing code), and it won't live in a separate nsCSSRenderingThebes.cpp file :)

This patch fixes a number of the bugs that are attached to this bug.  There are some known issues, mainly around dashed/dotted borders and -moz-border-radius:

- dotted/dashed border sides appear ugly, particularly at the corners
- mixing dotted/dashed borders with any other border style causes triangular overdraw at corners
- background rendering with -moz-border-radius doesn't agree with border rendering of the curve, causing the background to spill out past the border
- -moz-border-radius doesn't gracefully handle corner joins when the border widths differ
- color computation for groove/etc. is different (this may or may not be a problem; I need to look again at what the specs say for how those colors should be calculated)
Attachment #256111 - Flags: superreview?
Attachment #256111 - Flags: review?(bzbarsky)
Attachment #256111 - Flags: superreview? → superreview?(dbaron)
I'm not sure when I'll be able to look at this... Definitely not till sometime next week at best.  :(
Attached patch thebes border rendering (obsolete) — Splinter Review
Ok, an updated patch.  Some of the issues mentioned in the above still remain -- specifically, ugly dashed/dotted rendering with thick borders, and less "polish" at corner joins of borders with border radius and different thickness on the two sides.  This also handles outlines using much the same code as borders.

However, all the reftests pass (I think -- there are some phantom failures that seem to be related to a broken font on Vista, but nothing related to borders).

I've left it split out into a separate nsCSSRenderingThebes file for now for ease of review, but I would fold all the code into nsCSSRendering.cpp.
Attachment #256111 - Attachment is obsolete: true
Attachment #259158 - Flags: review?(dbaron)
Attachment #256111 - Flags: superreview?(dbaron)
Attachment #256111 - Flags: review?(bzbarsky)
FYI - this patch doesn't apply cleanly to the trunk anymore:

patching file `gfx/thebes/public/gfxColor.h'
patching file `gfx/thebes/public/gfxContext.h'
patching file `gfx/thebes/public/gfxRect.h'
patching file `gfx/thebes/src/gfxContext.cpp'
patching file `gfx/thebes/src/gfxRect.cpp'
Hunk #2 succeeded at 58 with fuzz 2.
Hunk #3 succeeded at 72 with fuzz 2 (offset 1 line).
patching file `gfx/thebes/test/gfxFontSelectionTests.h'
Hunk #1 FAILED at 154.
1 out of 1 hunk FAILED -- saving rejects to gfx/thebes/test/gfxFontSelectionTests.h.rej
patching file `layout/base/nsCSSRendering.cpp'
Hunk #1 succeeded at 364 (offset -1 lines).
Hunk #3 FAILED at 869.
Hunk #4 succeeded at 1959 (offset -5 lines).
1 out of 4 hunks FAILED -- saving rejects to layout/base/nsCSSRendering.cpp.rej
patching file `layout/base/nsCSSRendering.h'
patching file `layout/base/nsCSSRenderingThebes.cpp'
ugh.  I'll regenerate, though the gfxFontSelectionTests.h chunk shouldn't have been in there (and isn't relevant), and most of the nsCSSRendering.cpp bits are just #if 0'ing out large chunks of the file.
Comment on attachment 259158 [details] [diff] [review]
thebes border rendering

The gfxColor.h and gfxFontSelectionTests.h changes don't seem at all
related to this patch; they look fine but should at least go in with
different checkin comments.



+ * Figure out whether we need to draw using separate sides or not.  We
+ * don't do separate sides only if we have a SOLID or DOUBLE border
+ * with the same color on all sides, and no composite colors.

This comment is really hard to parse (too many negatives).  The code
below it makes sense, though.

Some misspellings (search for them, some repeated):
 strats, RECTANGE


+      // XXX this borderWidth calculation may be too simplistic
+      PRUint32 borderWidth = NSToCoordRound(float(gfxFloat(border.side(side)) / twipsPerPixel));

The style system should ensure you get only pixel-rounded border widths.
Can you depend on that, and simplify this?  If so, add an assertion.


+    // start a compositing group and render using SATURATE so that
+    // we get correct behaviour at the joins

So I've been thinking about whether saturate is the right operator.  I
don't think it is, because the math isn't symmetric.  I think (although
I don't have good sources for this) that the math is:
  A ADD B (Porter-Duff plus, quartz PlusLighter)
    C[out] = min(1, C[A] + C[B])    (same for alpha)
  A PlusDarker B (quartz only)
    alpha[out] = min(1, alpha[A] + alpha[B])
    C[out] = max(0, alpha[out] + C[A] + C[B] - alpha[A] - alpha[B])
    (which is the same as inverting the colors (pre-premultiplication),
    doing plus, and inverting the colors again)
  A SATURATE B:
    C[out] = C[A] * min(1, (1-alpha[B])/alpha[A]) + C[B].

Saturate also isn't implemented correctly in cairo-quartz-surface.c --
it uses PlusDarker.  Is the way you're using it going to always go
through pixman, or will this go through the platform surfaces?  If it
does, using saturate will mean we'll eventually be slower an quartz once
it has the slow fallback case written.

It's also worth noting that quartz's PlusLighter and PlusDarker (Apple's
canvas supports both) are always the same when the alphas add to less
than 1, which should always be the case for what you're doing, since
you're drawing non-overlapping regions onto an initially-transparent
surface and using this only to avoid antialising effects.

So I think what you want to get rgba() colors right is
CAIRO_OPERATOR_ADD... which is also the most interoperable.


Some reftests that I think we should add, that I'm hoping this patch
will make us pass, are:

 * In pixel-rounding (since they should be repeated for the different
   position and thickness possibilities), an assertion that if we have
   one side with a border color different from the background, we get
   the same rendering for:
    * all the other sides having border-*-color: transparent
    * all the other sides having border-*-color == background-color
   (repeated for each side)

 * A test asserting that a certain rgba() border on a certain
   background appears the same as an rgb() border computed to get the
   same color result (although I think there would be accumulated
   rounding error that would prevent this from passing).

Some other reftests that we ought to add that this patch shouldn't break
are reftests for aSkipSides (i.e., inlines broken across lines and lines
broken across columns), compared to reference renderings constructed out
of nested backgrounds.


+      // RECTANGE: used for dotted/dashed borders so that we can draw the right
+      // sides correctly.

I'm not sure this is the right approach in the long term, but it works
for now.

Please get bugs on file (if they aren't already, which they might be for
the first) on:
 * improving dashed/dotted border joins
 * improving border-radius combined with different border thicknesses


+#define DEBUG_NEW_BORDERS

You probably want that commented out or #undef-ed

+static void S(const gfxPoint& p) {}
+static void S(const gfxSize& s) {}
+static void S(const gfxRect& r) {}
+static void S(const gfxFloat f) {}
+static void S(const char *s) {}
+static void SN(const char *s = nsnull) {}
+static void SF(const char *fmt, ...) {}

|static inline void| for all.


Should groove and ridge borders use light/dark rather than light/solid?
I thought that's what we do now.


Have you looked at testcases for border-collapse tables?


PaintOutline looks like it doesn't have anything to not bevel the sides
when aSkipSides are set.  (I'm sort of surprised it has an aSkipSides
argument at all, though.)



I'm going to mark this r=dbaron although I haven't read every line.  I
think there's decreasing value in reading this code, although it would
be good to add reftests.  I'd appreciate if you could add reftests for
any of the interesting issues you've run into, and also for the issues I
mentioned above.  And I'm assuming you'll be responsive to fixing issues
that turn up (I actually prefer working that way in general, but I've
found that post-landing review comments tend to get ignored).

Given the amount of code that's repeated for each side, it's probably
also good to have tests that tests sides individually and in interesting
combinations rather than everything testing all sides.
Attachment #259158 - Flags: review?(dbaron) → review+
> +      PRUint32 borderWidth = NSToCoordRound(float(gfxFloat(border.side(side))
> / twipsPerPixel));

Shouldn't we do that using AppUnitsToDevUnits? twipsPerPixel is pre-unit fix (though I'm not too fond in Thebes/Cairo, so I might be wrong).
Here's a testcase using canvas for the different composite operations; dbaron and I convinced ourselves that ADD is what we want.
Updated patch, most likely what will land.  Fixed clipping to never do overdraw; uses OPERATOR_ADD now.
Attachment #259158 - Attachment is obsolete: true
Attachment #259158 - Flags: superreview?(roc)
(That patch still had #define DEBUG_NEW_BORDERS; I've #undef'd that now.)
Depends on: 379328
This is checked in; leaving bug open for followup.
No longer depends on: 379328
Depends on: 379328
Never mind, marking FIXED, but still following.
Blocks: 141715
No longer depends on: 141715
Comment on attachment 263293 [details] [diff] [review]
updated patch, thebes border rendering

In
>+void
>+nsCSSRendering::PaintBorder(nsPresContext* aPresContext,
>+                            nsIRenderingContext& aRenderingContext,
>+                            nsIFrame* aForFrame,
>+                            const nsRect& aDirtyRect,
>+                            const nsRect& aBorderArea,
>+                            const nsStyleBorder& aBorderStyle,
>+                            nsStyleContext* aStyleContext,
>+                            PRIntn aSkipSides,
>+                            nsRect* aGap,
>+                            nscoord aHardBorderSize,
>+                            PRBool  aShouldIgnoreRounded)
>+{
>   nsMargin            border;
>   nsStyleCoord        bordStyleRadius[4];
>+  PRInt16             borderRadii[4], i;
...
>+  for (PRInt32 i = 0; i < numSides; i++) {
and
>+void
>+nsCSSRendering::PaintOutline(nsPresContext* aPresContext,
>+                             nsIRenderingContext& aRenderingContext,
>+                             nsIFrame* aForFrame,
>+                             const nsRect& aDirtyRect,
>+                             const nsRect& aBorderArea,
>+                             const nsStyleBorder& aBorderStyle,
>+                             const nsStyleOutline& aOutlineStyle,
>+                             nsStyleContext* aStyleContext,
>+                             nsRect* aGap)
>+{
>+  nsStyleCoord        bordStyleRadius[4];
>+  PRInt16             borderRadii[4],i;
...
>+  for (PRInt32 i = 0; i < numSides; i++) {
did you mean to "redeclare" i?
The -moz-border-radius seems to be drawn incorrect. See attachment.

The background rendering should also be adjusted to match the new borders.
Setting a black background color on that object reveals an even worse problem: there is black around the red area.
Stefanik, please file new bugs on the issues, you are seeing that are caused by this patch and make it blocking this bug.
Depends on: 379419
Depends on: 379437
Depends on: 379446
Depends on: 379474
Depends on: 379479
Depends on: 379505
No longer depends on: 379497
Depends on: 379672
"Jobb törött drágakőként meghalni, mint sárként élni!" - Bruce Lee
Depends on: 379834
Blocks: 368555
It looks like Vlad forgot to really mark this fixed. Doing it now.
Status: NEW → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
Depends on: 380549
Depends on: 380601
Depends on: 380602
No longer blocks: 141715
Depends on: 141715
Reopening.  Going to do this using an alternate approach, because the way it's currently done (stroke only) causes too many issues that are unfixable/difficult to fix.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attached patch new border rendering approach (obsolete) — Splinter Review
Ok, here's a new patch.  This patch fixes all the current known regressions (with the exception of bug 379834) in addition to providing significant speedups in many of the most common border cases.  Bug 379834 requires a cairo upgrade to fix.

The previous approach attempted to use Stroke() with various clip regions to render borders.  This ended up being impossible for a number of reasons, most having to do with -moz-border-radius (when the border radius was less than the stroke width).

The most generic path through this patch renders borders using fill(), with a clip set for each side.

Dotted/dashed borders are done by rendering the corners, and then rendering the straight parts between them dotted/dashed.  (Caveats: -moz-border-*-colors does not work with dotted/dashed; also, -moz-border-radius: 100% will not result in a dotted/dashed circle, because the entire radius is considered a "corner".  The latter (well, both) could be fixed at some later time if someone's bored.)

Solid borders of equivalent thickness on all sides (and with no radius) are rendered using Stroke, because stroking is faster than filling for simple paths.  1px borders that require a topleft/bottomright split are also rendered using stroke, and are not antialiased in the two corner joins, because noone would notice, and it's much slower.
Attachment #266322 - Flags: review?(roc)
+        size.width -= k * 2.0;
+        size.height -= k * 2.0;

We don't allow negative widths/heights anywhere else in gfxRect, currently. How about throwing in PR_MAX(0, ...) here and elsewhere in this file?

     if (i == 0)
       firstSideStyle = borderRenderStyle;
     else if (borderRenderStyle != firstSideStyle)

Just initialize firstSideStyle to borderStyles[0] and firstSideColor to borderColors[0] outside the loop and avoid these conditionals...

+static void
+GetBorderCornerDimensions(const gfxRect& oRect,
+                          const gfxRect& iRect,
+                          gfxFloat *radii,
+                          gfxSize *oDims)

A comment here would be nice...

+  // the 1.0 could be a 2.0 (or anything > 1.0) to give more of a corner in dotted/dashed
+  // rendering cases.  It's not clear which looks better.

Define it as a constant instead of repeating 1.0 many times.

+  oDims[C_TL] = gfxSize(PR_MIN(halfWidth, leftWidth * 1.0), PR_MIN(halfHeight, topWidth * 1.0));

er... shouldn't these be PR_MAX?

+static void
+DoCornerClipSubPath(gfxContext *ctx,
+                    const gfxRect& oRect,
+                    const gfxRect& iRect,
+                    gfxFloat *radii,
+                    PRIntn dashedSides = 0xff)

need a comment?

I get the feeling that a lot of this code, like this function, could be rewritten as a loop over the 4 corners, with some judicious helper functions or helper arrays. We can talk more about how to do this... Without that I just have to rubberstamp all the geometry, it's too much.

+  gfxFloat zeroRadii[4] = { 0.0 };

Make this static? And make all the radii const gfxFloat *?

+  innerRadii[C_TL] = PR_MAX(0.0, radii[C_TL] - PR_MAX(borderSizes[NS_SIDE_TOP], borderSizes[NS_SIDE_LEFT]));
+  innerRadii[C_TR] = PR_MAX(0.0, radii[C_TR] - PR_MAX(borderSizes[NS_SIDE_TOP], borderSizes[NS_SIDE_RIGHT]));
+  innerRadii[C_BR] = PR_MAX(0.0, radii[C_BR] - PR_MAX(borderSizes[NS_SIDE_BOTTOM], borderSizes[NS_SIDE_RIGHT]));
+  innerRadii[C_BL] = PR_MAX(0.0, radii[C_BL] - PR_MAX(borderSizes[NS_SIDE_BOTTOM], borderSizes[NS_SIDE_LEFT]));

This code occurs 3 times I believe... It could be shared, or even made into a loop with the help of an array like this:
  struct SidePair {
    PRUint8 horz;
    PRUint8 vert;
  };
  SidePair cornerSides[4] = { { NS_SIDE_TOP, NS_SIDE_LEFT}, ... };

+        siRect.Outset(innerBorderWidths[NS_SIDE_TOP] + middleBorderWidths[NS_SIDE_TOP],
+                      innerBorderWidths[NS_SIDE_RIGHT] + middleBorderWidths[NS_SIDE_RIGHT],
+                      innerBorderWidths[NS_SIDE_BOTTOM] + middleBorderWidths[NS_SIDE_BOTTOM],
+                      innerBorderWidths[NS_SIDE_LEFT] + middleBorderWidths[NS_SIDE_LEFT]);

This code is repeated a few times...

+        soRect.Inset(outerBorderWidths[NS_SIDE_TOP],
+                     outerBorderWidths[NS_SIDE_RIGHT],
+                     outerBorderWidths[NS_SIDE_BOTTOM],
+                     outerBorderWidths[NS_SIDE_LEFT]);

So is this. How about making Outset and Inset take a gfxFloat* parameter. Then you could write
   siRect.Outset(innerBorderWidths);
   siRect.Outset(middleBorderWidths);
etc

+      soRect.Inset(fakeBorderSizes[0], fakeBorderSizes[1], fakeBorderSizes[2], fakeBorderSizes[3]);

Could use that here too.

+static void
+DrawDashedSide(gfxContext *ctx,

Up to here.
(In reply to comment #31)
> +        size.width -= k * 2.0;
> +        size.height -= k * 2.0;
> 
> We don't allow negative widths/heights anywhere else in gfxRect, currently. How
> about throwing in PR_MAX(0, ...) here and elsewhere in this file?


Done.

>      if (i == 0)
>        firstSideStyle = borderRenderStyle;
>      else if (borderRenderStyle != firstSideStyle)
> 
> Just initialize firstSideStyle to borderStyles[0] and firstSideColor to
> borderColors[0] outside the loop and avoid these conditionals...


Done.

> +static void
> +GetBorderCornerDimensions(const gfxRect& oRect,
> +                          const gfxRect& iRect,
> +                          gfxFloat *radii,
> +                          gfxSize *oDims)
> 
> A comment here would be nice...
> 
> +  // the 1.0 could be a 2.0 (or anything > 1.0) to give more of a corner in
> dotted/dashed
> +  // rendering cases.  It's not clear which looks better.
> 
> Define it as a constant instead of repeating 1.0 many times.

Done, added comments.

> +  oDims[C_TL] = gfxSize(PR_MIN(halfWidth, leftWidth * 1.0), PR_MIN(halfHeight,
> topWidth * 1.0));
> 
> er... shouldn't these be PR_MAX?
> 
> +static void
> +DoCornerClipSubPath(gfxContext *ctx,
> +                    const gfxRect& oRect,
> +                    const gfxRect& iRect,
> +                    gfxFloat *radii,
> +                    PRIntn dashedSides = 0xff)
> 
> need a comment?

Yup.

> I get the feeling that a lot of this code, like this function, could be
> rewritten as a loop over the 4 corners, with some judicious helper functions or
> helper arrays. We can talk more about how to do this... Without that I just
> have to rubberstamp all the geometry, it's too much.

So, it could be, but I think it'd be more complicated than necessary.  You'd need an array telling you for each corner whether to add or remove the radius from the x/y pos, and then you'd have a bunch of multiplies and the like.  You'd have to special case the MoveTo() at the start and ClosePath() at the end.  I don't think it'd be all that much cleaner.

FWIW, the geometry bits (CW/CCW rounded rects, border corners, etc.) are almost certainly correct; it was all prototyped using canvas, and I've tested it all using stroke() as well as looking at the path points.

> +  gfxFloat zeroRadii[4] = { 0.0 };
> 
> Make this static? And make all the radii const gfxFloat *?

Actually, this can go away; radii is never null.  I just got rid of it, and then sprinkled a pile of constswhere they should go.

> +  innerRadii[C_TL] = PR_MAX(0.0, radii[C_TL] -
> PR_MAX(borderSizes[NS_SIDE_TOP], borderSizes[NS_SIDE_LEFT]));
> +  innerRadii[C_TR] = PR_MAX(0.0, radii[C_TR] -
> PR_MAX(borderSizes[NS_SIDE_TOP], borderSizes[NS_SIDE_RIGHT]));
> +  innerRadii[C_BR] = PR_MAX(0.0, radii[C_BR] -
> PR_MAX(borderSizes[NS_SIDE_BOTTOM], borderSizes[NS_SIDE_RIGHT]));
> +  innerRadii[C_BL] = PR_MAX(0.0, radii[C_BL] -
> PR_MAX(borderSizes[NS_SIDE_BOTTOM], borderSizes[NS_SIDE_LEFT]));
> 
> This code occurs 3 times I believe... It could be shared, or even made into a
> loop with the help of an array like this:
>   struct SidePair {
>     PRUint8 horz;
>     PRUint8 vert;
>   };
>   SidePair cornerSides[4] = { { NS_SIDE_TOP, NS_SIDE_LEFT}, ... };
> 
> +        siRect.Outset(innerBorderWidths[NS_SIDE_TOP] +
> middleBorderWidths[NS_SIDE_TOP],
> +                      innerBorderWidths[NS_SIDE_RIGHT] +
> middleBorderWidths[NS_SIDE_RIGHT],
> +                      innerBorderWidths[NS_SIDE_BOTTOM] +
> middleBorderWidths[NS_SIDE_BOTTOM],
> +                      innerBorderWidths[NS_SIDE_LEFT] +
> middleBorderWidths[NS_SIDE_LEFT]);
> 
> This code is repeated a few times...
> 
> +        soRect.Inset(outerBorderWidths[NS_SIDE_TOP],
> +                     outerBorderWidths[NS_SIDE_RIGHT],
> +                     outerBorderWidths[NS_SIDE_BOTTOM],
> +                     outerBorderWidths[NS_SIDE_LEFT]);
> 
> So is this. How about making Outset and Inset take a gfxFloat* parameter. Then
> you could write
>    siRect.Outset(innerBorderWidths);
>    siRect.Outset(middleBorderWidths);
> etc

So, I thought about that, but I didn't want to hardcode the top, right, bottom, left order into gfxRect.  Though I guess it would make the code quite a bit cleaner, so I'll do that.

> +      soRect.Inset(fakeBorderSizes[0], fakeBorderSizes[1], fakeBorderSizes[2],
> fakeBorderSizes[3]);
> 
> Could use that here too.

Done.  Will attach an updated patch shortly.
Updated patch with review comments.  Ignore the bit about adding a new test (I don't want to check it in because it basically duplicates all of the border rendering code from nsCSSRendering... though I'll try to find some way to get the test harness in there.)
Attachment #266322 - Attachment is obsolete: true
Attachment #266451 - Flags: review?(roc)
Attachment #266322 - Flags: review?(roc)
Er, let's try that again; wrong (old) patch last time.
Attachment #266451 - Attachment is obsolete: true
Attachment #266454 - Flags: review?(roc)
Attachment #266451 - Flags: review?(roc)
Some comments from a passersby reviewer:
in gfxRect.h
    void Inset(const gfxFloat *sides) {
         Outset(sides[0], sides[1], sides[2], sides[3]);
    }
This doesn't feel right: Inset calling Outset...

Some other nits:

In NumBorderPasses:
    // if we have compositeColors, we have to do 4.  We could maybe do 1
    // if all the sides are the same, but we don't bother.
    if (compositeColors[side])
      return 4;
This can be moved above the case, so that two if/returns can be combined

    sideColor = borderColors[side];
    // do separate sides if we have different colors on different sides
    if (sideColor != firstSideColor)
      return 4;
Likewise, and there is no need to copy bordersColors[side] in to a variable (which has function scope, so it won't easily optimized out).

So:
    // if the styles or colors don't match, split into 4.
    // if compositeColors are used, we have to do in 4.
    // we could do extra checking on top/left and bottom/right to
    // see if we can split into two, but we don't bother.
    if (borderRenderStyle != firstSideStyle) ||
        compositeColors[side] ||
        (borderColors[side] != firstSideColor)
      return 4;


In DoBottomRightSidesBorderPath:
  gfxFloat zeroRadii[4] = { 0.0 };
  if (radii == nsnull)
    radii = zeroRadii;
This will result in an array allocated on the stack, and filled in runtime, even if zeroRadii is not used.

In FillFastBorderPath:
  // if all radii == 0.0 and all borderSizes are the same,
  // use stroke!
This comment should be move to above the related if statement.
    if () {
      ...
      return;
    } else if () {
Prefered style:
    if () {
      ...
      return;
    }
    if () {
      ...


But overall this looks very nice, and I hope that this will get the performance and quality improvement that the borders need.
+          borderWidths[0] == 1.0 &&
+          borderWidths[1] == 1.0 &&
+          borderWidths[2] == 1.0 &&
+          borderWidths[3] == 1.0 &&

This is duplicated ... share it.

+    gfxRect gapRect(gfxFloat(aGap->x) / twipsPerPixel,
+                    gfxFloat(aGap->y) / twipsPerPixel,
+                    gfxFloat(aGap->width) / twipsPerPixel,
+                    gfxFloat(aGap->height) / twipsPerPixel);
+    // draw the rectangle backwards, so that we get it
+    // clipped out via the winding rule
+    ctx->MoveTo(gapRect.pos);
+    ctx->LineTo(gapRect.pos + gfxSize(0.0, gapRect.size.height));
+    ctx->LineTo(gapRect.pos + gapRect.size);
+    ctx->LineTo(gapRect.pos + gfxSize(gapRect.size.width, 0.0));
+    ctx->ClosePath();
+    ctx->Clip();

This is duplicated ... share it.

Can't callers use CalculateInnerRadii instead of InsetBorderRadii?

+  if (radii[0] == 0.0 &&
+    if (borderWidths[0] == 1.0 &&
+          borderWidths[0] == 1.0 &&

A helper function to test if all 4 elements are equal to some value could be used in these 3 places.
+  if (numRenderPasses > 1 && !canAvoidGroup) {

This should just be "if (!canAvoidGroup)"

+  gfxRect oRect(gfxFloat(outerRect.x) / twipsPerPixel,
+                gfxFloat(outerRect.y) / twipsPerPixel,
+                gfxFloat(outerRect.width) / twipsPerPixel,
+                gfxFloat(outerRect.height) / twipsPerPixel);

Don't we have scaling routines that can do this less verbosely? I guess we don't, but we should. That could be used in many places...

That's all the specific comments I have. I find it hard to check the logic because there's so much code and geometry, so it's somewhat of a rubber-stamp review.

I do still think that a lot of the repetition could be avoided, but it's hard to be specific without actually doing the work myself. I suspect introducing a gfxMargin type instead of the 4-gfxFloat arrays would help a bit, e.g. here:
+    // make a copy, because we munge this during this function
+    for (int i = 0; i < 4; i++)
+      radii[i] = borderRadii[i];
We'd maybe also need some sort of abstraction for corners. It might be fun to figure this out. Maybe I'll have a look later.
Blocks: 381735
Ok, review comments addressed.  At some point we may want to look into having a gfxMargin-type class, maybe a gfxCornerSizes class or something.. or maybe a gfxRoundedRect class that knows how to do all the math/path stuff.
Attachment #266454 - Attachment is obsolete: true
Attachment #266536 - Flags: review?(roc)
Attachment #266454 - Flags: review?(roc)
Comment on attachment 266536 [details] [diff] [review]
new border rendering approach, v2.2

+static void
+RectToGfxRect(const nsRect& rect, nscoord twipsPerPixel, gfxRect& grect)

This should just return a gfxRect directly instead of taking grect as an out parameter.
Attachment #266536 - Flags: superreview+
Attachment #266536 - Flags: review?(roc)
Attachment #266536 - Flags: review+
Even better would be to make a constructor in gfxRect.h taking (const nsRect& rect, nscoord twipsPerPixel)

      gfxRect gapRect;
      RectToGfxRect(*aGap, twipsPerPixel, gapRect);
Then becomes:
      gfxRect gapRect(*aGap, twipsPerPixel);
Some very small nits:
in DrawDashedSide:
replace the if elseif/... with a switch/case

In DrawBorders:
if (numRenderPasses == 4) {: replace with switch/case

Question: regarding:
    if (sides == (SIDE_BIT_TOP | SIDE_BIT_LEFT) &&
               borderSizes[0] == 1.0 &&
               color.a == 1.0)
      ctx->SetLineWidth(1.0);
      ctx->NewPath();
      ctx->MoveTo(oRect.BottomLeft() + gfxSize(0.5, 0.0));
      ctx->LineTo(oRect.TopLeft() + gfxSize(0.5, 0.5));
      ctx->LineTo(oRect.TopRight() + gfxSize(0.0, 0.5));
      ctx->Stroke();
      return;
    }
Would this not also work for color.a != 1.0?
With borderSize[0] == 1.0, aliasing for alpha transparency is not really relevant.
gfxRect can't take a nsRect -- the two types don't know of eachother, and gfxRect won't include nsRect.  I guess the two things could be switch statements, though if..else is used in quite a lot of other places here, meh.

Good point on the traslucent colors... the 1px optimization could work for translucent colors, provided that OVER was used and we were careful to not draw overlap.  The problem is that the two halves are drawn with overlap in the bottom left and top right pixels; we could make it so that there's no overlap, but that would mean that, say, the bottom left was always drawn with a missing pixel on one side.  (Though as I type this, I guess that could be ok.)  Anyway, that can be fixed up in a followup.

Resolving this bug for now (again!).
Status: REOPENED → RESOLVED
Closed: 17 years ago17 years ago
Resolution: --- → FIXED
You could go completely overboard with testcases here with little effort, but committing at least the ones in the attachments is enough to satisfy me.  Getting the other testcases used in development committed would be good too.
Flags: in-testsuite?
Depends on: spirograph
Depends on: 382721
Depends on: 382791
Depends on: 383379
Depends on: 386944
Depends on: 389030
Depends on: 386072
Depends on: 392629
Depends on: 394630
Depends on: 395910
Depends on: 397303
Depends on: 405740
Depends on: 377419
Depends on: 422330
Depends on: 424946
No longer depends on: 424946
Depends on: 424950
Depends on: 424423
Depends on: 451522
Depends on: 448193
I can see bad regressions on the testcase with 3.6.3 (Windows) - should this bug be reopened? (Screenshot coming soon.)
This is how the testcase renders in 3.6.3 on my Windows box. Will check trunk soon.
Reproducible on trunk; regression range of 2008072303 to 2008072403 points to bug 424423 (slow border rendering).
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: