Closed Bug 249263 Opened 20 years ago Closed 15 years ago

Make W3C specify "liveness" of SVGTransform.matrix, and then deCOMify nsSVGMatrix

Categories

(Core :: SVG, defect)

Product:
Core
Component:
SVG
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED

People

(Reporter: benjamin, Assigned: jwatt)

References

(
URL
)

Details

(Whiteboard: SVGWG) 

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
Chris: Can the WG help here?
(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.

> - 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

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.
(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.

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
(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.

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
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.
> 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.
(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;

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.
Status: NEW → ASSIGNED
Whiteboard: SVGWG
Adding Robin to the CC list since he seems to be the only member of the SVG WG
actually pushing for an Errata.
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.
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
I integrated the proposed errata into the SVG 1.1 Full 2nd Ed. draft this week at the SVG WG F2F.

Closing FIXED.
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Is there a testcase in our code and/or in the spec?
You need to log in before you can comment on or make changes to this bug.