canvas: Properly restore all the remaining items in stateStack in endDrawing #12363
Conversation
From: Bot.io (Linux m4)ReceivedCommand cmd_test from @Snuffleupagus received. Current queue size: 0 Live output at: http://54.67.70.0:8877/3d5540d0a448259/output.txt |
From: Bot.io (Windows)ReceivedCommand cmd_test from @Snuffleupagus received. Current queue size: 0 Live output at: http://54.215.176.217:8877/a5d801e91e7441f/output.txt |
From: Bot.io (Linux m4)FailedFull output at http://54.67.70.0:8877/3d5540d0a448259/output.txt Total script time: 21.72 mins
Image differences available at: http://54.67.70.0:8877/3d5540d0a448259/reftest-analyzer.html#web=eq.log |
From: Bot.io (Windows)FailedFull output at http://54.215.176.217:8877/a5d801e91e7441f/output.txt Total script time: 24.42 mins
Image differences available at: http://54.215.176.217:8877/a5d801e91e7441f/reftest-analyzer.html#web=eq.log |
|
Unfortunately, as can be seen above, this causes a number of outright regressions in existing reference tests. Please see https://github.com/mozilla/pdf.js/wiki/Contributing#4-run-lint-and-testing for information about running tests locally. |
|
Ah, thank you! I naively just ran It seems we reset to default also to draw annotations, so that change is probably not quite correct... will look into the failures. Thanks @Snuffleupagus! |
|
(And I can try to add a test for this too :P) |
|
@Snuffleupagus can you run automation on the current PR? I'm running |
|
Please remove the unwanted commit properly, using something like
Let's try once more; but if there's still issues you'll unfortunately have to bite the bullet and run tests locally, since developing using the bots aren't exactly practical.
That's an unfortunate side-effect of not being able, for copyright reasons, to include all test PDF files in the repository. (Often times, creating reduced test-cases are unfortunately not that simple.) |
From: Bot.io (Linux m4)ReceivedCommand cmd_test from @Snuffleupagus received. Current queue size: 0 Live output at: http://54.67.70.0:8877/ab7e9da1268f368/output.txt |
From: Bot.io (Windows)ReceivedCommand cmd_test from @Snuffleupagus received. Current queue size: 0 Live output at: http://54.215.176.217:8877/ab82944e55017cc/output.txt |
From: Bot.io (Linux m4)FailedFull output at http://54.67.70.0:8877/ab7e9da1268f368/output.txt Total script time: 21.48 mins
Image differences available at: http://54.67.70.0:8877/ab7e9da1268f368/reftest-analyzer.html#web=eq.log |
From: Bot.io (Windows)FailedFull output at http://54.215.176.217:8877/ab82944e55017cc/output.txt Total script time: 23.31 mins
Image differences available at: http://54.215.176.217:8877/ab82944e55017cc/reftest-analyzer.html#web=eq.log |
|
@emilio If you're still having trouble downloading the PDF files for testing, send me an email (the address is in my Github profile) and I can provide all of them through a private Dropbox link. |
…Drawing. We were correctly finishing the SMask group but not restoring all the extra transformations applied in stateStack, so if somebody ends up drawing to the same context after canceling mid-draw we'd get artifacts. This fixes Mozilla bug 1664178[1]. [1]: https://bugzilla.mozilla.org/show_bug.cgi?id=1664178
emilio
force-pushed
the
better-cancelation
branch
from
464f3dd
to
ef1e9a1
Compare
Yup, I just didn't want to make destructive changes without knowing if that commit was the cuplrit, done now :)
Yeah, I understand, thanks!
I see, that is indeed unfortunate... Anyhow I left my laptop downloading yesterday and now I have all the test files locally :) The image differences above all seem like minor subpixel differences, unless I'm missing something? Which are not totally unexpected, I guess, as they are from tests that use this cancellation machinery which was leaving some existing state before this patch. Do you know how should I update the references? Or I guess that happens automatically after merging the change anyways... |
|
I have checked the unit test and reference test failures. You're right that your patch doesn't cause any regressions anymore and these unit/reference test failures are unfortunately intermittent and being dealt with in #12123, so for this patch it looks good. Updating references is done by our bot on command when we merge the PR, so you don't have to do anything for that. |
timvandermeij
changed the title
|
/botio-linux preview |
From: Bot.io (Linux m4)ReceivedCommand cmd_preview from @timvandermeij received. Current queue size: 0 Live output at: http://54.67.70.0:8877/fea874aa1a7268e/output.txt |
From: Bot.io (Linux m4)SuccessFull output at http://54.67.70.0:8877/fea874aa1a7268e/output.txt Total script time: 3.53 mins Published |
|
Looks good. Thanks! /botio makeref |
From: Bot.io (Linux m4)ReceivedCommand cmd_makeref from @timvandermeij received. Current queue size: 0 Live output at: http://54.67.70.0:8877/18f4740e7bac01c/output.txt |
From: Bot.io (Windows)ReceivedCommand cmd_makeref from @timvandermeij received. Current queue size: 1 Live output at: http://54.215.176.217:8877/dabf632c20c22e0/output.txt |
From: Bot.io (Linux m4)FailedFull output at http://54.67.70.0:8877/18f4740e7bac01c/output.txt Total script time: 19.91 mins
|
From: Bot.io (Windows)FailedFull output at http://54.215.176.217:8877/dabf632c20c22e0/output.txt Total script time: 21.59 mins
|
|
Hm, not sure why this didn't fail in the test run, but https://github.com/mozilla/pdf.js/blob/master/test/pdfs/bug852992_reduced.pdf now shows |
|
Thanks, looking. |
…stateStack is empty. This fixes the issue that caused mozilla#12363 to get reverted, see mozilla#12367. When we end the SMask group and stateStack.length is zero, nothing updates this.current to reflect it.
…Drawing. We were correctly finishing the SMask group but not restoring all the extra transformations applied in stateStack, so if somebody ends up drawing to the same context after canceling mid-draw we'd get artifacts. This re-lands mozilla#12363 and fixes Mozilla bug 1664178[1]. [1]: https://bugzilla.mozilla.org/show_bug.cgi?id=1664178
canvas: fix restore() with existing SMask groups and re-land #12363.
We were correctly finishing the SMask group but not restoring all the extra transformations applied in stateStack, so if somebody ends up drawing to the same context after canceling mid-draw we'd get artifacts.
This fixes Mozilla bug 16641781.