bfix: Fix base64 PNG rendering (#60)

* Fix base64 PNG rendering. Enable the full test, which now passes.

* Fix some potential leaks in AGGRenderer

* [AGG] Try removing the newlines from the base64 encoded string.

* Tidy up CPPAggRenderer a little

Author: karwa

Sources/AGGRenderer/AGGRenderer/AGGRenderer.swift

@@ -7,10 +7,11 @@ public class AGGRenderer: Renderer{
     public var offset = zeroPoint
     public var imageSize: Size {
         willSet {
-            agg_object = initializePlot(newValue.width, newValue.height, fontPath)
+          delete_plot(agg_object);
+          agg_object = initializePlot(newValue.width, newValue.height, fontPath)
         }
     }
-    var agg_object: UnsafeRawPointer
+    var agg_object: UnsafeMutableRawPointer
     var fontPath = ""
 
     public init(width w: Float = 1000, height h: Float = 660, fontPath: String = "") {
@@ -239,25 +240,40 @@ public class AGGRenderer: Renderer{
     }
 
     public func drawOutput(fileName name: String) throws {
-        var errorDescPtr = UnsafePointer<Int8>(bitPattern: 0)
+        var errorDescPtr: UnsafePointer<Int8>?
         let err = save_image(name, &errorDescPtr, agg_object)
         if err != 0, let errorDescPtr = errorDescPtr {
             throw DrawOutputError(errorCode: err, description: String(cString: errorDescPtr))
         }
     }
 
-    public func base64Png() -> String{
-        let pngBufferPointer: UnsafePointer<UInt8> = get_png_buffer(agg_object)
-        let bufferSize: Int = Int(get_png_buffer_size(agg_object))
-        return Data(
-            bytesNoCopy: UnsafeMutableRawPointer(mutating: pngBufferPointer),
-            count: bufferSize,
-            deallocator: .none
-        ).base64EncodedString(options: .lineLength64Characters)
+    public func base64Png() -> String {
+      var _bufferPtr: UnsafeMutablePointer<UInt8>?
+      var errorDescPtr: UnsafePointer<Int8>?
+      var bufferSize = 0
+      
+      let err = create_png_buffer(&_bufferPtr, &bufferSize, &errorDescPtr, agg_object)
+      guard let bufferPtr = _bufferPtr, err == 0 else {
+        // We'll probably never make this throwing, but log the error all the same.
+        print(
+          errorDescPtr.map { "Error rendering image: \(String(cString: $0))"} ??
+          "lodepng failed to render, but didn't produce an error."
+        )
+        return ""
+      }
+      
+      let base64String = Data(
+        bytesNoCopy: UnsafeMutableRawPointer(mutating: bufferPtr),
+        count: bufferSize,
+        deallocator: .none
+      ).base64EncodedString()
+      
+      free_png_buffer(&_bufferPtr)
+      return base64String
     }
 
     deinit {
-        delete_buffer(agg_object)
+        delete_plot(agg_object)
     }
 
 }

Sources/AGGRenderer/CAGGRenderer/CAGGRenderer.cpp

@@ -2,10 +2,14 @@
 #include "CPPAGGRenderer.h"
 #include <iostream>
 
-const void * initializePlot(float w, float h, const char* fontPath){
+void * initializePlot(float w, float h, const char* fontPath){
   return CPPAGGRenderer::initializePlot(w, h, fontPath);
 }
 
+void delete_plot(void *object){
+  CPPAGGRenderer::delete_plot(object);
+}
+
 void draw_rect(const float *x, const float *y, float thickness, float r, float g, float b, float a, const void *object){
   CPPAGGRenderer::draw_rect(x, y, thickness, r, g, b, a, object);
 }
@@ -46,14 +50,10 @@ unsigned save_image(const char *s, const char** errorDesc, const void *object){
   return CPPAGGRenderer::save_image(s, errorDesc, object);
 }
 
-const unsigned char* get_png_buffer(const void *object){
-  return CPPAGGRenderer::get_png_buffer(object);
-}
-
-int get_png_buffer_size(const void *object){
-  return CPPAGGRenderer::get_png_buffer_size(object);
+unsigned create_png_buffer(unsigned char** output, size_t *outputSize, const char** errorDesc, const void *object) {
+  return CPPAGGRenderer::create_png_buffer(output, outputSize, errorDesc, object);
 }
 
-void delete_buffer(const void *object){
-  CPPAGGRenderer::delete_buffer(object);
+void free_png_buffer(unsigned char** output) {
+  CPPAGGRenderer::free_png_buffer(output);
 }

Sources/AGGRenderer/CAGGRenderer/include/CAGGRenderer.h

@@ -1,9 +1,12 @@
 #include<stdbool.h>
+#include<stddef.h>
 #ifdef __cplusplus
 extern "C"  {
 #endif
 
-const void * initializePlot(float w, float h, const char* fontPath);
+void * initializePlot(float w, float h, const char* fontPath);
+
+void delete_plot(void *object);
 
 void draw_rect(const float *x, const float *y, float thickness, float r, float g, float b, float a, const void *object);
 
@@ -25,11 +28,9 @@ void get_text_size(const char *s, float size, float* outW, float* outH, const vo
 
 unsigned save_image(const char *s, const char** errorDesc, const void *object);
 
-const unsigned char* get_png_buffer(const void *object);
-
-int get_png_buffer_size(const void *object);
+unsigned create_png_buffer(unsigned char** output, size_t *outputSize, const char** errorDesc, const void *object);
 
-void delete_buffer(const void *object);
+void free_png_buffer(unsigned char** output);
 
 #ifdef __cplusplus
 }

Sources/AGGRenderer/CPPAGGRenderer/CPPAGGRenderer.cpp

@@ -52,34 +52,30 @@ namespace CPPAGGRenderer{
     saveBMP(buf, width, height, file_name);
   }
 
-  unsigned write_png(std::vector<unsigned char>& image, unsigned width, unsigned height, const char* filename, const char** errorDesc) {
+  unsigned write_png(const unsigned char* image, unsigned width, unsigned height, const char* filename, const char** errorDesc) {
     //Encode the image
     LodePNGColorType colorType = LCT_RGB;
     unsigned error = lodepng::encode(filename, image, width, height, colorType);
     if(error && errorDesc)
         *errorDesc = lodepng_error_text(error);
-      return error;
+    return error;
   }
 
-  std::vector<unsigned char> write_png_memory(const unsigned char* buf, unsigned width, unsigned height){
+  unsigned write_png_memory(const unsigned char *image, unsigned width, unsigned height,
+                            unsigned char **output, size_t *outputSize, const char **errorDesc){
     //Encode the image
     LodePNGColorType colorType = LCT_RGB;
-    std::vector<unsigned char> out;
-    unsigned error = lodepng::encode(out, buf, width, height, colorType);
-
-    //if there's an error, display it
-    if(error) std::cout << "encoder error " << error << ": "<< lodepng_error_text(error) << std::endl;
-    return out;
+    unsigned error = lodepng_encode_memory(output, outputSize, image, w, h, colorType, 8);
+    if(error && errorDesc)
+        *errorDesc = lodepng_error_text(error);
+    return error;
   }
 
   class Plot{
-
-  public:
     agg::rasterizer_scanline_aa<> m_ras;
     agg::scanline_p8              m_sl_p8;
     agg::line_cap_e buttCap = agg::butt_cap;
     renderer_aa ren_aa;
-    int pngBufferSize = 0;
 
     font_engine_type  m_feng;
     font_manager_type m_fman;
@@ -101,7 +97,9 @@ namespace CPPAGGRenderer{
     agg::int8u*           m_pattern;
     agg::rendering_buffer m_pattern_rbuf;
     renderer_base_pre rb_pre;
-
+    
+  public:
+    
     Plot(float width, float height, const char* fontPathPtr) :
     m_feng(),
     m_fman(m_feng),
@@ -110,10 +108,8 @@ namespace CPPAGGRenderer{
     frame_width(width),
     frame_height(height)
     {
-      if (buffer != NULL) {
-        delete[] buffer;
-      }
       buffer = new unsigned char[frame_width*frame_height*3];
+      memset(buffer, 255, frame_width*frame_height*3);
       m_curves.approximation_scale(2.0);
       m_contour.auto_detect_orientation(false);
       fontPath = fontPathPtr;
@@ -123,6 +119,10 @@ namespace CPPAGGRenderer{
         fontPath = dir_path.append("/Roboto-Regular.ttf");
       }
     }
+    
+    ~Plot() {
+      delete [] buffer;
+    }
 
     void generate_pattern(float r, float g, float b, float a, int hatch_pattern){
       agg::path_storage m_ps;
@@ -466,34 +466,27 @@ namespace CPPAGGRenderer{
       char* file_png = (char *) malloc(1 + strlen(s)+ strlen(".png") );
       strcpy(file_png, s);
       strcat(file_png, ".png");
-      std::vector<unsigned char> image(buffer, buffer + (frame_width*frame_height*3));
-      unsigned err = write_png(image, frame_width, frame_height, file_png, errorDesc);
+      unsigned err = write_png(buffer, frame_width, frame_height, file_png, errorDesc);
       free(file_png);
       return err;
     }
 
-    const unsigned char* getPngBuffer(){
-      std::vector<unsigned char> outputImage = write_png_memory(buffer, frame_width, frame_height);
-      pngBufferSize = outputImage.size();
-    	return outputImage.data();
-    }
-
-    int getPngBufferSize(){
-      return pngBufferSize;
+    unsigned create_png_buffer(unsigned char** output, size_t *outputSize, const char** errorDesc) {
+      return write_png_memory(buffer, frame_width, frame_height, output, outputSize, errorDesc);
     }
-
-    void delete_buffer(){
-      delete[] buffer;
-    }
-
   };
 
-  const void * initializePlot(float w, float h, const char* fontPath){
+  void * initializePlot(float w, float h, const char* fontPath){
     Plot *plot = new Plot(w, h, fontPath);
-    memset(plot->buffer, 255, plot->frame_width*plot->frame_height*3);
     return (void *)plot;
   }
 
+  void delete_plot(void *object) {
+    Plot *plot = (Plot *)object;
+    delete plot;
+    object = 0;
+  }
+
   void draw_rect(const float *x, const float *y, float thickness, float r, float g, float b, float a,
                  const void *object){
     Plot *plot = (Plot *)object;
@@ -545,19 +538,13 @@ namespace CPPAGGRenderer{
     return plot -> save_image(s, errorDesc);
   }
 
-  const unsigned char* get_png_buffer(const void *object){
+  unsigned create_png_buffer(unsigned char** output, size_t *outputSize, const char** errorDesc, const void *object) {
     Plot *plot = (Plot *)object;
-    return plot -> getPngBuffer();
+    return plot -> create_png_buffer(output, outputSize, errorDesc);
   }
 
-  int get_png_buffer_size(const void *object){
-    Plot *plot = (Plot *)object;
-    return plot -> getPngBufferSize();
+  void free_png_buffer(unsigned char** buffer) {
+    if (buffer) { free(*buffer); }
+    *buffer = 0;
   }
-
-  void delete_buffer(const void *object){
-    Plot *plot = (Plot *)object;
-    plot -> delete_buffer();
-  }
-
 }

Sources/AGGRenderer/CPPAGGRenderer/include/CPPAGGRenderer.h

@@ -1,6 +1,10 @@
+#include <stddef.h>
+
 namespace CPPAGGRenderer{
 
-  const void * initializePlot(float w, float h, const char* fontPath);
+  void * initializePlot(float w, float h, const char* fontPath);
+
+  void delete_plot(void *object);
 
   void draw_rect(const float *x, const float *y, float thickness, float r, float g, float b, float a, const void *object);
 
@@ -22,10 +26,8 @@ namespace CPPAGGRenderer{
 
   unsigned save_image(const char *s, const char** errorDesc, const void *object);
 
-  const unsigned char* get_png_buffer(const void *object);
-
-  int get_png_buffer_size(const void *object);
+  unsigned create_png_buffer(unsigned char **output, size_t *outputSize, const char **errorDesc, const void *object);
 
-  void delete_buffer(const void *object);
+  void free_png_buffer(unsigned char **buffer);
 
 }

Tests/SwiftPlotTests/AGGRenderer/agg-png-output.swift

@@ -5,8 +5,6 @@ import AGGRenderer
 
 extension AGGRendererTests {
 
-  /// **XFAIL**
-  ///
   /// Tests that base64 encoding is accurate by drawing a known graph directly in to
   /// a PNG buffer (no files), then verifying the base64-encoded data matches that from
   /// the reference file.
@@ -22,7 +20,7 @@ extension AGGRendererTests {
     let renderer = AGGRenderer()
     barGraph.drawGraph(renderer: renderer)
     let outputBase64 = renderer.base64Png()
-    XCTAssertEqual(outputBase64.count, 47397)
+    XCTAssertEqual(outputBase64.count, 46668)
 
     // First, sanity check: ensure *we* can decode the string.
     guard let _ = Data(base64Encoded: outputBase64, options: .ignoreUnknownCharacters) else {
@@ -36,9 +34,9 @@ extension AGGRendererTests {
       .appendingPathComponent(fileName)
       .appendingPathExtension(KnownRenderer.agg.fileExtension)
     let referenceBase64 = try Data(contentsOf: referenceFile)
-      .base64EncodedString()//(options: .lineLength64Characters)
+      .base64EncodedString()
 
-    //XCTAssertEqual(outputBase64, referenceBase64)
+    XCTAssertEqual(outputBase64, referenceBase64)
   }
 }