I'm attaching a few typical PDF files where resulting PostScript level 3 output of pdftops is significantly larger than pdftocairo would produce. Would be nice if the output was smaller to improve printing performance over slow links. Numbers (pdf file size, pdftocairo ps file size, pdftops ps file size): pdf : 69390 pdftocairo : 99349 pdftops : 7727545 pdf : 242906 pdftocairo : 625772 pdftops : 1550570 pdf : 87515 pdftocairo : 558576 pdftops : 4514389 pdf : 93719 pdftocairo : 728959 pdftops : 1803946 pdf : 226284 pdftocairo : 408044 pdftops : 947692 Tested with poppler-utils 0.26.1 and 0.24.5 on Ubuntu 14.04.
Created attachment 103465 [details] pdf1.pdf
Created attachment 103466 [details] pdf2.pdf
Created attachment 103467 [details] pdf3.pdf
Created attachment 103468 [details] pdf4.pdf
Created attachment 103469 [details] pdf5.pdf
I only took a quick look. pdf1 uses tranparency. pdftocairo uses deflate to compress the images; pdftops uses rle. pdftops rasterizes the whole page; pdftocairo tries to avoid rasterizing areas which would look the same were transparency not involved. Whole page vs just the image is the big difference.
xpdf 3.04 now uses LZW to compress images in the PS output. I expect this change will eventually be merged into poppler. Bug 66056 is for the excessive rasterization problem.
Created attachment 121480 [details] [review] Patch to add LZW compression based on xpdf-3.04 This patch adds support for LZW compression using the code from xpdf-3.04. xpdf had a psLZW flag in GlobalParams. I changed it to enableLZW in PSOutput. In Stream.cc, xpdf used getBlock(buf, len) in LZWEncoder. I updated it to doGetChars(len, buf). $ ls -l tux-yellow.pdf -rw-r--r-- 1 william william 65720 Feb 2 19:58 tux-yellow.pdf $ ./pdftops tux-yellow.pdf tux-with-lzw.ps $ ./pdftops -nolzw tux-yellow.pdf tux-with-rle.ps $ ls -l tux-with-lzw.ps tux-with-rle.ps -rw-rw-rw- 1 william william 1126352 Feb 3 06:31 tux-with-lzw.ps -rw-rw-rw- 1 william william 27576756 Feb 3 06:31 tux-with-rle.ps I also added a few other small changes from xpdf-3.04.
Created attachment 121501 [details] tux-yellow.pdf Here's the tux-yellow.pdf file mentionned above to test the patch
You need to initialize enableLZW (In reply to William Bader from comment #8) > Created attachment 121480 [details] [review] [review] > Patch to add LZW compression based on xpdf-3.04 > > This patch adds support for LZW compression using the code from xpdf-3.04. > xpdf had a psLZW flag in GlobalParams. I changed it to enableLZW in PSOutput. > In Stream.cc, xpdf used getBlock(buf, len) in LZWEncoder. I updated it to > doGetChars(len, buf). > $ ls -l tux-yellow.pdf > -rw-r--r-- 1 william william 65720 Feb 2 19:58 tux-yellow.pdf > $ ./pdftops tux-yellow.pdf tux-with-lzw.ps > $ ./pdftops -nolzw tux-yellow.pdf tux-with-rle.ps > $ ls -l tux-with-lzw.ps tux-with-rle.ps > -rw-rw-rw- 1 william william 1126352 Feb 3 06:31 tux-with-lzw.ps > -rw-rw-rw- 1 william william 27576756 Feb 3 06:31 tux-with-rle.ps > I also added a few other small changes from xpdf-3.04.
Now that I have an idea how the streams work, I am trying to add FlateCompression also. I am putting the code in FlateStream, which already uses zlib to make a flate decoder. It looks like Stream has its own non-zlib version of the flate decoder. One of the comments said that it was for "safety". Does that mean that using zlib's deflate is also unsafe? Anyway, when I started with flate, I saw that I forgot to initialize enableLZW in the constructor, but I think that the only way to get into that code is through pdftops, which always calls the setter. In any case, if the initialization of enableLZW is the only issue, if you want, I can make a new patch. Regards, William
The zlib deflate does not handle buggy flate streams as well. If making zlib mandatory for use with compressing streams we would need a build option to disable zlib for decompression. You could reuse my deflate patch from the PDFWriter patch set that I have not yet had time to get around to finishing: https://bugs.freedesktop.org/attachment.cgi?id=89776
+ {"-nolzw", argFlag, &disableLZW, 0, I don't see any value in adding a -nolzw option. It is not in xpdf-3.04.
I can remove the -nolzw command line option. xpdf left the flag in GlobalParams, so I thought that it would be useful to expose it for testing, but I think that you are correct that a -nolzw or -noflate flag wouldn't serve any purpose. Thanks for the deflate patch. I was following the example in http://www.zlib.net/zlib_how.html but I wasn't sure if the stream needed any header or wrapper to be usable in a postscript filter. I noticed that the patch creates new DeflateStream files. I currently added my new code to the end of FlateStream, but I was considering that also because it allows poppler to use its own internal flate decoder and to use zlib only for encoding. I want to get it working first, then I'll clean it up. I could make a new configure flag to enable flate encoding, and then the encoding and decoding could be configured independently. I didn't have a chance to work on it today because we are in the middle of an installation of the system that I am paid to work on. Regards, William
(In reply to William Bader from comment #11) > Now that I have an idea how the streams work, I am trying to add > FlateCompression also. I am putting the code in FlateStream, which already > uses zlib to make a flate decoder. Just wanted to note that Flate decoding is only supported in postscript Level3, so if you implement that, it should be turned off if the user selects -level2 or -level1. > In any case, if the initialization of enableLZW is the only issue, if you > want, I can make a new patch. I have not found additional issues in the patch either. I agree the option is kind of useless.
(In reply to William Bader from comment #14) > Thanks for the deflate patch. I was following the example in > http://www.zlib.net/zlib_how.html but I wasn't sure if the stream needed any > header or wrapper to be usable in a postscript filter. It needs to be wrapped in an ASCIIHex or ASCII85 stream. You can look at the pdftocairo -ps output to see how cairo does it. > I noticed that the patch creates new DeflateStream files. I currently added > my new code to the end of FlateStream, but I was considering that also > because it allows poppler to use its own internal flate decoder and to use > zlib only for encoding. I want to get it working first, then I'll clean it > up. I think it is better to keep code that depends on external libraries in a separate file. The deflate stream in my patch is well tested and based on similar code in cairo that has been in use for 10 years. > I could make a new configure flag to enable flate encoding, and then the > encoding and decoding could be configured independently. That capability is already in my patch.
(In reply to William Bader from comment #11) > I think that the only way to get into that > code is through pdftops, which always calls the setter. The Qt frontends use PSOutputDev so i guess they can also get there somehow?
I have it working. I'm going to clean it up and send patches Sunday night after the "taronjada" http://lameva.barcelona.cat/carnaval/ca/taronjada.php I made a new FlateEncoder.cc and FlateEncoder.h. It can be enabled with ./configure --enable-zlib-compress separately from the zlib flate decoder which can still be enabled by --enable-zlib. I removed the pdftops -nolzw flag. I initialized enableLZW. Flate happens only with enableFlate and Level 3 output. LZW happens only with enableLZW and Level 2 output (or Level 3 if !enableFlate). The Flate stream is encoded with ASCII85. Is that necessary? I thought that level 2 and higher supported binary data. The deflate stream patch was very helpful, but I did not use it because the deflate stream writes the compressed data while the flate encoder gets the compressed data pulled from it, which turns the logic inside out, especially with the way that the zlib deflate() function works. What should the encoder do if deflateInit() returns an error status? I am calling deflateInit with Z_DEFAULT_COMPRESSION. Should the compression level be settable? I used 16K buffers like the deflate stream patch and the zlib examples. Should the buffers be larger or configurable? Regards, William $ ls -l tux-yellow.ps -rw-rw-rw- 1 william william 183552 Feb 7 05:59 tux-yellow.ps $ pdftops -level2 tux-yellow.pdf tux-yellow2.ps $ pdftops -level3 tux-yellow.pdf tux-yellow3.ps $ ls -l tux-yellow.pdf tux-yellow2.ps tux-yellow3.ps -rw-r--r-- 1 william william 65720 Feb 2 19:58 tux-yellow.pdf -rw-rw-rw- 1 william william 1126366 Feb 7 06:39 tux-yellow2.ps -rw-rw-r-- 1 william william 183552 Feb 7 06:39 tux-yellow3.ps $ grep --text Decode tux-yellow2.ps tux-yellow3.ps | grep filter tux-yellow2.ps: /ASCII85Decode filter tux-yellow2.ps: /LZWDecode filter tux-yellow3.ps: /ReusableStreamDecode filter tux-yellow3.ps: /ASCII85Decode filter tux-yellow3.ps: /FlateDecode filter
(In reply to William Bader from comment #18) > I have it working. I'm going to clean it up and send patches Sunday night > after the "taronjada" http://lameva.barcelona.cat/carnaval/ca/taronjada.php > I made a new FlateEncoder.cc and FlateEncoder.h. > It can be enabled with ./configure --enable-zlib-compress separately from > the zlib flate decoder which can still be enabled by --enable-zlib. I would prefer the options --disable-zlib --enable-zlib-uncompress ie default to linking zlib and using it for compression and default to zlib decompression disabled. zlib is widely available and there is no reason anyone would not want to use it to reduce PS output size. > The Flate stream is encoded with ASCII85. Is that necessary? I thought that > level 2 and higher supported binary data. I don't think binary is guaranteed to work everywhere. It is better to default to ASCII and provide an option to enable binary. > What should the encoder do if deflateInit() returns an error status? Fallback to LZW. > I am calling deflateInit with Z_DEFAULT_COMPRESSION. Should the compression > level be settable? I don't think a compression level option is necessary. The default works well. > I used 16K buffers like the deflate stream patch and the zlib examples. > Should the buffers be larger or configurable? Unless performance testing shows a larger size is faster I would leave it as 16K.
>I would prefer the options --disable-zlib --enable-zlib-uncompress OK. I saw that is what your patches did, but I wanted to make the minimum amount of changes to the build system. I think that your patches worked so --disable-zlib also disabled zlib uncompression. Is it OK to tie them together, or is it possible that someone would want zlib uncompression and not zlib compression? >I don't think binary is guaranteed to work everywhere. It is better to default to ASCII and provide an option to enable binary. I used level1sep a lot, but to make files that I can software-separate and not because I have a level 1 RIP. I already made a -binary options to make those files smaller. Could -binary also skip the ascii encoding in level 2 and 3? >Fallback to LZW. What is the best way to do that? Can the FlateEncoder constructor return an LZWEncoder, or does it need to return an error flag, and then all the places in PSOutputDev need to check the status? That could get messy. FlateEncoder also has a reset() function that needs to call deflateEnd() and deflateInit(). At that point, I think that it would be very hard to recover. I looked at deflate.c in the zlib source. I think that deflateInit() can fail only from a failed memory allocation, so it is probably very unlikely to fail, and if it does fail, probably other parts of poppler are going to fail for the same reason of lack of memory. Regards, William
(In reply to William Bader from comment #20) > >I would prefer the options --disable-zlib --enable-zlib-uncompress > > OK. I saw that is what your patches did, but I wanted to make the minimum > amount of changes to the build system. > > I think that your patches worked so --disable-zlib also disabled zlib > uncompression. Is it OK to tie them together, or is it possible that someone > would want zlib uncompression and not zlib compression? There are two use cases: --disable-zlib means for some reason the user can't or doesn't want to link to zlib. Maybe the installed zlib is broken. --enable-zlib-uncompress means the user prefers zlib for uncompress I can not see why the user would want to disable zlib for compression (that works fine and has been used in other PS generating applications without any issues for years), and enable zlib for uncompression that is known to not work well. > >I don't think binary is guaranteed to work everywhere. It is better to default to ASCII and provide an option to enable binary. > > I used level1sep a lot, but to make files that I can software-separate and > not because I have a level 1 RIP. I already made a -binary options to make > those files smaller. Could -binary also skip the ascii encoding in level 2 > and 3? yes > I looked at deflate.c in the zlib source. I think that deflateInit() can > fail only from a failed memory allocation, so it is probably very unlikely > to fail, and if it does fail, probably other parts of poppler are going to > fail for the same reason of lack of memory. In that case just print an error and exit.
Created attachment 121582 [details] [review] patch to add LZW and Flate compression This patch adds LZW and Flate compression. Flate should happen only with Level 3. LZW should happen with Level 2 (and Level 3 when Flate is not enabled). ./configure has the options --disable-zlib = disable flate compression (the default is enabled if zlib is present) --enable-zlib-uncompress = enable flate decompression using zlib instead of poppler's built-in flate decompression. This corresponds to the old --enable-zlib and is unsafe because zlib does not handle invalid streams as well as poppler. cmake has corresponding options -DENABLE_ZLIB=OFF -DENABLE_ZLIB_UNCOMPRESS=ON. I tested configure more than cmake. Errors in deflateInit() set the stream eof flag and print an error message with error(errInternal). I removed the -nolzw option in pdftops. The LZW encoder is in Stream.cc and .h The Flate encoder is in the new files FlateEncoder.cc and .h. I didn't touch the copyrights except in FlateEncoder. Since most of the LZW encoder came from xpdf-3.04, Derek should probably get the credit for it. Regards, William
Comment on attachment 121582 [details] [review] patch to add LZW and Flate compression Review of attachment 121582 [details] [review]: ----------------------------------------------------------------- The LZW and Flate compression should be in two separate commits in order to preserve the provenance of the code since the LZW code was copied from xpdf while Flate is new code. The Glyph & Cog copyright dates in the source files, poppler-config.h and man pages need updating. ::: .gitignore @@ +36,4 @@ > *.exe > +*.pdf > +*.ps > +*.eps I don't think we want to add these to gitignore. ::: poppler/FlateEncoder.h @@ +60,5 @@ > + virtual GBool isEncoder() { return gTrue; } > + > +private: > + > + enum FlateEncoderEnum { inBufSize = 16384, outBufSize = inBufSize }; Why use enums to define constants? The rest of the code uses #define or static const. ::: poppler/Gfx.cc @@ +2130,5 @@ > + if (fabs(det) < 0.000001) { > + error(errSyntaxError, getPos(), "Singular matrix in tiling pattern fill"); > + return; > + } > + det = 1 / det; What are the Gfx.cc changes for? ::: poppler/PSOutputDev.cc @@ +3010,5 @@ > + if (useFlate) { > + str = new FlateEncoder(str); > + } else > +#endif > + if (useLZW) { I would prefer to avoid using multiple "#if ENABLE_ZLIB" checks in this file. In some places the flate code is wrapped in both the #if and "if (useFlate)". In other places it just has the "if (useFlate)". It is not clear to anyone modifying this file is there is a reason for the distinction. It would be better to have just one #ifndef ENABLE_ZLIB useFlate = gFalse; #endif and rely on checking useFlate in the rest of the code.
(In reply to Adrian Johnson from comment #23) > I would prefer to avoid using multiple "#if ENABLE_ZLIB" checks in this > file. In some places the flate code is wrapped in both the #if and "if > (useFlate)". In other places it just has the "if (useFlate)". It is not > clear to anyone modifying this file is there is a reason for the distinction. I now see some of these are to wrap the FlateEncoder class which is only defined when ENABLE_ZLIB is defined. Those case are ok. There is one case where the #if is not necessary: +#if ENABLE_ZLIB + if (useFlate) { + writePS(" /FlateDecode filter\n"); + } else +#endif
Thanks for looking at the patches. >The LZW and Flate compression should be in two separate commits in order to preserve the provenance OK. I'll try to have that ready next week. One of my first patches was just LZW, but I made a few improvements (like initializing more variables) in the second patch with LZW+Flate. >I don't think we want to add these to gitignore. OK. I did that so my test files wouldn't get picked up in my diffs. >Why use enums to define constants? The rest of the code uses #define or static const. enums for constants are better style, since they are scoped while #defines are global. Let me know if you still want me to convert the enums to #defines. >What are the Gfx.cc changes for? They are a fix from xpdf. Should I pull them out into a separate patch? >I now see some of these are to wrap the FlateEncoder class Yes, I had to wrap all references to the FlateEncoder class. I left the useFlate and useLZW declarations unwrapped so that I didn't have to wrap every use, but I had to wrap all references to the class. I can remove the #if in the place that you mentioned. I thought that it was better to wrap it to avoid generating extra code and data. William
(In reply to William Bader from comment #25) > OK. I did that so my test files wouldn't get picked up in my diffs. You could put them in your git global config. > enums for constants are better style, since they are scoped while #defines > are global. "static const" in the private section is scoped. I prefer that we stick to the existing poppler style which is #define or "static const". > >What are the Gfx.cc changes for? > > They are a fix from xpdf. Should I pull them out into a separate patch? Yes. It is unrelated to the compression patch. We will also need a comment to put in the commit message for the Gfx changes. I've got no idea what those changes are for. > I can remove the #if in the place that you mentioned. I thought that it was > better to wrap it to avoid generating extra code and data. I prefer to keep the #ifdef'd code to a minimum. The problem is, in addition to making the code harder to read, only the most commonly used build options (eg with zlib compress, without zlib uncompress) get regularly tested. Changes to the surrounding code may break the uncommon cases and not get picked up because none of the developers tested with uncommon build options.
Created attachment 121862 [details] [review] Patch 1 of 3, xpdf-3.04 change to check for a singular matrix in tiling pattern fills Merge xpdf-3.04 change in poppler/Gfx.cc to avoid attempting a tiling pattern fill with a singular transform matrix (abs(determinant) < 0.000001). This is independent of the other patches.
Created attachment 121863 [details] [review] patch 2 of 3, merge xpdf-3.04 support for LZW encoding Merge xpdf-3.04 support for LZW encoding in PSOutputDev and Stream. Level 2 and Level 3 PostScript now use LZW encoding instead of Run Length encoding, which can make some images one tenth the size. PSOutputDev provides setEnableLZW() and getEnableLZW() to control support for LZW encoding.
Created attachment 121864 [details] [review] patch 3 of 3, add support for Flate compression Add support for Flate compression in Level 3 PostScript output. The changes to the build variables are from Adrian Johnson's DeflateStream patches at https://bugs.freedesktop.org/attachment.cgi?id=89776 I changed the enum in the earlier series of patches to "static const int" to match the poppler coding style. I also cleaned up the way that several functions in PSOutputDev.cc set the variables to decide which filter to use. Instead of a series of if's that each set all of the variables but one to false (based on the way that the lzw encoder was added in xpdf-3.04), I now initialize the variables all to false and set only one variable to true in each if. This should be clearer and should be easier to generalize if other filters are added. PSOutputDev::getEnableFlate() now returns false if zlib is not enabled. This reduces the need for #if's in PSOutputDev.cc, and it also fixes an output problem in my previous patches when building with zlib disabled.
Created attachment 121868 [details] [review] patch 3 of 3, add support for Flate compression, revised This is a revised version of the previous patch. It improves the cmake build. CMakeLists.txt had an incorrect test. FlateEncoder is better protected when built with ENABLE_ZLIB=OFF, and FlateStream is better protected when built with ENABLE_ZLIB_UNCOMPRESS=OFF.
Adrian will you take care of reviewing & commiting this?
(In reply to Albert Astals Cid from comment #31) > Adrian will you take care of reviewing & commiting this? Yes, I can look at it sometime this week.
I've tested the patches and committed them. I included a "xpdf304" tag in the xpdf commits. It looks like your patches were the output of "git log -p". As a suggestion, it makes it easier to apply your patches if you use git format-patch. Then I can use git am which will preserve the author and commit message so I don't have to manually apply them. eg to generate patches for the last 3 commits on your current branch: git format-patch -o <output dir> HEAD~3
Thanks for testing and committing the patches! I made a fresh clone from the freedesktop repository and checked against my work area that all of the files are now there. William
Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.