Re: [poppler] [PATCH] Add -jpegopt optimize option support to utils

[Martin (gzlist)]

On 24/06/2018, Albert Astals Cid  wrote:
>
> Not an English native speaker myself, but are you sure of the
> Selects vs Select change?

Struggling to find a documentation style guide that actually states
it, but think in general imperative is preferred over simple present.
See for instance `man grep` -w and -x (and in fact the whole file).
The pdftoppm man page is not consistent, both tenses are used, compare
-x/-y -opw/-upw or -png -jpeg.

> So if Select is the correct one should we change that one too?

Probably. I don't want to rewrite the whole manpage as part of this
change, it seems unrelated.

I've changed the print ones in pdftocairo, see amended patch also with
other wording tweaks attached.

Martin
Add -jpegopt optimize option support to utils

New option 'optimize=y' for utils that take a -jpegopt param,
pdftocairo and pdftoppm. This corresponds to the cjpeg -optimize
flag, and slightly reduces the size of the output jpeg but uses
additional cpu and memory.

New jpegOptimize boolean in splash/SplashBitmap WriteImgParams.

New setOptimize method on goo/JpegWriter taking a boolean.

Update manpages for new option.

Signed-off-by: Martin Packman 
---
 goo/JpegWriter.cc  | 11 +++
 goo/JpegWriter.h   |  1 +
 splash/SplashBitmap.cc |  1 +
 splash/SplashBitmap.h  |  1 +
 utils/pdftocairo.1 | 17 -
 utils/pdftocairo.cc| 10 ++
 utils/pdftoppm.1   | 13 ++---
 utils/pdftoppm.cc  | 10 ++
 8 files changed, 56 insertions(+), 8 deletions(-)

diff --git a/goo/JpegWriter.cc b/goo/JpegWriter.cc
index 37c15c2a..9ac9b63b 100644
--- a/goo/JpegWriter.cc
+++ b/goo/JpegWriter.cc
@@ -25,6 +25,7 @@ extern "C" {
 
 struct JpegWriterPrivate {
   bool progressive;
+  bool optimize;
   int quality;
   JpegWriter::Format format;
   struct jpeg_compress_struct cinfo;
@@ -46,6 +47,7 @@ JpegWriter::JpegWriter(int q, bool p, Format formatA)
 {
   priv = new JpegWriterPrivate;
   priv->progressive = p;
+  priv->optimize = false;
   priv->quality = q;
   priv->format = formatA;
 }
@@ -54,6 +56,7 @@ JpegWriter::JpegWriter(Format formatA)
 {
   priv = new JpegWriterPrivate;
   priv->progressive = false;
+  priv->optimize = false;
   priv->quality = -1;
   priv->format = formatA;
 }
@@ -75,6 +78,11 @@ void JpegWriter::setProgressive(bool progressive)
   priv->progressive = progressive;
 }
 
+void JpegWriter::setOptimize(bool optimize)
+{
+  priv->optimize = optimize;
+}
+
 bool JpegWriter::init(FILE *f, int width, int height, int hDPI, int vDPI)
 {
   // Setup error handler
@@ -137,6 +145,9 @@ bool JpegWriter::init(FILE *f, int width, int height, int hDPI, int vDPI)
 jpeg_simple_progression(>cinfo);
   }
 
+  // Set whether to compute optimal Huffman coding tables
+  priv->cinfo.optimize_coding = priv->optimize;
+
   // Get ready for data
   jpeg_start_compress(>cinfo, TRUE);
 
diff --git a/goo/JpegWriter.h b/goo/JpegWriter.h
index 9a68bcc9..bb87b949 100644
--- a/goo/JpegWriter.h
+++ b/goo/JpegWriter.h
@@ -41,6 +41,7 @@ public:
 
   void setQuality(int quality);
   void setProgressive(bool progressive);
+  void setOptimize(bool optimize);
   bool init(FILE *f, int width, int height, int hDPI, int vDPI) override;
 
   bool writePointers(unsigned char **rowPointers, int rowCount) override;
diff --git a/splash/SplashBitmap.cc b/splash/SplashBitmap.cc
index 2bec9f93..6f5e4cf8 100644
--- a/splash/SplashBitmap.cc
+++ b/splash/SplashBitmap.cc
@@ -356,6 +356,7 @@ void SplashBitmap::setJpegParams(ImgWriter *writer, WriteImgParams* params)
 #ifdef ENABLE_LIBJPEG
   if (params) {
 static_cast(writer)->setProgressive(params->jpegProgressive);
+static_cast(writer)->setOptimize(params->jpegOptimize);
 if (params->jpegQuality >= 0)
   static_cast(writer)->setQuality(params->jpegQuality);
   }
diff --git a/splash/SplashBitmap.h b/splash/SplashBitmap.h
index 092bd4cf..9756164f 100644
--- a/splash/SplashBitmap.h
+++ b/splash/SplashBitmap.h
@@ -81,6 +81,7 @@ public:
 int jpegQuality = -1;
 GBool jpegProgressive = gFalse;
 GooString tiffCompression;
+GBool jpegOptimize = gFalse;
   };
 
   SplashError writeImgFile(SplashImageFileFormat format, char *fileName, int hDPI, int vDPI, WriteImgParams* params = nullptr);
diff --git a/utils/pdftocairo.1 b/utils/pdftocairo.1
index 3a60b73e..6be3c410 100644
--- a/utils/pdftocairo.1
+++ b/utils/pdftocairo.1
@@ -305,21 +305,28 @@ When JPEG output is specified, the \-jpegopt option can be used to control the J
 It takes a string of the form "=[,=]". Currently the available options are:
 .TP
 .BI quality
-Selects the JPEG quality value. The value must be an integer between 0 and 100.
+Select the JPEG quality. The value must be an integer between 0 and 100.
 .TP
 .BI progressive
-Select progressive JPEG output. The possible values are "y", "n",
-indicating progressive (yes) or non-progressive (no), respectively.
+Select whether to write the JPEG as multiple scans of increasing quality. The
+value must be "y" or 

Re: [poppler] Utils - Fix UTF-16 file name on Windows environment

[William Bader]

I haven't used VMS for about two decades, but I remember that the extra options 
to fopen() are important. Without them, I think that you end up with a record 
oriented file instead of a stream file, and stdio might not work as you expect, 
for example, writing '\n' might close the current record instead of writing a 
0x0A byte. The full list of parameters is in the entry for creat() at 
http://h41379.www4.hpe.com/commercial/c/docs/5763p021.html#index_x_600

HP sold VMS to a company that is porting it to Intel x86-64, so VMS might soon 
be easier to run. http://www.openvms.org/node/107
Regards, William


From: poppler  on behalf of Martin 
(gzlist) 
Sent: Sunday, June 24, 2018 8:05 AM
To: poppler@lists.freedesktop.org
Subject: Re: [poppler] Utils - Fix UTF-16 file name on Windows environment

On 19/06/2018, Albert Astals Cid  wrote:
>
> It does make some sense given that function exists in the first place but my
> experience with windows is very limited so I'd like for someone
> else to vouch for this before landing it.

Overall the change might be, but there are some tricky aspects.

On windows fopen takes a narrow string of variable codepage, and
openFile takes (sort-of*) utf-8 but the types are not distinguished in
the codebase so it's tricky to see what callers are providing.

In cases where the string being used comes straight from say, the
command line or environment block, it will not be utf-8 so non-ascii
characters will be mangled. That's probably best fixed by ensuring on
GooString construction that it's converted to utf-8 but at present
that's entirely unvalidated.

Also:

 #ifdef VMS
-f = fopen(fileName->getCString(), "rb", "ctx=stm");
+f = openFile(fileName->getCString(), "rb", "ctx=stm");
 #else
-f = fopen(fileName->getCString(), "rb");
+f = openFile(fileName->getCString(), "rb");
 #endif

Breaks compilation on VMS (if that's still a platform that matters) as
openFile takes two args only. Oddly, GooFile::open already includes
this logic but openFile does not. Can just drop the first branch
change for now.

Martin

*sort-of utf-8: gfile.cc has a pretty half-assed utf-8 to utf-16
conversion algorithm in several places that only correctly handles a
subset of inputs.
___
poppler mailing list
poppler@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/poppler
poppler Info Page - 
freedesktop.org
lists.freedesktop.org
Subscribing to poppler: Subscribe to poppler by filling out the following form. 
Use of all freedesktop.org lists is subject to our Code of Conduct.



___
poppler mailing list
poppler@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/poppler

[poppler] 3 commits - poppler/Stream.cc utils/pdfsig.cc

[Albert Astals Cid]

 poppler/Stream.cc |   28 +++-
 utils/pdfsig.cc   |5 -
 2 files changed, 15 insertions(+), 18 deletions(-)

New commits:
commit 267228bb071016621c80fc8514927905164aaeea
Author: Albert Astals Cid 
Date:   Sun Jun 24 18:28:22 2018 +0200

pdfsig: Use posix basename() instead of GNU one

So it builds on non GNU systems too

Bug #106783

diff --git a/utils/pdfsig.cc b/utils/pdfsig.cc
index c4e52fd8..592270ad 100644
--- a/utils/pdfsig.cc
+++ b/utils/pdfsig.cc
@@ -23,6 +23,7 @@
 #include 
 #include 
 #include 
+#include 
 #include "parseargs.h"
 #include "Object.h"
 #include "Array.h"
@@ -104,7 +105,9 @@ static void dumpSignature(int sig_num, int sigCount, 
FormWidgetSignature *sig_wi
 // since { is the magic character to replace things we need to put it 
twice where
 // we don't want it to be replaced
 GooString *format = GooString::format("{{0:s}}.sig{{1:{0:d}d}}", 
sigCountLength);
-GooString *path = GooString::format(format->getCString(), 
basename(filename), sig_num);
+char *filenameCopy = strdup(filename);
+GooString *path = GooString::format(format->getCString(), 
basename(filenameCopy), sig_num);
+free(filenameCopy);
 printf("Signature #%d (%u bytes) => %s\n", sig_num, 
signature->getLength(), path->getCString());
 std::ofstream outfile(path->getCString(), std::ofstream::binary);
 outfile.write(signature->getCString(), signature->getLength());
commit 459f369145a9d8f638fe123425f0e2880487640e
Author: Albert Astals Cid 
Date:   Sun Jun 24 11:51:20 2018 +0200

Move variable declarations closer to where they are used

Allows for declaring two of them as const

diff --git a/poppler/Stream.cc b/poppler/Stream.cc
index f43c3ef9..2c55f295 100644
--- a/poppler/Stream.cc
+++ b/poppler/Stream.cc
@@ -499,12 +499,6 @@ GBool ImageStream::getPixel(Guchar *pix) {
 }
 
 Guchar *ImageStream::getLine() {
-  Gulong buf, bitMask;
-  int bits;
-  int c;
-  int i;
-  Guchar *p;
-
   if (unlikely(inputLine == nullptr)) {
   return nullptr;
   }
@@ -512,9 +506,9 @@ Guchar *ImageStream::getLine() {
   int readChars = str->doGetChars(inputLineSize, inputLine);
   for ( ; readChars < inputLineSize; readChars++) inputLine[readChars] = EOF;
   if (nBits == 1) {
-p = inputLine;
-for (i = 0; i < nVals; i += 8) {
-  c = *p++;
+Guchar *p = inputLine;
+for (int i = 0; i < nVals; i += 8) {
+  const int c = *p++;
   imgLine[i+0] = (Guchar)((c >> 7) & 1);
   imgLine[i+1] = (Guchar)((c >> 6) & 1);
   imgLine[i+2] = (Guchar)((c >> 5) & 1);
@@ -531,17 +525,17 @@ Guchar *ImageStream::getLine() {
 // we assume a component fits in 8 bits, with this hack
 // we treat 16 bit images as 8 bit ones until it's fixed correctly.
 // The hack has another part on GfxImageColorMap::GfxImageColorMap
-p = inputLine;
-for (i = 0; i < nVals; ++i) {
+Guchar *p = inputLine;
+for (int i = 0; i < nVals; ++i) {
   imgLine[i] = *p++;
   p++;
 }
   } else {
-bitMask = (1 << nBits) - 1;
-buf = 0;
-bits = 0;
-p = inputLine;
-for (i = 0; i < nVals; ++i) {
+const Gulong bitMask = (1 << nBits) - 1;
+Gulong buf = 0;
+int bits = 0;
+Guchar *p = inputLine;
+for (int i = 0; i < nVals; ++i) {
   while (bits < nBits) {
buf = (buf << 8) | (*p++ & 0xff);
bits += 8;
commit e8b82c4239da638ae77dfab07faaff33af4af1cc
Author: Albert Astals Cid 
Date:   Sun Jun 24 11:46:36 2018 +0200

ImageStream::getLine: Fix ubsan undefined shift

I'm not totally sure this is the "correct" fix, but doesn't regress
any file on my test suite so seems one of those cases only happens
on bad files, and this helps oss-fuzz progress in its testing

Fixes oss-fuzz/8432

diff --git a/poppler/Stream.cc b/poppler/Stream.cc
index 21bd3b9b..f43c3ef9 100644
--- a/poppler/Stream.cc
+++ b/poppler/Stream.cc
@@ -504,7 +504,7 @@ Guchar *ImageStream::getLine() {
   int c;
   int i;
   Guchar *p;
-  
+
   if (unlikely(inputLine == nullptr)) {
   return nullptr;
   }
@@ -542,7 +542,7 @@ Guchar *ImageStream::getLine() {
 bits = 0;
 p = inputLine;
 for (i = 0; i < nVals; ++i) {
-  if (bits < nBits) {
+  while (bits < nBits) {
buf = (buf << 8) | (*p++ & 0xff);
bits += 8;
   }
___
poppler mailing list
poppler@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/poppler

Re: [poppler] [PATCH] Add -jpegopt optimize option support to utils

[Albert Astals Cid]

El dijous, 14 de juny de 2018, a les 0:27:32 CEST, Martin (gzlist) va 
escriure:
> On 13/06/2018, Albert Astals Cid  wrote:
> > You probably need to update the manpages of both utils to include that?
> 
> Done, see amended patch attached. Also updated the wording of the
> other options a little, with reference to the jpeg-turbo documentation
> on the options.
> 
> I note the copyright on AUTHOR below seems a little outdated, but left
> that alone.

Not an English native speaker myself, but are you sure of the 
Selects vs Select change?

Also below we have
   .BI source
  Selects the source paper ...

So if Select is the correct one should we change that one too?

Cheers,
  Albert

> 
> Martin




___
poppler mailing list
poppler@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/poppler