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<int>::min()`.

* cosmetic

* bump version

* cosmetic

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)

documentation/version-history.md

@@ -3,4 +3,5 @@ version history                 {#version_history}
 
  version            |  PR                                                  | comment
  ------------------ | ---------------------------------------------------- | ---------------------------------------------------
- 0.3.0              |  N/A                                                 | initial release
+ 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"

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

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<int>::min());// m_index_of_brick;
+                brick_coordinate_info.mIndex = tile_identifier.m_index.value_or(std::numeric_limits<int>::min());
                 brick_coordinate_info.scene_index = tile_identifier.scene_index.value_or(std::numeric_limits<int>::min());
                 brick_coordinate_info.x_position = rectangle_of_brick.x;
                 brick_coordinate_info.y_position = rectangle_of_brick.y;

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<int>(map_z_subblockindex.size());

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<uint32_t>(it->rectangle.w), static_cast<uint32_t>(it->rectangle.h) };
-        document_info.map_brickid_position.insert(pair<BrickInPlaneIdentifier, BrickRectPositionInfo>(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<int>::min());
-            brick_identifier.s_index = item.tile_identifier.scene_index.value_or(std::numeric_limits<int>::min());
-            BrickRectPositionInfo position_info{ item.rectangle.x, item.rectangle.y, static_cast<uint32_t>(item.rectangle.w), static_cast<uint32_t>(item.rectangle.h) };
-            // TODO(JBL): must not have a duplicate m-index
-            document_info.map_brickid_position.insert(pair<BrickInPlaneIdentifier, BrickRectPositionInfo>(brick_identifier, position_info));
-        }
+        BrickInPlaneIdentifier brick_identifier;
+        brick_identifier.m_index = item.tile_identifier.m_index.value_or(std::numeric_limits<int>::min());
+        brick_identifier.s_index = item.tile_identifier.scene_index.value_or(std::numeric_limits<int>::min());
+        BrickRectPositionInfo position_info{ item.rectangle.x, item.rectangle.y, static_cast<uint32_t>(item.rectangle.w), static_cast<uint32_t>(item.rectangle.h) };
+        document_info.map_brickid_position.insert(pair<BrickInPlaneIdentifier, BrickRectPositionInfo>(brick_identifier, position_info));
     }
 
     document_info.map_channelindex_pixeltype = CziHelpers::GetMapOfChannelsToPixeltype(czi_reader);

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

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<BrickInPlaneIdentifier, DestinationBrickInfo>(item.first, destination_brick_info));
+        this->map_brickid_destinationbrickinfo_.insert(pair<BrickInPlaneIdentifier, DestinationBrickInfo>(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<uint8_t, 16>* hash_code)
+bool DoWarp::TryGetHash(std::array<uint8_t, 16>* 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(

libwarpaffine/dowarp.h

@@ -72,7 +72,7 @@ class DoWarp
             std::vector<TilingRectAndMandSceneIndex> tiling;
         };
     private:
-        std::map<BrickInPlaneIdentifier, DestinationBrickInfo> map_brickid_destinatiobrickinfo_;
+        std::map<BrickInPlaneIdentifier, DestinationBrickInfo> 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<uint8_t, 16>* hash_code);
+    bool TryGetHash(std::array<uint8_t, 16>* hash_code) const;
 private:
     void InputBrick(const Brick& brick, const BrickCoordinateInfo& coordinate_info);
 private:

libwarpaffine/taskarena/taskarena_tbb.cpp

@@ -19,7 +19,7 @@ void TaskArenaTbb::AddTask(TaskType task_type, const std::function<void()>& 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<void(SuspendHandle)>&
     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
 }

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<int>::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<int>::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)
     {