Add support for optional marked content. #12095

brendandahl · 2020-07-14T22:44:22Z

Add a new method to the API to get the optional content configuration. Add
a new render task param that accepts the above configuration.
For now, the optional content is not controllable by the user in
the viewer, but renders with the default configuration in the PDF.

All of the test files added exhibit different uses of optional content.

Fixes #269.

brendandahl · 2020-07-14T22:44:39Z

/botio test

pdfjsbot · 2020-07-14T22:44:40Z

From: Bot.io (Windows)

Received

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

Live output at: http://54.215.176.217:8877/6ff05971f6f1fd5/output.txt

pdfjsbot · 2020-07-14T22:44:40Z

From: Bot.io (Linux m4)

Received

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

Live output at: http://54.67.70.0:8877/48e5f33dc64066f/output.txt

pdfjsbot · 2020-07-14T23:11:35Z

From: Bot.io (Linux m4)

Failed

Full output at http://54.67.70.0:8877/48e5f33dc64066f/output.txt

Total script time: 26.89 mins

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

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

pdfjsbot · 2020-07-14T23:16:00Z

From: Bot.io (Windows)

Failed

Full output at http://54.215.176.217:8877/6ff05971f6f1fd5/output.txt

Total script time: 31.32 mins

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

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

brendandahl · 2020-07-15T17:45:59Z

/botio test

pdfjsbot · 2020-07-15T17:46:00Z

From: Bot.io (Windows)

Received

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

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

pdfjsbot · 2020-07-15T17:46:00Z

From: Bot.io (Linux m4)

Received

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

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

Snuffleupagus · 2020-07-15T16:57:32Z

src/core/evaluator.js

+    } else if (isDict(contentProperties)) {
+      optionalContent = contentProperties;
+    } else {
+      throw new Error("Optional content properties malformed.");


Should be throw new FormatError(...), since that's used for errors with the PDF data itself.

Snuffleupagus · 2020-07-15T16:59:31Z

src/core/evaluator.js

@@ -1224,6 +1232,61 @@ class PartialEvaluator {
    throw new FormatError(`Unknown PatternName: ${patternName}`);
  }

+  parseMarkedContentProps(contentProperties, resources) {


Please make this method async, since the data fetching below (i.e. the Dict.get(...) calls) could throw which would thus break the OperatorList parsing, and that way you'll be able to (easier) deal with any errors gracefully (relying on the existing ignoreErrors functionality).

Snuffleupagus · 2020-07-15T17:03:23Z

src/core/evaluator.js

+              warn(`Expected name for beginMarkedContentProps arg0=${args[0]}`);
+              continue;
+            }
+            fn = OPS.beginMarkedContentProps;


This line looks superfluous, since fn === OPS.beginMarkedContentProps must hold in order for this block to have been entered!?

Snuffleupagus · 2020-07-15T17:07:33Z

src/core/obj.js

+    const properties = this.catDict.get("OCProperties");
+    if (!properties) {
+      return null;
+    }
+    const groupsData = properties.get("OCGs");
+    if (!Array.isArray(groupsData)) {
+      return null;
+    }
+    const groups = [];
+    const groupRefs = [];
+    // Ensure all the optional content groups are valid.
+    for (const groupRef of groupsData) {
+      if (!isRef(groupRef)) {
+        continue;
+      }
+      groupRefs.push(groupRef);
+      const group = properties.xref.fetchIfRef(groupRef);
+      groups.push({
+        id: groupRef.toString(),
+        name: group.get("Name"),
+        intent: group.get("Intent"), // TODO validate this.
+      });
+    }
+    const defaultConfig = properties.get("D");
+    if (!defaultConfig) {
+      return null;
+    }
+    const config = this._readOptionalContentConfig(defaultConfig, groupRefs);
+    config.groups = groups;
+    return config;


Similar to all other methods here, see e.g. get permissions() above, please wrap this in try...catch to prevent breaking errors if data fetching fails (since Dict.get(...) calls could throw).

Will get optionalContentConfig() always return the same data for a document with Optional Content groups?
If so, please shadow the return values to avoid having to repeatedly re-parsing this for every single page. (Or at least, please shadow the return null; cases.)

Snuffleupagus · 2020-07-15T17:07:57Z

src/core/obj.js

+  }
+
+  _readOptionalContentConfig(config, contentGroupRefs) {
+    // console.log(contentGroupRefs);


Left-over debugging statement?

Snuffleupagus · 2020-07-15T17:31:00Z

src/core/evaluator.js

+            }
+            fn = OPS.beginMarkedContentProps;
+            if (args[0].name === "OC") {
+              args = ["OC", self.parseMarkedContentProps(args[1], resources)];


With parseMarkedContentProps being asynchronous, as suggested above, this would need to become something like this (which thus handles any errors gracefully):

next( self.parseMarkedContentProps(args[1], resources) .then(data => { operatorList.addOp(OPS.beginMarkedContentProps, ["OC", data]); }) .catch(reason => { if (reason instanceof AbortException) { return; } if (self.options.ignoreErrors) { self.handler.send("UnsupportedFeature", { featureId: /* Some new UNSUPPORTED_FEATURES id */, }); warn(`getOperatorList - ignoring beginMarkedContentProps: "${reason}".`); return; } throw reason; }) ); return;

Snuffleupagus · 2020-07-15T17:35:02Z

src/core/evaluator.js

      };

+      if (dict.has("OC")) {
+        groupOptions.optionalContent = this.parseMarkedContentProps(


Nit: With parseMarkedContentProps being asynchronous, as suggested below, this would need to be groupOptions.optionalContent = await this.parseMarkedContentProps( instead.

Snuffleupagus · 2020-07-15T17:39:37Z

src/display/canvas.js

+          if (simpleFillText && !accent && this.contentVisible) {
            // common case
            ctx.fillText(character, scaledX, scaledY);
-          } else {
+          } else if (this.contentVisible) {


It would in my opinion be, a little bit, clearer if you simply enclosed this entire code-block

pdf.js/src/display/canvas.js

Lines 1698 to 1717 in 6c39aff

// Only attempt to draw the glyph if it is actually in the embedded font

// file or if there isn't a font file so the fallback font is shown.

if (glyph.isInFont || font.missingFile) {

if (simpleFillText && !accent) {

// common case

ctx.fillText(character, scaledX, scaledY);

} else {

this.paintChar(character, scaledX, scaledY, patternTransform);

if (accent) {

scaledAccentX = scaledX + accent.offset.x / fontSizeScale;

scaledAccentY = scaledY - accent.offset.y / fontSizeScale;

this.paintChar(

accent.fontChar,

scaledAccentX,

scaledAccentY,

patternTransform

);

}

}

}

in if (this.contentVisible) { ... } rather than "inlining" these checks.

Also, unless I'm mistaken there's no similar checks in the showType3Text below. Don't you need to account for this.contentVisible there as well?

Snuffleupagus · 2020-07-15T17:53:37Z

src/display/api.js

+      if (this._canvas) {
+        if (canvasInRendering.has(this._canvas)) {
+          this.capability.reject(
+            new Error(
+              "Cannot use the same canvas during multiple render() operations. " +
+                "Use different canvas or ensure previous operations were " +
+                "cancelled or completed."
+            )
+          );
+        } else {
+          canvasInRendering.add(this._canvas);
+        }
+      }


Which test-case required moving this code, and is there another way to deal with that?
(Since the commit message says: - Reject the InternalRenderTask earlier now that InitializeGraphics is waits for another promise.)

I've not actually tested it, but based on reading the code I'm not entirely convinced that moving this code will actually do the right thing in general.
Currently initializeGraphics will throw

pdf.js/src/display/api.js

Lines 2668 to 2672 in 6c39aff

throw new Error(

"Cannot use the same canvas during multiple render() operations. " +

"Use different canvas or ensure previous operations were " +

"cancelled or completed."

);

which thus means that we're reaching this catch

pdf.js/src/display/api.js

Lines 1118 to 1121 in 6c39aff

internalRenderTask.initializeGraphics(transparency);

internalRenderTask.operatorListChanged();

})

.catch(complete);

which thus calls

pdf.js/src/display/api.js

Lines 1054 to 1081 in 6c39aff

const complete = error => {

const i = intentState.renderTasks.indexOf(internalRenderTask);

if (i >= 0) {

intentState.renderTasks.splice(i, 1);

}

// Attempt to reduce memory usage during *printing*, by always running

// cleanup once rendering has finished (regardless of cleanupAfterRender).

if (this.cleanupAfterRender || renderingIntent === "print") {

this.pendingCleanup = true;

}

this._tryCleanup();

if (error) {

internalRenderTask.capability.reject(error);

this._abortOperatorList({

intentState,

reason: error,

});

} else {

internalRenderTask.capability.resolve();

}

if (this._stats) {

this._stats.timeEnd("Rendering");

this._stats.timeEnd("Overall");

}

};

as expected and all of the required clean-up will thus run.

With your changes here, it doesn't seem like you're actually guaranteed that complete is ever called correctly!?

It was the test that calls render twice. It doesn't seem to be failing anymore. This code is quite the mix of callbacks and old style promises.

pdfjsbot · 2020-07-15T18:12:53Z

From: Bot.io (Linux m4)

Failed

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

Total script time: 26.87 mins

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

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

pdfjsbot · 2020-07-15T18:16:35Z

From: Bot.io (Windows)

Failed

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

Total script time: 30.57 mins

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

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

Snuffleupagus · 2020-07-16T07:45:41Z

src/core/obj.js

+          continue;
+        }
+        groupRefs.push(groupRef);
+        const group = properties.xref.fetchIfRef(groupRef);


I missed this previously: You want this.xref.fetchIfRef(groupRef); instead here, since this.xref is guaranteed to be defined whereas a Dict instance may not always have an xref property.

Snuffleupagus · 2020-07-16T07:50:53Z

src/core/obj.js

+      if (ex instanceof MissingDataException) {
+        throw ex;
+      }
+      warn("Unable to optional content config. " + ex);


Missing e.g. the word "read" here?

warn(`Unable to read optional content config: ${ex}`);

Snuffleupagus · 2020-07-16T07:56:09Z

src/core/obj.js

+          name: group.get("Name"),
+          intent: group.get("Intent"), // TODO validate this.


Since this data is being sent between threads, whatever data this contains much be compatible with the "structured clone algorithm". Hence you may want to add at least some basic validation of these two properties since otherwise a corrupt PDF document might refuse to render altogether.

Snuffleupagus · 2020-07-16T07:58:37Z

src/core/evaluator.js

+        return {
+          type: optionalContentType,
+          ids: groupIds,
+          policy: optionalContent.get("P"),


Since this data is being sent between threads, whatever data this contains much be compatible with the "structured clone algorithm". Hence you may want to add at least some basic validation of the P property since otherwise a corrupt PDF document might refuse to render altogether.

According to the specification, https://www.adobe.com/content/dam/acom/en/devnet/acrobat/pdfs/PDF32000_2008.pdf#G7.3883825, this should be a Name. In the OptionalContentConfig class you're accessing them as if they were mere strings, which I don't think will work!?

I believe that you'll need to either validate that it's a name and (essentially) return optionalContent.get("P").name, here, or possibly return the name as-is a and fix the lookup in OptionalContentConfig instead.

I thought there was a test PDF for this, but appears there aren't any with a policy attribute. If I have some time I'll try to make some more tests.

Snuffleupagus · 2020-07-16T07:59:55Z

src/core/obj.js

+      name: config.get("Name"),
+      creator: config.get("Creator"),
+      baseState: config.get("BaseState"),
+      on: parseOnOff(config.get("ON")),
+      off: parseOnOff(config.get("OFF")),


Since this data is being sent between threads, whatever data this contains much be compatible with the "structured clone algorithm". Hence you may want to add at least some basic validation of the Name/Creator/BaseState properties since otherwise a corrupt PDF document might refuse to render altogether.

Snuffleupagus · 2020-07-16T08:06:31Z

src/display/api.js

+    return this.messageHandler
+      .sendWithPromise("GetOptionalContentConfig", null)
+      .then(results => {
+        const config = new OptionalContentConfig();


Just wondering: Any particular reason to not simply pass the results when creating the OptionalContentConfig instance, and thus essentially inline the initialize method in the constructor?

I had plans of doing some unit testing this differently. Not going to do that now, so I'll merge it.

Snuffleupagus · 2020-07-16T11:18:25Z

src/core/obj.js

+        const group = properties.xref.fetchIfRef(groupRef);
+        groups.push({
+          id: groupRef.toString(),
+          name: group.get("Name"),


Since this is, I believe, a string; don't you need to wrap it in stringToPDFString() to avoid issues?

Edit: Issue #7283 is one example which requires this.

Snuffleupagus · 2020-07-16T11:19:14Z

src/core/obj.js

+      name: config.get("Name"),
+      creator: config.get("Creator"),


Since these are, I believe, strings; don't you need to wrap them in stringToPDFString() to avoid issues?

Edit: Issue #7283 is one example which requires this.

Snuffleupagus · 2020-07-16T11:26:01Z

src/core/obj.js

+    return {
+      name: config.get("Name"),
+      creator: config.get("Creator"),
+      baseState: config.get("BaseState"),


According to the specification, https://www.adobe.com/content/dam/acom/en/devnet/acrobat/pdfs/PDF32000_2008.pdf#G7.3883884, this should be a Name. In the OptionalContentConfig class you're accessing them as if they were mere strings, which I don't think will work!?

I believe that you'll need to either validate that it's a name and (essentially) return config.get("BaseState").name, here, or possibly return the name as-is a and fix the lookup in OptionalContentConfig instead.

Snuffleupagus · 2020-07-16T11:29:21Z

src/core/evaluator.js

+        return {
+          type: optionalContentType,
+          ids: groupIds,
+          policy: optionalContent.get("P"),


According to the specification, https://www.adobe.com/content/dam/acom/en/devnet/acrobat/pdfs/PDF32000_2008.pdf#G7.3883825, this should be a Name. In the OptionalContentConfig class you're accessing them as if they were mere strings, which I don't think will work!?

I believe that you'll need to either validate that it's a name and (essentially) return optionalContent.get("P").name, here, or possibly return the name as-is a and fix the lookup in OptionalContentConfig instead.

Snuffleupagus · 2020-07-16T12:46:00Z

src/display/canvas.js

@@ -790,6 +794,15 @@ var CanvasGraphics = (function CanvasGraphicsClosure() {
    ctx.drawImage(mask, 0, 0);
  }

+  function isContentVisible(stack) {


Given that every call-sites seem to pass in this.markedContentStack manually, any reason not to make this a "normal" method and simply handle the stack internally instead?
E.g. something like this

_computeContentVisible() { const state = this.markedContentStack; for (let i = stack.length - 1; i >= 0; i--) { if (!stack[i].visible) { return false; } } return true; },

and the call-sites could then simply read this.contentVisible = this._computeContentVisible();

Snuffleupagus · 2020-07-16T12:47:18Z

src/core/evaluator.js

@@ -1695,6 +1755,45 @@ class PartialEvaluator {
            // e.g. as done in https://github.com/mozilla/pdf.js/pull/6266,
            // but doing so is meaningless without knowing the semantics.
            continue;
+          case OPS.beginMarkedContentProps:
+            if (!isName(args[0])) {
+              warn(`Expected name for beginMarkedContentProps arg0=${args[0]}`);


Would it actually make sense to simply move this check into the next handling below and throw a FormatError instead?
I.e. something similar to the next code that's used e.g. when parsing OPS.paintXObject earlier in this file.

Really sorry about the back-and-forth in some of these comments, but it's quite a lot of added code/functionality to "unpack" here and at first glance I unfortunately overlooked some of the finer details.
The overall functionality seem to work very nicely though :-)

I started to do do this, but sometimes the marked content isn't an "OC" which then causes us to wait async when we really don't need to.

Snuffleupagus

r=me, with the commits squashed and all tests passing.

Snuffleupagus · 2020-07-17T07:47:16Z

src/core/obj.js

+      const defaultConfig = properties.get("D");
+      if (!defaultConfig) {
+        return shadow(this, "optionalContentConfig", null);
+      }


Nit: Would it make sense to move this to occur earlier in the method, since the groupsData parsing above seems pointless in the !defaultConfig case?

timvandermeij

Minor comments, also looks good to me after this.

timvandermeij · 2020-07-17T18:01:01Z

src/core/evaluator.js

+
+        let expression = null;
+        if (optionalContent.get("VE")) {
+          // TODO support content epxressions.


Nit: expressions

timvandermeij · 2020-07-17T18:03:24Z

src/core/worker.js

@@ -485,6 +485,13 @@ class WorkerMessageHandler {
      return pdfManager.ensureCatalog("documentOutline");
    });

+    handler.on(
+      "GetOptionalContentConfig",
+      function wphSetupGetOptionalContentConfig(data) {


This handler can be shortened a bit by using an anonymous function like the one below.

timvandermeij · 2020-07-17T18:04:36Z

src/display/api.js

+  /**
+   * @returns {Promise} A promise that is resolved with an
+   * {OptionalContentConfig} that will have all the optional content groups (if
+   * the document has any). @see {OptionalContentConfig}


Shouldn't @see be on a new line for JSDoc?

timvandermeij · 2020-07-17T18:06:02Z

src/display/api.js

+ * @property {Promise} [optionalContentConfigPromise] - A promise that should
+ *                     resolve with an {OptionalContentConfig} created from
+ *                     PDFDocumentProxy.getOptionalContentConfig. If null, the
+ *                     config will automatically fetched with the default


Nit: automatically be fetched

timvandermeij · 2020-07-17T18:08:44Z

src/display/optional_content_config.js

@@ -0,0 +1,107 @@
+/* Copyright 2012 Mozilla Foundation


brendandahl · 2020-07-17T21:42:45Z

/botio test

pdfjsbot · 2020-07-17T21:42:46Z

From: Bot.io (Linux m4)

Received

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

Live output at: http://54.67.70.0:8877/871b55504c08464/output.txt

pdfjsbot · 2020-07-17T21:42:46Z

From: Bot.io (Windows)

Received

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

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

pdfjsbot · 2020-07-17T22:09:38Z

From: Bot.io (Linux m4)

Failed

Full output at http://54.67.70.0:8877/871b55504c08464/output.txt

Total script time: 26.85 mins

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

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

pdfjsbot · 2020-07-17T22:13:36Z

From: Bot.io (Windows)

Failed

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

Total script time: 30.81 mins

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

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

Snuffleupagus

According to the test logs, there's two actual errors thrown during rendering:

TEST-UNEXPECTED-FAIL | test failed issue7769 | in firefox | page1 round 1 | render : Error: Optional content group must be found.
TEST-UNEXPECTED-FAIL | test failed issue7769 | in chrome | page1 round 1 | render : Error: Optional content group must be found.

Unfortunately, it appears that this particular error occurs in such a way that the failures aren't displayed in the "Reftest analyzer" (and I thus missed it previously).

src/display/optional_content_config.js

janpe2 · 2020-07-19T18:17:40Z

src/core/evaluator.js

      };

+      if (dict.has("OC")) {


All form XObjects can have an OC entry, not just transparency groups that have a Group entry. Then you may need to pass the parsed OC value also to paintFormXObjectBegin/End and handle it in canvas.js.

Good catch, I'll just insert a beginMarkedContentProps before the form object so I don't have to handle this for both.

janpe2 · 2020-07-22T15:53:58Z

You should push a { visible: true } object also in beginMarkedContent ("BMC").

pdf.js/src/display/canvas.js

Line 2444 in d69fb44

beginMarkedContent: function CanvasGraphics_beginMarkedContent(tag) {

"EMC" is the end of both "BMC" and "BDC" sections. "BMC" is not related to optional content but some PDFs may use it and the marked content stack might go wrong.

Snuffleupagus · 2020-07-28T20:03:29Z

@brendandahl Given that this PR seems really close to being done, since there's just a few more comments to fix (such that all tests pass), do you perhaps have time to fix the remaining things such that this can land?

I've been playing around with this patch a fair bit locally[1], and it seems to work really well in practice :-)
Also, the overall implementation is a lot simpler than I thought it would be when reading the specification.

[1] I've got, more-or-less, finished patches for the viewer integration.

brendandahl · 2020-08-03T22:40:10Z

/botio test

pdfjsbot · 2020-08-03T22:40:11Z

From: Bot.io (Linux m4)

Received

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

Live output at: http://54.67.70.0:8877/48c34ec27dcef8c/output.txt

pdfjsbot · 2020-08-03T22:40:11Z

From: Bot.io (Windows)

Received

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

Live output at: http://54.215.176.217:8877/0f7ae373a4b37f7/output.txt

pdfjsbot · 2020-08-03T23:07:08Z

From: Bot.io (Linux m4)

Failed

Full output at http://54.67.70.0:8877/48c34ec27dcef8c/output.txt

Total script time: 26.94 mins

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

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

timvandermeij

I'm leaving the review for @Snuffleupagus, but I'm just checking the comments/types here. For context, we have been working a lot on that the last few days in #12102 and follow-up pull requests. The main priority was to get TypeScript definitions generated automatically from the source code and to get the comments in a consistent format, so this requires a small number of changes here to make sure that once #12156 lands everything is still consistent and the generated types are correct. Thanks!

timvandermeij · 2020-08-03T22:56:06Z

src/display/api.js

@@ -788,6 +789,16 @@ class PDFDocumentProxy {
    return this._transport.getOutline();
  }

+  /**
+   * @see {OptionalContentConfig}
+   * @returns {Promise<OptionalContentConfig>} A promise that is resolved with


I think this should be @returns {Promise<OptionalContentConfig | null>} looking at the catalog code. Moreover, @link should be used to refer to other types/code and it should be formatted in the standard way. In summary, this is my suggestion based on the existing comments:

/** * @returns {Promise<OptionalContentConfig | null>} A promise that is resolved * with an {@link OptionalContentConfig} that will have all the optional * content groups, or `null` if the document does not have any. */

timvandermeij · 2020-08-03T22:57:10Z

src/display/api.js

@@ -788,6 +789,16 @@ class PDFDocumentProxy {
    return this._transport.getOutline();
  }

+  /**
+   * @see {OptionalContentConfig}


This line can be removed since the type is in the @returns already so JSDoc will pick it up (we don't have this elsewhere too).

timvandermeij · 2020-08-03T23:03:45Z

src/display/api.js

@@ -965,6 +976,11 @@ class PDFDocumentProxy {
 *                    image). The default value is 'rgb(255,255,255)'.
 * @property {Object} [annotationStorage] - Storage for annotation data in
 *                    forms.
+ * @property {Promise} [optionalContentConfigPromise] - A promise that should


Once #12156 lands, this should be changed to mention the promise type and to have each line start with two spaces as per the standard.

pdfjsbot · 2020-08-03T23:11:41Z

From: Bot.io (Windows)

Failed

Full output at http://54.215.176.217:8877/0f7ae373a4b37f7/output.txt

Total script time: 31.48 mins

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

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

timvandermeij · 2020-08-03T23:15:08Z

By the way, the unit test failures (and most likely the reference test failures as well) are known intermittents and are tracked in #12123 (the Windows one is a new one that I now mentioned there).

Snuffleupagus · 2020-08-04T08:16:18Z

/botio test

pdfjsbot · 2020-08-04T08:16:20Z

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/fc2151d724bc8a7/output.txt

pdfjsbot · 2020-08-04T08:16:20Z

From: Bot.io (Windows)

Received

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

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

pdfjsbot · 2020-08-04T08:43:23Z

From: Bot.io (Linux m4)

Failed

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

Total script time: 27.04 mins

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

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

pdfjsbot · 2020-08-04T08:48:26Z

From: Bot.io (Windows)

Failed

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

Total script time: 32.10 mins

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

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

Snuffleupagus

Looks good to me, thank you!

Snuffleupagus · 2020-08-04T09:11:26Z

src/display/canvas.js

@@ -2446,15 +2496,28 @@ var CanvasGraphics = (function CanvasGraphicsClosure() {
    },
    beginMarkedContent: function CanvasGraphics_beginMarkedContent(tag) {
      // TODO Marked content.


Nit: Remove this comment?

Add a new method to the API to get the optional content configuration. Add a new render task param that accepts the above configuration. For now, the optional content is not controllable by the user in the viewer, but renders with the default configuration in the PDF. All of the test files added exhibit different uses of optional content. Fixes mozilla#269. Fix test to work with optional content. - Change the stopAtErrors test to ensure the operator list has something, instead of asserting the exact number of operators.

brendandahl · 2020-08-04T16:38:10Z

/botio makeref

pdfjsbot · 2020-08-04T16:38:12Z

From: Bot.io (Linux m4)

Received

Command cmd_makeref from @brendandahl received. Current queue size: 0

Live output at: http://54.67.70.0:8877/540ff90a3e49b5e/output.txt

pdfjsbot · 2020-08-04T16:38:12Z

From: Bot.io (Windows)

Received

Command cmd_makeref from @brendandahl received. Current queue size: 0

Live output at: http://54.215.176.217:8877/20d3f22d0d860ed/output.txt

pdfjsbot · 2020-08-04T17:03:41Z

From: Bot.io (Linux m4)

Success

Full output at http://54.67.70.0:8877/540ff90a3e49b5e/output.txt

Total script time: 25.47 mins

Lint: Passed
Make references: Passed
Check references: Passed

pdfjsbot · 2020-08-04T17:05:39Z

From: Bot.io (Windows)

Success

Full output at http://54.215.176.217:8877/20d3f22d0d860ed/output.txt

Total script time: 27.44 mins

Lint: Passed
Make references: Passed
Check references: Passed

brendandahl mentioned this pull request Jul 14, 2020

Add UI to control what optional content is visible #12096

Closed

Snuffleupagus requested changes Jul 15, 2020

View reviewed changes

brendandahl force-pushed the oc branch from d1891b6 to 24a0fad Compare July 15, 2020 22:46

Snuffleupagus reviewed Jul 16, 2020

View reviewed changes

Snuffleupagus requested changes Jul 16, 2020

View reviewed changes

Snuffleupagus reviewed Jul 16, 2020

View reviewed changes

brendandahl force-pushed the oc branch from 24a0fad to 8707bbe Compare July 16, 2020 21:25

timvandermeij added the core label Jul 16, 2020

Snuffleupagus approved these changes Jul 17, 2020

View reviewed changes

timvandermeij reviewed Jul 17, 2020

View reviewed changes

brendandahl force-pushed the oc branch from 8707bbe to 9bf3bf0 Compare July 17, 2020 21:42

Snuffleupagus requested changes Jul 18, 2020

View reviewed changes

Snuffleupagus reviewed Jul 18, 2020

View reviewed changes

src/display/optional_content_config.js Outdated Show resolved Hide resolved

janpe2 reviewed Jul 19, 2020

View reviewed changes

brendandahl force-pushed the oc branch 2 times, most recently from cbec96b to 5c370a5 Compare August 3, 2020 22:39

timvandermeij reviewed Aug 3, 2020

View reviewed changes

brendandahl force-pushed the oc branch from 5c370a5 to 32623a5 Compare August 3, 2020 23:27

Snuffleupagus approved these changes Aug 4, 2020

View reviewed changes

brendandahl force-pushed the oc branch from 32623a5 to ac494a2 Compare August 4, 2020 16:27

brendandahl merged commit 9989879 into mozilla:master Aug 4, 2020

Snuffleupagus mentioned this pull request Aug 10, 2020

[api-minor] Add support for toggling of Optional Content in the viewer (issue 12096) #12170

Merged

Snuffleupagus mentioned this pull request Aug 3, 2021

Null pointer exception in optional_content_config.js when viewing PDF document #13851

Closed

Snuffleupagus mentioned this pull request Aug 24, 2021

Support Optional Content in Image-/XObjects (issue 13931) #13932

Merged

	// Only attempt to draw the glyph if it is actually in the embedded font
	// file or if there isn't a font file so the fallback font is shown.
	if (glyph.isInFont \|\| font.missingFile) {
	if (simpleFillText && !accent) {
	// common case
	ctx.fillText(character, scaledX, scaledY);
	} else {
	this.paintChar(character, scaledX, scaledY, patternTransform);
	if (accent) {
	scaledAccentX = scaledX + accent.offset.x / fontSizeScale;
	scaledAccentY = scaledY - accent.offset.y / fontSizeScale;
	this.paintChar(
	accent.fontChar,
	scaledAccentX,
	scaledAccentY,
	patternTransform
	);
	}
	}
	}

	throw new Error(
	"Cannot use the same canvas during multiple render() operations. " +
	"Use different canvas or ensure previous operations were " +
	"cancelled or completed."
	);

	internalRenderTask.initializeGraphics(transparency);
	internalRenderTask.operatorListChanged();
	})
	.catch(complete);

	const complete = error => {
	const i = intentState.renderTasks.indexOf(internalRenderTask);
	if (i >= 0) {
	intentState.renderTasks.splice(i, 1);
	}

	// Attempt to reduce memory usage during printing, by always running
	// cleanup once rendering has finished (regardless of cleanupAfterRender).
	if (this.cleanupAfterRender \|\| renderingIntent === "print") {
	this.pendingCleanup = true;
	}
	this._tryCleanup();

	if (error) {
	internalRenderTask.capability.reject(error);

	this._abortOperatorList({
	intentState,
	reason: error,
	});
	} else {
	internalRenderTask.capability.resolve();
	}
	if (this._stats) {
	this._stats.timeEnd("Rendering");
	this._stats.timeEnd("Overall");
	}
	};

		name: group.get("Name"),
		intent: group.get("Intent"), // TODO validate this.

Add support for optional marked content. #12095

Add support for optional marked content. #12095

Conversation

brendandahl commented Jul 14, 2020

brendandahl commented Jul 14, 2020

pdfjsbot commented Jul 14, 2020

From: Bot.io (Windows)

Received

pdfjsbot commented Jul 14, 2020

From: Bot.io (Linux m4)

Received

pdfjsbot commented Jul 14, 2020

From: Bot.io (Linux m4)

Failed

pdfjsbot commented Jul 14, 2020

From: Bot.io (Windows)

Failed

brendandahl commented Jul 15, 2020

pdfjsbot commented Jul 15, 2020

From: Bot.io (Windows)

Received

pdfjsbot commented Jul 15, 2020

From: Bot.io (Linux m4)

Received

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pdfjsbot commented Jul 15, 2020

From: Bot.io (Linux m4)

Failed

pdfjsbot commented Jul 15, 2020

From: Bot.io (Windows)

Failed

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Snuffleupagus Jul 16, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Snuffleupagus Jul 16, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Snuffleupagus Jul 16, 2020 • edited

Choose a reason for hiding this comment

Snuffleupagus Jul 16, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Snuffleupagus left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timvandermeij left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brendandahl commented Jul 17, 2020

pdfjsbot commented Jul 17, 2020

From: Bot.io (Linux m4)

Received

pdfjsbot commented Jul 17, 2020

From: Bot.io (Windows)

Received

pdfjsbot commented Jul 17, 2020

Snuffleupagus Jul 16, 2020 •

edited

Snuffleupagus Jul 16, 2020 •

edited

Snuffleupagus Jul 16, 2020 •

edited

Snuffleupagus Jul 16, 2020 •

edited

Snuffleupagus left a comment •

edited

timvandermeij Aug 3, 2020 •

edited

timvandermeij Aug 3, 2020 •

edited