From b898a46d7fbb6131a805719f3ee050fddff88138 Mon Sep 17 00:00:00 2001 From: Nico Weber Date: Wed, 1 Mar 2023 18:34:01 -0500 Subject: [PATCH] AK: Make FixedPoint(FloatingPoint) ctor round instead of truncating This is needed to have code for creating an in-memory sRGB profile using the (floating-ppoint) numbers from the sRGB spec and having the fixed-point values in the profile match what they are in other software (such as GIMP). It has the side effect of making the FixedPoint ctor no longer constexpr (which seems fine; nothing was currently relying on that). Some of FixedPoint's member functions don't round yet, which requires tweaking a test. --- AK/FixedPoint.h | 6 ++++-- Tests/AK/TestFixedPoint.cpp | 22 ++++++++++++++++------ Tests/LibEDID/TestEDID.cpp | 17 +++++++++++++---- 3 files changed, 33 insertions(+), 12 deletions(-) diff --git a/AK/FixedPoint.h b/AK/FixedPoint.h index 9b4c717ec0..d80df7d3fd 100644 --- a/AK/FixedPoint.h +++ b/AK/FixedPoint.h @@ -36,11 +36,13 @@ public: { } +#ifndef KERNEL template - constexpr FixedPoint(F value) - : m_value(static_cast(value * (static_cast(1) << precision))) + FixedPoint(F value) + : m_value(round_to(value * (static_cast(1) << precision))) { } +#endif template explicit constexpr FixedPoint(FixedPoint const& other) diff --git a/Tests/AK/TestFixedPoint.cpp b/Tests/AK/TestFixedPoint.cpp index b7fb63e09e..a77da016c8 100644 --- a/Tests/AK/TestFixedPoint.cpp +++ b/Tests/AK/TestFixedPoint.cpp @@ -72,6 +72,16 @@ TEST_CASE(rounding) EXPECT_EQ(Type(-1.5).lfloor(), -2); EXPECT_EQ(Type(-1.5).lceil(), -1); EXPECT_EQ(Type(-1.5).ltrunk(), -1); + + // Check that sRGB TRC curve parameters match the s15fixed16 values stored in Gimp's built-in profile. + // (This only requires that the FixedPoint<> constructor rounds before truncating to the fixed-point value, + // as it should anyways.) + using S15Fixed16 = FixedPoint<16, i32>; + EXPECT_EQ(S15Fixed16(2.4).raw(), 0x26666); + EXPECT_EQ(S15Fixed16(1 / 1.055).raw(), 0xf2a7); + EXPECT_EQ(S15Fixed16(0.055 / 1.055).raw(), 0xd59); + EXPECT_EQ(S15Fixed16(1 / 12.92).raw(), 0x13d0); + EXPECT_EQ(S15Fixed16(0.04045).raw(), 0xa5b); } TEST_CASE(logarithm) @@ -115,7 +125,7 @@ TEST_CASE(cast) { FixedPoint<16, u32> downcast_value1(FixedPoint<32, u64>(123.4567)); EXPECT((double)downcast_value1 >= 123.4566 && (double)downcast_value1 <= 123.4568); - static constexpr FixedPoint<32, u64> value1(321.7654); + static FixedPoint<32, u64> const value1(321.7654); downcast_value1 = value1; EXPECT((double)downcast_value1 >= 321.7653 && (double)downcast_value1 <= 321.7655); FixedPoint<6, u32> downcast_value2(FixedPoint<32, u64>(4567.123456)); @@ -150,12 +160,12 @@ TEST_CASE(formatter) EXPECT_EQ(DeprecatedString::formatted("{}", FixedPoint<4>(123.456)), "123.4375"sv); EXPECT_EQ(DeprecatedString::formatted("{}", FixedPoint<4>(-123.456)), "-123.4375"sv); EXPECT_EQ(DeprecatedString::formatted("{}", FixedPoint<16> {}), "0"sv); - EXPECT_EQ(DeprecatedString::formatted("{}", FixedPoint<16>(0.1)), "0.09999"sv); - EXPECT_EQ(DeprecatedString::formatted("{}", FixedPoint<16>(0.02)), "0.019989"sv); - EXPECT_EQ(DeprecatedString::formatted("{}", FixedPoint<16>(0.003)), "0.00299"sv); + EXPECT_EQ(DeprecatedString::formatted("{}", FixedPoint<16>(0.1)), "0.100006"sv); + EXPECT_EQ(DeprecatedString::formatted("{}", FixedPoint<16>(0.02)), "0.020004"sv); + EXPECT_EQ(DeprecatedString::formatted("{}", FixedPoint<16>(0.003)), "0.003005"sv); EXPECT_EQ(DeprecatedString::formatted("{}", FixedPoint<16>(0.0004)), "0.000396"sv); EXPECT_EQ(DeprecatedString::formatted("{}", FixedPoint<16>(0.0000000005)), "0"sv); - EXPECT_EQ(DeprecatedString::formatted("{}", FixedPoint<16>(-0.1)), "-0.099991"sv); - EXPECT_EQ(DeprecatedString::formatted("{}", FixedPoint<16>(-0.02)), "-0.01999"sv); + EXPECT_EQ(DeprecatedString::formatted("{}", FixedPoint<16>(-0.1)), "-0.100007"sv); + EXPECT_EQ(DeprecatedString::formatted("{}", FixedPoint<16>(-0.02)), "-0.020005"sv); EXPECT_EQ(DeprecatedString::formatted("{}", FixedPoint<16>(-0.0000000005)), "0"sv); } diff --git a/Tests/LibEDID/TestEDID.cpp b/Tests/LibEDID/TestEDID.cpp index 9925b0c33f..ffe36e17ad 100644 --- a/Tests/LibEDID/TestEDID.cpp +++ b/Tests/LibEDID/TestEDID.cpp @@ -520,10 +520,19 @@ TEST_CASE(dmt_frequency) { auto* dmt = EDID::DMT::find_timing_by_dmt_id(0x4); EXPECT(dmt); - static constexpr FixedPoint<16, u32> expected_vertical_frequency(59.940); - EXPECT(dmt->vertical_frequency_hz() == expected_vertical_frequency); - static constexpr FixedPoint<16, u32> expected_horizontal_frequency(31.469); - EXPECT(dmt->horizontal_frequency_khz() == expected_horizontal_frequency); + + // FIXME: Use the FixedPoint(double) ctor like `expected_vertical_frequency(59.940)` instead of + // dividing by 1000 in the next line once FixedPoint::operator/ rounds. + // 1. DMT.cpp is built as part of the kernel (despite being in Userland/) + // 2. The Kernel can't use floating point + // 3. So it has to use FixedPoint(59940) / 1000 + // 4. The FixedPoint(double) ctor rounds, but FixedPoint::operator/ currently doesn't, + // so FixedPoint(59.940) has a different lowest bit than + // FixedPoint(59940) / 1000. So the test can't use the FixedPoint(double) ctor at the moment. + static FixedPoint<16, u32> const expected_vertical_frequency(59940); + EXPECT(dmt->vertical_frequency_hz() == expected_vertical_frequency / 1000); + static FixedPoint<16, u32> const expected_horizontal_frequency(31469); + EXPECT(dmt->horizontal_frequency_khz() == expected_horizontal_frequency / 1000); } TEST_CASE(vic)