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.

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

[wip] Freetext annotation DA parsing #12093

Closed
wants to merge 28 commits into from
Closed

[wip] Freetext annotation DA parsing #12093

wants to merge 28 commits into from

Conversation

escapewindow
Copy link
Contributor

@escapewindow escapewindow commented Jul 13, 2020

Rescuing changes from #10122 .

I got here from #11637 (comment) , which I think I got to from #7613 .

@escapewindow
Copy link
Contributor Author

Reftests are currently busted. Every test failure seems to be one-digit-color off (e.g., (198, 198, 198) instead of (197, 197, 197); I'm not sure how big a deal that is?) except for the two freetext annotation tests, which makes sense.

Original issue9552.pdf:

image

Patched, broken (or at least different) in both Fx and Chromium:

image

Original annotation-freetext.pdf:

image

Patched, broken in Chromium:

image

The latter definitely looks worse. I'm going to try to figure out what broke.

@escapewindow escapewindow changed the title Freetext annotation DA parsing [wip] Freetext annotation DA parsing Jul 14, 2020
src/core/annotation.js Outdated Show resolved Hide resolved
src/core/annotation.js Outdated Show resolved Hide resolved
src/core/annotation.js Outdated Show resolved Hide resolved
src/core/annotation.js Outdated Show resolved Hide resolved
src/display/annotation_layer.js Outdated Show resolved Hide resolved
src/display/annotation_layer.js Outdated Show resolved Hide resolved
src/display/annotation_layer.js Outdated Show resolved Hide resolved
src/display/annotation_layer.js Outdated Show resolved Hide resolved
src/display/annotation_layer.js Outdated Show resolved Hide resolved
test/test_manifest.json Show resolved Hide resolved

const opList = new OperatorList();
const appearanceStream = new Stream(stringToBytes(data.defaultAppearance));
const resourcesFonts = this.pdfManager.pdfDocument.catalog.fontCache.dict;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Unfortunately #10122 (comment) still applies here, there's simply no way that this works correctly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried resolving this in e9ed30c . Does that look sane?

@escapewindow
Copy link
Contributor Author

Hm, but I lost the freetext annotations in issue10122.pdf in b20af4a .

src/core/annotation.js Outdated Show resolved Hide resolved
})
.then(() => {
const args = opList.argsArray;
for (let i = 0, ii = opList.fnArray.length; i < ii; i++) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Since the OperatorList has a length getter, it should be possible to simply use:

Suggested change
for (let i = 0, ii = opList.fnArray.length; i < ii; i++) {
for (let i = 0, ii = opList.length; i < ii; i++) {

Copy link
Contributor Author

@escapewindow escapewindow Jul 21, 2020

Choose a reason for hiding this comment

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

OperatorList.length appears to return this.argsArray.length instead of this.fnArray.length. Are they always the same length?

@Snuffleupagus

src/core/annotation.js Outdated Show resolved Hide resolved
src/display/annotation_layer.js Outdated Show resolved Hide resolved
src/display/annotation_layer.js Outdated Show resolved Hide resolved
@escapewindow
Copy link
Contributor Author

escapewindow commented Jul 22, 2020

Even if changes in the reference tests are minor, please make sure that you can explain why they happen.
(Unexpected changes, without further investigation, could maybe point to something being wrong.)

This will likely take some time. For instance, in chrome-tracemonkey-text-page7, there's a single pixel that's now b7ffb7 instead of b8ffb8... similar small differences changed 24 different reftests. Has this type of single-pixel difference occurred before? Is there something I should look for, or do you have a recommended way of debugging?

Edit: found https://github.com/mozilla/pdf.js/wiki/Debugging-PDF.js

@timvandermeij
Copy link
Contributor

timvandermeij commented Jul 22, 2020

Oh, there are currently some regression test failures in Chrome due to intermittent rendering differences in the Chrome engine. If it's only in Chrome and not in Firefox, it's most likely not a concern (at least, not caused by your code).

For example, look at the test output comments at #12059. The patch does not contain functional changes, only refactoring, but in Chrome some tests fail because of this rendering engine problem since the last few Chrome versions.

@escapewindow
Copy link
Contributor Author

Ah, yes, the differences are all Chrome. Thanks! That saved me a lot of time.
I still need to debug why removing the div removed the annotations in b20af4a , and address this comment. Getting closer :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants