This is a bug to hold work on fixing the gradient and pattern support for PDF output from Batik.
Created attachment 16799 [details] Patch for majority of transform issues with patterns/gradients This patch fixes most of the problems with 'simple' gradients and patterns in PDF output. However it does not cover everything there are three major outstanding issues: 1) Gradients with an SVG setting of spreadMethod reflect or repeat, are rendered with just 'pad'. Fixing this requires resorting to rasterizing the content (it might be possible to get around this for linear gradients but certainly not with radial). 2) Patterns that contain gradients generate 'bad' PDF. I suspect the problem is resource referencing. Since I am fairly sure the problem existed before my fidling I'm going to leave it as is. 3) Patterns with 'overflow="visible"' ignore overflow. Number 3 is pretty simple to fix (just need to render the gn content multiple times to make the tile). I suspect Number 2 is not too hard if you understand PDF (which I don't really). Number 1 would require adding support for rasterizing unknown paint's and using that as a pattern fill (just make the size of the pattern fill the same as shape being painted). A few notes on the patch. 1) I simplified the PDFState.Data class, by removing the the 'concatenations' List. It doesn't seem to be used by anything. 2) I changed PDFState.getTransform it now just returns the current transform as this already includes all the previous transforms. Perhaps I misunderstood the code but things work now and the transforms make sense where as they didn't before. 3) I added the ability to provide a transform to PDFFactory.makeGradient this is required to handle some of the more complex cases of gradients with non-uniform scaling transforms.
Created attachment 16800 [details] Update that support patterns with overflow="visible" This version of the patch supports overflow on patterns but it requires the SVN version of Batik as I needed to add access to the 'overflow' member of the PatternPaint class. For this reason I didn't obsolete the previous patch.
(In reply to comment #1) <snip/> > 2) Patterns that contain gradients generate 'bad' PDF. I suspect > the problem is resource referencing. Since I am fairly sure the > problem existed before my fidling I'm going to leave it as is. Do you have a test case to reproduce this? > 3) Patterns with 'overflow="visible"' ignore overflow. > > Number 3 is pretty simple to fix (just need to render the > gn content multiple times to make the tile). I suspect Number > 2 is not too hard if you understand PDF (which I don't really). > Number 1 would require adding support for rasterizing unknown > paint's and using that as a pattern fill (just make the size > of the pattern fill the same as shape being painted). > > A few notes on the patch. > 1) I simplified the PDFState.Data class, by removing the > the 'concatenations' List. It doesn't seem to be used > by anything. You're wrong. It is used by the PDF Renderer to handle "fixed" positioned block-containers. This can't just be removed. The concatenate() method even has a proper javadoc comment that explains the purpose. > 2) I changed PDFState.getTransform it now just returns the > current transform as this already includes all the previous > transforms. Perhaps I misunderstood the code but things work > now and the transforms make sense where as they didn't before. As you can see in my comment for 1) this can't be the solution. If you can give be the test cases that are involved here I can try to help. > 3) I added the ability to provide a transform to > PDFFactory.makeGradient this is required to handle some of the > more complex cases of gradients with non-uniform scaling > transforms.
(In reply to comment #2) > Created an attachment (id=16800) [edit] > Update that support patterns with overflow="visible" > > This version of the patch supports overflow on patterns but it > requires the SVN version of Batik as I needed to add access to > the 'overflow' member of the PatternPaint class. For this > reason I didn't obsolete the previous patch. Ok, but that part might have to wait until after the first FOP release since we need to be able to ship a Batik release. The other option is to do a bugfix release for Batik. I assume that would be a good idea anyway given the number of recent changes.
(In reply to comment #3) > (In reply to comment #1) > <snip/> > > 2) Patterns that contain gradients generate 'bad' PDF. I suspect > > the problem is resource referencing. Since I am fairly sure the > > problem existed before my fidling I'm going to leave it as is. > > Do you have a test case to reproduce this? Yes, sorry this was generated by: http://svn.apache.org/repos/asf/xmlgraphics/batik/trunk/samples/tests/spec/paints/patternRegions.svg Most of the document converts correctly but the last test seems to generate a PDF error. I took a quick look at it but it basically looked believable to me... > > A few notes on the patch. > > 1) I simplified the PDFState.Data class, by removing the > > the 'concatenations' List. It doesn't seem to be used > > by anything. > > You're wrong. It is used by the PDF Renderer to handle "fixed" positioned > block-containers. This can't just be removed. The concatenate() method > even has a proper javadoc comment that explains the purpose. Sorry about that. It's a bit odd I thought was careful to delete all class files after this change to ensure that I hadn't missed anything, and everything still compiled (of course it doesn't now). Anyway if this is needed it can stay, the real problem code was the way that 'PDFState.getTransform' was coded which was definately producing wrong results (transforms applied twice, see below). > > 2) I changed PDFState.getTransform it now just returns the > > current transform as this already includes all the previous > > transforms. Perhaps I misunderstood the code but things work > > now and the transforms make sense where as they didn't before. > > As you can see in my comment for 1) this can't be the solution. This part of the change is essentially independent from the first point. > If you can give be the test cases that are involved here I can try > to help. You would hit this problem with any SVG file (try linearGradientOrientation.svg in the same directory as above). The way I detected it was by tracking what is handed to 'PDFState.setTransform' and what is returned by 'PDFState.getTransform'. The value returned by getTransform has the transform applied twice. The reason is pretty clear when you clone the Data object for a push you don't clear the transform of the new Data object (you did clear the concatenations list), thus you end up with two Data objects both of which have what ever the transform was. Then the old getTransform code would then concatenate both of these resulting in a double transform. I think only the gradient code uses the transform returned by this method, so for most content the bug wouldn't show up. There are two potential solutions, first clear the transform when you clone the Data object, but leave the concatenation code in getTransform. Second have getTransform just return the current Data.transform (as I did, but leave the concatenations List stuff alone). If you do the first I suspect the code in PDFRenderer could just use the transform from the Data object to restore state, and the concatenations list could go away, but I don't really know or understand the code in question - I'll just say that the current system appears way more complex then it needs to be, you should be able to accomplish everything with the existing push/pop on PDFState. Finally, doesn't it bother you to have one class getting an iterator from a raw member of an inner class belonging to another class?
(In reply to comment #5) <snip/> > > > 2) Patterns that contain gradients generate 'bad' PDF. I suspect > > > the problem is resource referencing. Since I am fairly sure the > > > problem existed before my fidling I'm going to leave it as is. > > > > Do you have a test case to reproduce this? > > Yes, sorry this was generated by: > http://svn.apache.org/repos/asf/xmlgraphics/batik/trunk/samples/tests/spec/paints/patternRegions.svg > > Most of the document converts correctly but the > last test seems to generate a PDF error. I took a quick > look at it but it basically looked believable to me... Thanks, I'll look into it ASAP. > > > A few notes on the patch. > > > 1) I simplified the PDFState.Data class, by removing the > > > the 'concatenations' List. It doesn't seem to be used > > > by anything. > > > > You're wrong. It is used by the PDF Renderer to handle "fixed" positioned > > block-containers. This can't just be removed. The concatenate() method > > even has a proper javadoc comment that explains the purpose. > > Sorry about that. It's a bit odd I thought was careful to delete > all class files after this change to ensure that I hadn't missed anything, > and everything still compiled (of course it doesn't now). Anyway > if this is needed it can stay, the real problem code was the way that > 'PDFState.getTransform' was coded which was definately producing wrong > results (transforms applied twice, see below). I see. That sounds bad. > > > 2) I changed PDFState.getTransform it now just returns the > > > current transform as this already includes all the previous > > > transforms. Perhaps I misunderstood the code but things work > > > now and the transforms make sense where as they didn't before. > > > > As you can see in my comment for 1) this can't be the solution. > > This part of the change is essentially independent from the first > point. > > > If you can give be the test cases that are involved here I can try > > to help. > > You would hit this problem with any SVG file (try > linearGradientOrientation.svg in the same directory as above). > The way I detected it was by tracking what is handed to > 'PDFState.setTransform' and what is returned by > 'PDFState.getTransform'. The value returned by getTransform > has the transform applied twice. > > The reason is pretty clear when you clone the Data object for a > push you don't clear the transform of the new Data object (you did > clear the concatenations list), thus you end up with two Data > objects both of which have what ever the transform was. Then the > old getTransform code would then concatenate both of these > resulting in a double transform. I think only the gradient code > uses the transform returned by this method, so for most content > the bug wouldn't show up. > > There are two potential solutions, first clear the transform when > you clone the Data object, but leave the concatenation code in > getTransform. Second have getTransform just return the current > Data.transform (as I did, but leave the concatenations List stuff > alone). > > If you do the first I suspect the code in PDFRenderer could just > use the transform from the Data object to restore state, and the > concatenations list could go away, but I don't really know or > understand the code in question - I'll just say that the current > system appears way more complex then it needs to be, you should be > able to accomplish everything with the existing push/pop on PDFState. > > Finally, doesn't it bother you to have one class getting > an iterator from a raw member of an inner class belonging to > another class? Well, it bothers me to have public member variables in the first place. I simply haven't enough of an itch to fix it, yet. That stuff evolved over time. When I recently updated the PS renderer I took a cleaner approach to handle state information. Maybe that could be ported to the PDF case. Still, the transformation list is still necessary to recreate the same state after a "break out" as needed when painting fixed block-containers. I haven't found a better way to handle this case, yet. Essentially, you're welcome to rework this stuff if you find a better way, as long as especially the testcase "block-container_absolute-position_fixed.xml" doesn't get broken. I'm going to look deeper into this ASAP. Right now I want to finish the documentation for FOP Trunk first to prepare for the initial release. Looks like we will still not be able to release this month and next week I'm away. :-( Thanks for explaining the problem. I understand much better now what's going on.
Created attachment 16810 [details] Update to gradient transform fix, passes all junit tests. This replaces the first patch (it has the imporant bits of the second patch commented out as well). This eliminates the concatenations list, but resets the Data transform after each push. When the PDFRenderer want's to rebuild state it just uses the transforms from the individual data elements (rather than all of the concatenations). This appears to pass all of the JUnit tests but since this is the first time I've tried to run them it's a bit hard to know what this really means (is the PDF output checked for example? Is there PDF output?).
(In reply to comment #7) > Created an attachment (id=16810) [edit] > Update to gradient transform fix, passes all junit tests. Patch applied. Thanks. > This replaces the first patch (it has the imporant bits of the > second patch commented out as well). > > This eliminates the concatenations list, but resets the Data > transform after each push. When the PDFRenderer want's to > rebuild state it just uses the transforms from the individual > data elements (rather than all of the concatenations). Looks good to me. > This appears to pass all of the JUnit tests but since this > is the first time I've tried to run them it's a bit hard to > know what this really means (is the PDF output checked for > example? Is there PDF output?). The JUnit test can currently only check the FO tree and the layout engine, not the renderers. We have a special renderer, the area tree renderer, which writes out the area tree to an XML file and we use XPath statements to check against this XML file. The area tree is the thing that the renderers convert to the final output format, so we're only checking the input for the renderers, not their output. I've written a small tool (BatchDiffer) which can create diff images from PDF/PS/Java2D output, but it's not integrated into JUnit, yet, and rather designed for manual visual checking. I've actually stolen a few lines of code from Batik for that. :-) The problem with checking PDF and PS is that you need an external tool like GhostScript to convert PDF/PS to PNG so we could do automated diff testing. We used to do checksum testing on the PDF files but that's not really useful. Some way to go, one step at a time.... I'm leaving the issue open so we can look at the overflow thing ASAP.
Created attachment 16837 [details] Adds support for gradient repeat and transparency through rasterization This patch adds the ability to rasterize 'unknown' paints or paints that can't be mapped natively to PDF. This includes gradients with repeats, and or transparent stops. This patch covers fill and stroke, however it does not handle text. This is because currently it works by treating the paint as an image which it clips with the path to be filled (it strokes the path using the JDK and the just 'fills' it). For text this would require turning the text into outlines. A better way to do this would be to turn the image into a tile pattern (which would be expected to be used only once), but the transforms to pull this off are non-trivial, also I'm not sure I know quite enough PDF to get this right.
Created attachment 16853 [details] Update to gradient repeat, fixed createGraphics problem. This patch is an update to 16837. It indirects the access to jpegCount so all the PDFGraphics2D share a common count. This prevents inadvertant reuse of the wrong image.
Patch #10 applied. Thanks. http://svn.apache.org/viewcvs?rev=332046&view=rev I'm looking forward to moving the Transcoders out after the FOP release. :-) Is there anything outstanding concerning this bug or can we close it? It starts to get confusing which patches are to be applied and so on. I assume you'll also help track the currently commented part that we'll have to uncomment as soon as we can rely on a later Batik version.
(In reply to comment #11) > Patch #10 applied. Thanks. > I'm looking forward to moving the Transcoders out after the FOP release. :-) It will likely make this stuff easier to track. > Is there anything outstanding concerning this bug or can we close it? As I see it there are two outstanding issues: 1) 'Complex' patterns on Text - While it is probably a 5% case it's bad that things won't work. 2) The overflow for pattern case being commented out. I would lean towards saying this is still broken enough to leave the bug open, but it's not my bug DB (so to speak). Having it open makes it at least possible that someone will have an answer to their question. > It starts to get confusing which patches are to be applied and > so on. Yes, I strongly considered going to a single PDFTranscoder patch instead of trying to track everything seperately. > I assume you'll also help track the currently commented part that > we'll have to uncomment as soon as we can rely on a later Batik version. I'll try and make sure of it. What do you think of a Batik 1.6.1 release? There are a number of small but significant improvements in Batik since 1.6.1. The real hurdle would be straightening out the XML lib lincensing (which really needs to be fixed anyways).
(In reply to comment #12) <snip/> > > Is there anything outstanding concerning this bug or can we close it? > > As I see it there are two outstanding issues: > 1) 'Complex' patterns on Text - While it is probably a 5% case it's > bad that things won't work. > 2) The overflow for pattern case being commented out. > > I would lean towards saying this is still broken enough to leave the > bug open, but it's not my bug DB (so to speak). Having it open makes > it at least possible that someone will have an answer to their question. Ok. <snip/> > > I assume you'll also help track the currently commented part that > > we'll have to uncomment as soon as we can rely on a later Batik version. > > I'll try and make sure of it. What do you think of a Batik 1.6.1 > release? There are a number of small but significant improvements > in Batik since 1.6.1. The real hurdle would be straightening out > the XML lib lincensing (which really needs to be fixed anyways). It's on my list to help you with the XML libs as soon as the FOP release is off my desk. Anyway, I think a Batik 1.6.1 is a good idea. A lot of things have been done since the last release.
resetting P2 open bugs to P3 pending further review
increase priority for bugs with a patch
change status from ASSIGNED to NEW for consistency