From f07747884a3a88bd8fb82af28497e94c59f9eb37 Mon Sep 17 00:00:00 2001 From: ptahmose Date: Fri, 17 May 2024 14:23:49 +0200 Subject: [PATCH] fix issue with CZIs of certain kind (#3) * fix The most significant changes include the update to the `AppContext::FatalError` function in `appcontext.cpp` where a comment about the exit code of the application was added. In `czi_brick_reader2.cpp`, the `CziBrickReader2::DoBrick` function was updated with a new instance of `BrickOutputInfo` being created. The `czi_helpers.cpp` file saw a large block of code replaced with a for loop that iterates over `vector_mindex_and_rect`. In `dowarp.cpp`, the `DoWarp::WaitUntilDone` and `DoWarp::TryGetHash` functions were updated, with the latter now being a const function and having a comment about handling exceptions more gracefully. Lastly, the `TaskArenaTbb::TaskArenaTbb` function in `taskarena_tbb.cpp` was updated to capture `this` and `task` in the lambda function. List of changes: 1. `AppContext::FatalError` function in `appcontext.cpp` updated with a comment about the exit code of the application. 2. Comment removed from `CziBrickReader::ReadBrick` function in `czi_brick_reader.cpp`. 3. `CziBrickReader2::DoBrick` function in `czi_brick_reader2.cpp` updated with a comment removed and a new instance of `BrickOutputInfo` created. 4. Large block of code in `czi_helpers.cpp` replaced with a for loop that iterates over `vector_mindex_and_rect`. 5. Comment in `document_info.h` updated to clarify the definition of a brick. 6. Comment in `BrickInPlaneIdentifier` struct in `document_info.h` updated to clarify the precedence of the `s_index`. 7. `DoWarp::WaitUntilDone` function in `dowarp.cpp` updated to make `TryGetHash` a const function. 8. `DoWarp::TryGetHash` function in `dowarp.cpp` updated with a comment about handling exceptions more gracefully. 9. `dowarp.h` file updated to make `TryGetHash` a const function. 10. `TaskArenaTbb::TaskArenaTbb` function in `taskarena_tbb.cpp` updated to capture `this` and `task` in the lambda function. 11. `TaskArenaTbb::SuspendCurrentTask` function in `taskarena_tbb.cpp` updated with a comment about the user-managed activity that processes async requests. 12. `TEST(Czi_Helpers, GetSubblocksForBrickNoMindexTest)` function in `czi_helpers_tests.cpp` updated to remove a comment and use `TileIdentifier::GetForNoMIndexAndNoSceneIndex()` instead of `numeric_limits::min()`. * cosmetic * bump version * cosmetic --- CMakeLists.txt | 2 +- documentation/version-history.md | 3 +- libwarpaffine/appcontext.cpp | 2 ++ .../brickreader/czi_brick_reader.cpp | 2 +- .../brickreader/czi_brick_reader2.cpp | 2 +- libwarpaffine/czi_helpers.cpp | 28 +++++++------------ libwarpaffine/document_info.h | 4 +-- libwarpaffine/dowarp.cpp | 14 ++++++---- libwarpaffine/dowarp.h | 4 +-- libwarpaffine/taskarena/taskarena_tbb.cpp | 4 +-- warpaffine_unittests/czi_helpers_tests.cpp | 4 +-- 11 files changed, 34 insertions(+), 35 deletions(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index 7ce4f6f..c0baf89 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -19,7 +19,7 @@ set(CMAKE_CXX_STANDARD_REQUIRED ON) set (CMAKE_MODULE_PATH "${CMAKE_SOURCE_DIR}/modules" ${CMAKE_MODULE_PATH}) project ("warpaffine" - VERSION 0.3.0 + VERSION 0.3.1 DESCRIPTION "experimental Deskew operation") option(WARPAFFINE_BUILD_CLANGTIDY "Build with Clang-Tidy" OFF) diff --git a/documentation/version-history.md b/documentation/version-history.md index d4fd3fe..60bda1a 100644 --- a/documentation/version-history.md +++ b/documentation/version-history.md @@ -3,4 +3,5 @@ version history {#version_history} version | PR | comment ------------------ | ---------------------------------------------------- | --------------------------------------------------- - 0.3.0 | N/A | initial release \ No newline at end of file + 0.3.0 | N/A | initial release + 0.3.1 | [3](https://github.com/ZEISS/warpaffine/pull/3) | bugfix for a crash for "CZIs containing a single brick but have an S-index" \ No newline at end of file diff --git a/libwarpaffine/appcontext.cpp b/libwarpaffine/appcontext.cpp index 19491af..20cb081 100644 --- a/libwarpaffine/appcontext.cpp +++ b/libwarpaffine/appcontext.cpp @@ -83,6 +83,8 @@ void AppContext::FatalError(const std::string& message) this->WriteDebugString(message.c_str()); #endif this->log_->WriteLineStdErr(message); + + // note: the exit-code of the application will be 134 in this case abort(); } diff --git a/libwarpaffine/brickreader/czi_brick_reader.cpp b/libwarpaffine/brickreader/czi_brick_reader.cpp index ad26919..9525e3e 100644 --- a/libwarpaffine/brickreader/czi_brick_reader.cpp +++ b/libwarpaffine/brickreader/czi_brick_reader.cpp @@ -158,7 +158,7 @@ void CziBrickReader::ReadBrick() { BrickCoordinateInfo brick_coordinate_info; brick_coordinate_info.coordinate = coordinate_of_brick; - brick_coordinate_info.mIndex = tile_identifier.m_index.value_or(std::numeric_limits::min());// m_index_of_brick; + brick_coordinate_info.mIndex = tile_identifier.m_index.value_or(std::numeric_limits::min()); brick_coordinate_info.scene_index = tile_identifier.scene_index.value_or(std::numeric_limits::min()); brick_coordinate_info.x_position = rectangle_of_brick.x; brick_coordinate_info.y_position = rectangle_of_brick.y; diff --git a/libwarpaffine/brickreader/czi_brick_reader2.cpp b/libwarpaffine/brickreader/czi_brick_reader2.cpp index ec59e59..832fea0 100644 --- a/libwarpaffine/brickreader/czi_brick_reader2.cpp +++ b/libwarpaffine/brickreader/czi_brick_reader2.cpp @@ -153,7 +153,7 @@ void CziBrickReader2::DoBrick(const libCZI::CDimCoordinate& coordinate, /*int m_ ostringstream ss; ss << "DoBrick: " << Utils::DimCoordinateToString(&coordinate) << " " << tile_identifier.ToInformalString() << " -> size=" << map_z_subblockindex.size() << endl; this->GetContextBase().WriteDebugString(ss.str().c_str()); -*/ +*/ BrickOutputInfo* brick_output_data = new BrickOutputInfo(); brick_output_data->max_count = static_cast(map_z_subblockindex.size()); diff --git a/libwarpaffine/czi_helpers.cpp b/libwarpaffine/czi_helpers.cpp index ecbc323..a342cbe 100644 --- a/libwarpaffine/czi_helpers.cpp +++ b/libwarpaffine/czi_helpers.cpp @@ -43,25 +43,17 @@ using namespace libCZI; document_info.document_origin_y = statistics.boundingBoxLayer0Only.y; auto vector_mindex_and_rect = CziHelpers::GetTileIdentifierRectangles(czi_reader); - if (vector_mindex_and_rect.size() == 1/*&& !Utils::IsValidMindex(vector_mindex_and_rect.cbegin()->mIndex)*/ ) // TODO(JBL) - { - // no m-index present, i.e. we assume a single tile per plane - const auto it = vector_mindex_and_rect.cbegin(); - BrickRectPositionInfo position_info{ it->rectangle.x, it->rectangle.y, static_cast(it->rectangle.w), static_cast(it->rectangle.h) }; - document_info.map_brickid_position.insert(pair(BrickInPlaneIdentifier(), position_info)); - } - else + + // TODO(JBL): + // If we have more than one element, then the tile-identifiers must be unique (and there must not be an invalid m-index). We should + // check for this here. + for (const auto& item : vector_mindex_and_rect) { - // TODO(JBL): in this case, the size must be >1 and there must not be an invalid m-index - which we should check for - for (const auto& item : vector_mindex_and_rect) - { - BrickInPlaneIdentifier brick_identifier; - brick_identifier.m_index = item.tile_identifier.m_index.value_or(std::numeric_limits::min()); - brick_identifier.s_index = item.tile_identifier.scene_index.value_or(std::numeric_limits::min()); - BrickRectPositionInfo position_info{ item.rectangle.x, item.rectangle.y, static_cast(item.rectangle.w), static_cast(item.rectangle.h) }; - // TODO(JBL): must not have a duplicate m-index - document_info.map_brickid_position.insert(pair(brick_identifier, position_info)); - } + BrickInPlaneIdentifier brick_identifier; + brick_identifier.m_index = item.tile_identifier.m_index.value_or(std::numeric_limits::min()); + brick_identifier.s_index = item.tile_identifier.scene_index.value_or(std::numeric_limits::min()); + BrickRectPositionInfo position_info{ item.rectangle.x, item.rectangle.y, static_cast(item.rectangle.w), static_cast(item.rectangle.h) }; + document_info.map_brickid_position.insert(pair(brick_identifier, position_info)); } document_info.map_channelindex_pixeltype = CziHelpers::GetMapOfChannelsToPixeltype(czi_reader); diff --git a/libwarpaffine/document_info.h b/libwarpaffine/document_info.h index bb1c752..ed9753a 100644 --- a/libwarpaffine/document_info.h +++ b/libwarpaffine/document_info.h @@ -12,7 +12,7 @@ #include "inc_libCZI.h" /// This struct is used to uniquely identify a brick (within the document). A brick here is defined -/// as a stack of subblock ("stacked" in Z), and we assume that all subblocks identified with this +/// as a stack of subblocks ("stacked" in Z), and we assume that all subblocks identified with this /// structure are at the same x-y-position (in pixel-coordinate-system) and have same extent. /// Note: In a well-formed CZI, the m-index is unique within a scene. However - what complicates matters /// is that an s-index may or may not be present. @@ -31,7 +31,7 @@ struct BrickInPlaneIdentifier { if (this->IsMIndexValid() && this->IsSIndexValid()) { - // s has highest precedence + // s has the highest precedence return this->s_index < other.s_index || (this->s_index == other.s_index && this->m_index < other.m_index); } else if (this->IsMIndexValid() && !this->IsSIndexValid()) diff --git a/libwarpaffine/dowarp.cpp b/libwarpaffine/dowarp.cpp index 62c0581..9849d5d 100644 --- a/libwarpaffine/dowarp.cpp +++ b/libwarpaffine/dowarp.cpp @@ -86,7 +86,7 @@ DoWarp::OutputBrickInfoRepository::OutputBrickInfoRepository(const AppContext& c destination_brick_info.tiling.push_back(tiling_rect_and_mindex); } - this->map_brickid_destinatiobrickinfo_.insert(pair(item.first, destination_brick_info)); + this->map_brickid_destinationbrickinfo_.insert(pair(item.first, destination_brick_info)); total_number_of_output_subblocks += tiling.size() * destination_brick_info.cuboid.depth; } @@ -95,7 +95,7 @@ DoWarp::OutputBrickInfoRepository::OutputBrickInfoRepository(const AppContext& c IntSize3 DoWarp::OutputBrickInfoRepository::GetOutputExtent(const BrickInPlaneIdentifier& brick_identifier) const { - const auto& item = this->map_brickid_destinatiobrickinfo_.at(brick_identifier); + const auto& item = this->map_brickid_destinationbrickinfo_.at(brick_identifier); IntSize3 extent; extent.width = item.cuboid.width; extent.height = item.cuboid.height; @@ -105,12 +105,12 @@ IntSize3 DoWarp::OutputBrickInfoRepository::GetOutputExtent(const BrickInPlaneId const DoWarp::OutputBrickInfoRepository::DestinationBrickInfo& DoWarp::OutputBrickInfoRepository::GetDestinationInfo(const BrickInPlaneIdentifier& brick_identifier) const { - return this->map_brickid_destinatiobrickinfo_.at(brick_identifier); + return this->map_brickid_destinationbrickinfo_.at(brick_identifier); } IntCuboid DoWarp::OutputBrickInfoRepository::GetOutputVolume(const BrickInPlaneIdentifier& brick_identifier) const { - const auto& item = this->map_brickid_destinatiobrickinfo_.at(brick_identifier); + const auto& item = this->map_brickid_destinationbrickinfo_.at(brick_identifier); return item.cuboid; } @@ -281,7 +281,7 @@ void DoWarp::WaitUntilDone() } } -bool DoWarp::TryGetHash(std::array* hash_code) +bool DoWarp::TryGetHash(std::array* hash_code) const { if (this->calculate_result_hash_) { @@ -301,7 +301,11 @@ void DoWarp::InputBrick(const Brick& brick, const BrickCoordinateInfo& coordinat BrickInPlaneIdentifier brick_in_plane_identifier; brick_in_plane_identifier.m_index = coordinate_info.mIndex; brick_in_plane_identifier.s_index = coordinate_info.scene_index; + + // TODO(JBL): in the unfortunate case where our bookkeeping is not correct (e.g. we have a brick which is not in our map), we currently + // would throw an exception and crash. We should handle this more gracefully, at least by logging an error message. const auto& destination_brick_info = this->output_brick_info_repository_.GetDestinationInfo(brick_in_plane_identifier); + for (size_t n = 0; n < destination_brick_info.tiling.size(); n++) { auto destination_brick = this->CreateBrickAndWaitUntilAvailable( diff --git a/libwarpaffine/dowarp.h b/libwarpaffine/dowarp.h index 9196bc0..448fd59 100644 --- a/libwarpaffine/dowarp.h +++ b/libwarpaffine/dowarp.h @@ -72,7 +72,7 @@ class DoWarp std::vector tiling; }; private: - std::map map_brickid_destinatiobrickinfo_; + std::map map_brickid_destinationbrickinfo_; std::uint32_t number_of_subblocks_to_output_{ 0 }; public: OutputBrickInfoRepository(const AppContext& context, const DeskewDocumentInfo& document_info, const Eigen::Matrix4d& transformation_matrix); @@ -158,7 +158,7 @@ class DoWarp void WaitUntilDone(); - bool TryGetHash(std::array* hash_code); + bool TryGetHash(std::array* hash_code) const; private: void InputBrick(const Brick& brick, const BrickCoordinateInfo& coordinate_info); private: diff --git a/libwarpaffine/taskarena/taskarena_tbb.cpp b/libwarpaffine/taskarena/taskarena_tbb.cpp index b64d60b..b38d748 100644 --- a/libwarpaffine/taskarena/taskarena_tbb.cpp +++ b/libwarpaffine/taskarena/taskarena_tbb.cpp @@ -19,7 +19,7 @@ void TaskArenaTbb::AddTask(TaskType task_type, const std::function& task { ++this->queue_length; this->arena.enqueue( - [=]() ->void + [this, task]() ->void { --this->queue_length; ++this->active_tasks; @@ -45,7 +45,7 @@ void TaskArenaTbb::SuspendCurrentTask(const std::function& tbb::task::suspend( [&](tbb::task::suspend_point tag) { - // Dedicated user-managed activity that processes async requests. + // Dedicated user-managed activity that processes async requests. func_pass_resume_handle(tag); // could be OpenCL/IO/Database/Network etc. }); // execution will be resumed after this function } diff --git a/warpaffine_unittests/czi_helpers_tests.cpp b/warpaffine_unittests/czi_helpers_tests.cpp index c569501..5089f84 100644 --- a/warpaffine_unittests/czi_helpers_tests.cpp +++ b/warpaffine_unittests/czi_helpers_tests.cpp @@ -162,7 +162,7 @@ TEST(Czi_Helpers, GetSubblocksForBrickNoMindexTest) reader->Open(inputStream); auto brick_coordinate = CDimCoordinate::Parse("C0"); - auto map_z_subblocks = CziHelpers::GetSubblocksForBrick(reader.get(), brick_coordinate, TileIdentifier::GetForNoMIndexAndNoSceneIndex() /*numeric_limits::min()*/); + auto map_z_subblocks = CziHelpers::GetSubblocksForBrick(reader.get(), brick_coordinate, TileIdentifier::GetForNoMIndexAndNoSceneIndex()); ASSERT_EQ(map_z_subblocks.size(), 10); for (auto iterator = map_z_subblocks.cbegin(); iterator != map_z_subblocks.cend(); ++iterator) { @@ -179,7 +179,7 @@ TEST(Czi_Helpers, GetSubblocksForBrickNoMindexTest) } brick_coordinate = CDimCoordinate::Parse("C1"); - map_z_subblocks = CziHelpers::GetSubblocksForBrick(reader.get(), brick_coordinate, TileIdentifier::GetForNoMIndexAndNoSceneIndex()/*numeric_limits::min()*/); + map_z_subblocks = CziHelpers::GetSubblocksForBrick(reader.get(), brick_coordinate, TileIdentifier::GetForNoMIndexAndNoSceneIndex()); ASSERT_EQ(map_z_subblocks.size(), 10); for (auto iterator = map_z_subblocks.cbegin(); iterator != map_z_subblocks.cend(); ++iterator) {