1254904 - Improve issue described in D.8. Conforming SVG Viewers to resolve float CTM matrix calculation

Status: NEW

(Core :: SVG, defect)

Description

[ramin]

This record addresses the issue of CTM matrix calculated in single precision as described in "D.8. Conforming SVG Viewers" (following link)
https://svgwg.org/svg2-draft/conform.html#ConformingSVGViewers

Comment 1

[ramin]

Attachment: Improve issue described in D.8. Conforming SVG Viewers to resolve float CTM matrix calculation

Attached is a patch for improving float precision CTM issue as described in D.8. Conforming SVG Viewers. Please kindly let us know required steps for contribution of this patch.

Comment 2

[Brian Birtles (:birtles)]

CC'ing some people who know about SVG code and Nicholas Nethercote since I believe he has been looking into the floating-point precision we use in graphics code recently.[1]

From what I understand, this patch makes us use double precision for various matrix calculations (without requiring us to use doubles elsewhere in the graphics pipeline) in line with the following section from SVG 2 D.8,

"The viewer must use at least single-precision floating point for intermediate calculations on any numerical operations for conversion of coordinates. However, in order to prevent the rounding error on coordinate transformation, at least double-precision floating point computation must be used on CTM generation processing."[2]

I'm not sure who would be a suitable reviewer for this. Nicholas, how does this tie into your investigation into this area? Do you know if this is something we would be interested in and, if so, who might be able to review this?

Ramin, apart from finding a reviewer, we also need to check the performance characteristics of this patch. I'll fire off some performance tests tomorrow.

[1] https://groups.google.com/d/topic/mozilla.dev.platform/q6GNsgEFu30/discussion
[2] https://svgwg.org/svg2-draft/conform.html#ConformingSVGViewers

Comment 3

[Jonathan Watt [:jwatt]]

Broadly I'm in favor of having the SVG code switch to double precision since we have an increasing number of precision related bugs that Chrome and Edge do not have (presumably the reason that the new text requiring double precision has been added to the SVG 2 draft).

A few flyby comments from a *very* quick skim of the patch:

 * the patch should be broken up a bit; the gfx/2d parts should be
   a separate patch for sure

 * It would be better to convert mozilla::gfx::Matrix et. al.
   to being templates rather than writing duplicate code
   (obviously Matrix would be typedef'ed to something like
   MatrixTemplate<Float>)

 * I think the option to switch between the single and double
   precision code should be a compile-time switch to change
   the precision of the type used in the SVG code rather than
   a runtime switch (so the code doesn't have |if-else| forks
   scattered all over)

I'm not sure Bas is going to be a major fan of templating Matrix et. al., but I think it's necessary we start using double precision for SVG again, and it's the obvious way to avoid a bunch of code duplication.

Comment 4

[Brian Birtles (:birtles)]

(In reply to Brian Birtles (:birtles) from comment #2)
> Ramin, apart from finding a reviewer, we also need to check the performance
> characteristics of this patch. I'll fire off some performance tests tomorrow.

I submitted the job but it will take a few hours before results start to appear: https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=3bce3e20e028&newProject=try&newRevision=25b6a49c4bd2&framework=1&showOnlyImportant=0

Regarding next steps, provided Nicholas, Bas, and others don't have any objections, I think the next step would be to incorporate the feedback Jonathan provided in comment 3 and then request review from Jonathan. Some parts will likely require review from someone in the graphics team but Jonathan can redirect the review request as appropriate.

Comment 5

[Nicholas Nethercote [inactive]]

I'm no expert here, Bas and jwatt know much more :)  All I know that our old gfx types use double-precision, and our new gfx types largely use single-precision, and converting old to new is sometimes problematic.

Comment 6

[ramin]

(In reply to Brian Birtles (:birtles) from comment #4)
> (In reply to Brian Birtles (:birtles) from comment #2)
> > Ramin, apart from finding a reviewer, we also need to check the performance
> > characteristics of this patch. I'll fire off some performance tests tomorrow.
> 
> I submitted the job but it will take a few hours before results start to
> appear:
> https://treeherder.mozilla.org/perf.html#/
> compare?originalProject=try&originalRevision=3bce3e20e028&newProject=try&newR
> evision=25b6a49c4bd2&framework=1&showOnlyImportant=0
Thank you for running the test. But I have no clue how to see the results.
for windows7-32 result is highlighted (2.02%), does that mean performance is degraded?

> Regarding next steps, provided Nicholas, Bas, and others don't have any
> objections, I think the next step would be to incorporate the feedback
> Jonathan provided in comment 3 and then request review from Jonathan. Some
> parts will likely require review from someone in the graphics team but
> Jonathan can redirect the review request as appropriate.
We are trying to apply Jonathan's comments to our new patch and will release it by 3/18.

Comment 7

[Brian Birtles (:birtles)]

(In reply to ramin from comment #6)
> (In reply to Brian Birtles (:birtles) from comment #4)
> > I submitted the job but it will take a few hours before results start to
> > appear:
> > https://treeherder.mozilla.org/perf.html#/
> > compare?originalProject=try&originalRevision=3bce3e20e028&newProject=try&newR
> > evision=25b6a49c4bd2&framework=1&showOnlyImportant=0
> Thank you for running the test. But I have no clue how to see the results.
> for windows7-32 result is highlighted (2.02%), does that mean performance is
> degraded?

I think this result is not significant enough to worry about. The performance results generally appear to be fine.

Comment 8

[Jonathan Watt [:jwatt]]

Bas, see comment 3, particularly the second bullet point. If you have different ideas it would be good to know it before ramin gets too far on with the patch.

Comment 9

[Bas Schouten (:bas.schouten)]

(In reply to Jonathan Watt [:jwatt] from comment #8)
> Bas, see comment 3, particularly the second bullet point. If you have
> different ideas it would be good to know it before ramin gets too far on
> with the patch.

You're right, if we need double precision we should template Gfx::Matrix, not a huge fan, but also not a huge dislike, as you said, it prevents a lot of code duplication.

Comment 10

[ramin]

Attachment: SVG_DoublePrecisionCTM_base-20160322.zip

attached is the new patch modified as follows
 * the patch is broken up and gfx/2d is separated
 * mozilla::gfx::Matrix et. al. converted to being templates
 * the option to switch between the single and double is a compile-time switch

Comment 11

[ramin]

Please kindly take a look in "SVG_DoublePrecisionCTM_base-20160322.zip" and let us know your opinion about
new modified patch. Thank you in advance.

Comment 12

[Robert Longson [:longsonr]]

please add the patch as separate uncompressed files rather than a zip.

Comment 13

[ramin]

Attachment: SVG_DoublePrecisionCTM_base-20160322.patch

SVG_DoublePrecisionCTM_base-20160322.patch is attached

Comment 14

[ramin]

Attachment: SVG_DoublePrecisionCTM_base-20160322_gfx-2d.patch

SVG_DoublePrecisionCTM_base-20160322_gfx-2d.patch is attached

Comment 16

[Thomas Wisniewski]

It seems that CanvasRenderingContext2D.cpp has changed recently, and its ObjectToMatrix() method will need a trivial change to use doubles for its elts array instead of Floats.

Also note that I plan to use this for bug 937494 so it can use double math internally for for matrix interpolation, to better-match that spec.

Comment 17

[ramin]

Hi! originally our intention was to contribute this patch for 32-bit version, since 64 bit seems to work fine with CTM matrix. if there are further issues to be solved regarding attached patch, please kindly let us know. thank you in advance.

Comment 18

[Robert Longson [:longsonr]]

Duplicating so much of the code as you have done will be a maintenance problem. Somehow you need to implement this patch without copying lots of code, either via changing some typedefs only or via C++ templates (or both).

Comment 19

[Jonathan Watt [:jwatt]]

(In reply to ramin from comment #10)
> Created attachment 8734190 [details]
> SVG_DoublePrecisionCTM_base-20160322.zip
> 
> attached is the new patch modified as follows
>  * the patch is broken up and gfx/2d is separated

Thanks!

>  * mozilla::gfx::Matrix et. al. converted to being templates

The changes here are not quite what I meant. I'll post more info as a comment on the patch.

>  * the option to switch between the single and double is a compile-time
> switch

Thanks!

Comment 20

[Jonathan Watt [:jwatt]]

Comment on attachment 8734251 [details] [diff] [review]
SVG_DoublePrecisionCTM_base-20160322_gfx-2d.patch

Review of attachment 8734251 [details] [diff] [review]:
-----------------------------------------------------------------

Thank you for working on this.

::: gfx/2d/Matrix.cpp
@@ +121,4 @@
>  Matrix&
>  Matrix::NudgeToIntegers()
>  {
> +  NudgeToInteger(&(Float&)_11);

(For what it's worth, you can't cast a double& to float&, pass it by reference to a function expecting a float& and expect the operation to result in the valid changes to the bits of a double. Instead you would have to overload NudgeToInteger to provide a double& version of that function.)

::: gfx/2d/Matrix.h
@@ +38,5 @@
>      , _31(a31), _32(a32)
>    {}
> +  double _11, _12;
> +  double _21, _22;
> +  double _31, _32;

We shouldn't change Matrix's members to double. We don't want to pay the performance cost of converting from float to double in all code paths that use Matrix. We only want to pay that cost in the SVG code and other code that specifically opts in to using that (and that's assuming the cost doesn't turn out to be too high).

Back in comment 3 when I asked for Matrix to be turned into a template class I literally meant you should make it:

  template <class T>
  class Matrix
  {
    //...

    union {
      struct {
        T _11, _12;
        T _21, _22;
        T _31, _32;
      };
      T components[6];
    };

    //...
  };

Could you do that please? And to make the code changes clearer in the patches that you put up for review it would be best to separate out the Moz2D patch to have separate name change patch(es). Perhaps something like:

 1. A patch to rename Matrix in Matrix.h, Matrix.cpp, gfx2DGlue.h etc.
    (all the code that should be templatized) to MatrixT (that name may
    change, but we don't need to worry about that just now). To keep the
    code compiling you'll also need to add a typedef in gfx/2d/Matrix.h
    to typedef 'Matrix' to 'MatrixT' of course.

 2. Make MatrixT a template class, and change the typedef of Matrix to be
    MatrixT<Float>. Changing MatrixT to a template class will also include
    changing helpers in gfx2DGlue.h to be templatized for example.

 3. Add your typedef to define the double precision Matrix type, as in:

      typedef MatrixT<double> MatrixDP;

    and add any helpers to convert between Matrix and MatrixDP. The tricky
    bit here is that the compile time switch should be able to switch the
    SVG code between using Matrix and MatrixDP under the hood without
    having to add ifdefs all over the SVG code.

Comment 21

[Jonathan Watt [:jwatt]]

Comment on attachment 8734250 [details] [diff] [review]
SVG_DoublePrecisionCTM_base-20160322.patch

This will need to change based on the changes to the other patch, so clearing the review request for now.

Comment 22

[ramin]

Hi Jonathan
thank you for your comments. 
we are working on the patch to fulfill above mentioned requirements in Comment 3 and 20.

Comment 23

[Jonathan Watt [:jwatt]]

Let me know if you need any help.