Hey Albert,
Seems like it duplicates some code from the other method, would it be possible to merge them a bit more?
I tried with a single function first. Too many special cases and you don't want any if's in those performance-sensitive inner loops. Eventually I split it into two functions, sacrificing brevity for readability, simplicity and performance.
I'm sure a better, faster, shorter solution is possible, but I'm no John Carmack. :)
valgrind does not complain about it when i do valgrind ./utils/pdftoppm -jpeg ~/pdf_reference_1-7.pdf -f 1 -l 1 foo Are you sure that is the problem?
It turns out that my transparent image patch seems to introduce the problem that the second patch fixes. It also contains another bug where if(withAlpha) should be if(!withAlpha) that the second patch also fixes.
I've moved the fix for the withAlpha issue to the first patch. So the second patch now only fixes the JPEG issue.
If you apply only the first patch Valgrind does indeed not complain. You get the corrupted JPEG, but no segfault or error from valgrind. What I believe is happening is that libjpeg is smart enough to detect the problem and aborts the image generation to protect it's host process.
Once you apply the second patch - which changes nothing other than freeing the temporary buffer after the call to writer->close() - the problem disappears.
Here is the valgrind output from a test run with the first patch only (note: no errors) and attached the resulting image (note: bottom corrupted/cut off):
m...@clymene:/atlas/www/txtbear/local/test$ valgrind ../../dist/poppler-http/utils/pdftoppm -jpeg -r 25 -f 1 -l 1 /atlas/www/txtbear/data/assets/00/01/14/7c/source.pdf outtest
==31979== Memcheck, a memory error detector ==31979== Copyright (C) 2002-2009, and GNU GPL'd, by Julian Seward et al.==31979== Using Valgrind-3.6.0.SVN-Debian and LibVEX; rerun with -h for copyright info ==31979== Command: ../../dist/poppler-http/utils/pdftoppm -jpeg -r 25 -f 1 -l 1 /atlas/www/txtbear/data/assets/00/01/14/7c/source.pdf outtest
==31979== ==31979== ==31979== HEAP SUMMARY: ==31979== in use at exit: 0 bytes in 0 blocks==31979== total heap usage: 51,067 allocs, 51,067 frees, 10,131,934 bytes allocated
==31979== ==31979== All heap blocks were freed -- no leaks are possible ==31979== ==31979== For counts of detected and suppressed errors, rerun with: -v ==31979== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 85 from 10) Go figure. Cheers, Stefan Thomas
<<attachment: outtest-01.jpg>>
>From 2aeb71cf92ba9e25be6745c98ce3fa96c3c2d95e Mon Sep 17 00:00:00 2001
From: Stefan Thomas <tho...@eload24.com>
Date: Fri, 9 Jul 2010 22:59:04 +0100
Subject: [PATCH 1/2] Add support for transparent PNGs to PNGWriter and
SplashBitmap.
---
goo/PNGWriter.cc | 5 +-
goo/PNGWriter.h | 4 +-
splash/SplashBitmap.cc | 130 ++++++++++++++++++++++++++++++++++++++++++++----
splash/SplashBitmap.h | 5 ++-
splash/SplashTypes.h | 3 +-
5 files changed, 132 insertions(+), 15 deletions(-)
diff --git a/goo/PNGWriter.cc b/goo/PNGWriter.cc
index aebab9e..1aa9532 100644
--- a/goo/PNGWriter.cc
+++ b/goo/PNGWriter.cc
@@ -18,8 +18,9 @@
#include "poppler/Error.h"
-PNGWriter::PNGWriter()
+PNGWriter::PNGWriter(GBool withAlphaChannel)
{
+ this->withAlphaChannel = withAlphaChannel;
}
PNGWriter::~PNGWriter()
@@ -59,7 +60,7 @@ bool PNGWriter::init(FILE *f, int width, int height, int
hDPI, int vDPI)
png_set_compression_level(png_ptr, Z_BEST_COMPRESSION);
png_byte bit_depth = 8;
- png_byte color_type = PNG_COLOR_TYPE_RGB;
+ png_byte color_type = (withAlphaChannel) ? PNG_COLOR_TYPE_RGB_ALPHA :
PNG_COLOR_TYPE_RGB;
png_byte interlace_type = PNG_INTERLACE_NONE;
png_set_IHDR(png_ptr, info_ptr, width, height, bit_depth, color_type,
interlace_type, PNG_COMPRESSION_TYPE_DEFAULT, PNG_FILTER_TYPE_DEFAULT);
diff --git a/goo/PNGWriter.h b/goo/PNGWriter.h
index 64ffc67..70bc86d 100644
--- a/goo/PNGWriter.h
+++ b/goo/PNGWriter.h
@@ -22,11 +22,12 @@
#include <cstdio>
#include <png.h>
#include "ImgWriter.h"
+#include "goo/gtypes.h"
class PNGWriter : public ImgWriter
{
public:
- PNGWriter();
+ PNGWriter(GBool withAlphaChannel = gFalse);
~PNGWriter();
bool init(FILE *f, int width, int height, int hDPI, int vDPI);
@@ -39,6 +40,7 @@ class PNGWriter : public ImgWriter
private:
png_structp png_ptr;
png_infop info_ptr;
+ GBool withAlphaChannel;
};
#endif
diff --git a/splash/SplashBitmap.cc b/splash/SplashBitmap.cc
index f983439..8aca37d 100644
--- a/splash/SplashBitmap.cc
+++ b/splash/SplashBitmap.cc
@@ -288,13 +288,20 @@ SplashError
SplashBitmap::writeImgFile(SplashImageFileFormat format, char *fileN
SplashError SplashBitmap::writeImgFile(SplashImageFileFormat format, FILE *f,
int hDPI, int vDPI) {
ImgWriter *writer;
- SplashError e;
+ SplashError e;
+
+ GBool withAlpha = gFalse;
switch (format) {
#ifdef ENABLE_LIBPNG
case splashFormatPng:
writer = new PNGWriter();
break;
+
+ case splashFormatPngAlpha:
+ writer = new PNGWriter(gTrue);
+ withAlpha = gTrue;
+ break;
#endif
#ifdef ENABLE_LIBJPEG
@@ -310,12 +317,15 @@ SplashError
SplashBitmap::writeImgFile(SplashImageFileFormat format, FILE *f, in
return splashErrGeneric;
}
- e = writeImgFile(writer, f, hDPI, vDPI);
- delete writer;
- return e;
+ e = writeImgFile(writer, f, hDPI, vDPI, withAlpha);
+ delete writer;
+ return e;
}
-SplashError SplashBitmap::writeImgFile(ImgWriter *writer, FILE *f, int hDPI,
int vDPI) {
+SplashError SplashBitmap::writeImgFile(ImgWriter *writer, FILE *f, int hDPI,
int vDPI, GBool withAlpha)
+{
+ SplashError e;
+
if (mode != splashModeRGB8 && mode != splashModeMono8 && mode !=
splashModeMono1 && mode != splashModeXBGR8) {
error(-1, "unsupported SplashBitmap mode");
return splashErrGeneric;
@@ -324,7 +334,24 @@ SplashError SplashBitmap::writeImgFile(ImgWriter *writer,
FILE *f, int hDPI, int
if (!writer->init(f, width, height, hDPI, vDPI)) {
return splashErrGeneric;
}
+
+ if (!withAlpha) {
+ e = writeImgDataRGB(writer);
+ } else {
+ e = writeImgDataRGBA(writer);
+ }
+
+ if (e) return e;
+
+ if (writer->close()) {
+ return splashErrGeneric;
+ }
+
+ return splashOk;
+}
+SplashError SplashBitmap::writeImgDataRGB(ImgWriter *writer)
+{
switch (mode) {
case splashModeRGB8:
{
@@ -408,10 +435,93 @@ SplashError SplashBitmap::writeImgFile(ImgWriter *writer,
FILE *f, int hDPI, int
// can't happen
break;
}
-
- if (writer->close()) {
- return splashErrGeneric;
- }
+}
- return splashOk;
+SplashError SplashBitmap::writeImgDataRGBA(ImgWriter *writer)
+{
+ unsigned char *row = new unsigned char[4 * width];
+
+ switch (mode) {
+ case splashModeRGB8:
+ {
+ for (int y = 0; y < height; y++) {
+ // Convert into a PNG row
+ for (int x = 0; x < width; x++) {
+ row[4*x] = data[y * rowSize + 3*x];
+ row[4*x+1] = data[y * rowSize + 3*x + 1];
+ row[4*x+2] = data[y * rowSize + 3*x + 2];
+ row[4*x+3] = alpha[y * width + x];
+ }
+
+ if (!writer->writeRow(&row)) {
+ delete[] row;
+ return splashErrGeneric;
+ }
+ }
+ }
+ break;
+
+ case splashModeXBGR8:
+ {
+ for (int y = 0; y < height; y++) {
+ // Convert into a PNG row
+ for (int x = 0; x < width; x++) {
+ row[4*x] = data[y * rowSize + x * 4 + 2];
+ row[4*x+1] = data[y * rowSize + x * 4 + 1];
+ row[4*x+2] = data[y * rowSize + x * 4];
+ row[4*x+3] = alpha[y * width + x];
+ }
+
+ if (!writer->writeRow(&row)) {
+ delete[] row;
+ return splashErrGeneric;
+ }
+ }
+ }
+ break;
+
+ case splashModeMono8:
+ {
+ for (int y = 0; y < height; y++) {
+ // Convert into a PNG row
+ for (int x = 0; x < width; x++) {
+ row[4*x] = data[y * rowSize + x];
+ row[4*x+1] = data[y * rowSize + x];
+ row[4*x+2] = data[y * rowSize + x];
+ row[4*x+3] = alpha[y * width + x];
+ }
+
+ if (!writer->writeRow(&row)) {
+ delete[] row;
+ return splashErrGeneric;
+ }
+ }
+ }
+ break;
+
+ case splashModeMono1:
+ {
+ for (int y = 0; y < height; y++) {
+ // Convert into a PNG row
+ for (int x = 0; x < width; x++) {
+ getPixel(x, y, &row[4*x]);
+ row[4*x+1] = row[4*x];
+ row[4*x+2] = row[4*x];
+ row[4*x+3] = alpha[y * width + x];
+ }
+
+ if (!writer->writeRow(&row)) {
+ delete[] row;
+ return splashErrGeneric;
+ }
+ }
+ }
+ break;
+
+ default:
+ // can't happen
+ break;
+ }
+
+ delete[] row;
}
diff --git a/splash/SplashBitmap.h b/splash/SplashBitmap.h
index e741a91..ec5b397 100644
--- a/splash/SplashBitmap.h
+++ b/splash/SplashBitmap.h
@@ -65,13 +65,16 @@ public:
SplashError writeImgFile(SplashImageFileFormat format, char *fileName, int
hDPI, int vDPI);
SplashError writeImgFile(SplashImageFileFormat format, FILE *f, int hDPI,
int vDPI);
- SplashError writeImgFile(ImgWriter *writer, FILE *f, int hDPI, int vDPI);
+ SplashError writeImgFile(ImgWriter *writer, FILE *f, int hDPI, int vDPI,
GBool withAlpha = gFalse);
void getPixel(int x, int y, SplashColorPtr pixel);
Guchar getAlpha(int x, int y);
private:
+ SplashError writeImgDataRGB(ImgWriter *writer);
+ SplashError writeImgDataRGBA(ImgWriter *writer);
+
int width, height; // size of bitmap
int rowSize; // size of one row of data, in bytes
// - negative for bottom-up bitmaps
diff --git a/splash/SplashTypes.h b/splash/SplashTypes.h
index 993dd46..3e34ea7 100644
--- a/splash/SplashTypes.h
+++ b/splash/SplashTypes.h
@@ -160,7 +160,8 @@ typedef int SplashError;
enum SplashImageFileFormat {
splashFormatJpeg,
- splashFormatPng
+ splashFormatPng,
+ splashFormatPngAlpha
};
#endif
--
1.7.0.4
>From 4bcd593b1d259d87bbda89ac00b04997f45d110f Mon Sep 17 00:00:00 2001
From: Stefan Thomas <tho...@txtbear.com>
Date: Fri, 16 Jul 2010 16:44:14 +0100
Subject: [PATCH 2/2] Fixes JPEG corruption problem in previous patch.
---
splash/SplashBitmap.cc | 19 +++++++++++++++----
1 files changed, 15 insertions(+), 4 deletions(-)
diff --git a/splash/SplashBitmap.cc b/splash/SplashBitmap.cc
index 8aca37d..e0fa788 100644
--- a/splash/SplashBitmap.cc
+++ b/splash/SplashBitmap.cc
@@ -342,10 +342,6 @@ SplashError SplashBitmap::writeImgFile(ImgWriter *writer,
FILE *f, int hDPI, int
}
if (e) return e;
-
- if (writer->close()) {
- return splashErrGeneric;
- }
return splashOk;
}
@@ -367,6 +363,9 @@ SplashError SplashBitmap::writeImgDataRGB(ImgWriter *writer)
delete[] row_pointers;
return splashErrGeneric;
}
+ if (!writer->close()) {
+ return splashErrGeneric;
+ }
delete[] row_pointers;
}
break;
@@ -387,6 +386,9 @@ SplashError SplashBitmap::writeImgDataRGB(ImgWriter *writer)
return splashErrGeneric;
}
}
+ if (!writer->close()) {
+ return splashErrGeneric;
+ }
delete[] row;
}
break;
@@ -407,6 +409,9 @@ SplashError SplashBitmap::writeImgDataRGB(ImgWriter *writer)
return splashErrGeneric;
}
}
+ if (!writer->close()) {
+ return splashErrGeneric;
+ }
delete[] row;
}
break;
@@ -427,6 +432,9 @@ SplashError SplashBitmap::writeImgDataRGB(ImgWriter *writer)
return splashErrGeneric;
}
}
+ if (!writer->close()) {
+ return splashErrGeneric;
+ }
delete[] row;
}
break;
@@ -523,5 +531,8 @@ SplashError SplashBitmap::writeImgDataRGBA(ImgWriter
*writer)
break;
}
+ if (!writer->close()) {
+ return splashErrGeneric;
+ }
delete[] row;
}
--
1.7.0.4
_______________________________________________ poppler mailing list poppler@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/poppler
