There's quite frankly no particular reason to special-case Type3-fonts with image resources, which are very rare anyway, now that we have a general mechanism for sending/receiving images globally.
While the `CharProcs` streams of Type3-fonts *usually* don't rely on dependencies, such as e.g. images, it does happen in some cases.
Currently any dependencies are simply appended to the parent operatorList, which in practice means *only* the operatorList of the *first* page where the Type3-font is being used.
However, there's one thing that's slightly unfortunate with that approach: Since fonts are global to the PDF document, we really ought to ensure that any Type3 dependencies are appended to the operatorList of *all* pages where the Type3-font is being used. Otherwise there's a theoretical risk that, if one page has its rendering paused, another page may try to use a Type3-font whose dependencies are not yet fully resolved. In that case there would be errors, since Type3 operatorLists are executed synchronously.
Hence this patch, which ensures that all relevant pages will have Type3 dependencies appended to the main operatorList. (Note here that the `OperatorList.addDependencies` method, via `OperatorList.addDependency`, ensures that a dependency is only added *once* to any operatorList.)
Finally, these changes also remove the need for the "waiting for the main-thread"-hack that was added to `PartialEvaluator.buildPaintImageXObject` as part of fixing issue 10717.
Jasmine >= 3.6.0 causes intermittent test failures because of random
task abortions.
Puppeteer >= 4.0.0 causes ENOTEMPTY/EBUSY errors during shutdown on the
Windows bot.
Moreover, `jasmine-core` is a dependency of `jasmine` so it doesn't have
to be required separately.
This patch should *hopefully* remove the `Unhandled promise rejection: ...` errors, by returning the "final" promise. Also, by pausing/delaying of rendering slightly the likelihood of the test failing in the first place should thus be reduced.
This uses a similar format to the reference-test logging, and will help determine in *exactly* which browser the failure occurred (since the tests run concurrently).
- Given the `DefaultExternalServices` implementation, the `PDFViewerApplication.supportsDocumentFonts` getter is guaranteed to be defined and we can thus remove some (now) unnecessary `PDFJSDev` checks from the `webViewerInitialized` function.
- By slightly tweaking the "pdfBugEnabled" definition in `web/app_options`, similar to the existing ones for "workerSrc" and "cMapUrl", we can remove some `PDFJSDev` checks from the `PDFViewerApplication._parseHashParameters` method.
The current behavior for `getPagesOverview` assumes we want to only
auto-rotate if:
- `enablePrintAutoRotate` is `true`
- `isFirstPagePortrait !== isPortraitOrientation(size)`
This second check is what is breaking #9297. The two PDFs linked have a
landscape orientation first page, as well as subsequent pages. Since
`false === false`, we print portrait.
Let's drop the comparison with `isFirstPagePortrait`, and print
landscape if `!isPortraitOrientation(size)`.
Fixes#9297.
When the old `Dict.getAll()` method was removed, it was replaced with a `Dict.getKeys()` call and `Dict.get(...)` calls (in a loop).
While this pattern obviously makes a lot of sense in many cases, there's some instances where we actually want the *raw* `Dict` values (i.e. `Ref`s where applicable). In those cases, `Dict.getRaw(...)` calls are instead used within the loop. However, by introducing a new `Dict.getRawValues()` method we can reduce the number of (strictly unnecessary) function calls by simply getting the *raw* `Dict` values directly.
Using a `Map` instead of an `Object` provides some advantages such as
cheaper ways to get the size of the cache, to find out if an entry is
contained in the cache and to iterate over the cache. Moreover, we can
clear and re-use the same `Map` object now instead of creating a new
one.
Since this method calls `Dict.get` to fetch data, there could thus be `Error`s thrown in corrupt PDF documents when attempting to resolve an indirect object.
To ensure that this won't ever become a problem, we change the method to be `async` such that a rejected Promise would be returned and general OperatorList parsing won't break.
- Replace the existing loops with `for...of` variants instead.
- Make use of `continue`, to reduce indentation and to make the code (slightly) easier to follow, when checking `/Resources` entries.
This case should no longer happen, given the `instanceof Ref` branch just above (added in PR 6971).
Also, I've run the entire test-suite locally with `continue` replaced by `throw new Error(...)` and didn't find any problems.
Given that this method is used during what's essentially a *pre*-parsing stage, before the actual OperatorList parsing occurs, on second thought it doesn't seem at all necessary to warn and trigger fallback in cases where there's lookup errors.
*Please note:* Any any errors will still be either suppressed or thrown, according to the `ignoreErrors` option, during the *actual* OperatorList parsing.
It turns out that `getTextContent` suffers from *similar* problems with repeated GStates as `getOperatorList`; please see the previous patch.
While only `/ExtGState` resources containing Fonts will actually be *parsed* by `PartialEvaluator.getTextContent`, we're still forced to fetch/validate repeated `/ExtGState` resources even though *most* of them won't affect the textContent (since they mostly contain purely graphical state).
With these changes we also no longer need to immediately reset the current text-state when encountering a `setGState` operator, which may thus improve text-selection in some cases.
This patch will help pathological cases the most, with issue 2813 being a particularily problematic example. While there's only *four* `/ExtGState` resources, there's a total `29062` of `setGState` operators. Even though parsing of a single `/ExtGState` resource is quite fast, having to re-parse them thousands of times does add up quite significantly.
For simplicity we'll only cache "simple" `/ExtGState` resource, since e.g. the general `SMask` case cannot be easily cached (without re-factoring other code, which may have undesirable effects on general parsing).
By caching "simple" `/ExtGState` resource, we thus improve performance by:
- Not having to fetch/validate/parse the same `/ExtGState` data over and over.
- Handling of repeated `setGState` operators becomes *synchronous* during the `OperatorList` building, instead of having to defer to the event-loop/microtask-queue since the `/ExtGState` parsing is done asynchronously.
---
Obviously I had intended to include (standard) benchmark results with this patch, but for reasons I don't understand the test run-time (even with `master`) of the document in issue 2813 is *a lot* slower than in the development viewer (making normal benchmarking infeasible).
However, testing this manually in the development viewer (using `pdfBug=Stats`) shows a *reduction* of `~10 %` in the rendering time of the PDF document in issue 2813.
Stop special-casing the (very unlikely) "no `/XObject` found"-scenario, when parsing `OPS.paintXObject` operators, in `PartialEvaluator.{getOperatorList, getTextContent}`
Originally there weren't any (generally) good ways to handle errors gracefully, on the worker-side, however that's no longer the case and we can simply fallback to the existing `ignoreErrors` functionality instead.
Also, please note that the "no `/XObject` found"-scenario should be *extremely* unlikely in practice and would only occur in corrupt/broken documents.
Note that the `PartialEvaluator.getOperatorList` case is especially bad currently, since we'll simply (attempt to) send the data as-is to the main-thread. This is quite bad, since in a corrupt/broken document the data *could* contain anything and e.g. be unclonable (which would cause breaking errors).
Also, we're (obviously) not attempting to do anything with this "raw" `OPS.paintXObject` data on the main-thread and simply ensuring that we never send it definately seems like the correct approach.
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).
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*.
Given the dummy-methods on `DefaultExternalServices`, there's no longer any compelling reason not to (attempt to) report telemetry unconditionally.
The only larger change consists of moving the `KNOWN_VERSIONS` and `KNOWN_GENERATORS` arrays ouf of the `PDFViewerApplication._initializeMetadata` method.
*Please note:* Most of this patch consists of whitespace-only changes.
There's a few things that could be improved in the current implementation, such as:
- It's currently necessary to *both* manually track the `featureId`s which should trigger delayed fallback, as well as manually report telemetry in affected cases.
Obviously there's only two call-sites as of now (forms and javaScript), but it still feels somewhat error-prone especially if more cases were to be added in the future. To address this, this patch adds a new (private) method which abstracts away these details from the call-sites.
- Generally, it also seems nice to reduce *and* simplify the amount of state we need to store on `PDFViewerApplication` in order to support the "delayedFallback" functionality.
Also, having to *manually* work with the "delayedFallback"-array in multiple places feels cumbersome and makes e.g. the `PDFViewerApplication.fallback` method less clear as to its behaviour.
- Having code *outside* of `PDFViewerApplication`, i.e. in the event handlers, directly access properties which are marked as "private" via a leading underscore doesn't seem that great in general.
Furthermore, having the event handlers directly deal with that should be internal state also seem unfortunate. To address this, the patch will instead make use of a new `PDFViewerApplication.triggerDelayedFallback` callback.
- There's at least one code-path in the viewer, see `PDFViewerApplication.error`, where `fallback` can be called without an argument.
It's currently possible (although maybe somewhat unlikely) that such a call *could* be overridden by the `featureId` of a pending "delayedFallback" call, thus not reporting the *correct* fallback reason.
- The "delayedFallback"-state weren't being reset on document close (which shouldn't affect Firefox, but nonetheless it ought to be fixed).
