Commit 098a594c authored by Dale Curtis's avatar Dale Curtis Committed by Commit Bot

Fix transfer function for TransferID::SMPTEST428_1.

The coefficients were completely wrong. I verified the correct
values suggested in the bug against ITU-T H.273.

This also resolves two comments in the test file remarking that
YCOCG and SMPTEST428_1 were "weird" due to negative values...
which was due to both matrices being wrong.

76fb0d6f fixed YCOCG, but didn't
resolve the test comments.

R=ccameron

Fixed: 1087073
Change-Id: If1d9bfe1810de81ddee1cabdf410c1f41e4602ba
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2218859
Commit-Queue: ccameron <ccameron@chromium.org>
Reviewed-by: default avatarccameron <ccameron@chromium.org>
Auto-Submit: Dale Curtis <dalecurtis@chromium.org>
Cr-Commit-Position: refs/heads/master@{#773058}
parent 2622cbac
...@@ -893,7 +893,7 @@ bool ColorSpace::GetTransferFunction(TransferID transfer, ...@@ -893,7 +893,7 @@ bool ColorSpace::GetTransferFunction(TransferID transfer,
// software uses the sRGB transfer function. // software uses the sRGB transfer function.
// * User studies shows that users don't really care. // * User studies shows that users don't really care.
// * Apple's CoreVideo uses gamma=1.961. // * Apple's CoreVideo uses gamma=1.961.
// Bearing all of that in mind, use the same transfer funciton as sRGB, // Bearing all of that in mind, use the same transfer function as sRGB,
// which will allow more optimization, and will more closely match other // which will allow more optimization, and will more closely match other
// media players. // media players.
case ColorSpace::TransferID::IEC61966_2_1: case ColorSpace::TransferID::IEC61966_2_1:
...@@ -908,8 +908,7 @@ bool ColorSpace::GetTransferFunction(TransferID transfer, ...@@ -908,8 +908,7 @@ bool ColorSpace::GetTransferFunction(TransferID transfer,
fn->g = 1.961000000000f; fn->g = 1.961000000000f;
return true; return true;
case ColorSpace::TransferID::SMPTEST428_1: case ColorSpace::TransferID::SMPTEST428_1:
fn->a = 0.225615407568f; fn->a = 1.034080527699f; // (52.37 / 48.0) ^ (1.0 / 2.6) per ITU-T H.273.
fn->e = -1.091041666667f;
fn->g = 2.600000000000f; fn->g = 2.600000000000f;
return true; return true;
case ColorSpace::TransferID::IEC61966_2_4: case ColorSpace::TransferID::IEC61966_2_4:
......
...@@ -40,6 +40,7 @@ ColorSpace::TransferID simple_transfers[] = { ...@@ -40,6 +40,7 @@ ColorSpace::TransferID simple_transfers[] = {
ColorSpace::TransferID::GAMMA28, ColorSpace::TransferID::GAMMA28,
ColorSpace::TransferID::SMPTE170M, ColorSpace::TransferID::SMPTE170M,
ColorSpace::TransferID::SMPTE240M, ColorSpace::TransferID::SMPTE240M,
ColorSpace::TransferID::SMPTEST428_1,
ColorSpace::TransferID::LINEAR, ColorSpace::TransferID::LINEAR,
ColorSpace::TransferID::LOG, ColorSpace::TransferID::LOG,
ColorSpace::TransferID::LOG_SQRT, ColorSpace::TransferID::LOG_SQRT,
...@@ -53,29 +54,16 @@ ColorSpace::TransferID simple_transfers[] = { ...@@ -53,29 +54,16 @@ ColorSpace::TransferID simple_transfers[] = {
ColorSpace::TransferID::IEC61966_2_1_HDR, ColorSpace::TransferID::IEC61966_2_1_HDR,
}; };
// This one is weird as the non-linear numbers are not between 0 and 1.
ColorSpace::TransferID noninvertible_transfers[] = {
ColorSpace::TransferID::SMPTEST428_1,
};
ColorSpace::TransferID extended_transfers[] = { ColorSpace::TransferID extended_transfers[] = {
ColorSpace::TransferID::LINEAR_HDR, ColorSpace::TransferID::LINEAR_HDR,
ColorSpace::TransferID::IEC61966_2_1_HDR, ColorSpace::TransferID::IEC61966_2_1_HDR,
}; };
ColorSpace::MatrixID all_matrices[] = { ColorSpace::MatrixID all_matrices[] = {
ColorSpace::MatrixID::RGB, ColorSpace::MatrixID::RGB, ColorSpace::MatrixID::BT709,
ColorSpace::MatrixID::BT709, ColorSpace::MatrixID::FCC, ColorSpace::MatrixID::BT470BG,
ColorSpace::MatrixID::FCC, ColorSpace::MatrixID::SMPTE170M, ColorSpace::MatrixID::SMPTE240M,
ColorSpace::MatrixID::BT470BG, ColorSpace::MatrixID::YCOCG, ColorSpace::MatrixID::BT2020_NCL,
ColorSpace::MatrixID::SMPTE170M,
ColorSpace::MatrixID::SMPTE240M,
// YCOCG produces lots of negative values which isn't compatible with many
// transfer functions.
// TODO(hubbe): Test this separately.
// ColorSpace::MatrixID::YCOCG,
ColorSpace::MatrixID::BT2020_NCL,
ColorSpace::MatrixID::YDZDX, ColorSpace::MatrixID::YDZDX,
}; };
...@@ -578,35 +566,6 @@ INSTANTIATE_TEST_SUITE_P(ColorSpace, ...@@ -578,35 +566,6 @@ INSTANTIATE_TEST_SUITE_P(ColorSpace,
TransferTest, TransferTest,
testing::ValuesIn(simple_transfers)); testing::ValuesIn(simple_transfers));
class NonInvertibleTransferTest
: public testing::TestWithParam<ColorSpace::TransferID> {};
TEST_P(NonInvertibleTransferTest, basicTest) {
gfx::ColorSpace space_with_transfer(ColorSpace::PrimaryID::BT709, GetParam(),
ColorSpace::MatrixID::RGB,
ColorSpace::RangeID::FULL);
gfx::ColorSpace space_linear(
ColorSpace::PrimaryID::BT709, ColorSpace::TransferID::LINEAR,
ColorSpace::MatrixID::RGB, ColorSpace::RangeID::FULL);
std::unique_ptr<ColorTransform> to_linear(ColorTransform::NewColorTransform(
space_with_transfer, space_linear,
ColorTransform::Intent::INTENT_ABSOLUTE));
std::unique_ptr<ColorTransform> from_linear(ColorTransform::NewColorTransform(
space_linear, space_with_transfer,
ColorTransform::Intent::INTENT_ABSOLUTE));
// These transforms should not crash when created or applied.
float x = 0.5;
ColorTransform::TriStim tristim(x, x, x);
to_linear->Transform(&tristim, 1);
from_linear->Transform(&tristim, 1);
}
INSTANTIATE_TEST_SUITE_P(ColorSpace,
NonInvertibleTransferTest,
testing::ValuesIn(noninvertible_transfers));
class ExtendedTransferTest class ExtendedTransferTest
: public testing::TestWithParam<ColorSpace::TransferID> {}; : public testing::TestWithParam<ColorSpace::TransferID> {};
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment