934182 - Decide on, document and test the Moz2D behavior for degenerate paths

Status: NEW

(Core :: Graphics, defect)

Description

[Jonathan Watt [:jwatt]]

While working on the SVG conversion to Moz2D I encountering several issues related to undocumented and inconsistent behavior for nonsensical paths. Decisions should be made about what the Moz2D behavior should be, that behavior should then be documented in the relevant header files, and it should be tested in the unit tests.

The initial response to the examples below might be "just don't do that", but because SVG allows content authors to specify path commands for a path we either need Moz2D behavior that "just does something acceptable" or, if the behavior that's decided upon is not acceptable, we'll have to add code to validate the paths to make sure no funky permutations that will make Moz2D unhappy will make it through to Moz2D. It seems like the right thing to do might be to define sensible and consistent behavior for Moz2D though.

Some examples:

  RefPtr<PathBuilder> pathBuilder = drawTarget->CreatePathBuilder();
  pathBuilder->MoveTo(Point(10, 10));
  RefPtr<Path> path = pathBuilder->Finish();
  Rect rect = path->GetBounds();

For at least some of the Moz2D backends, the GetBounds() call will return a Rect that can contain non-finite values.

The same thing happens if you do multiple consecutive MoveTo()'s with the same point.

Same thing again for combinations of multiple Close() and MoveTo()'s with the same point.

There are also issues with zero length paths, such as:

  RefPtr<PathBuilder> pathBuilder = drawTarget->CreatePathBuilder();
  pathBuilder->MoveTo(Point(10, 10));
  pathBuilder->LineTo(Point(10, 10));
  RefPtr<Path> path = pathBuilder->Finish();
  Rect rect = path->GetStrokedBounds(strokeOptions);

In this case the Rect returned by GetStrokedBounds() can also contain non-finite values, depending on the line cap style.

Comment 1

[Andreas Gal :gal]

How about a helper that wraps path and sanitizes? Doing this per backend is error prone.

Comment 2

[Matt Woodrow (:mattwoodrow)]

I think we should ensure that bounds rects are always finite. The cairo backend does this already, so we should just make the others match.

Comment 3

[Bas Schouten (:bas.schouten)]

(In reply to Matt Woodrow (:mattwoodrow) from comment #2)
> I think we should ensure that bounds rects are always finite. The cairo
> backend does this already, so we should just make the others match.

I agree with Matt. Sanitizing all incoming paths has a fairly high overhead since true 'complete' sanitization can require analysis (and thus storage) of an arbitrary number of operations into the past.

Fwiw, for D2D we've also already made it guarantee the bounds are always Finite.

Comment 4

[Bas Schouten (:bas.schouten)]

I'll add unittests for this and make sure all platforms support it.

Comment 5

[Jonathan Watt [:jwatt]]

I added some data here: https://etherpad.mozilla.org/min-line-length