Bug 47311 - [PATCH] Support for bleed, trim and crop box and scaling
[PATCH] Support for bleed, trim and crop box and scaling
Status: REOPENED
Product: Fop - Now in Jira
Classification: Unclassified
Component: general
all
All All
: P2 enhancement
: ---
Assigned To: fop-dev
:
Depends on:
Blocks:
  Show dependency tree
 
Reported: 2009-06-03 10:53 UTC by Peter Coppens
Modified: 2012-04-11 03:22 UTC (History)
0 users



Attachments
Patch adding support for bleed, trim and crop box and scaling (17.52 KB, patch)
2009-06-03 10:53 UTC, Peter Coppens
Details | Diff
updated patch in an attempt to address fop-dev suggestions (27.69 KB, application/octet-stream)
2009-07-08 07:01 UTC, Peter Coppens
Details
Updated patch avoid awt issue (27.67 KB, application/octet-stream)
2009-07-14 08:25 UTC, Peter Coppens
Details
AWTRenderer change to take into account fop:scale setting (2.17 KB, patch)
2009-07-22 00:04 UTC, Peter Coppens
Details | Diff
Box implementation for Java2d now uses "pdf box settings" (29.89 KB, application/octet-stream)
2009-07-28 00:02 UTC, Peter Coppens
Details
Updated patch with some clean-up and modifications (34.67 KB, patch)
2009-07-28 04:31 UTC, Vincent Hennebert
Details | Diff
Patch including Jeremias'es proposed changes (30.02 KB, patch)
2009-07-30 01:25 UTC, Peter Coppens
Details | Diff
Fix for /trunk (715 bytes, patch)
2009-11-23 23:52 UTC, Boris Y.
Details | Diff
Full patch for FOP-0.95 version (52.74 KB, patch)
2009-11-23 23:52 UTC, Boris Y.
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Peter Coppens 2009-06-03 10:53:30 UTC
Created attachment 23751 [details]
Patch adding support for bleed, trim and crop box and scaling

The attached patch adds support for 4 new simple-page-master fop extension attributes, namely

- fox:crop-box
- fox:trim-box
- fox:bleed-box
- fox:scale

The box attributes can consist out of up to 4 numeric values (top,left, width, height) and can have units as suffix to each of the numbers.

The scale attribute can consist out of two numbers each between 0 and 1

It is implemented for PDF and Java2D renderers.
Comment 1 Andreas L. Delmelle 2009-06-03 11:53:21 UTC
Thanks for this patch, Peter!

I know this is going to be very useful, as questions regarding that functionality pop up on the user list from time to time. We'll look into it, and will keep you informed. I currently have other priorities, but as soon as I see an available slot of time, if no one beats me to it, I'll go into it in more detail.
Comment 2 Peter Coppens 2009-06-17 22:46:19 UTC
(In reply to comment #1)
> Thanks for this patch, Peter!
> 
> I know this is going to be very useful, as questions regarding that
> functionality pop up on the user list from time to time. We'll look into it,
> and will keep you informed. I currently have other priorities, but as soon as I
> see an available slot of time, if no one beats me to it, I'll go into it in
> more detail.


I was wondering whether there is anything I can do to make this move forward. 

Thanks,


Peter
Comment 3 Vincent Hennebert 2009-06-18 04:07:38 UTC
Hi Peter,

(In reply to comment #2)
> (In reply to comment #1)
> > Thanks for this patch, Peter!
> > 
> > I know this is going to be very useful, as questions regarding that
> > functionality pop up on the user list from time to time. We'll look into it,
> > and will keep you informed. I currently have other priorities, but as soon as I
> > see an available slot of time, if no one beats me to it, I'll go into it in
> > more detail.
> 
> 
> I was wondering whether there is anything I can do to make this move forward. 

Not much until we have reviewed the patch and provided comments. Then, if modifications are needed you may want to do them yourself and provide an updated patch, which would speed up its integration in the code base.

I've only had a quick look so far and can't give much feedback yet. I'll try to have a deeper look in the next days. A few things I noticed, though:
- the IllegalArgumentException in ExtensionElementMapping will have to be replaced by a call to FOP's event notification mechanism (so that the message can be localized, among others things). Have a look in, e.g., org.apache.fop.fo.flow.Table.java to see how it is used (TableEventProducer in that case)
- the new features will have to be documented on the website. The corresponding source files may be found in the src/documentation/content/xdocs/trunk directory. The ideal place probably is extensions.xml, with a link in output.xml.
- there are Checkstyle warnings in the new code. You can set up Checkstyle using the checkstyle-4.0.xml at the root of the project.

Those are things that we will have to do before applying the patch anyway, so if you want to speed up the process you can have a go at them.

Back later, hopefully, for comments on the functionalities themselves.

Thanks!
Vincent
Comment 4 Vincent Hennebert 2009-06-26 03:08:28 UTC
I've had a more detailed look at the patch and it looks good to me. A few other comments:
- the definition and parsing of the extension properties should not be put in ExtensionElementMapping, but in a class (and a sub-package) of its own. Where exactly it should be put is not entirely clear to me yet. Probably in a new o.a.f.render.extensions package. How to name the new sub-package also is an open question (scalingcropbleedtrim? rather ugly... pageboundaries (taken from the PDF spec)? prepress?).
- amendment to what I said about the event notification mechanism: the extension should not use it, rather throw appropriate exceptions, which would be passed over by the PDF library (the classes in o.a.f.pdf) to the PDFDocumentHandler. The former two should remain independent of the notification mechanism to allow later extraction from the FOP codebase and modularization. Only the PDFDocumentHandler must be aware of the notification mechanism.
- the regexp parsing properties should be made slightly more robust and unit-tested.
- I have a concern about when the properties are actually parsed. Like it is now they will be parsed only at rendering stage, so if there is a mistake in them the error will be thrown rather 'late' in the process (only after layout has been performed). May that be a problem? Do we want a 'fail-fast' behaviour instead? Open question.

I'm happy to do all the mentioned changes myself, but this will be in one week at the earliest as I'm away next week. Peter, if meanwhile you want to submit an updated patch feel free to do so :-)

Thanks,
Vincent
Comment 5 Andreas L. Delmelle 2009-06-26 03:54:12 UTC
(In reply to comment #4)
> - I have a concern about when the properties are actually parsed. Like it is
> now they will be parsed only at rendering stage, so if there is a mistake in
> them the error will be thrown rather 'late' in the process (only after layout
> has been performed). May that be a problem? Do we want a 'fail-fast' behaviour
> instead? Open question.

Well, since the extension attributes are specific to the PDF renderer, I think it is not a blocker to treat them as plain 'foreign attributes' at parse-time. One case comes to mind where we would need to treat them as genuine 'properties': suppose we want to be able to specify them as expressions, based on other properties.
Comment 6 Andreas L. Delmelle 2009-06-26 04:02:05 UTC
(In reply to comment #5)
> 
> Well, since the extension attributes are specific to the PDF renderer, I think

Sorry, ignore this. I just noticed that in the patch, the extension is also implemented for the Java2D-based renderers...
Comment 7 Andreas L. Delmelle 2009-06-26 04:37:10 UTC
Just noting this for general interest (has little or nothing to do with this issue per se):
The previous comments suddenly reminded me that I have always considered the current way that extension /properties/ are handled, as lacking in robustness. We practically force potential implementors of extension properties to modify FOP's codebase. For extension elements or attributes, the pattern is much more open and generic. They can be implemented without necessarily having to modify FOP and recompile. The same should become the case for extension properties, eventually. We probably will want to take a look at offering something like an 'ExtensionPropertyMapping' (currently non-existent), so that implementors can re-use existing PropertyMakers, define their own initial values/enums/keywords, mark properties as inherited, and so on...

There are already a few 'native' extension properties, for which we define symbolic literals in fo.Constants and which are also registered in fo.FOPropertyMapping. I have never really been too happy with that practice. Contributors are invited to follow that pattern, which will only lead to more clutter.

Tackling that issue, however, is not for the faint of heart. It would likely also imply revisiting the way properties are attached to the FONodes and how they are exposed to the outside (layoutengine & renderers), so would lead to changes in a Lot of classes (capital 'L', if you catch the drift...)
Comment 8 Peter Coppens 2009-07-08 07:01:16 UTC
Created attachment 23941 [details]
updated patch in an attempt to address fop-dev suggestions
Comment 9 Sean Griffin 2009-07-09 07:01:12 UTC
I think I might have missed something on the mailing lists explaining these features, but Peter, can you explain what these new properties do?  What they offer that it's in the spec?
Comment 10 Peter Coppens 2009-07-09 07:21:32 UTC
(In reply to comment #9)
> I think I might have missed something on the mailing lists explaining these
> features, but Peter, can you explain what these new properties do?  What they
> offer that it's in the spec?

See beginning of bugzilla entry which says
<quote>

Patch adding support for bleed, trim and crop box and scaling

The attached patch adds support for 4 new simple-page-master fop extension
attributes, namely

- fox:crop-box
- fox:trim-box
- fox:bleed-box
- fox:scale

The box attributes can consist out of up to 4 numeric values (top,left, width,
height) and can have units as suffix to each of the numbers.

The scale attribute can consist out of two numbers each between 0 and 1

It is implemented for PDF and Java2D renderers.
</quote>
Comment 11 Sean Griffin 2009-07-09 07:29:51 UTC
Yeah, I saw that, but that still doesn't tell me what I'm looking for.  What's the use case for using these properties?  What functionality do they offer that's not in other properties in the specification?  Is this for background images or something?  Watermarks?

Based on Andreas's comments in this bug this sounds like a common feature request, I've just never heard of it.

I'm only curious because I use FOP pretty extensively and wonder if these extensions add something I would find useful.
Comment 12 Peter Coppens 2009-07-09 07:42:02 UTC
(In reply to comment #11)
> Yeah, I saw that, but that still doesn't tell me what I'm looking for.  What's
> the use case for using these properties?  What functionality do they offer
> that's not in other properties in the specification?  Is this for background
> images or something?  Watermarks?
> 
> Based on Andreas's comments in this bug this sounds like a common feature
> request, I've just never heard of it.
> 
> I'm only curious because I use FOP pretty extensively and wonder if these
> extensions add something I would find useful.

Ah..ic

The box'es set the pdf page boxes with the same name. I found http://www.prepressure.com/pdf/basics/page_boxes useful (not necessarily finding the pdf spec an easy read ;)). These boxes are often used to drive what is actually printed as compared to what e.g. acrobat displays

Scale is something we needed to use fop for creating adverts. The size of the output depends on e.g. the magazine you want your advert to be published in. As we want to use same stylesheet for different magazines a scale factor makes that possible
Comment 13 Vincent Hennebert 2009-07-14 04:23:42 UTC
(In reply to comment #8)
> Created an attachment (id=23941) [details]
> updated patch in an attempt to address fop-dev suggestions

Thanks for the updated patch. I tried to run the AWT renderer and it appears to have been broken by the changes. The main window is opened but no document is displayed, and I get the following exception:
Exception in thread "AWT-EventQueue-0" java.awt.image.RasterFormatException: (x + width) is outside raster
	at sun.awt.image.IntegerInterleavedRaster.createWritableChild(IntegerInterleavedRaster.java:450)
	at java.awt.image.BufferedImage.getSubimage(BufferedImage.java:1156)
	at org.apache.fop.render.java2d.Java2DRenderer.getPageImage(Java2DRenderer.java:379)
	at org.apache.fop.render.java2d.Java2DRenderer.getPageImage(Java2DRenderer.java:425)
	at org.apache.fop.render.awt.viewer.ImageProxyPanel.paintComponent(ImageProxyPanel.java:124)
	at javax.swing.JComponent.paint(JComponent.java:1027)
	at javax.swing.JComponent.paintChildren(JComponent.java:864)
	at javax.swing.JComponent.paint(JComponent.java:1036)
	at javax.swing.JComponent.paintToOffscreen(JComponent.java:5122)
	at javax.swing.BufferStrategyPaintManager.paint(BufferStrategyPaintManager.java:277)
	at javax.swing.RepaintManager.paint(RepaintManager.java:1217)
	at javax.swing.JComponent._paintImmediately(JComponent.java:5070)
	at javax.swing.JComponent.paintImmediately(JComponent.java:4880)
	at javax.swing.RepaintManager.paintDirtyRegions(RepaintManager.java:803)
	at javax.swing.RepaintManager.paintDirtyRegions(RepaintManager.java:714)
	at javax.swing.RepaintManager.seqPaintDirtyRegions(RepaintManager.java:694)
	at javax.swing.SystemEventQueueUtilities$ComponentWorkRequest.run(SystemEventQueueUtilities.java:128)
	at java.awt.event.InvocationEvent.dispatch(InvocationEvent.java:209)
	at java.awt.EventQueue.dispatchEvent(EventQueue.java:597)
	at java.awt.EventDispatchThread.pumpOneEventForFilters(EventDispatchThread.java:269)
	at java.awt.EventDispatchThread.pumpEventsForFilter(EventDispatchThread.java:184)
	at java.awt.EventDispatchThread.pumpEventsForHierarchy(EventDispatchThread.java:174)
	at java.awt.EventDispatchThread.pumpEvents(EventDispatchThread.java:169)
	at java.awt.EventDispatchThread.pumpEvents(EventDispatchThread.java:161)
	at java.awt.EventDispatchThread.run(EventDispatchThread.java:122)

I'll investigate (but: do you really need this feature also for the AWT renderer? ;-) )

Thanks,
Vincent
Comment 14 Peter Coppens 2009-07-14 05:07:06 UTC
> 
> Thanks for the updated patch. I tried to run the AWT renderer and it appears to
> have been broken by the changes. The main window is opened but no document is
> displayed, and I get the following exception:
> Exception in thread "AWT-EventQueue-0" java.awt.image.RasterFormatException: (x
> + width) is outside raster
>     at

Oops...that is not so good

> 
> I'll investigate (but: do you really need this feature also for the AWT
> renderer? ;-) )
> 
Actually 'only' for png output but both awt and png output probably originate from the Java2DRenderer ?
Comment 15 Peter Coppens 2009-07-14 08:25:06 UTC
Created attachment 23978 [details]
Updated patch avoid awt issue

Another attempt (hopefully) not breaking awt renderer
Comment 16 Vincent Hennebert 2009-07-16 04:21:28 UTC
There seems to be another problem: in the Swing preview the zoom no longer works. I fixed that by multiplying the scales by scaleFactor instead of overwriting that latter:
    if (scales != null) {
        scaleX *= scales.getX();
        scaleY *= scales.getY();
    }
but then scrolling bars appear the same way as if fox:scale had not been specified. When resizing the window they appear whereas there is obviously still room for the whole document to fit in.

I have doubts about that scale extension, I must say. It seems very ad-hoc to me. Can't that be left to some post-processing mechanism? For PDF output this usually is a job that is handled by the printer. For PNG output I'm sure that there are plenty of programs that can do that very well (actually I had a better quality result when re-scaling the PNG output with an external program than by using the new extension —might be a problem with the Java2D renderer though).

Also, is there a use case for a non-proportional scale (x scale != y scale)? Not that having different x and y factors makes the whole thing a lot more complicated, but...

Thanks,
Vincent

(In reply to comment #13)
> (In reply to comment #8)
> > Created an attachment (id=23941) [details] [details]
> > updated patch in an attempt to address fop-dev suggestions
> 
> Thanks for the updated patch. I tried to run the AWT renderer and it appears to
> have been broken by the changes. The main window is opened but no document is
> displayed, and I get the following exception:
> Exception in thread "AWT-EventQueue-0" java.awt.image.RasterFormatException: (x
> + width) is outside raster
>     at
> sun.awt.image.IntegerInterleavedRaster.createWritableChild(IntegerInterleavedRaster.java:450)
>     at java.awt.image.BufferedImage.getSubimage(BufferedImage.java:1156)
>     at
> org.apache.fop.render.java2d.Java2DRenderer.getPageImage(Java2DRenderer.java:379)
>     at
> org.apache.fop.render.java2d.Java2DRenderer.getPageImage(Java2DRenderer.java:425)
>     at
> org.apache.fop.render.awt.viewer.ImageProxyPanel.paintComponent(ImageProxyPanel.java:124)
>     at javax.swing.JComponent.paint(JComponent.java:1027)
>     at javax.swing.JComponent.paintChildren(JComponent.java:864)
>     at javax.swing.JComponent.paint(JComponent.java:1036)
>     at javax.swing.JComponent.paintToOffscreen(JComponent.java:5122)
>     at
> javax.swing.BufferStrategyPaintManager.paint(BufferStrategyPaintManager.java:277)
>     at javax.swing.RepaintManager.paint(RepaintManager.java:1217)
>     at javax.swing.JComponent._paintImmediately(JComponent.java:5070)
>     at javax.swing.JComponent.paintImmediately(JComponent.java:4880)
>     at javax.swing.RepaintManager.paintDirtyRegions(RepaintManager.java:803)
>     at javax.swing.RepaintManager.paintDirtyRegions(RepaintManager.java:714)
>     at javax.swing.RepaintManager.seqPaintDirtyRegions(RepaintManager.java:694)
>     at
> javax.swing.SystemEventQueueUtilities$ComponentWorkRequest.run(SystemEventQueueUtilities.java:128)
>     at java.awt.event.InvocationEvent.dispatch(InvocationEvent.java:209)
>     at java.awt.EventQueue.dispatchEvent(EventQueue.java:597)
>     at
> java.awt.EventDispatchThread.pumpOneEventForFilters(EventDispatchThread.java:269)
>     at
> java.awt.EventDispatchThread.pumpEventsForFilter(EventDispatchThread.java:184)
>     at
> java.awt.EventDispatchThread.pumpEventsForHierarchy(EventDispatchThread.java:174)
>     at java.awt.EventDispatchThread.pumpEvents(EventDispatchThread.java:169)
>     at java.awt.EventDispatchThread.pumpEvents(EventDispatchThread.java:161)
>     at java.awt.EventDispatchThread.run(EventDispatchThread.java:122)
> 
> I'll investigate (but: do you really need this feature also for the AWT
> renderer? ;-) )
> 
> Thanks,
> Vincent
Comment 17 Peter Coppens 2009-07-22 00:04:55 UTC
Created attachment 24018 [details]
AWTRenderer change to take into account fop:scale setting
Comment 18 Peter Coppens 2009-07-22 00:16:37 UTC
>>I fixed that by multiplying the scales by scaleFactor instead of
>>overwriting that latter:
Yeah..that seems correct.


> I have doubts about that scale extension, I must say. It seems very ad-hoc to
> me. Can't that be left to some post-processing mechanism? For PDF output this
> usually is a job that is handled by the printer. For PNG output I'm sure that
> there are plenty of programs that can do that very well (actually I had a
> better quality result when re-scaling the PNG output with an external program
> than by using the new extension —might be a problem with the Java2D renderer
> though).
>
Obviously scaling can be handled through a post processing step, just like adding the pdf boxes can be handled using e.g. PDFBox after fop has rendered the stylesheet to pdf. This is what we currently use. But it is very inelegant as we now need to also store 'template/stylesheet' information outside the stylesheet, dispatch postprocessing based on output type, and it also adds extra processing overhead where, with the integrated approach, almost no extra overhead is needed. Once confronted with things like 'adverts' where page size options are very restricted by publishers it does seem to make sense to integrate it all together, at least from a 'users' perspective. Whether it makes sense for fo(p), I feel not very well placed to comment (at lease the box requirement has been requested before)
 
> 
> Also, is there a use case for a non-proportional scale (x scale != y scale)?
> Not that having different x and y factors makes the whole thing a lot more
> complicated, but...
> 
Publishers do restrict aspect ratio's. It does not make sense, layout wise, to do 'big' non-proportional scalings, but small factors allow to reuse the same stylesheet page content, for different 'publishers' and that does make the amount of maintenance a lot more manageable.

Thanks

Peter
Comment 19 Vincent Hennebert 2009-07-23 04:16:23 UTC
Hi,

(In reply to comment #18)
> >>I fixed that by multiplying the scales by scaleFactor instead of
> >>overwriting that latter:
> Yeah..that seems correct.

In the meantime, I found out why the Swing renderer doesn't take the scale extension into account: the AWTRenderer class (which should have been named SwingRenderer really) defines its own getPageImageSize method instead of re-using the stuff from Java2DRenderer.getPageImage. I'll see if I find the energy to fix that.


> > I have doubts about that scale extension, I must say. It seems very ad-hoc to
> > me. Can't that be left to some post-processing mechanism? For PDF output this
> > usually is a job that is handled by the printer. For PNG output I'm sure that
> > there are plenty of programs that can do that very well (actually I had a
> > better quality result when re-scaling the PNG output with an external program
> > than by using the new extension —might be a problem with the Java2D renderer
> > though).
> >
> Obviously scaling can be handled through a post processing step, just like
> adding the pdf boxes can be handled using e.g. PDFBox after fop has rendered
> the stylesheet to pdf. This is what we currently use. But it is very inelegant
> as we now need to also store 'template/stylesheet' information outside the
> stylesheet, dispatch postprocessing based on output type, and it also adds
> extra processing overhead where, with the integrated approach, almost no extra
> overhead is needed. Once confronted with things like 'adverts' where page size
> options are very restricted by publishers it does seem to make sense to
> integrate it all together, at least from a 'users' perspective. Whether it
> makes sense for fo(p), I feel not very well placed to comment (at lease the box
> requirement has been requested before)

Note that I don't question the box extensions, which are indeed useful and have already been requested in the past. Only the scale extension was looking very specific to me.


> > Also, is there a use case for a non-proportional scale (x scale != y scale)?
> > Not that having different x and y factors makes the whole thing a lot more
> > complicated, but...
> > 
> Publishers do restrict aspect ratio's. It does not make sense, layout wise, to
> do 'big' non-proportional scalings, but small factors allow to reuse the same
> stylesheet page content, for different 'publishers' and that does make the
> amount of maintenance a lot more manageable.

Ok. Makes sense.

There is an inconsistency between PDF and Java2D regarding the coordinates of the boxes: in PDF the x and y coordinates are relative to the left and bottom sides, in Java2D they are relative to the left and /top/ sides. One of the two possibilities will have to be chosen, probably the PDF way.


Thanks,
Vincent
Comment 20 Peter Coppens 2009-07-28 00:02:42 UTC
Created attachment 24048 [details]
Box implementation for Java2d now uses "pdf box settings"

The attached patch fixes the previous comment

"There is an inconsistency between PDF and Java2D regarding the coordinates of
the boxes: in PDF the x and y coordinates are relative to the left and bottom
sides, in Java2D they are relative to the left and /top/ sides. One of the two
possibilities will have to be chosen, probably the PDF way."

The boxes for the Java2D renderer now also follows the "PDF way"
Comment 21 Jeremias Maerki 2009-07-28 01:39:04 UTC
Hi Peter,
incidentally, I need this functionality myself in a project I'm currently working on. I've locally applied your patch to play with it. I apologize for the late feedback.

I notice you chose the page size of the simple-page-master as the baseline for the MediaBox. I would have expected the SPM's size to define the TrimBox instead. Bleed and cut marks would then lie outside the actual logical page. I've checked what other FO implementations do and they seem to follow that pattern rather than your approach.

I'm also finding the specification of the areas a bit counter-intuitive. A simple value only sets the left side, rather than the value for all four sides as with other FO properties. I guess that also points to Andreas' comment about reusing property infrastructure where this is already handled. Granted, it is not easy to use. I'm not even sure myself if this can easily be reused with some serious refactoring. At any rate, a print shop will usually just give you the information that you should use 2 or 3 mm for the bleed area. Just specifying one simple value is quite handy.

Rather than just criticizing, I'm willing to invest some time to help with this. I would like to make a counter-proposal for the extensions:

The simple-page-master's width and height properties shall define the TrimBox. If there is no bleed and crop mark area, the MediaBox will be equal to the TrimBox, or rather just the MediaBox is generated in this case, like it happens today.

fox:bleed: <length>{1,4}
Default: 0pt
If there is only one value, it applies to all sides. If there are two values, the top and bottom bleed widths are set to the first value and the right and left bleed widths are set to the second. If there are three values, the top is set to the first value, the left and right are set to the second, and the bottom is set to the third. If there are four values, they apply to the top, right, bottom, and left, respectively. (Corresponds to http://www.w3.org/TR/xsl11/#padding)
The BleedBox is calculated by expanding the TrimBox by the bleed widths.
(I'd prefer to call the property fox:bleed rather than fox:bleed-box as we don't set the BleedBox values directly. We specify the bleed amount.)

fox:crop-offset: <length>{1,4}
Default: 0pt
Same behaviour as with fox:bleed.
The MediaBox is calculated by expanding the BleedBox by the crop offsets.

BTW, the naming above pretty much matches other FO implementations, so should we ever have a standard for these properties (like by reviving exslfo.sf.net), it's likely we probably don't have to change much besides the namespace prefix.

fox:crop-box: (trim-box|bleed-box|media-box)
Default: media-box
The crop box controls how Acrobat display the page or how the Java2DRenderer sizes the output media. The PDF spec defines that the CropBox defaults to the MediaBox, so it makes sense to do the same here. We could define a fox:crop-box extension which could take three "magic" values: "trim-box", "bleed-box" and "media-box" to set the CropBox to one of those three other boxes. That should cover 95% of all use cases. If anyone needs more control, that could easily added later.

I don't have much feedback on fox:scale. I guess it can be useful but I don't see a big use case for an extension. I'd rather want to control that from application code. But it shouldn't get in the way of anything so I have not problem with it.

WDYT?
Comment 22 Vincent Hennebert 2009-07-28 04:28:01 UTC
Hi Jeremias,

I can't afford to spend much time on this patch so I'm happy to hand over the job to you. I'm attaching a new patch with my own modifications as I had started to prepare it for commit. Not sure it's going to be any useful given the different approach you'd like to take, but who knows.

A note about the patch: it seemed more sensible to me to deal with integer values rather than double. Indeed every length value is internally converted into a whole number expressed in mpt (AFAICT). That means that I'm using Rectangle instead of Rectangle2D, and I changed some parameters into some classes accordingly (mainly, o.a.f.layoutmgr.Page.java, o.a.f.area.PageViewport.java). But then I noticed that the AreaTreeParser reads the value of the "bounds" attribute as double instead of integer. AFAIK, the XML area tree is produced with whole numbers for the page boundaries; I don't see why a user would change that into decimal numbers (which would mean a precision below the mpt!). So I also modified the AreaTreeParser. I don't think this may create any problem?

Also, a few questions:

(In reply to comment #21)
<snip/>
> 
> The simple-page-master's width and height properties shall define the TrimBox.
> If there is no bleed and crop mark area, the MediaBox will be equal to the
> TrimBox, or rather just the MediaBox is generated in this case, like it happens
> today.
> 
> fox:bleed: <length>{1,4}
> Default: 0pt
> If there is only one value, it applies to all sides. If there are two values,
> the top and bottom bleed widths are set to the first value and the right and
> left bleed widths are set to the second. If there are three values, the top is
> set to the first value, the left and right are set to the second, and the
> bottom is set to the third. If there are four values, they apply to the top,
> right, bottom, and left, respectively. (Corresponds to
> http://www.w3.org/TR/xsl11/#padding)
> The BleedBox is calculated by expanding the TrimBox by the bleed widths.
> (I'd prefer to call the property fox:bleed rather than fox:bleed-box as we
> don't set the BleedBox values directly. We specify the bleed amount.)
> 
> fox:crop-offset: <length>{1,4}
> Default: 0pt
> Same behaviour as with fox:bleed.
> The MediaBox is calculated by expanding the BleedBox by the crop offsets.

Just to be sure: values will be allowed to be negative, right?

It would make more sense to me to set the default value of crop-offset to the value of bleed.


> BTW, the naming above pretty much matches other FO implementations, so should
> we ever have a standard for these properties (like by reviving exslfo.sf.net),
> it's likely we probably don't have to change much besides the namespace prefix.
> 
> fox:crop-box: (trim-box|bleed-box|media-box)
> Default: media-box
> The crop box controls how Acrobat display the page or how the Java2DRenderer
> sizes the output media. The PDF spec defines that the CropBox defaults to the
> MediaBox, so it makes sense to do the same here. We could define a fox:crop-box
> extension which could take three "magic" values: "trim-box", "bleed-box" and
> "media-box" to set the CropBox to one of those three other boxes. That should
> cover 95% of all use cases. If anyone needs more control, that could easily
> added later.

I'm really not sure about that one. Calling it 'crop-box' is likely to create confusion with the 'crop-offset' property above. Apparently the CropBox is not used in prepress at all, so I would suggest to forget about that for the moment. That is, make the CropBox match with the MediaBox.


> I don't have much feedback on fox:scale. I guess it can be useful but I don't
> see a big use case for an extension. I'd rather want to control that from
> application code. But it shouldn't get in the way of anything so I have not
> problem with it.
> 
> WDYT?


Vincent
Comment 23 Vincent Hennebert 2009-07-28 04:31:34 UTC
Created attachment 24049 [details]
Updated patch with some clean-up and modifications
Comment 24 Peter Coppens 2009-07-28 04:42:22 UTC
We were just about to start to rework the patch according to the suggestions made by Jeremias which seem to make a lot of sense, but I am not so sure anymore that is still useful given Vincent's alternative implementation/efforts which we were unaware of.

It's a bit unclear right now how we can continue to contribute given the different implementation efforts

We still have a bit of bandwith to spare on this (it is important for us) but we'd rather make sure those efforts are part of a consolidated action

Can someone advice?

Thanks

Peter
Comment 25 Jeremias Maerki 2009-07-28 04:54:14 UTC
Peter, I'll respond to Vincent's comments shortly. I think his changes shouldn't affect your changes too much. I think Vincent's comments make sense and I'll see to it that they can be committed separately (they don't really have much to do with the issue at hand). Then it will ideally just be a "svn up" on your side to resolve this. And a minor SVN conflict in the worst case. I suggest you just continue as intended.
Comment 26 Jeremias Maerki 2009-07-28 05:03:05 UTC
Hi Vincent,

(In reply to comment #22)
> I can't afford to spend much time on this patch so I'm happy to hand over the
> job to you. I'm attaching a new patch with my own modifications as I had
> started to prepare it for commit. Not sure it's going to be any useful given
> the different approach you'd like to take, but who knows.

I'm glad to take over.

> A note about the patch: it seemed more sensible to me to deal with integer
> values rather than double. Indeed every length value is internally converted
> into a whole number expressed in mpt (AFAICT). That means that I'm using
> Rectangle instead of Rectangle2D, and I changed some parameters into some
> classes accordingly (mainly, o.a.f.layoutmgr.Page.java,
> o.a.f.area.PageViewport.java). But then I noticed that the AreaTreeParser reads
> the value of the "bounds" attribute as double instead of integer. AFAIK, the
> XML area tree is produced with whole numbers for the page boundaries; I don't
> see why a user would change that into decimal numbers (which would mean a
> precision below the mpt!). So I also modified the AreaTreeParser. I don't think
> this may create any problem?

I don't think so. I'll extract your changes from your patch and apply that separately. That should make it easier for Peter, too.

> Also, a few questions:
<snip/>
> > fox:crop-offset: <length>{1,4}
> > Default: 0pt
> > Same behaviour as with fox:bleed.
> > The MediaBox is calculated by expanding the BleedBox by the crop offsets.
> 
> Just to be sure: values will be allowed to be negative, right?

Negative values don't make much sense, if I didn't miss anything.

> It would make more sense to me to set the default value of crop-offset to the
> value of bleed.

Right.

> > BTW, the naming above pretty much matches other FO implementations, so should
> > we ever have a standard for these properties (like by reviving exslfo.sf.net),
> > it's likely we probably don't have to change much besides the namespace prefix.
> > 
> > fox:crop-box: (trim-box|bleed-box|media-box)
> > Default: media-box
> > The crop box controls how Acrobat display the page or how the Java2DRenderer
> > sizes the output media. The PDF spec defines that the CropBox defaults to the
> > MediaBox, so it makes sense to do the same here. We could define a fox:crop-box
> > extension which could take three "magic" values: "trim-box", "bleed-box" and
> > "media-box" to set the CropBox to one of those three other boxes. That should
> > cover 95% of all use cases. If anyone needs more control, that could easily
> > added later.
> 
> I'm really not sure about that one. Calling it 'crop-box' is likely to create
> confusion with the 'crop-offset' property above. Apparently the CropBox is not
> used in prepress at all, so I would suggest to forget about that for the
> moment. That is, make the CropBox match with the MediaBox.

Yes, I thought about that naming closeness, too. Not sure if that's so easy to resolve. If Peter is OK with this, I guess the fox:crop-box could be left out for the moment. I personally don't need it although at some point, such a property might be useful to some.

<snip/>
Comment 27 Jeremias Maerki 2009-07-28 05:57:22 UTC
Partially applied Vincent's patch (from today). I've also applied the undisputed parts of Peter's patch. So the next patch by Peter will only need to be about the extensions and their integration into the renderers. I hope the result will merge as painlessly as possible.
http://svn.apache.org/viewvc?rev=798511&view=rev
Comment 28 Peter Coppens 2009-07-28 06:46:26 UTC
> > > 
> > > fox:crop-box: (trim-box|bleed-box|media-box)
> > > Default: media-box
> > > The crop box controls how Acrobat display the page or how the Java2DRenderer
> > > sizes the output media. The PDF spec defines that the CropBox defaults to the
> > > MediaBox, so it makes sense to do the same here. We could define a fox:crop-box
> > > extension which could take three "magic" values: "trim-box", "bleed-box" and
> > > "media-box" to set the CropBox to one of those three other boxes. That should
> > > cover 95% of all use cases. If anyone needs more control, that could easily
> > > added later.
> > 
> > I'm really not sure about that one. Calling it 'crop-box' is likely to create
> > confusion with the 'crop-offset' property above. Apparently the CropBox is not
> > used in prepress at all, so I would suggest to forget about that for the
> > moment. That is, make the CropBox match with the MediaBox.
> 
> Yes, I thought about that naming closeness, too. Not sure if that's so easy to
> resolve. If Peter is OK with this, I guess the fox:crop-box could be left out
> for the moment. I personally don't need it although at some point, such a
> property might be useful to some.
> 
> <snip/>

Hmmm...not entirely happy with this unfortunately. We have a webapp that using a flex wysiwyg editor allows creation of print materials but that uses adobe's pdf browser plugin for end-user proofing and approval. In some cases we'd rather display the trimbox iso the mediabox so my guess is we should be able to control the CropBox setting, no?

Perhaps a name change for the attribute is sufficient? E.g. something like crop-box-selector, or crop-box-source or something similar?
Comment 29 Vincent Hennebert 2009-07-28 07:23:29 UTC
(In reply to comment #26)
> Hi Vincent,
> 
> (In reply to comment #22)
<snip/>
> > A note about the patch: it seemed more sensible to me to deal with integer
> > values rather than double. Indeed every length value is internally converted
> > into a whole number expressed in mpt (AFAICT). That means that I'm using
> > Rectangle instead of Rectangle2D, and I changed some parameters into some
> > classes accordingly (mainly, o.a.f.layoutmgr.Page.java,
> > o.a.f.area.PageViewport.java). But then I noticed that the AreaTreeParser reads
> > the value of the "bounds" attribute as double instead of integer. AFAIK, the
> > XML area tree is produced with whole numbers for the page boundaries; I don't
> > see why a user would change that into decimal numbers (which would mean a
> > precision below the mpt!). So I also modified the AreaTreeParser. I don't think
> > this may create any problem?
> 
> I don't think so. I'll extract your changes from your patch and apply that
> separately. That should make it easier for Peter, too.

I've just committed a change to also replace Rectangle2D with Rectangle in
PDFFactory and PDFPage. Forgot to do that in my patch.


> > Also, a few questions:
> <snip/>
> > > fox:crop-offset: <length>{1,4}
> > > Default: 0pt
> > > Same behaviour as with fox:bleed.
> > > The MediaBox is calculated by expanding the BleedBox by the crop offsets.
> > 
> > Just to be sure: values will be allowed to be negative, right?
> 
> Negative values don't make much sense, if I didn't miss anything.

Actually not, because the simple-page-master's width and height would define
the TrimBox. What I had in mind is to make the BleedBox coincide with the
simple-page-master's boundaries, and define a TrimBox inside it. I was thinking
that sometimes the document may be easier to design this way (especially when
you want areas with dark backgrounds to bleed off the boundary of the final
page). I guess it's not actually the case.

<snip/>

Vincent
Comment 30 Jeremias Maerki 2009-07-28 23:54:23 UTC
I've started a Wiki page for the prepress feature: http://wiki.apache.org/xmlgraphics-fop/PrepressSupport

I'll follow up with some additional thoughts on the fop-dev list as discussion in Bugzilla is a bit awkward.
Comment 31 Peter Coppens 2009-07-30 01:25:35 UTC
Created attachment 24063 [details]
Patch including Jeremias'es proposed changes
Comment 32 Jeremias Maerki 2009-07-30 06:37:10 UTC
Thanks for the new patch, Peter. I've taken a look and found a few issues. I've already started fixing them. Among them:
- In the Wiki I've switched the meaning for crop-offset to align with what AntennaHouse did (see their illustration): crop-offset expands from the TrimBox, not the BleedBox. That is currently not reflected in the code, so I'll change that, too, if there's no opposition.
- In the AWT preview, the positioning wasn't ok, yet, when there's a bleed or crop-offset.
- The Java2DRenderer generates an ugly page border which I've disabled locally. Not sure why we even had that in place. It doesn't make much sense.
- Also, I've changed the background painting in the Java2DRenderer to use the BleedBox instead of the page size.

Otherwise, the patch makes a good impression. The whole thing is now very intuitive to use, just like I imagined it should be. I'll just allow some time for additional feedback from others. In the meantime, I'll finish the changes to the patch I've started and finally commit the whole thing. BTW, I've also written a little demo FO which demonstrates the features and how I would go about doing crop marks with SVG. I'll commit that after the patch is processed.
Comment 33 Jeremias Maerki 2009-07-30 07:16:51 UTC
Peter, why can fox:scale not have any number greater than 1? Why only support shrinking but not upsizing?
Comment 34 Peter Coppens 2009-07-30 07:27:33 UTC
(In reply to comment #33)
> Peter, why can fox:scale not have any number greater than 1? Why only support
> shrinking but not upsizing?

Hmmm...probably just an oversight on our end. For the sake of generality it should be possible to also use >1 values. You want us to send a new patch?
Comment 35 Peter Coppens 2009-07-30 07:28:18 UTC
(In reply to comment #32)
> Thanks for the new patch, Peter. I've taken a look and found a few issues. I've
> already started fixing them. Among them:
> - In the Wiki I've switched the meaning for crop-offset to align with what
> AntennaHouse did (see their illustration): crop-offset expands from the
> TrimBox, not the BleedBox. That is currently not reflected in the code, so I'll
> change that, too, if there's no opposition.
Ok for me.
Comment 36 Jeremias Maerki 2009-07-30 07:47:10 UTC
(In reply to comment #34)
> (In reply to comment #33)
> > Peter, why can fox:scale not have any number greater than 1? Why only support
> > shrinking but not upsizing?
> 
> Hmmm...probably just an oversight on our end. For the sake of generality it
> should be possible to also use >1 values. You want us to send a new patch?

Not necessary, thanks. I'll just change that myself then.
Comment 37 Jeremias Maerki 2009-08-02 12:46:22 UTC
Applied the patch with the changes already mentioned. Thanks a lot for your patch Peter, and thanks also for your patience with my late involvement.
http://svn.apache.org/viewvc?rev=800142&view=rev

I hope we will have a chance some day to improve the property subsystem so we can also handle extension like this more elegantly.
Comment 38 Vincent Hennebert 2009-08-03 04:13:09 UTC
(In reply to comment #37)
> Applied the patch with the changes already mentioned. Thanks a lot for your
> patch Peter, and thanks also for your patience with my late involvement.
> http://svn.apache.org/viewvc?rev=800142&view=rev
> 
> I hope we will have a chance some day to improve the property subsystem so we
> can also handle extension like this more elegantly.

This is not quite finished I'm afraid:
- I thought we agreed to set the default value of crop-offset to bleed
- errors in the specification of extension properties are not redirected through the event mechanism
- there are typos in the PageBoundariesAttributes and PageScaleAttributes classes
- there is an encapsulation problem: the fall back boxes used when one of the properties has not been defined are controlled by the client code, instead of being handled inside the extension class itself (PageBoundariesAttributes). This can easily lead to inconsistencies (one default value used in the PDF renderer, another one in the Java2D renderer).

Re-opening the bug as a reminder, as I don't have the time to handle that right now.
Comment 39 Jeremias Maerki 2009-08-03 07:34:06 UTC
3 of the 4 latest comments have been hopefully satisfyingly taken care of:
http://svn.apache.org/viewvc?rev=800401&view=rev
Comment 40 Vincent Hennebert 2009-08-04 04:16:56 UTC
(In reply to comment #39)
> 3 of the 4 latest comments have been hopefully satisfyingly taken care of:
> http://svn.apache.org/viewvc?rev=800401&view=rev

This is looking much better now, thanks. There are still a few remaining issues that I'll handle in the next days.
Comment 41 Boris Y. 2009-11-23 23:52:14 UTC
Created attachment 24599 [details]
Fix for /trunk
Comment 42 Boris Y. 2009-11-23 23:52:58 UTC
Created attachment 24600 [details]
Full patch for FOP-0.95 version
Comment 43 Boris Y. 2009-11-23 23:54:24 UTC
Hi All,

We have found one issue during testing this new feature.
The issue lies in PageBoundaries.java in calculating crop/bleed boxes.

The offsets  order is: [top, right, bottom, left], so to calculate Y size of the final box we should use the 'bottom' instead of 'top' offset :


         return new Rectangle(originalRect.x - coords[3],
-                originalRect.y - coords[0],
+                originalRect.y - coords[2],
                 originalRect.width + coords[3] + coords[1],
                 originalRect.height + coords[0] + coords[2]);
		 
		 
		 
Please find in the attachments the fix patch. (Comment#41)
Also I have attached the full patch for FOP-0.95 version (Comment#42) if somebody will have a need to use this feature with previous version.
Comment 44 Jeremias Maerki 2009-11-25 12:01:00 UTC
(In reply to comment #43)
> Hi All,
> 
> We have found one issue during testing this new feature.
> The issue lies in PageBoundaries.java in calculating crop/bleed boxes.
> 
> The offsets  order is: [top, right, bottom, left], so to calculate Y size of
> the final box we should use the 'bottom' instead of 'top' offset :
> 
> 
>          return new Rectangle(originalRect.x - coords[3],
> -                originalRect.y - coords[0],
> +                originalRect.y - coords[2],
>                  originalRect.width + coords[3] + coords[1],
>                  originalRect.height + coords[0] + coords[2]);
> 
> 
> 
> Please find in the attachments the fix patch. (Comment#41)
> Also I have attached the full patch for FOP-0.95 version (Comment#42) if
> somebody will have a need to use this feature with previous version.

I've taken a look at that. Thanks for spotting the problem, Boris, but your solution was not the right one. But you brought me on the right track. I've just found out what our mistake is: PDF specifies the boxes as "rectangles" which are defined as "llx lly urx ury" (i.e. lower left to upper right). But our/FOP's Rectangle2D objects are actually "upper left to lower right". In PageBoundaries we're still in FOP's coordinate system which starts at the upper left. So we have to calculate the right values for the default PDF coordinate system. Boris' change would have broken a test case and created a bug on the bitmap production side. So the right change is to do a transformation from FOP's internal coordinate system to PDF's default one in PDFDocumentHandler: http://svn.apache.org/viewvc?rev=884241&view=rev

Boris, can you please verify that this fix also work for you? Thanks!
Comment 45 Glenn Adams 2012-04-11 03:22:26 UTC
increase priority for bugs with a patch