Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Proposal: Rasterize with Gmagick/Imagick if available #98

Open
meyfa opened this issue Oct 11, 2020 · 4 comments
Open

Proposal: Rasterize with Gmagick/Imagick if available #98

meyfa opened this issue Oct 11, 2020 · 4 comments
Labels
feature New feature or request help wanted Extra attention is needed

Comments

@meyfa
Copy link
Owner

meyfa commented Oct 11, 2020

The current rasterizer implementation is severely lacking due to limitations with PHP's GD module. The only advantage of GD over Gmagick, and the reason this library exists, is that GD is much more widely available.

I propose to add a simple adapter for Gmagick/Imagick nonetheless. A check would need to be done, determining whether Gmagick/Imagick is available. If it is, use it. Otherwise, use the GD rasterizer.

Work on GD would still need to continue, but this way, if you are lucky enough to have one of the aforementioned packages installed on your system, you could profit immediately.

@meyfa meyfa added feature New feature or request help wanted Extra attention is needed labels Oct 11, 2020
@doublex
Copy link

doublex commented Oct 12, 2020

The great thing of 'php-svg' is that it has so little dependency.
Really great if you don't need to render super complex SVGs.

@meyfa
Copy link
Owner Author

meyfa commented Oct 12, 2020

The great thing of 'php-svg' is that it has so little dependency.

Yes, and there is no plan to change that.

@Niellles
Copy link
Contributor

I am thinking SVG::toRasterImage() should return a new (abstract) class SVG\Rasterization\BaseRasterImage, might also be an interface, which is just a wrapper around \GDImage|resource|Imagick....

Then SVG::toRasterImage() can use whatever rasterizer is available (e.g. GdRasterizer or ImagickRasterizer).
BaseRasterImage can then be used to output to an actual image (png, jpg, etc.).

I've made a start on this work, but there's plenty more to do:

  • Write tests for the work done so far..
  • Add additional rasterizers
  • Test additional rasterizer
  • Seperate GdRasterizer logic out of SVGNodes
  • Probably more...

@meyfa Can you kindly share your thoughts on this proposed solution/direction? I'd be happy to work some more on this. I've made a start in #216 (it's a mess.. I know..).

SVG::toRasterImage() just calls $rasterizer->rasterize($document), for the GdRasterizer. I just moved the original code from toRasterImage() to GdRasterizer::rasterize(), which seems the quickest way to implement this.


From a users perspective this woud look something like:

<?php
// Unchanged
$image = new SVG(100, 100);
$doc = $image->getDocument();

// circle with radius 20 and green border, center at (50, 50)
$doc->addChild(
    (new SVGCircle(50, 50, 20))
        ->setStyle('fill', 'none')
        ->setStyle('stroke', '#0F0')
        ->setStyle('stroke-width', '2px')
);

// Change start
// rasterize to a 200x200 image, i.e. the original SVG size scaled by 2.
// the background will be transparent by default.
$rasterImage = $image->toRasterImage(200, 200);

header('Content-Type: image/png');
//old: imagepng($rasterImage);
$rasterImage->toPng()

// Or write to a file:
//old: imagepng($rasterImage, 'some/path/to/file.png');
$rasterImage->toPng('some/path/to/file.png');

The above would pick the optimal rasterizer based on available extensions (probably based on some order of preference for rasterizers).

Alternatively which rasterizer should be used can be specified, like so:

<?php
// new SVG(100, 100);

$rasterImage = $image->toRasterImage(200, 200, GdRasterizer::class);

Alternatively a preferred rasterizer can be set like so:

<?php
// new SVG(100, 100);

$rasterImage = $image->toRasterImage(200, 200, GdRasterizer::class);
$rasterImage = $image->toRasterImage(200, 200); // Library determines which rasterizer to use.

// Set preferred rasterizer:
RasterizerFactory::setRasterizer(ImagickRasterizer::class);
$rasterImage = $image->toRasterImage(200, 200); // Uses `ImagickRasterizer`.

@meyfa
Copy link
Owner Author

meyfa commented Feb 14, 2024

@Niellles I think your proposal is reasonable. It's probably important to make the rasterizer configurable, e.g. with an argument, like you propose. Keep in mind that toRasterImage already has a third optional argument (background color) that users would be forced to provide if they want to provide a rasterizer as the final argument. However, for people on 8.0 and later, named arguments solves this issue elegantly.

Basically, we should support:

  • outputting images as an HTTP response or to a file
  • different raster image formats (PNG, JPEG, ...)
  • have a sane default choice
    • This probably means picking the "best" rasterizer that we're able to detect, as opposed to e.g. always using the least common denominator (which would lead to bad-looking outputs by default), and as opposed to enforcing Gmagick for everyone, which would not be compatible with many installs.
  • allow overriding that choice

Your design seems to satisfy all requirements 👍 We could also have a global configuration setting, but this can always be added later (to just override the default) and is also perhaps not the best design.

Small nitpick: The functions should probably be called toPNG etc. as opposed to toPng to be consistent with SVG in this library.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New feature or request help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

3 participants