From ea59db5acd8fd0e6d61c6885cb1e48590239e051 Mon Sep 17 00:00:00 2001 From: Si Date: Sat, 26 Sep 2026 18:31:53 +0200 Subject: [PATCH] Refuse loads and renders that would not fit in memory Opening a project of ninety 17 MP photos got Effector killed by the kernel's OOM killer: 4.6 GB of decoded RGB on a machine with 3.6 GB free, and no core dump to show for it. Bulk allocations are now sized first and refused with an OutOfMemoryError past three quarters of the free memory (MemAvailable, or less under a cgroup memory.max). loadImages sizes the batch from the file headers and refuses it before decoding anything, and holds the decodes in flight to what is left; renderChain and renderFrames refuse a canvas at 24 bytes a pixel, plus any picture a Path param loads. The interface keeps a refused batch's paths, so saving the project never drops them. Version 0.6.1. Co-Authored-By: Claude Opus 5.5 --- CHANGELOG.md | 11 ++++ CLAUDE.md | 15 +++++ CMakeLists.txt | 3 +- packaging/arch/PKGBUILD | 2 +- src/core/chain.cpp | 26 +++++++++ src/core/image.cpp | 27 ++++++++- src/core/image.hpp | 9 +++ src/core/memory.cpp | 123 ++++++++++++++++++++++++++++++++++++++++ src/core/memory.hpp | 39 +++++++++++++ src/gui/window.cpp | 35 +++++++++++- src/gui/window.hpp | 6 ++ tests/core_tests.cpp | 67 ++++++++++++++++++++++ tests/gui_tests.cpp | 42 ++++++++++++++ 13 files changed, 401 insertions(+), 4 deletions(-) create mode 100644 src/core/memory.cpp create mode 100644 src/core/memory.hpp diff --git a/CHANGELOG.md b/CHANGELOG.md index 704d1d4..f828460 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,17 @@ Every release of Effector, newest first. The version lives in one place, the `project()` line of `CMakeLists.txt`, and each release is a `vX.Y.Z` tag on the commit that made it. The interface shows this file under Changelog in its menu. +## [0.6.1] - 2026-09-26 + +### Fixed +- Opening a project whose images would not fit in memory no longer gets + Effector killed by the system. The images are measured before any is read, + and a batch that would not fit is refused with a message saying how much it + needs; the images stay in the project, so saving it keeps them. +- A canvas too large to render in the free memory is refused the same way, + in the interface and on the command line, instead of taking the machine's + memory down with it. + ## [0.6.0] - 2026-09-26 ### Added diff --git a/CLAUDE.md b/CLAUDE.md index 34a77b7..e114c8c 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -261,6 +261,21 @@ then copies pixels in parallel. copy-on-write via `mutateSources()`, so a render in flight can never race with an add/remove. +### Memory is checked before it is taken + +Running out of memory does not fail cleanly on Linux: the kernel's OOM killer +ends the program (and often the terminal or session it runs in), leaving no +core dump. So bulk allocations are sized first and refused with an +`OutOfMemoryError` (`src/core/memory.hpp`) when they would take more than three +quarters of `availableMemory()` — the system's MemAvailable, or less under a +cgroup `memory.max`. `loadImages` sizes the whole batch from the file headers +(`imageFilePixels`) and refuses it before decoding anything, and holds the +decodes it runs at once to what is left; `renderChain` and `renderFrames` +refuse a canvas at 24 bytes a pixel (13 measured, plus the interface's copy), +plus any picture a `Path` param loads. The interface keeps a refused batch's +paths in `refusedSources_`, so saving the project never drops them; the tests +fake a small machine with `limitAvailableMemory`. + ### Image buffer `Image` is tightly packed 8-bit RGB with stride exactly `width * 3` — effects diff --git a/CMakeLists.txt b/CMakeLists.txt index 0ea4f7a..0a83737 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -1,5 +1,5 @@ cmake_minimum_required(VERSION 3.20) -project(effector VERSION 0.6.0 LANGUAGES CXX) +project(effector VERSION 0.6.1 LANGUAGES CXX) set(CMAKE_CXX_STANDARD 20) set(CMAKE_CXX_STANDARD_REQUIRED ON) @@ -35,6 +35,7 @@ add_library(effector_core STATIC src/core/chain.cpp src/core/effect.cpp src/core/image.cpp + src/core/memory.cpp src/core/params.cpp src/core/project.cpp src/core/registry.cpp diff --git a/packaging/arch/PKGBUILD b/packaging/arch/PKGBUILD index bc75631..7728724 100644 --- a/packaging/arch/PKGBUILD +++ b/packaging/arch/PKGBUILD @@ -2,7 +2,7 @@ # so pkgver must follow the project() line of CMakeLists.txt (the version core # test checks it). From this folder: makepkg -si pkgname=effector -pkgver=0.6.0 +pkgver=0.6.1 pkgrel=1 pkgdesc='Image effects app: a GTK4 interface and a headless CLI over pluggable effects' arch=('x86_64' 'aarch64') diff --git a/src/core/chain.cpp b/src/core/chain.cpp index a8ded83..d841f6b 100644 --- a/src/core/chain.cpp +++ b/src/core/chain.cpp @@ -2,6 +2,8 @@ #include +#include "core/memory.hpp" + namespace effector { std::vector enabledStages(const std::vector& stages) { @@ -48,6 +50,28 @@ std::vector runningStages(const std::vector& stag return running; } +// Throws OutOfMemoryError for a render that would not fit. A render holds a +// few canvases at once -- the stage before, the one being drawn, the input +// a transform copies -- and the file it is saved to; 13 bytes a pixel is +// what the heaviest chains were measured at, and the interface keeps one +// more copy to show, so 24 leaves room. A picture a stage loads itself (an +// overlay, a text box's picture) comes on top, with its alpha and pixbuf. +void ensureRenderFits(const std::vector& running, int width, int height, + const StyleSet& styles) { + std::size_t needed = static_cast(std::max(width, 0)) * + static_cast(std::max(height, 0)) * 24; + for (const auto* stage : running) { + const auto values = stageValues(*stage, styles); + for (const auto& spec : stage->effect->params()) { + if (spec.type != ParamSpec::Type::Path) continue; + const std::string path = paramText(values, spec.key, ""); + if (!path.empty()) needed += imageFilePixels(path) * 8; + } + } + ensureMemory(needed, "Rendering a " + std::to_string(width) + " × " + std::to_string(height) + + " canvas"); +} + // Runs running[first, last) on `image`, which holds the sources' place for // the first stage of the chain and the previous stage's output after it. Image runStages(const std::vector& running, std::size_t first, @@ -76,6 +100,7 @@ Image renderChain(const std::vector& stages, const std::vector& stages, const std::vector const VideoSettings& video, const std::function& onFrame) { const auto running = runningStages(stages); + ensureRenderFits(running, width, height, styles); const int count = video.frameCount(); const int fps = std::clamp(video.fps, kMinFps, kMaxFps); diff --git a/src/core/image.cpp b/src/core/image.cpp index aa218a1..cf37662 100644 --- a/src/core/image.cpp +++ b/src/core/image.cpp @@ -11,6 +11,8 @@ #include #include +#include "core/memory.hpp" + namespace effector { namespace { @@ -152,9 +154,31 @@ void Image::save(const std::string& path) const { if (saved == FALSE) throw std::runtime_error(takeError(error)); } +std::size_t imageFilePixels(const std::string& path) { + int width = 0; + int height = 0; + if (gdk_pixbuf_get_file_info(path.c_str(), &width, &height) == nullptr) return 0; + return static_cast(std::max(width, 0)) * static_cast(std::max(height, 0)); +} + std::vector loadImages(const std::vector& paths, std::vector& failures, const LoadProgress* progress) { + // What the batch holds once decoded, and what one decode needs on top + // while it runs: the pixbuf (with alpha, at worst) and its rotated copy. + std::size_t kept = 0; + std::size_t largest = 0; + for (const auto& path : paths) { + const std::size_t pixels = imageFilePixels(path); + kept += pixels * 3; + largest = std::max(largest, pixels * 8); + } + const std::string what = paths.size() == 1 + ? std::string("Loading this image") + : "Loading these " + std::to_string(paths.size()) + " images"; + ensureMemory(kept + largest, what); + const std::size_t spare = usableMemory() - std::min(usableMemory(), kept); + // Decoding is the dominant cost of a headless run, and the files are // independent, so they are decoded in parallel. Threads take the next path // from a shared cursor rather than a fixed slice, so one huge file cannot @@ -167,7 +191,8 @@ std::vector loadImages(const std::vector& paths, unsigned hardware = std::thread::hardware_concurrency(); if (hardware == 0) hardware = 1; - const std::size_t workers = std::min(hardware, std::max(paths.size(), 1)); + std::size_t workers = std::min(hardware, std::max(paths.size(), 1)); + if (largest > 0) workers = std::clamp(spare / largest, 1, workers); const auto decode = [&] { for (;;) { diff --git a/src/core/image.hpp b/src/core/image.hpp index ffa584d..c446df2 100644 --- a/src/core/image.hpp +++ b/src/core/image.hpp @@ -68,9 +68,18 @@ struct LoadProgress { std::atomic* cancel = nullptr; }; +// The pixel count of an image file from its header alone, without decoding +// it; 0 when the header cannot be read. +std::size_t imageFilePixels(const std::string& path); + // Loads every path, collecting rather than throwing on individual failures. // When `progress->cancel` is set the remaining paths are skipped, so the result // is short and should be discarded rather than used. +// +// Before decoding anything the whole batch is sized from the file headers, +// and a batch that would not fit in the free memory throws OutOfMemoryError +// (core/memory.hpp) with nothing loaded; the decodes that run at once are +// also held to what is left over. std::vector loadImages(const std::vector& paths, std::vector& failures, const LoadProgress* progress = nullptr); diff --git a/src/core/memory.cpp b/src/core/memory.cpp new file mode 100644 index 0000000..3e1689a --- /dev/null +++ b/src/core/memory.cpp @@ -0,0 +1,123 @@ +#include "core/memory.hpp" + +#include + +#include +#include +#include +#include +#include +#include + +namespace effector { +namespace { + +std::atomic cap{0}; + +// MemAvailable from /proc/meminfo, falling back on the free pages; 0 when +// neither says. +std::size_t systemAvailable() { + std::ifstream meminfo("/proc/meminfo"); + std::string key; + std::size_t kilobytes = 0; + std::string unit; + while (meminfo >> key >> kilobytes >> unit) { + if (key == "MemAvailable:") return kilobytes * 1024; + } + const long pages = sysconf(_SC_AVPHYS_PAGES); + const long pageSize = sysconf(_SC_PAGESIZE); + if (pages > 0 && pageSize > 0) { + return static_cast(pages) * static_cast(pageSize); + } + return 0; +} + +// A number from a cgroup file, or nothing for "max" or a file that is not +// there. +bool readCount(const std::filesystem::path& file, std::size_t& out) { + std::ifstream in(file); + std::string word; + if (!(in >> word) || word == "max") return false; + try { + out = static_cast(std::stoull(word)); + return true; + } catch (const std::exception&) { + return false; + } +} + +std::size_t inactiveFile(const std::filesystem::path& dir) { + std::ifstream in(dir / "memory.stat"); + std::string key; + std::size_t value = 0; + while (in >> key >> value) { + if (key == "inactive_file") return value; + } + return 0; +} + +// The room left under the tightest memory.max on the way from this +// process's cgroup (v2) up to the root, counting the page cache it could +// drop as room; the largest value when nothing caps it. +std::size_t cgroupAvailable() { + std::size_t room = std::numeric_limits::max(); + std::ifstream self("/proc/self/cgroup"); + std::string line; + while (std::getline(self, line)) { + if (line.rfind("0::", 0) != 0) continue; + // "0::/user.slice/app.slice/x.scope", walked from the scope upwards. + for (std::filesystem::path group = line.substr(4); !group.empty(); + group = group.parent_path()) { + const auto dir = std::filesystem::path("/sys/fs/cgroup") / group; + std::size_t max = 0; + std::size_t current = 0; + if (!readCount(dir / "memory.max", max) || !readCount(dir / "memory.current", current)) { + continue; + } + const std::size_t used = current - std::min(current, inactiveFile(dir)); + room = std::min(room, max - std::min(max, used)); + } + } + return room; +} + +} // namespace + +std::size_t availableMemory() { + std::size_t available = systemAvailable(); + const std::size_t room = cgroupAvailable(); + if (room != std::numeric_limits::max()) { + available = available == 0 ? room : std::min(available, room); + } + const std::size_t capped = cap.load(std::memory_order_relaxed); + if (capped != 0 && (available == 0 || capped < available)) available = capped; + return available; +} + +void limitAvailableMemory(std::size_t bytes) { cap.store(bytes, std::memory_order_relaxed); } + +std::size_t usableMemory() { + const std::size_t available = availableMemory(); + return available == 0 ? std::numeric_limits::max() : available / 4 * 3; +} + +void ensureMemory(std::size_t needed, const std::string& what) { + const std::size_t available = availableMemory(); + if (available == 0 || needed <= available / 4 * 3) return; + throw OutOfMemoryError(what + " needs about " + formatBytes(needed) + + " of memory, more than is safe to take with " + formatBytes(available) + + " free."); +} + +std::string formatBytes(std::size_t bytes) { + char text[32]; + const double gb = static_cast(bytes) / (1024.0 * 1024.0 * 1024.0); + if (gb >= 1.0) { + std::snprintf(text, sizeof text, "%.1f GB", gb); + } else { + std::snprintf(text, sizeof text, "%.0f MB", static_cast(bytes) / (1024.0 * 1024.0)); + } + return text; +} + +} // namespace effector diff --git a/src/core/memory.hpp b/src/core/memory.hpp new file mode 100644 index 0000000..485dfdf --- /dev/null +++ b/src/core/memory.hpp @@ -0,0 +1,39 @@ +#pragma once + +#include +#include +#include + +namespace effector { + +// Thrown instead of starting work that would not fit in memory. Decoding a +// batch of photographs or rendering a huge canvas past what the machine has +// free does not fail cleanly: the kernel's OOM killer ends the program, and +// often others with it. +class OutOfMemoryError : public std::runtime_error { +public: + using std::runtime_error::runtime_error; +}; + +// The bytes this process could still allocate without swapping: the +// system's MemAvailable, or less when a cgroup (a systemd unit, a container) +// caps the process below it. Zero when neither can be read. +std::size_t availableMemory(); + +// Caps what availableMemory() reports, for the tests; 0 lifts the cap. +void limitAvailableMemory(std::size_t bytes); + +// Throws OutOfMemoryError when `needed` more bytes would take more than three +// quarters of availableMemory() -- the rest is left to the desktop and to +// whatever else the program holds. `what` names the work in the message. +// Does nothing when the free memory cannot be read. +void ensureMemory(std::size_t needed, const std::string& what); + +// The bytes ensureMemory() would let through right now; the largest value +// when the free memory cannot be read. +std::size_t usableMemory(); + +// "4.6 GB", "380 MB". +std::string formatBytes(std::size_t bytes); + +} // namespace effector diff --git a/src/gui/window.cpp b/src/gui/window.cpp index 05bd237..d77f2bf 100644 --- a/src/gui/window.cpp +++ b/src/gui/window.cpp @@ -10,6 +10,7 @@ #include "core/canvas.hpp" #include "core/chain.hpp" +#include "core/memory.hpp" #include "core/project.hpp" #include "core/registry.hpp" #include "core/rng.hpp" @@ -1349,11 +1350,19 @@ void EffectorWindow::addPaths(const std::vector& paths) { }; std::vector failures; - auto images = loadImages(paths, failures, &progress); + std::vector images; + std::string refused; + try { + images = loadImages(paths, failures, &progress); + } catch (const OutOfMemoryError& error) { + refused = error.what(); + } { std::lock_guard lock(loadMutex_); loadedImages_ = std::move(images); loadFailures_ = std::move(failures); + loadRefused_ = std::move(refused); + loadRefusedPaths_ = loadRefused_.empty() ? std::vector{} : paths; } loadFinished_.emit(); }); @@ -1384,12 +1393,18 @@ void EffectorWindow::onLoadFinished() { std::vector images; std::vector failures; + std::string refused; + std::vector refusedPaths; { std::lock_guard lock(loadMutex_); images = std::move(loadedImages_); failures = std::move(loadFailures_); + refused = std::move(loadRefused_); + refusedPaths = std::move(loadRefusedPaths_); loadedImages_.clear(); loadFailures_.clear(); + loadRefused_.clear(); + loadRefusedPaths_.clear(); } if (loadCancel_.load(std::memory_order_relaxed)) { @@ -1406,6 +1421,22 @@ void EffectorWindow::onLoadFinished() { dropLoadedImages_ = false; images.clear(); failures.clear(); + refused.clear(); + refusedPaths.clear(); + } + + if (!refused.empty()) { + // Nothing of a refused batch was decoded. Its paths are kept for the + // project file, and whatever was queued behind it still gets its turn. + refusedSources_.insert(refusedSources_.end(), refusedPaths.begin(), refusedPaths.end()); + showError(refused + " They were not loaded, but stay in the project when it is saved. " + "Close other programs, or load fewer or smaller images."); + if (!loadQueue_.empty()) { + auto queued = std::move(loadQueue_); + loadQueue_.clear(); + addPaths(queued); + } + return; } for (const auto& image : images) appendSourceRow(image.path()); @@ -2273,6 +2304,7 @@ Gtk::Widget& EffectorWindow::buildMainMenu() { Project EffectorWindow::currentProject() const { Project project; for (const auto& image : *sources_) project.sources.push_back(image.path()); + project.sources.insert(project.sources.end(), refusedSources_.begin(), refusedSources_.end()); project.stages = stages_; project.styles = styles_; project.width = static_cast(widthSpin_.get_value()); @@ -2320,6 +2352,7 @@ void EffectorWindow::clearSources() { while (auto* row = sourceList_.get_row_at_index(0)) sourceList_.remove(*row); updateSourcesLabel(); mutateSources([](SourceList& list) { list.clear(); }); + refusedSources_.clear(); loadQueue_.clear(); if (loading_) dropLoadedImages_ = true; diff --git a/src/gui/window.hpp b/src/gui/window.hpp index 5172eff..e01cfc0 100644 --- a/src/gui/window.hpp +++ b/src/gui/window.hpp @@ -219,6 +219,12 @@ private: std::string loadCurrent_; std::vector loadedImages_; std::vector loadFailures_; + std::string loadRefused_; // why the batch would not fit in memory, if it would not + std::vector loadRefusedPaths_; + // Sources refused as too large to hold. They are not shown or rendered, + // but a project saved now keeps them, so a refusal never costs the + // project its images. + std::vector refusedSources_; std::vector loadQueue_; // chosen while another batch was loading bool loading_ = false; bool dropLoadedImages_ = false; // the batch in flight belongs to a replaced project diff --git a/tests/core_tests.cpp b/tests/core_tests.cpp index 9b8359b..beb4a84 100644 --- a/tests/core_tests.cpp +++ b/tests/core_tests.cpp @@ -20,6 +20,7 @@ #include "cli/options.hpp" #include "core/canvas.hpp" #include "core/chain.hpp" +#include "core/memory.hpp" #include "core/project.hpp" #include "core/registry.hpp" #include "core/rng.hpp" @@ -1718,6 +1719,72 @@ int main() { std::filesystem::remove_all(dir); } + std::cout << "memory\n"; + { + check(availableMemory() > 0, "the free memory can be read"); + check(formatBytes(std::size_t{5} << 29) == "2.5 GB" && formatBytes(std::size_t{380} << 20) == "380 MB", + "sizes are written in GB and MB"); + + const auto dir = std::filesystem::temp_directory_path() / "effector_test_memory"; + std::filesystem::create_directories(dir); + std::vector paths; + for (int i = 0; i < 3; ++i) { + const auto path = dir / ("src" + std::to_string(i) + ".png"); + gradient(400, 300, i + 1).save(path.string()); + paths.push_back(path.string()); + } + check(imageFilePixels(paths[0]) == 400u * 300 && imageFilePixels((dir / "missing.png").string()) == 0, + "an image is sized from its header"); + + // Three 400 x 300 images hold 1 MB decoded; with 1 MB free they do not fit. + limitAvailableMemory(std::size_t{1} << 20); + std::atomic decoded{0}; + LoadProgress counting; + counting.onFile = [&](std::size_t, std::size_t, const std::string&) { ++decoded; }; + std::vector failures; + bool refused = false; + std::string message; + try { + loadImages(paths, failures, &counting); + } catch (const OutOfMemoryError& error) { + refused = true; + message = error.what(); + } + check(refused, "a batch too large for the free memory is refused"); + check(decoded.load() == 0, "before a single file is decoded"); + check(message.find("Loading these 3 images needs about") == 0 && message.find("with 1 MB free") != std::string::npos, + "and the message says how much is needed and how much is free"); + + const std::vector scanlines = {{effectByName("scanlines"), {}}}; + const std::vector small = {gradient(64, 64, 7)}; + bool renderRefused = false; + try { + Rng rng(1); + renderChain(scanlines, small, 4000, 4000, rng); + } catch (const OutOfMemoryError&) { + renderRefused = true; + } + check(renderRefused, "a render too large for the free memory is refused"); + bool videoRefused = false; + try { + VideoSettings clip; + clip.enabled = true; + renderFrames(scanlines, small, 4000, 4000, 1, {}, clip, [](int, Image&&) { return true; }); + } catch (const OutOfMemoryError&) { + videoRefused = true; + } + check(videoRefused, "and so is a clip"); + Rng smallRng(1); + check(renderChain(scanlines, small, 100, 100, smallRng).width() == 100, + "a render that fits still runs"); + + limitAvailableMemory(0); + failures.clear(); + check(loadImages(paths, failures).size() == 3, "with the memory there the batch loads"); + + std::filesystem::remove_all(dir); + } + std::cout << "video\n"; { using effector::ChainStage; diff --git a/tests/gui_tests.cpp b/tests/gui_tests.cpp index 020268a..2b18067 100644 --- a/tests/gui_tests.cpp +++ b/tests/gui_tests.cpp @@ -14,6 +14,8 @@ #include #include "cli/options.hpp" +#include "core/image.hpp" +#include "core/memory.hpp" #include "core/project.hpp" #include "core/registry.hpp" #include "core/version.hpp" @@ -514,6 +516,45 @@ void testVideo(Gtk::Window& window, const std::filesystem::path& dir) { stack->set_visible_child("chain"); } +// A project whose images will not fit is refused before any is decoded, and +// saving it then must not quietly drop them from the file. +void testMemoryGuard(Gtk::Window& window, const std::filesystem::path& dir) { + std::cout << "memory guard\n"; + std::filesystem::create_directories(dir); + effector::Project project; + for (int i = 0; i < 2; ++i) { + const auto source = dir / ("big" + std::to_string(i) + ".png"); + effector::Image(600, 600).save(source.string()); + project.sources.push_back(source.string()); + } + project.stages.push_back({effector::effectByName("scanlines"), {}}); + const auto path = dir / "big.json"; + effector::saveProject(path.string(), project); + + effector::limitAvailableMemory(std::size_t{1} << 20); // 2.1 MB of images, 1 MB free + window.activate_action("win.open-recent", + Glib::Variant::create(Glib::filename_to_uri(path.string()))); + pump(500); + auto* sources = named(window, "sources"); + auto lists = sources != nullptr ? all(*sources) : std::vector{}; + check(!lists.empty() && rowCount(*lists.front()) == 0, "images too large for the memory are not loaded"); + bool told = false; + for (auto* label : all(window)) { + const std::string text = label->get_text(); + if (label->is_drawable() && text.find("Loading these 2 images needs about") != std::string::npos && + text.find("stay in the project") != std::string::npos) { + told = true; + } + } + check(told, "and the window says why"); + + window.activate_action("win.save-project"); + pump(100); + check(effector::loadProject(path.string()).sources.size() == 2, + "saving the project keeps the images it could not load"); + effector::limitAvailableMemory(0); +} + } // namespace int main(int argc, char** argv) { @@ -542,6 +583,7 @@ int main(int argc, char** argv) { testHelpWindows(*window); testRecentProjects(*window, dataDir / "projects"); testVideo(*window, dataDir / "video"); + testMemoryGuard(*window, dataDir / "memory"); app->remove_window(*window); }); const int status = app->run(argc > 0 ? 1 : 0, argv);