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

SVG rendering seems incorrect. #301

Closed
izaakschroeder opened this issue Nov 16, 2015 · 17 comments
Closed

SVG rendering seems incorrect. #301

izaakschroeder opened this issue Nov 16, 2015 · 17 comments
Labels
Milestone

Comments

@izaakschroeder
Copy link

I have attempted to write a webpack loader for the glorious sharp. It is available here: https://github.com/izaakschroeder/sharp-loader. It's supposed to expose most of sharp to webpack to do things like create 1x,2x,3x versions of assets or webp for chrome and so on. The bug I'm dealing with right now is just about SVG though.

Sometimes original assets are present in SVG and PNG is needed for browsers that don't support SVG (cough old satanic versions of IE).

In the spirit of that, using the aforementioned loader to do something like:

require('./visa.svg?height=100&format=png');

Should generate a sensible image (and indeed vips pngsave visa.svg visa.png produces something sane). Sadly, sharp appears to produce unintelligible gibberish when it comes to SVGs 😢

test

^ Generated via vips command-line.

visa d556617b

^ Generated via sharp.

visa f5c80e77

^ Generated via sharp with flatten() applied first.

Not quite sure what's going on here, but some general direction to troubleshooting this would be super appreciated 😄

vips-8.1.1-Thu Nov 12 18:00:15 PST 2015
[email protected]

The results come from running this example: https://github.com/izaakschroeder/sharp-loader/tree/master/example and should be reproducible by something like:

git clone [email protected]:izaakschroeder/sharp-loader.git
cd sharp-loader
npm install
cd example
npm install webpack
./node_modules/.bin/webpack
@lovell
Copy link
Owner

lovell commented Nov 16, 2015

Hi Izaak,

The mostly-white output with the blue bar at the top looks like (un)premultiply gone wrong - I suspect *magick is reporting the SVG as having an alpha channel but then not supplying the right values.

The green and red output looks like a colourspace conversion gone wrong, perhaps as a cumulative effect of an incorrect alpha channel.

What does sharp(svgFile).metadata().then(console); output? Is https://github.com/izaakschroeder/sharp-loader/blob/master/example/img/visa.svg the original SVG file?

There are known (but probably unrelated) density/resolution problems when using SVG - see #110.

@papandreou
Copy link
Contributor

sharp-loader looks similar to https://github.com/papandreou/express-processimage wrt. the features and the supported syntax. I'm glad that others are picking up the same idea as having such an image processing pipeline is very powerful.

For express-processimage we gave up on imagemagick/graphicsmagick's lacklustre SVG support and used inkscape instead via https://github.com/papandreou/node-inkscape

@lovell lovell added the triage label Nov 16, 2015
@izaakschroeder
Copy link
Author

@lovell thanks for the fast reply! Here is the metadata output:

{ format: 'magick',
  width: 228,
  height: 152,
  space: 'rgb16',
  channels: 4,
  hasProfile: false,
  hasAlpha: true }

And yes, the example SVG you linked is the one I used to create the above raster images.

@izaakschroeder
Copy link
Author

It seems weird that *magic would be incorrectly reporting the value, given the vips command line tool appears to do the right thing at first glance (though I'm not very familiar with it so...).

@izaakschroeder
Copy link
Author

@papandreou cool! Trying to keep the number of external program/library dependencies down... people have a hard enough time installing native libs as it is it seems... But if this doesn't work out then I'll certainly explore that!

@lovell
Copy link
Owner

lovell commented Nov 16, 2015

I think this problem relates to sharp's use of vips_premultiply with 16-bit input images that contain an alpha channel - I'll try the suggestion here.

@lovell lovell added bug and removed triage labels Nov 16, 2015
@izaakschroeder
Copy link
Author

Great 😄 Let me know if you need me to test anything!

@lovell
Copy link
Owner

lovell commented Nov 16, 2015

Thanks @izaakschroeder. This problem affects sharp when used with libvips 8.1.0+ (as sharp uses the new vips_premultiply operation instead of its own version).

There is a test for SVG to PNG but unluckily it's one of the few remaining scenarios that is not fully automated and I hadn't spotted the broken output. I'll automate the test before fixing the bug!

@izaakschroeder
Copy link
Author

Woohoo thanks @lovell!

@lovell lovell added this to the v0.12.0 milestone Nov 17, 2015
lovell added a commit that referenced this issue Nov 21, 2015
Improves SVG support as *magick serves these as 16-bit

Add automated tests for SVG and 16-bit+alpha PNG inputs
@lovell
Copy link
Owner

lovell commented Nov 21, 2015

Commit 05dd191 on the look branch should fix this problem for any 16-bit input image with an alpha channel.

npm install lovell/sharp#look

@lovell
Copy link
Owner

lovell commented Nov 22, 2015

The look branch has now been merged to the master branch, so please re-test this via:

npm install lovell/sharp

@izaakschroeder
Copy link
Author

Testing... and I get:

../src/utilities.cc:43:35: error: use of undeclared identifier 'round'
    New<Integer>(static_cast<int>(round(vips_tracked_get_mem() / 1048576)))
                                  ^
../src/utilities.cc:46:35: error: use of undeclared identifier 'round'
    New<Integer>(static_cast<int>(round(vips_tracked_get_mem_highwater() / 1048576)))
                                  ^
../src/utilities.cc:49:35: error: use of undeclared identifier 'round'
    New<Integer>(static_cast<int>(round(vips_cache_get_max_mem() / 1048576)))

😄

@izaakschroeder
Copy link
Author

Maybe it needs:

#include <cmath>

^ @lovell

lovell added a commit that referenced this issue Nov 22, 2015
@lovell
Copy link
Owner

lovell commented Nov 22, 2015

Yes, sorry, I forgot to add that to keep clang happy - please try now.

@izaakschroeder
Copy link
Author

Success! The new version produces:

visa b45b2e73

@izaakschroeder
Copy link
Author

Does this mean 0.12 is shipping soon™? 😄

Thanks again for all the fantastic work on this! 🎉

@lovell
Copy link
Owner

lovell commented Nov 22, 2015

Thanks Izaak for confirming this solves the problem and for reporting it in the first place.

v0.12.0 should be published within the next few days.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants