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

[joeykchen]

No description provided.

[fennoai]

FennoAI Review — PR #321: match Scratch SVG MIP power-of-two scaling rule

The two refactors are net improvements: the per-row memcpy + convertToRGBA() path is cleaner than the manual per-pixel ARGB→RGBA unpack (and also correctly honors row stride and un-premultiplies alpha), and the power-of-two loop is more readable than the fixed thresholds.

However, raising the scale cap from 8 to 1024 makes several latent size-math weaknesses in create_image_from_utf8_buffer practically reachable. The most serious findings are a 32-bit integer overflow in the allocation size and a missing null/dimension check before the copy loop — together these can under-allocate and then write out of bounds. Details are inline.

Additional notes (no reliable inline anchor):

• modules/svg/image_loader_svg.cpp:40 — #include <iostream> appears unused (the file uses Godot's ERR_FAIL_* / vformat, no std::cout/cerr). Pre-existing, not introduced here, but this PR adds the correct #include <cstring> for memcpy, so removing the stale <iostream> while you're here would be reasonable cleanup.
• Consider whether the new behavior warrants a test (e.g. a semi-transparent SVG to lock in the convertToRGBA un-premultiply behavior, and a scale boundary case).

Non-blocking review (event: COMMENT).

[fennoai]

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.

[fennoai]

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.

[fennoai]

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.

[fennoai]

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.

[gemini-code-assist]

Code Review

This pull request refactors SVG scale calculation to use absolute values and dynamically compute power-of-two scales up to 1024, matching Scratch's SVG MIP rule. It also optimizes SVG image loading by using lunasvg's convertToRGBA() and replacing a manual pixel-copying loop with a faster memcpy operation. Feedback was provided regarding a potential crash if renderToBitmap returns a null bitmap, suggesting a safety check before proceeding with conversion and copying.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

[gemini-code-assist]

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();