You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@pdfbox.apache.org by GitBox <gi...@apache.org> on 2021/08/07 00:41:08 UTC
[GitHub] [pdfbox] sschwieb opened a new pull request #127: Lazier clipping
sschwieb opened a new pull request #127:
URL: https://github.com/apache/pdfbox/pull/127
Calculating the intersection of two `Area` can take a lot of time. However, depending on the `Graphics2D` that is used for rendering, it may not be necessary to actually perform this operation.
For instance, when generating an SVG, the individual clipping paths can be serialized individually, and the intersection is then calculated at runtime, when the SVG file is rendered.
The idea of this PR is to replace `PDGraphicsState.clippingPath` with a list of `GeneralPath`s, which is lazily evaluated, truncated & cached when `getCurrentClippingPath()` is called (effectively leaving the current behaviour of PdfBox unchanged, and it should also not have any significant impact on the performance).
Additionally, a new method `protected void transferClip(PDGraphicsState graphicsState, Graphics2D graphics)` is added to `PageDrawer`. By default, this method makes use of `getCurrentClippingPath()` to call `graphics.setClip(...)`, which again is what PdfBox currently does.
However, classes that extend `PageDrawer` can override this method, and directly access the individual clipping paths, without any need to calculate their intersection.
In some cases (shading fills & transparency groups), it is still necessary to calculate the intersection.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@pdfbox.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@pdfbox.apache.org
For additional commands, e-mail: dev-help@pdfbox.apache.org
[GitHub] [pdfbox] sschwieb edited a comment on pull request #127: Lazier clipping
Posted by GitBox <gi...@apache.org>.
sschwieb edited a comment on pull request #127:
URL: https://github.com/apache/pdfbox/pull/127#issuecomment-894871845
I did some testing, with rendering the PDFs linked in [PDFBOX-4718](https://issues.apache.org/jira/browse/PDFBOX-4718) and [PDFBOX-4743](https://issues.apache.org/jira/browse/PDFBOX-4743) via the `PDFDebugger`.
By overriding `transferClip` as
```
protected void transferClip(PDGraphicsState graphicsState, Graphics2D graphics) {
List<GeneralPath> paths = graphicsState.getCurrentClippingPaths();
boolean allDone = true;
for (GeneralPath path : paths) {
if (!path.getPathIterator(null).isDone()) {
allDone = false;
break;
}
}
if (allDone)
{
// PDFBOX-4821: avoid bug with java printing that empty clipping path is ignored by
// replacing with empty rectangle, works because this is not an empty path
graphics.setClip(new Rectangle());
return;
}
GeneralPath first = paths.get(0);
graphics.setClip(first);
for (int i = 1; i < paths.size(); i++)
{
graphics.clip(paths.get(i));
}
}
```
, the OOM that occurred in the 1st PDF disappears, and the 2nd PDF is rendered much faster (still needs several seconds though). It seems that `SunGraphics2D.clip()` is doing a much better job than `Area.intersect()`. If it turns out that this is always the case, maybe the remaining calls to `getCurrentClippingPath()` could be refactored as well - but I haven't looked into that yet, can't tell.
DYT it's worth to refactor `PageDrawer` and `PDGraphicsState`, to allow users to customize how the clip is transferred to the graphics?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@pdfbox.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@pdfbox.apache.org
For additional commands, e-mail: dev-help@pdfbox.apache.org
[GitHub] [pdfbox] lehmi commented on a change in pull request #127: Lazier clipping
Posted by GitBox <gi...@apache.org>.
lehmi commented on a change in pull request #127:
URL: https://github.com/apache/pdfbox/pull/127#discussion_r685687060
##########
File path: pdfbox/src/main/java/org/apache/pdfbox/pdmodel/graphics/state/PDGraphicsState.java
##########
@@ -601,7 +612,30 @@ public void intersectClippingPath(Area area)
*/
public Area getCurrentClippingPath()
{
- return clippingPath;
+ if (clippingPaths.size() == 1)
+ {
+ // If there is just a single clipping path, no intersections are needed.
+ GeneralPath path = clippingPaths.get(0);
+ return clippingCache.computeIfAbsent(path, Area::new);
+ }
+ // If there are multiple clipping paths, combine them to a single area.
+ Area clippingArea = new Area();
+ clippingArea.add(new Area(clippingPaths.get(0)));
+ for (int i = 1; i < clippingPaths.size(); i++)
+ {
+ clippingArea.intersect(new Area(clippingPaths.get(i)));
+ }
+ // Replace the list of individual clipping paths with the intersection, and add it to the cache.
+ GeneralPath newPath = new GeneralPath(clippingArea);
+ clippingPaths = new ArrayList<>();
+ clippingPaths.add(newPath);
+ clippingCache.put(newPath, clippingArea);
+ return clippingArea;
+ }
+
+ public List<GeneralPath> getCurrentClippingPaths()
+ {
+ return clippingPaths;
Review comment:
Maybe it is a good idea to return an unmodifiable version of that list so that it can't be modified from outside?
`return Collections.unmodifiableList(clippingPaths)`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@pdfbox.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@pdfbox.apache.org
For additional commands, e-mail: dev-help@pdfbox.apache.org
[GitHub] [pdfbox] sschwieb commented on pull request #127: Lazier clipping
Posted by GitBox <gi...@apache.org>.
sschwieb commented on pull request #127:
URL: https://github.com/apache/pdfbox/pull/127#issuecomment-896501439
Glad to hear it worked! And thanks a lot for looking into this. 😄
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@pdfbox.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@pdfbox.apache.org
For additional commands, e-mail: dev-help@pdfbox.apache.org
[GitHub] [pdfbox] sschwieb commented on a change in pull request #127: Lazier clipping
Posted by GitBox <gi...@apache.org>.
sschwieb commented on a change in pull request #127:
URL: https://github.com/apache/pdfbox/pull/127#discussion_r684560995
##########
File path: pdfbox/src/main/java/org/apache/pdfbox/rendering/PageDrawer.java
##########
@@ -382,25 +382,30 @@ else if (!(colorSpace instanceof PDPattern))
*/
protected final void setClip()
{
- Area clippingPath = getGraphicsState().getCurrentClippingPath();
- if (clippingPath != lastClip)
+ List<GeneralPath> clippingPaths = getGraphicsState().getCurrentClippingPaths();
+ if (clippingPaths != lastClips)
{
- if (clippingPath.getPathIterator(null).isDone())
- {
- // PDFBOX-4821: avoid bug with java printing that empty clipping path is ignored by
- // replacing with empty rectangle, works because this is not an empty path
- graphics.setClip(new Rectangle());
- }
- else
- {
- graphics.setClip(clippingPath);
- }
+ transferClip(getGraphicsState(), graphics);
if (initialClip != null)
{
// apply the remembered initial clip, but transform it first
//TODO see PDFBOX-4583
}
- lastClip = clippingPath;
+ lastClips = clippingPaths;
+ }
+ }
+
+ protected void transferClip(PDGraphicsState graphicsState, Graphics2D graphics) {
+ Area clippingPath = graphicsState.getCurrentClippingPath();
Review comment:
Subclasses can override this method and decide what to do by looking at `getCurrentClippingPaths()`. For instance, they could ignore the initial clipping path (page boundaries) if this seems reasonable.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@pdfbox.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@pdfbox.apache.org
For additional commands, e-mail: dev-help@pdfbox.apache.org
[GitHub] [pdfbox] THausherr edited a comment on pull request #127: Lazier clipping
Posted by GitBox <gi...@apache.org>.
THausherr edited a comment on pull request #127:
URL: https://github.com/apache/pdfbox/pull/127#issuecomment-898886126
Here the benchmarks (not very reliable):
old: 287, 344, 328
changes, old transfer: 347, 298, 269, 282, 305
new transfer: 402, 444, 437
So your initial changes may have made it faster (if my previous comment is correct), but the change in `transfer` made it slower. Thus I'll commit your main change but not the improvement.
There were no problems with the rendering differences. PDFBOX-1094-096213-p18.pdf was improved at 96 dpi but this is a pattern, so small differences in the pattern sometimes have a big effect for the area. (No differences at higher dpi)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@pdfbox.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@pdfbox.apache.org
For additional commands, e-mail: dev-help@pdfbox.apache.org
[GitHub] [pdfbox] sschwieb commented on pull request #127: Lazier clipping
Posted by GitBox <gi...@apache.org>.
sschwieb commented on pull request #127:
URL: https://github.com/apache/pdfbox/pull/127#issuecomment-898961574
> So your initial changes may have made it faster (if my previous comment is correct), but the change in transfer made it slower. Thus I'll commit your main change but not the improvement.
Yes please, and apologies for not explaining well. I did not want to suggest the snippet in my [comment](https://github.com/apache/pdfbox/pull/127#issuecomment-894871845) as the new default, I just wanted to show how this _could_ be changed if the use case requires/allows it.
> Thank you for your contribution. I'd like to do two more changes:
>
> pass only the graphics device in the transfer method to make the parameter list smaller
>
> use the Path2D abstract class instead of Path2D.Double on the left side of the assignments, and as list / map elements.
Thanks, that both sounds good to me!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@pdfbox.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@pdfbox.apache.org
For additional commands, e-mail: dev-help@pdfbox.apache.org
[GitHub] [pdfbox] sschwieb commented on pull request #127: Lazier clipping
Posted by GitBox <gi...@apache.org>.
sschwieb commented on pull request #127:
URL: https://github.com/apache/pdfbox/pull/127#issuecomment-898964813
> What is the cache for? To save the time to call "new Area(path)" ?
Oh sorry, I missed that question. But yes, that was the motivation.
The `Area` constructor calls `AreaOp.calculate`, which calls `AreaOp.pruneEdges`, which can be very slow (and even cause that OOM error). So the idea was to optimise for that.
But feel free to ignore the cache - I could do some benchmarking with & without it, to figure out how big the impact really is (and create a separate PR if it turns out to make a difference).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@pdfbox.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@pdfbox.apache.org
For additional commands, e-mail: dev-help@pdfbox.apache.org
[GitHub] [pdfbox] THausherr commented on pull request #127: Lazier clipping
Posted by GitBox <gi...@apache.org>.
THausherr commented on pull request #127:
URL: https://github.com/apache/pdfbox/pull/127#issuecomment-898889067
https://issues.apache.org/jira/browse/PDFBOX-5258
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@pdfbox.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@pdfbox.apache.org
For additional commands, e-mail: dev-help@pdfbox.apache.org
[GitHub] [pdfbox] sschwieb commented on a change in pull request #127: Lazier clipping
Posted by GitBox <gi...@apache.org>.
sschwieb commented on a change in pull request #127:
URL: https://github.com/apache/pdfbox/pull/127#discussion_r684561085
##########
File path: pdfbox/src/main/java/org/apache/pdfbox/pdmodel/graphics/state/PDGraphicsState.java
##########
@@ -39,7 +44,8 @@
public class PDGraphicsState implements Cloneable
{
private boolean isClippingPathDirty;
- private Area clippingPath;
+ private List<GeneralPath> clippingPaths = new ArrayList<>();
+ private Map<GeneralPath, Area> clippingCache = new IdentityHashMap<>();
Review comment:
I'm not sure is it too naive to have an unbound cache here, but it could easily be improved/size-limited.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@pdfbox.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@pdfbox.apache.org
For additional commands, e-mail: dev-help@pdfbox.apache.org
[GitHub] [pdfbox] asfgit closed pull request #127: Lazier clipping
Posted by GitBox <gi...@apache.org>.
asfgit closed pull request #127:
URL: https://github.com/apache/pdfbox/pull/127
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@pdfbox.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@pdfbox.apache.org
For additional commands, e-mail: dev-help@pdfbox.apache.org
[GitHub] [pdfbox] sschwieb commented on a change in pull request #127: Lazier clipping
Posted by GitBox <gi...@apache.org>.
sschwieb commented on a change in pull request #127:
URL: https://github.com/apache/pdfbox/pull/127#discussion_r684561346
##########
File path: pdfbox/src/main/java/org/apache/pdfbox/rendering/PageDrawer.java
##########
@@ -1602,9 +1607,9 @@ private TransparencyGroup(PDTransparencyGroup form, boolean isSoftMask, Matrix c
GeneralPath transformedBox = form.getBBox().transform(transform);
// clip the bbox to prevent giant bboxes from consuming all memory
- Area clip = (Area)getGraphicsState().getCurrentClippingPath().clone();
- clip.intersect(new Area(transformedBox));
- Rectangle2D clipRect = clip.getBounds2D();
+ Area transformed = new Area(transformedBox);
+ transformed.intersect(getGraphicsState().getCurrentClippingPath());
+ Rectangle2D clipRect = transformed.getBounds2D();
Review comment:
Unrelated, but I noticed that the call to `clone()` is not necessary if the areas are swapped.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@pdfbox.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@pdfbox.apache.org
For additional commands, e-mail: dev-help@pdfbox.apache.org
[GitHub] [pdfbox] sschwieb commented on pull request #127: Lazier clipping
Posted by GitBox <gi...@apache.org>.
sschwieb commented on pull request #127:
URL: https://github.com/apache/pdfbox/pull/127#issuecomment-896412506
> The file from mozilla/pdf.js#6961 has a slight loss in quality, even at 400%. Shouldn't the results be identical?
Good catch, thanks! 😄
They should be identical - it looks like the float-precision of `GeneralPath` vs the previous double-precision of `Area` causes the quality loss. I'll fix that & rebase onto the most recent trunk.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@pdfbox.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@pdfbox.apache.org
For additional commands, e-mail: dev-help@pdfbox.apache.org
[GitHub] [pdfbox] THausherr commented on pull request #127: Lazier clipping
Posted by GitBox <gi...@apache.org>.
THausherr commented on pull request #127:
URL: https://github.com/apache/pdfbox/pull/127#issuecomment-897856798
What is the cache for? To save the time to call "new Area(path)" ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@pdfbox.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@pdfbox.apache.org
For additional commands, e-mail: dev-help@pdfbox.apache.org
[GitHub] [pdfbox] THausherr commented on pull request #127: Lazier clipping
Posted by GitBox <gi...@apache.org>.
THausherr commented on pull request #127:
URL: https://github.com/apache/pdfbox/pull/127#issuecomment-899012893
Thank you. I thought about the cache - it is needed because it approaches what the old code did: the old code kept the final clipping intersection results in cases where it was used twice. The modified transferClip method does not cache the final clipping.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@pdfbox.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@pdfbox.apache.org
For additional commands, e-mail: dev-help@pdfbox.apache.org
[GitHub] [pdfbox] sschwieb commented on a change in pull request #127: Lazier clipping
Posted by GitBox <gi...@apache.org>.
sschwieb commented on a change in pull request #127:
URL: https://github.com/apache/pdfbox/pull/127#discussion_r686419849
##########
File path: pdfbox/src/main/java/org/apache/pdfbox/pdmodel/graphics/state/PDGraphicsState.java
##########
@@ -601,7 +612,30 @@ public void intersectClippingPath(Area area)
*/
public Area getCurrentClippingPath()
{
- return clippingPath;
+ if (clippingPaths.size() == 1)
+ {
+ // If there is just a single clipping path, no intersections are needed.
+ GeneralPath path = clippingPaths.get(0);
+ return clippingCache.computeIfAbsent(path, Area::new);
+ }
+ // If there are multiple clipping paths, combine them to a single area.
+ Area clippingArea = new Area();
+ clippingArea.add(new Area(clippingPaths.get(0)));
+ for (int i = 1; i < clippingPaths.size(); i++)
+ {
+ clippingArea.intersect(new Area(clippingPaths.get(i)));
+ }
+ // Replace the list of individual clipping paths with the intersection, and add it to the cache.
+ GeneralPath newPath = new GeneralPath(clippingArea);
+ clippingPaths = new ArrayList<>();
+ clippingPaths.add(newPath);
+ clippingCache.put(newPath, clippingArea);
+ return clippingArea;
+ }
+
+ public List<GeneralPath> getCurrentClippingPaths()
+ {
+ return clippingPaths;
Review comment:
In that case, the check in [PageDrawer.setClip](https://github.com/apache/pdfbox/pull/127/files#diff-66f2c150ea226006468292d4bfc54515e251b5b8e50d15450675a774bc70144eR386) would always return `false`, and break the optimisation.
It would also only prevent modifications on the list itself, but the returned paths would still be mutable. What do you think about a warning in the JavaDoc, similar to how it is currently done with `getCurrentClippingPath`?
Something like
```
This will get the current clipping paths. Do not modify the list or individual elements in it!
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@pdfbox.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@pdfbox.apache.org
For additional commands, e-mail: dev-help@pdfbox.apache.org
[GitHub] [pdfbox] THausherr commented on pull request #127: Lazier clipping
Posted by GitBox <gi...@apache.org>.
THausherr commented on pull request #127:
URL: https://github.com/apache/pdfbox/pull/127#issuecomment-896490864
Thanks that worked!
I also tested replacing transferClip, this does make 500 different renderings but that isn't surprising. I need to review these individually (it's not as long as it sounds but still needs time). I also need to compare the time before / after your changes, I forgot that.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@pdfbox.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@pdfbox.apache.org
For additional commands, e-mail: dev-help@pdfbox.apache.org
[GitHub] [pdfbox] lehmi commented on a change in pull request #127: Lazier clipping
Posted by GitBox <gi...@apache.org>.
lehmi commented on a change in pull request #127:
URL: https://github.com/apache/pdfbox/pull/127#discussion_r686528083
##########
File path: pdfbox/src/main/java/org/apache/pdfbox/pdmodel/graphics/state/PDGraphicsState.java
##########
@@ -601,7 +612,30 @@ public void intersectClippingPath(Area area)
*/
public Area getCurrentClippingPath()
{
- return clippingPath;
+ if (clippingPaths.size() == 1)
+ {
+ // If there is just a single clipping path, no intersections are needed.
+ GeneralPath path = clippingPaths.get(0);
+ return clippingCache.computeIfAbsent(path, Area::new);
+ }
+ // If there are multiple clipping paths, combine them to a single area.
+ Area clippingArea = new Area();
+ clippingArea.add(new Area(clippingPaths.get(0)));
+ for (int i = 1; i < clippingPaths.size(); i++)
+ {
+ clippingArea.intersect(new Area(clippingPaths.get(i)));
+ }
+ // Replace the list of individual clipping paths with the intersection, and add it to the cache.
+ GeneralPath newPath = new GeneralPath(clippingArea);
+ clippingPaths = new ArrayList<>();
+ clippingPaths.add(newPath);
+ clippingCache.put(newPath, clippingArea);
+ return clippingArea;
+ }
+
+ public List<GeneralPath> getCurrentClippingPaths()
+ {
+ return clippingPaths;
Review comment:
I prefer to keep things strict from the beginning if possible. However, you right in your case it won't work. So, let us start with something like the proposed JavaDoc
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@pdfbox.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@pdfbox.apache.org
For additional commands, e-mail: dev-help@pdfbox.apache.org
[GitHub] [pdfbox] THausherr commented on pull request #127: Lazier clipping
Posted by GitBox <gi...@apache.org>.
THausherr commented on pull request #127:
URL: https://github.com/apache/pdfbox/pull/127#issuecomment-896122913
Please merge the current version into your PR, I committed the two optimizations.
I tried my rendering tests and there are some minor differences. I did not yet include the changes to "transferClip".
The file from https://github.com/mozilla/pdf.js/issues/6961 has a slight loss in quality, even at 400%. Shouldn't the results be identical?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@pdfbox.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@pdfbox.apache.org
For additional commands, e-mail: dev-help@pdfbox.apache.org
[GitHub] [pdfbox] THausherr commented on pull request #127: Lazier clipping
Posted by GitBox <gi...@apache.org>.
THausherr commented on pull request #127:
URL: https://github.com/apache/pdfbox/pull/127#issuecomment-898886126
Here the benchmarks (not very reliable):
old: 287, 344, 328
changes, old transfer: 347, 298, 269, 282, 305
new transfer: 402, 444, 437
So your initial changes may have made it faster (if my previous comment is correct), but the change in `transfer` made it slower. Thus I'll commit your main change but not the improvement.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@pdfbox.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@pdfbox.apache.org
For additional commands, e-mail: dev-help@pdfbox.apache.org
[GitHub] [pdfbox] sschwieb commented on pull request #127: Lazier clipping
Posted by GitBox <gi...@apache.org>.
sschwieb commented on pull request #127:
URL: https://github.com/apache/pdfbox/pull/127#issuecomment-894871845
I did some testing, with the PDFs linked in [PDFBOX-4718](https://issues.apache.org/jira/browse/PDFBOX-4718) and [PDFBOX-4743](https://issues.apache.org/jira/browse/PDFBOX-4743).
By overriding `transferClip` as
```
protected void transferClip(PDGraphicsState graphicsState, Graphics2D graphics) {
List<GeneralPath> paths = graphicsState.getCurrentClippingPaths();
boolean allDone = true;
for (GeneralPath path : paths) {
if (!path.getPathIterator(null).isDone()) {
allDone = false;
break;
}
}
if (allDone)
{
// PDFBOX-4821: avoid bug with java printing that empty clipping path is ignored by
// replacing with empty rectangle, works because this is not an empty path
graphics.setClip(new Rectangle());
return;
}
GeneralPath first = paths.get(0);
graphics.setClip(first);
for (int i = 1; i < paths.size(); i++)
{
graphics.clip(paths.get(i));
}
}
```
, the OOM that occurred in the 1st PDF disappears, and the 2nd PDF is rendered much faster (still needs several seconds though). It seems that `SunGraphics2D.clip()` is doing a much better job than `Area.intersect()`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@pdfbox.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@pdfbox.apache.org
For additional commands, e-mail: dev-help@pdfbox.apache.org
[GitHub] [pdfbox] THausherr commented on pull request #127: Lazier clipping
Posted by GitBox <gi...@apache.org>.
THausherr commented on pull request #127:
URL: https://github.com/apache/pdfbox/pull/127#issuecomment-898897488
Thank you for your contribution. I'd like to do two more changes:
1) pass only the graphics device in the transfer method to make the parameter list smaller
2) use the `Path2D` abstract class instead of `Path2D.Double` on the left side of the assignments, and as list / map elements.
Any reason not to do that?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@pdfbox.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@pdfbox.apache.org
For additional commands, e-mail: dev-help@pdfbox.apache.org