Skip to content

refactor Image::convert() #1324

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

Closed
mcdubhghlas wants to merge 1 commit into from
Closed

refactor Image::convert() #1324

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
288 changes: 59 additions & 229 deletions core/io/image.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,8 @@
#include "core/config/project_settings.h"
#include "core/error/error_macros.h"
#include "core/io/image_loader.h"
#include "core/io/image_rw.h"
#include "core/io/io.h"
#include "core/io/resource_loader.h"
#include "core/math/math_funcs.h"
#include "core/templates/hash_map.h"
Expand Down Expand Up @@ -475,69 +477,6 @@ int Image::get_mipmap_count() const {
}
}

/// Using template generates perfectly optimized code due to constant expression reduction and unused variable removal present in all compilers.
template <uint32_t read_bytes, bool read_alpha, uint32_t write_bytes, bool write_alpha, bool read_gray, bool write_gray>
static void _convert(int p_width, int p_height, const uint8_t *p_src, uint8_t *p_dst) {
constexpr uint32_t max_bytes = MAX(read_bytes, write_bytes);

for (int y = 0; y < p_height; y++) {
for (int x = 0; x < p_width; x++) {
const uint8_t *rofs = &p_src[((y * p_width) + x) * (read_bytes + (read_alpha ? 1 : 0))];
uint8_t *wofs = &p_dst[((y * p_width) + x) * (write_bytes + (write_alpha ? 1 : 0))];

uint8_t rgba[4] = { 0, 0, 0, 255 };

if constexpr (read_gray) {
rgba[0] = rofs[0];
rgba[1] = rofs[0];
rgba[2] = rofs[0];
} else {
for (uint32_t i = 0; i < max_bytes; i++) {
rgba[i] = (i < read_bytes) ? rofs[i] : 0;
}
}

if constexpr (read_alpha || write_alpha) {
rgba[3] = read_alpha ? rofs[read_bytes] : 255;
}

if constexpr (write_gray) {
// REC.709
const uint8_t luminance = (13938U * rgba[0] + 46869U * rgba[1] + 4729U * rgba[2] + 32768U) >> 16U;
wofs[0] = luminance;
} else {
for (uint32_t i = 0; i < write_bytes; i++) {
wofs[i] = rgba[i];
}
}

if constexpr (write_alpha) {
wofs[write_bytes] = rgba[3];
}
}
}
}

template <typename T, uint32_t read_channels, uint32_t write_channels, T def_zero, T def_one>
static void _convert_fast(int p_width, int p_height, const T *p_src, T *p_dst) {
uint32_t dst_count = 0;
uint32_t src_count = 0;

const int resolution = p_width * p_height;

for (int i = 0; i < resolution; i++) {
memcpy(p_dst + dst_count, p_src + src_count, MIN(read_channels, write_channels) * sizeof(T));

if constexpr (write_channels > read_channels) {
const T def_value[4] = { def_zero, def_zero, def_zero, def_one };
memcpy(p_dst + dst_count + read_channels, &def_value[read_channels], (write_channels - read_channels) * sizeof(T));
}

dst_count += write_channels;
src_count += read_channels;
}
}

static bool _are_formats_compatible(Image::Format p_format0, Image::Format p_format1) {
if (p_format0 <= Image::FORMAT_RGBA8 && p_format1 <= Image::FORMAT_RGBA8) {
return true;
Expand All @@ -551,6 +490,7 @@ static bool _are_formats_compatible(Image::Format p_format0, Image::Format p_for
}

void Image::convert(Format p_new_format) {
IO::Error err = IO::Error::Okay;
ERR_FAIL_INDEX_MSG(p_new_format, FORMAT_MAX, vformat("The Image format specified (%d) is out of range. See Image's Format enum.", p_new_format));

if (data.is_empty() || p_new_format == format) {
Expand Down Expand Up @@ -592,185 +532,75 @@ void Image::convert(Format p_new_format) {
// Convert the formats in an optimized way by removing/adding color channels if necessary.
Image new_img(width, height, mipmaps, p_new_format);

const int conversion_type = format | p_new_format << 8;

for (int mip = 0; mip < mipmap_count; mip++) {
int64_t mip_offset = 0;
int64_t mip_size = 0;
int mip_width = 0;
int mip_height = 0;
IO::Reader reader = {};
IO::Writer writer = {};
IO::Image::Reader imReader = {};
IO::Image::Writer imWriter = {};
ColorRGBAF32x16 block;
get_mipmap_offset_size_and_dimensions(mip, mip_offset, mip_size, mip_width, mip_height);

const uint8_t *rptr = data.ptr() + mip_offset;
uint8_t *wptr = new_img.data.ptrw() + new_img.get_mipmap_offset(mip);

switch (conversion_type) {
case FORMAT_L8 | (FORMAT_LA8 << 8):
_convert<1, false, 1, true, true, true>(mip_width, mip_height, rptr, wptr);
break;
case FORMAT_L8 | (FORMAT_R8 << 8):
_convert<1, false, 1, false, true, false>(mip_width, mip_height, rptr, wptr);
break;
case FORMAT_L8 | (FORMAT_RG8 << 8):
_convert<1, false, 2, false, true, false>(mip_width, mip_height, rptr, wptr);
break;
case FORMAT_L8 | (FORMAT_RGB8 << 8):
_convert<1, false, 3, false, true, false>(mip_width, mip_height, rptr, wptr);
break;
case FORMAT_L8 | (FORMAT_RGBA8 << 8):
_convert<1, false, 3, true, true, false>(mip_width, mip_height, rptr, wptr);
break;
case FORMAT_LA8 | (FORMAT_L8 << 8):
_convert<1, true, 1, false, true, true>(mip_width, mip_height, rptr, wptr);
break;
case FORMAT_LA8 | (FORMAT_R8 << 8):
_convert<1, true, 1, false, true, false>(mip_width, mip_height, rptr, wptr);
break;
case FORMAT_LA8 | (FORMAT_RG8 << 8):
_convert<1, true, 2, false, true, false>(mip_width, mip_height, rptr, wptr);
break;
case FORMAT_LA8 | (FORMAT_RGB8 << 8):
_convert<1, true, 3, false, true, false>(mip_width, mip_height, rptr, wptr);
break;
case FORMAT_LA8 | (FORMAT_RGBA8 << 8):
_convert<1, true, 3, true, true, false>(mip_width, mip_height, rptr, wptr);
break;
case FORMAT_R8 | (FORMAT_L8 << 8):
_convert<1, false, 1, false, false, true>(mip_width, mip_height, rptr, wptr);
break;
case FORMAT_R8 | (FORMAT_LA8 << 8):
_convert<1, false, 1, true, false, true>(mip_width, mip_height, rptr, wptr);
break;
case FORMAT_R8 | (FORMAT_RG8 << 8):
_convert<1, false, 2, false, false, false>(mip_width, mip_height, rptr, wptr);
break;
case FORMAT_R8 | (FORMAT_RGB8 << 8):
_convert<1, false, 3, false, false, false>(mip_width, mip_height, rptr, wptr);
break;
case FORMAT_R8 | (FORMAT_RGBA8 << 8):
_convert<1, false, 3, true, false, false>(mip_width, mip_height, rptr, wptr);
break;
case FORMAT_RG8 | (FORMAT_L8 << 8):
_convert<2, false, 1, false, false, true>(mip_width, mip_height, rptr, wptr);
break;
case FORMAT_RG8 | (FORMAT_LA8 << 8):
_convert<2, false, 1, true, false, true>(mip_width, mip_height, rptr, wptr);
break;
case FORMAT_RG8 | (FORMAT_R8 << 8):
_convert<2, false, 1, false, false, false>(mip_width, mip_height, rptr, wptr);
break;
case FORMAT_RG8 | (FORMAT_RGB8 << 8):
_convert<2, false, 3, false, false, false>(mip_width, mip_height, rptr, wptr);
break;
case FORMAT_RG8 | (FORMAT_RGBA8 << 8):
_convert<2, false, 3, true, false, false>(mip_width, mip_height, rptr, wptr);
break;
case FORMAT_RGB8 | (FORMAT_L8 << 8):
_convert<3, false, 1, false, false, true>(mip_width, mip_height, rptr, wptr);
break;
case FORMAT_RGB8 | (FORMAT_LA8 << 8):
_convert<3, false, 1, true, false, true>(mip_width, mip_height, rptr, wptr);
break;
case FORMAT_RGB8 | (FORMAT_R8 << 8):
_convert<3, false, 1, false, false, false>(mip_width, mip_height, rptr, wptr);
break;
case FORMAT_RGB8 | (FORMAT_RG8 << 8):
_convert<3, false, 2, false, false, false>(mip_width, mip_height, rptr, wptr);
break;
case FORMAT_RGB8 | (FORMAT_RGBA8 << 8):
_convert<3, false, 3, true, false, false>(mip_width, mip_height, rptr, wptr);
break;
case FORMAT_RGBA8 | (FORMAT_L8 << 8):
_convert<3, true, 1, false, false, true>(mip_width, mip_height, rptr, wptr);
break;
case FORMAT_RGBA8 | (FORMAT_LA8 << 8):
_convert<3, true, 1, true, false, true>(mip_width, mip_height, rptr, wptr);
break;
case FORMAT_RGBA8 | (FORMAT_R8 << 8):
_convert<3, true, 1, false, false, false>(mip_width, mip_height, rptr, wptr);
break;
case FORMAT_RGBA8 | (FORMAT_RG8 << 8):
_convert<3, true, 2, false, false, false>(mip_width, mip_height, rptr, wptr);
break;
case FORMAT_RGBA8 | (FORMAT_RGB8 << 8):
_convert<3, true, 3, false, false, false>(mip_width, mip_height, rptr, wptr);
break;
case FORMAT_RH | (FORMAT_RGH << 8):
_convert_fast<uint16_t, 1, 2, 0x0000, 0x3C00>(mip_width, mip_height, (const uint16_t *)rptr, (uint16_t *)wptr);
break;
case FORMAT_RH | (FORMAT_RGBH << 8):
_convert_fast<uint16_t, 1, 3, 0x0000, 0x3C00>(mip_width, mip_height, (const uint16_t *)rptr, (uint16_t *)wptr);
break;
case FORMAT_RH | (FORMAT_RGBAH << 8):
_convert_fast<uint16_t, 1, 4, 0x0000, 0x3C00>(mip_width, mip_height, (const uint16_t *)rptr, (uint16_t *)wptr);
break;
case FORMAT_RGH | (FORMAT_RH << 8):
_convert_fast<uint16_t, 2, 1, 0x0000, 0x3C00>(mip_width, mip_height, (const uint16_t *)rptr, (uint16_t *)wptr);
break;
case FORMAT_RGH | (FORMAT_RGBH << 8):
_convert_fast<uint16_t, 2, 3, 0x0000, 0x3C00>(mip_width, mip_height, (const uint16_t *)rptr, (uint16_t *)wptr);
break;
case FORMAT_RGH | (FORMAT_RGBAH << 8):
_convert_fast<uint16_t, 2, 4, 0x0000, 0x3C00>(mip_width, mip_height, (const uint16_t *)rptr, (uint16_t *)wptr);
break;
case FORMAT_RGBH | (FORMAT_RH << 8):
_convert_fast<uint16_t, 3, 1, 0x0000, 0x3C00>(mip_width, mip_height, (const uint16_t *)rptr, (uint16_t *)wptr);
break;
case FORMAT_RGBH | (FORMAT_RGH << 8):
_convert_fast<uint16_t, 3, 2, 0x0000, 0x3C00>(mip_width, mip_height, (const uint16_t *)rptr, (uint16_t *)wptr);
break;
case FORMAT_RGBH | (FORMAT_RGBAH << 8):
_convert_fast<uint16_t, 3, 4, 0x0000, 0x3C00>(mip_width, mip_height, (const uint16_t *)rptr, (uint16_t *)wptr);
break;
case FORMAT_RGBAH | (FORMAT_RH << 8):
_convert_fast<uint16_t, 4, 1, 0x0000, 0x3C00>(mip_width, mip_height, (const uint16_t *)rptr, (uint16_t *)wptr);
break;
case FORMAT_RGBAH | (FORMAT_RGH << 8):
_convert_fast<uint16_t, 4, 2, 0x0000, 0x3C00>(mip_width, mip_height, (const uint16_t *)rptr, (uint16_t *)wptr);
break;
case FORMAT_RGBAH | (FORMAT_RGBH << 8):
_convert_fast<uint16_t, 4, 3, 0x0000, 0x3C00>(mip_width, mip_height, (const uint16_t *)rptr, (uint16_t *)wptr);
break;
case FORMAT_RF | (FORMAT_RGF << 8):
_convert_fast<uint32_t, 1, 2, 0x00000000, 0x3F800000>(mip_width, mip_height, (const uint32_t *)rptr, (uint32_t *)wptr);
break;
case FORMAT_RF | (FORMAT_RGBF << 8):
_convert_fast<uint32_t, 1, 3, 0x00000000, 0x3F800000>(mip_width, mip_height, (const uint32_t *)rptr, (uint32_t *)wptr);
break;
case FORMAT_RF | (FORMAT_RGBAF << 8):
_convert_fast<uint32_t, 1, 4, 0x00000000, 0x3F800000>(mip_width, mip_height, (const uint32_t *)rptr, (uint32_t *)wptr);
break;
case FORMAT_RGF | (FORMAT_RF << 8):
_convert_fast<uint32_t, 2, 1, 0x00000000, 0x3F800000>(mip_width, mip_height, (const uint32_t *)rptr, (uint32_t *)wptr);
break;
case FORMAT_RGF | (FORMAT_RGBF << 8):
_convert_fast<uint32_t, 2, 3, 0x00000000, 0x3F800000>(mip_width, mip_height, (const uint32_t *)rptr, (uint32_t *)wptr);
break;
case FORMAT_RGF | (FORMAT_RGBAF << 8):
_convert_fast<uint32_t, 2, 4, 0x00000000, 0x3F800000>(mip_width, mip_height, (const uint32_t *)rptr, (uint32_t *)wptr);
break;
case FORMAT_RGBF | (FORMAT_RF << 8):
_convert_fast<uint32_t, 3, 1, 0x00000000, 0x3F800000>(mip_width, mip_height, (const uint32_t *)rptr, (uint32_t *)wptr);
break;
case FORMAT_RGBF | (FORMAT_RGF << 8):
_convert_fast<uint32_t, 3, 2, 0x00000000, 0x3F800000>(mip_width, mip_height, (const uint32_t *)rptr, (uint32_t *)wptr);
break;
case FORMAT_RGBF | (FORMAT_RGBAF << 8):
_convert_fast<uint32_t, 3, 4, 0x00000000, 0x3F800000>(mip_width, mip_height, (const uint32_t *)rptr, (uint32_t *)wptr);
break;
case FORMAT_RGBAF | (FORMAT_RF << 8):
_convert_fast<uint32_t, 4, 1, 0x00000000, 0x3F800000>(mip_width, mip_height, (const uint32_t *)rptr, (uint32_t *)wptr);
break;
case FORMAT_RGBAF | (FORMAT_RGF << 8):
_convert_fast<uint32_t, 4, 2, 0x00000000, 0x3F800000>(mip_width, mip_height, (const uint32_t *)rptr, (uint32_t *)wptr);
break;
case FORMAT_RGBAF | (FORMAT_RGBF << 8):
_convert_fast<uint32_t, 4, 3, 0x00000000, 0x3F800000>(mip_width, mip_height, (const uint32_t *)rptr, (uint32_t *)wptr);
break;
IO::Reader::make(
&reader,
{
.data = (void*)rptr,
.length = (size_t)mip_size,
}
);
err = IO::Image::Reader::make(
&imReader,
reader,
format,
mip_width,
mip_height
);
if (err != IO::Error::Okay)
{
break;
}
new_img.get_mipmap_offset_and_size(mip, mip_offset, mip_size);
err = IO::Writer::make(
&writer,
{
.data = (void*)wptr,
.length = (size_t)(mip_size),
}
);
if (err != IO::Error::Okay)
{
break;
}
IO::Image::Writer::make(
&imWriter,
writer,
new_img.format,
mip_width,
mip_height
);
Comment on lines +549 to +585

@coderabbitai coderabbitai Bot Jul 24, 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.

🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "Files matching image_rw.cpp/image.cpp:"
fd -a 'image_rw\.cpp$|image\.cpp$' . | sed 's#^\./##'

echo
echo "Relevant image.cpp region:"
if [ -f core/io/image.cpp ]; then
  sed -n '520,610p' core/io/image.cpp | nl -ba -v520
fi

echo
echo "Locate IO::Image::Writer::make definitions and uses:"
rg -n "namespace IO|class Writer|Writer::make|void IO::Image::Writer::make|IO::Image::Writer::make|struct Image" -S . | head -200

echo
echo "Locate writer implementation in image_rw files:"
fd -i '.*image.*[.](cpp|h|hpp)$' . -x sh -n {} -c 'echo "--- {}", wc -l "$1"; sed -n "1,220p" "$1"' sh {}

Repository: Redot-Engine/redot-engine

Length of output: 540

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "Relevant image.cpp region:"
sed -n '520,620p' core/io/image.cpp

echo
echo "Relevant image_rw.cpp region around Image::Writer::make:"
rg -n "Image::Writer::make|Image::Writer::flush|Image::Reader::make|Reader::make|Writer::make|read|write|NULL|nullptr|NotImplemented|FORMAT_RGB565|_are_formats_compatible" core/io/image_rw.cpp core/io/image.cpp

echo
echo "image_rw.cpp implementation summary:"
python3 - <<'PY'
from pathlib import Path
p = Path('core/io/image_rw.cpp')
s = p.read_text().splitlines()
targets = [
    'IO::Image::Writer::make',
    'IO::Image::Writer::flush',
    'IO::Image::Reader::make',
    'IO::Image::Reader::read',
    'IO::Image::Writer::write',
]
for target in targets:
    print(f'--- {target}')
    found = False
    for i,line in enumerate(s):
        if target in line or ('Writer' in line and 'VTBL' in line):
            found = True
            start = max(0,i-5); end=min(len(s),i+80)
            for n in range(start,end):
                print(f'{n+1}: {s[n]}')
            break
    if not found:
        print('not found')
PY

echo
echo "image.cpp relevant compact parse-ish:"
python3 - <<'PY'
from pathlib import Path
p = Path('core/io/image.cpp')
lines=p.read_text().splitlines()
for n in range(500,610):
    print(f'{n}: {lines[n-1]}')
PY

Repository: Redot-Engine/redot-engine

Length of output: 48493

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "Image format compatibility functions:"
python3 - <<'PY'
from pathlib import Path
p = Path('core/io/image.cpp').read_text().splitlines()
for start,title in [(450,'is_format_compressed'),(470,'_are_formats_compatible')]:
    print('---', title)
    for _ in range(start, min(len(p),start+45)):
        print(f'{_:1}: {p[_]}')
PY

echo
echo "io.h image/vtbl declarations: Reader::make and Image::Writer::make:"
fd -i 'io.*\.(h|hpp)$' . -x sh -n {} -c 'echo "--- $1"; rg -n "Reader::make|Image::Writer::make|struct Writer|vtbl|Writer::write|Writer::flush|Error::Okay|NotImplemented" "$1"' sh {}

echo
echo "image_rw.cpp Image::Writer::make and Image::Writer::write implementations:"
python3 - <<'PY'
from pathlib import Path
lines = Path('core/io/image_rw.cpp').read_text().splitlines()
for label, pat in [('Image::Writer::make','IO::Error Writer::make'),('Image::Writer::write','void Writer::write')]:
    print('---', label)
    found=False
    for i,line in enumerate(lines):
        if pat in line:
            found=True
            for n in range(max(0,i-5), min(len(lines),i+90)):
                print(f'{n+1}: {lines[n]}')
            break
    if not found:
        print('not found')
PY

echo
echo "Programmatic check: compatible formats and Image::Writer write vtbl null entries:"
python3 - <<'PY'
from pathlib import Path
txt = Path('core/io/image.cpp').read_text()
rw = Path('core/io/image_rw.cpp').read_text()

# Extract likely scalar write table blocks by index alignment with enum values.
# Locate "FlushProc defaultFlush" block around FORMAT_RGB565 comments.
import re
for idx, label in enumerate(['FORMAT_RGB565', 'FORMAT_RGBA4444', 'FORMAT_RGBE9995']):
    m = re.search(rf'WriteProc\)nullptr, \.destroy = free \}, // {label}$', rw, re.M)
    print(label, 'exists:', bool(m), 'line:', m.start().splitlines(True)[:1][0] if m else None)

# Locate is_format_compressed helper definitions textually and parse brace block.
start = txt.find('static bool Image::is_format_compressed(Image::Format p_format)')
end = txt.find('\n', txt.find('}', start))
print('is_format_compressed excerpt:', '\n'.join(txt[start:start+280].splitlines(maxsplit=4)[:5]))

# Locate _are_formats_compatible block and extract formatted bool-ish table.
start = txt.find('bool Image::_are_formats_compatible(Image::Format p_format0, Image::Format p_format1)')
print('found _are_formats_compatible start:', start != -1)
snippet = '\n'.join(txt[start:start+400].splitlines()[:80])
print(snippet)
PY

Repository: Redot-Engine/redot-engine

Length of output: 41789

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "image_rw.cpp Image::Writer::make / Writer::write implementation:"
sed -n '1020,1120p' core/io/image_rw.cpp

echo
echo "image_rw.cpp Image::Writer destructor/destroy implementation:"
sed -n '1090,1115p' core/io/image_rw.cpp

echo
echo "io.h relevant declarations:"
sed -n '1,90p' core/io/io.h

echo
echo "Enum/format declaration around scalar image formats:"
sed -n '1,190p' core/io/image_rw.cpp

echo
echo "Focused write table entries:"
grep -n "FORMAT_RGB565\|FORMAT_RGBA4444\|FORMAT_RGBE9995\|.flush = defaultFlush, .write = \.write = \.destroy = free" core/io/image_rw.cpp

Repository: Redot-Engine/redot-engine

Length of output: 11119

Propagate IO::Image::Writer::make() failure before the conversion loop.

IO::Image::Writer::make(&imWriter, ...) returns IO::Error::NotImplemented when write is nullptr in the destination format’s vtbl; FORMAT_RGB565, FORMAT_RGBA4444, and FORMAT_RGBE9995 have null writes, and they are covered by _are_formats_compatible. When this happens in the optimized path with format = FORMAT_RGB8 going to p_new_format = FORMAT_RGB565, the code skips error handling and later calls IO::Image::Writer::write(imWriter, &block), dereferencing a null function pointer. Store and check the return value, destroying any initialized handles before returning.

🤖 Prompt for AI Agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@core/io/image.cpp` around lines 549 - 585, Capture the return value of
IO::Image::Writer::make in the conversion setup before the loop, and handle
non-Okay results like the existing reader and writer initialization failures.
Destroy any already initialized imReader, reader, and writer handles before
returning the error, preventing IO::Image::Writer::write from using an invalid
writer.

do
{
err = IO::Image::Reader::read(imReader, &block);
if (err == IO::Error::Okay)
{
err = IO::Image::Writer::write(imWriter, &block);
}
} while (err == IO::Error::Okay);
err = IO::Image::Writer::flush(imWriter);
IO::Image::Reader::destroy(&imReader);
IO::Image::Writer::destroy(&imWriter);
IO::Reader::destroy(&reader);
IO::Writer::destroy(&writer);
if (err != IO::Error::Okay) { return; }
}

_copy_internals_from(new_img);
return;
}
Comment on lines +556 to 604

@coderabbitai coderabbitai Bot Jul 24, 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.

🗄️ Data Integrity & Integration | 🔴 Critical | ⚡ Quick win

On make() failure, break falls through to an unconditional _copy_internals_from(new_img), committing a corrupted image.

Both failure paths (563-566, 575-578) break out of the mip loop, but the code after the loop (line 602) unconditionally calls _copy_internals_from(new_img); return; regardless of whether the loop broke early due to an error. This differs from the do…while loop's own error handling just below (line 599: if (err != IO::Error::Okay) { return; }), which correctly avoids committing on failure. As a result, a make() failure for any mip (e.g. RGB565, per the comment above) will still overwrite this with a partially/never-populated new_img, and skips destroying the already-created reader/imReader handles.

🐛 Proposed fix: return (with cleanup) instead of break 
 		if (err != IO::Error::Okay)
 		{
-			break;
+			IO::Reader::destroy(&reader);
+			return;
 		}

(apply analogously to the second failure branch, additionally destroying imReader)

🤖 Prompt for AI Agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@core/io/image.cpp` around lines 556 - 604, Update both `make()` failure
branches in the mip-processing loop to clean up all handles already created for
the current mip and return immediately instead of breaking to
`_copy_internals_from(new_img)`. Ensure the `IO::Reader`, `IO::Writer`, and
image reader/writer handles are destroyed appropriately, including `imReader` in
the second failure path, so failed mip creation cannot commit the partially
initialized image.


Image::Format Image::get_format() const {
Expand Down
13 changes: 13 additions & 0 deletions core/io/image.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,19 @@ typedef Error (*SaveDDSFunc)(const String &p_path, const Ref<Image> &p_img);
typedef Vector<uint8_t> (*SaveDDSBufferFunc)(const Ref<Image> &p_img);
/// @}

// by 16 for avx512 eventually (TM)
union alignas(64) ColorRGBAF32x16
{
struct
{
float r[16];
float g[16];
float b[16];
float a[16];
};
float c[4][16];
};

class Image : public Resource {
GDCLASS(Image, Resource);

Expand Down
Loading
Loading