Avoid setting an invalid mask color (-1) in doc::Image (fix #4651)

This regression came from 09bb5cc3d3 as
now we don't Sprite::setTransparentColor() on each undo/redo and only
when needed. This brought a new kind of error where the mask color for
images was set to -1 after convert_pixel_format().

This also fixes a conversion from Indexed -> RGB where the transparent
color was not set back to 0. And the transparent color is always set
when we are in indexed mode to avoid any assert in debug mode.

src/app/cmd/set_pixel_format.cpp

@@ -139,20 +139,38 @@ SetPixelFormat::SetPixelFormat(Sprite* sprite,
     }
   }
 
-  // Set all cels opacity to 100% if we are converting to indexed.
+  // By default, when converting to RGB or grayscale, the mask color
+  // is always 0.
+  int newMaskIndex = 0;
   if (newFormat == IMAGE_INDEXED) {
+    // Set all cels opacity to 100% if we are converting to indexed.
     // TODO remove this (?)
     for (Cel* cel : sprite->uniqueCels()) {
       if (cel->opacity() < 255)
         m_pre.add(new cmd::SetCelOpacity(cel, 255));
     }
 
-    int newMaskIndex = sprite->palette(0)->findMaskColor();
+    // When converting to indexed mode the mask color depends if the
+    // palette includes a fully transparent entry.
+    newMaskIndex = sprite->palette(0)->findMaskColor();
     if (newMaskIndex < 0)
       newMaskIndex = 0;
+
+    // We change the transparent color after (m_post) changing the
+    // color mode (when we are already in indexed mode).
     if (newMaskIndex != sprite->transparentColor())
       m_post.add(new cmd::SetTransparentColor(sprite, newMaskIndex));
   }
+  else if (m_oldFormat == IMAGE_INDEXED) {
+    // We change the transparent color before (m_pre) changing the
+    // color mode (when we are still in indexed mode).
+    if (newMaskIndex != sprite->transparentColor())
+      m_pre.add(new cmd::SetTransparentColor(sprite, newMaskIndex));
+  }
+  else {
+    // RGB <-> Grayscale
+    ASSERT(sprite->transparentColor() == 0);
+  }
 
   // When we are converting to grayscale color mode, we've to destroy
   // all palettes and put only one grayscaled-palette at the first

src/app/util/clipboard.cpp

@@ -230,21 +230,13 @@ void Clipboard::setData(Image* image,
     // Copy tilemap to the native clipboard
     if (isTilemap) {
       ASSERT(tileset);
-      setNativeBitmap(image, mask, palette, tileset);
+      setNativeBitmap(image, mask, palette, tileset, -1);
     }
     // Copy non-tilemap images to the native clipboard
     else {
-      color_t oldMask = 0;
+      setNativeBitmap(
-      if (image) {
+        image, mask, palette, nullptr,
-        oldMask = image->maskColor();
+        image_source_is_transparent ? image->maskColor(): -1);
-        if (!image_source_is_transparent)
-          image->setMaskColor(-1);
-      }
-
-      setNativeBitmap(image, mask, palette);
-
-      if (image && !image_source_is_transparent)
-        image->setMaskColor(oldMask);
     }
   }
 }

src/app/util/clipboard.h

@@ -108,7 +108,8 @@ namespace app {
     bool setNativeBitmap(const doc::Image* image,
                          const doc::Mask* mask,
                          const doc::Palette* palette,
-                         const doc::Tileset* tileset = nullptr);
+                         const doc::Tileset* tileset,
+                         const doc::color_t indexMaskColor);
     bool getNativeBitmap(doc::Image** image,
                          doc::Mask** mask,
                          doc::Palette** palette,

src/app/util/clipboard_native.cpp

@@ -95,7 +95,8 @@ bool Clipboard::hasNativeBitmap() const
 bool Clipboard::setNativeBitmap(const doc::Image* image,
                                 const doc::Mask* mask,
                                 const doc::Palette* palette,
-                                const doc::Tileset* tileset)
+                                const doc::Tileset* tileset,
+                                const doc::color_t indexMaskColor)
 {
   clip::lock l(native_window_handle());
   if (!l.locked())
@@ -180,7 +181,7 @@ bool Clipboard::setNativeBitmap(const doc::Image* image,
           doc::color_t c = palette->getEntry(*it);
 
           // Use alpha=0 for mask color
-          if (*it == image->maskColor())
+          if (*it == indexMaskColor)
             c &= doc::rgba_rgb_mask;
 
           *(dst++) = c;

src/doc/image_spec.h

@@ -1,5 +1,5 @@
 // Aseprite Document Library
-// Copyright (C) 2018-2020 Igara Studio S.A.
+// Copyright (C) 2018-2024 Igara Studio S.A.
 // Copyright (c) 2016 David Capello
 //
 // This file is released under the terms of the MIT license.
@@ -54,8 +54,18 @@ namespace doc {
     void setColorMode(const ColorMode colorMode) { m_colorMode = colorMode; }
     void setWidth(const int width) { m_size.w = width; }
     void setHeight(const int height) { m_size.h = height; }
-    void setMaskColor(const color_t color) { m_maskColor = color; }
+
-    void setColorSpace(const gfx::ColorSpaceRef& cs) { m_colorSpace = cs; }
+    void setMaskColor(const color_t color) {
+#if 0 // Sometimes, mask color = -1 is temporarily used to paint an
+      // opaque indexed image in PixelsMovement.
+      ASSERT(color != -1);
+#endif
+      m_maskColor = color;
+    }
+
+    void setColorSpace(const gfx::ColorSpaceRef& cs) {
+      m_colorSpace = cs;
+    }
 
     void setSize(const int width,
                  const int height) {

src/doc/sprite.cpp

@@ -250,8 +250,11 @@ bool Sprite::supportAlpha() const
 void Sprite::setTransparentColor(color_t color)
 {
 #if _DEBUG
-  if (colorMode() != ColorMode::INDEXED) {
+  if (colorMode() == ColorMode::INDEXED) {
-    ASSERT(color == 0);
+    ASSERT(color != -1);       // Setting mask = -1 is a logic error
+  }
+  else {
+    ASSERT(color == 0);        // Always 0 for non-indexed color modes
   }
 #endif // _DEBUG
 

src/render/quantization.cpp

@@ -142,18 +142,21 @@ Palette* create_palette_from_sprite(
 Image* convert_pixel_format(
   const Image* image,
   Image* new_image,
-  PixelFormat pixelFormat,
+  const PixelFormat pixelFormat,
   const Dithering& dithering,
   const RgbMap* rgbmap,
   const Palette* palette,
-  bool is_background,
+  const bool is_background,
-  color_t new_mask_color,
+  const color_t new_mask_color,
   rgba_to_graya_func toGray,
   TaskDelegate* delegate)
 {
   if (!new_image)
     new_image = Image::create(pixelFormat, image->width(), image->height());
-  new_image->setMaskColor(new_mask_color);
+
+  // Don't set the image mask color to -1
+  const color_t new_mask_color0 = (new_mask_color == -1 ? 0 : new_mask_color);
+  new_image->setMaskColor(new_mask_color0);
 
   // RGB -> Indexed with ordered dithering
   if (image->pixelFormat() == IMAGE_RGB &&
@@ -236,7 +239,7 @@ Image* convert_pixel_format(
             a = rgba_geta(c);
 
             if (a == 0)
-              *dst_it = (new_mask_color == -1? 0 : new_mask_color);
+              *dst_it = new_mask_color0;
             else if (rgbmap)
               *dst_it = rgbmap->mapColor(c);
             else
@@ -293,7 +296,7 @@ Image* convert_pixel_format(
             c = graya_getv(c);
 
             if (a == 0)
-              *dst_it = (new_mask_color == -1? 0 : new_mask_color);
+              *dst_it = new_mask_color0;
             else if (rgbmap)
               *dst_it = rgbmap->mapColor(c, c, c, a);
             else
@@ -372,7 +375,7 @@ Image* convert_pixel_format(
             c = *src_it;
 
             if (!is_background && c == image->maskColor())
-              *dst_it = new_mask_color;
+              *dst_it = new_mask_color0;
             else {
               c = palette->getEntry(c);
               r = rgba_getr(c);