Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: is_equal_to_sixteen in PNG I/O was less-than test #650

Merged

Conversation

striezel
Copy link
Contributor

@striezel striezel commented Apr 25, 2022

Description

While is_equal_to_sixteen should (as the name suggests) check for equality to 16, it actually checked whether the bit depth was less than 16.

Since Boost.MP11 does not contain an mp11::mp_equal but only mp11::mp_less, the equality check is performed by making sure that

  • the bit depth is not less than 16,
  • 16 is not less than the bit depth,
  • and both of those hold true.

It's a bit laborious, but it works.

References

The wrong comparison was introduced by PR #274 in 5611bd5#diff-e2c958c778b464b94146992b6aeb7c814839ddbcbbb901f9f4175e797fa543d2 when boost::mpl was replaced with boost::mp11. In that commit the original mpl::equal_to was replaced by mp11::mp_less. To be fair though, unlike MPL the MP11 library does not contain an equivalent to mpl::equal_to.

Tasklist

  • Ensure all CI builds pass
  • Review and approve

While is_equal_to_sixteen should (as the name suggests) check for
equality to 16, it actually checked whether the bit depth was
less than 16.
@codecov
Copy link

codecov bot commented Apr 25, 2022

Codecov Report

Merging #650 (6a77ba5) into develop (caf92fa) will decrease coverage by 0.00%.
The diff coverage is n/a.

@@             Coverage Diff             @@
##           develop     #650      +/-   ##
===========================================
- Coverage    78.77%   78.76%   -0.01%     
===========================================
  Files          117      117              
  Lines         5031     5030       -1     
===========================================
- Hits          3963     3962       -1     
  Misses        1068     1068              

@mloskot
Copy link
Member

mloskot commented Apr 25, 2022

Good catch, @striezel !

It looks like I did a copy & paste mistake copying struct is_less_than_eight : mp11::mp_less as template of is_equal_to_sixteen.


Hmm, since this bug was introduced by #274 in Boost 1.72 which is

between Boost 1.71 and Boost 1.74

as @striezel points out in #633 (comment), so any chance it is related to the issue #633 ?

@mloskot mloskot changed the title fix is_equal_to_sixteen in PNG I/O extension fix: is_equal_to_sixteen in PNG I/O was less-than test Apr 25, 2022
@mloskot mloskot added the cat/bug But reports and bug fixes label Apr 25, 2022
@mloskot mloskot added this to the Boost 1.78+ milestone Apr 25, 2022
@striezel
Copy link
Contributor Author

Hmm, since this bug was introduced by #274 in Boost 1.72 which is

between Boost 1.71 and Boost 1.74

as @striezel points out in #633 (comment), so any chance it is related to the issue #633 ?

Well, that is what I am hoping for. Fixing is_equal_to_sixteen may possibly fix the issue in #633, too. That's how I found it in the first place, because I was looking into the issue with 16 bit PNGs.

@striezel
Copy link
Contributor Author

so any chance it is related to the issue #633 ?

Seems like it. I just made another test with the example from #633 (comment). Using Boost from the current master branch (boostorg/boost@0f43af9) still has the weird colors in the 16 bit image. However, when I switch the submodule for GIL to the version from this PR, then the 16 bit image comes out fine.

@mloskot
Copy link
Member

mloskot commented Apr 26, 2022

Sounds good, thank you @striezel for your help

@mloskot mloskot added the ext/io boost/gil/extension/io/ label Apr 26, 2022
@mloskot mloskot merged commit 7911486 into boostorg:develop Apr 26, 2022
@striezel striezel deleted the fix-png-is-equal-to-sixteen branch April 26, 2022 19:33
@sdebionne
Copy link
Contributor

Sorry for the late feedback but shouldn't is_equal_to_sixteen be just:

struct is_equal_to_sixteen : std::is_same
        <
            std::integral_constant<int, Info::_bit_depth>,
            std::integral_constant<int, 16>
        >
    {};

@mloskot
Copy link
Member

mloskot commented May 2, 2022

Yes, there is no requirement to use mp11 features here.

@striezel
Copy link
Contributor Author

striezel commented May 2, 2022

As I understand it, std::is_same is for comparing types, not values:

template< class T, class U >
struct is_same;

If T and U name the same type (taking into account const/volatile qualifications), provides the member constant value equal to true. Otherwise value is false.

(from https://en.cppreference.com/w/cpp/types/is_same)

In contrast to that boost::mp11::mp_less compares the values:

mp_less<T1, T2> is mp_true when the numeric value of T1::value is less than the numeric value of T2::value, mp_false otherwise.

(from https://www.boost.org/doc/libs/1_79_0/libs/mp11/doc/html/mp11.html#mp_lesst1_t2)

To summarize it, the mp11 stuff compares values while std::is_same compares types.

So this would not be the same as the implementation that got merged. Or am I missing something here?

@sdebionne
Copy link
Contributor

sdebionne commented May 3, 2022

std::is_same is indeed for comparing types. Since std::integral_constant<int, Info::_bit_depth> and std::integral_constant<int, 16> are both types that represents compile time integral values it's appropriate to use std::is_same for comparing the values of the same underlying type (int). One could use mp11::mp_similar to make the comparison a bit looser (e.g. discarding constness, etc...).

Note that I am not saying your implementation does not work, it's just a bit over complicated.

template <typename Info>
struct is_equal_to_sixteen : std::is_same
    <
        std::integral_constant<int, Info::_bit_depth>,
        std::integral_constant<int, 16>
    >
{};

https://godbolt.org/z/5qGz4WKTc

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cat/bug But reports and bug fixes ext/io boost/gil/extension/io/
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants