Use librsvg for PNG rendering #16
Conversation
* Remove batik support * Use librsvg * Breaking change: Requires additional package librsvg2-dev see https://www.npmjs.com/package/librsvg
|
@wikimedia/math this is a really huge performance benefit, and no temp files for png images anymore |
|
cc @pkra |
* Get the dimensions for the PNG image
* Make PNG images slightly bigger. * Scaling factor 2 was chosen as default
|
Thanks for the cc. I'm guessing that means you want to submit this upstream at mathjax-node as well? The first question that comes to mind: could you abstract the converter? It'd be great if people could switch out their personal favorite, in particular svg2png is a fan favorite. |
|
But it would be good to make a PR on mathjax-node to discuss this further. Oh and also: really great to see this! 💯 |
|
@pkra I would love to make that this modular. I was thinking about that, but I'm not not sure what's the best way to do this. Maybe @d00rman has an idea. |
Maybe discuss upstream? |
| @@ -1,5 +1,13 @@ | |||
| -sudo: false | |||
| language: node_js | |||
| node_js: | |||
| - "4.2.4" | |||
| - "4.2" | |||
There was a problem hiding this comment.
We want to keep 4.2.4 because that's the version used in WMF prod.
There was a problem hiding this comment.
ok. But we should keep that consistent. I think in other repos we have just 4.2
There was a problem hiding this comment.
We'll soon switch to node 4.3, so you might as well put that instead :)
There was a problem hiding this comment.
You could specify just "4", which pulls in the latest 4.x automatically.
|
In principle, I welcome this change, but I'm afraid WMF isn't ready for it. We have our own pkgs for |
|
@d00rman did you file a bug for librsvg-dev? I think the absence of PNG export is the most important blocker for further developments in the math environment. We might consider taking a special path for math since we do not feed arbitrary svg images to librsvg. |
|
Give me a day to finish up some work for an upstream modularization of the png generation. |
|
@physikerwelt I've just spoken to @Stype about it, and he says that the newest Jessie librsvg contain all of our security fixes, so we are actually good to go on that front. |
|
@physikerwelt module the minor Travis config nits, LGTM, so when these are corrected I'd be happy to merge. |
| @@ -1,6 +1,6 @@ | |||
| { | |||
| "name": "mathoid-mathjax-node", | |||
| "version": "0.5.0", | |||
| "version": "0.5.1", | |||
There was a problem hiding this comment.
Actually, since this a breaking change, v0.6.0 would be more appropriate.
| }); | ||
| return synch; // This keeps the queue from continuing until the readFile() is complete | ||
| } | ||
| var Readable = require('stream').Readable; |
There was a problem hiding this comment.
Any reason why this is not in the file's header?
There was a problem hiding this comment.
oh no not really.Should I move it?
| var svgFile = result.svgfile; delete result.svgfile; | ||
| var width = result.width; delete result.width; | ||
| var height = result.height; delete result.height; | ||
| if (data.png) { |
There was a problem hiding this comment.
If data.png is false-y, no return is specified. Shouldn't synch be retuned anyway?
There was a problem hiding this comment.
I did not change that...
Replace delete with "= undefined".
Use librsvg for PNG rendering
librsvg2-dev see https://www.npmjs.com/package/librsvg