From bd52e93fca3a90794d03c2ee6f020432d2ad2828 Mon Sep 17 00:00:00 2001 From: Lovell Fuller Date: Sun, 12 Jan 2020 19:59:39 +0000 Subject: [PATCH] Deprecate limitInputPixels and sequentialRead, move to input options --- docs/api-constructor.md | 5 ++ docs/api-input.md | 34 -------------- docs/changelog.md | 2 + lib/constructor.js | 8 ++-- lib/input.js | 57 ++++++++++++++--------- src/common.cc | 15 ++++-- src/common.h | 6 ++- src/metadata.cc | 2 +- src/pipeline.cc | 33 +++++-------- src/pipeline.h | 3 -- src/stats.cc | 3 +- src/stats.h | 1 - test/bench/perf.js | 3 +- test/unit/io.js | 100 ++++++++++++++++++++++++++++++++++++++-- 14 files changed, 176 insertions(+), 96 deletions(-) diff --git a/docs/api-constructor.md b/docs/api-constructor.md index 2451a879..bbc61fea 100644 --- a/docs/api-constructor.md +++ b/docs/api-constructor.md @@ -11,6 +11,11 @@ - `options` **[Object][3]?** if present, is an Object with optional attributes. - `options.failOnError` **[Boolean][4]** by default halt processing and raise an error when loading invalid images. Set this flag to `false` if you'd rather apply a "best effort" to decode images, even if the data is corrupt or invalid. (optional, default `true`) + - `options.limitInputPixels` **([Number][5] \| [Boolean][4])** Do not process input images where the number of pixels + (width x height) exceeds this limit. Assumes image dimensions contained in the input metadata can be trusted. + An integral Number of pixels, zero or false to remove limit, true to use default limit of 268402689 (0x3FFF x 0x3FFF). (optional, default `268402689`) + - `options.sequentialRead` **[Boolean][4]** Set this to `true` to use sequential rather than random access where possible. + This can reduce memory usage and might improve performance on some systems. (optional, default `false`) - `options.density` **[Number][5]** number representing the DPI for vector images. (optional, default `72`) - `options.pages` **[Number][5]** number of pages to extract for multi-page input (GIF, TIFF, PDF), use -1 for all pages. (optional, default `1`) - `options.page` **[Number][5]** page number to start extracting from for multi-page input (GIF, TIFF, PDF), zero based. (optional, default `0`) diff --git a/docs/api-input.md b/docs/api-input.md index 483671ec..e87756ae 100644 --- a/docs/api-input.md +++ b/docs/api-input.md @@ -88,34 +88,6 @@ image Returns **[Promise][5]<[Object][6]>** -## limitInputPixels - -Do not process input images where the number of pixels (width x height) exceeds this limit. -Assumes image dimensions contained in the input metadata can be trusted. -The default limit is 268402689 (0x3FFF x 0x3FFF) pixels. - -### Parameters - -- `limit` **([Number][7] \| [Boolean][8])** an integral Number of pixels, zero or false to remove limit, true to use default limit. - - -- Throws **[Error][9]** Invalid limit - -Returns **Sharp** - -## sequentialRead - -An advanced setting that switches the libvips access method to `VIPS_ACCESS_SEQUENTIAL`. -This will reduce memory usage and can improve performance on some systems. - -The default behaviour _before_ function call is `false`, meaning the libvips access method is not sequential. - -### Parameters - -- `sequentialRead` **[Boolean][8]** (optional, default `true`) - -Returns **Sharp** - [1]: https://github.com/libvips/libvips/blob/master/libvips/iofuncs/enumtypes.c#L636 [2]: https://github.com/libvips/libvips/blob/master/libvips/iofuncs/enumtypes.c#L672 @@ -127,9 +99,3 @@ Returns **Sharp** [5]: https://developer.mozilla.org/docs/Web/JavaScript/Reference/Global_Objects/Promise [6]: https://developer.mozilla.org/docs/Web/JavaScript/Reference/Global_Objects/Object - -[7]: https://developer.mozilla.org/docs/Web/JavaScript/Reference/Global_Objects/Number - -[8]: https://developer.mozilla.org/docs/Web/JavaScript/Reference/Global_Objects/Boolean - -[9]: https://developer.mozilla.org/docs/Web/JavaScript/Reference/Global_Objects/Error diff --git a/docs/changelog.md b/docs/changelog.md index d12a7253..c4a4c0cf 100644 --- a/docs/changelog.md +++ b/docs/changelog.md @@ -12,6 +12,8 @@ Requires libvips v8.9.0. * Drop support for undefined input where options also provided. [#1768](https://github.com/lovell/sharp/issues/1768) +* Move `limitInputPixels` and `sequentialRead` to input options, deprecating functions of the same name. + * Expose `delay` and `loop` metadata for animated images. [#1905](https://github.com/lovell/sharp/issues/1905) diff --git a/lib/constructor.js b/lib/constructor.js index 2465bf42..e4a2c94b 100644 --- a/lib/constructor.js +++ b/lib/constructor.js @@ -86,6 +86,11 @@ const debuglog = util.debuglog('sharp'); * @param {Object} [options] - if present, is an Object with optional attributes. * @param {Boolean} [options.failOnError=true] - by default halt processing and raise an error when loading invalid images. * Set this flag to `false` if you'd rather apply a "best effort" to decode images, even if the data is corrupt or invalid. + * @param {Number|Boolean} [options.limitInputPixels=268402689] - Do not process input images where the number of pixels + * (width x height) exceeds this limit. Assumes image dimensions contained in the input metadata can be trusted. + * An integral Number of pixels, zero or false to remove limit, true to use default limit of 268402689 (0x3FFF x 0x3FFF). + * @param {Boolean} [options.sequentialRead=false] - Set this to `true` to use sequential rather than random access where possible. + * This can reduce memory usage and might improve performance on some systems. * @param {Number} [options.density=72] - number representing the DPI for vector images. * @param {Number} [options.pages=1] - number of pages to extract for multi-page input (GIF, TIFF, PDF), use -1 for all pages. * @param {Number} [options.page=0] - page number to start extracting from for multi-page input (GIF, TIFF, PDF), zero based. @@ -110,9 +115,6 @@ const Sharp = function (input, options) { } stream.Duplex.call(this); this.options = { - // input options - sequentialRead: false, - limitInputPixels: Math.pow(0x3FFF, 2), // resize options topOffsetPre: -1, leftOffsetPre: -1, diff --git a/lib/input.js b/lib/input.js index 2d28f4ce..fa83f1fb 100644 --- a/lib/input.js +++ b/lib/input.js @@ -1,5 +1,6 @@ 'use strict'; +const util = require('util'); const color = require('color'); const is = require('./is'); const sharp = require('../build/Release/sharp.node'); @@ -9,7 +10,11 @@ const sharp = require('../build/Release/sharp.node'); * @private */ function _createInputDescriptor (input, inputOptions, containerOptions) { - const inputDescriptor = { failOnError: true }; + const inputDescriptor = { + failOnError: true, + limitInputPixels: Math.pow(0x3FFF, 2), + sequentialRead: false + }; if (is.string(input)) { // filesystem inputDescriptor.file = input; @@ -48,6 +53,26 @@ function _createInputDescriptor (input, inputOptions, containerOptions) { throw is.invalidParameterError('density', 'number between 1 and 2400', inputOptions.density); } } + // limitInputPixels + if (is.defined(inputOptions.limitInputPixels)) { + if (is.bool(inputOptions.limitInputPixels)) { + inputDescriptor.limitInputPixels = inputOptions.limitInputPixels + ? Math.pow(0x3FFF, 2) + : 0; + } else if (is.integer(inputOptions.limitInputPixels) && inputOptions.limitInputPixels >= 0) { + inputDescriptor.limitInputPixels = inputOptions.limitInputPixels; + } else { + throw is.invalidParameterError('limitInputPixels', 'integer >= 0', inputOptions.limitInputPixels); + } + } + // sequentialRead + if (is.defined(inputOptions.sequentialRead)) { + if (is.bool(inputOptions.sequentialRead)) { + inputDescriptor.sequentialRead = inputOptions.sequentialRead; + } else { + throw is.invalidParameterError('sequentialRead', 'boolean', inputOptions.sequentialRead); + } + } // Raw pixel input if (is.defined(inputOptions.raw)) { if ( @@ -307,14 +332,9 @@ function stats (callback) { } /** - * Do not process input images where the number of pixels (width x height) exceeds this limit. - * Assumes image dimensions contained in the input metadata can be trusted. - * The default limit is 268402689 (0x3FFF x 0x3FFF) pixels. - * @param {(Number|Boolean)} limit - an integral Number of pixels, zero or false to remove limit, true to use default limit. - * @returns {Sharp} - * @throws {Error} Invalid limit -*/ -function limitInputPixels (limit) { + * @private + */ +const limitInputPixels = util.deprecate(function limitInputPixels (limit) { // if we pass in false we represent the integer as 0 to disable if (limit === false) { limit = 0; @@ -322,26 +342,20 @@ function limitInputPixels (limit) { limit = Math.pow(0x3FFF, 2); } if (is.integer(limit) && limit >= 0) { - this.options.limitInputPixels = limit; + this.options.input.limitInputPixels = limit; } else { throw is.invalidParameterError('limitInputPixels', 'integer', limit); } return this; -} +}, 'limitInputPixels is deprecated, use sharp(input, { limitInputPixels: false }) instead'); /** - * An advanced setting that switches the libvips access method to `VIPS_ACCESS_SEQUENTIAL`. - * This will reduce memory usage and can improve performance on some systems. - * - * The default behaviour *before* function call is `false`, meaning the libvips access method is not sequential. - * - * @param {Boolean} [sequentialRead=true] - * @returns {Sharp} + * @private */ -function sequentialRead (sequentialRead) { - this.options.sequentialRead = is.bool(sequentialRead) ? sequentialRead : true; +const sequentialRead = util.deprecate(function sequentialRead (sequentialRead) { + this.options.input.sequentialRead = is.bool(sequentialRead) ? sequentialRead : true; return this; -} +}, 'sequentialRead is deprecated, use sharp(input, { sequentialRead: true }) instead'); /** * Decorate the Sharp prototype with input-related functions. @@ -357,6 +371,7 @@ module.exports = function (Sharp) { // Public metadata, stats, + // Deprecated limitInputPixels, sequentialRead }); diff --git a/src/common.cc b/src/common.cc index 6cea7000..fdfa157b 100644 --- a/src/common.cc +++ b/src/common.cc @@ -86,6 +86,10 @@ namespace sharp { descriptor->createHeight = AttrTo(input, "createHeight"); descriptor->createBackground = AttrAsRgba(input, "createBackground"); } + // Limit input images to a given number of pixels, where pixels = width * height + descriptor->limitInputPixels = AttrTo(input, "limitInputPixels"); + // Allow switch from random to sequential access + descriptor->access = AttrTo(input, "sequentialRead") ? VIPS_ACCESS_SEQUENTIAL : VIPS_ACCESS_RANDOM; return descriptor; } @@ -244,7 +248,7 @@ namespace sharp { /* Open an image from the given InputDescriptor (filesystem, compressed buffer, raw pixel data) */ - std::tuple OpenInput(InputDescriptor *descriptor, VipsAccess accessMethod) { + std::tuple OpenInput(InputDescriptor *descriptor) { VImage image; ImageType imageType; if (descriptor->isBuffer) { @@ -264,7 +268,7 @@ namespace sharp { if (imageType != ImageType::UNKNOWN) { try { vips::VOption *option = VImage::option() - ->set("access", accessMethod) + ->set("access", descriptor->access) ->set("fail", descriptor->failOnError); if (imageType == ImageType::SVG || imageType == ImageType::PDF) { option->set("dpi", descriptor->density); @@ -310,7 +314,7 @@ namespace sharp { if (imageType != ImageType::UNKNOWN) { try { vips::VOption *option = VImage::option() - ->set("access", accessMethod) + ->set("access", descriptor->access) ->set("fail", descriptor->failOnError); if (imageType == ImageType::SVG || imageType == ImageType::PDF) { option->set("dpi", descriptor->density); @@ -334,6 +338,11 @@ namespace sharp { } } } + // Limit input images to a given number of pixels, where pixels = width * height + if (descriptor->limitInputPixels > 0 && + static_cast(image.width() * image.height()) > static_cast(descriptor->limitInputPixels)) { + throw vips::VError("Input image exceeds pixel limit"); + } return std::make_tuple(image, imageType); } diff --git a/src/common.h b/src/common.h index 088d05d2..6b464810 100644 --- a/src/common.h +++ b/src/common.h @@ -48,6 +48,8 @@ namespace sharp { std::string file; char *buffer; bool failOnError; + int limitInputPixels; + VipsAccess access; size_t bufferLength; bool isBuffer; double density; @@ -64,6 +66,8 @@ namespace sharp { InputDescriptor(): buffer(nullptr), failOnError(TRUE), + limitInputPixels(0x3FFF * 0x3FFF), + access(VIPS_ACCESS_RANDOM), bufferLength(0), isBuffer(FALSE), density(72.0), @@ -156,7 +160,7 @@ namespace sharp { /* Open an image from the given InputDescriptor (filesystem, compressed buffer, raw pixel data) */ - std::tuple OpenInput(InputDescriptor *descriptor, VipsAccess accessMethod); + std::tuple OpenInput(InputDescriptor *descriptor); /* Does this image have an embedded profile? diff --git a/src/metadata.cc b/src/metadata.cc index 66bc9a2d..e9a095f2 100644 --- a/src/metadata.cc +++ b/src/metadata.cc @@ -46,7 +46,7 @@ class MetadataWorker : public Nan::AsyncWorker { vips::VImage image; sharp::ImageType imageType = sharp::ImageType::UNKNOWN; try { - std::tie(image, imageType) = OpenInput(baton->input, VIPS_ACCESS_SEQUENTIAL); + std::tie(image, imageType) = OpenInput(baton->input); } catch (vips::VError const &err) { (baton->err).append(err.what()); } diff --git a/src/pipeline.cc b/src/pipeline.cc index 2e1e4d73..751c45b4 100644 --- a/src/pipeline.cc +++ b/src/pipeline.cc @@ -77,15 +77,7 @@ class PipelineWorker : public Nan::AsyncWorker { // Open input vips::VImage image; ImageType inputImageType; - std::tie(image, inputImageType) = sharp::OpenInput(baton->input, baton->accessMethod); - - // Limit input images to a given number of pixels, where pixels = width * height - // Ignore if 0 - if (baton->limitInputPixels > 0 && - static_cast(image.width() * image.height()) > static_cast(baton->limitInputPixels)) { - (baton->err).append("Input image exceeds pixel limit"); - return Error(); - } + std::tie(image, inputImageType) = sharp::OpenInput(baton->input); // Calculate angle of rotation VipsAngle rotation; @@ -270,7 +262,7 @@ class PipelineWorker : public Nan::AsyncWorker { if (shrink_on_load > 1) { // Reload input using shrink-on-load vips::VOption *option = VImage::option() - ->set("access", baton->accessMethod) + ->set("access", baton->input->access) ->set("shrink", shrink_on_load) ->set("fail", baton->input->failOnError); if (baton->input->buffer != nullptr) { @@ -426,7 +418,7 @@ class PipelineWorker : public Nan::AsyncWorker { ImageType joinImageType = ImageType::UNKNOWN; for (unsigned int i = 0; i < baton->joinChannelIn.size(); i++) { - std::tie(joinImage, joinImageType) = sharp::OpenInput(baton->joinChannelIn[i], baton->accessMethod); + std::tie(joinImage, joinImageType) = sharp::OpenInput(baton->joinChannelIn[i]); image = image.bandjoin(joinImage); } image = image.copy(VImage::option()->set("interpretation", baton->colourspace)); @@ -479,7 +471,7 @@ class PipelineWorker : public Nan::AsyncWorker { baton->height = image.height(); } image = image.tilecache(VImage::option() - ->set("access", baton->accessMethod) + ->set("access", VIPS_ACCESS_RANDOM) ->set("threaded", TRUE)); image = image.smartcrop(baton->width, baton->height, VImage::option() ->set("interesting", baton->position == 16 ? VIPS_INTERESTING_ENTROPY : VIPS_INTERESTING_ATTENTION)); @@ -556,7 +548,7 @@ class PipelineWorker : public Nan::AsyncWorker { for (Composite *composite : baton->composite) { VImage compositeImage; ImageType compositeImageType = ImageType::UNKNOWN; - std::tie(compositeImage, compositeImageType) = OpenInput(composite->input, baton->accessMethod); + std::tie(compositeImage, compositeImageType) = OpenInput(composite->input); // Verify within current dimensions if (compositeImage.width() > image.width() || compositeImage.height() > image.height()) { throw vips::VError("Image to composite must have same dimensions or smaller"); @@ -646,7 +638,7 @@ class PipelineWorker : public Nan::AsyncWorker { if (baton->boolean != nullptr) { VImage booleanImage; ImageType booleanImageType = ImageType::UNKNOWN; - std::tie(booleanImage, booleanImageType) = sharp::OpenInput(baton->boolean, baton->accessMethod); + std::tie(booleanImage, booleanImageType) = sharp::OpenInput(baton->boolean); image = sharp::Boolean(image, booleanImage, baton->booleanOp); } @@ -1193,10 +1185,6 @@ NAN_METHOD(pipeline) { // Input baton->input = CreateInputDescriptor(AttrAs(options, "input"), buffersToPersist); - baton->accessMethod = AttrTo(options, "sequentialRead") ? - VIPS_ACCESS_SEQUENTIAL : VIPS_ACCESS_RANDOM; - // Limit input images to a given number of pixels, where pixels = width * height - baton->limitInputPixels = AttrTo(options, "limitInputPixels"); // Extract image options baton->topOffsetPre = AttrTo(options, "topOffsetPre"); baton->leftOffsetPre = AttrTo(options, "leftOffsetPre"); @@ -1408,11 +1396,12 @@ NAN_METHOD(pipeline) { // signal that we do not want to pass any value to dzSave baton->tileDepth = VIPS_FOREIGN_DZ_DEPTH_LAST; } + // Force random access for certain operations - if (baton->accessMethod == VIPS_ACCESS_SEQUENTIAL && ( - baton->trimThreshold > 0.0 || baton->normalise || - baton->position == 16 || baton->position == 17)) { - baton->accessMethod = VIPS_ACCESS_RANDOM; + if (baton->input->access == VIPS_ACCESS_SEQUENTIAL) { + if (baton->trimThreshold > 0.0 || baton->normalise || baton->position == 16 || baton->position == 17) { + baton->input->access = VIPS_ACCESS_RANDOM; + } } // Function to notify of libvips warnings diff --git a/src/pipeline.h b/src/pipeline.h index 97b32969..084feffd 100644 --- a/src/pipeline.h +++ b/src/pipeline.h @@ -55,7 +55,6 @@ struct Composite { struct PipelineBaton { sharp::InputDescriptor *input; - int limitInputPixels; std::string formatOut; std::string fileOut; void *bufferOut; @@ -119,7 +118,6 @@ struct PipelineBaton { int extendRight; std::vector extendBackground; bool withoutEnlargement; - VipsAccess accessMethod; int jpegQuality; bool jpegProgressive; std::string jpegChromaSubsampling; @@ -182,7 +180,6 @@ struct PipelineBaton { PipelineBaton(): input(nullptr), - limitInputPixels(0), bufferOutLength(0), topOffsetPre(-1), topOffsetPost(-1), diff --git a/src/stats.cc b/src/stats.cc index 6109700c..adeaee17 100644 --- a/src/stats.cc +++ b/src/stats.cc @@ -62,7 +62,7 @@ class StatsWorker : public Nan::AsyncWorker { sharp::ImageType imageType = sharp::ImageType::UNKNOWN; try { - std::tie(image, imageType) = OpenInput(baton->input, baton->accessMethod); + std::tie(image, imageType) = OpenInput(baton->input); } catch (vips::VError const &err) { (baton->err).append(err.what()); } @@ -178,7 +178,6 @@ NAN_METHOD(stats) { // Input baton->input = sharp::CreateInputDescriptor(sharp::AttrAs(options, "input"), buffersToPersist); - baton->accessMethod = AttrTo(options, "sequentialRead") ? VIPS_ACCESS_SEQUENTIAL : VIPS_ACCESS_RANDOM; // Function to notify of libvips warnings Nan::Callback *debuglog = new Nan::Callback(sharp::AttrAs(options, "debuglog")); diff --git a/src/stats.h b/src/stats.h index ca557825..9288eba6 100644 --- a/src/stats.h +++ b/src/stats.h @@ -46,7 +46,6 @@ struct ChannelStats { struct StatsBaton { // Input sharp::InputDescriptor *input; - VipsAccess accessMethod; // Output std::vector channelStats; diff --git a/test/bench/perf.js b/test/bench/perf.js index 65ec8455..f13ba6e7 100644 --- a/test/bench/perf.js +++ b/test/bench/perf.js @@ -460,8 +460,7 @@ async.series({ }).add('sharp-sequentialRead', { defer: true, fn: function (deferred) { - sharp(inputJpgBuffer) - .sequentialRead() + sharp(inputJpgBuffer, { sequentialRead: true }) .resize(width, height) .toBuffer(function (err, buffer) { if (err) { diff --git a/test/unit/io.js b/test/unit/io.js index 99ff0c55..e37db7aa 100644 --- a/test/unit/io.js +++ b/test/unit/io.js @@ -214,7 +214,27 @@ describe('Input/output', function () { }); }); - it('Sequential read, force JPEG', function (done) { + it('Invalid sequential read option throws', () => { + assert.throws(() => { + sharp({ sequentialRead: 'fail' }); + }, /Expected boolean for sequentialRead but received fail of type string/); + }); + + it('Sequential read, force JPEG', () => + sharp(fixtures.inputJpg, { sequentialRead: true }) + .resize(320, 240) + .toFormat(sharp.format.jpeg) + .toBuffer({ resolveWithObject: true }) + .then(({ data, info }) => { + assert.strictEqual(data.length > 0, true); + assert.strictEqual(data.length, info.size); + assert.strictEqual(info.format, 'jpeg'); + assert.strictEqual(info.width, 320); + assert.strictEqual(info.height, 240); + }) + ); + + it('Sequential read, force JPEG - deprecated', function (done) { sharp(fixtures.inputJpg) .sequentialRead() .resize(320, 240) @@ -230,7 +250,21 @@ describe('Input/output', function () { }); }); - it('Not sequential read, force JPEG', function (done) { + it('Not sequential read, force JPEG', () => + sharp(fixtures.inputJpg, { sequentialRead: false }) + .resize(320, 240) + .toFormat('jpeg') + .toBuffer({ resolveWithObject: true }) + .then(({ data, info }) => { + assert.strictEqual(data.length > 0, true); + assert.strictEqual(data.length, info.size); + assert.strictEqual(info.format, 'jpeg'); + assert.strictEqual(info.width, 320); + assert.strictEqual(info.height, 240); + }) + ); + + it('Not sequential read, force JPEG - deprecated', function (done) { sharp(fixtures.inputJpg) .sequentialRead(false) .resize(320, 240) @@ -559,7 +593,67 @@ describe('Input/output', function () { }); }); - describe('Limit pixel count of input image', function () { + describe('Limit pixel count of input image', () => { + it('Invalid fails - negative', () => { + assert.throws(() => { + sharp({ limitInputPixels: -1 }); + }); + }); + + it('Invalid fails - float', () => { + assert.throws(() => { + sharp({ limitInputPixels: 12.3 }); + }); + }); + + it('Invalid fails - string', () => { + assert.throws(() => { + sharp({ limitInputPixels: 'fail' }); + }); + }); + + it('Same size as input works', () => + sharp(fixtures.inputJpg) + .metadata() + .then(({ width, height }) => + sharp(fixtures.inputJpg, { limitInputPixels: width * height }).toBuffer() + ) + ); + + it('Disabling limit works', () => + sharp(fixtures.inputJpgLarge, { limitInputPixels: false }) + .resize(2) + .toBuffer() + ); + + it('Enabling default limit works and fails with a large image', () => + sharp(fixtures.inputJpgLarge, { limitInputPixels: true }) + .toBuffer() + .then(() => { + assert.fail('Expected to fail'); + }) + .catch(err => { + assert.strictEqual(err.message, 'Input image exceeds pixel limit'); + }) + ); + + it('Smaller than input fails', () => + sharp(fixtures.inputJpg) + .metadata() + .then(({ width, height }) => + sharp(fixtures.inputJpg, { limitInputPixels: width * height - 1 }) + .toBuffer() + .then(() => { + assert.fail('Expected to fail'); + }) + .catch(err => { + assert.strictEqual(err.message, 'Input image exceeds pixel limit'); + }) + ) + ); + }); + + describe('Limit pixel count of input image - deprecated', function () { it('Invalid fails - negative', function (done) { let isValid = false; try {