From 9c3a33762be4ba4ee361b537516179c3c469d61a Mon Sep 17 00:00:00 2001 From: Ben Wiederhake Date: Sun, 6 Sep 2020 23:59:20 +0200 Subject: [PATCH] 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 | 4 +- Applications/PixelPaint/SprayTool.cpp | 2 +- Libraries/LibGUI/Clipboard.cpp | 2 +- Libraries/LibGfx/Bitmap.cpp | 43 ++++++--- Libraries/LibGfx/Bitmap.h | 124 +++++++++++-------------- 5 files changed, 89 insertions(+), 86 deletions(-) diff --git a/Applications/PixelPaint/BucketTool.cpp b/Applications/PixelPaint/BucketTool.cpp index 92f3ab2aa8..978df6a5ef 100644 --- a/Applications/PixelPaint/BucketTool.cpp +++ b/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(position.x(), position.y()) != target_color) + if (bitmap.get_pixel(position.x(), position.y()) != target_color) continue; - bitmap.set_pixel(position.x(), position.y(), fill_color); + bitmap.set_pixel(position.x(), position.y(), fill_color); if (position.x() != 0) queue.enqueue(position.translated(-1, 0)); diff --git a/Applications/PixelPaint/SprayTool.cpp b/Applications/PixelPaint/SprayTool.cpp index 85a6200f97..f1f73c29aa 100644 --- a/Applications/PixelPaint/SprayTool.cpp +++ b/Applications/PixelPaint/SprayTool.cpp @@ -77,7 +77,7 @@ void SprayTool::paint_it() continue; if (ypos < 0 || ypos >= bitmap.height()) continue; - bitmap.set_pixel(xpos, ypos, m_color); + bitmap.set_pixel(xpos, ypos, m_color); } layer->did_modify_bitmap(*m_editor->image()); diff --git a/Libraries/LibGUI/Clipboard.cpp b/Libraries/LibGUI/Clipboard.cpp index a711fa2597..33c64c7157 100644 --- a/Libraries/LibGUI/Clipboard.cpp +++ b/Libraries/LibGUI/Clipboard.cpp @@ -141,7 +141,7 @@ RefPtr 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) { diff --git a/Libraries/LibGfx/Bitmap.cpp b/Libraries/LibGfx/Bitmap.cpp index 484e6ff733..6b306f40f7 100644 --- a/Libraries/LibGfx/Bitmap.cpp +++ b/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::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::multiplication_would_overflow(pitch, size.height()); } RefPtr Bitmap::create(BitmapFormat format, const IntSize& size) @@ -72,7 +90,7 @@ RefPtr 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::create_wrapper(BitmapFormat format, const IntSize& size, size_t pitch, RGBA32* data) +RefPtr 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::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::create_with_shared_buffer(BitmapFormat format, NonnullRef Bitmap::Bitmap(BitmapFormat format, NonnullRefPtr&& shared_buffer, const IntSize& size, const Vector& 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)) { diff --git a/Libraries/LibGfx/Bitmap.h b/Libraries/LibGfx/Bitmap.h index f60ddfdd0a..022c841b80 100644 --- a/Libraries/LibGfx/Bitmap.h +++ b/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 { public: static RefPtr create(BitmapFormat, const IntSize&); static RefPtr create_purgeable(BitmapFormat, const IntSize&); - static RefPtr create_wrapper(BitmapFormat, const IntSize&, size_t pitch, RGBA32*); + static RefPtr create_wrapper(BitmapFormat, const IntSize&, size_t pitch, void*); static RefPtr load_from_file(const StringView& path); static RefPtr create_with_shared_buffer(BitmapFormat, NonnullRefPtr&&, const IntSize&); static RefPtr create_with_shared_buffer(BitmapFormat, NonnullRefPtr&&, const IntSize&, const Vector& 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 - Color get_pixel(int x, int y) const - { - (void)x; - (void)y; - ASSERT_NOT_REACHED(); - } - + template + 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 - void set_pixel(int x, int y, Color) - { - (void)x; - (void)y; - ASSERT_NOT_REACHED(); - } - + template + 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&&, const IntSize&, const Vector& palette); void allocate_palette_from_format(BitmapFormat, const Vector& 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(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(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(int x, int y) const +inline Color Bitmap::get_pixel(int x, int y) const { return Color::from_rgb(scanline(y)[x]); } template<> -inline Color Bitmap::get_pixel(int x, int y) const +inline Color Bitmap::get_pixel(int x, int y) const { return Color::from_rgba(scanline(y)[x]); } template<> -inline Color Bitmap::get_pixel(int x, int y) const -{ - return Color::from_rgb(m_palette[scanline_u8(y)[x]]); -} - -template<> -inline Color Bitmap::get_pixel(int x, int y) const -{ - return Color::from_rgb(m_palette[scanline_u8(y)[x]]); -} - -template<> -inline Color Bitmap::get_pixel(int x, int y) const -{ - return Color::from_rgb(m_palette[scanline_u8(y)[x]]); -} - -template<> -inline Color Bitmap::get_pixel(int x, int y) const +inline Color Bitmap::get_pixel(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(x, y); - case BitmapFormat::RGBA32: - return get_pixel(x, y); - case BitmapFormat::Indexed1: - return get_pixel(x, y); - case BitmapFormat::Indexed2: - return get_pixel(x, y); - case BitmapFormat::Indexed4: - return get_pixel(x, y); - case BitmapFormat::Indexed8: - return get_pixel(x, y); + switch (determine_storage_format(m_format)) { + case StorageFormat::RGB32: + return get_pixel(x, y); + case StorageFormat::RGBA32: + return get_pixel(x, y); + case StorageFormat::Indexed8: + return get_pixel(x, y); default: ASSERT_NOT_REACHED(); } } template<> -inline void Bitmap::set_pixel(int x, int y, Color color) +inline void Bitmap::set_pixel(int x, int y, Color color) { scanline(y)[x] = color.value(); } - template<> -inline void Bitmap::set_pixel(int x, int y, Color color) +inline void Bitmap::set_pixel(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(x, y, color); + switch (determine_storage_format(m_format)) { + case StorageFormat::RGB32: + set_pixel(x, y, color); break; - case BitmapFormat::RGBA32: - set_pixel(x, y, color); + case StorageFormat::RGBA32: + set_pixel(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();