Improved palette handling with saving PNG files.

Instead of converting alpha to a mask an attempt is made to write a palettised PNG file with an ARGB palette using a maximum of 256 transparency values where formerly just up to one was supported. GIF images with 256 colours and transparency can now also be saved as a palettised PNG instead of true colour, making the image a lot smaller.

Applied (modified) patch by troelsk. Closes #12850.



git-svn-id: https://svn.wxwidgets.org/svn/wx/wxWidgets/trunk@67101 c3d73ce0-8a6f-49c7-b76d-6d57e0e08775

src/common/imagpng.cpp

@@ -646,27 +646,42 @@ error:
 }
 
 // ----------------------------------------------------------------------------
-// SaveFile() helpers
+// SaveFile() palette helpers
 // ----------------------------------------------------------------------------
 
-#if wxUSE_PALETTE
+typedef wxLongToLongHashMap PaletteMap;
 
-static int PaletteFind(const png_color& clr, const png_color *pal, int palCount)
+static unsigned long PaletteMakeKey(const png_color_8& clr)
 {
-   for (int i = 0; i < palCount; ++i)
+    return (wxImageHistogram::MakeKey(clr.red, clr.green, clr.blue) << 8) | clr.alpha;
-   {
-      if (    (clr.red   == pal[i].red)
-           && (clr.green == pal[i].green)
-           && (clr.blue  == pal[i].blue))
-      {
-         return i;
-      }
-   }
-
-   return wxNOT_FOUND;
 }
 
-#endif // wxUSE_PALETTE
+static long PaletteFind(const PaletteMap& palette, const png_color_8& clr)
+{
+    unsigned long value = PaletteMakeKey(clr);
+    PaletteMap::const_iterator it = palette.find(value);
+
+    return (it != palette.end()) ? it->second : wxNOT_FOUND;
+}
+
+static long PaletteAdd(PaletteMap *palette, const png_color_8& clr)
+{
+    unsigned long value = PaletteMakeKey(clr);
+    PaletteMap::const_iterator it = palette->find(value);
+    size_t index;
+
+    if (it == palette->end())
+    {
+        index = palette->size();
+        (*palette)[value] = index;
+    }
+    else
+    {
+        index = it->second;
+    }
+
+    return index;
+}
 
 // ----------------------------------------------------------------------------
 // writing PNGs
@@ -720,6 +735,9 @@ bool wxPNGHandler::SaveFile( wxImage *image, wxOutputStream& stream, bool verbos
     //     explanation why this line is mandatory
     png_set_write_fn( png_ptr, &wxinfo, wx_PNG_stream_writer, NULL);
 
+    const int iHeight = image->GetHeight();
+    const int iWidth = image->GetWidth();
+
     const bool bHasPngFormatOption
         = image->HasOption(wxIMAGE_OPTION_PNG_FORMAT);
 
@@ -730,49 +748,86 @@ bool wxPNGHandler::SaveFile( wxImage *image, wxOutputStream& stream, bool verbos
     bool bHasAlpha = image->HasAlpha();
     bool bHasMask = image->HasMask();
 
+    bool bUsePalette = iColorType == wxPNG_TYPE_PALETTE
 #if wxUSE_PALETTE
-    /*
+        || (!bHasPngFormatOption && image->HasPalette() )
-    Only save as an indexed image if the number of palette entries does not
+#endif
-    exceed libpng's limit (256).
+    ;
-    We assume here that we will need an extra palette entry if there's an
-    alpha or mask, regardless of whether a possibly needed conversion from
-    alpha to a mask fails (unlikely), or whether the mask colour already
-    can be found in the palette (more likely). In the latter case an extra
-    palette entry would not be required later on and the image could actually
-    be saved as a palettised PNG (instead now it will be saved as true colour).
-    A little bit of precision is lost, but at the benefit of a lot more
-    simplified code.
-    */
-    bool bUsePalette =
-        (!bHasPngFormatOption || iColorType == wxPNG_TYPE_PALETTE)
-        && image->HasPalette()
-        && image->GetPalette().GetColoursCount()
-            + ((bHasAlpha || bHasMask) ? 1 : 0) <= PNG_MAX_PALETTE_LENGTH;
 
-    wxImage temp_image(*image);
+    png_color_8 mask;
-    if (bUsePalette && image->HasAlpha() && !bHasMask)
+
+    if (bHasMask)
     {
-        /*
+        mask.red   = image->GetMaskRed();
-        Only convert alpha to mask if saving as a palettised image was
+        mask.green = image->GetMaskGreen();
-        explicitly requested. We don't want to lose alpha's precision
+        mask.blue  = image->GetMaskBlue();
-        by converting to a mask just to be able to save palettised.
+        mask.alpha = 0;
-        */
+        mask.gray  = 0;
-        if (iColorType == wxPNG_TYPE_PALETTE
+    }
-            && temp_image.ConvertAlphaToMask())
+
+    PaletteMap palette;
+
+    if (bUsePalette)
+    {
+        png_color png_rgb  [PNG_MAX_PALETTE_LENGTH];
+        png_byte  png_trans[PNG_MAX_PALETTE_LENGTH];
+
+        const unsigned char *pColors = image->GetData();
+        const unsigned char* pAlpha  = image->GetAlpha();
+
+        if (bHasMask && !pAlpha)
         {
-            image = &temp_image;
+            // Mask must be first
-            bHasMask = true;
+            PaletteAdd(&palette, mask);
-            bHasAlpha = image->HasAlpha();
         }
-        else
+
+        for (int y = 0; y < iHeight; y++)
         {
-            bUsePalette = false;
+            for (int x = 0; x < iWidth; x++)
-            iColorType = wxPNG_TYPE_COLOUR;
+            {
+                png_color_8 rgba;
+
+                rgba.red   = *pColors++;
+                rgba.green = *pColors++;
+                rgba.blue  = *pColors++;
+                rgba.gray  = 0;
+                rgba.alpha = (pAlpha && !bHasMask) ? *pAlpha++ : 0;
+
+                // save in our palette
+                long index = PaletteAdd(&palette, rgba);
+
+                if (index < PNG_MAX_PALETTE_LENGTH)
+                {
+                    // save in libpng's palette
+                    png_rgb[index].red   = rgba.red;
+                    png_rgb[index].green = rgba.green;
+                    png_rgb[index].blue  = rgba.blue;
+                    png_trans[index]     = rgba.alpha;
+                }
+                else
+                {
+                    bUsePalette = false;
+                    break;
+                }
+            }
+        }
+
+        if (bUsePalette)
+        {
+            png_set_PLTE(png_ptr, info_ptr, png_rgb, palette.size());
+
+            if (bHasMask && !pAlpha)
+            {
+                wxASSERT(PaletteFind(palette, mask) == 0);
+                png_trans[0] = 0;
+                png_set_tRNS(png_ptr, info_ptr, png_trans, 1, NULL);
+            }
+            else if (pAlpha && !bHasMask)
+            {
+                png_set_tRNS(png_ptr, info_ptr, png_trans, palette.size(), NULL);
+            }
         }
     }
-#else
-    bool bUsePalette = false;
-#endif // wxUSE_PALETTE
 
     /*
     If saving palettised was requested but it was decided we can't use a
@@ -785,26 +840,14 @@ bool wxPNGHandler::SaveFile( wxImage *image, wxOutputStream& stream, bool verbos
 
     bool bUseAlpha = !bUsePalette && (bHasAlpha || bHasMask);
 
-    png_color mask;
-    if (bHasMask)
-    {
-        mask.red   = image->GetMaskRed();
-        mask.green = image->GetMaskGreen();
-        mask.blue  = image->GetMaskBlue();
-    }
-
-
     int iPngColorType;
 
-#if wxUSE_PALETTE
     if (bUsePalette)
     {
         iPngColorType = PNG_COLOR_TYPE_PALETTE;
         iColorType = wxPNG_TYPE_PALETTE;
     }
-    else
+    else if ( iColorType==wxPNG_TYPE_COLOUR )
-#endif // wxUSE_PALETTE
-    if ( iColorType==wxPNG_TYPE_COLOUR )
     {
         iPngColorType = bUseAlpha ? PNG_COLOR_TYPE_RGB_ALPHA
                                   : PNG_COLOR_TYPE_RGB;
@@ -839,62 +882,6 @@ bool wxPNGHandler::SaveFile( wxImage *image, wxOutputStream& stream, bool verbos
                   PNG_INTERLACE_NONE, PNG_COMPRESSION_TYPE_BASE,
                   PNG_FILTER_TYPE_BASE);
 
-#if wxUSE_PALETTE
-    png_colorp palette = NULL;
-    int numPalette = 0;
-
-    if (bUsePalette)
-    {
-        const wxPalette& pal = image->GetPalette();
-        const int palCount = pal.GetColoursCount();
-        palette = (png_colorp) malloc(
-            (palCount + 1 /*headroom for trans */) * sizeof(png_color));
-
-        if (!palette)
-        {
-            png_destroy_write_struct( &png_ptr, (png_infopp)NULL );
-            if (verbose)
-            {
-               wxLogError(_("Couldn't save PNG image."));
-            }
-            return false;
-        }
-
-        for (int i = 0; i < palCount; ++i)
-        {
-            pal.GetRGB(i, &palette[i].red, &palette[i].green, &palette[i].blue);
-        }
-
-        numPalette = palCount;
-        if (bHasMask)
-        {
-            int index = PaletteFind(mask, palette, numPalette);
-
-            if (index)
-            {
-                if (index == wxNOT_FOUND)
-                {
-                    numPalette++;
-                    index = palCount;
-                    palette[index] = mask;
-                }
-
-                wxSwap(palette[0], palette[index]);
-            }
-
-            png_byte trans = 0;
-            png_set_tRNS(png_ptr, info_ptr, &trans, 1, NULL);
-        }
-
-        png_set_PLTE(png_ptr, info_ptr, palette, numPalette);
-        free (palette);
-        palette = NULL;
-
-        // Let palette point to libpng's copy of the palette.
-        (void) png_get_PLTE(png_ptr, info_ptr, &palette, &numPalette);
-    }
-#endif // wxUSE_PALETTE
-
     int iElements;
     png_color_8 sig_bit;
 
@@ -911,7 +898,7 @@ bool wxPNGHandler::SaveFile( wxImage *image, wxOutputStream& stream, bool verbos
         iElements = 1;
     }
 
-    if ( iPngColorType & PNG_COLOR_MASK_ALPHA )
+    if ( bUseAlpha )
     {
         sig_bit.alpha = (png_byte)iBitDepth;
         iElements++;
@@ -960,22 +947,22 @@ bool wxPNGHandler::SaveFile( wxImage *image, wxOutputStream& stream, bool verbos
         return false;
     }
 
-    unsigned char *
+    const unsigned char *
-        pAlpha = (unsigned char *)(bHasAlpha ? image->GetAlpha() : NULL);
+        pAlpha = (const unsigned char *)(bHasAlpha ? image->GetAlpha() : NULL);
-    int iHeight = image->GetHeight();
-    int iWidth = image->GetWidth();
 
-    unsigned char *pColors = image->GetData();
+    const unsigned char *pColors = image->GetData();
 
     for (int y = 0; y != iHeight; ++y)
     {
         unsigned char *pData = data;
         for (int x = 0; x != iWidth; x++)
         {
-            png_color clr;
+            png_color_8 clr;
             clr.red   = *pColors++;
             clr.green = *pColors++;
             clr.blue  = *pColors++;
+            clr.gray  = 0;
+            clr.alpha = (bUsePalette && pAlpha) ? *pAlpha++ : 0; // use with wxPNG_TYPE_PALETTE only
 
             switch ( iColorType )
             {
@@ -1016,12 +1003,9 @@ bool wxPNGHandler::SaveFile( wxImage *image, wxOutputStream& stream, bool verbos
                         *pData++ = 0;
                     break;
 
-#if wxUSE_PALETTE
                 case wxPNG_TYPE_PALETTE:
-                    *pData++ = (unsigned char) PaletteFind(clr,
+                    *pData++ = (unsigned char) PaletteFind(palette, clr);
-                        palette, numPalette);
                     break;
-#endif // wxUSE_PALETTE
             }
 
             if ( bUseAlpha )

tests/image/image.cpp

@@ -71,6 +71,7 @@ private:
         CPPUNIT_TEST( SizeImage );
         CPPUNIT_TEST( CompareLoadedImage );
         CPPUNIT_TEST( CompareSavedImage );
+        CPPUNIT_TEST( SavePNG );
         CPPUNIT_TEST( SaveAnimatedGIF );
         CPPUNIT_TEST( ReadCorruptedTGA );
         CPPUNIT_TEST( GIFComment );
@@ -82,6 +83,7 @@ private:
     void SizeImage();
     void CompareLoadedImage();
     void CompareSavedImage();
+    void SavePNG();
     void SaveAnimatedGIF();
     void ReadCorruptedTGA();
     void GIFComment();
@@ -1023,77 +1025,78 @@ void ImageTestCase::CompareSavedImage()
         CompareImage(*handler, expected24);
         CompareImage(*handler, expected32, wxIMAGE_HAVE_ALPHA);
     }
+}
+
+void ImageTestCase::SavePNG()
+{
+    wxImage expected24("horse.png");
+    CPPUNIT_ASSERT( expected24.IsOk() );
+#if wxUSE_PALETTE
+    CPPUNIT_ASSERT( !expected24.HasPalette() );
+#endif // #if wxUSE_PALETTE
+
+    wxImage expected8 = expected24.ConvertToGreyscale();
+
+    /*
+    horse.png converted to greyscale should be saved without a palette.
+    */
+    CompareImage(*wxImage::FindHandler(wxBITMAP_TYPE_PNG), expected8);
+
+    /*
+    But if we explicitly ask for trying to save with a palette, it should work.
+    */
+    expected8.SetOption(wxIMAGE_OPTION_PNG_FORMAT, wxPNG_TYPE_PALETTE);
+
+    CompareImage(*wxImage::FindHandler(wxBITMAP_TYPE_PNG),
+        expected8, wxIMAGE_HAVE_PALETTE);
 
 
-    expected8.LoadFile("horse.gif");
+    CPPUNIT_ASSERT( expected8.LoadFile("horse.gif") );
-    CPPUNIT_ASSERT( expected8.IsOk() );
 #if wxUSE_PALETTE
     CPPUNIT_ASSERT( expected8.HasPalette() );
 #endif // #if wxUSE_PALETTE
 
+    CompareImage(*wxImage::FindHandler(wxBITMAP_TYPE_PNG),
+        expected8, wxIMAGE_HAVE_PALETTE);
+
+    /*
+    Add alpha to the image in such a way that there will still be a maximum
+    of 256 unique RGBA combinations. This should result in a saved
+    PNG image still being palettised and having alpha.
+    */
     expected8.SetAlpha();
 
-    width = expected8.GetWidth();
+    int x, y;
-    height = expected8.GetHeight();
+    const int width = expected8.GetWidth();
+    const int height = expected8.GetHeight();
     for (y = 0; y < height; ++y)
     {
         for (x = 0; x < width; ++x)
         {
-            expected8.SetAlpha(x, y, (x*y) & wxIMAGE_ALPHA_OPAQUE);
+            expected8.SetAlpha(x, y, expected8.GetRed(x, y));
         }
     }
 
-    /*
+    CompareImage(*wxImage::FindHandler(wxBITMAP_TYPE_PNG),
-    Explicitly make known we want a palettised PNG. If we don't then this
+        expected8, wxIMAGE_HAVE_ALPHA|wxIMAGE_HAVE_PALETTE);
-    particular image gets saved as a true colour image because there's an
-    alpha channel present and the PNG saver prefers to keep the alpha over
-    saving as a palettised image that has alpha converted to a mask.
-    */
-    expected8.SetOption(wxIMAGE_OPTION_PNG_FORMAT, wxPNG_TYPE_PALETTE);
 
     /*
-    The image contains 256 indexed colours and needs another palette entry
+    Now change the alpha of the first pixel so that we can't save palettised
-    for storing the transparency index. This results in wanting 257 palette
+    anymore because there will be 256+1 entries which is beyond PNGs limit
-    entries but that amount is not supported by PNG, as such this image
+    of 256 entries.
-    should not contain a palette (but still have alpha) and be stored as a
-    true colour image instead.
     */
+    expected8.SetAlpha(0, 0, 1);
+
     CompareImage(*wxImage::FindHandler(wxBITMAP_TYPE_PNG),
         expected8, wxIMAGE_HAVE_ALPHA);
 
-#if wxUSE_PALETTE
     /*
-    Now do the same test again but remove one (random) palette entry. This
+    Even if we explicitly ask for saving palettised it should not be done.
-    should result in saving the PNG with a palette.
     */
-    unsigned char red[256], green[256], blue[256];
+    expected8.SetOption(wxIMAGE_OPTION_PNG_FORMAT, wxPNG_TYPE_PALETTE);
-    const wxPalette& pal = expected8.GetPalette();
-    const int paletteCount = pal.GetColoursCount();
-    for (i = 0; i < paletteCount; ++i)
-    {
-        expected8.GetPalette().GetRGB(i, &red[i], &green[i], &blue[i]);
-    }
-    wxPalette newPal(paletteCount - 1, red, green, blue);
-    expected8.Replace(
-        red[paletteCount-1], green[paletteCount-1], blue[paletteCount-1],
-        red[paletteCount-2], green[paletteCount-2], blue[paletteCount-2]);
-
-    expected8.SetPalette(newPal);
-
-    wxImage ref8 = expected8;
-
-    /*
-    Convert the alpha channel to a mask like the PNG saver does. Also convert
-    the colour used for transparency from 1,0,0 to 2,0,0. The latter gets
-    done by the PNG loader in search of an unused colour to use for
-    transparency (this should be fixed).
-    */
-    ref8.ConvertAlphaToMask();
-    ref8.Replace(1, 0, 0,  2, 0, 0);
-
     CompareImage(*wxImage::FindHandler(wxBITMAP_TYPE_PNG),
-        expected8, wxIMAGE_HAVE_PALETTE, &ref8);
+        expected8, wxIMAGE_HAVE_ALPHA);
-#endif
+
 }
 
 void ImageTestCase::SaveAnimatedGIF()