Mercurial > hg > orthanc

changeset 4642:69bbb4bd35cb

Find changesets by keywords (author, files, the commit message), revision number or hash, or revset expression.
PngWriter recreates a new libpng context for each encoding
author Sebastien Jodogne <s.jodogne@gmail.com>
date Wed, 28 Apr 2021 11:22:39 +0200
parents b02dc8303cf6
children bd0ba9ff0efa
files OrthancFramework/Sources/Images/PngWriter.cpp OrthancFramework/Sources/Images/PngWriter.h OrthancFramework/UnitTestsSources/ImageTests.cpp
diffstat 3 files changed, 178 insertions(+), 140 deletions(-) [+]
line wrap: on
line diff
--- 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<uint8_t*> 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<uint8_t*>(reinterpret_cast<const uint8_t*>(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<uint8_t*>(reinterpret_cast<const uint8_t*>(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);
   }
--- 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> 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();
   };
 }
--- 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
+  }  
+}