# HG changeset patch # User Sebastien Jodogne # Date 1619601759 -7200 # Node ID 69bbb4bd35cb394b9d57d766286d8a562930cd30 # Parent b02dc8303cf6c6d85e1119c950a391c11753df94 PngWriter recreates a new libpng context for each encoding diff -r b02dc8303cf6 -r 69bbb4bd35cb OrthancFramework/Sources/Images/PngWriter.cpp --- a/OrthancFramework/Sources/Images/PngWriter.cpp Tue Apr 27 09:49:20 2021 +0200 +++ b/OrthancFramework/Sources/Images/PngWriter.cpp Wed Apr 28 11:22:39 2021 +0200 @@ -62,14 +62,23 @@ static void WarningHandler(png_structp png, png_const_charp message) { - printf("++ %d\n", (int)message); +printf("++ %d\n", (int)message); }*/ namespace Orthanc { - struct PngWriter::PImpl + /** + * The "png_" structure cannot be safely reused if the bpp changes + * between successive invocations. This can lead to "Invalid reads" + * reported by valgrind if writing a 16bpp image, then a 8bpp image + * using the same "PngWriter" object. Starting with Orthanc 1.9.3, + * we recreate a new "png_" context each time a PNG image must be + * written so as to prevent such invalid reads. + **/ + class PngWriter::Context : public boost::noncopyable { + private: png_structp png_; png_infop info_; @@ -77,123 +86,127 @@ std::vector rows_; int bitDepth_; int colorType_; - }; - - - PngWriter::PngWriter() : pimpl_(new PImpl) - { - pimpl_->png_ = NULL; - pimpl_->info_ = NULL; - - pimpl_->png_ = png_create_write_struct - (PNG_LIBPNG_VER_STRING, NULL, NULL, NULL); //this, ErrorHandler, WarningHandler); - if (!pimpl_->png_) - { - throw OrthancException(ErrorCode_NotEnoughMemory); - } - - pimpl_->info_ = png_create_info_struct(pimpl_->png_); - if (!pimpl_->info_) - { - png_destroy_write_struct(&pimpl_->png_, NULL); - throw OrthancException(ErrorCode_NotEnoughMemory); - } - } - - PngWriter::~PngWriter() - { - if (pimpl_->info_) - { - png_destroy_info_struct(pimpl_->png_, &pimpl_->info_); - } - - if (pimpl_->png_) - { - png_destroy_write_struct(&pimpl_->png_, NULL); - } - } - - - - void PngWriter::Prepare(unsigned int width, - unsigned int height, - unsigned int pitch, - PixelFormat format, - const void* buffer) - { - pimpl_->rows_.resize(height); - for (unsigned int y = 0; y < height; y++) - { - pimpl_->rows_[y] = const_cast(reinterpret_cast(buffer)) + y * pitch; - } - - switch (format) + public: + Context() : + png_(NULL), + info_(NULL) { - case PixelFormat_RGB24: - pimpl_->bitDepth_ = 8; - pimpl_->colorType_ = PNG_COLOR_TYPE_RGB; - break; - - case PixelFormat_RGBA32: - pimpl_->bitDepth_ = 8; - pimpl_->colorType_ = PNG_COLOR_TYPE_RGBA; - break; - - case PixelFormat_Grayscale8: - pimpl_->bitDepth_ = 8; - pimpl_->colorType_ = PNG_COLOR_TYPE_GRAY; - break; - - case PixelFormat_Grayscale16: - case PixelFormat_SignedGrayscale16: - pimpl_->bitDepth_ = 16; - pimpl_->colorType_ = PNG_COLOR_TYPE_GRAY; - break; - - default: - throw OrthancException(ErrorCode_NotImplemented); - } - } - - - void PngWriter::Compress(unsigned int width, - unsigned int height, - unsigned int pitch, - PixelFormat format) - { - png_set_IHDR(pimpl_->png_, pimpl_->info_, width, height, - pimpl_->bitDepth_, pimpl_->colorType_, PNG_INTERLACE_NONE, - PNG_COMPRESSION_TYPE_BASE, PNG_FILTER_TYPE_BASE); - - png_write_info(pimpl_->png_, pimpl_->info_); - - if (height > 0) - { - switch (format) + png_ = png_create_write_struct + (PNG_LIBPNG_VER_STRING, NULL, NULL, NULL); //this, ErrorHandler, WarningHandler); + if (!png_) { - case PixelFormat_Grayscale16: - case PixelFormat_SignedGrayscale16: - { - int transforms = 0; - if (Toolbox::DetectEndianness() == Endianness_Little) - { - transforms = PNG_TRANSFORM_SWAP_ENDIAN; - } - - png_set_rows(pimpl_->png_, pimpl_->info_, &pimpl_->rows_[0]); - png_write_png(pimpl_->png_, pimpl_->info_, transforms, NULL); - - break; + throw OrthancException(ErrorCode_NotEnoughMemory); } - default: - png_write_image(pimpl_->png_, &pimpl_->rows_[0]); + info_ = png_create_info_struct(png_); + if (!info_) + { + png_destroy_write_struct(&png_, NULL); + throw OrthancException(ErrorCode_NotEnoughMemory); } } - png_write_end(pimpl_->png_, NULL); - } + + ~Context() + { + if (info_) + { + png_destroy_info_struct(png_, &info_); + } + + if (png_) + { + png_destroy_write_struct(&png_, NULL); + } + } + + + png_structp GetObject() const + { + return png_; + } + + + void Prepare(unsigned int width, + unsigned int height, + unsigned int pitch, + PixelFormat format, + const void* buffer) + { + rows_.resize(height); + for (unsigned int y = 0; y < height; y++) + { + rows_[y] = const_cast(reinterpret_cast(buffer)) + y * pitch; + } + + switch (format) + { + case PixelFormat_RGB24: + bitDepth_ = 8; + colorType_ = PNG_COLOR_TYPE_RGB; + break; + + case PixelFormat_RGBA32: + bitDepth_ = 8; + colorType_ = PNG_COLOR_TYPE_RGBA; + break; + + case PixelFormat_Grayscale8: + bitDepth_ = 8; + colorType_ = PNG_COLOR_TYPE_GRAY; + break; + + case PixelFormat_Grayscale16: + case PixelFormat_SignedGrayscale16: + bitDepth_ = 16; + colorType_ = PNG_COLOR_TYPE_GRAY; + break; + + default: + throw OrthancException(ErrorCode_NotImplemented); + } + } + + + void Compress(unsigned int width, + unsigned int height, + unsigned int pitch, + PixelFormat format) + { + png_set_IHDR(png_, info_, width, height, + bitDepth_, colorType_, PNG_INTERLACE_NONE, + PNG_COMPRESSION_TYPE_BASE, PNG_FILTER_TYPE_BASE); + + png_write_info(png_, info_); + + if (height > 0) + { + switch (format) + { + case PixelFormat_Grayscale16: + case PixelFormat_SignedGrayscale16: + { + int transforms = 0; + if (Toolbox::DetectEndianness() == Endianness_Little) + { + transforms = PNG_TRANSFORM_SWAP_ENDIAN; + } + + png_set_rows(png_, info_, &rows_[0]); + png_write_png(png_, info_, transforms, NULL); + + break; + } + + default: + png_write_image(png_, &rows_[0]); + } + } + + png_write_end(png_, NULL); + } + }; #if ORTHANC_SANDBOXED == 0 @@ -204,7 +217,9 @@ PixelFormat format, const void* buffer) { - Prepare(width, height, pitch, format, buffer); + Context context; + + context.Prepare(width, height, pitch, format, buffer); FILE* fp = SystemToolbox::OpenFile(filename, FileMode_WriteBinary); if (!fp) @@ -212,15 +227,15 @@ throw OrthancException(ErrorCode_CannotWriteFile); } - png_init_io(pimpl_->png_, fp); + png_init_io(context.GetObject(), fp); - if (setjmp(png_jmpbuf(pimpl_->png_))) + if (setjmp(png_jmpbuf(context.GetObject()))) { // Error during writing PNG throw OrthancException(ErrorCode_CannotWriteFile); } - Compress(width, height, pitch, format); + context.Compress(width, height, pitch, format); fclose(fp); } @@ -236,7 +251,6 @@ } - void PngWriter::WriteToMemoryInternal(std::string& png, unsigned int width, unsigned int height, @@ -244,19 +258,21 @@ PixelFormat format, const void* buffer) { + Context context; + ChunkedBuffer chunks; - Prepare(width, height, pitch, format, buffer); + context.Prepare(width, height, pitch, format, buffer); - if (setjmp(png_jmpbuf(pimpl_->png_))) + if (setjmp(png_jmpbuf(context.GetObject()))) { // Error during writing PNG throw OrthancException(ErrorCode_InternalError); } - png_set_write_fn(pimpl_->png_, &chunks, MemoryCallback, NULL); + png_set_write_fn(context.GetObject(), &chunks, MemoryCallback, NULL); - Compress(width, height, pitch, format); + context.Compress(width, height, pitch, format); chunks.Flatten(png); } diff -r b02dc8303cf6 -r 69bbb4bd35cb OrthancFramework/Sources/Images/PngWriter.h --- a/OrthancFramework/Sources/Images/PngWriter.h Tue Apr 27 09:49:20 2021 +0200 +++ b/OrthancFramework/Sources/Images/PngWriter.h Wed Apr 28 11:22:39 2021 +0200 @@ -39,6 +39,9 @@ { class ORTHANC_PUBLIC PngWriter : public IImageWriter { + private: + class Context; + protected: #if ORTHANC_SANDBOXED == 0 virtual void WriteToFileInternal(const std::string& filename, @@ -55,25 +58,5 @@ unsigned int pitch, PixelFormat format, const void* buffer) ORTHANC_OVERRIDE; - - private: - struct PImpl; - boost::shared_ptr pimpl_; - - void Compress(unsigned int width, - unsigned int height, - unsigned int pitch, - PixelFormat format); - - void Prepare(unsigned int width, - unsigned int height, - unsigned int pitch, - PixelFormat format, - const void* buffer); - - public: - PngWriter(); - - ~PngWriter(); }; } diff -r b02dc8303cf6 -r 69bbb4bd35cb OrthancFramework/UnitTestsSources/ImageTests.cpp --- a/OrthancFramework/UnitTestsSources/ImageTests.cpp Tue Apr 27 09:49:20 2021 +0200 +++ b/OrthancFramework/UnitTestsSources/ImageTests.cpp Wed Apr 28 11:22:39 2021 +0200 @@ -521,3 +521,42 @@ } #endif } + + +TEST(PngWriter, Gray16Then8) +{ + Orthanc::Image image16(Orthanc::PixelFormat_Grayscale16, 32, 32, false); + Orthanc::Image image8(Orthanc::PixelFormat_Grayscale8, 32, 32, false); + + memset(image16.GetBuffer(), 0, image16.GetHeight() * image16.GetPitch()); + memset(image8.GetBuffer(), 0, image8.GetHeight() * image8.GetPitch()); + + { + Orthanc::PamWriter w; + std::string s; + Orthanc::IImageWriter::WriteToMemory(w, s, image16); + Orthanc::IImageWriter::WriteToMemory(w, s, image8); // No problem here + } + + { + Orthanc::PamWriter w; + std::string s; + Orthanc::IImageWriter::WriteToMemory(w, s, image8); + Orthanc::IImageWriter::WriteToMemory(w, s, image16); // No problem here + } + + { + Orthanc::PngWriter w; + std::string s; + Orthanc::IImageWriter::WriteToMemory(w, s, image8); + Orthanc::IImageWriter::WriteToMemory(w, s, image16); // No problem here + } + + { + // The following call leads to "Invalid read of size 1" in Orthanc <= 1.9.2 + Orthanc::PngWriter w; + std::string s; + Orthanc::IImageWriter::WriteToMemory(w, s, image16); + Orthanc::IImageWriter::WriteToMemory(w, s, image8); // Problem here + } +}