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.