Modify the memory allocation method of matd_t to comply with ISO standards

[zhaoxi-scut]

The original way of writing matd_t was

typedef struct
{
    unsigned int nrows, ncols;
    double data[];
//    double *data;
} matd_t;

which does not conform to the ISO standard. For example, it will generate a warning in C++ compilers such as g++, with the following message:

/path/to/apriltag/common/matd.h:48:12: warning: ISO C++ forbids flexible array member ‘data’ [-Wpedantic]
   48 |     double data[];
      |            ^~~~

Therefore, it was changed to use double *, and separate memory was allocated for m and m->data. Additionally, resource release for m->data was added in the matd_destroy function.

[christian-rauch]

Can you also add the compiler flags (add_compile_options(-Wall -Wextra -Wpedantic -Werror)) to make sure that the warning does not appear again?

[zhaoxi-scut]

The original line 46 in CMakeLists.txt (-Wpedantic) was commented out, now it is uncommented. 😃

[christian-rauch]

The original line 46 in CMakeLists.txt (-Wpedantic) was commented out, now it is uncommented. 😃

Right. It was actually me who left this commented out (9959575). Probably a reminder that about unsolved issues.

But now the checking seems to be too strict as the CI fails for clang with gnu-zero-variadic-macro-arguments (https://github.com/AprilRobotics/apriltag/actions/runs/11869401035/job/33080645704?pr=362). If you don't know how to fix this, you can add a -Wno-gnu-zero-variadic-macro-arguments to skip that specific issue.

[mkrogius]

The original approach in matd_t was done intentionally with the goal of improving performance. It saves one pointer dereference by keeping data within the matd_t struct instead of having it allocated separately.

I doubt this performance difference matters for the usage of matd within this project. Since most of the compute of apiltag detection is not within matrix calculations I imagine it won't make much difference. But it might be nice to just double check for a single image that this doesn't make a noticeable difference to compute speed

[zhaoxi-scut]

Alright, to pass the CI-CD, I temporarily commented out the -Wpedantic.

The performance improvement brought by using double data[] might not be significant and instead introduces some safety issues. I did a benchmark test on the performance. 😃

----------------------------------------------------------------------------------
Benchmark                                        Time             CPU   Iterations
----------------------------------------------------------------------------------
Ptr (double *data)/iterations:10000000        30.9 ns         30.9 ns     10000000
FAM (double data[])/iterations:10000000       18.1 ns         18.1 ns     10000000

As you can see, the performance improvement brought by using flexible array memory is indeed noticeable, but it is at the ns level, mainly occurring in the calloc calls, and the performance improvement in image detection is almost negligible. Therefore, I also conducted an overall performance test:

static void matd(benchmark::State &state)
{
    cv::Mat gray = cv::imread("1.png", cv::IMREAD_GRAYSCALE);
    auto tf = tag25h9_create();
    auto td = apriltag_detector_create();
    // init
    apriltag_detector_add_family(td, tf);


    for (auto _ : state)
    {
        // format conversion
        image_u8_t apriltag_img = {gray.cols, gray.rows, gray.cols, gray.data};
        // detect
        zarray_t *detections = apriltag_detector_detect(td, &apriltag_img);
        // release
        apriltag_detections_destroy(detections);
    }

    apriltag_detector_destroy(td);
    tag25h9_destroy(tf);
}

BENCHMARK(matd)->Name("detect by (double *data)")->Iterations(100);

And the result of this perf test are as follows:

-----------------------------------------------------------------------------------
Benchmark                                         Time             CPU   Iterations
-----------------------------------------------------------------------------------
detect by (double *data)/iterations:100     1862518 ns      1862318 ns          100
detect by (double data[])/iterations:100    1867628 ns      1867464 ns          100

In fact, I use this project as a 3rdparty library for my own project. During the build, a flexible array memory double data[] warning appeared because I included the apriltag.h header file in a .cpp file, and this header file also nested includes matd.h, where the flexible array declaration is present. This is the reason why the project build does not meet ISO standards.

There are two solutions to this problem.

1. Modify the matd.h header file to ensure that the C++ translation unit including apriltag.h complies with ISO standards during the build (this is the method I'm currently using).
2. Ensure apriltag.h does not include or recursively include matd.h.

[zhaoxi-scut]

Oh, by the way, the environment of this perf test are as follows.

• CPU: i5-12400F
• OS: Ubuntu 22.04
• Compiler: GNU GCC/G++ 11.4.0
• Apriltag version: master (latest)

[mkrogius]

Thank you for running the performance test. I agree that this seems to make very little difference to performance and that this is a good change

[christian-rauch]

Alright, to pass the CI-CD, I temporarily commented out the -Wpedantic.

Did you try running with -Wno-gnu-zero-variadic-macro-arguments, or similar, to skip that specific issue?

[zhaoxi-scut]

Did you try running with -Wno-gnu-zero-variadic-macro-arguments, or similar, to skip that specific issue?

I just tried using the -Wno-gnu-zero-variadic-macro-arguments option with my clang compiler and it worked fine during local testing, so I added a line specifically for the clang compiler in the CMakeLists.txt.

[christian-rauch]

Did you try running with -Wno-gnu-zero-variadic-macro-arguments, or similar, to skip that specific issue?

I just tried using the -Wno-gnu-zero-variadic-macro-arguments option with my clang compiler and it worked fine during local testing, so I added a line specifically for the clang compiler in the CMakeLists.txt.

Thanks. But this is yet again raising another error strict-prototypes as it seems to expect void for C functions that take no parameters. Could you also add a -Wno-strict-prototypes for the time being?

[zhaoxi-scut]

Why does the CICD for MacOS Jazzy fail when running the brew install command? 🥲

[christian-rauch]

Why does the CICD for MacOS Jazzy fail when running the brew install command? 🥲

I don't know why the brew installation now conflicts with the already installed packages. Maybe the GitHub runners or the brew behaviour changed.

brew installs these packages: brew install asio assimp bison bullet cmake console_bridge cppcheck cunit eigen freetype log4cxx opencv openssl pcre poco [email protected] qt spdlog tinyxml tinyxml2 wget lcov but fails with installing pkg-config:

  Error: The `brew link` step did not complete successfully
  The formula built, but is not symlinked into /opt/homebrew
  Could not symlink bin/pkg-config
  Target /opt/homebrew/bin/pkg-config
  is a symlink belonging to [email protected]. You can unlink it:
    brew unlink [email protected]

I guess we just have to wait until this is fixed in the GitHub action that we are using.

[christian-rauch]

Alright. Finally, the CI passes. Thank you for going the extra mile and your patience.