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

Canvas 2.0 🚀 #743

Closed
LinusU opened this issue Mar 25, 2016 · 35 comments
Closed

Canvas 2.0 🚀 #743

LinusU opened this issue Mar 25, 2016 · 35 comments

Comments

@LinusU
Copy link
Collaborator

LinusU commented Mar 25, 2016

I would like to start bringing up the discussion on the next major bump. This is a quick list on the top of my head on what I personally would like to get in.

Would love to get some comments from both maintainers and users of the module 🙌

@jakeg
Copy link
Contributor

jakeg commented Mar 25, 2016

Sounds great! Especially pango.

Does anyone with necessary know-how know if getting svg import (not export!) #494 . Would be possible? Happy to sponsor someone's time to work on this. In fact generally is it possible to donate some money for your and other developers' time?

@zbjornson
Copy link
Collaborator

Awesome! I'd add:

Re: remove sync streams, any use in adding a sync API? (e.g. let pngBuffer = canvas.toPng();)

@LinusU
Copy link
Collaborator Author

LinusU commented Mar 26, 2016

That's right, I forgot about that one 👍

I think we already have sync api? canvas.toBuffer('png')

@jakeg
Copy link
Contributor

jakeg commented Apr 15, 2016

Would love to see CMYK JPG import support too #425, or at least an exception thrown if trying to add one to a canvas.

@piranna
Copy link
Contributor

piranna commented Apr 30, 2016

I would like to have static build so there's no dependencies on libraries installed on the system, this will fix a lot of install issues. Also I would like to have configurable backend support, so node-canvas could be used to draw on the Linux framebuffer and other on-screen outputs. I have support for both of them on my pull-request.

@chearon
Copy link
Collaborator

chearon commented May 5, 2016

I would like to have static build so there's no dependencies on libraries installed on the system, this will fix a lot of install issues

node-pre-gyp seems to be the new way do do that. Canvas could move to that and there wouldn't have to be compilation on the client. I think people would still have to have Cairo and stuff though and of course Windows is still a nightmare.

Downsides are that it adds another step for publishing, and it relies on S3 which has to be paid for

@piranna
Copy link
Contributor

piranna commented May 5, 2016

node-pre-gyp is a partial fix, my pull-request fix it for all platforms
(including OSX and Windows) by allowing to use statically linked libraries,
no external dependencies on Cairo, and also makes prebuilds more portable.
On the other hand, prebuild-install is a simpler and more flexible
alternative to node-pre-gyp, and by default uses GitHub releases for
distribution, so there's no publishing problem here, too.
El 5/5/2016 6:37 PM, "Caleb Hearon" notifications@github.com escribió:

I would like to have static build so there's no dependencies on libraries
installed on the system, this will fix a lot of install issues

node-pre-gyp https://github.com/mapbox/node-pre-gyp seems to be the new
way do do that. Canvas could move to that and there wouldn't have to be
compilation on the client. I think people would still have to have Cairo
and stuff though and of course Windows is still a nightmare.

Downsides are that it adds another step for publishing, and it relies on
S3 which has to be paid for


You are receiving this because you commented.
Reply to this email directly or view it on GitHub
#743 (comment)

@piranna
Copy link
Contributor

piranna commented May 14, 2016

Another thing I would like to see is a "compliant mode", a way to have a Canvas object that only implements the W3C Canvas API without the node-canvas extras, so it would be easy to stick to it to be sure that a project will be compatible with both environments.

@piranna
Copy link
Contributor

piranna commented Sep 11, 2016

I have finally managed to update my pull-request with support for statically linked build and configurable graphic backend to nan2, with direct screen support for FBDev and X11 but it could be possible easily to add other backends. Could be able someone to review and merge it or give me some comments?

@zbjornson
Copy link
Collaborator

@LinusU what do you think of starting a 2.0 staging branch so these PRs can start landing? :)

At the moment I think that might just be pango (#715). I need to update #740, which is straightforward.

@adamhooper
Copy link
Contributor

I'm of the opinion toBuffer() should give raw pixels, and toPNG() / toJPEG() should be the sync APIs. Or -- better yet -- encoding could move to other libraries. See #819

I started using node-canvas yesterday, so I'm an expert ;).

@guileen
Copy link

guileen commented Oct 7, 2016

Any update on 2.0?

@piranna
Copy link
Contributor

piranna commented Oct 7, 2016

I'm splitting my pull-request on several smaller ones hopping they could be easily reviewed and merged. The first one I've done so far is for static building, that would also fix the build dependencies problems both on WIndows and OSX but also on Linux.

@chearon
Copy link
Collaborator

chearon commented Oct 22, 2016

@LinusU is there anything we can do to help with this release? I've been keeping #715 conflict-free, and I know it's a ton to review but I will make myself available to answer questions, I'd really love to see #740 land soon too

@LinusU
Copy link
Collaborator Author

LinusU commented Oct 23, 2016

screen shot 2016-10-23 at 13 48 11

master is now for version 2.x 🙌 🚀

Let's start the merging!

@piranna
Copy link
Contributor

piranna commented Oct 23, 2016

Do you accept pull-requests gocused gor the 2.0 release?

@LinusU
Copy link
Collaborator Author

LinusU commented Oct 23, 2016

Yes, that's the plan 👍

@piranna
Copy link
Contributor

piranna commented Oct 23, 2016

I did a cleaned one for the static build, I'll do the same for the backends. I'll first do it for the current available ones and later move for the native graphic systems, it will be easier to merge and develop. What do you think?

@piranna
Copy link
Contributor

piranna commented Oct 26, 2016

I've splitted and isolated the support for backends on the pull-request #829 and also re-enabled the support for PDF and SVG backends so tests can pass, now I hope it could be easier reviewed and merged. I know it's already a big one, but hope it's easier to review, and also I'm open for comments. Once that we could get it merged in master, I'll also port the support for FBDev and X11 backends as independent pull-requests, and in the future we could also look to add support for GDI (Windows) and Cocoa (OSX).

@mathjoy
Copy link

mathjoy commented Dec 6, 2016

did node-conva support node js 6.0?

@LinusU
Copy link
Collaborator Author

LinusU commented Dec 6, 2016

Both 1.x and 2.x supports Node.js 6.0

@chearon
Copy link
Collaborator

chearon commented Feb 14, 2017

In an attempt to jump start the 2.0 discussion... #740 is ready and I think we still need a PR to remove the sync streams? I'd be happy to do that or anything else needed to get this done. A beta of what is currently on master would be nice too!

@LinusU
Copy link
Collaborator Author

LinusU commented Feb 15, 2017

Nice! I'd totally missed that one. I'll merge it, remove sync streams and then publish the first beta in the next few days :)

@piranna
Copy link
Contributor

piranna commented Feb 15, 2017

@LinusU, could you be able to review the static build pull-request too? I would like to have it merged before moving forward the backends support and start adding graphic device backends like fbdev or X11. I think these would be good additions for the 2.0 version and more people are asking for them too...

@LinusU
Copy link
Collaborator Author

LinusU commented Feb 15, 2017

The static build is backwards compatible though? So that could land regardless of 2.x or not :)

Yeah, I've really been meaning to take a proper look at it, I'm sorry that it taking so long, there is only so much time in a day

@piranna
Copy link
Contributor

piranna commented Feb 15, 2017

The static build is backwards compatible though? So that could land regardless of 2.x or not :)

Yes, it is fully backwards compatible, the code of the "dynamic" version is untouched :-) Static version will be build automatically if Cairo library is not found (maybe should the other ones and/or the heards check too) or explicitly when using the --has_cairo=false flag. The only thing that maybe would need to be done (if any) is to configure the tests to also generate a static version and pass them.

Yeah, I've really been meaning to take a proper look at it, I'm sorry that it taking so long, there is only so much time in a day

Yeah, seems everybody is too busy lately with work and real-life dutties, me too... :-/

@adamhooper
Copy link
Contributor

Heretical question here, but ... why not remove streams altogether? They incur more CPU overhead than the encoding itself. They don't save a palpable amount of memory, since the encoded output is an order of magnitude smaller than the original canvas buffer. They're race-prone unless you dup the buffer -- in which case they cost a ghastly amount of memory, plus the time it takes to malloc.

Encoding a 640x480 PNG takes 3ms (Skylake i5-6600K), and encoding JPEG is far faster.

On a typical web server, I feel it's smarter to encode first, then stream the encoded Buffer. I'd expect an order of magnitude of memory savings (since fewer raw buffers would hang around) and throughput (since the CPU wouldn't waste cycles on async-function overhead). A 3ms stall is a small price to pay for such a performance increase.

What's the real-world use case in which streaming does more good than harm?

@zbjornson
Copy link
Collaborator

I tend to agree (at least that the non-streaming API must be solid), but here's one example where streaming was essentially required: #778

@jakeg
Copy link
Contributor

jakeg commented Aug 22, 2017

Yeah I was thinking of my issue #778 - I never got any further with that, but fortunately didn't have any other customers (yet) with such big files.

@adamhooper
Copy link
Contributor

So as I understand it, streaming good when the output data is >1GB, because v8 doesn't allow values >1GB.

For the sake of brainstorming, here's an idea: change the new Canvas(..., ..., 'pdf') ctor arguments to allow an optional filename as fourth argument -- and pass that to Cairo. That would provide a workaround for generating 1GB PDFs. (It's not perfect -- users shouldn't read from the file until writes are finished -- but that probably won't prevent any users from doing their jobs.) Then all the streaming APIs could be destroyed.

The reason I'm advocating for destroying streaming APIs: when I first started using node-canvas, I thought I "should" use streams: the README only allows some options (PNG palettes, JPEG encoding) in the async/streaming APIs. The sync APIs are spartan, which suggested to me -- a newbie -- that I shouldn't want them.

My impression was completely wrong: for most people, Buffers are the way to go. Streaming has its place, but only after the image is encoded and the Buffer is freed. Separate libraries like stream-buffers are a good fit for that.

It seems to me like the streaming APIs add an unneeded learning curve.

@zbjornson
Copy link
Collaborator

zbjornson commented Aug 23, 2017

Personally I'd rather keep the streaming APIs so we don't break compatibility and since streams are a reasonable solution for big canvases. Specifying an output file wouldn't (nicely) help in cases where e.g. you want to pipe a big image to an HTTP response.

+1 for making your performance points clear in the documentation. On my to-do list is making a significant update to README.md and I'll include these points if no one else does it before I do.

The sync APIs are spartan

It definitely makes sense to ensure the streaming and non-streaming APIs support the same options. It looks like all of the interfaces are consistent, as well as they can be, but again the docs need to be improved.

  • Both pngStream and toBuffer accept compression_level and filter_value arguments when writing PNGs.
  • pngStream supports palette and backgroundIndex properties when encoding indexed PNGs. There is no way to make indexed PNGs via toBuffer currently though.
  • jpegStream supports bufsize, quality and progressive properties. There's also no way to make JPEGs via toBuffer though. @adamhooper I remember previously you were an advocate of using separate node.js modules for doing all encoding, but perhaps toBuffer("JPEG") should be supported so there's a built-in non-streaming API for this.

(Edit: or we add toPNG and toJPEG as you proposed earlier in this thread. Nice, explicit functions.)

Streaming has its place, but only after the image is encoded and the Buffer is freed.

BTW, I'm curious what makes you say this. ("Just" performance?) libpng's encoder is at its core a streaming encoder; it works nicely together with node's streams.

@adamhooper
Copy link
Contributor

Keen to answer :)

@adamhooper I remember previously you were an advocate of using separate node.js modules for doing all encoding

Yes, I did that successfully: first with my custom-made lightnpng, and then with turbo-jpeg when I found out how slow PNG conversion is and will always be. (The task was to generate 200,000 images in <2hrs, and PNG conversion can only be as fast as zlib compression.)

Raw access is definitely fantastic. At the same time, HTML5 <canvas> has toDataURL(), so if you want to support the HTML5 API you have no choice but to bundle JPEG+PNG libraries.

Streaming has its place, but only after the image is encoded and the Buffer is freed.

Sure, I'll ramble on the topic :).

Yes, PNG and JPEG formats both support streaming. But that stream is far more useful for the client (when decoding) than it is for the server (when encoding).

The reason: on the server, if you're generating lots of images concurrently, you'll be limited by CPU and memory. "Great," you'll say, "streaming means I get effectively unlimited memory, and streaming means one task won't hog the CPU to the detriment of other tasks."

But that's misleading in this specific case. Streaming actually costs memory in node-canvas: as long as the stream is open, node-canvas keeps an uncompressed buffer with the entire image contents -- that's an order of magnitude more data held in memory than the stream will transmit in total. For instance, streaming a 1024x1024 PNG costs 4MB, even if the stream only produces 200kb of PNG data. Under load, this is out-of-memory territory. A programmer's instinct should be to limit the number of simultaneously-open memory buffers ... which is hard to do with async APIs. The easiest way to limit memory usage is to guarantee a maximum of one open memory buffer at a time -- that is, generate the entire image in a single synchronous function.

As for CPU: jpeg-turbo and (especially) libpng hog CPU, yes ... but for a large image, we're talking milliseconds or tens of milliseconds. If a sync API solves out-of-memory errors at the cost of 5ms stutters when generating images, that seems like a win to me. Furthermore, without the overhead of async function calls, image generation is much faster. (I'm sorry to say I've misplaced my benchmarks that led me to this conclusion.)

Furthermore, streaming has to happen after encoding, otherwise there will be races:

ctx.fillRect(0, 0, 10, 10)
canvas.toDataURL((err, result) => console.log(err, result))
ctx.clearRect(0, 0, canvas.width, canvas.height)

... if toDataURL() is async, then the value sent to console.log() is undefined: we don't know whether clearRect() happens before or after (or halfway-through) encoding. (One can solve this race by duplicating the buffer, but it's absurd to duplicate a huge buffer to support streaming in the name of ... memory savings.)

It's great that libpng and turbo-jpeg support streaming: that's really useful when transcribing an uncompressed file into a compressed bytestream using a teensy amount of memory. But transcribing uncompressed memory into a compressed bytestream is a completely different problem. Streams could only be useful for an expert asynchronous programmer generating enormous images concurrently on a RAM-heavy computer.

So implementation-wise, if there is to be an async API, I think the best implementation is to first generate a compressed Buffer (synchronously), and then turn that compressed Buffer into a stream. That should lead to better performance, lower RAM usage, and no races.

@zbjornson
Copy link
Collaborator

how slow PNG conversion is and will always be

Side note: I'm happy to talk about fast PNG encoding if you want, my email is in my profile. I've tested a bunch of combinations of PNG encoders, DEFLATE implementations, compression levels and other tweaks and wound up with a method that is 5-10x faster than libpng with similar resulting file sizes (using Intel's igzip and FFmpeg's adler32 impl). Second to that (since igzip is a pain to compile), libdeflate with the custom adler32 was still a significant speedup over zlib. An issue that came up was that encoding time can't be evaluated separately from the file size, as the network or disk I/O times for the difference in file sizes got to be on-par with the difference in encoding times. /ramble.


Agree with everything you say 🍻. Two points though:

  1. Streaming is still useful for SVG and PDF canvases, since those formats can be streamed out of Cairo itself without creating a buffer ahead of time.

streaming has to happen after encoding, otherwise there will be races ... One can solve this race by duplicating the buffer, but it's absurd to duplicate a huge buffer to support streaming in the name of ... memory savings.

  1. This is actually a bug in node-canvas, good point. What Chrome and FF appear to do for canvas.toBlob(callback) (see https://chromium.googlesource.com/chromium/src/+/67dee0e0028d56abb569d569f8d1db0249ff5da1/third_party/WebKit/Source/core/html/canvas/CanvasAsyncBlobCreator.cpp and https://dxr.mozilla.org/mozilla-central/source/dom/canvas/CanvasRenderingContextHelper.cpp#109, https://dxr.mozilla.org/mozilla-central/source/dom/canvas/CanvasRenderingContext2D.cpp#2083) is similar to what you say in the second half about duplicating the buffer -- get the rasterized image data synchronously, and then do the PNG line encoding asynchronously. Since node-canvas already retains the rasterized buffer for the duration of the streaming operation but just generates it "late" (after a turn of the event loop), I think this could be fixed without worsening the memory overhead, but haven't looked very closely yet.

(BTW, canvas.toBlob(callback) to me is the closest equivalent to canvas.toBuffer(callback), as far as the goal of providing a similar API to HTMLCanvas.)

(Also, there's a distinction between async and streaming; canvas.toBlob/canvas.toBuffer are async but not streaming. Streaming seems to be the main issue, but I guess your argument is that anything that prolongs the lifetime of the buffer is potentially harmful.)


I still lean toward (a) improving the docs to encourage people to use toBuffer (or toPNG/toJPEG/toGIF if we add them) and explain the performance reasons, (b) keep the streaming APIs to avoid breaking stuff 🙊, (c) ensuring the toBuffer/to<Format> APIs are full-featured and as fast as possible...

@zbjornson
Copy link
Collaborator

Since it's been two years... 😨

Once #1111 and #1110 are dealt with (polishing up the prebuild issues), is it time to just publish 2.0.0 and defer other major changes to 3.0.0? A lot of new and open issues are (a) users who don't realize the default README.md is for 2.x, (b) issues that are fixed in 2.x but users don't want to use an alpha. 3.0.0 doesn't have to be far down the road and the pending semver major issues are all gonna be hefty to start/finish/land.

@piranna
Copy link
Contributor

piranna commented Mar 14, 2018

I don't find it good to have 19 open pull-requests, so I'm not sure if they should be merged previously to release a major 2.0.0, or do a stable release and start accepting them just after that.

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

No branches or pull requests

8 participants