TPC: fix pair swap when reading CalArray<PadFlags> written by the custom streamer

The custom member streamer for CalArray<PadFlags>::mData writes the count N/2
followed by N single shorts, but reads N/2 ints and reinterprets each as a pair
of flags (the layout written by ROOT < 6.33). Every object written by the
streamer itself (ROOT >= 6.33) therefore comes back with each pair of adjacent
pads swapped. This affects all IDC_PadStatusMap objects written since the
streamer was introduced (e.g. 2026 data, ROOT 6.36.04): the dead channel map
used in reconstruction marks the neighbour of an isolated bad pad instead of
the pad itself. Objects written with ROOT < 6.33 (e.g. 2025, ROOT 6.32.06) are
read correctly.

The class version did not change, so the reader now selects the layout by the
ROOT version that wrote the buffer (TBuffer::GetVersionOwner(); the current ROOT
if the buffer has no file): < 6.33 as before, >= 6.33 N shorts. The writer is
unchanged, so existing objects stay readable and no re-upload is needed.

Add a ROOT I/O round-trip unit test for CalDet<PadFlags>, which fails with the
old reader (349440 of 524160 flags differ) and passes with the new one.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

Author: matthias-kleiner

Detectors/TPC/baserecsim/src/TPCFlagsMemberCustomStreamer.cxx

@@ -12,6 +12,7 @@
 #include "TPCBase/CalArray.h"
 #include <TMemberStreamer.h>
 #include <TBuffer.h>
+#include <TROOT.h>
 #include <DataFormatsTPC/Defs.h>
 #include <iostream>
 
@@ -26,6 +27,14 @@
 // This member was written incorrectly to the TFile in ROOT versions < 6.36, causing
 // segfaults when reading on ARM64 (occassionally).
 // We continue to write it in the incorrect format and fix the reading back.
+//
+// Two layouts exist on disk, both with the count N/2 (N = number of flags):
+// - written by ROOT < 6.33 (no custom streamer): N/2 ints, each holding a pair of
+//   flags as they sit in memory;
+// - written by this streamer (ROOT >= 6.33): N single shorts.
+// Reading the second layout as ints swaps every pair of adjacent flags, so the
+// layout is chosen by the ROOT version that wrote the buffer. The class version
+// did not change, so this is the only marker.
 
 // See also:
 // - https://github.com/root-project/root/pull/17009
@@ -40,23 +49,40 @@ void MemberVectorPadFlagsStreamer(TBuffer& R__b, void* objp, int n)
   std::vector<o2::tpc::PadFlags>* obj = static_cast<std::vector<o2::tpc::PadFlags>*>(objp);
   if (R__b.IsReading()) {
     obj->clear();
-    std::vector<int> R__stl;
-    R__stl.clear();
     int R__n;
     R__b >> R__n;
+    // ROOT version of the file the buffer belongs to (e.g. 63206); 0 without a file (e.g. TMessage):
+    // then writer and reader run the same ROOT
+    int writerVersion = R__b.GetVersionOwner();
+    if (writerVersion <= 0) {
+      writerVersion = gROOT->GetVersionInt();
+    }
+    constexpr size_t bloatfactor = sizeof(int) / sizeof(o2::tpc::PadFlags);
+    if (writerVersion >= 63300) {
+      // written by this streamer: N single shorts
+      obj->reserve(bloatfactor * R__n);
+      for (size_t i = 0; i < bloatfactor * R__n; ++i) {
+        Short_t readtemp;
+        R__b >> readtemp;
+        obj->push_back(static_cast<o2::tpc::PadFlags>(static_cast<unsigned short>(readtemp)));
+      }
+      return;
+    }
+    // written by ROOT < 6.33: N/2 ints, each a pair of flags in memory order
+    std::vector<int> R__stl;
     R__stl.reserve(R__n);
     for (int R__i = 0; R__i < R__n; R__i++) {
       Int_t readtemp;
       R__b >> readtemp;
       R__stl.push_back(readtemp);
     }
     auto data = reinterpret_cast<unsigned short*>(R__stl.data());
-    constexpr size_t bloatfactor = sizeof(int) / sizeof(o2::tpc::PadFlags);
-    for (int i = 0; i < bloatfactor * R__n; ++i) {
+    for (size_t i = 0; i < bloatfactor * R__n; ++i) {
       obj->push_back(static_cast<o2::tpc::PadFlags>(data[i]));
     }
   } else {
-    // We always save things with the old format.
+    // count N/2, then N single shorts: NOT the old (ROOT < 6.33) layout of N/2 ints; the reader above
+    // tells the two apart by the ROOT version. Unchanged, so that existing objects stay readable.
     R__b << (int)obj->size() / 2;
     for (size_t i = 0; i < obj->size(); i++) {
       R__b << (short)obj->at(i);

Detectors/TPC/baserecsim/test/testTPCCalDet.cxx

@@ -346,6 +346,44 @@ BOOST_AUTO_TEST_CASE(CalDetTypeTest)
   BOOST_CHECK(testDict == true);
 }
 
+BOOST_AUTO_TEST_CASE(CalDetPadFlags_ROOTIO)
+{
+  // CalArray<PadFlags>::mData has a custom member streamer (TPCFlagsMemberCustomStreamer.cxx);
+  // a write + read round trip must give back every flag at its own pad (a pattern in which
+  // adjacent pads always differ, so that a swap of neighbouring pads shows up)
+  CalDet<PadFlags> flags("flags", PadSubset::Region);
+  const PadFlags bad = PadFlags::flagSkip | PadFlags::flagLowPad;
+  size_t n = 0;
+  for (auto& arr : flags.getData()) {
+    for (size_t i = 0; i < arr.getData().size(); ++i, ++n) {
+      arr.setValue(i, (n % 3 == 0) ? bad : PadFlags::flagGoodPad);
+    }
+  }
+
+  auto f = TFile::Open("CalDetPadFlags_ROOTIO.root", "recreate");
+  f->WriteObject(&flags, "flags");
+  f->Close();
+  delete f;
+
+  f = TFile::Open("CalDetPadFlags_ROOTIO.root");
+  auto* read = f->Get<CalDet<PadFlags>>("flags");
+  BOOST_REQUIRE(read != nullptr);
+  BOOST_REQUIRE_EQUAL(read->getData().size(), flags.getData().size());
+  size_t nDiffer = 0;
+  for (size_t iarr = 0; iarr < flags.getData().size(); ++iarr) {
+    const auto& written = flags.getCalArray(iarr).getData();
+    const auto& back = read->getCalArray(iarr).getData();
+    BOOST_REQUIRE_EQUAL(back.size(), written.size());
+    for (size_t i = 0; i < written.size(); ++i) {
+      nDiffer += back[i] != written[i];
+    }
+  }
+  BOOST_CHECK_EQUAL(nDiffer, 0);
+  delete read;
+  f->Close();
+  delete f;
+}
+
 BOOST_AUTO_TEST_CASE(CalDetStreamerTest)
 {
   // simple code executing the TPC IDCPadFlags loading in a standalone env --> easy to valgrind