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.

Sign up for GitHub

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

Add local caching of ColorSpaces, by name, in PartialEvaluator.getOperatorList (issue 2504) #12001

Merged
merged 1 commit into from
Jun 14, 2020
Merged

Add local caching of ColorSpaces, by name, in PartialEvaluator.getOperatorList (issue 2504) #12001

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
78 changes: 62 additions & 16 deletions src/core/evaluator.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -381,7 +381,8 @@ var PartialEvaluator = (function PartialEvaluatorClosure() {
smask,
operatorList,
task,
initialState
initialState,
localColorSpaceCache
) {
var dict = xobj.dict;
var matrix = dict.getArray("Matrix");
Expand All @@ -407,10 +408,19 @@ var PartialEvaluator = (function PartialEvaluatorClosure() {
groupOptions.isolated = group.get("I") || false;
groupOptions.knockout = group.get("K") || false;
if (group.has("CS")) {
colorSpace = await this.parseColorSpace({
cs: group.get("CS"),
resources,
});
const cs = group.get("CS");

const localColorSpace =
cs instanceof Name && localColorSpaceCache.getByName(cs.name);
if (localColorSpace) {
colorSpace = localColorSpace;
} else {
colorSpace = await this.parseColorSpace({
cs,
resources,
localColorSpaceCache,
});
}
}
}

Expand Down Expand Up @@ -619,7 +629,8 @@ var PartialEvaluator = (function PartialEvaluatorClosure() {
resources,
operatorList,
task,
stateManager
stateManager,
localColorSpaceCache
) {
var smaskContent = smask.get("G");
var smaskOptions = {
Expand Down Expand Up @@ -648,7 +659,8 @@ var PartialEvaluator = (function PartialEvaluatorClosure() {
smaskOptions,
operatorList,
task,
stateManager.state.clone()
stateManager.state.clone(),
localColorSpaceCache
);
},

Expand Down Expand Up @@ -800,7 +812,8 @@ var PartialEvaluator = (function PartialEvaluatorClosure() {
gState,
operatorList,
task,
stateManager
stateManager,
localColorSpaceCache
) {
// This array holds the converted/processed state data.
var gStateObj = [];
Expand Down Expand Up @@ -853,7 +866,8 @@ var PartialEvaluator = (function PartialEvaluatorClosure() {
resources,
operatorList,
task,
stateManager
stateManager,
localColorSpaceCache
);
});
gStateObj.push([key, true]);
Expand Down Expand Up @@ -1117,11 +1131,20 @@ var PartialEvaluator = (function PartialEvaluatorClosure() {
}
},

parseColorSpace({ cs, resources }) {
parseColorSpace({ cs, resources, localColorSpaceCache }) {
return new Promise(resolve => {
resolve(
ColorSpace.parse(cs, this.xref, resources, this.pdfFunctionFactory)
const parsedColorSpace = ColorSpace.parse(
cs,
this.xref,
resources,
this.pdfFunctionFactory
);

const csName = cs instanceof Name ? cs.name : null;
if (csName) {
localColorSpaceCache.set(csName, /* ref = */ null, parsedColorSpace);
}
resolve(parsedColorSpace);
}).catch(reason => {
if (reason instanceof AbortException) {
return null;
Expand Down Expand Up @@ -1198,6 +1221,7 @@ var PartialEvaluator = (function PartialEvaluatorClosure() {
var xref = this.xref;
let parsingText = false;
const localImageCache = new LocalImageCache();
const localColorSpaceCache = new LocalImageCache();
Copy link
Contributor

Choose a reason for hiding this comment

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

Just a note for possible follow-ups: if we start using this cache more often, we might want to consider renaming it since it's now used for more than just image caching.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

You're absolutely correct here; this was just me being lazy :-)

We could easily just have a base class, with Image/ColorSpace/etc. ones extending it as needed. If we want to cache by reference as well, which may help for the SMask code-path, then we probably need slightly different set methods anyway...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is now being fixed, among other things, in PR #12012 :-)


var xobjs = resources.get("XObject") || Dict.empty;
var patterns = resources.get("Pattern") || Dict.empty;
Expand Down Expand Up @@ -1309,7 +1333,8 @@ var PartialEvaluator = (function PartialEvaluatorClosure() {
null,
operatorList,
task,
stateManager.state.clone()
stateManager.state.clone(),
localColorSpaceCache
)
.then(function () {
stateManager.restore();
Expand Down Expand Up @@ -1454,12 +1479,21 @@ var PartialEvaluator = (function PartialEvaluatorClosure() {
stateManager.state.textRenderingMode = args[0];
break;

case OPS.setFillColorSpace:
case OPS.setFillColorSpace: {
const localColorSpace =
args[0] instanceof Name &&
localColorSpaceCache.getByName(args[0].name);
if (localColorSpace) {
stateManager.state.fillColorSpace = localColorSpace;
continue;
}

next(
self
.parseColorSpace({
cs: args[0],
resources,
localColorSpaceCache,
})
.then(function (colorSpace) {
if (colorSpace) {
Expand All @@ -1468,12 +1502,22 @@ var PartialEvaluator = (function PartialEvaluatorClosure() {
})
);
return;
case OPS.setStrokeColorSpace:
}
case OPS.setStrokeColorSpace: {
Copy link
Contributor

Choose a reason for hiding this comment

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

The curly braces here and above are a bit inconsistent with the other cases and don't really seem necessary, but I can imagine it looks a bit better to group the code. I don't mind either way, but perhaps we might consider making it more consistent later.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That was actually necessary because of how scopes work with switch-statements, please see https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/switch#Block-scope_variables_within_switch_statements

const localColorSpace =
args[0] instanceof Name &&
localColorSpaceCache.getByName(args[0].name);
if (localColorSpace) {
stateManager.state.strokeColorSpace = localColorSpace;
continue;
}

next(
self
.parseColorSpace({
cs: args[0],
resources,
localColorSpaceCache,
})
.then(function (colorSpace) {
if (colorSpace) {
Expand All @@ -1482,6 +1526,7 @@ var PartialEvaluator = (function PartialEvaluatorClosure() {
})
);
return;
}
case OPS.setFillColor:
cs = stateManager.state.fillColorSpace;
args = cs.getRgb(args, 0);
Expand Down Expand Up @@ -1597,7 +1642,8 @@ var PartialEvaluator = (function PartialEvaluatorClosure() {
gState,
operatorList,
task,
stateManager
stateManager,
localColorSpaceCache
)
);
return;
Expand Down