Merge pull request #10245 from Pokechu22/libpng-errors

Fix saving RGBA images

Source/Core/Common/Image.cpp

@@ -8,6 +8,7 @@
 
 #include <png.h>
 
+#include "Common/Assert.h"
 #include "Common/CommonTypes.h"
 #include "Common/IOFile.h"
 #include "Common/ImageC.h"
@@ -48,6 +49,18 @@ static void WriteCallback(png_structp png_ptr, png_bytep data, size_t length)
   buffer->insert(buffer->end(), data, data + length);
 }
 
+static void ErrorCallback(ErrorHandler* self, const char* msg)
+{
+  std::vector<std::string>* errors = static_cast<std::vector<std::string>*>(self->error_list);
+  errors->emplace_back(msg);
+}
+
+static void WarningCallback(ErrorHandler* self, const char* msg)
+{
+  std::vector<std::string>* warnings = static_cast<std::vector<std::string>*>(self->warning_list);
+  warnings->emplace_back(msg);
+}
+
 bool SavePNG(const std::string& path, const u8* input, ImageByteFormat format, u32 width,
              u32 height, int stride, int level)
 {
@@ -55,18 +68,19 @@ bool SavePNG(const std::string& path, const u8* input, ImageByteFormat format, u
   timer.Start();
 
   size_t byte_per_pixel;
-  int png_format;
+  int color_type;
   switch (format)
   {
   case ImageByteFormat::RGB:
-    png_format = PNG_FORMAT_RGB;
+    color_type = PNG_COLOR_TYPE_RGB;
     byte_per_pixel = 3;
     break;
   case ImageByteFormat::RGBA:
-    png_format = PNG_FORMAT_RGBA;
+    color_type = PNG_COLOR_TYPE_RGBA;
     byte_per_pixel = 4;
     break;
   default:
+    ASSERT_MSG(FRAMEDUMP, false, "Invalid format %d", static_cast<int>(format));
     return false;
   }
 
@@ -75,6 +89,14 @@ bool SavePNG(const std::string& path, const u8* input, ImageByteFormat format, u
   std::vector<u8> buffer;
   buffer.reserve(byte_per_pixel * width * height);
 
+  std::vector<std::string> warnings;
+  std::vector<std::string> errors;
+  ErrorHandler error_handler;
+  error_handler.error_list = &errors;
+  error_handler.warning_list = &warnings;
+  error_handler.StoreError = ErrorCallback;
+  error_handler.StoreWarning = WarningCallback;
+
   std::vector<const u8*> rows;
   rows.reserve(height);
   for (u32 row = 0; row < height; row++)
@@ -82,13 +104,14 @@ bool SavePNG(const std::string& path, const u8* input, ImageByteFormat format, u
     rows.push_back(&input[row * stride]);
   }
 
-  png_structp png_ptr = png_create_write_struct(PNG_LIBPNG_VER_STRING, 0, 0, 0);
+  png_structp png_ptr =
+      png_create_write_struct(PNG_LIBPNG_VER_STRING, &error_handler, PngError, PngWarning);
   png_infop info_ptr = png_create_info_struct(png_ptr);
 
   bool success = false;
   if (png_ptr != nullptr && info_ptr != nullptr)
   {
-    success = SavePNG0(png_ptr, info_ptr, png_format, width, height, level, &buffer, WriteCallback,
+    success = SavePNG0(png_ptr, info_ptr, color_type, width, height, level, &buffer, WriteCallback,
                        const_cast<u8**>(rows.data()));
   }
   png_destroy_write_struct(&png_ptr, &info_ptr);
@@ -103,6 +126,23 @@ bool SavePNG(const std::string& path, const u8* input, ImageByteFormat format, u
     timer.Stop();
     INFO_LOG_FMT(FRAMEDUMP, "{} byte {} by {} image saved to {} at level {} in {}", buffer.size(),
                  width, height, path, level, timer.GetTimeElapsedFormatted());
+    ASSERT(errors.size() == 0);
+    if (warnings.size() != 0)
+    {
+      WARN_LOG_FMT(FRAMEDUMP, "Saved with {} warnings:", warnings.size());
+      for (auto& warning : warnings)
+        WARN_LOG_FMT(FRAMEDUMP, "libpng warning: {}", warning);
+    }
+  }
+  else
+  {
+    ERROR_LOG_FMT(FRAMEDUMP,
+                  "Failed to save {} by {} image to {} at level {}: {} warnings, {} errors", width,
+                  height, path, level, warnings.size(), errors.size());
+    for (auto& error : errors)
+      ERROR_LOG_FMT(FRAMEDUMP, "libpng error: {}", error);
+    for (auto& warning : warnings)
+      WARN_LOG_FMT(FRAMEDUMP, "libpng warning: {}", warning);
   }
 
   return success;

Source/Core/Common/ImageC.c

@@ -9,7 +9,7 @@
 // The main purpose of this function is to allow specifying the compression level, which
 // png_image_write_to_memory does not allow.  row_pointers is not modified by libpng, but also isn't
 // const for some reason.
-bool SavePNG0(png_structp png_ptr, png_infop info_ptr, int png_format, png_uint_32 width,
+bool SavePNG0(png_structp png_ptr, png_infop info_ptr, int color_type, png_uint_32 width,
               png_uint_32 height, int level, png_voidp io_ptr, png_rw_ptr write_fn,
               png_bytepp row_pointers)
 {
@@ -17,7 +17,7 @@ bool SavePNG0(png_structp png_ptr, png_infop info_ptr, int png_format, png_uint_
     return false;
 
   png_set_compression_level(png_ptr, level);
-  png_set_IHDR(png_ptr, info_ptr, width, height, 8, png_format, PNG_INTERLACE_NONE,
+  png_set_IHDR(png_ptr, info_ptr, width, height, 8, color_type, PNG_INTERLACE_NONE,
                PNG_COMPRESSION_TYPE_DEFAULT, PNG_FILTER_TYPE_DEFAULT);
   png_set_rows(png_ptr, info_ptr, row_pointers);
   png_set_write_fn(png_ptr, io_ptr, write_fn, NULL);
@@ -25,3 +25,19 @@ bool SavePNG0(png_structp png_ptr, png_infop info_ptr, int png_format, png_uint_
 
   return true;
 }
+
+// pngerror.c says: "Note that the error function MUST NOT return to the calling routine or serious
+// problems will occur.  The return method used in the default routine calls
+// longjmp(png_ptr->jmp_buf_ptr, 1)"
+void PngError(png_structp png_ptr, png_const_charp msg)
+{
+  struct ErrorHandler* error_logger = (struct ErrorHandler*)png_get_error_ptr(png_ptr);
+  error_logger->StoreError(error_logger, msg);
+  png_longjmp(png_ptr, 1);
+}
+
+void PngWarning(png_structp png_ptr, png_const_charp msg)
+{
+  struct ErrorHandler* error_logger = (struct ErrorHandler*)png_get_error_ptr(png_ptr);
+  error_logger->StoreWarning(error_logger, msg);
+}

Source/Core/Common/ImageC.h

@@ -7,9 +7,23 @@
 #include <stdbool.h>
 
 #ifdef __cplusplus
-extern "C"
+extern "C" {
+#endif
+struct ErrorHandler
+{
+  void* error_list;    // std::vector<std::string>*
+  void* warning_list;  // std::vector<std::string>*
+  void (*StoreError)(struct ErrorHandler* self, const char* msg);
+  void (*StoreWarning)(struct ErrorHandler* self, const char* msg);
+};
+
+bool SavePNG0(png_structp png_ptr, png_infop info_ptr, int color_type, png_uint_32 width,
+              png_uint_32 height, int level, png_voidp io_ptr, png_rw_ptr write_fn,
+              png_bytepp row_pointers);
+
+void PngError(png_structp png_ptr, png_const_charp msg);
+void PngWarning(png_structp png_ptr, png_const_charp msg);
+
+#ifdef __cplusplus
+}
 #endif
-    bool
-    SavePNG0(png_structp png_ptr, png_infop info_ptr, int png_format, png_uint_32 width,
-             png_uint_32 height, int level, png_voidp io_ptr, png_rw_ptr write_fn,
-             png_bytepp row_pointers);