Move svg:clipPath generation from clip to endPath #8542
Conversation
|
/botio test |
src/display/svg.js
Outdated
| @@ -264,6 +264,7 @@ var SVGExtraState = (function SVGExtraStateClosure() { | |||
| this.dependencies = []; | |||
|
|
|||
| // Clipping | |||
| this.pendingClip = null; | |||
There was a problem hiding this comment.
It does not look like you store the pending clip in the SVGExtraState object (but instead as a property of SVGGraphics), so I think this should be moved to above https://github.com/mozilla/pdf.js/pull/8542/files?w=1#diff-84f753e2bb39669e059ce3447e5a0057R365, just like in canvas.js.
src/display/svg.js
Outdated
| @@ -894,36 +896,41 @@ SVGGraphics = (function SVGGraphicsClosure() { | |||
| current.setCurrentPoint(x, y); | |||
| }, | |||
|
|
|||
| endPath: function SVGGraphics_endPath() {}, | |||
| endPath: function SVGGraphics_endPath() { | |||
| if (this.pendingClip) { | |||
There was a problem hiding this comment.
I think we should use this instead:
if (!this.pendingClip) {
return;
}That way we avoid a level of indenting, which improves readability.
In the PDF from issue 8527, the clip operator (W) shows up before a path
is defined. The current SVG backend however expects a path to exist
before generating a `<svg:clipPath>` element.
In the example, the path was defined after the clip, followed by a
endPath operator (n).
So this commit fixes the bug by moving the path generation logic from
clip to endPath.
Our canvas backend appears to use similar logic:
`CanvasGraphics_endPath` calls `consumePath`, which in turn draws the
clip and resets the `pendingClip` state. The canvas backend calls
`consumePath` from multiple other places, so we probably need to check
whether doing so is also necessary for the SVG backend.
I scanned our corpus of PDF files in test/pdfs, and found that in every
instance (except for one), the "W" PDF operator (clip) is immediately
followed by "n" (endPath). The new test from this commit (clippath.pdf)
starts with "W", followed by a path definition and then "n".
# Commands used to find some of the clipping commands:
grep -ra '^W$' -C7 | less -S
grep -ra '^W ' -C7 | less -S
grep -ra ' W$' -C7 | less -S
test/pdfs/issue6413.pdf is the only file where "W" (a tline 55) is not
followed by "n". In fact, the "W" is the last operation of a series of
XObject painting operations, and removing it does not have any effect
on the rendered PDF (confirmed by looking at the output of PDF.js's
canvas backend, and ImageMagick's convert command).
|
/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/f5beb583d30e081/output.txt |
From: Bot.io (Linux m4)SuccessFull output at http://54.67.70.0:8877/f5beb583d30e081/output.txt Total script time: 2.32 mins Published |
|
/botio test |
From: Bot.io (Linux m4)ReceivedCommand cmd_test from @timvandermeij received. Current queue size: 0 Live output at: http://54.67.70.0:8877/2f854761658bdca/output.txt |
From: Bot.io (Windows)ReceivedCommand cmd_test from @timvandermeij received. Current queue size: 0 Live output at: http://54.215.176.217:8877/b2cad8e67781cf0/output.txt |
From: Bot.io (Linux m4)SuccessFull output at http://54.67.70.0:8877/2f854761658bdca/output.txt Total script time: 16.50 mins
|
From: Bot.io (Windows)SuccessFull output at http://54.215.176.217:8877/b2cad8e67781cf0/output.txt Total script time: 30.21 mins
|
|
/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/d2dfef059244d19/output.txt |
From: Bot.io (Windows)ReceivedCommand cmd_makeref from @timvandermeij received. Current queue size: 0 Live output at: http://54.215.176.217:8877/a7cd634efb6bae3/output.txt |
From: Bot.io (Linux m4)SuccessFull output at http://54.67.70.0:8877/d2dfef059244d19/output.txt Total script time: 15.45 mins
|
From: Bot.io (Windows)SuccessFull output at http://54.215.176.217:8877/a7cd634efb6bae3/output.txt Total script time: 28.77 mins
|
|
Thank you for fixing this! It's good that the canvas and SVG back-end are more similar now. |
Move svg:clipPath generation from clip to endPath
See the commit message for a detailed explanation.
I think that we also need to check whether it is necessary to add more
.endPathcalls to src/display/svg.js, similar to the manyconsumePathcalls in src/display/canvas.js. However I don't understand that part of the code well enough to tell whether it's needed.Fixes #8527
Seems to improve #8496 (#8496 (comment))