Eeschema/pcbnew: fix pasted image saving

The m_imageData buffer in the BITMAP_BASE is an
internal implementation - the public API to persist the
data to a stream appears to be SaveImageData - so use that,
which hides the implementation (and generates the image data
"live" when needed).

Remove the public access to the m_imageData buffer as it isn't
needed for public use, and also is misleading.

Also break out the formatting of the data into KICAD_FORMAT,
as it's currently replicated in eeschema/pcb/pagelayout
formatting code.

Fixes: https://gitlab.com/kicad/code/kicad/-/issues/19772

common/bitmap_base.cpp

@@ -239,7 +239,7 @@ bool BITMAP_BASE::LoadLegacyData( LINE_READER& aLine, wxString& aErrorMsg )
             m_image->LoadFile( istream, wxBITMAP_TYPE_ANY );
             m_bitmap = new wxBitmap( *m_image );
             m_originalImage = new wxImage( *m_image );
-            UpdateImageDataBuffer();
+            updateImageDataBuffer();
             break;
         }
 
@@ -433,7 +433,7 @@ void BITMAP_BASE::Mirror( FLIP_DIRECTION aFlipDirection )
             m_isMirroredX = !m_isMirroredX;
 
         rebuildBitmap( false );
-        UpdateImageDataBuffer();
+        updateImageDataBuffer();
     }
 }
 
@@ -458,7 +458,7 @@ void BITMAP_BASE::Rotate( bool aRotateCCW )
 
         m_rotation += ( aRotateCCW ? ANGLE_90 : -ANGLE_90 );
         rebuildBitmap( false );
-        UpdateImageDataBuffer();
+        updateImageDataBuffer();
     }
 }
 
@@ -470,7 +470,7 @@ void BITMAP_BASE::ConvertToGreyscale()
         *m_image  = m_image->ConvertToGreyscale();
         *m_originalImage = m_originalImage->ConvertToGreyscale();
         rebuildBitmap();
-        UpdateImageDataBuffer();
+        updateImageDataBuffer();
     }
 }
 
@@ -490,7 +490,7 @@ void BITMAP_BASE::PlotImage( PLOTTER*       aPlotter, const VECTOR2I& aPos,
 }
 
 
-void BITMAP_BASE::UpdateImageDataBuffer()
+void BITMAP_BASE::updateImageDataBuffer()
 {
     if( m_image )
     {

common/drawing_sheet/ds_data_model_io.cpp

@@ -34,10 +34,11 @@
 #include <drawing_sheet/drawing_sheet_lexer.h>
 #include <drawing_sheet/ds_file_versions.h>
 #include <font/font.h>
+#include <io/kicad/kicad_io_utils.h>
 #include <string_utils.h>
 
-#include <wx/base64.h>
 #include <wx/msgdlg.h>
+#include <wx/mstream.h>
 
 using namespace DRAWINGSHEET_T;
 
@@ -422,21 +423,11 @@ void DS_DATA_MODEL_IO::format( DS_DATA_ITEM_BITMAP* aItem ) const
     // Write image in png readable format
     m_out->Print( "(data" );
 
-    wxString out = wxBase64Encode( aItem->m_ImageBitmap->GetImageDataBuffer() );
+    wxMemoryOutputStream stream;
+    aItem->m_ImageBitmap->SaveImageData( stream );
 
-    // Apparently the MIME standard character width for base64 encoding is 76 (unconfirmed)
+    KICAD_FORMAT::FormatStreamData( *m_out, *stream.GetOutputStreamBuffer() );
-    // so use it in a vain attempt to be standard like.
-#define MIME_BASE64_LENGTH 76
 
-    size_t first = 0;
-
-    while( first < out.Length() )
-    {
-        m_out->Print( "\n\"%s\"", TO_UTF8( out( first, MIME_BASE64_LENGTH ) ) );
-        first += MIME_BASE64_LENGTH;
-    }
-
-    m_out->Print( ")" );  // Closes data token.
     m_out->Print( ")" );  // Closes bitmap token.
 }
 

common/io/kicad/kicad_io_utils.cpp

@@ -17,10 +17,16 @@
  * with this program.  If not, see <http://www.gnu.org/licenses/>.
  */
 
+#include "io/kicad/kicad_io_utils.h"
+
+// For some reason wxWidgets is built with wxUSE_BASE64 unset so expose the wxWidgets
+// base64 code.
+#define wxUSE_BASE64 1
+#include <wx/base64.h>
+
 #include <fmt/format.h>
 
 #include <kiid.h>
-#include <io/kicad/kicad_io_utils.h>
 #include <richio.h>
 #include <string_utils.h>
 
@@ -37,6 +43,29 @@ void FormatUuid( OUTPUTFORMATTER* aOut, const KIID& aUuid )
     aOut->Print( "(uuid %s)", aOut->Quotew( aUuid.AsString() ).c_str() );
 }
 
+
+void FormatStreamData( OUTPUTFORMATTER& aOut, const wxStreamBuffer& aStream )
+{
+    aOut.Print( "(data" );
+
+    const wxString out = wxBase64Encode( aStream.GetBufferStart(), aStream.GetBufferSize() );
+
+    // Apparently the MIME standard character width for base64 encoding is 76 (unconfirmed)
+    // so use it in a vein attempt to be standard like.
+    static constexpr unsigned MIME_BASE64_LENGTH = 76;
+
+    size_t first = 0;
+
+    while( first < out.Length() )
+    {
+        aOut.Print( "\n\"%s\"", TO_UTF8( out( first, MIME_BASE64_LENGTH ) ) );
+        first += MIME_BASE64_LENGTH;
+    }
+
+    aOut.Print( ")" ); // Closes data token.
+}
+
+
 /*
  * Formatting rules:
  * - All extra (non-indentation) whitespace is trimmed

eeschema/sch_io/kicad_sexpr/sch_io_kicad_sexpr.cpp

@@ -22,10 +22,6 @@
 
 #include <algorithm>
 
-// For some reason wxWidgets is built with wxUSE_BASE64 unset so expose the wxWidgets
-// base64 code.
-#define wxUSE_BASE64 1
-#include <wx/base64.h>
 #include <wx/log.h>
 #include <wx/mstream.h>
 
@@ -958,23 +954,11 @@ void SCH_IO_KICAD_SEXPR::saveBitmap( const SCH_BITMAP& aBitmap )
 
     KICAD_FORMAT::FormatUuid( m_out, aBitmap.m_Uuid );
 
-    m_out->Print( "(data" );
+    wxMemoryOutputStream stream;
+    bitmapBase.SaveImageData( stream );
 
-    wxString out = wxBase64Encode( bitmapBase.GetImageDataBuffer() );
+    KICAD_FORMAT::FormatStreamData( *m_out, *stream.GetOutputStreamBuffer() );
 
-    // Apparently the MIME standard character width for base64 encoding is 76 (unconfirmed)
-    // so use it in a vein attempt to be standard like.
-#define MIME_BASE64_LENGTH 76
-
-    size_t first = 0;
-
-    while( first < out.Length() )
-    {
-        m_out->Print( "\n\"%s\"", TO_UTF8( out( first, MIME_BASE64_LENGTH ) ) );
-        first += MIME_BASE64_LENGTH;
-    }
-
-    m_out->Print( ")" );        // Closes data token.
     m_out->Print( ")" );        // Closes image token.
 }
 

eeschema/tools/sch_editor_control.cpp

@@ -1657,7 +1657,14 @@ int SCH_EDITOR_CONTROL::Paste( const TOOL_EVENT& aEvent )
     {
         // Just image data
         auto bitmap = std::make_unique<SCH_BITMAP>();
-        bitmap->GetReferenceImage().SetImage( *clipImg );
+
+        bool ok = bitmap->GetReferenceImage().SetImage( *clipImg );
+
+        if( !ok )
+        {
+            delete tempScreen;
+            return 0;
+        }
 
         tempScreen->Append( bitmap.release() );
     }

include/bitmap_base.h

@@ -240,17 +240,12 @@ public:
      */
     void SetImageType( wxBitmapType aType ) { m_imageType = aType; }
 
-    /**
+private:
-     * @return the image data buffer.
-     */
-    const wxMemoryBuffer& GetImageDataBuffer() const { return m_imageData; }
-
     /**
      * Resets the image data buffer using the current image data.
      */
-    void UpdateImageDataBuffer();
+    void updateImageDataBuffer();
 
-private:
     /**
      * Rebuild the internal bitmap used to draw/plot image.
      *

include/io/kicad/kicad_io_utils.h

@@ -17,11 +17,13 @@
  * with this program.  If not, see <http://www.gnu.org/licenses/>.
  */
 
-#ifndef KICAD_IO_UTILS_H
+#pragma once
-#define KICAD_IO_UTILS_H
 
+#include <wx/stream.h>
 #include <wx/string.h>
 
+#include <kicommon.h>
+
 class OUTPUTFORMATTER;
 class KIID;
 
@@ -38,8 +40,11 @@ KICOMMON_API void FormatBool( OUTPUTFORMATTER* aOut, const wxString& aKey, bool
 
 KICOMMON_API void FormatUuid( OUTPUTFORMATTER* aOut, const KIID& aUuid );
 
+/**
+ * Write binary data to the formatter as base 64 encoded string.
+ */
+KICOMMON_API void FormatStreamData( OUTPUTFORMATTER& aOut, const wxStreamBuffer& aStream );
+
 KICOMMON_API void Prettify( std::string& aSource, bool aCompactSave );
 
 } // namespace KICAD_FORMAT
-
-#endif //KICAD_IO_UTILS_H

pcbnew/pcb_io/kicad_sexpr/pcb_io_kicad_sexpr.cpp

@@ -22,9 +22,6 @@
  * 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301, USA
  */
 
-// base64 code. Needed for PCB_REFERENCE_IMAGE
-#define wxUSE_BASE64 1
-#include <wx/base64.h>
 #include <wx/dir.h>
 #include <wx/ffile.h>
 #include <wx/log.h>
@@ -1066,23 +1063,10 @@ void PCB_IO_KICAD_SEXPR::format( const PCB_REFERENCE_IMAGE* aBitmap ) const
     if( aBitmap->IsLocked() )
         KICAD_FORMAT::FormatBool( m_out, "locked", true );
 
-    m_out->Print( "(data" );
+    wxMemoryOutputStream ostream;
+    refImage.GetImage().SaveImageData( ostream );
 
-    wxString out = wxBase64Encode( refImage.GetImage().GetImageDataBuffer() );
+    KICAD_FORMAT::FormatStreamData( *m_out, *ostream.GetOutputStreamBuffer() );
-
-    // Apparently the MIME standard character width for base64 encoding is 76 (unconfirmed)
-    // so use it in a vain attempt to be standard like.
-#define MIME_BASE64_LENGTH 76
-
-    size_t first = 0;
-
-    while( first < out.Length() )
-    {
-        m_out->Print( "\n\"%s\"", TO_UTF8( out( first, MIME_BASE64_LENGTH ) ) );
-        first += MIME_BASE64_LENGTH;
-    }
-
-    m_out->Print( ")" );  // Closes data token.
 
     KICAD_FORMAT::FormatUuid( m_out, aBitmap->m_Uuid );
     m_out->Print( ")" );      // Closes image token.

qa/qa_utils/include/qa_utils/wx_utils/unit_test_utils.h

@@ -142,6 +142,7 @@ std::ostream& boost_test_print_type( std::ostream& os, std::pair<K, V> const& aP
  */
 std::ostream& boost_test_print_type( std::ostream& os, wxPoint const& aVec );
 
+std::ostream& boost_test_print_type( std::ostream& os, wxSize const& aSize );
 
 namespace KI_TEST
 {

qa/qa_utils/wx_utils/unit_test_utils.cpp

@@ -31,6 +31,13 @@ std::ostream& boost_test_print_type( std::ostream& os, wxPoint const& aPt )
 }
 
 
+std::ostream& boost_test_print_type( std::ostream& os, wxSize const& aSize )
+{
+    os << "wxSize[ x=\"" << aSize.x << "\" y=\"" << aSize.y << "\" ]";
+    return os;
+}
+
+
 std::string KI_TEST::GetEeschemaTestDataDir()
 {
     const char* env = std::getenv( "KICAD_TEST_EESCHEMA_DATA_DIR" );

qa/tests/common/test_bitmap_base.cpp

@@ -244,4 +244,37 @@ BOOST_AUTO_TEST_CASE( MirrorImage )
     }
 }
 
+/**
+ * Check setting image data by SetImage produces saveable data
+ * via SaveImageData.
+ *
+ * Regression test for: https://gitlab.com/kicad/code/kicad/-/issues/19772
+ */
+BOOST_AUTO_TEST_CASE( SetImageOutputsData )
+{
+    BITMAP_BASE bitmap;
+    wxImage     img( 2, 2 );
+    img.SetRGB( 0, 0, 0, 255, 0 );
+    img.SetRGB( 1, 0, 0, 0, 255 );
+    img.SetRGB( 0, 1, 255, 0, 0 );
+    img.SetRGB( 1, 1, 0, 0, 255 );
+
+    // Set the wxImage directly, not via a stream/file
+    // (this happens, e.g. when the clipboard gives you a wxImage)
+    bitmap.SetImage( img );
+
+    wxMemoryOutputStream mos;
+    BOOST_CHECK( bitmap.SaveImageData( mos ) );
+
+    BOOST_REQUIRE( mos.GetSize() > 0 );
+
+    // check the output is the same as the input
+    wxMemoryInputStream mis( mos );
+    wxImage             img2;
+
+    BOOST_CHECK( img2.LoadFile( mis, wxBITMAP_TYPE_PNG ) );
+
+    BOOST_CHECK_PREDICATE( KI_TEST::ImagesHaveSamePixels, (img) ( img2 ) );
+}
+
 BOOST_AUTO_TEST_SUITE_END()

qa/tests/common/wximage_test_utils.cpp

@@ -28,15 +28,7 @@
 
 namespace KI_TEST
 {
-/**
+
- * Predicate to check an image pixel matches color and alpha
- *
- * @param  aImage  the image to check
- * @param  aX      pixel x-coordinate
- * @param  aY      pixel y-coordinate
- * @param  aColor  expected color (alpha is 1.0 if image doesn't support alpha)
- * @return         true if colour match
- */
 bool IsImagePixelOfColor( const wxImage& aImage, int aX, int aY, const KIGFX::COLOR4D& aColor )
 {
     const wxSize imageSize = aImage.GetSize();
@@ -64,6 +56,40 @@ bool IsImagePixelOfColor( const wxImage& aImage, int aX, int aY, const KIGFX::CO
     return true;
 }
 
+
+bool ImagesHaveSamePixels( const wxImage& aImgA, const wxImage& aImgB )
+{
+    if( aImgA.GetSize() != aImgB.GetSize() )
+    {
+        BOOST_TEST_INFO( "Image sizes differ: " << aImgA.GetSize() << " vs " << aImgB.GetSize() );
+        return false;
+    }
+
+    for( int y = 0; y < aImgA.GetHeight(); ++y )
+    {
+        for( int x = 0; x < aImgA.GetWidth(); ++x )
+        {
+            const int rA = aImgA.GetRed( x, y );
+            const int gA = aImgA.GetGreen( x, y );
+            const int bA = aImgA.GetBlue( x, y );
+
+            const int rB = aImgB.GetRed( x, y );
+            const int gB = aImgB.GetGreen( x, y );
+            const int bB = aImgB.GetBlue( x, y );
+
+            if( rA != rB || gA != gB || bA != bB )
+            {
+                BOOST_TEST_INFO( "Pixel (" << x << ", " << y << ") differs: "
+                                           << "A(" << rA << ", " << gA << ", " << bA << ") "
+                                           << "B(" << rB << ", " << gB << ", " << bB << ")" );
+                return false;
+            }
+        }
+    }
+
+    return true;
+}
+
 } // namespace KI_TEST
 
 

qa/tests/common/wximage_test_utils.h

@@ -43,6 +43,13 @@ namespace KI_TEST
  */
 bool IsImagePixelOfColor( const wxImage& aImage, int aX, int aY, const KIGFX::COLOR4D& aColor );
 
+/**
+ * Check if an image is identical to another image, pixel by pixel
+ *
+ * (this is exhaustive, avoid using it for large images)
+ */
+bool ImagesHaveSamePixels( const wxImage& aImgA, const wxImage& aImgB );
+
 } // namespace KI_TEST