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

Crude PDF stream implementation #781

Merged
merged 1 commit into from
Sep 11, 2016
Merged

Conversation

kkoopa
Copy link
Contributor

@kkoopa kkoopa commented Jun 9, 2016

A Minimial Working PDFStream.

Fixes #778

However, I would not advise merging it as such. It would be better to clean up all the streams code, avoiding the need to keep everything in memory and eliminating unnecessary copying of data. It would also be nice to finally have the asynchronous streams. This would entail a major rewrite of the library and its API, which I do not have time to do.

@zbjornson
Copy link
Collaborator

Nice.

"It would also be nice to finally have the asynchronous streams"

Could you explain how the current streams are synchronous please? This came up when I was working on revising streams in #740, but I'm not sure in what way a stream can be synchronous.

@kkoopa
Copy link
Contributor Author

kkoopa commented Jun 10, 2016

Because they block the event loop by running on the same thread as V8. For example, this keeps going (with intermittent calls to the JS stream callback) until all data in the Cairo surface has been processed.

To make them async, they should run in a separate thread. Nan::AsyncProgressWorker could be shoehorned in to do that.

However, what I would like to see is a way of operating directly on a stream, without storing an entire Cairo surface in memory. For example, create a Canvas bound to a stream which could then be redirected to a file (or a Buffer, or a socket, or anything else). Operations on the Canvas would stream out in chunks, never using more memory than the size of a chunk. A canvas created in this fashion could not conceivably support toBuffer or deletion, but would be highly useful for non-interactive work.

stream.on('end', function () {
done();
});
stream.on('error', function () {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Missing err in the functions parameter list...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, yes. I must have missed it. However, the code always returns CAIRO_STATUS_SUCCESS, since nothing can fail in that sense, so there will actually never be an error event. But I guess it's better to leave it in case someone changes the code in the future.

@LinusU
Copy link
Collaborator

LinusU commented Jun 18, 2016

Sorry for the delay on merging this, just one tiny nit and then we're good to go 👍

@jakeg
Copy link
Contributor

jakeg commented Sep 11, 2016

Any progress on this? Was the "tiny nit" fixed?

@LinusU
Copy link
Collaborator

LinusU commented Sep 11, 2016

Sorry for the delay, merging now 👍

@LinusU LinusU merged commit 77872ca into Automattic:master Sep 11, 2016
@kkoopa kkoopa deleted the pdfstream branch February 14, 2017 14:41
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

Successfully merging this pull request may close these issues.

4 participants