From ea6a0e4435615ba99486a9a938b0e706c7fefa14 Mon Sep 17 00:00:00 2001 From: Jonas Jenwald Date: Fri, 10 Jul 2020 16:01:33 +0200 Subject: [PATCH 1/2] Remove the IR (internal representation) part of the ColorSpace parsing Originally ColorSpaces were only *partially* parsed on the worker-thread, to obtain an IR-format which was sent to the main-thread. This had the somewhat unfortunate side-effect of causing the majority of the (potentially heavy) ColorSpace parsing to happen on the main-thread. Hence PR 4824 which, among other things, changed ColorSpaces to be *fully* parsed on the worker-thread with only RGB-data being sent to the main-thread. While it thus originally was necessary to have `ColorSpace.{parseToIR, fromIR}` methods, to handle the worker/main-thread split, that's no longer the case and we can thus reduce all of the ColorSpace parsing to one method instead. Currently, when parsing a ColorSpace, we call `ColorSpace.parseToIR` which parses the ColorSpace-data from the document and then creates the IR-format. We then, immediately, call `ColorSpace.fromIR` which parses the IR-format and then finally creates the actual ColorSpace.[1] All-in-all, this leads to a fair amount of unnecessary indirection which also (in my opinion) makes the code less clear. Obviously these changes are not really expected to have a significant effect on performance, especially with the recently added caching of ColorSpaces, however there'll now be strictly fewer function calls, less memory allocated, and overall less parsing required during ColorSpace-handling. --- [1] For ICCBased ColorSpaces, given the validation necessary, this currently even leads to parsing an /Alternate ColorSpace *twice*. --- src/core/colorspace.js | 147 +++++++++++++---------------------------- 1 file changed, 47 insertions(+), 100 deletions(-) diff --git a/src/core/colorspace.js b/src/core/colorspace.js index c65cb15a5..ff07c2547 100644 --- a/src/core/colorspace.js +++ b/src/core/colorspace.js @@ -336,8 +336,12 @@ class ColorSpace { "before calling `ColorSpace.parseAsync`." ); } - const IR = this.parseToIR(cs, xref, resources, pdfFunctionFactory); - const parsedColorSpace = this.fromIR(IR); + const parsedColorSpace = this._parse( + cs, + xref, + resources, + pdfFunctionFactory + ); // Attempt to cache the parsed ColorSpace, by name and/or reference. this._cache(cs, xref, localColorSpaceCache, parsedColorSpace); @@ -356,8 +360,12 @@ class ColorSpace { if (cachedColorSpace) { return cachedColorSpace; } - const IR = this.parseToIR(cs, xref, resources, pdfFunctionFactory); - const parsedColorSpace = this.fromIR(IR); + const parsedColorSpace = this._parse( + cs, + xref, + resources, + pdfFunctionFactory + ); // Attempt to cache the parsed ColorSpace, by name and/or reference. this._cache(cs, xref, localColorSpaceCache, parsedColorSpace); @@ -365,69 +373,24 @@ class ColorSpace { return parsedColorSpace; } - static fromIR(IR) { - const name = Array.isArray(IR) ? IR[0] : IR; - let whitePoint, blackPoint, gamma; - - switch (name) { - case "DeviceGrayCS": - return this.singletons.gray; - case "DeviceRgbCS": - return this.singletons.rgb; - case "DeviceCmykCS": - return this.singletons.cmyk; - case "CalGrayCS": - whitePoint = IR[1]; - blackPoint = IR[2]; - gamma = IR[3]; - return new CalGrayCS(whitePoint, blackPoint, gamma); - case "CalRGBCS": - whitePoint = IR[1]; - blackPoint = IR[2]; - gamma = IR[3]; - const matrix = IR[4]; - return new CalRGBCS(whitePoint, blackPoint, gamma, matrix); - case "PatternCS": - let basePatternCS = IR[1]; - if (basePatternCS) { - basePatternCS = this.fromIR(basePatternCS); - } - return new PatternCS(basePatternCS); - case "IndexedCS": - const baseIndexedCS = IR[1]; - const hiVal = IR[2]; - const lookup = IR[3]; - return new IndexedCS(this.fromIR(baseIndexedCS), hiVal, lookup); - case "AlternateCS": - const numComps = IR[1]; - const alt = IR[2]; - const tintFn = IR[3]; - return new AlternateCS(numComps, this.fromIR(alt), tintFn); - case "LabCS": - whitePoint = IR[1]; - blackPoint = IR[2]; - const range = IR[3]; - return new LabCS(whitePoint, blackPoint, range); - default: - throw new FormatError(`Unknown colorspace name: ${name}`); - } - } - - static parseToIR(cs, xref, resources = null, pdfFunctionFactory) { + /** + * @private + */ + static _parse(cs, xref, resources = null, pdfFunctionFactory) { cs = xref.fetchIfRef(cs); if (isName(cs)) { switch (cs.name) { case "DeviceGray": case "G": - return "DeviceGrayCS"; + return this.singletons.gray; case "DeviceRGB": case "RGB": - return "DeviceRgbCS"; + return this.singletons.rgb; case "DeviceCMYK": case "CMYK": - return "DeviceCmykCS"; + return this.singletons.cmyk; case "Pattern": - return ["PatternCS", null]; + return new PatternCS(/* baseCS = */ null); default: if (isDict(resources)) { const colorSpaces = resources.get("ColorSpace"); @@ -435,7 +398,7 @@ class ColorSpace { const resourcesCS = colorSpaces.get(cs.name); if (resourcesCS) { if (isName(resourcesCS)) { - return this.parseToIR( + return this._parse( resourcesCS, xref, resources, @@ -447,107 +410,91 @@ class ColorSpace { } } } - throw new FormatError(`unrecognized colorspace ${cs.name}`); + throw new FormatError(`Unrecognized ColorSpace: ${cs.name}`); } } if (Array.isArray(cs)) { const mode = xref.fetchIfRef(cs[0]).name; - let numComps, params, alt, whitePoint, blackPoint, gamma; + let params, numComps, baseCS, whitePoint, blackPoint, gamma; switch (mode) { case "DeviceGray": case "G": - return "DeviceGrayCS"; + return this.singletons.gray; case "DeviceRGB": case "RGB": - return "DeviceRgbCS"; + return this.singletons.rgb; case "DeviceCMYK": case "CMYK": - return "DeviceCmykCS"; + return this.singletons.cmyk; case "CalGray": params = xref.fetchIfRef(cs[1]); whitePoint = params.getArray("WhitePoint"); blackPoint = params.getArray("BlackPoint"); gamma = params.get("Gamma"); - return ["CalGrayCS", whitePoint, blackPoint, gamma]; + return new CalGrayCS(whitePoint, blackPoint, gamma); case "CalRGB": params = xref.fetchIfRef(cs[1]); whitePoint = params.getArray("WhitePoint"); blackPoint = params.getArray("BlackPoint"); gamma = params.getArray("Gamma"); const matrix = params.getArray("Matrix"); - return ["CalRGBCS", whitePoint, blackPoint, gamma, matrix]; + return new CalRGBCS(whitePoint, blackPoint, gamma, matrix); case "ICCBased": const stream = xref.fetchIfRef(cs[1]); const dict = stream.dict; numComps = dict.get("N"); - alt = dict.get("Alternate"); + const alt = dict.get("Alternate"); if (alt) { - const altIR = this.parseToIR( - alt, - xref, - resources, - pdfFunctionFactory - ); - // Parse the /Alternate CS to ensure that the number of components - // are correct, and also (indirectly) that it is not a PatternCS. - const altCS = this.fromIR(altIR); + const altCS = this._parse(alt, xref, resources, pdfFunctionFactory); + // Ensure that the number of components are correct, + // and also (indirectly) that it is not a PatternCS. if (altCS.numComps === numComps) { - return altIR; + return altCS; } warn("ICCBased color space: Ignoring incorrect /Alternate entry."); } if (numComps === 1) { - return "DeviceGrayCS"; + return this.singletons.gray; } else if (numComps === 3) { - return "DeviceRgbCS"; + return this.singletons.rgb; } else if (numComps === 4) { - return "DeviceCmykCS"; + return this.singletons.cmyk; } break; case "Pattern": - let basePatternCS = cs[1] || null; - if (basePatternCS) { - basePatternCS = this.parseToIR( - basePatternCS, - xref, - resources, - pdfFunctionFactory - ); + baseCS = cs[1] || null; + if (baseCS) { + baseCS = this._parse(baseCS, xref, resources, pdfFunctionFactory); } - return ["PatternCS", basePatternCS]; + return new PatternCS(baseCS); case "Indexed": case "I": - const baseIndexedCS = this.parseToIR( - cs[1], - xref, - resources, - pdfFunctionFactory - ); + baseCS = this._parse(cs[1], xref, resources, pdfFunctionFactory); const hiVal = xref.fetchIfRef(cs[2]) + 1; let lookup = xref.fetchIfRef(cs[3]); if (isStream(lookup)) { lookup = lookup.getBytes(); } - return ["IndexedCS", baseIndexedCS, hiVal, lookup]; + return new IndexedCS(baseCS, hiVal, lookup); case "Separation": case "DeviceN": const name = xref.fetchIfRef(cs[1]); numComps = Array.isArray(name) ? name.length : 1; - alt = this.parseToIR(cs[2], xref, resources, pdfFunctionFactory); + baseCS = this._parse(cs[2], xref, resources, pdfFunctionFactory); const tintFn = pdfFunctionFactory.create(cs[3]); - return ["AlternateCS", numComps, alt, tintFn]; + return new AlternateCS(numComps, baseCS, tintFn); case "Lab": params = xref.fetchIfRef(cs[1]); whitePoint = params.getArray("WhitePoint"); blackPoint = params.getArray("BlackPoint"); const range = params.getArray("Range"); - return ["LabCS", whitePoint, blackPoint, range]; + return new LabCS(whitePoint, blackPoint, range); default: - throw new FormatError(`unimplemented color space object "${mode}"`); + throw new FormatError(`Unimplemented ColorSpace object: ${mode}`); } } - throw new FormatError(`unrecognized color space object: "${cs}"`); + throw new FormatError(`Unrecognized ColorSpace object: ${cs}`); } /** From d18cf474191fca913c1abaee7ce9fef7987055b3 Mon Sep 17 00:00:00 2001 From: Jonas Jenwald Date: Fri, 10 Jul 2020 16:12:20 +0200 Subject: [PATCH 2/2] Remove the special handling, used when creating Indexed ColorSpaces, for the case where the `lookup`-data is a `Stream` This special-case was added in PR 1992, however it became unnecessary with the changes in PR 4824 since all of the ColorSpace parsing is now done on the worker-thread (with only RGB-data being sent to the main-thread). --- src/core/colorspace.js | 20 ++++++-------------- test/unit/colorspace_spec.js | 12 +++++++----- 2 files changed, 13 insertions(+), 19 deletions(-) diff --git a/src/core/colorspace.js b/src/core/colorspace.js index ff07c2547..219584ddd 100644 --- a/src/core/colorspace.js +++ b/src/core/colorspace.js @@ -17,7 +17,6 @@ import { assert, FormatError, info, - isString, shadow, unreachable, warn, @@ -472,10 +471,7 @@ class ColorSpace { case "I": baseCS = this._parse(cs[1], xref, resources, pdfFunctionFactory); const hiVal = xref.fetchIfRef(cs[2]) + 1; - let lookup = xref.fetchIfRef(cs[3]); - if (isStream(lookup)) { - lookup = lookup.getBytes(); - } + const lookup = xref.fetchIfRef(cs[3]); return new IndexedCS(baseCS, hiVal, lookup); case "Separation": case "DeviceN": @@ -643,22 +639,18 @@ class IndexedCS extends ColorSpace { this.base = base; this.highVal = highVal; - const baseNumComps = base.numComps; - const length = baseNumComps * highVal; + const length = base.numComps * highVal; + this.lookup = new Uint8Array(length); if (isStream(lookup)) { - this.lookup = new Uint8Array(length); const bytes = lookup.getBytes(length); this.lookup.set(bytes); - } else if (isString(lookup)) { - this.lookup = new Uint8Array(length); + } else if (typeof lookup === "string") { for (let i = 0; i < length; ++i) { - this.lookup[i] = lookup.charCodeAt(i); + this.lookup[i] = lookup.charCodeAt(i) & 0xff; } - } else if (lookup instanceof Uint8Array) { - this.lookup = lookup; } else { - throw new FormatError(`Unrecognized lookup table: ${lookup}`); + throw new FormatError(`IndexedCS - unrecognized lookup table: ${lookup}`); } } diff --git a/test/unit/colorspace_spec.js b/test/unit/colorspace_spec.js index eeb381e33..ca56de381 100644 --- a/test/unit/colorspace_spec.js +++ b/test/unit/colorspace_spec.js @@ -679,11 +679,13 @@ describe("colorspace", function () { describe("IndexedCS", function () { it("should handle the case when cs is an array", function () { // prettier-ignore - const lookup = new Uint8Array([ - 23, 155, 35, - 147, 69, 93, - 255, 109, 70 - ]); + const lookup = new Stream( + new Uint8Array([ + 23, 155, 35, + 147, 69, 93, + 255, 109, 70 + ]) + ); const cs = [Name.get("Indexed"), Name.get("DeviceRGB"), 2, lookup]; const xref = new XRefMock([ {