Last Comment Bug 589648 - stroke-linecap:square doesn't show a line cap for zero-length path in SVG
: stroke-linecap:square doesn't show a line cap for zero-length path in SVG
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: SVG (show other bugs)
: Trunk
: x86 All
: -- normal (vote)
: ---
Assigned To: Jonathan Watt [:jwatt]
:
: Jet Villegas (:jet)
Mentors:
http://samples.msdn.microsoft.com/iet...
: 608751 (view as bug list)
Depends on: 657862 788862
Blocks: svg11tests ietestcenter
  Show dependency treegraph
 
Reported: 2010-08-22 18:03 PDT by Darxus
Modified: 2013-11-10 14:04 PST (History)
12 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
wanted


Attachments
patch 1: preliminary patch to add explicit 'segStart' (8.46 KB, patch)
2011-01-19 17:34 PST, Jonathan Watt [:jwatt]
longsonr: review+
Details | Diff | Splinter Review
patch 2: preliminary patch to stop hardcoding segment arg count (6.33 KB, patch)
2011-01-19 17:35 PST, Jonathan Watt [:jwatt]
longsonr: review+
Details | Diff | Splinter Review
patch 3: work around lack of cairo support (13.54 KB, patch)
2011-01-19 17:37 PST, Jonathan Watt [:jwatt]
longsonr: review-
Details | Diff | Splinter Review
patch 3: work around lack of cairo support (22.50 KB, patch)
2011-01-23 17:51 PST, Jonathan Watt [:jwatt]
longsonr: review+
Details | Diff | Splinter Review
patch 3: work around lack of cairo support (23.13 KB, patch)
2011-01-24 08:03 PST, Jonathan Watt [:jwatt]
roc: approval2.0-
Details | Diff | Splinter Review
patch 3: work around lack of cairo support (22.58 KB, patch)
2011-04-19 09:52 PDT, Jonathan Watt [:jwatt]
no flags Details | Diff | Splinter Review

Description Darxus 2010-08-22 18:03:30 PDT
User-Agent:       Mozilla/5.0 (X11; Linux x86_64; rv:2.0b4pre) Gecko/20100817 Minefield/4.0b4pre
Build Identifier: 

No blue square.

Reproducible: Always
Comment 1 Boris Zbarsky [:bz] (still a bit busy) 2010-08-23 00:00:24 PDT
Does cairo's behavior for LINE_CAP_SQUARE just not match the one the SVG spec calls for here?
Comment 2 Robert Longson 2010-08-23 17:24:25 PDT
No it doesn't. You have to invent an orientation for the square so it's a bit artificial.
Comment 3 Robert Longson 2010-11-01 10:07:28 PDT
*** Bug 608751 has been marked as a duplicate of this bug. ***
Comment 4 Jonathan Watt [:jwatt] 2010-12-15 22:26:21 PST
This causes us to fail:

http://dev.w3.org/SVG/profiles/1.1F2/test/harness/htmlObjectMiniApproved/painting-control-04-f.html
Comment 5 Mikko Virkkilä 2011-01-17 12:59:31 PST
I'm seeing this bug in 2d canvas with ctx.lineCap="square". Should I open a separate bug?
Comment 6 Robert Longson 2011-01-17 13:03:34 PST
Perhaps you should try seeing whether cairo is willing to change?
Comment 7 Jeff Muizelaar [:jrmuizel] 2011-01-17 13:20:37 PST
(In reply to comment #6)
> Perhaps you should try seeing whether cairo is willing to change?

I'd like to see a justification for cairo having square caps for zero-length paths. Adobe Acrobat doesn't have square caps for zero-length paths which is where cairo's current behavior comes from.
Comment 8 Boris Zbarsky [:bz] (still a bit busy) 2011-01-17 13:24:01 PST
Mikko, the canvas thing is almost certainly a duplicate of this bug.  As long as you're willing to track this bug and retest once it's fixed, probably no separate bug needed...
Comment 9 Jonathan Watt [:jwatt] 2011-01-17 18:26:19 PST
(In reply to comment #7)
> I'd like to see a justification for cairo having square caps for zero-length
> paths. Adobe Acrobat doesn't have square caps for zero-length paths which is
> where cairo's current behavior comes from.

For consistency with cairo's /round/ caps behavior?

Alternatively, to allow a major cairo consumer (us) to conform to the specs for one of the open web languages we support?
Comment 10 Jonathan Watt [:jwatt] 2011-01-19 17:34:50 PST
Created attachment 505290 [details] [diff] [review]
patch 1: preliminary patch to add explicit 'segStart'
Comment 11 Jonathan Watt [:jwatt] 2011-01-19 17:35:51 PST
Created attachment 505292 [details] [diff] [review]
patch 2: preliminary patch to stop hardcoding segment arg count
Comment 12 Jonathan Watt [:jwatt] 2011-01-19 17:37:58 PST
Created attachment 505294 [details] [diff] [review]
patch 3: work around lack of cairo support

I'm not particularly in love with these hacks, but I don't think it's too bad.

Jeff, can you comment on the "XXX tinyAdvance should probably depend on the CTM" comment in this patch?
Comment 13 Robert Longson 2011-01-19 20:58:11 PST
Comment on attachment 505292 [details] [diff] [review]
patch 2: preliminary patch to stop hardcoding segment arg count

Why not just increment i by SVGPathSegUtils::ArgCountForType(segType) at the end directly rather than having a separate argCount variable?
Comment 14 Jonathan Watt [:jwatt] 2011-01-19 22:51:51 PST
Because I need that argCount variable in the subsequent patch.
Comment 15 Jonathan Watt [:jwatt] 2011-01-19 23:42:00 PST
Err, or I was in an earlier incarnation of the third patch. That's no longer the case in the current version, so I'll change it as you suggest.
Comment 16 Robert Longson 2011-01-20 12:28:01 PST
Looking at bug 584623, it seems like linecap round has issues too. Should patch 3 do linecap round too?

The issue with linecap round is that cairo doesn't think there is a bounding box. This lead to the fix in bug Bug 510177. This fix should be reverted if we can get a proper solution to the linecap round issue.
Comment 17 Jonathan Watt [:jwatt] 2011-01-20 12:59:54 PST
I just commented on bug 584623 - it's WFM in beta 9. I can add a test for linecap round similar to the one included in patch 3 though.
Comment 18 Robert Longson 2011-01-22 03:36:36 PST
Comment on attachment 505294 [details] [diff] [review]
patch 3: work around lack of cairo support

>+static PRBool IsMoveto(PRUint16 aSegType)

Nit: I realise you just moved this but IsMoveTo would be nicer :-) If you look at the comments further on though you might
not need to move this at all.

>+  // XXX tinyAdvance should probably depend on the CTM

I doubt it, we made that mistake with radial gradients and then had to change the delta not to depend on the CTM.

>+  double tinyAdvance = 0.002;

static const double. Capitalised?

I'm surprised this is sufficient. Cairo as we use it is 24.8 and 1/256 = 0.00390625
which is the smallest difference cairo can detect isn't it.

>+#define MAYBE_APPROXIMATE_ZERO_LENGTH_SUBPATH_SQUARE_CAPS                     \
>+  do {                                                                        \
>+    if (capsAreSquare && !subpathHasLength && subpathContainsNonArc &&        \
>+        SVGPathSegUtils::IsValidType(prevSegType) &&                          \
>+        (!IsMoveto(prevSegType) ||                                            \
>+         segType == nsIDOMSVGPathSeg::PATHSEG_CLOSEPATH)) {                   \
>+      ApproximateZeroLengthSubpathSquareCaps(segStart, aCtx);                 \
>+    }                                                                         \
>+  } while(0)

What's the do ... while(0) for? I thought you only needed that for macros that might be defined as nothing sometimes.

Could this not be an inline function instead?

>+    if (!IsMoveto(segType) && segEnd != segStart ||
>+        SVGPathSegUtils::IsQuadraticType(segType) && segEnd != cp1 ||
>+        SVGPathSegUtils::IsCubicType(segType) && (segEnd != cp1 || segEnd != cp2)) {
>+      subpathHasLength = PR_TRUE;
>+    }

Since the code above is a big switch on segType, couldn't you set this in the appropriate switch options?

>+    if (!IsMoveto(segType) && !IsArc(segType)) {
>+      subpathContainsNonArc = PR_TRUE;

ditto.

>diff --git a/layout/svg/base/src/nsSVGPathGeometryFrame.cpp b/layout/svg/base/src/nsSVGPathGeometryFrame.cpp
>--- a/layout/svg/base/src/nsSVGPathGeometryFrame.cpp
>+++ b/layout/svg/base/src/nsSVGPathGeometryFrame.cpp
>@@ -446,16 +446,22 @@ nsSVGPathGeometryFrame::Render(nsSVGRend
>   default:
>     gfx->SetAntialiasMode(gfxContext::MODE_COVERAGE);
>     break;
>   }
> 
>   /* save/restore the state so we don't screw up the xform */
>   gfx->Save();
> 
>+  // Hack to let SVGPathData::ConstructPath know if we have square caps:

Omit "Hack to".

>+  const nsStyleSVG* style = GetStyleSVG();
>+  if (style->mStrokeLinecap == NS_STYLE_STROKE_LINECAP_SQUARE) {
>+    gfx->SetLineCap(gfxContext::LINE_CAP_SQUARE);
>+  }
>+

Why is this only in nsSVGPathGeometryFrame::Render? Doesn't it need to be in GeneratePath? What about GetFrameForPoint, UpdateCoveredRegion. You should add a mochitest for GetFrameForPoint or add to the existing GetFrameForPoint mochitest. You'd need some kind of dynamic reftest where you move shapes about to prove that UpdateCoveredRegion works and you're not leaving artifacts.
Comment 19 Jonathan Watt [:jwatt] 2011-01-23 15:36:26 PST
(In reply to comment #18)
> Comment on attachment 505294 [details] [diff] [review]
> >+static PRBool IsMoveto(PRUint16 aSegType)
> 
> Nit: I realise you just moved this but IsMoveTo would be nicer :-)

I deliberately didn't uppercase the "t" in "to" for consistency with the SVG spec.

> >+  // XXX tinyAdvance should probably depend on the CTM
> 
> I doubt it, we made that mistake with radial gradients and then had to change
> the delta not to depend on the CTM.

This case is pretty different, and we definitely depend on the CTM (zoom out and squares stop showing, zoom in and they still show). I think the patch coming up handles this correctly.

> >+  double tinyAdvance = 0.002;
> 
> static const double. Capitalised?

const, sure, but why static? Not sure about the capitalized either, since we don't tend to do that, do we(?) (and it is only a four line function).

> I'm surprised this is sufficient. Cairo as we use it is 24.8 and 1/256 =
> 0.00390625
> which is the smallest difference cairo can detect isn't it.

I'd guess it was working for me because 0.002 is just over half of 0.00390625, so it was being rounded up. This code has now changed though.

> What's the do ... while(0) for? I thought you only needed that for macros that
> might be defined as nothing sometimes.

So that I could add a semicolon after uses of the macro, which feels better.

> Could this not be an inline function instead?

It could, but it would take seven arguments so probably doesn't generate much less code and would be less readable.

> Since the code above is a big switch on segType, couldn't you set this in the
> appropriate switch options?

Yeah, okay. I was just trying not to clutter all the switch statements, but I guess that's what I should do.

> Omit "Hack to".

Hmm, I think it is a hack really, especially after it's moved to GeneratePath as you rightly point out it should be.
Comment 20 Jonathan Watt [:jwatt] 2011-01-23 17:51:16 PST
Created attachment 506285 [details] [diff] [review]
patch 3: work around lack of cairo support
Comment 21 Robert Longson 2011-01-24 02:06:30 PST
> > >+  double tinyAdvance = 0.002;
> > 
> > static const double. Capitalised?
> 
> const, sure, but why static? Not sure about the capitalized either, since we
> don't tend to do that, do we(?) (and it is only a four line function).

static data is only initialised once. You get better performance.

Everything else seems fine.
Comment 22 Jonathan Watt [:jwatt] 2011-01-24 06:48:33 PST
Is that really true for |double|? I'd assumed that the bit pattern for the double value would just have been hardcoded into the binary implementation of the function generated by the compiler and no double initializer code would be invoked for it.

Anyway, now that the variable is a gfxSize instead of a double it can't be static:

https://developer.mozilla.org/index.php?title=en/C%2b%2b_Portability_Guide#Don%27t_use_static_constructors

Probably the only rational from that section that applies is the "may confuse leak tools" one which may not apply in this case, but we should probably follow the letter of the law anyway?
Comment 23 Jonathan Watt [:jwatt] 2011-01-24 06:51:49 PST
Comment on attachment 506285 [details] [diff] [review]
patch 3: work around lack of cairo support

Requesting approval in advance of Jeff's feedback on the implementation of ApproximateZeroLengthSubpathSquareCaps. I think it's actually okay, but in the worst case we'll just fail to draw caps under certain transforms or something, and if there's a problem like that we can retrospectively fix it.
Comment 24 Jonathan Watt [:jwatt] 2011-01-24 06:52:56 PST
Note the approval request is for all three patches. Maybe plusing needs to be marked on all three explicitly?
Comment 25 Jonathan Watt [:jwatt] 2011-01-24 08:03:48 PST
Created attachment 506407 [details] [diff] [review]
patch 3: work around lack of cairo support

Of course we can't set subpathHasLength blindly or else that variable should really be named "lastSegHasLength", which is not what we want.
Comment 26 Jonathan Watt [:jwatt] 2011-01-24 12:42:35 PST
Modulo reftest failures due to random orange and the problem addressed in comment 25, the patches pass try server.
Comment 27 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-01-24 14:08:32 PST
Comment on attachment 506407 [details] [diff] [review]
patch 3: work around lack of cairo support

Let's take this after FF4.
Comment 28 Jonathan Watt [:jwatt] 2011-04-19 09:52:35 PDT
Created attachment 527021 [details] [diff] [review]
patch 3: work around lack of cairo support

unbitrot
Comment 30 Jonathan Watt [:jwatt] 2013-11-10 14:04:06 PST
longsonr: do you remember why arc was excluded in this patch? (That is, why subpathContainsNonArc exists.)

Note You need to log in before you can comment on or make changes to this bug.