Skip to content

fix: match Scratch SVG MIP power-of-two scaling rule #321

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
joeykchen wants to merge 1 commit into
base: spx4.4.1
Choose a base branch
from
Open

fix: match Scratch SVG MIP power-of-two scaling rule #321

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 10 additions & 10 deletions modules/spx/svg_mgr.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
#include "svg_mgr.h"

#include "core/io/image_loader.h"
#include "core/math/math_funcs.h"

#include "spx.h"
#include "spx_engine.h"
Expand Down Expand Up @@ -201,22 +202,21 @@ void SvgManager::update_caches(const Vector<String> &files) {
}

int SvgManager::calculate_svg_scale(Vector2 required_scale) {
float scale = MAX(required_scale.x, required_scale.y);
float scale = MAX(Math::abs(required_scale.x), Math::abs(required_scale.y));
return calculate_svg_scale(scale);
}

int SvgManager::calculate_svg_scale(float required_scale) {
// Use powers of 2: 1, 2, 4, 8, 16...
if (required_scale <= 1.5f) {
float scale = Math::abs(required_scale);
if (scale <= 1.0f) {

@fennoai fennoai Bot Jul 3, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Behavior change: threshold shifted 1.5 → 1.0. The old code returned scale 1 for any required_scale <= 1.5; this returns 1 only for scale <= 1.0 and 2 for anything in (1.0, 2.0]. So content rendered at 1.1x–1.5x (a common near-native range) now rasterizes at 2x — roughly 4x the texture memory and raster cost. If matching Scratch's "not smaller than requested" rule is the explicit intent, this is correct by design; flagging so reviewers confirm the trade-off is intended.

return 1;
}

if (required_scale <= 3.0f) {
return 2;
// Match Scratch's SVG MIP rule: choose the smallest power-of-two scale
// whose rasterized texture is not smaller than the requested render scale.
int target_scale = 1;
while ((float)target_scale < scale && target_scale < 1024) {

@fennoai fennoai Bot Jul 3, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Scale cap raised 8 → 1024 (high-impact enabler). The old code hard-capped the rasterization multiplier at 8; this loop caps at 1024. Because the loader allocates width * height * 4 bytes, the maximum rasterized area grows by up to ~16,000x, which is what makes the integer-overflow and null-bitmap paths in image_loader_svg.cpp reachable, and can also request multi-GB allocations (memory-exhaustion / DoS) even without overflow.

Consider capping the final pixel dimensions (width×height after scaling) rather than only the multiplier, and confirm 1024 is intentional. Also note the comment above ("smallest power-of-two … not smaller than the requested render scale") no longer holds once scale > 1024, where the result is silently clamped and becomes smaller than requested — worth documenting the ceiling.

target_scale <<= 1;
}

if (required_scale <= 6.0f) {
return 4;
}
return 8;
return target_scale;
}
16 changes: 6 additions & 10 deletions modules/svg/image_loader_svg.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@

#include <lunasvg.h>

#include <cstring>
#include <iostream>

HashMap<Color, Color> ImageLoaderSVG::forced_color_map = HashMap<Color, Color>();
Expand Down Expand Up @@ -98,21 +99,16 @@ Error ImageLoaderSVG::create_image_from_utf8_buffer(Ref<Image> p_image, const ui
height *= p_scale;

auto bitmap = document->renderToBitmap(width, height, 0x00000000);
bitmap.convertToRGBA();

@fennoai fennoai Bot Jul 3, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing null/empty-bitmap check; copy bounded by requested rather than actual dimensions (high). document->renderToBitmap(width, height, ...) returns an empty Bitmap() (with data() == nullptr, stride() == 0) when the surface can't be created — e.g. when the requested dimensions are too large (now far more likely with the 1024 cap) or allocation fails. This loop still uses the caller-requested width/height as bounds, so on a null bitmap the first memcpy dereferences nullptr (crash), and more generally the copy trusts the requested geometry rather than the bitmap's real width()/height().

Recommend: after renderToBitmap, verify !bitmap.isNull() (bail with ERR_INVALID_DATA otherwise), and drive the resize, the copy bounds, and set_data from bitmap.width()/bitmap.height() — the authoritative dimensions — rather than the requested width/height.

Comment on lines 101 to +102

@gemini-code-assist gemini-code-assist Bot Jul 3, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

If renderToBitmap fails (e.g., due to memory allocation failure or rendering issues), it may return an empty/invalid bitmap where bitmap.data() is nullptr. Calling convertToRGBA() or subsequently performing memcpy on a null buffer will result in a crash or undefined behavior. It is safer to check if bitmap.data() is nullptr before proceeding.

Suggested change
auto bitmap = document->renderToBitmap(width, height, 0x00000000);
bitmap.convertToRGBA();
auto bitmap = document->renderToBitmap(width, height, 0x00000000);
if (bitmap.data() == nullptr) {
return ERR_INVALID_DATA;
}
bitmap.convertToRGBA();


Vector<uint8_t> result;
result.resize(width * height * 4);

uint32_t *buffer = (uint32_t *)bitmap.data();

const uint8_t *buffer = bitmap.data();
const int stride = bitmap.stride();
uint8_t *dst = result.ptrw();
for (uint32_t y = 0; y < height; y++) {
for (uint32_t x = 0; x < width; x++) {
uint32_t n = buffer[y * width + x];
const size_t offset = sizeof(uint32_t) * width * y + sizeof(uint32_t) * x;
result.write[offset + 0] = (n >> 16) & 0xff;
result.write[offset + 1] = (n >> 8) & 0xff;
result.write[offset + 2] = n & 0xff;
result.write[offset + 3] = (n >> 24) & 0xff;
}
memcpy(dst + (width * 4 * y), buffer + (stride * y), width * 4);

@fennoai fennoai Bot Jul 3, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Integer overflow in the allocation size (high). width and height are uint32_t, so width * height * 4 on the result.resize(...) above is evaluated entirely in 32-bit unsigned arithmetic and wraps modulo 2^32 before being widened to the int64_t that Vector::resize accepts (CowData::Size is int64_t in this tree). With the scale cap now raised to 1024, a moderately large source SVG can push width/height high enough that this product wraps to a small value — resize then allocates a small buffer while width/height remain huge, and this memcpy loop writes far past the end (heap buffer overflow).

Compute the size in 64-bit and reject/clamp before allocating, e.g. (uint64_t)width * height * 4, with an explicit upper bound.

}

p_image->set_data(width, height, false, Image::FORMAT_RGBA8, result);
Expand Down
Loading