LibGfx: Saner memory usage of indexed bitmaps

Indexed bitmaps used to allocate four times the required amount of memory.

Also, we should acknowledge that the underlying data is not always RGBA32,
and instead cast it only when the true type is known.

Applications/PixelPaint/BucketTool.cpp

@@ -58,10 +58,10 @@ static void flood_fill(Gfx::Bitmap& bitmap, const Gfx::IntPoint& start_position,
     while (!queue.is_empty()) {
         auto position = queue.dequeue();
 
-        if (bitmap.get_pixel<Gfx::BitmapFormat::RGBA32>(position.x(), position.y()) != target_color)
+        if (bitmap.get_pixel<Gfx::StorageFormat::RGBA32>(position.x(), position.y()) != target_color)
             continue;
 
-        bitmap.set_pixel<Gfx::BitmapFormat::RGBA32>(position.x(), position.y(), fill_color);
+        bitmap.set_pixel<Gfx::StorageFormat::RGBA32>(position.x(), position.y(), fill_color);
 
         if (position.x() != 0)
             queue.enqueue(position.translated(-1, 0));

Applications/PixelPaint/SprayTool.cpp

@@ -77,7 +77,7 @@ void SprayTool::paint_it()
             continue;
         if (ypos < 0 || ypos >= bitmap.height())
             continue;
-        bitmap.set_pixel<Gfx::BitmapFormat::RGB32>(xpos, ypos, m_color);
+        bitmap.set_pixel<Gfx::StorageFormat::RGBA32>(xpos, ypos, m_color);
     }
 
     layer->did_modify_bitmap(*m_editor->image());

Libraries/LibGUI/Clipboard.cpp

@@ -141,7 +141,7 @@ RefPtr<Gfx::Bitmap> Clipboard::bitmap() const
     if (!format.has_value() || format.value() == 0)
         return nullptr;
 
-    auto clipping_bitmap = Gfx::Bitmap::create_wrapper((Gfx::BitmapFormat)format.value(), { (int)width.value(), (int)height.value() }, pitch.value(), (Gfx::RGBA32*)clipping.data.data());
+    auto clipping_bitmap = Gfx::Bitmap::create_wrapper((Gfx::BitmapFormat)format.value(), { (int)width.value(), (int)height.value() }, pitch.value(), clipping.data.data());
     auto bitmap = Gfx::Bitmap::create(Gfx::BitmapFormat::RGBA32, { (int)width.value(), (int)height.value() });
 
     for (int y = 0; y < clipping_bitmap->height(); ++y) {

Libraries/LibGfx/Bitmap.cpp

@@ -44,16 +44,34 @@
 
 namespace Gfx {
 
-static bool size_would_overflow(BitmapFormat, const IntSize& size)
+size_t Bitmap::minimum_pitch(size_t width, BitmapFormat format)
+{
+    size_t element_size;
+    switch (determine_storage_format(format)) {
+    case StorageFormat::Indexed8:
+        element_size = 1;
+        break;
+    case StorageFormat::RGB32:
+    case StorageFormat::RGBA32:
+        element_size = 4;
+        break;
+    default:
+        ASSERT_NOT_REACHED();
+    }
+
+    return width * element_size;
+}
+
+static bool size_would_overflow(BitmapFormat format, const IntSize& size)
 {
     if (size.width() < 0 || size.height() < 0)
         return true;
     // This check is a bit arbitrary, but should protect us from most shenanigans:
     if (size.width() >= 32768 || size.height() >= 32768)
         return true;
-    // This check is absolutely necessary. Note that Bitmap::Bitmap always stores
-    // data as RGBA32 internally, so currently we ignore the indicated format.
-    return Checked<size_t>::multiplication_would_overflow(size.width(), size.height(), sizeof(RGBA32));
+    // In contrast, this check is absolutely necessary:
+    size_t pitch = Bitmap::minimum_pitch(size.width(), format);
+    return Checked<size_t>::multiplication_would_overflow(pitch, size.height());
 }
 
 RefPtr<Bitmap> Bitmap::create(BitmapFormat format, const IntSize& size)
@@ -72,7 +90,7 @@ RefPtr<Bitmap> Bitmap::create_purgeable(BitmapFormat format, const IntSize& size
 
 Bitmap::Bitmap(BitmapFormat format, const IntSize& size, Purgeable purgeable)
     : m_size(size)
-    , m_pitch(round_up_to_power_of_two(size.width() * sizeof(RGBA32), 16))
+    , m_pitch(minimum_pitch(size.width(), format))
     , m_format(format)
     , m_purgeable(purgeable == Purgeable::Yes)
 {
@@ -81,10 +99,10 @@ Bitmap::Bitmap(BitmapFormat format, const IntSize& size, Purgeable purgeable)
     allocate_palette_from_format(format, {});
 #ifdef __serenity__
     int map_flags = purgeable == Purgeable::Yes ? (MAP_PURGEABLE | MAP_PRIVATE) : (MAP_ANONYMOUS | MAP_PRIVATE);
-    m_data = (RGBA32*)mmap_with_name(nullptr, size_in_bytes(), PROT_READ | PROT_WRITE, map_flags, 0, 0, String::format("GraphicsBitmap [%dx%d]", width(), height()).characters());
+    m_data = mmap_with_name(nullptr, size_in_bytes(), PROT_READ | PROT_WRITE, map_flags, 0, 0, String::format("GraphicsBitmap [%dx%d]", width(), height()).characters());
 #else
     int map_flags = (MAP_ANONYMOUS | MAP_PRIVATE);
-    m_data = (RGBA32*)mmap(nullptr, size_in_bytes(), PROT_READ | PROT_WRITE, map_flags, 0, 0);
+    m_data = mmap(nullptr, size_in_bytes(), PROT_READ | PROT_WRITE, map_flags, 0, 0);
 #endif
     if (m_data == MAP_FAILED) {
         perror("mmap");
@@ -94,7 +112,7 @@ Bitmap::Bitmap(BitmapFormat format, const IntSize& size, Purgeable purgeable)
     m_needs_munmap = true;
 }
 
-RefPtr<Bitmap> Bitmap::create_wrapper(BitmapFormat format, const IntSize& size, size_t pitch, RGBA32* data)
+RefPtr<Bitmap> Bitmap::create_wrapper(BitmapFormat format, const IntSize& size, size_t pitch, void* data)
 {
     if (size_would_overflow(format, size))
         return nullptr;
@@ -112,13 +130,16 @@ RefPtr<Bitmap> Bitmap::load_from_file(const StringView& path)
     return nullptr;
 }
 
-Bitmap::Bitmap(BitmapFormat format, const IntSize& size, size_t pitch, RGBA32* data)
+Bitmap::Bitmap(BitmapFormat format, const IntSize& size, size_t pitch, void* data)
     : m_size(size)
     , m_data(data)
     , m_pitch(pitch)
     , m_format(format)
 {
+    ASSERT(pitch >= minimum_pitch(size.width(), format));
     ASSERT(!size_would_overflow(format, size));
+    // FIXME: assert that `data` is actually long enough!
+
     allocate_palette_from_format(format, {});
 }
 
@@ -138,8 +159,8 @@ RefPtr<Bitmap> Bitmap::create_with_shared_buffer(BitmapFormat format, NonnullRef
 
 Bitmap::Bitmap(BitmapFormat format, NonnullRefPtr<SharedBuffer>&& shared_buffer, const IntSize& size, const Vector<RGBA32>& palette)
     : m_size(size)
-    , m_data((RGBA32*)shared_buffer->data())
-    , m_pitch(round_up_to_power_of_two(size.width() * sizeof(RGBA32), 16))
+    , m_data(shared_buffer->data())
+    , m_pitch(minimum_pitch(size.width(), format))
     , m_format(format)
     , m_shared_buffer(move(shared_buffer))
 {

Libraries/LibGfx/Bitmap.h

@@ -56,6 +56,29 @@ enum class BitmapFormat {
     RGBA32,
 };
 
+enum class StorageFormat {
+    Indexed8,
+    RGB32,
+    RGBA32,
+};
+
+static StorageFormat determine_storage_format(BitmapFormat format)
+{
+    switch (format) {
+    case BitmapFormat::RGB32:
+        return StorageFormat::RGB32;
+    case BitmapFormat::RGBA32:
+        return StorageFormat::RGBA32;
+    case BitmapFormat::Indexed1:
+    case BitmapFormat::Indexed2:
+    case BitmapFormat::Indexed4:
+    case BitmapFormat::Indexed8:
+        return StorageFormat::Indexed8;
+    default:
+        ASSERT_NOT_REACHED();
+    }
+}
+
 enum RotationDirection {
     Left,
     Right
@@ -65,7 +88,7 @@ class Bitmap : public RefCounted<Bitmap> {
 public:
     static RefPtr<Bitmap> create(BitmapFormat, const IntSize&);
     static RefPtr<Bitmap> create_purgeable(BitmapFormat, const IntSize&);
-    static RefPtr<Bitmap> create_wrapper(BitmapFormat, const IntSize&, size_t pitch, RGBA32*);
+    static RefPtr<Bitmap> create_wrapper(BitmapFormat, const IntSize&, size_t pitch, void*);
     static RefPtr<Bitmap> load_from_file(const StringView& path);
     static RefPtr<Bitmap> create_with_shared_buffer(BitmapFormat, NonnullRefPtr<SharedBuffer>&&, const IntSize&);
     static RefPtr<Bitmap> create_with_shared_buffer(BitmapFormat, NonnullRefPtr<SharedBuffer>&&, const IntSize&, const Vector<RGBA32>& palette);
@@ -153,6 +176,8 @@ public:
         }
     }
 
+    static size_t minimum_pitch(size_t width, BitmapFormat);
+
     unsigned bpp() const
     {
         return bpp_for_format(m_format);
@@ -170,31 +195,17 @@ public:
     Color palette_color(u8 index) const { return Color::from_rgba(m_palette[index]); }
     void set_palette_color(u8 index, Color color) { m_palette[index] = color.value(); }
 
-    template<BitmapFormat>
-    Color get_pixel(int x, int y) const
-    {
-        (void)x;
-        (void)y;
-        ASSERT_NOT_REACHED();
-    }
-
+    template<StorageFormat>
+    Color get_pixel(int x, int y) const;
     Color get_pixel(int x, int y) const;
-
     Color get_pixel(const IntPoint& position) const
     {
         return get_pixel(position.x(), position.y());
     }
 
-    template<BitmapFormat>
-    void set_pixel(int x, int y, Color)
-    {
-        (void)x;
-        (void)y;
-        ASSERT_NOT_REACHED();
-    }
-
+    template<StorageFormat>
+    void set_pixel(int x, int y, Color);
     void set_pixel(int x, int y, Color);
-
     void set_pixel(const IntPoint& position, Color color)
     {
         set_pixel(position.x(), position.y(), color);
@@ -211,13 +222,13 @@ private:
         Yes
     };
     Bitmap(BitmapFormat, const IntSize&, Purgeable);
-    Bitmap(BitmapFormat, const IntSize&, size_t pitch, RGBA32*);
+    Bitmap(BitmapFormat, const IntSize&, size_t pitch, void*);
     Bitmap(BitmapFormat, NonnullRefPtr<SharedBuffer>&&, const IntSize&, const Vector<RGBA32>& palette);
 
     void allocate_palette_from_format(BitmapFormat, const Vector<RGBA32>& source_palette);
 
     IntSize m_size;
-    RGBA32* m_data { nullptr };
+    void* m_data { nullptr };
     RGBA32* m_palette { nullptr };
     size_t m_pitch { 0 };
     BitmapFormat m_format { BitmapFormat::Invalid };
@@ -229,12 +240,12 @@ private:
 
 inline u8* Bitmap::scanline_u8(int y)
 {
-    return (u8*)m_data + (y * m_pitch);
+    return reinterpret_cast<u8*>(m_data) + (y * m_pitch);
 }
 
 inline const u8* Bitmap::scanline_u8(int y) const
 {
-    return (const u8*)m_data + (y * m_pitch);
+    return reinterpret_cast<const u8*>(m_data) + (y * m_pitch);
 }
 
 inline RGBA32* Bitmap::scanline(int y)
@@ -248,86 +259,57 @@ inline const RGBA32* Bitmap::scanline(int y) const
 }
 
 template<>
-inline Color Bitmap::get_pixel<BitmapFormat::RGB32>(int x, int y) const
+inline Color Bitmap::get_pixel<StorageFormat::RGB32>(int x, int y) const
 {
     return Color::from_rgb(scanline(y)[x]);
 }
 
 template<>
-inline Color Bitmap::get_pixel<BitmapFormat::RGBA32>(int x, int y) const
+inline Color Bitmap::get_pixel<StorageFormat::RGBA32>(int x, int y) const
 {
     return Color::from_rgba(scanline(y)[x]);
 }
 
 template<>
-inline Color Bitmap::get_pixel<BitmapFormat::Indexed1>(int x, int y) const
-{
-    return Color::from_rgb(m_palette[scanline_u8(y)[x]]);
-}
-
-template<>
-inline Color Bitmap::get_pixel<BitmapFormat::Indexed2>(int x, int y) const
-{
-    return Color::from_rgb(m_palette[scanline_u8(y)[x]]);
-}
-
-template<>
-inline Color Bitmap::get_pixel<BitmapFormat::Indexed4>(int x, int y) const
-{
-    return Color::from_rgb(m_palette[scanline_u8(y)[x]]);
-}
-
-template<>
-inline Color Bitmap::get_pixel<BitmapFormat::Indexed8>(int x, int y) const
+inline Color Bitmap::get_pixel<StorageFormat::Indexed8>(int x, int y) const
 {
     return Color::from_rgb(m_palette[scanline_u8(y)[x]]);
 }
 
 inline Color Bitmap::get_pixel(int x, int y) const
 {
-    switch (m_format) {
-    case BitmapFormat::RGB32:
-        return get_pixel<BitmapFormat::RGB32>(x, y);
-    case BitmapFormat::RGBA32:
-        return get_pixel<BitmapFormat::RGBA32>(x, y);
-    case BitmapFormat::Indexed1:
-        return get_pixel<BitmapFormat::Indexed1>(x, y);
-    case BitmapFormat::Indexed2:
-        return get_pixel<BitmapFormat::Indexed2>(x, y);
-    case BitmapFormat::Indexed4:
-        return get_pixel<BitmapFormat::Indexed4>(x, y);
-    case BitmapFormat::Indexed8:
-        return get_pixel<BitmapFormat::Indexed8>(x, y);
+    switch (determine_storage_format(m_format)) {
+    case StorageFormat::RGB32:
+        return get_pixel<StorageFormat::RGB32>(x, y);
+    case StorageFormat::RGBA32:
+        return get_pixel<StorageFormat::RGBA32>(x, y);
+    case StorageFormat::Indexed8:
+        return get_pixel<StorageFormat::Indexed8>(x, y);
     default:
         ASSERT_NOT_REACHED();
     }
 }
 
 template<>
-inline void Bitmap::set_pixel<BitmapFormat::RGB32>(int x, int y, Color color)
+inline void Bitmap::set_pixel<StorageFormat::RGB32>(int x, int y, Color color)
 {
     scanline(y)[x] = color.value();
 }
-
 template<>
-inline void Bitmap::set_pixel<BitmapFormat::RGBA32>(int x, int y, Color color)
+inline void Bitmap::set_pixel<StorageFormat::RGBA32>(int x, int y, Color color)
 {
-    scanline(y)[x] = color.value();
+    scanline(y)[x] = color.value(); // drop alpha
 }
-
 inline void Bitmap::set_pixel(int x, int y, Color color)
 {
-    switch (m_format) {
-    case BitmapFormat::RGB32:
-        set_pixel<BitmapFormat::RGB32>(x, y, color);
+    switch (determine_storage_format(m_format)) {
+    case StorageFormat::RGB32:
+        set_pixel<StorageFormat::RGB32>(x, y, color);
         break;
-    case BitmapFormat::RGBA32:
-        set_pixel<BitmapFormat::RGBA32>(x, y, color);
+    case StorageFormat::RGBA32:
+        set_pixel<StorageFormat::RGBA32>(x, y, color);
         break;
-    case BitmapFormat::Indexed1:
-    case BitmapFormat::Indexed2:
-    case BitmapFormat::Indexed4:
-    case BitmapFormat::Indexed8:
+    case StorageFormat::Indexed8:
         ASSERT_NOT_REACHED();
     default:
         ASSERT_NOT_REACHED();