Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1779163002:
[qcms] Update primaries used to build internal sRGB profile (Closed)
|
Created:
4 years, 9 months ago by radu.velea Modified:
4 years, 8 months ago Reviewers:
Noel Gordon CC:
chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[qcms] Update primaries used to build internal sRGB profile
Before this change:
Test qcms internal sRGB primaries against official sRGB
IEC61966-2.1 profile (D50) primaries
-14 11 -4
-8 10 -2
0 3 -17
Total error = 0x45 [0.001053], and an RMS white point error
of 0.000031 compared to the ICC spec PCS D50 white point.
Update the XYZ primaries used to construct the internal sRGB
profile, found after iterating through many different sources
of the sRGB XYZ primaries (refer to spreadsheet added to the
bug for details).
Update qcms_test_internal_srgb to check if the internal sRGB
profile is bogus. Also display it's media white point and the
difference (error) from the expected D65 XYZ:
Test media white point against expected D65 XYZ values:
Internal values = [0xF351, 0x10000, 0x116CC]
White point error = [0, 0, 0]
Also test the internal sRGB profile PCS white point: compute
the profile white point in xyY, then compare to the ICC spec
PCS D50 in xyY:
Test PCS white point error
Profile White point test: xyY [ 0.345703 0.358538 1.000 ]
White point error: 0.000001
BUG=580917
Committed: https://crrev.com/4371edbc78a477a0c13947adea0137b7aed86b29
Cr-Commit-Position: refs/heads/master@{#383696}
Patch Set 1 #
Total comments: 12
Patch Set 2 : Added internal profile bogus test. Fix D50 -> D65 #
Total comments: 20
Patch Set 3 : Changed print messages and comments #
Total comments: 8
Patch Set 4 : Changed Tabs to Spaces #Patch Set 5 : #Patch Set 6 : #Patch Set 7 : #Patch Set 8 : Updated TestExpectations #Patch Set 9 : Rebase #Patch Set 10 : Rebase 2 #
Total comments: 3
Patch Set 11 : Update with values from http://brucelindbloom.com/ #Patch Set 12 : Replaced primaries with values from ninedegreesbelow.com; Updated internal test #Patch Set 13 : Adjust indentation #Patch Set 14 : Update TestExpectations after testing with ninedegreesbelow primaries #
Total comments: 8
Patch Set 15 : Fix comment #
Total comments: 3
Patch Set 16 : Created auxliary test function. NeedsManualRebaseline -> NeedsRebaseline #
Total comments: 7
Patch Set 17 : Update Readme #
Total comments: 2
Patch Set 18 : Add helper functions #
Total comments: 10
Patch Set 19 : Moved printfs, added comments, removed newlines #
Messages
Total messages: 65 (28 generated)
Description was changed from ========== [qcms] Used official colorants to build internal sRGB profile BUG=580917 ========== to ========== [qcms] Used official colorants to build internal sRGB profile Before this change: Test qcms internal sRGB colorant matrix values against the official sRGB IEC61966-2.1 color profile (D50) primaries. [ -14 11 -4 ] [ -8 10 -2 ] [ 0 3 -17 ] Total error = 0x45 [0.001053] After: Test qcms internal sRGB colorant matrix against official sRGB IEC61966-2.1 color (D50) primaries 0 0 0 0 0 0 0 0 0 Total error = 0x0 [0.000000] BUG=580917 ==========
radu.velea@intel.com changed reviewers: + noel@chromium.org
Description was changed from ========== [qcms] Used official colorants to build internal sRGB profile Before this change: Test qcms internal sRGB colorant matrix values against the official sRGB IEC61966-2.1 color profile (D50) primaries. [ -14 11 -4 ] [ -8 10 -2 ] [ 0 3 -17 ] Total error = 0x45 [0.001053] After: Test qcms internal sRGB colorant matrix against official sRGB IEC61966-2.1 color (D50) primaries 0 0 0 0 0 0 0 0 0 Total error = 0x0 [0.000000] BUG=580917 ========== to ========== [qcms] Used official colorants to build internal sRGB profile Before this change: Test qcms internal sRGB colorant matrix against official sRGB IEC61966-2.1 color (D50) primaries -14 11 -4 -8 10 -2 0 3 -17 Total error = 0x45 [0.001053] After: Test qcms internal sRGB colorant matrix against official sRGB IEC61966-2.1 color (D50) primaries 0 0 0 0 0 0 0 0 0 Total error = 0x0 [0.000000] BUG=580917 ==========
Description was changed from ========== [qcms] Used official colorants to build internal sRGB profile Before this change: Test qcms internal sRGB colorant matrix against official sRGB IEC61966-2.1 color (D50) primaries -14 11 -4 -8 10 -2 0 3 -17 Total error = 0x45 [0.001053] After: Test qcms internal sRGB colorant matrix against official sRGB IEC61966-2.1 color (D50) primaries 0 0 0 0 0 0 0 0 0 Total error = 0x0 [0.000000] BUG=580917 ========== to ========== [qcms] Used official colorants to build internal sRGB profile Before this change: Test qcms internal sRGB colorant matrix against official sRGB IEC61966-2.1 color (D50) primaries -14 11 -4 -8 10 -2 0 3 -17 Total error = 0x45 [0.001053] After: Test qcms internal sRGB colorant matrix against official sRGB IEC61966-2.1 color (D50) primaries 0 0 0 0 0 0 0 0 0 Total error = 0x0 [0.000000] BUG=580917 ==========
https://codereview.chromium.org/1779163002/diff/1/third_party/qcms/src/iccread.c File third_party/qcms/src/iccread.c (right): https://codereview.chromium.org/1779163002/diff/1/third_party/qcms/src/iccrea... third_party/qcms/src/iccread.c:1206: /*unused*/ qcms_profile* qcms_profile_create_rgb_with_table( I think there are a lot of unused functions that can be removed after this change. https://codereview.chromium.org/1779163002/diff/1/third_party/qcms/src/iccrea... third_party/qcms/src/iccread.c:1241: /*unused*/ qcms_CIE_xyY white_point_from_temp(int temp_K) This is an example.
https://codereview.chromium.org/1779163002/diff/1/third_party/qcms/src/iccread.c File third_party/qcms/src/iccread.c (right): https://codereview.chromium.org/1779163002/diff/1/third_party/qcms/src/iccrea... third_party/qcms/src/iccread.c:1206: /*unused*/ qcms_profile* qcms_profile_create_rgb_with_table( On 2016/03/10 15:08:32, radu.velea wrote: > I think there are a lot of unused functions that can be removed after this > change. This is can probably go, but in a future CL. https://codereview.chromium.org/1779163002/diff/1/third_party/qcms/src/iccrea... third_party/qcms/src/iccread.c:1241: /*unused*/ qcms_CIE_xyY white_point_from_temp(int temp_K) On 2016/03/10 15:08:32, radu.velea wrote: > This is an example. I would retain this code, not remove it, it is used by Mozilla. https://codereview.chromium.org/1779163002/diff/1/third_party/qcms/src/iccrea... third_party/qcms/src/iccread.c:1294: struct XYZNumber D50 = {0xF351, 0x10000, 0x116CC}; // TODO - find actual values. The values are correct, decoding them gives: 0x0f351 0.95045471191406 (0.9505) 0x01000 1 (1.0000) 0x116cc 1.08905029296875 (1.0891) namely, D65 in XYZ coordinates. This is the reference media white point, which is defined to be D65 for the sRGB / Rec.709 color profiles. So change this line struct XYZNumber D50 = {0xF351, 0x10000, 0x116CC}; // TODO - find actual values. to read ... // Standard Illuminant D65 in XYZ coordinates, which is the standard // sRGB IEC61966-2.1 / Rec.709 profile reference media white point. struct XYZNumber D65 = { 0xf351, 0x10000, 0x116cc // ( 0.950455, 1.000000, 1.089050 ) }; https://codereview.chromium.org/1779163002/diff/1/third_party/qcms/src/iccrea... third_party/qcms/src/iccread.c:1295: // D50 white point and colorant matrix for the official sRGB IEC61966-2.1 color profile. // sRGB IEC61966-2.1 / Rec.709 color profile primaries, chromatically // adapted (via Bradford procedures) to the ICC PCS white point, D50. s15Fixed16Number primaries[3][3] = { { 0x06fa2, 0x06299, 0x024a0 }, // ( 0.436066, 0.385147, 0.143066 ) { 0x038f5, 0x0b785, 0x00f84 }, // ( 0.222488, 0.716873, 0.060608 ) { 0x00390, 0x018da, 0x0b6cf }, // ( 0.013916, 0.097076, 0.714096 ) }; https://codereview.chromium.org/1779163002/diff/1/third_party/qcms/src/iccrea... third_party/qcms/src/iccread.c:1323: profile->redColorant.X = colorants[0][0]; colorants -> primaries
Re the ChangeLog: does the test harness report the media white points?
Description was changed from ========== [qcms] Used official colorants to build internal sRGB profile Before this change: Test qcms internal sRGB colorant matrix against official sRGB IEC61966-2.1 color (D50) primaries -14 11 -4 -8 10 -2 0 3 -17 Total error = 0x45 [0.001053] After: Test qcms internal sRGB colorant matrix against official sRGB IEC61966-2.1 color (D50) primaries 0 0 0 0 0 0 0 0 0 Total error = 0x0 [0.000000] BUG=580917 ========== to ========== [qcms] Used official colorants to build internal sRGB profile Before this change: Test qcms internal sRGB colorant matrix against official sRGB IEC61966-2.1 color (D50) primaries -14 11 -4 -8 10 -2 0 3 -17 Total error = 0x45 [0.001053] After: Test qcms internal sRGB colorant matrix against official sRGB IEC61966-2.1 color (D50) primaries 0 0 0 0 0 0 0 0 0 Total error = 0x0 [0.000000] BUG=580917 ==========
https://codereview.chromium.org/1779163002/diff/1/third_party/qcms/src/iccread.c File third_party/qcms/src/iccread.c (right): https://codereview.chromium.org/1779163002/diff/1/third_party/qcms/src/iccrea... third_party/qcms/src/iccread.c:1206: /*unused*/ qcms_profile* qcms_profile_create_rgb_with_table( On 2016/03/11 13:53:48, noel gordon wrote: > On 2016/03/10 15:08:32, radu.velea wrote: > > I think there are a lot of unused functions that can be removed after this > > change. > > This is can probably go, but in a future CL. Done. https://codereview.chromium.org/1779163002/diff/1/third_party/qcms/src/iccrea... third_party/qcms/src/iccread.c:1241: /*unused*/ qcms_CIE_xyY white_point_from_temp(int temp_K) On 2016/03/11 13:53:48, noel gordon wrote: > On 2016/03/10 15:08:32, radu.velea wrote: > > This is an example. > > I would retain this code, not remove it, it is used by Mozilla. OK, but removing the static keyword so it does not generate a compile-time warning/error. https://codereview.chromium.org/1779163002/diff/1/third_party/qcms/src/iccrea... third_party/qcms/src/iccread.c:1294: struct XYZNumber D50 = {0xF351, 0x10000, 0x116CC}; // TODO - find actual values. On 2016/03/11 13:53:48, noel gordon wrote: > The values are correct, decoding them gives: > > 0x0f351 0.95045471191406 (0.9505) > 0x01000 1 (1.0000) > 0x116cc 1.08905029296875 (1.0891) > > namely, D65 in XYZ coordinates. This is the reference media white point, which > is defined to be D65 for the sRGB / Rec.709 color profiles. So change this line > > struct XYZNumber D50 = {0xF351, 0x10000, 0x116CC}; // TODO - find actual values. > > to read ... > > // Standard Illuminant D65 in XYZ coordinates, which is the standard > // sRGB IEC61966-2.1 / Rec.709 profile reference media white point. > > struct XYZNumber D65 = { > 0xf351, 0x10000, 0x116cc // ( 0.950455, 1.000000, 1.089050 ) > }; Done. https://codereview.chromium.org/1779163002/diff/1/third_party/qcms/src/iccrea... third_party/qcms/src/iccread.c:1295: // D50 white point and colorant matrix for the official sRGB IEC61966-2.1 color profile. On 2016/03/11 13:53:48, noel gordon wrote: > // sRGB IEC61966-2.1 / Rec.709 color profile primaries, chromatically > // adapted (via Bradford procedures) to the ICC PCS white point, D50. > > s15Fixed16Number primaries[3][3] = { > { 0x06fa2, 0x06299, 0x024a0 }, // ( 0.436066, 0.385147, 0.143066 ) > { 0x038f5, 0x0b785, 0x00f84 }, // ( 0.222488, 0.716873, 0.060608 ) > { 0x00390, 0x018da, 0x0b6cf }, // ( 0.013916, 0.097076, 0.714096 ) > }; Done. https://codereview.chromium.org/1779163002/diff/1/third_party/qcms/src/iccrea... third_party/qcms/src/iccread.c:1323: profile->redColorant.X = colorants[0][0]; On 2016/03/11 13:53:48, noel gordon wrote: > colorants -> primaries Done.
Description was changed from ========== [qcms] Used official colorants to build internal sRGB profile Before this change: Test qcms internal sRGB colorant matrix against official sRGB IEC61966-2.1 color (D50) primaries -14 11 -4 -8 10 -2 0 3 -17 Total error = 0x45 [0.001053] After: Test qcms internal sRGB colorant matrix against official sRGB IEC61966-2.1 color (D50) primaries 0 0 0 0 0 0 0 0 0 Total error = 0x0 [0.000000] BUG=580917 ========== to ========== [qcms] Used official colorants to build internal sRGB profile Before this change: Test qcms internal sRGB colorant matrix against official sRGB IEC61966-2.1 color (D50) primaries -14 11 -4 -8 10 -2 0 3 -17 Total error = 0x45 [0.001053] After: Test qcms internal sRGB colorant matrix against official sRGB IEC61966-2.1 color (D50) primaries 0 0 0 0 0 0 0 0 0 Total error = 0x0 [0.000000] Updated qcms_test_internal_srgb to check if internal profile is bogus, display media white point and the difference between reference D65 and actual XYZ values. BUG=580917 ==========
https://codereview.chromium.org/1779163002/diff/20001/third_party/qcms/src/ic... File third_party/qcms/src/iccread.c (right): https://codereview.chromium.org/1779163002/diff/20001/third_party/qcms/src/ic... third_party/qcms/src/iccread.c:1294: // Standard Illuminant D65 in XYZ coordinates, which is the standard space before this line. https://codereview.chromium.org/1779163002/diff/20001/third_party/qcms/src/ic... third_party/qcms/src/iccread.c:1299: // sRGB IEC61966-2.1 / Rec.709 color profile primaries, chromatically space before this line. https://codereview.chromium.org/1779163002/diff/20001/third_party/qcms/src/te... File third_party/qcms/src/tests/qcms_test_internal_srgb.c (right): https://codereview.chromium.org/1779163002/diff/20001/third_party/qcms/src/te... third_party/qcms/src/tests/qcms_test_internal_srgb.c:12: // Colorant matrix and media white point for the official sRGB IEC61966-2.1 color (D50) primaries. // D50 adapted color primaries of the sRGB IEC61966-2.1 color profile. https://codereview.chromium.org/1779163002/diff/20001/third_party/qcms/src/te... third_party/qcms/src/tests/qcms_test_internal_srgb.c:16: {0x00390 /*0.013916*/, /*0.097076*/ 0x018da, /*0.714096*/ 0x0b6cf}}; 0x0b6cf}}; -> 0x0b6cf} }; https://codereview.chromium.org/1779163002/diff/20001/third_party/qcms/src/te... third_party/qcms/src/tests/qcms_test_internal_srgb.c:17: static struct XYZNumber D65 = { space before this line // Reference media white point of the sRGB IEC61966-2.1 color profile. static struct XYZNumber D65 = { ... https://codereview.chromium.org/1779163002/diff/20001/third_party/qcms/src/te... third_party/qcms/src/tests/qcms_test_internal_srgb.c:34: fprintf(stderr, "Profile is bogus\n"); "Failure: the internal sRGB profile failed the bogus profile check\n" https://codereview.chromium.org/1779163002/diff/20001/third_party/qcms/src/te... third_party/qcms/src/tests/qcms_test_internal_srgb.c:39: printf("Test qcms internal sRGB colorant matrix against official sRGB IEC61966-2.1 color (D50) primaries\n"); printf("Test qcms internal sRGB colorant primaries against sRGB IEC61966-2.1 color primaries\n"); https://codereview.chromium.org/1779163002/diff/20001/third_party/qcms/src/te... third_party/qcms/src/tests/qcms_test_internal_srgb.c:62: printf("Test media white point against official sRGB IEC61966-2.1 D65 XYZ values\n"); printf("Test media white point against expected sRGB IEC61966-2.1 D65 XYZ values\n");
https://codereview.chromium.org/1779163002/diff/20001/third_party/qcms/src/ic... File third_party/qcms/src/iccread.c (right): https://codereview.chromium.org/1779163002/diff/20001/third_party/qcms/src/ic... third_party/qcms/src/iccread.c:1293: int num_entries = 1024; remove this. https://codereview.chromium.org/1779163002/diff/20001/third_party/qcms/src/ic... third_party/qcms/src/iccread.c:1307: table = build_sRGB_gamma_table(num_entries); No need to change this line.
https://codereview.chromium.org/1779163002/diff/20001/third_party/qcms/src/ic... File third_party/qcms/src/iccread.c (right): https://codereview.chromium.org/1779163002/diff/20001/third_party/qcms/src/ic... third_party/qcms/src/iccread.c:1293: int num_entries = 1024; On 2016/03/11 15:20:44, noel gordon wrote: > remove this. Done. https://codereview.chromium.org/1779163002/diff/20001/third_party/qcms/src/ic... third_party/qcms/src/iccread.c:1294: // Standard Illuminant D65 in XYZ coordinates, which is the standard On 2016/03/11 15:19:20, noel gordon wrote: > space before this line. Done. https://codereview.chromium.org/1779163002/diff/20001/third_party/qcms/src/ic... third_party/qcms/src/iccread.c:1299: // sRGB IEC61966-2.1 / Rec.709 color profile primaries, chromatically On 2016/03/11 15:19:21, noel gordon wrote: > space before this line. Done. https://codereview.chromium.org/1779163002/diff/20001/third_party/qcms/src/ic... third_party/qcms/src/iccread.c:1307: table = build_sRGB_gamma_table(num_entries); On 2016/03/11 15:20:44, noel gordon wrote: > No need to change this line. Done. https://codereview.chromium.org/1779163002/diff/20001/third_party/qcms/src/te... File third_party/qcms/src/tests/qcms_test_internal_srgb.c (right): https://codereview.chromium.org/1779163002/diff/20001/third_party/qcms/src/te... third_party/qcms/src/tests/qcms_test_internal_srgb.c:12: // Colorant matrix and media white point for the official sRGB IEC61966-2.1 color (D50) primaries. On 2016/03/11 15:19:21, noel gordon wrote: > // D50 adapted color primaries of the sRGB IEC61966-2.1 color profile. Done. https://codereview.chromium.org/1779163002/diff/20001/third_party/qcms/src/te... third_party/qcms/src/tests/qcms_test_internal_srgb.c:16: {0x00390 /*0.013916*/, /*0.097076*/ 0x018da, /*0.714096*/ 0x0b6cf}}; On 2016/03/11 15:19:21, noel gordon wrote: > 0x0b6cf}}; -> > > 0x0b6cf} > }; Done. https://codereview.chromium.org/1779163002/diff/20001/third_party/qcms/src/te... third_party/qcms/src/tests/qcms_test_internal_srgb.c:17: static struct XYZNumber D65 = { On 2016/03/11 15:19:21, noel gordon wrote: > space before this line > // Reference media white point of the sRGB IEC61966-2.1 color profile. > static struct XYZNumber D65 = { > ... Done. https://codereview.chromium.org/1779163002/diff/20001/third_party/qcms/src/te... third_party/qcms/src/tests/qcms_test_internal_srgb.c:34: fprintf(stderr, "Profile is bogus\n"); On 2016/03/11 15:19:21, noel gordon wrote: > "Failure: the internal sRGB profile failed the bogus profile check\n" Done. https://codereview.chromium.org/1779163002/diff/20001/third_party/qcms/src/te... third_party/qcms/src/tests/qcms_test_internal_srgb.c:39: printf("Test qcms internal sRGB colorant matrix against official sRGB IEC61966-2.1 color (D50) primaries\n"); On 2016/03/11 15:19:21, noel gordon wrote: > printf("Test qcms internal sRGB colorant primaries against sRGB IEC61966-2.1 > color primaries\n"); Done. https://codereview.chromium.org/1779163002/diff/20001/third_party/qcms/src/te... third_party/qcms/src/tests/qcms_test_internal_srgb.c:62: printf("Test media white point against official sRGB IEC61966-2.1 D65 XYZ values\n"); On 2016/03/11 15:19:21, noel gordon wrote: > printf("Test media white point against expected sRGB IEC61966-2.1 D65 XYZ > values\n"); Done.
nits. https://codereview.chromium.org/1779163002/diff/40001/third_party/qcms/src/ic... File third_party/qcms/src/iccread.c (right): https://codereview.chromium.org/1779163002/diff/40001/third_party/qcms/src/ic... third_party/qcms/src/iccread.c:1297: 0xf351, 0x10000, 0x116cc // ( 0.950455, 1.000000, 1.089050 ) Nit: one tab too many. https://codereview.chromium.org/1779163002/diff/40001/third_party/qcms/src/ic... third_party/qcms/src/iccread.c:1303: { 0x06fa2, 0x06299, 0x024a0 }, // ( 0.436066, 0.385147, 0.143066 ) 1303-1304 ditto, one tab too many. https://codereview.chromium.org/1779163002/diff/40001/third_party/qcms/src/te... File third_party/qcms/src/tests/qcms_test_internal_srgb.c (right): https://codereview.chromium.org/1779163002/diff/40001/third_party/qcms/src/te... third_party/qcms/src/tests/qcms_test_internal_srgb.c:16: {0x00390 /*0.013916*/, /*0.097076*/ 0x018da, /*0.714096*/ 0x0b6cf} Nit: One tab too many. https://codereview.chromium.org/1779163002/diff/40001/third_party/qcms/src/te... third_party/qcms/src/tests/qcms_test_internal_srgb.c:21: 0xf351, 0x10000, 0x116cc // ( 0.950455, 1.000000, 1.089050 ) nit: One tab too many.
https://codereview.chromium.org/1779163002/diff/40001/third_party/qcms/src/ic... File third_party/qcms/src/iccread.c (right): https://codereview.chromium.org/1779163002/diff/40001/third_party/qcms/src/ic... third_party/qcms/src/iccread.c:1297: 0xf351, 0x10000, 0x116cc // ( 0.950455, 1.000000, 1.089050 ) On 2016/03/11 15:45:08, noel gordon wrote: > Nit: one tab too many. Done. https://codereview.chromium.org/1779163002/diff/40001/third_party/qcms/src/ic... third_party/qcms/src/iccread.c:1303: { 0x06fa2, 0x06299, 0x024a0 }, // ( 0.436066, 0.385147, 0.143066 ) On 2016/03/11 15:45:08, noel gordon wrote: > 1303-1304 ditto, one tab too many. Done. https://codereview.chromium.org/1779163002/diff/40001/third_party/qcms/src/te... File third_party/qcms/src/tests/qcms_test_internal_srgb.c (right): https://codereview.chromium.org/1779163002/diff/40001/third_party/qcms/src/te... third_party/qcms/src/tests/qcms_test_internal_srgb.c:16: {0x00390 /*0.013916*/, /*0.097076*/ 0x018da, /*0.714096*/ 0x0b6cf} On 2016/03/11 15:45:08, noel gordon wrote: > Nit: One tab too many. Done. https://codereview.chromium.org/1779163002/diff/40001/third_party/qcms/src/te... third_party/qcms/src/tests/qcms_test_internal_srgb.c:21: 0xf351, 0x10000, 0x116cc // ( 0.950455, 1.000000, 1.089050 ) On 2016/03/11 15:45:08, noel gordon wrote: > nit: One tab too many. Done.
With those done, add the whitespace.txt so we run webkit tests (expect their would be changes there).
LGTM.
The CQ bit was checked by noel@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1779163002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1779163002/120001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_asan_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by radu.velea@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1779163002/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1779163002/180001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
https://codereview.chromium.org/1779163002/diff/180001/third_party/qcms/src/i... File third_party/qcms/src/iccread.c (right): https://codereview.chromium.org/1779163002/diff/180001/third_party/qcms/src/i... third_party/qcms/src/iccread.c:1306: }; What were the values produced for this matrix prior to this change?
https://codereview.chromium.org/1779163002/diff/180001/third_party/qcms/src/i... File third_party/qcms/src/iccread.c (right): https://codereview.chromium.org/1779163002/diff/180001/third_party/qcms/src/i... third_party/qcms/src/iccread.c:1306: }; On 2016/03/14 21:31:37, noel gordon wrote: > What were the values produced for this matrix prior to this change? And how does everything compare to the matrix published in the standard? [1]. [1] https://www.w3.org/Graphics/Color/srgb But based on the note attached to https://www.w3.org/Graphics/Color/sRGB.html (at the top of the page), one has to question the numerical accuracy of those tristimulus primaries. Indeed, "Would the real sRGB profile please stand up?" Refer to article with name, http://ninedegreesbelow.com/photography/srgb-profile-comparison.html#conclusion So let's not submit until we've sorted the about out, and can explain the why our munsell chart layout tests have regressed.
https://codereview.chromium.org/1779163002/diff/180001/third_party/qcms/src/i... File third_party/qcms/src/iccread.c (right): https://codereview.chromium.org/1779163002/diff/180001/third_party/qcms/src/i... third_party/qcms/src/iccread.c:1306: }; On 2016/03/14 21:31:37, noel gordon wrote: > What were the values produced for this matrix prior to this change? Original values: 0x06f94 0x062a4 0x0249c 0x038ed 0x0b78f 0x00f82 0x00390 0x018dd 0x0b6be
On 2016/03/15 14:50:28, radu.velea wrote: > https://codereview.chromium.org/1779163002/diff/180001/third_party/qcms/src/i... > File third_party/qcms/src/iccread.c (right): > > https://codereview.chromium.org/1779163002/diff/180001/third_party/qcms/src/i... > third_party/qcms/src/iccread.c:1306: }; > On 2016/03/14 21:31:37, noel gordon wrote: > > What were the values produced for this matrix prior to this change? > > Original values: > 0x06f94 0x062a4 0x0249c > 0x038ed 0x0b78f 0x00f82 > 0x00390 0x018dd 0x0b6be Re-ran the tests with different primaries. The best results for the tests I got with the values from http://brucelindbloom.com/index.html?Eqn_RGB_XYZ_Matrix.html: 0x06fa3 0x06294 0x024a1 0x038f6 0x0b785 0x00f85 0x00391 0x018dc 0x0b6d4
The CQ bit was checked by radu.velea@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1779163002/240001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1779163002/240001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Description was changed from ========== [qcms] Used official colorants to build internal sRGB profile Before this change: Test qcms internal sRGB colorant matrix against official sRGB IEC61966-2.1 color (D50) primaries -14 11 -4 -8 10 -2 0 3 -17 Total error = 0x45 [0.001053] After: Test qcms internal sRGB colorant matrix against official sRGB IEC61966-2.1 color (D50) primaries 0 0 0 0 0 0 0 0 0 Total error = 0x0 [0.000000] Updated qcms_test_internal_srgb to check if internal profile is bogus, display media white point and the difference between reference D65 and actual XYZ values. BUG=580917 ========== to ========== [qcms] Used official colorants to build internal sRGB profile XYZ tristimulus values were taken from: http://ninedegreesbelow.com/photography/srgb-profile-comparison.html#addendum Before this change: Test qcms internal sRGB colorant matrix against official sRGB IEC61966-2.1 color (D50) primaries -14 11 -4 -8 10 -2 0 3 -17 Total error = 0x45 [0.001053] After: Test qcms internal sRGB colorant matrix against official sRGB IEC61966-2.1 color (D50) primaries 0 0 0 0 0 0 0 0 0 Total error = 0x0 [0.000000] Updated qcms_test_internal_srgb to check if internal profile is bogus, display media white point and the difference between reference D65 and actual XYZ values: Test media white point against expected sRGB IEC61966-2.1 D65 XYZ values Internal values = [0xF351, 0x10000, 0x116CC] White point error = [0, 0, 0] Test PCS white point error Profile White point test: xyY [ 0.345703 0.358538.6 1.000000 ] White point error: 0.000001 The results and data after iterating through many different versions of sRGB XYZ primaries can be found here: https://docs.google.com/spreadsheets/d/19fg7Jo76Q7talI2HanmnKPRlmFDUT4VCYQfkU... BUG=580917 ==========
Description was changed from ========== [qcms] Used official colorants to build internal sRGB profile XYZ tristimulus values were taken from: http://ninedegreesbelow.com/photography/srgb-profile-comparison.html#addendum Before this change: Test qcms internal sRGB colorant matrix against official sRGB IEC61966-2.1 color (D50) primaries -14 11 -4 -8 10 -2 0 3 -17 Total error = 0x45 [0.001053] After: Test qcms internal sRGB colorant matrix against official sRGB IEC61966-2.1 color (D50) primaries 0 0 0 0 0 0 0 0 0 Total error = 0x0 [0.000000] Updated qcms_test_internal_srgb to check if internal profile is bogus, display media white point and the difference between reference D65 and actual XYZ values: Test media white point against expected sRGB IEC61966-2.1 D65 XYZ values Internal values = [0xF351, 0x10000, 0x116CC] White point error = [0, 0, 0] Test PCS white point error Profile White point test: xyY [ 0.345703 0.358538.6 1.000000 ] White point error: 0.000001 The results and data after iterating through many different versions of sRGB XYZ primaries can be found here: https://docs.google.com/spreadsheets/d/19fg7Jo76Q7talI2HanmnKPRlmFDUT4VCYQfkU... BUG=580917 ========== to ========== [qcms] Used official colorants to build internal sRGB profile XYZ tristimulus values were taken from: http://ninedegreesbelow.com/photography/srgb-profile-comparison.html#addendum Before this change: Test qcms internal sRGB colorant matrix against official sRGB IEC61966-2.1 color (D50) primaries -14 11 -4 -8 10 -2 0 3 -17 Total error = 0x45 [0.001053] After: Test qcms internal sRGB colorant matrix against official sRGB IEC61966-2.1 color (D50) primaries 0 0 0 0 0 0 0 0 0 Total error = 0x0 [0.000000] Updated qcms_test_internal_srgb to check if internal profile is bogus, display media white point and the difference between reference D65 and actual XYZ values: Test media white point against expected sRGB IEC61966-2.1 D65 XYZ values Internal values = [0xF351, 0x10000, 0x116CC] White point error = [0, 0, 0] Test PCS white point error Profile White point test: xyY [ 0.345703 0.358538.6 1.000000 ] White point error: 0.000001 The results and data after iterating through many different versions of sRGB XYZ primaries can be found here: https://docs.google.com/spreadsheets/d/19fg7Jo76Q7talI2HanmnKPRlmFDUT4VCYQfkU... BUG=580917 ==========
Description was changed from ========== [qcms] Used official colorants to build internal sRGB profile XYZ tristimulus values were taken from: http://ninedegreesbelow.com/photography/srgb-profile-comparison.html#addendum Before this change: Test qcms internal sRGB colorant matrix against official sRGB IEC61966-2.1 color (D50) primaries -14 11 -4 -8 10 -2 0 3 -17 Total error = 0x45 [0.001053] After: Test qcms internal sRGB colorant matrix against official sRGB IEC61966-2.1 color (D50) primaries 0 0 0 0 0 0 0 0 0 Total error = 0x0 [0.000000] Updated qcms_test_internal_srgb to check if internal profile is bogus, display media white point and the difference between reference D65 and actual XYZ values: Test media white point against expected sRGB IEC61966-2.1 D65 XYZ values Internal values = [0xF351, 0x10000, 0x116CC] White point error = [0, 0, 0] Test PCS white point error Profile White point test: xyY [ 0.345703 0.358538.6 1.000000 ] White point error: 0.000001 The results and data after iterating through many different versions of sRGB XYZ primaries can be found here: https://docs.google.com/spreadsheets/d/19fg7Jo76Q7talI2HanmnKPRlmFDUT4VCYQfkU... BUG=580917 ========== to ========== [qcms] Used official colorants to build internal sRGB profile XYZ tristimulus values were taken from: http://ninedegreesbelow.com/photography/srgb-profile-comparison.html#addendum Before this change: Test qcms internal sRGB colorant matrix against official sRGB IEC61966-2.1 color (D50) primaries -14 11 -4 -8 10 -2 0 3 -17 Total error = 0x45 [0.001053] After: Test qcms internal sRGB colorant matrix against official sRGB IEC61966-2.1 color (D50) primaries 0 0 0 0 0 0 0 0 0 Total error = 0x0 [0.000000] Updated qcms_test_internal_srgb to check if internal profile is bogus, display media white point and the difference between reference D65 and actual XYZ values: Test media white point against expected sRGB IEC61966-2.1 D65 XYZ values Internal values = [0xF351, 0x10000, 0x116CC] White point error = [0, 0, 0] Test PCS white point error Profile White point test: xyY [ 0.345703 0.358538.6 1.000000 ] White point error: 0.000001 The results and data after iterating through many different versions of sRGB XYZ primaries can be found here: https://docs.google.com/spreadsheets/d/19fg7Jo76Q7talI2HanmnKPRlmFDUT4VCYQfkU... BUG=580917 ==========
https://codereview.chromium.org/1779163002/diff/260001/third_party/qcms/src/i... File third_party/qcms/src/iccread.c (right): https://codereview.chromium.org/1779163002/diff/260001/third_party/qcms/src/i... third_party/qcms/src/iccread.c:1302: // For details, refer to crbug/580917 Comment is not quite right. // sRGB IEC61966-2.1 / Rec.709 color profile primaries, chromatically // adapted (via Bradford procedures) to D50 white point. For details, // refer to crbug.com/580917 The array have comments indicating their D50 reference white point data. https://codereview.chromium.org/1779163002/diff/260001/third_party/qcms/src/i... third_party/qcms/src/iccread.c:1337: qcms_profile_release(profile); nit: write ti like so qcms_profile_release(profile); free(table); return NO_MEM_PROFILE; https://codereview.chromium.org/1779163002/diff/260001/third_party/qcms/src/t... File third_party/qcms/src/tests/qcms_test_internal_srgb.c (right): https://codereview.chromium.org/1779163002/diff/260001/third_party/qcms/src/t... third_party/qcms/src/tests/qcms_test_internal_srgb.c:13: // D50 adapted color primaries of the sRGB IEC61966-2.1 color profile. D50 adapted color primaries of the internal sRGB color profile. https://codereview.chromium.org/1779163002/diff/260001/third_party/qcms/src/t... third_party/qcms/src/tests/qcms_test_internal_srgb.c:78: X = s15Fixed16Number_to_float(profile->redColorant.X + profile->greenColorant.X + profile->blueColorant.X); You should make Compute distance from ICC D50 xyY a sub routine, and put this code in it. > X = s15Fixed16Number_to_float(profile->redColorant.X + profile->greenColorant.X + profile->blueColorant.X); and convert the numbers individually float rX = s15Fixed16Number_to_float(profile->redColorant.X); float gX = s15Fixed16Number_to_float(profile->greenColorant.X); float bX = s15Fixed16Number_to_float(profile->blueColorant.X); and so on.
https://codereview.chromium.org/1779163002/diff/280001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/TestExpectations (right): https://codereview.chromium.org/1779163002/diff/280001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/TestExpectations:306: crbug.com/580917 compositing/color-matching/image-color-matching.html [ NeedsManualRebaseline ] NeedsManualRebaseline -> NeedsRebaseline
Description was changed from ========== [qcms] Used official colorants to build internal sRGB profile XYZ tristimulus values were taken from: http://ninedegreesbelow.com/photography/srgb-profile-comparison.html#addendum Before this change: Test qcms internal sRGB colorant matrix against official sRGB IEC61966-2.1 color (D50) primaries -14 11 -4 -8 10 -2 0 3 -17 Total error = 0x45 [0.001053] After: Test qcms internal sRGB colorant matrix against official sRGB IEC61966-2.1 color (D50) primaries 0 0 0 0 0 0 0 0 0 Total error = 0x0 [0.000000] Updated qcms_test_internal_srgb to check if internal profile is bogus, display media white point and the difference between reference D65 and actual XYZ values: Test media white point against expected sRGB IEC61966-2.1 D65 XYZ values Internal values = [0xF351, 0x10000, 0x116CC] White point error = [0, 0, 0] Test PCS white point error Profile White point test: xyY [ 0.345703 0.358538.6 1.000000 ] White point error: 0.000001 The results and data after iterating through many different versions of sRGB XYZ primaries can be found here: https://docs.google.com/spreadsheets/d/19fg7Jo76Q7talI2HanmnKPRlmFDUT4VCYQfkU... BUG=580917 ========== to ========== [qcms] Used primaries used to build internal sRGB profile Before this change: Test qcms internal sRGB colorant matrix against official sRGB IEC61966-2.1 color (D50) primaries -14 11 -4 -8 10 -2 0 3 -17 Total error = 0x45 [0.001053] After: Test qcms internal sRGB colorant matrix against official sRGB IEC61966-2.1 color (D50) primaries 0 0 0 0 0 0 0 0 0 Total error = 0x0 [0.000000] Updated qcms_test_internal_srgb to check if internal profile is bogus, display media white point and the difference between reference D65 and actual XYZ values: Test media white point against expected sRGB IEC61966-2.1 D65 XYZ values Internal values = [0xF351, 0x10000, 0x116CC] White point error = [0, 0, 0] Test PCS white point error Profile White point test: xyY [ 0.345703 0.358538.6 1.000000 ] White point error: 0.000001 The results and data after iterating through many different versions of sRGB XYZ primaries can be found here: https://docs.google.com/spreadsheets/d/19fg7Jo76Q7talI2HanmnKPRlmFDUT4VCYQfkU... BUG=580917 ==========
Description was changed from ========== [qcms] Used primaries used to build internal sRGB profile Before this change: Test qcms internal sRGB colorant matrix against official sRGB IEC61966-2.1 color (D50) primaries -14 11 -4 -8 10 -2 0 3 -17 Total error = 0x45 [0.001053] After: Test qcms internal sRGB colorant matrix against official sRGB IEC61966-2.1 color (D50) primaries 0 0 0 0 0 0 0 0 0 Total error = 0x0 [0.000000] Updated qcms_test_internal_srgb to check if internal profile is bogus, display media white point and the difference between reference D65 and actual XYZ values: Test media white point against expected sRGB IEC61966-2.1 D65 XYZ values Internal values = [0xF351, 0x10000, 0x116CC] White point error = [0, 0, 0] Test PCS white point error Profile White point test: xyY [ 0.345703 0.358538.6 1.000000 ] White point error: 0.000001 The results and data after iterating through many different versions of sRGB XYZ primaries can be found here: https://docs.google.com/spreadsheets/d/19fg7Jo76Q7talI2HanmnKPRlmFDUT4VCYQfkU... BUG=580917 ========== to ========== [qcms] Update primaries used to build internal sRGB profile Before this change: Test qcms internal sRGB colorant matrix against official sRGB IEC61966-2.1 color (D50) primaries -14 11 -4 -8 10 -2 0 3 -17 Total error = 0x45 [0.001053] After: Test qcms internal sRGB colorant matrix against official sRGB IEC61966-2.1 color (D50) primaries 0 0 0 0 0 0 0 0 0 Total error = 0x0 [0.000000] Updated qcms_test_internal_srgb to check if internal profile is bogus, display media white point and the difference between reference D65 and actual XYZ values: Test media white point against expected sRGB IEC61966-2.1 D65 XYZ values Internal values = [0xF351, 0x10000, 0x116CC] White point error = [0, 0, 0] Test PCS white point error Profile White point test: xyY [ 0.345703 0.358538.6 1.000000 ] White point error: 0.000001 The results and data after iterating through many different versions of sRGB XYZ primaries can be found here: https://docs.google.com/spreadsheets/d/19fg7Jo76Q7talI2HanmnKPRlmFDUT4VCYQfkU... BUG=580917 ==========
https://codereview.chromium.org/1779163002/diff/260001/third_party/qcms/src/i... File third_party/qcms/src/iccread.c (right): https://codereview.chromium.org/1779163002/diff/260001/third_party/qcms/src/i... third_party/qcms/src/iccread.c:1302: // For details, refer to crbug/580917 On 2016/03/25 09:43:11, noel gordon wrote: > Comment is not quite right. > > // sRGB IEC61966-2.1 / Rec.709 color profile primaries, chromatically > // adapted (via Bradford procedures) to D50 white point. For details, > // refer to crbug.com/580917 > > The array have comments indicating their D50 reference white point data. Done. https://codereview.chromium.org/1779163002/diff/260001/third_party/qcms/src/i... third_party/qcms/src/iccread.c:1337: qcms_profile_release(profile); On 2016/03/25 09:43:10, noel gordon wrote: > nit: write ti like so > > qcms_profile_release(profile); > free(table); > return NO_MEM_PROFILE; Done. https://codereview.chromium.org/1779163002/diff/260001/third_party/qcms/src/t... File third_party/qcms/src/tests/qcms_test_internal_srgb.c (right): https://codereview.chromium.org/1779163002/diff/260001/third_party/qcms/src/t... third_party/qcms/src/tests/qcms_test_internal_srgb.c:13: // D50 adapted color primaries of the sRGB IEC61966-2.1 color profile. On 2016/03/25 09:43:11, noel gordon wrote: > D50 adapted color primaries of the internal sRGB color profile. Done. https://codereview.chromium.org/1779163002/diff/260001/third_party/qcms/src/t... third_party/qcms/src/tests/qcms_test_internal_srgb.c:78: X = s15Fixed16Number_to_float(profile->redColorant.X + profile->greenColorant.X + profile->blueColorant.X); On 2016/03/25 09:43:11, noel gordon wrote: > You should make Compute distance from ICC D50 xyY a sub routine, and put this > code in it. > > > X = s15Fixed16Number_to_float(profile->redColorant.X + > profile->greenColorant.X + profile->blueColorant.X); > > and convert the numbers individually > > float rX = s15Fixed16Number_to_float(profile->redColorant.X); > float gX = s15Fixed16Number_to_float(profile->greenColorant.X); > float bX = s15Fixed16Number_to_float(profile->blueColorant.X); > > and so on. Done. https://codereview.chromium.org/1779163002/diff/280001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/TestExpectations (right): https://codereview.chromium.org/1779163002/diff/280001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/TestExpectations:306: crbug.com/580917 compositing/color-matching/image-color-matching.html [ NeedsManualRebaseline ] On 2016/03/25 09:48:33, noel gordon wrote: > NeedsManualRebaseline -> NeedsRebaseline Done. https://codereview.chromium.org/1779163002/diff/280001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/TestExpectations:306: crbug.com/580917 compositing/color-matching/image-color-matching.html [ NeedsManualRebaseline ] On 2016/03/25 09:48:33, noel gordon wrote: > NeedsManualRebaseline -> NeedsRebaseline Done.
Description was changed from ========== [qcms] Update primaries used to build internal sRGB profile Before this change: Test qcms internal sRGB colorant matrix against official sRGB IEC61966-2.1 color (D50) primaries -14 11 -4 -8 10 -2 0 3 -17 Total error = 0x45 [0.001053] After: Test qcms internal sRGB colorant matrix against official sRGB IEC61966-2.1 color (D50) primaries 0 0 0 0 0 0 0 0 0 Total error = 0x0 [0.000000] Updated qcms_test_internal_srgb to check if internal profile is bogus, display media white point and the difference between reference D65 and actual XYZ values: Test media white point against expected sRGB IEC61966-2.1 D65 XYZ values Internal values = [0xF351, 0x10000, 0x116CC] White point error = [0, 0, 0] Test PCS white point error Profile White point test: xyY [ 0.345703 0.358538.6 1.000000 ] White point error: 0.000001 The results and data after iterating through many different versions of sRGB XYZ primaries can be found here: https://docs.google.com/spreadsheets/d/19fg7Jo76Q7talI2HanmnKPRlmFDUT4VCYQfkU... BUG=580917 ========== to ========== [qcms] Update primaries used to build internal sRGB profile Before this change: Test qcms internal sRGB colorant matrix against official sRGB IEC61966-2.1 color (D50) primaries -14 11 -4 -8 10 -2 0 3 -17 Total error = 0x45 [0.001053], and an RMS white point error of 0.000031 from the ICC spec D50. Update the XYZ primaries used to construct the internal sRGB color profile, found after iterating through many different sources of sRGB XYZ primaries, refer to spreadsheet added to the bug. Updated qcms_test_internal_srgb to check if internal profile is bogus. Also display media white point and the difference between reference D65 and actual XYZ values: Test media white point against expected sRGB IEC61966-2.1 D65 XYZ values: Internal values = [0xF351, 0x10000, 0x116CC] White point error = [0, 0, 0] Also test the's profile white point: compute the profile white point in xyY and compare to the ICC spec D50 in xyY. Test PCS white point error Profile White point test: xyY [ 0.345703 0.358538.6 1.000000 ] White point error: 0.000001 BUG=580917 ==========
Description was changed from ========== [qcms] Update primaries used to build internal sRGB profile Before this change: Test qcms internal sRGB colorant matrix against official sRGB IEC61966-2.1 color (D50) primaries -14 11 -4 -8 10 -2 0 3 -17 Total error = 0x45 [0.001053], and an RMS white point error of 0.000031 from the ICC spec D50. Update the XYZ primaries used to construct the internal sRGB color profile, found after iterating through many different sources of sRGB XYZ primaries, refer to spreadsheet added to the bug. Updated qcms_test_internal_srgb to check if internal profile is bogus. Also display media white point and the difference between reference D65 and actual XYZ values: Test media white point against expected sRGB IEC61966-2.1 D65 XYZ values: Internal values = [0xF351, 0x10000, 0x116CC] White point error = [0, 0, 0] Also test the's profile white point: compute the profile white point in xyY and compare to the ICC spec D50 in xyY. Test PCS white point error Profile White point test: xyY [ 0.345703 0.358538.6 1.000000 ] White point error: 0.000001 BUG=580917 ========== to ========== [qcms] Update primaries used to build internal sRGB profile Before this change: Test qcms internal sRGB colorant matrix against official sRGB IEC61966-2.1 color (D50) primaries -14 11 -4 -8 10 -2 0 3 -17 Total error = 0x45 [0.001053], and an RMS white point error of 0.000031 from the ICC spec D50. Update the XYZ primaries used to construct the internal sRGB color profile, found after iterating through many different sources of sRGB XYZ primaries, refer to spreadsheet added to the bug for details. Updated qcms_test_internal_srgb to check if internal profile is bogus. Also display media white point and the difference (error) from expected D65 XYZ values: Test media white point against expected D65 XYZ values: Internal values = [0xF351, 0x10000, 0x116CC] White point error = [0, 0, 0] Also test the profile white point: compute the profile white point in xyY and compare to the ICC spec D50 in xyY. Test PCS white point error Profile White point test: xyY [ 0.345703 0.358538.6 1.000000 ] White point error: 0.000001 BUG=580917 ==========
https://codereview.chromium.org/1779163002/diff/300001/third_party/qcms/READM... File third_party/qcms/README.chromium (right): https://codereview.chromium.org/1779163002/diff/300001/third_party/qcms/READM... third_party/qcms/README.chromium:146: - Built internal sRGB profile from official D50 colorants and white point Make this match the change description. https://codereview.chromium.org/1779163002/diff/300001/third_party/qcms/src/t... File third_party/qcms/src/tests/qcms_test_internal_srgb.c (right): https://codereview.chromium.org/1779163002/diff/300001/third_party/qcms/src/t... third_party/qcms/src/tests/qcms_test_internal_srgb.c:44: printf("Profile White point test: xyY [ %.6f %.6f %.6f ]\n", x, y, Y); "Profile" -> "Profile D50" https://codereview.chromium.org/1779163002/diff/300001/third_party/qcms/src/t... third_party/qcms/src/tests/qcms_test_internal_srgb.c:95: printf("Test media white point against expected sRGB IEC61966-2.1 D65 XYZ values\n"); "sRGB IEC61966-2.1 D65" -> "D65" https://codereview.chromium.org/1779163002/diff/300001/third_party/qcms/src/t... third_party/qcms/src/tests/qcms_test_internal_srgb.c:103: Better: and perhaps all this printf-ing should be in the helper too? Then we might find a better name for the helper routine.
The CQ bit was checked by noel@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1779163002/320001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1779163002/320001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== [qcms] Update primaries used to build internal sRGB profile Before this change: Test qcms internal sRGB colorant matrix against official sRGB IEC61966-2.1 color (D50) primaries -14 11 -4 -8 10 -2 0 3 -17 Total error = 0x45 [0.001053], and an RMS white point error of 0.000031 from the ICC spec D50. Update the XYZ primaries used to construct the internal sRGB color profile, found after iterating through many different sources of sRGB XYZ primaries, refer to spreadsheet added to the bug for details. Updated qcms_test_internal_srgb to check if internal profile is bogus. Also display media white point and the difference (error) from expected D65 XYZ values: Test media white point against expected D65 XYZ values: Internal values = [0xF351, 0x10000, 0x116CC] White point error = [0, 0, 0] Also test the profile white point: compute the profile white point in xyY and compare to the ICC spec D50 in xyY. Test PCS white point error Profile White point test: xyY [ 0.345703 0.358538.6 1.000000 ] White point error: 0.000001 BUG=580917 ========== to ========== [qcms] Update primaries used to build internal sRGB profile Before this change: Test qcms internal sRGB primaries against official sRGB IEC61966-2.1 profile (D50) primaries -14 11 -4 -8 10 -2 0 3 -17 Total error = 0x45 [0.001053], and an RMS white point error of 0.000031 compared to the ICC spec PCS D50 white point. Update the XYZ primaries used to construct the internal sRGB profile, found after iterating through many different sources of the sRGB XYZ primaries (refer to spreadsheet added to the bug for details). Update qcms_test_internal_srgb to check if the internal sRGB profile is bogus. Also display it's media white point and the difference (error) from the expected D65 XYZ: Test media white point against expected D65 XYZ values: Internal values = [0xF351, 0x10000, 0x116CC] White point error = [0, 0, 0] Also test the internal sRGB profile PCS white point: compute the profile white point in xyY, then compare to the ICC spec PCS D50 in xyY: Test PCS white point error Profile White point test: xyY [ 0.345703 0.358538 1.000 ] White point error: 0.000001 BUG=580917 ==========
https://codereview.chromium.org/1779163002/diff/320001/third_party/qcms/src/t... File third_party/qcms/src/tests/qcms_test_internal_srgb.c (right): https://codereview.chromium.org/1779163002/diff/320001/third_party/qcms/src/t... third_party/qcms/src/tests/qcms_test_internal_srgb.c:72: printf("Test qcms internal sRGB color primaries against sRGB IEC61966-2.1 color primaries\n"); Also since you check 3 different things now (primaries, media white point, and pcs white point), then perhaps having three helper functions would be better? Maybe like so ... check_profile_primaries(profile); check_profile_media_white_point(profile); check_profile_pcs_white_point(profile); which might make the test a lot easier to read.
https://codereview.chromium.org/1779163002/diff/320001/third_party/qcms/src/t... File third_party/qcms/src/tests/qcms_test_internal_srgb.c (right): https://codereview.chromium.org/1779163002/diff/320001/third_party/qcms/src/t... third_party/qcms/src/tests/qcms_test_internal_srgb.c:72: printf("Test qcms internal sRGB color primaries against sRGB IEC61966-2.1 color primaries\n"); On 2016/03/26 03:53:40, noel gordon wrote: > Also since you check 3 different things now (primaries, media white point, and > pcs white point), then perhaps having three helper functions would be better? > Maybe like so ... > > check_profile_primaries(profile); > check_profile_media_white_point(profile); > check_profile_pcs_white_point(profile); > > which might make the test a lot easier to read. Done.
On 2016/03/25 10:21:57, noel gordon wrote: > https://codereview.chromium.org/1779163002/diff/300001/third_party/qcms/src/t... > third_party/qcms/src/tests/qcms_test_internal_srgb.c:44: printf("Profile White > point test: xyY [ %.6f %.6f %.6f ]\n", x, y, Y); > "Profile" -> "Profile D50" Still todo. > https://codereview.chromium.org/1779163002/diff/300001/third_party/qcms/src/t... > third_party/qcms/src/tests/qcms_test_internal_srgb.c:95: printf("Test media > white point against expected sRGB IEC61966-2.1 D65 XYZ values\n"); > "sRGB IEC61966-2.1 D65" -> "D65" Still todo.
... plus other final nits. Please fix them all and go ahead and submit. https://codereview.chromium.org/1779163002/diff/340001/third_party/qcms/src/t... File third_party/qcms/src/tests/qcms_test_internal_srgb.c (right): https://codereview.chromium.org/1779163002/diff/340001/third_party/qcms/src/t... third_party/qcms/src/tests/qcms_test_internal_srgb.c:111: nit: delete this empty line. https://codereview.chromium.org/1779163002/diff/340001/third_party/qcms/src/t... third_party/qcms/src/tests/qcms_test_internal_srgb.c:113: Ditto. https://codereview.chromium.org/1779163002/diff/340001/third_party/qcms/src/t... third_party/qcms/src/tests/qcms_test_internal_srgb.c:118: Ditto. https://codereview.chromium.org/1779163002/diff/340001/third_party/qcms/src/t... third_party/qcms/src/tests/qcms_test_internal_srgb.c:120: Ditto. https://codereview.chromium.org/1779163002/diff/340001/third_party/qcms/src/t... third_party/qcms/src/tests/qcms_test_internal_srgb.c:123: Ditto.
https://codereview.chromium.org/1779163002/diff/300001/third_party/qcms/src/t... File third_party/qcms/src/tests/qcms_test_internal_srgb.c (right): https://codereview.chromium.org/1779163002/diff/300001/third_party/qcms/src/t... third_party/qcms/src/tests/qcms_test_internal_srgb.c:44: printf("Profile White point test: xyY [ %.6f %.6f %.6f ]\n", x, y, Y); On 2016/03/25 10:21:57, noel gordon wrote: > "Profile" -> "Profile D50" Done. https://codereview.chromium.org/1779163002/diff/300001/third_party/qcms/src/t... third_party/qcms/src/tests/qcms_test_internal_srgb.c:95: printf("Test media white point against expected sRGB IEC61966-2.1 D65 XYZ values\n"); On 2016/03/25 10:21:57, noel gordon wrote: > "sRGB IEC61966-2.1 D65" -> "D65" Done. https://codereview.chromium.org/1779163002/diff/300001/third_party/qcms/src/t... third_party/qcms/src/tests/qcms_test_internal_srgb.c:103: On 2016/03/25 10:21:57, noel gordon wrote: > Better: and perhaps all this printf-ing should be in the helper too? Then we > might find a better name for the helper routine. I've moved most of the printfs inside their related function. I've also added some comments before each helper. Done. https://codereview.chromium.org/1779163002/diff/340001/third_party/qcms/src/t... File third_party/qcms/src/tests/qcms_test_internal_srgb.c (right): https://codereview.chromium.org/1779163002/diff/340001/third_party/qcms/src/t... third_party/qcms/src/tests/qcms_test_internal_srgb.c:111: On 2016/03/28 20:55:56, noel gordon wrote: > nit: delete this empty line. Done. https://codereview.chromium.org/1779163002/diff/340001/third_party/qcms/src/t... third_party/qcms/src/tests/qcms_test_internal_srgb.c:113: On 2016/03/28 20:55:56, noel gordon wrote: > Ditto. Done. https://codereview.chromium.org/1779163002/diff/340001/third_party/qcms/src/t... third_party/qcms/src/tests/qcms_test_internal_srgb.c:118: On 2016/03/28 20:55:56, noel gordon wrote: > Ditto. Done. https://codereview.chromium.org/1779163002/diff/340001/third_party/qcms/src/t... third_party/qcms/src/tests/qcms_test_internal_srgb.c:120: On 2016/03/28 20:55:56, noel gordon wrote: > Ditto. Done. https://codereview.chromium.org/1779163002/diff/340001/third_party/qcms/src/t... third_party/qcms/src/tests/qcms_test_internal_srgb.c:123: On 2016/03/28 20:55:56, noel gordon wrote: > Ditto. Done.
The CQ bit was checked by radu.velea@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1779163002/360001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1779163002/360001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by radu.velea@intel.com
The patchset sent to the CQ was uploaded after l-g-t-m from noel@chromium.org Link to the patchset: https://codereview.chromium.org/1779163002/#ps360001 (title: "Moved printfs, added comments, removed newlines")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1779163002/360001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1779163002/360001
Message was sent while issue was closed.
Description was changed from ========== [qcms] Update primaries used to build internal sRGB profile Before this change: Test qcms internal sRGB primaries against official sRGB IEC61966-2.1 profile (D50) primaries -14 11 -4 -8 10 -2 0 3 -17 Total error = 0x45 [0.001053], and an RMS white point error of 0.000031 compared to the ICC spec PCS D50 white point. Update the XYZ primaries used to construct the internal sRGB profile, found after iterating through many different sources of the sRGB XYZ primaries (refer to spreadsheet added to the bug for details). Update qcms_test_internal_srgb to check if the internal sRGB profile is bogus. Also display it's media white point and the difference (error) from the expected D65 XYZ: Test media white point against expected D65 XYZ values: Internal values = [0xF351, 0x10000, 0x116CC] White point error = [0, 0, 0] Also test the internal sRGB profile PCS white point: compute the profile white point in xyY, then compare to the ICC spec PCS D50 in xyY: Test PCS white point error Profile White point test: xyY [ 0.345703 0.358538 1.000 ] White point error: 0.000001 BUG=580917 ========== to ========== [qcms] Update primaries used to build internal sRGB profile Before this change: Test qcms internal sRGB primaries against official sRGB IEC61966-2.1 profile (D50) primaries -14 11 -4 -8 10 -2 0 3 -17 Total error = 0x45 [0.001053], and an RMS white point error of 0.000031 compared to the ICC spec PCS D50 white point. Update the XYZ primaries used to construct the internal sRGB profile, found after iterating through many different sources of the sRGB XYZ primaries (refer to spreadsheet added to the bug for details). Update qcms_test_internal_srgb to check if the internal sRGB profile is bogus. Also display it's media white point and the difference (error) from the expected D65 XYZ: Test media white point against expected D65 XYZ values: Internal values = [0xF351, 0x10000, 0x116CC] White point error = [0, 0, 0] Also test the internal sRGB profile PCS white point: compute the profile white point in xyY, then compare to the ICC spec PCS D50 in xyY: Test PCS white point error Profile White point test: xyY [ 0.345703 0.358538 1.000 ] White point error: 0.000001 BUG=580917 ==========
Message was sent while issue was closed.
Committed patchset #19 (id:360001)
Message was sent while issue was closed.
Description was changed from ========== [qcms] Update primaries used to build internal sRGB profile Before this change: Test qcms internal sRGB primaries against official sRGB IEC61966-2.1 profile (D50) primaries -14 11 -4 -8 10 -2 0 3 -17 Total error = 0x45 [0.001053], and an RMS white point error of 0.000031 compared to the ICC spec PCS D50 white point. Update the XYZ primaries used to construct the internal sRGB profile, found after iterating through many different sources of the sRGB XYZ primaries (refer to spreadsheet added to the bug for details). Update qcms_test_internal_srgb to check if the internal sRGB profile is bogus. Also display it's media white point and the difference (error) from the expected D65 XYZ: Test media white point against expected D65 XYZ values: Internal values = [0xF351, 0x10000, 0x116CC] White point error = [0, 0, 0] Also test the internal sRGB profile PCS white point: compute the profile white point in xyY, then compare to the ICC spec PCS D50 in xyY: Test PCS white point error Profile White point test: xyY [ 0.345703 0.358538 1.000 ] White point error: 0.000001 BUG=580917 ========== to ========== [qcms] Update primaries used to build internal sRGB profile Before this change: Test qcms internal sRGB primaries against official sRGB IEC61966-2.1 profile (D50) primaries -14 11 -4 -8 10 -2 0 3 -17 Total error = 0x45 [0.001053], and an RMS white point error of 0.000031 compared to the ICC spec PCS D50 white point. Update the XYZ primaries used to construct the internal sRGB profile, found after iterating through many different sources of the sRGB XYZ primaries (refer to spreadsheet added to the bug for details). Update qcms_test_internal_srgb to check if the internal sRGB profile is bogus. Also display it's media white point and the difference (error) from the expected D65 XYZ: Test media white point against expected D65 XYZ values: Internal values = [0xF351, 0x10000, 0x116CC] White point error = [0, 0, 0] Also test the internal sRGB profile PCS white point: compute the profile white point in xyY, then compare to the ICC spec PCS D50 in xyY: Test PCS white point error Profile White point test: xyY [ 0.345703 0.358538 1.000 ] White point error: 0.000001 BUG=580917 Committed: https://crrev.com/4371edbc78a477a0c13947adea0137b7aed86b29 Cr-Commit-Position: refs/heads/master@{#383696} ==========
Message was sent while issue was closed.
Patchset 19 (id:??) landed as https://crrev.com/4371edbc78a477a0c13947adea0137b7aed86b29 Cr-Commit-Position: refs/heads/master@{#383696} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
