[api-minor] Use the `NodeCanvasFactory`/`NodeCMapReaderFactory` classes as defaults in Node.js environments (issue 11900) #12039

Snuffleupagus · 2020-06-29T12:27:01Z

This moves, and slightly simplifies, code that's currently residing in the unit-test utils into the actual library, such that it's bundled with GENERIC-builds and used in e.g. the API-code.

As an added bonus, this also brings out-of-the-box support for CMaps in e.g. the Node.js examples.

Fixes #11900

Edit: Slightly smaller diff with https://github.com/mozilla/pdf.js/pull/12039/files?w=1

Snuffleupagus · 2020-06-29T12:44:44Z

src/display/api.js

@@ -863,9 +874,9 @@ class PDFDocumentProxy {
 *                    just before viewport transform.
 * @property {Object} [imageLayer] - An object that has beginLayout,
 *                    endLayout and appendImage functions.
- * @property {Object} [canvasFactory] - The factory that will be used
+ * @property {Object} [canvasFactory] - The factory instance that will be used


I'm really not happy about the discrepancy between CMapReaderFactory (on getDocument) and canvasFactory, since in the first case the user needs to pass a class and in the second one a class instance.
I've tried to make this at least slightly clearer by updating the JSDoc comment here, but it still doesn't seem great. Unfortunately it might simply be (years) too late to try and "fix" this, and furthermore I'm also not entirely sure how to improve it (given how canvasFactory is being used). Oh, well...

Snuffleupagus · 2020-06-29T12:45:30Z

examples/node/pdf2png/pdf2png.js

@@ -50,14 +50,20 @@ NodeCanvasFactory.prototype = {



Maybe we should also export the various canvas/CMap-factories in the API as well, such that we could remove things like e.g.

pdf.js/examples/node/pdf2png/pdf2png.js

Lines 20 to 49 in 9993397

function NodeCanvasFactory() {}

NodeCanvasFactory.prototype = {

create: function NodeCanvasFactory_create(width, height) {

assert(width > 0 && height > 0, "Invalid canvas size");

var canvas = Canvas.createCanvas(width, height);

var context = canvas.getContext("2d");

return {

canvas: canvas,

context: context,

};

},

reset: function NodeCanvasFactory_reset(canvasAndContext, width, height) {

assert(canvasAndContext.canvas, "Canvas is not specified");

assert(width > 0 && height > 0, "Invalid canvas size");

canvasAndContext.canvas.width = width;

canvasAndContext.canvas.height = height;

},

destroy: function NodeCanvasFactory_destroy(canvasAndContext) {

assert(canvasAndContext.canvas, "Canvas is not specified");

// Zeroing the width and height cause Firefox to release graphics

// resources immediately, which can greatly reduce memory consumption.

canvasAndContext.canvas.width = 0;

canvasAndContext.canvas.height = 0;

canvasAndContext.canvas = null;

canvasAndContext.context = null;

},

};

However, I'm not sure if that's (generally) desirable and it also felt somewhat orthogonal to the rest of the patch...

pdfjsbot · 2020-06-29T13:52:40Z

From: Bot.io (Windows)

Received

Command cmd_test from @Snuffleupagus received. Current queue size: 0

Live output at: http://54.215.176.217:8877/9335ecda106e40a/output.txt

pdfjsbot · 2020-06-29T14:21:20Z

From: Bot.io (Windows)

Failed

Full output at http://54.215.176.217:8877/9335ecda106e40a/output.txt

Total script time: 28.66 mins

Font tests: Passed
Unit tests: Passed
Regression tests: FAILED

Image differences available at: http://54.215.176.217:8877/9335ecda106e40a/reftest-analyzer.html#web=eq.log

pdfjsbot · 2020-07-02T02:06:25Z

From: Bot.io (Linux m4)

Received

Command cmd_test from @Snuffleupagus received. Current queue size: 0

Live output at: http://54.67.70.0:8877/f56549a31f87230/output.txt

pdfjsbot · 2020-07-02T02:06:25Z

From: Bot.io (Windows)

Received

Command cmd_test from @Snuffleupagus received. Current queue size: 0

Live output at: http://54.215.176.217:8877/c02c37c1fd358b1/output.txt

pdfjsbot · 2020-07-02T02:32:52Z

From: Bot.io (Linux m4)

Failed

Full output at http://54.67.70.0:8877/f56549a31f87230/output.txt

Total script time: 26.44 mins

Font tests: Passed
Unit tests: Passed
Regression tests: FAILED

Image differences available at: http://54.67.70.0:8877/f56549a31f87230/reftest-analyzer.html#web=eq.log

pdfjsbot · 2020-07-02T02:35:41Z

From: Bot.io (Windows)

Failed

Full output at http://54.215.176.217:8877/c02c37c1fd358b1/output.txt

Total script time: 29.26 mins

Font tests: Passed
Unit tests: Passed
Regression tests: FAILED

Image differences available at: http://54.215.176.217:8877/c02c37c1fd358b1/reftest-analyzer.html#web=eq.log

…es as defaults in Node.js environments (issue 11900) This moves, and slightly simplifies, code that's currently residing in the unit-test utils into the actual library, such that it's bundled with `GENERIC`-builds and used in e.g. the API-code. As an added bonus, this also brings out-of-the-box support for CMaps in e.g. the Node.js examples.

Snuffleupagus · 2020-07-02T09:45:00Z

/botio test

pdfjsbot · 2020-07-02T09:45:02Z

From: Bot.io (Linux m4)

Received

Command cmd_test from @Snuffleupagus received. Current queue size: 0

Live output at: http://54.67.70.0:8877/e675f011ae31818/output.txt

pdfjsbot · 2020-07-02T09:45:02Z

From: Bot.io (Windows)

Received

Command cmd_test from @Snuffleupagus received. Current queue size: 0

Live output at: http://54.215.176.217:8877/10a595b8c4ad3a2/output.txt

pdfjsbot · 2020-07-02T10:11:20Z

From: Bot.io (Linux m4)

Failed

Full output at http://54.67.70.0:8877/e675f011ae31818/output.txt

Total script time: 26.30 mins

Font tests: Passed
Unit tests: Passed
Regression tests: FAILED

Image differences available at: http://54.67.70.0:8877/e675f011ae31818/reftest-analyzer.html#web=eq.log

pdfjsbot · 2020-07-02T10:15:54Z

From: Bot.io (Windows)

Failed

Full output at http://54.215.176.217:8877/10a595b8c4ad3a2/output.txt

Total script time: 30.86 mins

Font tests: Passed
Unit tests: FAILED
Regression tests: FAILED

Image differences available at: http://54.215.176.217:8877/10a595b8c4ad3a2/reftest-analyzer.html#web=eq.log

timvandermeij · 2020-07-03T22:43:06Z

Looks like a good step forwards to me. Thank you!

Snuffleupagus force-pushed the Node-utils branch from e7d13b1 to a26aed7 Compare June 29, 2020 12:42

Snuffleupagus commented Jun 29, 2020

View reviewed changes

Snuffleupagus force-pushed the Node-utils branch from a26aed7 to ae0580e Compare June 29, 2020 12:48

timvandermeij added the core label Jun 29, 2020

Snuffleupagus force-pushed the Node-utils branch 5 times, most recently from 9dbb6d2 to cb6e745 Compare July 2, 2020 01:43

Snuffleupagus force-pushed the Node-utils branch from cb6e745 to 4a7e298 Compare July 2, 2020 02:44

timvandermeij approved these changes Jul 3, 2020

View reviewed changes

timvandermeij merged commit 1f71755 into mozilla:master Jul 3, 2020

Snuffleupagus deleted the Node-utils branch July 3, 2020 22:49

Snuffleupagus mentioned this pull request Jan 8, 2021

Remove unncessary CanvasFactory/CMapReaderFactory/FileReaderFactory duplication in unit-tests #12837

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[api-minor] Use the `NodeCanvasFactory`/`NodeCMapReaderFactory` classes as defaults in Node.js environments (issue 11900) #12039

[api-minor] Use the `NodeCanvasFactory`/`NodeCMapReaderFactory` classes as defaults in Node.js environments (issue 11900) #12039

Snuffleupagus commented Jun 29, 2020 •

edited

Loading

Snuffleupagus Jun 29, 2020

Snuffleupagus Jun 29, 2020

pdfjsbot commented Jun 29, 2020

pdfjsbot commented Jun 29, 2020

pdfjsbot commented Jul 2, 2020

pdfjsbot commented Jul 2, 2020

pdfjsbot commented Jul 2, 2020

pdfjsbot commented Jul 2, 2020

Snuffleupagus commented Jul 2, 2020

pdfjsbot commented Jul 2, 2020

pdfjsbot commented Jul 2, 2020

pdfjsbot commented Jul 2, 2020

pdfjsbot commented Jul 2, 2020

timvandermeij commented Jul 3, 2020

	function NodeCanvasFactory() {}
	NodeCanvasFactory.prototype = {
	create: function NodeCanvasFactory_create(width, height) {
	assert(width > 0 && height > 0, "Invalid canvas size");
	var canvas = Canvas.createCanvas(width, height);
	var context = canvas.getContext("2d");
	return {
	canvas: canvas,
	context: context,
	};
	},

	reset: function NodeCanvasFactory_reset(canvasAndContext, width, height) {
	assert(canvasAndContext.canvas, "Canvas is not specified");
	assert(width > 0 && height > 0, "Invalid canvas size");
	canvasAndContext.canvas.width = width;
	canvasAndContext.canvas.height = height;
	},

	destroy: function NodeCanvasFactory_destroy(canvasAndContext) {
	assert(canvasAndContext.canvas, "Canvas is not specified");

	// Zeroing the width and height cause Firefox to release graphics
	// resources immediately, which can greatly reduce memory consumption.
	canvasAndContext.canvas.width = 0;
	canvasAndContext.canvas.height = 0;
	canvasAndContext.canvas = null;
	canvasAndContext.context = null;
	},
	};

[api-minor] Use the NodeCanvasFactory/NodeCMapReaderFactory classes as defaults in Node.js environments (issue 11900) #12039

[api-minor] Use the NodeCanvasFactory/NodeCMapReaderFactory classes as defaults in Node.js environments (issue 11900) #12039

Conversation

Snuffleupagus commented Jun 29, 2020 • edited Loading

Snuffleupagus Jun 29, 2020

Choose a reason for hiding this comment

Snuffleupagus Jun 29, 2020

Choose a reason for hiding this comment

pdfjsbot commented Jun 29, 2020

From: Bot.io (Windows)

Received

pdfjsbot commented Jun 29, 2020

From: Bot.io (Windows)

Failed

pdfjsbot commented Jul 2, 2020

From: Bot.io (Linux m4)

Received

pdfjsbot commented Jul 2, 2020

From: Bot.io (Windows)

Received

pdfjsbot commented Jul 2, 2020

From: Bot.io (Linux m4)

Failed

pdfjsbot commented Jul 2, 2020

From: Bot.io (Windows)

Failed

Snuffleupagus commented Jul 2, 2020

pdfjsbot commented Jul 2, 2020

From: Bot.io (Linux m4)

Received

pdfjsbot commented Jul 2, 2020

From: Bot.io (Windows)

Received

pdfjsbot commented Jul 2, 2020

From: Bot.io (Linux m4)

Failed

pdfjsbot commented Jul 2, 2020

From: Bot.io (Windows)

Failed

timvandermeij commented Jul 3, 2020

[api-minor] Use the `NodeCanvasFactory`/`NodeCMapReaderFactory` classes as defaults in Node.js environments (issue 11900) #12039

[api-minor] Use the `NodeCanvasFactory`/`NodeCMapReaderFactory` classes as defaults in Node.js environments (issue 11900) #12039

Snuffleupagus commented Jun 29, 2020 •

edited

Loading