From e365721e21d76630f2a7ba19b4a1a1c45b67b61e Mon Sep 17 00:00:00 2001 From: Arnaud Botella Date: Wed, 21 Oct 2026 09:10:50 +0200 Subject: [PATCH 01/12] fix(VTKOutput): never leave a truncated file when writing fails The output file was truncated as soon as the writer was created, so any failure (exception, full disk) left an empty or partial file and lost the previous one. The file is now written in a temporary file which replaces the output only once complete. Co-Authored-By: Claude Opus 5.5 --- include/geode/io/image/detail/vtk_output.hpp | 74 ++++++++++++++++++-- tests/image/CMakeLists.txt | 1 + tests/image/test-raster-image.cpp | 70 ++++++++++++++++++ 3 files changed, 141 insertions(+), 4 deletions(-) diff --git a/include/geode/io/image/detail/vtk_output.hpp b/include/geode/io/image/detail/vtk_output.hpp index 60f4f441..b0d1cbef 100644 --- a/include/geode/io/image/detail/vtk_output.hpp +++ b/include/geode/io/image/detail/vtk_output.hpp @@ -29,6 +29,7 @@ #include #include #include +#include #include #include #include @@ -193,17 +194,37 @@ namespace geode { write_with_appended_data( root ); } - file_.flush(); - OpenGeodeIOImageException::check_exception( file_.good(), + file_.close(); + OpenGeodeIOImageException::check_exception( !file_.fail(), nullptr, OpenGeodeException::TYPE::data, "[VTKOutput] Error while writing file: ", filename_ ); + written_ = true; + if( temporary_filename_.empty() ) + { + return; + } + std::error_code error; + std::filesystem::rename( + temporary_filename_, target_filename_, error ); + if( error ) + { + std::filesystem::remove( temporary_filename_, error ); + throw OpenGeodeIOImageException{ nullptr, + OpenGeodeException::TYPE::data, + "[VTKOutput] Error while writing file: ", filename_, + " (", error.message(), ")" }; + } } protected: VTKOutputImpl( std::string_view filename, const Mesh& mesh, const char* type ) : filename_{ filename }, - file_{ to_string( filename ), std::ios::binary }, + target_filename_{ target_file( filename ) }, + temporary_filename_{ temporary_file( target_filename_ ) }, + file_{ temporary_filename_.empty() ? target_filename_ + : temporary_filename_, + std::ios::binary }, mesh_( mesh ), type_{ type } { @@ -212,7 +233,49 @@ namespace geode "[VTKOutput] Error while writing file: ", filename ); } - virtual ~VTKOutputImpl() {} + // The file is written in a temporary file which replaces the + // output file once complete: a failure never leaves a truncated + // file nor erases an existing one + virtual ~VTKOutputImpl() + { + if( written_ || temporary_filename_.empty() ) + { + return; + } + file_.close(); + std::error_code error; + std::filesystem::remove( temporary_filename_, error ); + } + + // Symbolic links are followed to replace their target + static std::string target_file( std::string_view filename ) + { + const std::filesystem::path path{ to_string( filename ) }; + std::error_code error; + if( std::filesystem::is_symlink( path, error ) ) + { + const auto target = + std::filesystem::canonical( path, error ); + if( !error ) + { + return target.string(); + } + } + return path.string(); + } + + // Special files (e.g. devices) cannot be replaced: they are written + // directly, without temporary file + static std::string temporary_file( const std::string& target ) + { + std::error_code error; + const auto status = std::filesystem::status( target, error ); + if( std::filesystem::is_other( status ) ) + { + return {}; + } + return absl::StrCat( target, ".tmp" ); + } const Mesh& mesh() const { @@ -583,7 +646,10 @@ namespace geode private: std::string_view filename_; + std::string target_filename_; + std::string temporary_filename_; std::ofstream file_; + bool written_{ false }; const Mesh& mesh_; pugi::xml_document document_; const char* type_; diff --git a/tests/image/CMakeLists.txt b/tests/image/CMakeLists.txt index 2dc2b510..14ee4926 100755 --- a/tests/image/CMakeLists.txt +++ b/tests/image/CMakeLists.txt @@ -24,5 +24,6 @@ add_geode_test( OpenGeode::basic OpenGeode::mesh ${PROJECT_NAME}::image + pugixml::pugixml ESSENTIAL ) \ No newline at end of file diff --git a/tests/image/test-raster-image.cpp b/tests/image/test-raster-image.cpp index e307a9f4..32741c3e 100644 --- a/tests/image/test-raster-image.cpp +++ b/tests/image/test-raster-image.cpp @@ -23,6 +23,7 @@ #include +#include #include #include @@ -42,6 +43,7 @@ #include #include +#include #include void test_jpg_from_gimp_input() @@ -158,6 +160,72 @@ void test_empty_image_output() "[TEST] Wrong extent written for empty image" ); } +class FailingVTIOutput + : public geode::detail::VTIOutputImpl< geode::RasterImage2D > +{ +public: + FailingVTIOutput( + const geode::RasterImage2D& raster, std::string_view filename ) + : geode::detail::VTIOutputImpl< geode::RasterImage2D >{ raster, + filename } + { + } + +private: + void write_piece( pugi::xml_node& /*unused*/ ) final + { + throw geode::OpenGeodeIOImageException{ nullptr, + geode::OpenGeodeException::TYPE::data, "Failure while writing" }; + } +}; + +void test_failed_output_keeps_existing_file() +{ + const auto filename = "existing.vti"; + std::ofstream{ filename } << "previous content"; + const geode::RasterImage2D raster{ { 2, 2 } }; + bool thrown{ false }; + try + { + FailingVTIOutput output{ raster, filename }; + output.write_file(); + } + catch( const geode::OpenGeodeException& ) + { + thrown = true; + } + geode::OpenGeodeIOImageException::test( + thrown, "[TEST] Failing output should throw" ); + std::ifstream file{ filename }; + const std::string content{ std::istreambuf_iterator< char >{ file }, + std::istreambuf_iterator< char >{} }; + geode::OpenGeodeIOImageException::test( content == "previous content", + "[TEST] Failing output should keep the existing file" ); + geode::OpenGeodeIOImageException::test( + !std::filesystem::exists( absl::StrCat( filename, ".tmp" ) ), + "[TEST] Failing output should not leave a temporary file" ); +} + +void test_output_through_symbolic_link() +{ +#ifdef __linux__ + std::filesystem::remove( "link.vti" ); + std::ofstream{ "link_target.vti" } << "previous content"; + std::filesystem::create_symlink( "link_target.vti", "link.vti" ); + const geode::RasterImage2D raster{ { 2, 2 } }; + geode::save_raster_image( raster, "link.vti" ); + geode::OpenGeodeIOImageException::test( + std::filesystem::is_symlink( "link.vti" ), + "[TEST] Output should keep the symbolic link" ); + std::ifstream file{ "link_target.vti" }; + const std::string content{ std::istreambuf_iterator< char >{ file }, + std::istreambuf_iterator< char >{} }; + geode::OpenGeodeIOImageException::test( + absl::StrContains( content, "VTKFile" ), + "[TEST] Output should be written in the symbolic link target" ); +#endif +} + int main() { try @@ -170,6 +238,8 @@ int main() test_png_input(); test_tiff_input(); test_empty_image_output(); + test_failed_output_keeps_existing_file(); + test_output_through_symbolic_link(); geode::Logger::info( "TEST SUCCESS" ); return 0; } From e481bbc30f29b07fd7bb434d50dbe6b7de28d841 Mon Sep 17 00:00:00 2001 From: Arnaud Botella Date: Wed, 21 Oct 2026 09:11:09 +0200 Subject: [PATCH 02/12] perf(VTIRasterImageOutput): write colors from the image storage Colors were copied pixel by pixel through RasterImage::color() into a temporary buffer. The VTK writer can now reference external data, so the contiguous color storage is written as is, without any copy. Co-Authored-By: Claude Opus 5.5 --- include/geode/io/image/detail/vtk_output.hpp | 25 +++++++++--- .../io/image/vti_raster_image_output.cpp | 21 +++++----- tests/image/test-raster-image.cpp | 39 +++++++++++++++++++ 3 files changed, 67 insertions(+), 18 deletions(-) diff --git a/include/geode/io/image/detail/vtk_output.hpp b/include/geode/io/image/detail/vtk_output.hpp index b0d1cbef..b862985e 100644 --- a/include/geode/io/image/detail/vtk_output.hpp +++ b/include/geode/io/image/detail/vtk_output.hpp @@ -361,6 +361,22 @@ namespace geode std::string_view name, std::vector< T >&& values, local_index_t nb_components = 1 ) + { + auto owner = + std::make_shared< std::vector< T > >( std::move( values ) ); + auto data_array = write_external_data_array< T >( + parent, name, *owner, nb_components ); + appended_blocks_.back().owner = std::move( owner ); + return data_array; + } + + // Same as above, the values are neither copied nor owned: they + // must stay alive until the file is written + template < typename T > + pugi::xml_node write_external_data_array( pugi::xml_node& parent, + std::string_view name, + absl::Span< const T > values, + local_index_t nb_components = 1 ) { auto data_array = parent.append_child( "DataArray" ); data_array.append_attribute( "type" ).set_value( @@ -370,16 +386,13 @@ namespace geode data_array.append_attribute( "format" ).set_value( "appended" ); data_array.append_attribute( "NumberOfComponents" ) .set_value( nb_components ); - write_range( data_array, absl::Span< const T >{ values } ); + write_range( data_array, values ); data_array.append_attribute( "offset" ) .set_value( appended_size_ ); - auto owner = - std::make_shared< std::vector< T > >( std::move( values ) ); AppendedBlock block; - block.nb_bytes = owner->size() * sizeof( T ); + block.nb_bytes = values.size() * sizeof( T ); // NOLINTNEXTLINE(cppcoreguidelines-pro-type-reinterpret-cast) - block.data = reinterpret_cast< const char* >( owner->data() ); - block.owner = std::move( owner ); + block.data = reinterpret_cast< const char* >( values.data() ); appended_size_ += sizeof( HeaderType ) + block.nb_bytes; appended_blocks_.push_back( std::move( block ) ); return data_array; diff --git a/src/geode/io/image/vti_raster_image_output.cpp b/src/geode/io/image/vti_raster_image_output.cpp index 0a56a5f4..201af875 100644 --- a/src/geode/io/image/vti_raster_image_output.cpp +++ b/src/geode/io/image/vti_raster_image_output.cpp @@ -62,18 +62,15 @@ namespace { auto point_data = piece.append_child( "PointData" ); point_data.append_attribute( "Scalars" ).set_value( "Color" ); - std::vector< uint8_t > values; - values.reserve( - static_cast< size_t >( this->mesh().nb_cells() ) * 3 ); - for( const auto c : geode::Range{ this->mesh().nb_cells() } ) - { - const auto& color = this->mesh().color( c ); - values.push_back( color.red() ); - values.push_back( color.green() ); - values.push_back( color.blue() ); - } - this->template write_data_array< uint8_t >( - point_data, "Color", std::move( values ), 3 ); + static_assert( sizeof( geode::RGBColor ) == 3, + "[VTIRasterImageOutput] Colors should be stored as 3 bytes" ); + const auto colors = this->mesh().colors(); + // Colors are written from the image storage, without copy + // NOLINTNEXTLINE(cppcoreguidelines-pro-type-reinterpret-cast) + const auto* values = + reinterpret_cast< const uint8_t* >( colors.data() ); + this->template write_external_data_array< uint8_t >( + point_data, "Color", { values, 3 * colors.size() }, 3 ); } }; } // namespace diff --git a/tests/image/test-raster-image.cpp b/tests/image/test-raster-image.cpp index 32741c3e..338ca52c 100644 --- a/tests/image/test-raster-image.cpp +++ b/tests/image/test-raster-image.cpp @@ -23,6 +23,7 @@ #include +#include #include #include #include @@ -160,6 +161,43 @@ void test_empty_image_output() "[TEST] Wrong extent written for empty image" ); } +void test_vti_colors_output() +{ + geode::RasterImage2D raster{ { 3, 2 } }; + for( const auto cell : geode::Range{ raster.nb_cells() } ) + { + const auto value = static_cast< geode::local_index_t >( 10 * cell ); + raster.set_color( + cell, { value, static_cast< geode::local_index_t >( value + 1 ), + static_cast< geode::local_index_t >( value + 2 ) } ); + } + geode::save_raster_image( raster, "colors.vti" ); + std::ifstream file{ "colors.vti", std::ios::binary }; + const std::string content{ std::istreambuf_iterator< char >{ file }, + std::istreambuf_iterator< char >{} }; + const auto start = content.find( "" ); + geode::OpenGeodeIOImageException::test( + start != std::string::npos, "[TEST] No appended data in VTI file" ); + const auto data = content.find( '_', start ) + 1; + std::uint64_t nb_bytes{ 0 }; + std::memcpy( &nb_bytes, content.data() + data, sizeof( nb_bytes ) ); + geode::OpenGeodeIOImageException::test( + nb_bytes == 3 * raster.nb_cells(), "[TEST] Wrong VTI colors size" ); + for( const auto cell : geode::Range{ raster.nb_cells() } ) + { + const auto* color = + content.data() + data + sizeof( nb_bytes ) + 3 * cell; + const auto& expected = raster.color( cell ); + geode::OpenGeodeIOImageException::test( + static_cast< geode::local_index_t >( color[0] ) == expected.red() + && static_cast< geode::local_index_t >( color[1] ) + == expected.green() + && static_cast< geode::local_index_t >( color[2] ) + == expected.blue(), + "[TEST] Wrong VTI color for cell ", cell ); + } +} + class FailingVTIOutput : public geode::detail::VTIOutputImpl< geode::RasterImage2D > { @@ -240,6 +278,7 @@ int main() test_empty_image_output(); test_failed_output_keeps_existing_file(); test_output_through_symbolic_link(); + test_vti_colors_output(); geode::Logger::info( "TEST SUCCESS" ); return 0; } From b801423f631690d6fe7ce466f363b5b88d3287b2 Mon Sep 17 00:00:00 2001 From: Arnaud Botella Date: Wed, 21 Oct 2026 09:11:40 +0200 Subject: [PATCH 03/12] fix(BMPInput): load BMP images with the same row order as other formats GDAL returns the top row first for every format, but unlike the PNG, JPG and TIFF inputs the BMP input did not reverse the rows: BMP images were loaded upside down. Co-Authored-By: Claude Opus 5.5 --- src/geode/io/image/bmp_input.cpp | 2 +- tests/data/image_rgb.bmp | Bin 0 -> 90 bytes tests/data/image_rgb.png | Bin 0 -> 81 bytes tests/image/test-raster-image.cpp | 53 ++++++++++++++++++++++++++++++ 4 files changed, 54 insertions(+), 1 deletion(-) create mode 100644 tests/data/image_rgb.bmp create mode 100644 tests/data/image_rgb.png diff --git a/src/geode/io/image/bmp_input.cpp b/src/geode/io/image/bmp_input.cpp index 6a710814..e7f0490f 100644 --- a/src/geode/io/image/bmp_input.cpp +++ b/src/geode/io/image/bmp_input.cpp @@ -34,7 +34,7 @@ namespace geode RasterImage2D BMPInput::read() { ImageInputImpl reader{ filename() }; - return reader.read_file(); + return reader.read_reversed_y_axis_file(); } auto BMPInput::additional_files() const -> AdditionalFiles diff --git a/tests/data/image_rgb.bmp b/tests/data/image_rgb.bmp new file mode 100644 index 0000000000000000000000000000000000000000..29c35273bc954d93038660051872b57719098f1e GIT binary patch literal 90 zcmY+#u?>Jg3`0>P7$F4%P #include #include +#include #include #include @@ -149,6 +150,57 @@ void test_tiff_input() geode::save_raster_image( raster, "cea.vti" ); } +// Test images are 4x3 pixels, image row 0 being the top row +constexpr geode::index_t TEST_IMAGE_WIDTH = 4; +constexpr geode::index_t TEST_IMAGE_HEIGHT = 3; + +std::array< geode::index_t, 2 > test_image_pixel( geode::index_t cell ) +{ + return { cell % TEST_IMAGE_WIDTH, + TEST_IMAGE_HEIGHT - 1 - cell / TEST_IMAGE_WIDTH }; +} + +geode::RasterImage2D load_test_image( std::string_view filename ) +{ + auto raster = geode::load_raster_image< 2 >( + absl::StrCat( geode::DATA_PATH, filename ) ); + geode::OpenGeodeIOImageException::test( + raster.nb_cells_in_direction( 0 ) == TEST_IMAGE_WIDTH + && raster.nb_cells_in_direction( 1 ) == TEST_IMAGE_HEIGHT, + "[TEST] Wrong size for ", filename ); + return raster; +} + +// Colors of the bands of image_rgb files +geode::RGBColor test_image_rgb( geode::index_t cell ) +{ + const auto pixel = test_image_pixel( cell ); + return { static_cast< geode::local_index_t >( 20 * pixel[0] ), + static_cast< geode::local_index_t >( 40 * pixel[1] ), 130 }; +} + +void check_test_image_colors( std::string_view filename, + const std::function< geode::RGBColor( geode::index_t ) >& expected ) +{ + const auto raster = load_test_image( filename ); + for( const auto cell : geode::Range{ raster.nb_cells() } ) + { + const auto& color = raster.color( cell ); + const auto expected_color = expected( cell ); + geode::OpenGeodeIOImageException::test( color == expected_color, + "[TEST] Wrong color for cell ", cell, " of ", filename, ": (", + color.red(), ", ", color.green(), ", ", color.blue(), + ") instead of (", expected_color.red(), ", ", + expected_color.green(), ", ", expected_color.blue(), ")" ); + } +} + +void test_bmp_input() +{ + check_test_image_colors( "image_rgb.png", test_image_rgb ); + check_test_image_colors( "image_rgb.bmp", test_image_rgb ); +} + void test_empty_image_output() { const geode::RasterImage2D raster; @@ -275,6 +327,7 @@ int main() test_jpg_from_paraview_input(); test_png_input(); test_tiff_input(); + test_bmp_input(); test_empty_image_output(); test_failed_output_keeps_existing_file(); test_output_through_symbolic_link(); From 12ccc26bcef8da2ab6dbacdf7d6c7e086705a25f Mon Sep 17 00:00:00 2001 From: Arnaud Botella Date: Wed, 21 Oct 2026 09:12:44 +0200 Subject: [PATCH 04/12] perf(ImageInput): decode pixels directly into the raster image colors Each band was read in its own buffer, then every pixel was copied with one RasterImage::set_color call. GDAL now writes the pixels interleaved straight into the color storage, rows reversed through a negative line spacing. Loading a 64 Mpx image goes from 1.0s to 0.47s (PNG) and from 0.70s to 0.26s (TIFF), at half the peak memory. Co-Authored-By: Claude Opus 5.5 --- .../io/image/internal/raster_image_input.hpp | 2 - src/geode/io/image/bmp_input.cpp | 2 +- src/geode/io/image/jpg_input.cpp | 2 +- src/geode/io/image/png_input.cpp | 2 +- src/geode/io/image/raster_image_input.cpp | 146 ++++++++---------- src/geode/io/image/tiff_input.cpp | 2 +- 6 files changed, 71 insertions(+), 85 deletions(-) diff --git a/include/geode/io/image/internal/raster_image_input.hpp b/include/geode/io/image/internal/raster_image_input.hpp index c24ea3df..3c64d6d9 100644 --- a/include/geode/io/image/internal/raster_image_input.hpp +++ b/include/geode/io/image/internal/raster_image_input.hpp @@ -44,8 +44,6 @@ namespace geode RasterImage2D read_file(); - RasterImage2D read_reversed_y_axis_file(); - Percentage is_loadable(); }; } // namespace internal diff --git a/src/geode/io/image/bmp_input.cpp b/src/geode/io/image/bmp_input.cpp index e7f0490f..6a710814 100644 --- a/src/geode/io/image/bmp_input.cpp +++ b/src/geode/io/image/bmp_input.cpp @@ -34,7 +34,7 @@ namespace geode RasterImage2D BMPInput::read() { ImageInputImpl reader{ filename() }; - return reader.read_reversed_y_axis_file(); + return reader.read_file(); } auto BMPInput::additional_files() const -> AdditionalFiles diff --git a/src/geode/io/image/jpg_input.cpp b/src/geode/io/image/jpg_input.cpp index b7627ba0..388a4b12 100644 --- a/src/geode/io/image/jpg_input.cpp +++ b/src/geode/io/image/jpg_input.cpp @@ -34,7 +34,7 @@ namespace geode RasterImage2D JPGInput::read() { ImageInputImpl reader{ filename() }; - return reader.read_reversed_y_axis_file(); + return reader.read_file(); } auto JPGInput::additional_files() const -> AdditionalFiles diff --git a/src/geode/io/image/png_input.cpp b/src/geode/io/image/png_input.cpp index 3be2872d..1b39989f 100644 --- a/src/geode/io/image/png_input.cpp +++ b/src/geode/io/image/png_input.cpp @@ -34,7 +34,7 @@ namespace geode RasterImage2D PNGInput::read() { ImageInputImpl reader{ filename() }; - return reader.read_reversed_y_axis_file(); + return reader.read_file(); } auto PNGInput::additional_files() const -> AdditionalFiles diff --git a/src/geode/io/image/raster_image_input.cpp b/src/geode/io/image/raster_image_input.cpp index 1d0ecbf5..3874fde2 100644 --- a/src/geode/io/image/raster_image_input.cpp +++ b/src/geode/io/image/raster_image_input.cpp @@ -25,8 +25,8 @@ #include -#include -#include +#include + #include #include @@ -34,31 +34,58 @@ namespace { - absl::FixedArray< GByte > read_color_component( - const geode::RasterImage2D& raster, - GDALDataset& gdal_data, - int component ) + static_assert( sizeof( geode::RGBColor ) == 3, + "[ImageInputImpl] Colors should be stored as 3 bytes" ); + + // Image files store their top row first whereas raster images start from + // the bottom row: GDAL writes the rows backward from the last one + GByte* last_row_start( absl::Span< geode::RGBColor > colors, int width ) + { + // NOLINTNEXTLINE(cppcoreguidelines-pro-type-reinterpret-cast) + return reinterpret_cast< GByte* >( colors.data() + colors.size() ) + - static_cast< std::ptrdiff_t >( 3 ) * width; + } + + void check_read( CPLErr status ) { - const auto width = raster.nb_cells_in_direction( 0 ); - const auto height = raster.nb_cells_in_direction( 1 ); - absl::FixedArray< GByte > values( raster.nb_cells() ); - const auto status = - gdal_data.GetRasterBand( component ) - ->RasterIO( GF_Read, 0, 0, width, height, values.data(), width, - height, GDT_Byte, 0, 0 ); geode::OpenGeodeIOImageException::check_exception( status == CE_None, nullptr, geode::OpenGeodeException::TYPE::data, - "[ImageInputImpl] Failed to read color component" ); - return values; + "[ImageInputImpl] Failed to read image pixels" ); + } + + void read_grey_band( GDALRasterBand& band, + absl::Span< geode::RGBColor > colors, + int width, + int height ) + { + check_read( band.RasterIO( GF_Read, 0, 0, width, height, + last_row_start( colors, width ), width, height, GDT_Byte, 3, + -static_cast< GSpacing >( 3 ) * width ) ); + for( auto& color : colors ) + { + color.set_green( color.red() ); + color.set_blue( color.red() ); + } + } + + void read_rgb_bands( GDALDataset& dataset, + std::array< int, 3 > bands, + absl::Span< geode::RGBColor > colors, + int width, + int height ) + { + check_read( dataset.RasterIO( GF_Read, 0, 0, width, height, + last_row_start( colors, width ), width, height, GDT_Byte, 3, + bands.data(), 3, -static_cast< GSpacing >( 3 ) * width, 1, + nullptr ) ); } - std::array< geode::index_t, 3 > get_rgb_indices( GDALDataset& gdal_data ) + + std::array< int, 3 > get_rgb_indices( GDALDataset& gdal_data ) { - std::array< geode::index_t, 3 > rgb_indices{ 0, 0, 0 }; - const auto nb_color_components = - static_cast< geode::index_t >( gdal_data.GetRasterCount() ); - for( const auto id : geode::Range( nb_color_components ) ) + std::array< int, 3 > rgb_indices{ 0, 0, 0 }; + const auto nb_color_components = gdal_data.GetRasterCount(); + for( int band_id = 1; band_id <= nb_color_components; band_id++ ) { - const auto band_id = id + 1; GDALColorInterp colorInterp = gdal_data.GetRasterBand( band_id )->GetColorInterpretation(); if( colorInterp == GCI_RedBand ) @@ -76,57 +103,6 @@ namespace } return rgb_indices; } - - template < typename SpecificRange > - geode::RasterImage2D read_file( GDALDataset& gdal_data ) - { - const auto width = gdal_data.GetRasterXSize(); - const auto height = gdal_data.GetRasterYSize(); - geode::RasterImage2D raster{ { static_cast< geode::index_t >( width ), - static_cast< geode::index_t >( height ) } }; - const auto nb_color_components = - static_cast< geode::index_t >( gdal_data.GetRasterCount() ); - if( nb_color_components <= 2 ) - { - const auto grey_scale = - read_color_component( raster, gdal_data, 1 ); - geode::index_t cell{ 0 }; - for( const auto image_j : - SpecificRange{ raster.nb_cells_in_direction( 1 ) } ) - { - for( const auto image_i : - geode::Range{ raster.nb_cells_in_direction( 0 ) } ) - { - const auto pixel = image_i + width * image_j; - const auto value = grey_scale[pixel]; - raster.set_color( cell++, { value, value, value } ); - } - } - } - else if( nb_color_components <= 4 ) - { - const auto rgb_indices = get_rgb_indices( gdal_data ); - const auto red = - read_color_component( raster, gdal_data, rgb_indices[0] ); - const auto green = - read_color_component( raster, gdal_data, rgb_indices[1] ); - const auto blue = - read_color_component( raster, gdal_data, rgb_indices[2] ); - geode::index_t cell{ 0 }; - for( const auto image_j : - SpecificRange{ raster.nb_cells_in_direction( 1 ) } ) - { - for( const auto image_i : - geode::Range{ raster.nb_cells_in_direction( 0 ) } ) - { - const auto pixel = image_i + width * image_j; - raster.set_color( - cell++, { red[pixel], green[pixel], blue[pixel] } ); - } - } - } - return raster; - } } // namespace namespace geode @@ -140,12 +116,24 @@ namespace geode RasterImage2D ImageInputImpl::read_file() { - return ::read_file< Range >( dataset() ); - } - - RasterImage2D ImageInputImpl::read_reversed_y_axis_file() - { - return ::read_file< ReverseRange >( dataset() ); + auto& gdal_data = dataset(); + const auto width = gdal_data.GetRasterXSize(); + const auto height = gdal_data.GetRasterYSize(); + RasterImage2D raster{ { static_cast< index_t >( width ), + static_cast< index_t >( height ) } }; + const auto colors = raster.modifiable_colors(); + const auto nb_color_components = gdal_data.GetRasterCount(); + if( nb_color_components <= 2 ) + { + read_grey_band( + *gdal_data.GetRasterBand( 1 ), colors, width, height ); + } + else if( nb_color_components <= 4 ) + { + read_rgb_bands( gdal_data, get_rgb_indices( gdal_data ), colors, + width, height ); + } + return raster; } Percentage ImageInputImpl::is_loadable() @@ -157,4 +145,4 @@ namespace geode return Percentage{ 1 }; } } // namespace internal -} // namespace geode \ No newline at end of file +} // namespace geode diff --git a/src/geode/io/image/tiff_input.cpp b/src/geode/io/image/tiff_input.cpp index 3ed84fff..e1cae915 100644 --- a/src/geode/io/image/tiff_input.cpp +++ b/src/geode/io/image/tiff_input.cpp @@ -34,7 +34,7 @@ namespace geode RasterImage2D TIFFInput::read() { ImageInputImpl reader{ filename() }; - return reader.read_reversed_y_axis_file(); + return reader.read_file(); } auto TIFFInput::additional_files() const -> AdditionalFiles From a7881ea253c8f10f949b20de0c7b0ab9df797fe6 Mon Sep 17 00:00:00 2001 From: Arnaud Botella Date: Wed, 21 Oct 2026 09:13:25 +0200 Subject: [PATCH 05/12] fix(ImageInput): load images whose bands are not tagged as RGB Images with 3 or 4 bands without red/green/blue color interpretation (e.g. TIFF with MINISBLACK photometric) used band 0, which does not exist: the load crashed. The first three bands are now used, with a warning. Co-Authored-By: Claude Opus 5.5 --- src/geode/io/image/raster_image_input.cpp | 9 +++++++++ tests/data/image_untagged_bands.tif | Bin 0 -> 224 bytes tests/image/test-raster-image.cpp | 6 ++++++ 3 files changed, 15 insertions(+) create mode 100644 tests/data/image_untagged_bands.tif diff --git a/src/geode/io/image/raster_image_input.cpp b/src/geode/io/image/raster_image_input.cpp index 3874fde2..ab45dd8c 100644 --- a/src/geode/io/image/raster_image_input.cpp +++ b/src/geode/io/image/raster_image_input.cpp @@ -25,8 +25,10 @@ #include +#include #include +#include #include #include @@ -101,6 +103,13 @@ namespace rgb_indices[2] = band_id; } } + if( absl::c_linear_search( rgb_indices, 0 ) ) + { + geode::Logger::warning( "[ImageInputImpl] Red, green and blue " + "bands are not identified: the first " + "three bands are used" ); + return { 1, 2, 3 }; + } return rgb_indices; } } // namespace diff --git a/tests/data/image_untagged_bands.tif b/tests/data/image_untagged_bands.tif new file mode 100644 index 0000000000000000000000000000000000000000..1d7af4ca95df873bf922b74b3bb577395e4345e2 GIT binary patch literal 224 zcmebD)MDUZU|`^3U|?isU<9&QfS3`=W(KmEfNT&w4~Ute;vjWwNNhny7OK$*+D=yhz5aRsG4mMF3@El$ODFZfMO?r_zDm+f+2&5h7BA8 NMKypR00^3p005r25a9p- literal 0 HcmV?d00001 diff --git a/tests/image/test-raster-image.cpp b/tests/image/test-raster-image.cpp index 3a9b90e2..fa8bf0ef 100644 --- a/tests/image/test-raster-image.cpp +++ b/tests/image/test-raster-image.cpp @@ -201,6 +201,11 @@ void test_bmp_input() check_test_image_colors( "image_rgb.bmp", test_image_rgb ); } +void test_untagged_bands_input() +{ + check_test_image_colors( "image_untagged_bands.tif", test_image_rgb ); +} + void test_empty_image_output() { const geode::RasterImage2D raster; @@ -328,6 +333,7 @@ int main() test_png_input(); test_tiff_input(); test_bmp_input(); + test_untagged_bands_input(); test_empty_image_output(); test_failed_output_keeps_existing_file(); test_output_through_symbolic_link(); From 05966a0d14ffb4563dac7f163a750236b7b49465 Mon Sep 17 00:00:00 2001 From: Arnaud Botella Date: Wed, 21 Oct 2026 09:13:50 +0200 Subject: [PATCH 06/12] fix(ImageInput): load images with more than four bands Images with 5 bands or more (e.g. multispectral TIFF) were silently loaded all black. Their red, green and blue bands, or their first three bands, are now used. Co-Authored-By: Claude Opus 5.5 --- src/geode/io/image/raster_image_input.cpp | 2 +- tests/data/image_five_bands.tif | Bin 0 -> 276 bytes tests/image/test-raster-image.cpp | 6 ++++++ 3 files changed, 7 insertions(+), 1 deletion(-) create mode 100644 tests/data/image_five_bands.tif diff --git a/src/geode/io/image/raster_image_input.cpp b/src/geode/io/image/raster_image_input.cpp index ab45dd8c..99045026 100644 --- a/src/geode/io/image/raster_image_input.cpp +++ b/src/geode/io/image/raster_image_input.cpp @@ -137,7 +137,7 @@ namespace geode read_grey_band( *gdal_data.GetRasterBand( 1 ), colors, width, height ); } - else if( nb_color_components <= 4 ) + else { read_rgb_bands( gdal_data, get_rgb_indices( gdal_data ), colors, width, height ); diff --git a/tests/data/image_five_bands.tif b/tests/data/image_five_bands.tif new file mode 100644 index 0000000000000000000000000000000000000000..35d1fa355fc4c5e4b0695ad76f8b93f2b11950a4 GIT binary patch literal 276 zcmebD)MDUZU|`^3U|?isU<9&QfS3`=W(KmEfNWMEJr9VPq2eHQY)EWDMi#J|O+ax` zs5nTE7?ce%M;yooiLC%)8K^iDkR1eMgX}y8#KBNC=YVVw;DAFOpbH`J22lJ75PtyT kKOi+A$3Oui&<7C6AfjOd$3R&PAP4}0CM3{<1g0SY0H}u`@&Et; literal 0 HcmV?d00001 diff --git a/tests/image/test-raster-image.cpp b/tests/image/test-raster-image.cpp index fa8bf0ef..83a65a37 100644 --- a/tests/image/test-raster-image.cpp +++ b/tests/image/test-raster-image.cpp @@ -206,6 +206,11 @@ void test_untagged_bands_input() check_test_image_colors( "image_untagged_bands.tif", test_image_rgb ); } +void test_five_bands_input() +{ + check_test_image_colors( "image_five_bands.tif", test_image_rgb ); +} + void test_empty_image_output() { const geode::RasterImage2D raster; @@ -334,6 +339,7 @@ int main() test_tiff_input(); test_bmp_input(); test_untagged_bands_input(); + test_five_bands_input(); test_empty_image_output(); test_failed_output_keeps_existing_file(); test_output_through_symbolic_link(); From bbffd7def3d9d92f4b082f7080633d78c265f928 Mon Sep 17 00:00:00 2001 From: Arnaud Botella Date: Wed, 21 Oct 2026 09:14:31 +0200 Subject: [PATCH 07/12] fix(ImageInput): apply the color table of palette images Palette images (8-bit PNG, BMP, TIFF) were loaded as grey levels of their palette indices. The RGB or grey palette colors are now used. Co-Authored-By: Claude Opus 5.5 --- src/geode/io/image/raster_image_input.cpp | 75 +++++++++++++++++++++- tests/data/image_palette.png | Bin 0 -> 128 bytes tests/image/test-raster-image.cpp | 13 ++++ 3 files changed, 85 insertions(+), 3 deletions(-) create mode 100644 tests/data/image_palette.png diff --git a/src/geode/io/image/raster_image_input.cpp b/src/geode/io/image/raster_image_input.cpp index 99045026..c8811e29 100644 --- a/src/geode/io/image/raster_image_input.cpp +++ b/src/geode/io/image/raster_image_input.cpp @@ -23,6 +23,9 @@ #include +#include +#include + #include #include @@ -31,6 +34,7 @@ #include #include +#include #include #include @@ -70,6 +74,66 @@ namespace } } + // Colors of the palette entries when the band is a palette index + std::optional< std::vector< geode::RGBColor > > palette_colors( + GDALRasterBand& band ) + { + if( band.GetColorInterpretation() != GCI_PaletteIndex ) + { + return std::nullopt; + } + const auto* table = band.GetColorTable(); + if( table == nullptr ) + { + return std::nullopt; + } + const auto interpretation = table->GetPaletteInterpretation(); + if( interpretation != GPI_RGB && interpretation != GPI_Gray ) + { + geode::Logger::warning( "[ImageInputImpl] Palette is neither RGB " + "nor grey: palette indices are read as " + "grey levels" ); + return std::nullopt; + } + std::vector< geode::RGBColor > colors( + static_cast< size_t >( table->GetColorEntryCount() ) ); + for( auto entry_id = 0; entry_id < table->GetColorEntryCount(); + entry_id++ ) + { + const auto& entry = *table->GetColorEntry( entry_id ); + const auto red = geode::detail::color_channel( entry.c1 ); + if( interpretation == GPI_Gray ) + { + colors[entry_id] = { red, red, red }; + continue; + } + colors[entry_id] = { red, geode::detail::color_channel( entry.c2 ), + geode::detail::color_channel( entry.c3 ) }; + } + return colors; + } + + void read_palette_band( GDALRasterBand& band, + absl::Span< const geode::RGBColor > palette, + absl::Span< geode::RGBColor > colors, + int width, + int height ) + { + std::vector< GUInt16 > indices( colors.size() ); + constexpr auto INDEX_SIZE = + static_cast< GSpacing >( sizeof( GUInt16 ) ); + check_read( band.RasterIO( GF_Read, 0, 0, width, height, + indices.data() + indices.size() - width, width, height, GDT_UInt16, + INDEX_SIZE, -INDEX_SIZE * width ) ); + for( const auto pixel : geode::Indices{ indices } ) + { + const auto index = indices[pixel]; + // Indices without palette entry are black + colors[pixel] = + index < palette.size() ? palette[index] : geode::RGBColor{}; + } + } + void read_rgb_bands( GDALDataset& dataset, std::array< int, 3 > bands, absl::Span< geode::RGBColor > colors, @@ -132,10 +196,15 @@ namespace geode static_cast< index_t >( height ) } }; const auto colors = raster.modifiable_colors(); const auto nb_color_components = gdal_data.GetRasterCount(); - if( nb_color_components <= 2 ) + auto& first_band = *gdal_data.GetRasterBand( 1 ); + if( const auto palette = palette_colors( first_band ) ) + { + read_palette_band( + first_band, palette.value(), colors, width, height ); + } + else if( nb_color_components <= 2 ) { - read_grey_band( - *gdal_data.GetRasterBand( 1 ), colors, width, height ); + read_grey_band( first_band, colors, width, height ); } else { diff --git a/tests/data/image_palette.png b/tests/data/image_palette.png new file mode 100644 index 0000000000000000000000000000000000000000..5cf82b3de1dd58f80d5c0b33e3a284ddb8f6e79a GIT binary patch literal 128 zcmeAS@N?(olHy`uVBq!ia0vp^EI`c6!3-puwc@@2DU|@95Lbr(3?i>tHLi2n9Oe&L zFOo7_vZ7tCr&xJGyv7!9-4o_Uca+WlpDc+6su%ZkaSY*@nVgW2l9ZN^n3|ma;K7qe aYz&P449<_)gWdv_FnGH9xvX( + 20 * index ), + static_cast< geode::local_index_t >( 255 - 20 * index ), + static_cast< geode::local_index_t >( 5 * index ) }; + } ); +} + void test_empty_image_output() { const geode::RasterImage2D raster; @@ -340,6 +352,7 @@ int main() test_bmp_input(); test_untagged_bands_input(); test_five_bands_input(); + test_palette_input(); test_empty_image_output(); test_failed_output_keeps_existing_file(); test_output_through_symbolic_link(); From ca5a8c897aa691ff74a5dc78a900d400aa16bb2f Mon Sep 17 00:00:00 2001 From: Arnaud Botella Date: Wed, 21 Oct 2026 09:15:25 +0200 Subject: [PATCH 08/12] fix(ImageInput): rescale 16-bit and floating point images to colors Pixels were converted to 8-bit by GDAL with clamping: 16-bit images were loaded all white and floating point images all black. Values are now linearly rescaled from the range of the image values, nodata and NaN pixels being black. Co-Authored-By: Claude Opus 5.5 --- src/geode/io/image/raster_image_input.cpp | 124 ++++++++++++++++++++-- tests/data/image_float.tif | Bin 0 -> 212 bytes tests/data/image_grey16.png | Bin 0 -> 92 bytes tests/data/image_rgb16.tif | Bin 0 -> 248 bytes tests/image/test-raster-image.cpp | 37 +++++++ 5 files changed, 155 insertions(+), 6 deletions(-) create mode 100644 tests/data/image_float.tif create mode 100644 tests/data/image_grey16.png create mode 100644 tests/data/image_rgb16.tif diff --git a/src/geode/io/image/raster_image_input.cpp b/src/geode/io/image/raster_image_input.cpp index c8811e29..5ce7662d 100644 --- a/src/geode/io/image/raster_image_input.cpp +++ b/src/geode/io/image/raster_image_input.cpp @@ -23,6 +23,8 @@ #include +#include +#include #include #include @@ -146,6 +148,106 @@ namespace nullptr ) ); } + bool are_byte_bands( GDALDataset& dataset, absl::Span< const int > bands ) + { + return absl::c_all_of( bands, [&dataset]( int band ) { + return dataset.GetRasterBand( band )->GetRasterDataType() + == GDT_Byte; + } ); + } + + struct ValueRange + { + double min{ std::numeric_limits< double >::max() }; + double max{ std::numeric_limits< double >::lowest() }; + }; + + // Range of the band values, nodata values excluded + ValueRange bands_range( + GDALDataset& dataset, absl::Span< const int > bands ) + { + ValueRange range; + for( const auto band : bands ) + { + std::array< double, 2 > min_max; + if( dataset.GetRasterBand( band )->ComputeRasterMinMax( + false, min_max.data() ) + == CE_None ) + { + range.min = std::min( range.min, min_max[0] ); + range.max = std::max( range.max, min_max[1] ); + } + } + return range; + } + + void set_channel( + geode::RGBColor& color, size_t channel, geode::local_index_t value ) + { + if( channel == 0 ) + { + color.set_red( value ); + } + else if( channel == 1 ) + { + color.set_green( value ); + } + else + { + color.set_blue( value ); + } + } + + // Values which are not 8-bit are linearly rescaled from the range of the + // values of all the bands to the color range, nodata values are black + void read_rescaled_bands( GDALDataset& dataset, + std::array< int, 3 > bands, + absl::Span< geode::RGBColor > colors, + int width, + int height ) + { + const auto range = bands_range( dataset, bands ); + const auto scale = + range.max > range.min + ? std::numeric_limits< geode::local_index_t >::max() + / ( range.max - range.min ) + : 0.; + std::vector< double > values( colors.size() ); + constexpr auto VALUE_SIZE = static_cast< GSpacing >( sizeof( double ) ); + for( const auto channel : geode::Indices{ bands } ) + { + if( absl::c_find( bands, bands[channel] ) + != bands.begin() + channel ) + { + // Band already read for a previous channel + continue; + } + auto& band = *dataset.GetRasterBand( bands[channel] ); + check_read( band.RasterIO( GF_Read, 0, 0, width, height, + values.data() + values.size() - width, width, height, + GDT_Float64, VALUE_SIZE, -VALUE_SIZE * width ) ); + int has_nodata{ 0 }; + const auto nodata = band.GetNoDataValue( &has_nodata ); + for( const auto pixel : geode::Indices{ values } ) + { + const auto value = values[pixel]; + const auto is_nodata = + std::isnan( value ) || ( has_nodata && value == nodata ); + const auto channel_value = + is_nodata ? geode::local_index_t{ 0 } + : geode::detail::color_channel( + ( value - range.min ) * scale ); + for( const auto other : geode::Indices{ bands } ) + { + if( bands[other] == bands[channel] ) + { + set_channel( colors[pixel], other, channel_value ); + } + } + } + } + } + std::array< int, 3 > get_rgb_indices( GDALDataset& gdal_data ) { std::array< int, 3 > rgb_indices{ 0, 0, 0 }; @@ -202,14 +304,24 @@ namespace geode read_palette_band( first_band, palette.value(), colors, width, height ); } - else if( nb_color_components <= 2 ) - { - read_grey_band( first_band, colors, width, height ); - } else { - read_rgb_bands( gdal_data, get_rgb_indices( gdal_data ), colors, - width, height ); + const auto bands = nb_color_components <= 2 + ? std::array< int, 3 >{ 1, 1, 1 } + : get_rgb_indices( gdal_data ); + if( !are_byte_bands( gdal_data, bands ) ) + { + read_rescaled_bands( + gdal_data, bands, colors, width, height ); + } + else if( nb_color_components <= 2 ) + { + read_grey_band( first_band, colors, width, height ); + } + else + { + read_rgb_bands( gdal_data, bands, colors, width, height ); + } } return raster; } diff --git a/tests/data/image_float.tif b/tests/data/image_float.tif new file mode 100644 index 0000000000000000000000000000000000000000..d1a0bb2861afa25c9a75073bd867d044aaab9479 GIT binary patch literal 212 zcmebD)MDUZU|`^3U|?isU<9&QfS3`=W(KmEplk&on;FUmsbfQ83o^2R^)3O5iz11O zLD?X4#Gz^ofNU8gHNi+~8kaCJumR=g0Wk>ZS^@!t$-rRGz|a812Y{Htfx$-R7?54h Tz>weo6bIr7K)e8mH#h(Q|CkO_ literal 0 HcmV?d00001 diff --git a/tests/data/image_grey16.png b/tests/data/image_grey16.png new file mode 100644 index 0000000000000000000000000000000000000000..69ff37a5e5ce8633f2ce5f3c4db86605ed5a98a6 GIT binary patch literal 92 zcmeAS@N?(olHy`uVBq!ia0vp^EI`aG03;X=^6N$dDP>O=#}JO0$q97_A06J*7~0af pwdpJ0Z06fSY-N1kWWI6Bs4)ahW_=^~baewro2RRv%Q~loCIH^08UFwP literal 0 HcmV?d00001 diff --git a/tests/data/image_rgb16.tif b/tests/data/image_rgb16.tif new file mode 100644 index 0000000000000000000000000000000000000000..9a0de9b2cd306c79fee65b1580cba346ef5e4a9b GIT binary patch literal 248 zcmebD)MDUZU|`^8U|?isU<9&QfS3`=W(KmEfNT&w35c1Y;vjWwP&N~gEy&0MRx=OC z7Ddu4hQt;JvO(s|0Ad*=y}?lNRX}kN5CB671`yl;q!PUd{fh&Z22e%9l2siLN;ql;I!z;j7!gqtu KhJOJ+ivR$o&l(~C literal 0 HcmV?d00001 diff --git a/tests/image/test-raster-image.cpp b/tests/image/test-raster-image.cpp index 506ca572..bf126847 100644 --- a/tests/image/test-raster-image.cpp +++ b/tests/image/test-raster-image.cpp @@ -23,6 +23,7 @@ #include +#include #include #include #include @@ -223,6 +224,41 @@ void test_palette_input() } ); } +geode::local_index_t rescaled( double value, double min, double max ) +{ + return static_cast< geode::local_index_t >( + std::round( ( value - min ) * 255. / ( max - min ) ) ); +} + +void test_non_8_bit_input() +{ + const auto index = []( geode::index_t cell ) { + const auto pixel = test_image_pixel( cell ); + return pixel[0] + TEST_IMAGE_WIDTH * pixel[1]; + }; + check_test_image_colors( "image_grey16.png", [&index]( + geode::index_t cell ) { + const auto grey = rescaled( 1000. + 100. * index( cell ), 1000, 2100 ); + return geode::RGBColor{ grey, grey, grey }; + } ); + check_test_image_colors( + "image_rgb16.tif", [&index]( geode::index_t cell ) { + const auto value = 100. * index( cell ); + return geode::RGBColor{ rescaled( 1000. + value, 1000, 4100 ), + rescaled( 2000. + value, 1000, 4100 ), + rescaled( 3000. + value, 1000, 4100 ) }; + } ); + // Pixel 5 has the nodata value and pixel 6 is NaN + check_test_image_colors( + "image_float.tif", [&index]( geode::index_t cell ) { + const auto pixel = index( cell ); + const auto grey = pixel == 5 || pixel == 6 + ? geode::local_index_t{ 0 } + : rescaled( 0.5 * pixel, 0, 5.5 ); + return geode::RGBColor{ grey, grey, grey }; + } ); +} + void test_empty_image_output() { const geode::RasterImage2D raster; @@ -353,6 +389,7 @@ int main() test_untagged_bands_input(); test_five_bands_input(); test_palette_input(); + test_non_8_bit_input(); test_empty_image_output(); test_failed_output_keeps_existing_file(); test_output_through_symbolic_link(); From 514bef4b075a37d62ea7010fd1d5c120523e4f19 Mon Sep 17 00:00:00 2001 From: Arnaud Botella Date: Wed, 21 Oct 2026 09:16:01 +0200 Subject: [PATCH 09/12] fix(GDALFile): list only existing associated files as additional files The main file was returned as an additional file and every file was flagged as missing, although GDAL only lists existing files: any image was reported with missing additional files. Co-Authored-By: Claude Opus 5.5 --- include/geode/io/image/detail/gdal_file.hpp | 3 ++- src/geode/io/image/gdal_file.cpp | 13 +++++++++++++ tests/data/image_world_file.pgw | 6 ++++++ tests/data/image_world_file.png | Bin 0 -> 81 bytes tests/image/test-raster-image.cpp | 20 ++++++++++++++++++++ 5 files changed, 41 insertions(+), 1 deletion(-) create mode 100644 tests/data/image_world_file.pgw create mode 100644 tests/data/image_world_file.png diff --git a/include/geode/io/image/detail/gdal_file.hpp b/include/geode/io/image/detail/gdal_file.hpp index 185e7251..eff598c6 100644 --- a/include/geode/io/image/detail/gdal_file.hpp +++ b/include/geode/io/image/detail/gdal_file.hpp @@ -49,6 +49,7 @@ namespace geode bool is_coordinate_system_loadable(); + // Existing files used with the main file (e.g. world files) template < typename AdditionalFiles > AdditionalFiles additional_files() { @@ -56,7 +57,7 @@ namespace geode for( auto& file : associated_files() ) { files.optional_files.emplace_back( - std::move( file ), true ); + std::move( file ), false ); } return files; } diff --git a/src/geode/io/image/gdal_file.cpp b/src/geode/io/image/gdal_file.cpp index 6013ceb8..425dcd02 100644 --- a/src/geode/io/image/gdal_file.cpp +++ b/src/geode/io/image/gdal_file.cpp @@ -23,6 +23,8 @@ #include +#include + #include #include @@ -85,9 +87,20 @@ namespace geode { return {}; } + // The list starts with the main file, which is not associated + const std::filesystem::path main_file{ + dataset_->GetDescription() + }; std::vector< std::string > files; for( int i = 0; papszFileList[i] != nullptr; i++ ) { + std::error_code error; + if( papszFileList[i] == main_file + || std::filesystem::equivalent( + papszFileList[i], main_file, error ) ) + { + continue; + } files.emplace_back( papszFileList[i] ); } CSLDestroy( papszFileList ); diff --git a/tests/data/image_world_file.pgw b/tests/data/image_world_file.pgw new file mode 100644 index 00000000..956580e1 --- /dev/null +++ b/tests/data/image_world_file.pgw @@ -0,0 +1,6 @@ +1.0 +0.0 +0.0 +-1.0 +0.5 +2.5 diff --git a/tests/data/image_world_file.png b/tests/data/image_world_file.png new file mode 100644 index 0000000000000000000000000000000000000000..b7f599029f41d5b7d11c577178b13c44a674a333 GIT binary patch literal 81 zcmeAS@N?(olHy`uVBq!ia0vp^EI`c6!2~3&r&&$}QWBmnjv*W~lT#8hgc1`P6vWtg e*(@9$GB7A;F-jb^EIt8L!{F)a=d#Wzp$Pyr%@BJ4 literal 0 HcmV?d00001 diff --git a/tests/image/test-raster-image.cpp b/tests/image/test-raster-image.cpp index bf126847..9a90cc45 100644 --- a/tests/image/test-raster-image.cpp +++ b/tests/image/test-raster-image.cpp @@ -259,6 +259,25 @@ void test_non_8_bit_input() } ); } +void test_additional_files() +{ + const auto no_files = geode::raster_image_additional_files< 2 >( + absl::StrCat( geode::DATA_PATH, "image_rgb.png" ) ); + geode::OpenGeodeIOImageException::test( + no_files.optional_files.empty() && !no_files.has_additional_files(), + "[TEST] Image without associated file should have no additional " + "file" ); + const auto files = geode::raster_image_additional_files< 2 >( + absl::StrCat( geode::DATA_PATH, "image_world_file.png" ) ); + geode::OpenGeodeIOImageException::test( + files.optional_files.size() == 1 && !files.has_additional_files(), + "[TEST] Wrong additional files of image with world file" ); + geode::OpenGeodeIOImageException::test( + absl::EndsWith( + files.optional_files.front().filename, "image_world_file.pgw" ), + "[TEST] World file should be an additional file" ); +} + void test_empty_image_output() { const geode::RasterImage2D raster; @@ -390,6 +409,7 @@ int main() test_five_bands_input(); test_palette_input(); test_non_8_bit_input(); + test_additional_files(); test_empty_image_output(); test_failed_output_keeps_existing_file(); test_output_through_symbolic_link(); From c7176b4ca73f9715f46bcfb50275be8bcd652ff5 Mon Sep 17 00:00:00 2001 From: Arnaud Botella Date: Wed, 21 Oct 2026 09:16:38 +0200 Subject: [PATCH 10/12] fix(ImageInput): open image files only with image format drivers Image files were opened by any GDAL driver: a VRT file named .png was loaded and could make GDAL read any other local file or URL. Image inputs now only accept the PNG, JPEG, BMP and GTiff drivers, so a misnamed image of these formats still loads. GDALFile accepts a list of allowed drivers. Co-Authored-By: Claude Opus 5.5 --- include/geode/io/image/detail/gdal_file.hpp | 9 ++++++- src/geode/io/image/gdal_file.cpp | 29 ++++++++++++++++++--- src/geode/io/image/raster_image_input.cpp | 7 ++++- tests/data/image_vrt.png | 8 ++++++ tests/image/test-raster-image.cpp | 18 +++++++++++++ 5 files changed, 65 insertions(+), 6 deletions(-) create mode 100644 tests/data/image_vrt.png diff --git a/include/geode/io/image/detail/gdal_file.hpp b/include/geode/io/image/detail/gdal_file.hpp index eff598c6..3c028637 100644 --- a/include/geode/io/image/detail/gdal_file.hpp +++ b/include/geode/io/image/detail/gdal_file.hpp @@ -23,6 +23,8 @@ #pragma once +#include + #include #include @@ -42,7 +44,12 @@ namespace geode class opengeode_io_image_api GDALFile { public: - GDALFile( std::string_view filename ); + /*! + * @param[in] allowed_drivers Names of the GDAL drivers which can + * open the file, all the drivers if empty + */ + explicit GDALFile( std::string_view filename, + absl::Span< const char* const > allowed_drivers = {} ); ~GDALFile(); CoordinateSystem2D read_coordinate_system(); diff --git a/src/geode/io/image/gdal_file.cpp b/src/geode/io/image/gdal_file.cpp index 425dcd02..28371f60 100644 --- a/src/geode/io/image/gdal_file.cpp +++ b/src/geode/io/image/gdal_file.cpp @@ -38,9 +38,9 @@ namespace geode class GDALFile::Impl { public: - Impl( std::string_view filename ) - : dataset_{ GDALDataset::Open( - geode::to_string( filename ).c_str(), GDAL_OF_READONLY ) } + Impl( std::string_view filename, + absl::Span< const char* const > allowed_drivers ) + : dataset_{ open( filename, allowed_drivers ) } { OpenGeodeIOImageException::check_exception( dataset_ != nullptr, nullptr, OpenGeodeException::TYPE::data, @@ -107,11 +107,32 @@ namespace geode return files; } + private: + static GDALDataset* open( std::string_view filename, + absl::Span< const char* const > allowed_drivers ) + { + if( allowed_drivers.empty() ) + { + return GDALDataset::Open( + geode::to_string( filename ).c_str(), + GDAL_OF_READONLY ); + } + std::vector< const char* > drivers{ allowed_drivers.begin(), + allowed_drivers.end() }; + drivers.push_back( nullptr ); + return GDALDataset::Open( geode::to_string( filename ).c_str(), + GDAL_OF_READONLY, drivers.data() ); + } + private: GDALDatasetUniquePtr dataset_; }; - GDALFile::GDALFile( std::string_view filename ) : impl_{ filename } {} + GDALFile::GDALFile( std::string_view filename, + absl::Span< const char* const > allowed_drivers ) + : impl_{ filename, allowed_drivers } + { + } GDALFile::~GDALFile() = default; diff --git a/src/geode/io/image/raster_image_input.cpp b/src/geode/io/image/raster_image_input.cpp index 5ce7662d..fedcc5a2 100644 --- a/src/geode/io/image/raster_image_input.cpp +++ b/src/geode/io/image/raster_image_input.cpp @@ -42,6 +42,11 @@ namespace { + // Image formats only: other GDAL formats (e.g. VRT) can reference any + // other file or URL + constexpr std::array< const char*, 4 > IMAGE_DRIVERS{ "PNG", "JPEG", "BMP", + "GTiff" }; + static_assert( sizeof( geode::RGBColor ) == 3, "[ImageInputImpl] Colors should be stored as 3 bytes" ); @@ -285,7 +290,7 @@ namespace geode namespace internal { ImageInputImpl::ImageInputImpl( std::string_view filename ) - : detail::GDALFile{ filename } + : detail::GDALFile{ filename, IMAGE_DRIVERS } { } diff --git a/tests/data/image_vrt.png b/tests/data/image_vrt.png new file mode 100644 index 00000000..c754c381 --- /dev/null +++ b/tests/data/image_vrt.png @@ -0,0 +1,8 @@ + + + + image_rgb.png + 1 + + + diff --git a/tests/image/test-raster-image.cpp b/tests/image/test-raster-image.cpp index 9a90cc45..8048374b 100644 --- a/tests/image/test-raster-image.cpp +++ b/tests/image/test-raster-image.cpp @@ -278,6 +278,23 @@ void test_additional_files() "[TEST] World file should be an additional file" ); } +void test_non_image_format_input() +{ + // A VRT file, referencing image_rgb.png, named as a PNG file + bool thrown{ false }; + try + { + const auto raster = geode::load_raster_image< 2 >( + absl::StrCat( geode::DATA_PATH, "image_vrt.png" ) ); + } + catch( const geode::OpenGeodeException& ) + { + thrown = true; + } + geode::OpenGeodeIOImageException::test( + thrown, "[TEST] Non image formats should not be loaded" ); +} + void test_empty_image_output() { const geode::RasterImage2D raster; @@ -410,6 +427,7 @@ int main() test_palette_input(); test_non_8_bit_input(); test_additional_files(); + test_non_image_format_input(); test_empty_image_output(); test_failed_output_keeps_existing_file(); test_output_through_symbolic_link(); From 2e71b97419440a88dac4cb5168529348e31cdd08 Mon Sep 17 00:00:00 2001 From: Arnaud Botella Date: Wed, 21 Oct 2026 09:17:57 +0200 Subject: [PATCH 11/12] fix(ImageInput): report GDAL errors and unreadable files as not loadable is_loadable() threw on files GDAL cannot open instead of returning 0. GDAL errors were printed on stderr and missing from the exceptions, which only said the file failed to open. GDAL warnings are now logged and GDAL error messages are part of the exceptions. Co-Authored-By: Claude Opus 5.5 --- include/geode/io/image/detail/gdal_file.hpp | 17 ++++++++ .../io/image/internal/raster_image_input.hpp | 3 +- src/geode/io/image/bmp_input.cpp | 3 +- src/geode/io/image/gdal_file.cpp | 39 +++++++++++++++++-- src/geode/io/image/jpg_input.cpp | 3 +- src/geode/io/image/png_input.cpp | 3 +- src/geode/io/image/raster_image_input.cpp | 22 +++++++++-- src/geode/io/image/tiff_input.cpp | 3 +- tests/data/image_garbage.png | 1 + tests/image/test-raster-image.cpp | 21 ++++++++++ 10 files changed, 98 insertions(+), 17 deletions(-) create mode 100644 tests/data/image_garbage.png diff --git a/include/geode/io/image/detail/gdal_file.hpp b/include/geode/io/image/detail/gdal_file.hpp index 3c028637..1b1217e9 100644 --- a/include/geode/io/image/detail/gdal_file.hpp +++ b/include/geode/io/image/detail/gdal_file.hpp @@ -41,6 +41,23 @@ namespace geode { namespace detail { + /*! + * While alive, GDAL warnings are logged and GDAL errors are not + * printed: they should be reported with last_error() + */ + class opengeode_io_image_api GDALErrorScope + { + public: + GDALErrorScope(); + ~GDALErrorScope(); + GDALErrorScope( const GDALErrorScope& ) = delete; + GDALErrorScope& operator=( const GDALErrorScope& ) = delete; + GDALErrorScope( GDALErrorScope&& ) = delete; + GDALErrorScope& operator=( GDALErrorScope&& ) = delete; + + std::string last_error() const; + }; + class opengeode_io_image_api GDALFile { public: diff --git a/include/geode/io/image/internal/raster_image_input.hpp b/include/geode/io/image/internal/raster_image_input.hpp index 3c64d6d9..ee7ea8b1 100644 --- a/include/geode/io/image/internal/raster_image_input.hpp +++ b/include/geode/io/image/internal/raster_image_input.hpp @@ -44,7 +44,8 @@ namespace geode RasterImage2D read_file(); - Percentage is_loadable(); + // Unreadable files are not loadable + static Percentage is_loadable( std::string_view filename ); }; } // namespace internal } // namespace geode \ No newline at end of file diff --git a/src/geode/io/image/bmp_input.cpp b/src/geode/io/image/bmp_input.cpp index 6a710814..51f10801 100644 --- a/src/geode/io/image/bmp_input.cpp +++ b/src/geode/io/image/bmp_input.cpp @@ -45,8 +45,7 @@ namespace geode Percentage BMPInput::is_loadable() const { - ImageInputImpl reader{ filename() }; - return reader.is_loadable(); + return ImageInputImpl::is_loadable( filename() ); } } // namespace internal } // namespace geode diff --git a/src/geode/io/image/gdal_file.cpp b/src/geode/io/image/gdal_file.cpp index 28371f60..7fdc1b6b 100644 --- a/src/geode/io/image/gdal_file.cpp +++ b/src/geode/io/image/gdal_file.cpp @@ -27,24 +27,55 @@ #include +#include #include #include +namespace +{ + void CPL_STDCALL log_gdal_warnings( + CPLErr type, CPLErrorNum /*unused*/, const char* message ) + { + if( type == CE_Warning ) + { + geode::Logger::warning( "[GDAL] ", message ); + } + } +} // namespace + namespace geode { namespace detail { + GDALErrorScope::GDALErrorScope() + { + CPLPushErrorHandler( log_gdal_warnings ); + CPLErrorReset(); + } + + GDALErrorScope::~GDALErrorScope() + { + CPLPopErrorHandler(); + } + + std::string GDALErrorScope::last_error() const + { + return CPLGetLastErrorMsg(); + } + class GDALFile::Impl { public: Impl( std::string_view filename, absl::Span< const char* const > allowed_drivers ) - : dataset_{ open( filename, allowed_drivers ) } { + const GDALErrorScope errors; + dataset_.reset( open( filename, allowed_drivers ) ); OpenGeodeIOImageException::check_exception( dataset_ != nullptr, nullptr, OpenGeodeException::TYPE::data, - "[GDALFile] Failed to open file ", filename ); + "[GDALFile] Failed to open file ", filename, ": ", + errors.last_error() ); } GDALDataset& dataset() @@ -115,13 +146,13 @@ namespace geode { return GDALDataset::Open( geode::to_string( filename ).c_str(), - GDAL_OF_READONLY ); + GDAL_OF_READONLY | GDAL_OF_VERBOSE_ERROR ); } std::vector< const char* > drivers{ allowed_drivers.begin(), allowed_drivers.end() }; drivers.push_back( nullptr ); return GDALDataset::Open( geode::to_string( filename ).c_str(), - GDAL_OF_READONLY, drivers.data() ); + GDAL_OF_READONLY | GDAL_OF_VERBOSE_ERROR, drivers.data() ); } private: diff --git a/src/geode/io/image/jpg_input.cpp b/src/geode/io/image/jpg_input.cpp index 388a4b12..1d8ce74b 100644 --- a/src/geode/io/image/jpg_input.cpp +++ b/src/geode/io/image/jpg_input.cpp @@ -45,8 +45,7 @@ namespace geode Percentage JPGInput::is_loadable() const { - ImageInputImpl reader{ filename() }; - return reader.is_loadable(); + return ImageInputImpl::is_loadable( filename() ); } } // namespace internal } // namespace geode diff --git a/src/geode/io/image/png_input.cpp b/src/geode/io/image/png_input.cpp index 1b39989f..1f271023 100644 --- a/src/geode/io/image/png_input.cpp +++ b/src/geode/io/image/png_input.cpp @@ -45,8 +45,7 @@ namespace geode Percentage PNGInput::is_loadable() const { - ImageInputImpl reader{ filename() }; - return reader.is_loadable(); + return ImageInputImpl::is_loadable( filename() ); } } // namespace internal } // namespace geode diff --git a/src/geode/io/image/raster_image_input.cpp b/src/geode/io/image/raster_image_input.cpp index fedcc5a2..03bac312 100644 --- a/src/geode/io/image/raster_image_input.cpp +++ b/src/geode/io/image/raster_image_input.cpp @@ -63,7 +63,8 @@ namespace { geode::OpenGeodeIOImageException::check_exception( status == CE_None, nullptr, geode::OpenGeodeException::TYPE::data, - "[ImageInputImpl] Failed to read image pixels" ); + "[ImageInputImpl] Failed to read image pixels: ", + CPLGetLastErrorMsg() ); } void read_grey_band( GDALRasterBand& band, @@ -296,7 +297,12 @@ namespace geode RasterImage2D ImageInputImpl::read_file() { + const detail::GDALErrorScope errors; auto& gdal_data = dataset(); + OpenGeodeIOImageException::check_exception( + gdal_data.GetRasterCount() > 0, nullptr, + OpenGeodeException::TYPE::data, + "[ImageInputImpl] No raster band in image file" ); const auto width = gdal_data.GetRasterXSize(); const auto height = gdal_data.GetRasterYSize(); RasterImage2D raster{ { static_cast< index_t >( width ), @@ -331,13 +337,21 @@ namespace geode return raster; } - Percentage ImageInputImpl::is_loadable() + Percentage ImageInputImpl::is_loadable( std::string_view filename ) { - if( dataset().GetRasterCount() == 0 ) + try + { + ImageInputImpl reader{ filename }; + if( reader.dataset().GetRasterCount() == 0 ) + { + return Percentage{ 0 }; + } + return Percentage{ 1 }; + } + catch( const OpenGeodeException& ) { return Percentage{ 0 }; } - return Percentage{ 1 }; } } // namespace internal } // namespace geode diff --git a/src/geode/io/image/tiff_input.cpp b/src/geode/io/image/tiff_input.cpp index e1cae915..09c44416 100644 --- a/src/geode/io/image/tiff_input.cpp +++ b/src/geode/io/image/tiff_input.cpp @@ -45,8 +45,7 @@ namespace geode Percentage TIFFInput::is_loadable() const { - ImageInputImpl reader{ filename() }; - return reader.is_loadable(); + return ImageInputImpl::is_loadable( filename() ); } } // namespace internal } // namespace geode diff --git a/tests/data/image_garbage.png b/tests/data/image_garbage.png new file mode 100644 index 00000000..c93adf7e --- /dev/null +++ b/tests/data/image_garbage.png @@ -0,0 +1 @@ +this is not an image diff --git a/tests/image/test-raster-image.cpp b/tests/image/test-raster-image.cpp index 8048374b..8b317087 100644 --- a/tests/image/test-raster-image.cpp +++ b/tests/image/test-raster-image.cpp @@ -295,6 +295,26 @@ void test_non_image_format_input() thrown, "[TEST] Non image formats should not be loaded" ); } +void test_unreadable_input() +{ + const auto filename = absl::StrCat( geode::DATA_PATH, "image_garbage.png" ); + geode::OpenGeodeIOImageException::test( + geode::is_raster_image_loadable< 2 >( filename ).value() == 0, + "[TEST] Unreadable image should not be loadable" ); + std::string message; + try + { + const auto raster = geode::load_raster_image< 2 >( filename ); + } + catch( const geode::OpenGeodeException& exception ) + { + message = exception.what(); + } + geode::OpenGeodeIOImageException::test( + absl::StrContains( message, "not recognized" ), + "[TEST] Load error should give the GDAL reason, got: ", message ); +} + void test_empty_image_output() { const geode::RasterImage2D raster; @@ -428,6 +448,7 @@ int main() test_non_8_bit_input(); test_additional_files(); test_non_image_format_input(); + test_unreadable_input(); test_empty_image_output(); test_failed_output_keeps_existing_file(); test_output_through_symbolic_link(); From 6e3f5903a2da297beaf79462fd7fcba37a542342 Mon Sep 17 00:00:00 2001 From: Arnaud Botella Date: Wed, 21 Oct 2026 10:13:19 +0200 Subject: [PATCH 12/12] fix(ImageTests): find pugixml when tests are built against an install The image test uses the VTK writer header, which needs pugixml. When the tests are configured on their own against an installed OpenGeode-IO, the pugixml target was not defined. Co-Authored-By: Claude Opus 5.5 --- tests/image/CMakeLists.txt | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/tests/image/CMakeLists.txt b/tests/image/CMakeLists.txt index 14ee4926..5a80bfd6 100755 --- a/tests/image/CMakeLists.txt +++ b/tests/image/CMakeLists.txt @@ -18,6 +18,12 @@ # OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE # SOFTWARE. +# The test uses the VTK writer, which needs pugixml, also when the tests are +# built against an installed OpenGeode-IO +if(NOT TARGET pugixml::pugixml) + find_package(pugixml REQUIRED CONFIG) +endif() + add_geode_test( SOURCE "test-raster-image.cpp" DEPENDENCIES