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

Jump to bottom

Reset all the canvas states after rendering each annotations (#14105) #14998

Merged
merged 1 commit into from
Jun 7, 2022
Merged

Reset all the canvas states after rendering each annotations (#14105) #14998

merged 1 commit into from
Jun 7, 2022

Conversation

calixteman
Copy link
Contributor

@calixteman calixteman commented Jun 6, 2022 •
edited
Loading

  • each annotation must be rendered independently of the others. So
    after having rendered each annotation, the canvas states are reset
    in order to have something clean to render the next one.

@calixteman calixteman requested a review from Snuffleupagus June 6, 2022 15:25
@Snuffleupagus
Copy link
Collaborator

It looks this patch causes Warning: Tried to restore a ctx when the stack was already empty. to be printed in the console for every single page that contain Annotations, which seems a little annoying.

Furthermore, the endDrawing-method was probably implemented to only be called at very the end of drawing, so is there a risk that something breaks if we now call it twice per page?
For example: While we don't really support the imageLayer-parameter, we should probably not call its endLayout method until the actual last endDrawing-call.

@Snuffleupagus Snuffleupagus linked an issue Jun 6, 2022 that may be closed by this pull request
stamps are not rendered correctly #14105
Closed
Snuffleupagus
Snuffleupagus approved these changes Jun 6, 2022
View reviewed changes
Copy link
Collaborator

@Snuffleupagus Snuffleupagus left a comment

Choose a reason for hiding this comment

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

r=me, with passing tests; thank you!

@Snuffleupagus
Copy link
Collaborator

/botio test

@pdfjsbot
Copy link

pdfjsbot commented Jun 6, 2022

From: Bot.io (Windows)

Received

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

Live output at: http://54.193.163.58:8877/ec27d7cb7fd42b5/output.txt

@pdfjsbot
Copy link

pdfjsbot commented Jun 6, 2022

From: Bot.io (Linux m4)

Received

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

Live output at: http://54.241.84.105:8877/3485c74107ec080/output.txt

@pdfjsbot
Copy link

pdfjsbot commented Jun 6, 2022

From: Bot.io (Linux m4)

Failed

Full output at http://54.241.84.105:8877/3485c74107ec080/output.txt

Total script time: 4.79 mins

  • Font tests: Passed
  • Unit tests: FAILED
  • Integration Tests: Passed
  • Regression tests: FAILED

Image differences available at: http://54.241.84.105:8877/3485c74107ec080/reftest-analyzer.html#web=eq.log

@pdfjsbot
Copy link

pdfjsbot commented Jun 6, 2022

From: Bot.io (Windows)

Failed

Full output at http://54.193.163.58:8877/ec27d7cb7fd42b5/output.txt

Total script time: 7.89 mins

  • Font tests: Passed
  • Unit tests: FAILED
  • Integration Tests: FAILED
  • Regression tests: FAILED

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

@calixteman
Copy link
Contributor Author

/botio test

@pdfjsbot
Copy link

pdfjsbot commented Jun 6, 2022

From: Bot.io (Linux m4)

Received

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

Live output at: http://54.241.84.105:8877/b781c989e4427d1/output.txt

@pdfjsbot
Copy link

pdfjsbot commented Jun 6, 2022

From: Bot.io (Windows)

Received

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

Live output at: http://54.193.163.58:8877/ab5480db3b06a3f/output.txt

@mozilla mozilla deleted a comment from pdfjsbot Jun 6, 2022
@mozilla mozilla deleted a comment from pdfjsbot Jun 6, 2022
@calixteman
Copy link
Contributor Author

/botio test

@pdfjsbot
Copy link

pdfjsbot commented Jun 6, 2022

From: Bot.io (Linux m4)

Received

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

Live output at: http://54.241.84.105:8877/e633b6d31d6b8ed/output.txt

@pdfjsbot
Copy link

pdfjsbot commented Jun 6, 2022

From: Bot.io (Windows)

Received

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

Live output at: http://54.193.163.58:8877/d605760d898d0aa/output.txt

@pdfjsbot
Copy link

pdfjsbot commented Jun 6, 2022

From: Bot.io (Linux m4)

Failed

Full output at http://54.241.84.105:8877/e633b6d31d6b8ed/output.txt

Total script time: 26.57 mins

  • Font tests: Passed
  • Unit tests: FAILED
  • Integration Tests: Passed
  • Regression tests: FAILED
  different ref/snapshot: 30
  different first/second rendering: 1

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

@pdfjsbot
Copy link

pdfjsbot commented Jun 6, 2022

From: Bot.io (Windows)

Failed

Full output at http://54.193.163.58:8877/d605760d898d0aa/output.txt

Total script time: 27.99 mins

  • Font tests: Passed
  • Unit tests: Passed
  • Integration Tests: FAILED
  • Regression tests: FAILED
  different ref/snapshot: 3

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

@calixteman
Copy link
Contributor Author

issue8047 is wrong, I should probably apply the global transform.

@calixteman
Copy link
Contributor Author

So issue8047 was failing because there is an extra wrong Q command in the On state appearance:

BT
/26EBF91525C1400B96AF9B0E30E84747 8.74 Tf
2.853 2.08489006823351 Td
(4) Tj
ET
Q

After thinking about, I think the correct way to fix that stuff is to reset the states for each annotation: the rendering of an annotation is independent of the others.

@calixteman calixteman changed the title Reset all the canvas states before rendering the annotations (#14105) Reset all the canvas states after rendering each annotations (#14105) Jun 7, 2022
@Snuffleupagus
Copy link
Collaborator

Another question (possibly stupid on my part), looking at the latest version of the patch where you reset for every Annotation:

Would the code possibly become a tiny bit simpler if we place this code at the start of the beginAnnotation-method, rather than having that in both of the beginAnnotations and endAnnotation methods?

@calixteman
Copy link
Contributor Author

I've the feeling that the "restore" code should be in endFoo, hence the one in beginAnnotations should likely be in a endPageDrawing but it can be a matter of taste.
I tried your suggestion, it works well and it avoids a bit of redundancy.
What do you prefer ?

@Snuffleupagus
Copy link
Collaborator

Snuffleupagus commented Jun 7, 2022 •
edited
Loading

I've the feeling that the "restore" code should be in endFoo, hence the one in beginAnnotations should likely be in a endPageDrawing but it can be a matter of taste.

That'd essentially require introducing a new custom operator, since we currently don't have a specific operator to signal "end of opList", which I'd generally suggest that we don't do unless absolutely necessary.

I tried your suggestion, it works well and it avoids a bit of redundancy.
What do you prefer ?

Indeed I thought that less code would be nice, and having it at the start of each individual beginAnnotation-call makes it very clear what's happening. Hence this option would be my preference :-)

@calixteman
Reset all the canvas states after rendering each annotations (mozilla…
96d0d22 
…#14105)

- each annotation must be rendered independently of the others. So
  after having rendered each annotation, the canvas states are reset
  in order to have something clean to render the next one.
@calixteman
Copy link
Contributor Author

/botio test

@pdfjsbot
Copy link

pdfjsbot commented Jun 7, 2022

From: Bot.io (Linux m4)

Received

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

Live output at: http://54.241.84.105:8877/a4cbf86dd13ba83/output.txt

@pdfjsbot
Copy link

pdfjsbot commented Jun 7, 2022

From: Bot.io (Windows)

Received

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

Live output at: http://54.193.163.58:8877/34559bfc11cfb19/output.txt

@pdfjsbot
Copy link

pdfjsbot commented Jun 7, 2022

From: Bot.io (Linux m4)

Failed

Full output at http://54.241.84.105:8877/a4cbf86dd13ba83/output.txt

Total script time: 26.12 mins

  • Font tests: Passed
  • Unit tests: FAILED
  • Integration Tests: Passed
  • Regression tests: FAILED
  different ref/snapshot: 25
  different first/second rendering: 1

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

@pdfjsbot
Copy link

pdfjsbot commented Jun 7, 2022

From: Bot.io (Windows)

Failed

Full output at http://54.193.163.58:8877/34559bfc11cfb19/output.txt

Total script time: 29.44 mins

  • Font tests: Passed
  • Unit tests: Passed
  • Integration Tests: FAILED
  • Regression tests: FAILED
  different ref/snapshot: 2

Image differences available at: http://54.193.163.58:8877/34559bfc11cfb19/reftest-analyzer.html#web=eq.log

@calixteman calixteman requested a review from Snuffleupagus June 7, 2022 14:03
@Snuffleupagus Snuffleupagus merged commit c25429b into mozilla:master Jun 7, 2022
@Snuffleupagus
Copy link
Collaborator

/botio makeref

@pdfjsbot
Copy link

pdfjsbot commented Jun 7, 2022

From: Bot.io (Linux m4)

Received

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

Live output at: http://54.241.84.105:8877/cca759dd088b604/output.txt

@pdfjsbot
Copy link

pdfjsbot commented Jun 7, 2022

From: Bot.io (Windows)

Received

Command cmd_makeref from @Snuffleupagus received. Current queue size: 1

Live output at: http://54.193.163.58:8877/9d1ee73d15c4986/output.txt

@pdfjsbot
Copy link

pdfjsbot commented Jun 7, 2022

From: Bot.io (Linux m4)

Success

Full output at http://54.241.84.105:8877/cca759dd088b604/output.txt

Total script time: 22.65 mins

  • Lint: Passed
  • Make references: Passed
  • Check references: Passed

@pdfjsbot
Copy link

pdfjsbot commented Jun 7, 2022

From: Bot.io (Windows)

Success

Full output at http://54.193.163.58:8877/9d1ee73d15c4986/output.txt

Total script time: 22.13 mins

  • Lint: Passed
  • Make references: Passed
  • Check references: Passed

@Snuffleupagus Snuffleupagus mentioned this pull request Jun 11, 2022
Merged
@Snuffleupagus Snuffleupagus linked an issue Jun 11, 2022 that may be closed by this pull request
Text appears inverted and is misplaced #9461
Closed
@ljhaust
Copy link

ljhaust commented May 25, 2023

Can you tell me why it needs to be modified like this? Is it because this PDF doesn't comply with Adobe's specifications, so it requires these adjustments and modifications @calixteman

@ZeroXClem ZeroXClem mentioned this pull request Aug 12, 2024
Closed
@earthywh earthywh mentioned this pull request Sep 24, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Snuffleupagus Snuffleupagus Snuffleupagus approved these changes

Assignees
No one assigned
Labels
annotations
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

stamps are not rendered correctly Text appears inverted and is misplaced
4 participants
@calixteman @Snuffleupagus @pdfjsbot @ljhaust