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

Add support for WebP decoding #1280

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

LinusU
Copy link
Collaborator

@LinusU LinusU commented Oct 12, 2018

This is currently a bit of a work in progress, see #1258

Thanks for contributing!

  • Have you updated CHANGELOG.md?

Fixes #1258

@zbjornson
Copy link
Collaborator

zbjornson commented Oct 14, 2018

Neat!

Sorta related... I'm playing with switching node-canvas from Cairo to Skia, and Skia ships with WebP (and all the other common format) support, and it now has a WebAssembly build, andddd they're working on an npm module for that build. (Although... it looks like they might disable libwebp in the WebAssembly build by default, not sure why. Maybe the same issue you hit.)

Regardless of whether we'd use the WebAssembly or C++ build*, we'd get WebP support automatically (SkImage automatically handles all the formats from a unified interface, which would remove a ton of code from this lib). I'm hoping Skia would also fix a few other issues we have, especially around text.

*Skia and some of its dependencies like libjpeg-turbo are written to use modern x86 extensions and GPU acceleration when available. Curious to benchmark the two builds.


BTW when this merges, should it go in a Canvas 3 branch? It seems a little early to drop Node 6.x support. It's not EOL until April 2019.

@zbjornson zbjornson mentioned this pull request Oct 14, 2018
@piranna
Copy link
Contributor

piranna commented Oct 14, 2018

WebAssembly Skia would be useful to don't have the C++ compilation problems, isn't it? How native backends (FbDev, X11, Cocoa...) could be integrated?

@zbjornson
Copy link
Collaborator

zbjornson commented Oct 14, 2018

WebAssembly Skia would be useful to don't have the C++ compilation problems, isn't it?

That is the only benefit. But again, performance is a high priority at least for me.

How native backends (FbDev, X11, Cocoa...) could be integrated?

I still don't really think these are in-scope for this module, but if they are added, they would have to stay in the cairo version. Skia supports all of the pixel formats that Cairo does (and more!), so my example of using node-canvas a framebuffer device with 565 format would still work.

Note that Skia supports a PDF surface.

@piranna
Copy link
Contributor

piranna commented Oct 15, 2018

That is the only benefit. But again, performance is a high-priority at least for me.

So do you propose to have WebAssembly Skia and Cairo, or Skia in WebAssembly and native? We would need benchmarks for each case.

so my example of using node-canvas a framebuffer device with 565 format would still work.

It would work for static images, for dynamic ones performance would be a bit low due to data copying... Advantage of native backends is of writting directly to FbDev or whatever. Maybe Skia has support for them, would need to investigate... question is if this support is available in the WebAssembly version (that would be great :-D ).

@chearon
Copy link
Collaborator

chearon commented Oct 15, 2018

I'm hoping Skia would also fix a few other issues we have, especially around text.

They do have a drawText API that accepts a string of characters, but me trying to read the source = it draws a bunch of 0-id glyphs. So if we did have to give it glyphs, it could be extracted from Pango or just make the full switch to HarfBuzz.

@zbjornson
Copy link
Collaborator

It draws a bunch of 0-id glyphs. So if we did have to give it glyphs,

https://fiddle.skia.org/c/@measure_text_bounds
Don't need to feed it glyphs.

@chearon
Copy link
Collaborator

chearon commented Oct 15, 2018

What's it using for shaping though? Is it a toy API that just maps chars to glyphs? It would be nice if it used HarfBuzz so node-canvas looks similar everywhere. And does it use a proper cascade list for fallback fonts? How do we add manual fonts to that list? I couldn't find answers to any of that very easily.

@@ -1,26 +1,89 @@
'use strict';
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@LinusU until the memory issue is resolved, do you wanna pull out the nice refactoring you did in this file into a separate PR? Your version is way more readable 😍

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

Successfully merging this pull request may close these issues.

4 participants