Issue #598: ASAN crashes when drawing sprites

issue was that we were not doing deep copies, and then had dangling pointers.
calband · Jan 19, 2025 · 9f8c105 · 9f8c105
1 parent 92ed3dd
commit 9f8c105
Show file tree

Hide file tree

Showing 14 changed files with 181 additions and 74 deletions.
diff --git a/LATEST_RELEASE_NOTES.md b/LATEST_RELEASE_NOTES.md
@@ -3,6 +3,7 @@
 Bugs addressed in this release:
 
 * [#593](../../issues/593) Are the draw colors for selected and reference inverted on the drawing setup
+* [#598](../../issues/598) ASAN crashes when drawing sprites
 
 Other changes:
 

diff --git a/src/AnimationView.cpp b/src/AnimationView.cpp
@@ -68,7 +68,7 @@ AnimationView::~AnimationView()
 {
 }
 
-void AnimationView::RegenerateImages() const
+void AnimationView::RegenerateImages()
 {
     auto spriteScale = mConfig.Get_SpriteBitmapScale();
     if (spriteScale == mScaleSize) {
@@ -93,7 +93,10 @@ void AnimationView::RegenerateImages() const
     constexpr auto image_Y = 128;
     auto images = GenerateSpriteImages(image, mSpriteCalChartImages.size(), image_X, image_Y, mScaleSize);
     std::transform(images.begin(), images.end(), mSpriteCalChartImages.begin(), [](auto&& image) {
-        return std::make_shared<CalChart::ImageData>(wxCalChart::ConvertToImageData(image));
+        return BitmapSize_t{ std::make_shared<wxCalChart::BitmapHolder>(image), wxCalChart::toCoord(image.GetSize()) };
+    });
+    std::transform(images.begin(), images.end(), mSelectedSpriteCalChartImages.begin(), [](auto&& image) {
+        return BitmapSize_t{ std::make_shared<wxCalChart::BitmapHolder>(image.ConvertToGreyscale()), wxCalChart::toCoord(image.GetSize()) };
     });
 }
 
@@ -111,9 +114,9 @@ void AnimationView::OnDraw(wxDC* dc)
             mCurrentBeat,
             mDrawCollisionWarning,
             onBeat,
-            [this](CalChart::Radian angle, CalChart::Animation::ImageBeat imageBeat) {
+            [this](CalChart::Radian angle, CalChart::Animation::ImageBeat imageBeat, bool selected) {
                 auto imageIndex = CalChart::AngleToQuadrant(angle) + CalChart::toUType(imageBeat) * 8;
-                return mSpriteCalChartImages[imageIndex];
+                return selected ? mSelectedSpriteCalChartImages[imageIndex] : mSpriteCalChartImages[imageIndex];
             }));
 }
 

diff --git a/src/AnimationView.h b/src/AnimationView.h
@@ -31,6 +31,9 @@
 
 class AnimationPanel;
 class CalChartView;
+namespace wxCalChart {
+struct BitmapHolder;
+}
 namespace CalChart {
 class Configuration;
 class Continuity;
@@ -88,7 +91,7 @@ class AnimationView : public wxView {
     void Generate();
     void RefreshFrame();
 
-    void RegenerateImages() const;
+    void RegenerateImages();
     [[nodiscard]] auto GenerateDraw(CalChart::Configuration const& config) const -> std::vector<CalChart::Draw::DrawCommand>;
     [[nodiscard]] auto GenerateDrawSprites(CalChart::Configuration const& config, CalChart::Animation::AngleStepToImageFunction imageFunction) const -> std::vector<CalChart::Draw::DrawCommand>;
 
@@ -104,9 +107,10 @@ class AnimationView : public wxView {
     bool mPlayCollisionWarning = false;
 
     static constexpr auto kAngles = 8;
-    // these need to be mutable because effectively they are a cache and may need to be regenerated.
-    mutable double mScaleSize = 0;
-    mutable std::array<std::shared_ptr<CalChart::ImageData>, kAngles * CalChart::toUType(CalChart::Animation::ImageBeat::Size)> mSpriteCalChartImages;
+    double mScaleSize = 0;
+    using BitmapSize_t = std::tuple<std::shared_ptr<wxCalChart::BitmapHolder>, CalChart::Coord>;
+    std::array<BitmapSize_t, kAngles * CalChart::toUType(CalChart::Animation::ImageBeat::Size)> mSpriteCalChartImages;
+    std::array<BitmapSize_t, kAngles * CalChart::toUType(CalChart::Animation::ImageBeat::Size)> mSelectedSpriteCalChartImages;
 
     CalChart::MeasureDuration mMeasure;
 };
diff --git a/src/BackgroundImages.cpp b/src/BackgroundImages.cpp
@@ -28,22 +28,24 @@
 
 class BackgroundImage {
 public:
-    BackgroundImage(wxImage const& image, int x, int y, int scaled_width, int scaled_height);
+    explicit BackgroundImage(CalChart::ImageInfo const& image);
 
-    bool MouseClickIsHit(wxMouseEvent const& event, wxDC const& dc) const;
+    [[nodiscard]] auto MouseClickIsHit(wxMouseEvent const& event, wxDC const& dc) const -> bool;
     void OnMouseLeftDown(wxMouseEvent const& event, wxDC const& dc);
+
     // returns left, top, width, height
-    std::array<int, 4> OnMouseLeftUp(wxMouseEvent const& event, wxDC const& dc);
+    [[nodiscard]] auto OnMouseLeftUp(wxMouseEvent const& event, wxDC const& dc) -> std::array<int, 4>;
 
     void OnMouseMove(wxMouseEvent const& event, wxDC const& dc);
     void OnPaint(wxDC& dc, bool drawPicAdjustDots, bool selected) const;
 
 private:
     static constexpr auto kCircleSize = 6;
 
-    wxImage mImage;
+    CalChart::Coord mPosition{};
+    CalChart::Coord mScaledSize{};
+    wxImage mRawImage;
     wxBitmap mBitmap;
-    wxPoint mBitmapPoint;
 
     // what type of background adjustments could we do
     enum class BackgroundAdjustType {
@@ -60,7 +62,7 @@ class BackgroundImage {
     };
     using BackgroundAdjustTypeIterator = CalChart::Iterator<BackgroundAdjustType, BackgroundAdjustType::kUpperLeft, BackgroundAdjustType::kLowerRight>;
     BackgroundAdjustType mBackgroundAdjustType;
-    BackgroundAdjustType WhereIsMouseClick(const wxMouseEvent& event, const wxDC& dc) const;
+    [[nodiscard]] auto WhereIsMouseClick(const wxMouseEvent& event, const wxDC& dc) const -> BackgroundAdjustType;
 
     static auto toOffsetX(BackgroundAdjustType where)
     {
@@ -103,16 +105,16 @@ class BackgroundImage {
         float mAspectRatio;
         BackgroundAdjustType mAdjustType;
     };
-    std::unique_ptr<CalculateScaleAndMove> mScaleAndMove;
+    std::optional<CalculateScaleAndMove> mScaleAndMove;
 };
 
-BackgroundImage::BackgroundImage(const wxImage& image, int x, int y, int scaled_width, int scaled_height)
-    : mImage(image)
-    , mBitmapPoint(x, y)
-    , // always adjust when we get created
-    mBackgroundAdjustType(BackgroundAdjustType::kLast)
+BackgroundImage::BackgroundImage(CalChart::ImageInfo const& image)
+    : mPosition{ image.left, image.top }
+    , mScaledSize{ image.scaledWidth, image.scaledHeight }
+    , mRawImage{ wxCalChart::towxImage(image.data) }
+    , mBitmap{ mRawImage.Scale(mScaledSize.x, mScaledSize.y, wxIMAGE_QUALITY_HIGH) }
+    , mBackgroundAdjustType(BackgroundAdjustType::kLast) // always adjust when we get created
 {
-    mBitmap = wxBitmap(mImage.Scale(scaled_width, scaled_height, wxIMAGE_QUALITY_HIGH));
 }
 
 BackgroundImage::BackgroundAdjustType BackgroundImage::WhereIsMouseClick(const wxMouseEvent& event, const wxDC& dc) const
@@ -122,10 +124,10 @@ BackgroundImage::BackgroundAdjustType BackgroundImage::WhereIsMouseClick(const w
     auto y = dc.DeviceToLogicalY(point.y);
 
     // where are we?
-    auto bitmapSize = mBitmap.GetSize();
-    auto middle = mBitmapPoint + bitmapSize / 2;
+    auto bitmapSize = wxCalChart::toSize(mScaledSize);
+    auto middle = wxCalChart::toPoint(mPosition) + bitmapSize / 2;
     for (auto where : BackgroundAdjustTypeIterator()) {
-        if (where == BackgroundAdjustType::kMove && wxRect{ mBitmapPoint, mBitmap.GetSize() }.Contains(x, y)) {
+        if (where == BackgroundAdjustType::kMove && wxRect{ wxCalChart::toPoint(mPosition), wxCalChart::toSize(mScaledSize) }.Contains(x, y)) {
             return where;
         }
         auto offsetX = toOffsetX(where);
@@ -160,15 +162,17 @@ void BackgroundImage::OnMouseLeftDown(wxMouseEvent const& event, wxDC const& dc)
         return;
     }
     mBackgroundAdjustType = where;
-    mScaleAndMove = std::make_unique<CalculateScaleAndMove>(wxPoint{ x, y }, wxRect{ mBitmapPoint, mBitmap.GetSize() }, mBackgroundAdjustType);
+    mScaleAndMove = { wxPoint{ x, y }, wxRect{ wxCalChart::toPoint(mPosition), wxCalChart::toSize(mScaledSize) }, mBackgroundAdjustType };
 }
 
 std::array<int, 4> BackgroundImage::OnMouseLeftUp(const wxMouseEvent&, const wxDC&)
 {
     if (mScaleAndMove) {
         // done moving, lock down the picture and make it pretty:
-        mBitmap = wxBitmap(mImage.Scale(mBitmap.GetWidth(), mBitmap.GetHeight(), wxIMAGE_QUALITY_HIGH));
-        std::array<int, 4> data{ { mBitmapPoint.x, mBitmapPoint.y, mBitmap.GetWidth(), mBitmap.GetHeight() } };
+        mBitmap = wxBitmap(mRawImage.Scale(mBitmap.GetWidth(), mBitmap.GetHeight(), wxIMAGE_QUALITY_HIGH));
+        auto [width, height] = wxCalChart::toSize(mScaledSize);
+        auto [x, y] = wxCalChart::toPoint(mPosition);
+        std::array<int, 4> data{ { x, y, width, height } };
         mScaleAndMove.reset();
         mBackgroundAdjustType = BackgroundAdjustType::kLast;
         return data;
@@ -183,19 +187,21 @@ void BackgroundImage::OnMouseMove(const wxMouseEvent& event, const wxDC& dc)
     auto y = dc.DeviceToLogicalY(point.y);
 
     if (event.Dragging() && event.LeftIsDown() && mScaleAndMove) {
-        auto rect = (*mScaleAndMove)(x, y, { mBitmapPoint, mBitmap.GetSize() });
-        mBitmapPoint = rect.GetPosition();
-        mBitmap = wxBitmap(mImage.Scale(rect.width, rect.height));
+        auto rect = (*mScaleAndMove)(x, y, { wxCalChart::toPoint(mPosition), wxCalChart::toSize(mScaledSize) });
+        mPosition = wxCalChart::toCoord(rect.GetPosition());
+        mScaledSize = wxCalChart::toCoord(rect.GetSize());
+        mBitmap = wxBitmap(mRawImage.Scale(mScaledSize.x, mScaledSize.y));
     }
 }
 
 void BackgroundImage::OnPaint(wxDC& dc, bool drawPicAdjustDots, bool selected) const
 {
-    dc.DrawBitmap(mBitmap, mBitmapPoint.x, mBitmapPoint.y, true);
+    auto pos = wxCalChart::toPoint(mPosition);
+    dc.DrawBitmap(mBitmap, pos.x, pos.y, true);
     if (drawPicAdjustDots) {
         // draw guide dots
-        auto bitmapSize = mBitmap.GetSize();
-        auto middle = mBitmapPoint + bitmapSize / 2;
+        auto bitmapSize = wxCalChart::toSize(mScaledSize);
+        auto middle = wxCalChart::toPoint(mPosition) + bitmapSize / 2;
         dc.SetBrush(*wxBLUE_BRUSH);
         dc.SetPen(*wxBLUE_PEN);
         for (auto where : BackgroundAdjustTypeIterator()) {
@@ -320,7 +326,7 @@ void BackgroundImages::SetBackgroundImages(std::vector<CalChart::ImageInfo> cons
     mBackgroundImages.clear();
     mWhichBackgroundIndex = -1;
     for (auto&& image : images) {
-        mBackgroundImages.emplace_back(wxCalChart::ConvertTowxImage(image.data), image.left, image.top, image.scaled_width, image.scaled_height);
+        mBackgroundImages.emplace_back(image);
     }
 }
 

diff --git a/src/CalChartDrawPrimativesHelper.h b/src/CalChartDrawPrimativesHelper.h
@@ -26,6 +26,7 @@
  * Helper functions for creating wxWidgets from drawing primatives
  */
 
+#include "CalChartDrawCommand.h"
 #include "CalChartDrawPrimatives.h"
 #include "CalChartImage.h"
 #include "CalChartSizes.h"
@@ -45,6 +46,26 @@ namespace wxCalChart {
 
 inline auto make_wxSize(CalChart::Coord coord) { return wxSize{ coord.x, coord.y }; }
 
+inline auto toSize(CalChart::Coord size) -> wxSize
+{
+    return { size.x, size.y };
+}
+
+inline auto toPoint(CalChart::Coord point) -> wxPoint
+{
+    return { point.x, point.y };
+}
+
+inline auto toCoord(wxSize size) -> CalChart::Coord
+{
+    return { size.x, size.y };
+}
+
+inline auto toCoord(wxPoint point) -> CalChart::Coord
+{
+    return { point.x, point.y };
+}
+
 inline auto toColour(CalChart::Color::ColorRGB c) -> wxColour
 {
     return { c.red, c.green, c.blue, c.alpha };
@@ -232,6 +253,47 @@ inline auto setFont(wxDC& dc, CalChart::Font font)
     dc.SetFont(wxFont(size, family, style, weight));
 }
 
+// Creates a deep copy the image data
+inline auto towxImage(CalChart::ImageData const& image) -> wxImage
+{
+    auto data = std::unique_ptr<unsigned char, void (*)(void*)>{
+        static_cast<unsigned char*>(std::malloc(image.data.size())),
+        [](void* p) { std::free(p); }
+    };
+    std::copy(image.data.begin(), image.data.end(), data.get());
+    if (image.alpha.empty()) {
+        return wxImage{ image.width, image.height, data.release() };
+    }
+    auto alpha = std::unique_ptr<unsigned char, void (*)(void*)>{
+        static_cast<unsigned char*>(std::malloc(image.alpha.size())),
+        [](void* p) { std::free(p); }
+    };
+
+    std::copy(image.alpha.begin(), image.alpha.end(), alpha.get());
+    return wxImage{ image.width, image.height, data.release(), alpha.release() };
+}
+
+struct BitmapHolder : CalChart::Draw::OpaqueImageData {
+    BitmapHolder(wxImage const& image)
+        : bitmap(image)
+    {
+    }
+    ~BitmapHolder() override = default;
+    BitmapHolder(CalChart::ImageData const& image)
+    {
+        wxImage converted = [](CalChart::ImageData const& image) {
+            auto data = image.data;
+            if (image.alpha.size()) {
+                auto alpha = image.alpha;
+                return wxImage{ image.width, image.height, data.data(), alpha.data(), true };
+            }
+            return wxImage{ image.width, image.height, data.data(), true };
+        }(image);
+        bitmap = converted;
+    }
+    wxBitmap bitmap;
+};
+
 inline auto ConvertToImageData(wxImage const& image) -> CalChart::ImageData
 {
     auto width = image.GetWidth();
@@ -246,7 +308,7 @@ inline auto ConvertToImageData(wxImage const& image) -> CalChart::ImageData
         std::copy(a, a + width * height, alpha.data());
     }
 
-    return { width, height, data, alpha };
+    return { width, height, data, alpha, std::make_shared<BitmapHolder>(image) };
 }
 
 inline auto ConvertToImageInfo(wxImage const& image, int x = 0, int y = 0) -> CalChart::ImageInfo
@@ -256,13 +318,21 @@ inline auto ConvertToImageInfo(wxImage const& image, int x = 0, int y = 0) -> Ca
     return CalChart::ImageInfo{ x, y, width, height, ConvertToImageData(image) };
 }
 
-inline auto ConvertTowxImage(CalChart::ImageData const& image) -> wxImage
+inline auto ConvertTowxBitmap(CalChart::Draw::Image const& image) -> wxBitmap
 {
-    auto data = image.data;
-    if (image.alpha.size()) {
-        auto alpha = image.alpha;
-        return { image.image_width, image.image_height, data.data(), alpha.data(), true };
-    }
-    return { image.image_width, image.image_height, data.data(), true };
+    // visit
+    return std::visit(
+        CalChart::overloaded{
+            [](std::shared_ptr<CalChart::Draw::OpaqueImageData> data) {
+                return dynamic_cast<BitmapHolder&>(*data).bitmap;
+            },
+            [](std::shared_ptr<CalChart::ImageData> data) {
+                if (data->render == nullptr) {
+                    data->render = std::make_shared<BitmapHolder>(*data);
+                }
+                return dynamic_cast<BitmapHolder&>(*(data->render)).bitmap;
+            },
+        },
+        image.mImage);
 }
 }
diff --git a/src/CalChartDrawing.cpp b/src/CalChartDrawing.cpp
@@ -736,10 +736,10 @@ namespace details {
                 },
                 [&dc, layoutPoint = surface.origin](CalChart::Draw::Image const& c) {
                     auto where = wxCalChart::to_wxPoint(c.mStart) + layoutPoint;
-                    auto image = wxCalChart::ConvertTowxImage(*c.mImage);
-                    if (c.mGreyscale) {
-                        image = image.ConvertToGreyscale();
-                    }
+                    auto image = wxCalChart::ConvertTowxBitmap(c);
+                    // if (c.mGreyscale) {
+                    //     image = image.ConvertToGreyscale();
+                    // }
                     dc.DrawBitmap(image, fDIP(where));
                 },
                 []([[maybe_unused]] CalChart::Draw::Ignore const& c) {

diff --git a/src/CalChartDrawingGetMinSize.h b/src/CalChartDrawingGetMinSize.h
@@ -159,8 +159,16 @@ namespace details {
 
     inline auto GetMinSize([[maybe_unused]] Context context, CalChart::Draw::Image const& image) -> StackSize
     {
-        // TODO: should this have scale in it?
-        return wxSize(image.mImage->image_width, image.mImage->image_width);
+        return std::visit(
+            CalChart::overloaded{
+                [](std::shared_ptr<CalChart::ImageData> data) {
+                    return wxSize(data->width, data->width);
+                },
+                [](std::shared_ptr<CalChart::Draw::OpaqueImageData> data) {
+                    return dynamic_cast<wxCalChart::BitmapHolder&>(*data).bitmap.GetSize();
+                },
+            },
+            image.mImage);
     }
 
     inline auto GetMinSize(Context context, CalChart::Draw::DrawManipulators const& cmd) -> StackSize;

diff --git a/src/core/CalChartAnimation.cpp b/src/core/CalChartAnimation.cpp
@@ -230,11 +230,11 @@ auto Animation::GenerateSpritesDrawCommands(beats_t whichBeat, SelectionList con
                 }
                 return *onBeat ? ImageBeat::Left : ImageBeat::Right;
             }();
-            auto image = imageFunction(info.mMarcherInfo.mFacingDirection, image_offset);
+            auto [image, size] = imageFunction(info.mMarcherInfo.mFacingDirection, image_offset, selectionList.contains(index));
             auto position = info.mMarcherInfo.mPosition;
-            auto offset = CalChart::Coord(image->image_width * comp_X, image->image_height * comp_Y);
+            auto offset = CalChart::Coord(size.x * comp_X, size.y * comp_Y);
 
-            return CalChart::Draw::Image{ position, image, selectionList.contains(index) } - offset;
+            return CalChart::Draw::Image{ position, image } - offset;
         }));
     return drawCmds;
 }

diff --git a/src/core/CalChartAnimation.h b/src/core/CalChartAnimation.h
@@ -115,7 +115,7 @@ class Animation {
         Right,
         Size
     };
-    using AngleStepToImageFunction = std::function<std::shared_ptr<ImageData>(Radian, ImageBeat)>;
+    using AngleStepToImageFunction = std::function<std::tuple<std::shared_ptr<Draw::OpaqueImageData>, CalChart::Coord>(Radian, ImageBeat, bool)>;
 
     // Drawing commands
     [[nodiscard]] auto GenerateDotsDrawCommands(beats_t whichBeat, SelectionList const& selectionList, bool drawCollisionWarning, Configuration const& config) const -> std::vector<CalChart::Draw::DrawCommand>;