Last Comment Bug 249263 - Make W3C specify "liveness" of SVGTransform.matrix, and then deCOMify nsSVGMatrix
: Make W3C specify "liveness" of SVGTransform.matrix, and then deCOMify nsSVGMa...
Status: RESOLVED FIXED
SVGWG
:
Product: Core
Classification: Components
Component: SVG (show other bugs)
: Trunk
: All All
: -- normal (vote)
: ---
Assigned To: Jonathan Watt [:jwatt] (back in October - email directly if necessary)
: Hixie (not reading bugmail)
:
Mentors:
http://lists.w3.org/Archives/Public/w...
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2004-06-30 11:04 PDT by Benjamin Smedberg [:bsmedberg]
Modified: 2014-04-25 15:17 PDT (History)
11 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments

Description Benjamin Smedberg [:bsmedberg] 2004-06-30 11:04:08 PDT
This comes from a discussion with jwatt regarding the "live"ness of
SVGTransform.matrix in the SVG DOM. He asked that question and received no good
answers: "look at batik" was the most helpful ;)

http://lists.w3.org/Archives/Public/www-svg/2004Jun/thread.html#16

> Batik doesn't keep a reference to the SVGMatrix passed in to setMatrix,
> it just uses the SVGMatrix to modify the SVGTransform's internal
> representation of the transformation.  And when the matrix attribute is 
> referred to a new SVGMatrix object is returned.  But modifying this new
> SVGMatrix object still does affect the SVGTransform.
> 
> This sort of thing happens a lot in the Batik code.  In this case, when
> you refer to the matrix attribute of the SVGTransform an instance of an
> anonymous class inheriting from AbstractSVGMatrix is returned, whose
> setA..setF methods affect the SVGTransform.

Which seems like a weird solution (and certainly not specified anywhere).

1) We need the SVG WG to provide an official answer about live-ness.

When looking at the code involved, I also found that nsSVGTransform is using the
nsIDOMSVGMatrix methods of nsSVGMatrix to perform transformations. This is
hideously slow and codesize-heavy for math-intensive code like this. It should
be defining a pseudo-IID for nsSVGMatrix and using the concrete class members
directly.

2) deCOM nsSVGMatrix and use the concrete class instead of nsIDOMSVGMatrix pointers
Comment 1 Benjamin Smedberg [:bsmedberg] 2004-06-30 11:17:20 PDT
http://www.w3.org/TR/SVG11/coords.html#InterfaceSVGTransform is the "specification".
Comment 2 Hixie (not reading bugmail) 2004-06-30 12:26:30 PDT
Chris: Can the WG help here?
Comment 3 Alex Fritze 2004-06-30 13:15:16 PDT
(In reply to comment #0)
> 1) We need the SVG WG to provide an official answer about live-ness.

I prefer our current implementation to Batik's. 'Live' matrices that can be
shared amongst several transforms come in very handy for things like
implementing dynamic canvases where part of the objects should be
transformation-invariant.
 
> When looking at the code involved, I also found that nsSVGTransform is using the
> nsIDOMSVGMatrix methods of nsSVGMatrix to perform transformations. This is
> hideously slow and codesize-heavy for math-intensive code like this. 

I'm somewhat sceptical at haphazardly optimizing the SVG code at and introducing
native interfaces at this stage in development and given the resources we have.
The transform code is not an inner loop and there are reasons for using
nsIDOMSVGMatrix methods:
- We don't want to have to maintain native interfaces that duplicate DOM
interface functionality. 
- SVG DOM interfaces are well documented.
- Using SVG DOM interfaces throughout C++ implementation means we need less JS
DOM testing.

It should
> be defining a pseudo-IID for nsSVGMatrix and using the concrete class members
> directly.
> 2) deCOM nsSVGMatrix and use the concrete class instead of nsIDOMSVGMatrix
pointers

We have decom bug#246474.
This is from an email to jwatt:

Using pseudo-IIDs and nsRefPtr<> instead of pointers to interfaces sounds like a
good idea, because it will remove much QI'ing in the SVG source plus it will
remove the need for full-fledged internal interfaces like nsISVGValue or
nsISVGSVGElement. This would actually make the code less verbose and easier to
maintain (plus it will be a performance benefit - but I don't care about that at
this stage). And as you rightly say, it will remove the matrix identity problem
and similar problems (i'm sure we have tons of cases like this - whereever a DOM
method accepts an interface parameter).

We should go about this carefully and systematically though and not expose any
non-virtual methods without careful consideration. I.e. I don't think that, in
the case of nsSVGMatrix, we should make mA, mB, etc. be part of the public
signature of the class. Neither I think should we have non-virtual getters or
setter methods, but instead use the virtual SVG DOM interfaces as much as
possible for the reason I listed in my previous email. Similarly, I think making
nsSVGMatrix a friend of nsSVGTransform should be considered separately.
We should really have 2 stages:

(1)
A decomification stage where we systematically change the whole SVG code to use
pseudo-iids and 'normal' ref-counted pointers instead of COMPtr's.  Also
internal interfaces such as nsISVGValue and nsISVGSVGElement should be merged
into the corresponding implementing objects' class signatures.
The decomification stage will require that we move class definition which now
sit in '.cpp' files into '.h' files and carefully vet the public & private parts
of the class signatures (I think at the moment the class definitions are often
wholly public or wholly private because it doesn't really matter if only they
are not exposed to the outside world).

(2)
A performance tuning stage, where we look at de-virtualising some of the SVG
interfaces for internal use (e.g. offer a non-virtual SetA() method for
nsISVGMatrix.
Comment 4 Brendan Eich [:brendan] 2004-06-30 13:33:42 PDT
> - Using SVG DOM interfaces throughout C++ implementation means we need less JS
> DOM testing.

How does that help?  First, the JS<=>XPCOM bridge used by the DOM is unit- and
compound-tested by almost all the scriptable code in Mozilla, especially the
HTML and XUL DOM code.  Second, you want to support scriptable SVG DOM, so you
can only put off this testing, or hope coverage differs for SVG users enough to
spare you some latent bugs biting.

> maintain (plus it will be a performance benefit - but I don't care about that
> at this stage)

The bulk of our deCOM experience has been driven by profiling, and it has been
costly, requiring hand-crafting.  This is not something you should repeat.  The
old saw (Kernighan and Pike?) about "build it right, then make it fast" does not
help here.  Building something right, but adding lots of XPCOM, or just lots of
virtual methods and dynamic casting, can leave you with a system that is hard to
change later to use non-virtual methods and static types.

Sorry to do a drive-by comment, but these parts didn't ring true.

/be
Comment 5 Robert O'Callahan (:roc) (email my personal email if necessary) 2004-06-30 13:43:42 PDT
As I understand it, Alex is talking about a situation where IDL interfaces
already exist and must continue to exist, and defining an additional C++-only
interface would simply be additional work. In that case I agree with Alex that
it makes sense to defer the work until performance analysis shows it's worth doing.

Most of the current deCOM work has been focused on internal Gecko interfaces
with no standard IDL requirement.
Comment 6 Alex Fritze 2004-06-30 13:58:10 PDT
(In reply to comment #4)
> > - Using SVG DOM interfaces throughout C++ implementation means we need less JS
> > DOM testing.
> 
> How does that help?  First, the JS<=>XPCOM bridge used by the DOM is unit- and
> compound-tested by almost all the scriptable code in Mozilla, especially the
> HTML and XUL DOM code.  Second, you want to support scriptable SVG DOM, so you
> can only put off this testing, or hope coverage differs for SVG users enough to
> spare you some latent bugs biting.

What I meant was that if we use the SVG DOM internally in our C++ implementation
 then if things work as expected in C++ we can be pretty certain that the same
things will work from JS as well. 

> > maintain (plus it will be a performance benefit - but I don't care about that
> > at this stage)
> 
> The bulk of our deCOM experience has been driven by profiling, and it has been
> costly, requiring hand-crafting.  This is not something you should repeat.  The
> old saw (Kernighan and Pike?) about "build it right, then make it fast" does not
> help here.  Building something right, but adding lots of XPCOM, or just lots of
> virtual methods and dynamic casting, can leave you with a system that is hard to
> change later to use non-virtual methods and static types.

As Robert says, in our case the dom interfaces were there first.
Comment 7 Brendan Eich [:brendan] 2004-06-30 14:07:12 PDT
I would say it differently: IDL plus debugged typelibs and xpconnect mean you
can be sure C++ and JS work equally well with IDL-defined interfaces.  Period,
no need for special testing in any language -- this goes for PyXPCOM too,
assuming it is in working order.

IOW, bugs in the low level type mappings can bite any code, depending.  If you
use some rarely-used IDL feature, maybe you'll suffer.  But generally there's no
need to count an extra testing cost against one or another IDL-bound programming
language.

Good to hear you can use the DOM for a lot of stuff -- it sounded from jwatts'
mail that XPCOM was causing some identity or equivalence problems, though.

/be
Comment 8 Alex Fritze 2004-06-30 14:57:15 PDT
(In reply to comment #7)
> I would say it differently: IDL plus debugged typelibs and xpconnect mean you
> can be sure C++ and JS work equally well with IDL-defined interfaces.  Period,
> no need for special testing in any language -- this goes for PyXPCOM too,
> assuming it is in working order.

I'm not doubting the soundness of the IDL/typelibs/XPConnect machinery but the
soundness of the SVG DOM implementation.

Example:
nsSVGTransform contains a member 'matrix' which needs to be accessed by other
C++ classes and can also be accessed through the dom property
nsIDOMSVGMatrix::matrix.

Consider the implementation
  /* readonly attribute nsIDOMSVGMatrix matrix; */
  NS_IMETHODIMP nsSVGTransform::GetMatrix(nsIDOMSVGMatrix * *aMatrix)
  {
    *aMatrix = mMatrix;
    return NS_OK;
  }

This SVG DOM method is missing an NS_ADDREF(*aMatrix), so will sooner or later
lead to problems if called.
If we provide an alternative native access method to the matrix for use by C++
callers - something like
 nsSVGMatrix *nsSVGTransform::GetMatrix() { return mMatrix; }
- then the bug in the DOM method is likely to go unnoticed for a long time
(until someone, probably without a debugbuild/stacktrace, tries to execute a JS
script that contains 'transform.matrix').
If on the other hand we use the DOM method for accessing the matrix from C++ as
well (instead of a native non-virtual method), then this bug will surface much
earlier during development.
Comment 9 Brendan Eich [:brendan] 2004-06-30 16:25:16 PDT
Ok, sorry I misread your point -- it sounded more like JS glue concern than bad
ref-counting in unused-by-C++ XPCOM methods.  I agree on not wanting non-XPCOM
C++ methods if they're not necessary.  I still have trepidation about too much
XPCOM usage being hard to step away from, but if you're using it only where the
SVG DOM requires it, I'm happy.

Is there any pressure from the OMG IDL used by the spec on XPIDL, which does not
implement all OMG IDL types?  Mail me or file/cite a spin-off bug if so.

/be
Comment 10 Dean Jackson 2004-07-19 08:47:22 PDT
SVGMatrix objects are not live.
setMatrix() copies the SVGMatrix into the internal version in the transform.
There would be some optimisations available to scripts if they wanted to use the same matrix on 
multiple objects, but in most cases it is quicker (and more efficient) to group the objects and transform 
the group. However, it would be nice to have a live matrix, I agree.

True, none of this is described in the 800+ pages of the SVG specification. Complain to the authors! 
Er... me. The SVG DOM was created in an especially drunken moment.
Comment 11 Dean Jackson 2004-07-19 09:58:55 PDT
> True, none of this is described in the 800+ pages of the SVG specification. Complain to the authors! 
> Er... me. The SVG DOM was created in an especially drunken moment.

It was pointed out to me that I left off the smiley. I guess I assumed that most people would see the 
self-deprecating Australian humour. This was actually a reference to the time some Mozilla guy, can't 
remember who, called us mad.

Anyway, I'd be interested to hear if you have good reasons for wanting the matrix to be live, other than 
convenience in some situations.
Comment 12 Alex Fritze 2004-07-19 13:52:52 PDT
(In reply to comment #11)
> Anyway, I'd be interested to hear if you have good reasons for wanting the
matrix to be live, other than 
> convenience in some situations.

Ok, I'll have a go :-)

Firstly, it is useful in situations where parts of a graphic are supposed to
transform differently to others and where the z-ordering is such that the
elements can't be grouped together. E.g. in CrocodileMaths we have shapes
(triangles, squares, etc) where the user can drag the vertices. We'd like the
shapes to transform, but the vertex handles to keep their size and (circular)
shapes. With live matrices, one shared matrix is all that needs to be updated
when the transformation changes.

Secondly, it seems strange that points, pathsegments, lengths and transforms
should all be 'live' but matrices shouldn't. 

Thirdly, if matrices are not live then, to manipulate a transform's a,b,c,d,e,f
would require something like:
var m = element.transform.matrix;
m.a += x;
m.b += y;
element.transform.setMatrix(m);

If matrices are live then this condenses to:
element.transform.matrix.a += x;
element.transform.matrix.b += y;
Comment 13 Jonathan Watt [:jwatt] (back in October - email directly if necessary) 2004-07-19 16:28:04 PDT
Adding Dean to the CC list so he sees future comments. BTW Dean, in case you
don't know already, Alex replied to your last comment. 
Comment 14 Jonathan Watt [:jwatt] (back in October - email directly if necessary) 2005-02-28 09:25:04 PST
Adding Robin to the CC list since he seems to be the only member of the SVG WG
actually pushing for an Errata.
Comment 15 Jonathan Watt [:jwatt] (back in October - email directly if necessary) 2005-02-28 10:57:07 PST
For the record, I don't have access to SVG WG meetings, and I didn't mean to
mislead with my previous comment. Robin is the only SVG WG member that I happen
to have seen publicly express support for an errata. Since making the above
comment I've been assured that other WG members would also like to see an errata
published. Anyway, if Robin is going to push for an errata I'd like to make sure
he's aware of this bug.
Comment 16 Cameron McCormack (:heycam) 2009-02-04 20:27:36 PST
An erratum was issue stating that the matrix passed in to SVGTransform.setMatrix() is not adopted, and that its values are just copied over:

http://dev.w3.org/SVG/profiles/1.1F2/errata/errata.xml#setmatrix_createsvgtransformfrommatrix

And one to state that the SVGTransform.matrix is live in as much as fetching a, b, c, etc. would result in different values as the SVGTransform itself changes:

http://www.w3.org/2003/01/REC-SVG11-20030114-errata#mention-live-values

I filed a new issue with the WG for whether modifying the SVGMatrix returned by SVGTransform.matrix does anything:

http://www.w3.org/Graphics/SVG/WG/track/issues/2210
Comment 17 Jonathan Watt [:jwatt] (back in October - email directly if necessary) 2009-06-11 13:13:57 PDT
I integrated the proposed errata into the SVG 1.1 Full 2nd Ed. draft this week at the SVG WG F2F.

Closing FIXED.
Comment 18 Benjamin Smedberg [:bsmedberg] 2009-06-11 13:30:24 PDT
Is there a testcase in our code and/or in the spec?

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