boostorg / gil

Boost.GIL - Generic Image Library | Requires C++14 since Boost 1.80
https://boostorg.github.io/gil
Boost Software License 1.0
179 stars 163 forks source link

for_each_pixel return value is incorrect for not 1d traversable view #620

Closed sdebionne closed 2 years ago

sdebionne commented 3 years ago

Actual behavior

I believe the bug was introduced in #139. The behavior is an incorrect return value from for_each_pixel() when the input view is not 1d traversable.

The (removed) re-assignement was here to cope with this use-case.

Expected behavior

In the following loop

        for (std::ptrdiff_t y = 0; y < view.height(); ++y)
            std::for_each(view.row_begin(y), view.row_end(y), fun);

the fun should be carried over from y interation to the other, not using a fresh copy of the input fun.

C++ Minimal Working Example

Here are additional test cases for the for_each_pixel algo, the test_function_object_not_1d_traversable one is currently failing:

#include <boost/gil/algorithm.hpp>
#include <boost/gil/image.hpp>
#include <boost/gil/image_view_factory.hpp>

#include <boost/core/lightweight_test.hpp>

namespace gil = boost::gil;

struct accumulator
{
    void operator()(gil::gray8_pixel_t const& p) {
        sum += gil::at_c<0>(p);
    }

    int sum;
};

void test_function_object_1d_traversable()
{
    gil::gray8_pixel_t const gray128(128);
    gil::gray8_image_t image(2, 2, gray128);

    accumulator acc;
    acc = gil::for_each_pixel(
        gil::const_view(image),
        acc
    );
    BOOST_TEST_EQ(acc.sum, 2 * 2 * 128);
}

void test_function_object_not_1d_traversable()
{
    gil::gray8_pixel_t const gray128(128);
    gil::gray8_image_t image(4, 4, gray128);

    accumulator acc;
    acc = gil::for_each_pixel(
        gil::subimage_view(gil::const_view(image), gil::point_t{1, 1}, gil::point_t{2, 2}),
        acc
    );
    BOOST_TEST_EQ(acc.sum, 2 * 2 * 128);
}

int main()
{
    test_function_object_1d_traversable();
    test_function_object_not_1d_traversable();

    return ::boost::report_errors();
}

See https://godbolt.org/z/a9vdMq43n

sdebionne commented 3 years ago

I believe that the fix is in #187, reopening it.