Expand QCMS tests. Add Munsell test for transform accuracy

Expand QCMS tests. Add Munsell test for transform accuracy

Add a test driver (re-named to qcms_tests) now there are two tests, and
update the build targets (gyp/gn/Makefile) to match.

Put the tests into separate files, add qcms_test_main.c to handle which
test to run and options, add new options to control the tests.

Brief usage:
   qcms_tests -w WIDTH -h HEIGHT -n ITERATIONS -t TEST

Options are:
     -w INT     test image width
     -h INT     test image height
     -n INT     number of iterations for each test
     -a         run all tests
     -l         list available tests
     -s         force software(non-sse) transform function, where available
     -i STRING  specify input icc color profile
     -o STRING  specify output icc color profile
     -t STRING  run specific test: use -l to list possible values

Tests are:
   src/tests/qcms_test_tetra_clut_rgba.c
     - test to compare tetra SSE with the software tetra code, for
       speed and that the color transform results for random input
       data (the source test image) match.

   src/tests/qcms_test_munsell.c
     - test color transforms of Munsell RGB colors from a known in
       color space (AdobeRGB1998, or sRGB) to the out color space,
       and report color accuracy (RMS color error).

BUG=532910
Committed: https://crrev.com/daae8f2139bd5681ff0bfbf620a86b645934cd34
Cr-Commit-Position: refs/heads/master@{#353255}

[radu.velea, 2015-09-29 14:52:17]

radu.velea@intel.com changed reviewers:
+ noel@chromium.org, robert.bradford@intel.com

[radu.velea, 2015-09-29 14:53:26]

On 2015/09/29 14:52:17, radu.velea wrote:
> mailto:radu.velea@intel.com changed reviewers:
> + mailto:noel@chromium.org, mailto:robert.bradford@intel.com

Hello Noel,

I created a new patch set with the Munsell test.
It generates a timestamp-ed csv file with some useful 
statistics. Let me know what you think.

Thanks,

Radu

[Noel Gordon, 2015-10-07 12:26:31]

https://codereview.chromium.org/1374953003/diff/100001/third_party/qcms/BUILD.gn
File third_party/qcms/BUILD.gn (left):

https://codereview.chromium.org/1374953003/diff/100001/third_party/qcms/BUILD...
third_party/qcms/BUILD.gn:43: executable("qcms_test") {
qcms_test -> qcms_tests

https://codereview.chromium.org/1374953003/diff/100001/third_party/qcms/qcms.gyp
File third_party/qcms/qcms.gyp (right):

https://codereview.chromium.org/1374953003/diff/100001/third_party/qcms/qcms....
third_party/qcms/qcms.gyp:51: 'target_name': 'qcms_test',
qcms_test -> qcms_tests

https://codereview.chromium.org/1374953003/diff/100001/third_party/qcms/src/t...
File third_party/qcms/src/tests/qcms_test_main.c (right):

https://codereview.chromium.org/1374953003/diff/100001/third_party/qcms/src/t...
third_party/qcms/src/tests/qcms_test_main.c:14: // Manually update the items
bellow to add more tests.
bellow -> below.

https://codereview.chromium.org/1374953003/diff/100001/third_party/qcms/src/t...
third_party/qcms/src/tests/qcms_test_main.c:19: #define TEST_SIZE   
(sizeof(qcms_test) / sizeof(qcms_test[0]))
TEST_SIZE -> TEST_CASES

https://codereview.chromium.org/1374953003/diff/100001/third_party/qcms/src/t...
third_party/qcms/src/tests/qcms_test_main.c:21: static void init_tests()
initialize_tests

https://codereview.chromium.org/1374953003/diff/100001/third_party/qcms/src/t...
third_party/qcms/src/tests/qcms_test_main.c:41: printf("Usage:\n\tqcms_test -w
WIDTH -h HEIGHT -n ITERATIONS -t TEST\n");
Could you make this usage match that given in the change description please?

https://codereview.chromium.org/1374953003/diff/100001/third_party/qcms/src/t...
third_party/qcms/src/tests/qcms_test_main.c:58: for (i = 0; i < TEST_SIZE; ++i)
{
space before this "for" line.

https://codereview.chromium.org/1374953003/diff/100001/third_party/qcms/src/t...
third_party/qcms/src/tests/qcms_test_main.c:60: qcms_test[i].status =
QCMS_TEST_ENABLED;
Perhaps count the number of tests enabled here ...

https://codereview.chromium.org/1374953003/diff/100001/third_party/qcms/src/t...
third_party/qcms/src/tests/qcms_test_main.c:62: }
... and if no tests were enabled, should you maybe call list_tests()?

https://codereview.chromium.org/1374953003/diff/100001/third_party/qcms/src/t...
third_party/qcms/src/tests/qcms_test_main.c:82: int force_software = 0;
int force_software = 0;
int i;

initialize_tests();

https://codereview.chromium.org/1374953003/diff/100001/third_party/qcms/src/t...
third_party/qcms/src/tests/qcms_test_main.c:112: 
Define |int exit_status| in the usual place, and initialize it here.

exit_status = 0;

https://codereview.chromium.org/1374953003/diff/100001/third_party/qcms/src/t...
third_party/qcms/src/tests/qcms_test_main.c:115: qcms_test[i].test_fn(width,
height, iterations, in, out, force_software);
exit_status += qcms_test[i].test_fn(...

https://codereview.chromium.org/1374953003/diff/100001/third_party/qcms/src/t...
third_party/qcms/src/tests/qcms_test_main.c:118: return EXIT_SUCCESS;
return exit_status;

[radu.velea, 2015-10-07 13:22:36]

https://codereview.chromium.org/1374953003/diff/100001/third_party/qcms/BUILD.gn
File third_party/qcms/BUILD.gn (left):

https://codereview.chromium.org/1374953003/diff/100001/third_party/qcms/BUILD...
third_party/qcms/BUILD.gn:43: executable("qcms_test") {
On 2015/10/07 12:26:31, noel gordon wrote:
> qcms_test -> qcms_tests

Done.

https://codereview.chromium.org/1374953003/diff/100001/third_party/qcms/qcms.gyp
File third_party/qcms/qcms.gyp (right):

https://codereview.chromium.org/1374953003/diff/100001/third_party/qcms/qcms....
third_party/qcms/qcms.gyp:51: 'target_name': 'qcms_test',
On 2015/10/07 12:26:31, noel gordon wrote:
> qcms_test -> qcms_tests

Done.

https://codereview.chromium.org/1374953003/diff/100001/third_party/qcms/src/t...
File third_party/qcms/src/tests/qcms_test_main.c (right):

https://codereview.chromium.org/1374953003/diff/100001/third_party/qcms/src/t...
third_party/qcms/src/tests/qcms_test_main.c:14: // Manually update the items
bellow to add more tests.
On 2015/10/07 12:26:31, noel gordon wrote:
> bellow -> below.

Done.

https://codereview.chromium.org/1374953003/diff/100001/third_party/qcms/src/t...
third_party/qcms/src/tests/qcms_test_main.c:19: #define TEST_SIZE   
(sizeof(qcms_test) / sizeof(qcms_test[0]))
On 2015/10/07 12:26:31, noel gordon wrote:
> TEST_SIZE -> TEST_CASES

Done.

https://codereview.chromium.org/1374953003/diff/100001/third_party/qcms/src/t...
third_party/qcms/src/tests/qcms_test_main.c:21: static void init_tests()
On 2015/10/07 12:26:31, noel gordon wrote:
> initialize_tests

Done.

https://codereview.chromium.org/1374953003/diff/100001/third_party/qcms/src/t...
third_party/qcms/src/tests/qcms_test_main.c:41: printf("Usage:\n\tqcms_test -w
WIDTH -h HEIGHT -n ITERATIONS -t TEST\n");
On 2015/10/07 12:26:31, noel gordon wrote:
> Could you make this usage match that given in the change description please?

Done.

https://codereview.chromium.org/1374953003/diff/100001/third_party/qcms/src/t...
third_party/qcms/src/tests/qcms_test_main.c:58: for (i = 0; i < TEST_SIZE; ++i)
{
On 2015/10/07 12:26:31, noel gordon wrote:
> space before this "for" line.

Done.

https://codereview.chromium.org/1374953003/diff/100001/third_party/qcms/src/t...
third_party/qcms/src/tests/qcms_test_main.c:60: qcms_test[i].status =
QCMS_TEST_ENABLED;
On 2015/10/07 12:26:31, noel gordon wrote:
> Perhaps count the number of tests enabled here ...

Done.

https://codereview.chromium.org/1374953003/diff/100001/third_party/qcms/src/t...
third_party/qcms/src/tests/qcms_test_main.c:62: }
On 2015/10/07 12:26:31, noel gordon wrote:
> ... and if no tests were enabled, should you maybe call list_tests()?

Done.

https://codereview.chromium.org/1374953003/diff/100001/third_party/qcms/src/t...
third_party/qcms/src/tests/qcms_test_main.c:82: int force_software = 0;
On 2015/10/07 12:26:31, noel gordon wrote:
> int force_software = 0;
> int i;
> 
> initialize_tests();

Done.

https://codereview.chromium.org/1374953003/diff/100001/third_party/qcms/src/t...
third_party/qcms/src/tests/qcms_test_main.c:112: 
On 2015/10/07 12:26:31, noel gordon wrote:
> Define |int exit_status| in the usual place, and initialize it here.
> 
> exit_status = 0;

Done.

https://codereview.chromium.org/1374953003/diff/100001/third_party/qcms/src/t...
third_party/qcms/src/tests/qcms_test_main.c:115: qcms_test[i].test_fn(width,
height, iterations, in, out, force_software);
On 2015/10/07 12:26:31, noel gordon wrote:
> exit_status += qcms_test[i].test_fn(...

Done.

https://codereview.chromium.org/1374953003/diff/100001/third_party/qcms/src/t...
third_party/qcms/src/tests/qcms_test_main.c:118: return EXIT_SUCCESS;
On 2015/10/07 12:26:31, noel gordon wrote:
> return exit_status;

Done.

[Noel Gordon, 2015-10-07 14:03:07]

https://codereview.chromium.org/1374953003/diff/140001/third_party/qcms/src/t...
File third_party/qcms/src/tests/qcms_test_main.c (right):

https://codereview.chromium.org/1374953003/diff/140001/third_party/qcms/src/t...
third_party/qcms/src/tests/qcms_test_main.c:42: printf("\t-w
INT\t\tauto-generated image width\n");
auto-generated?  No clue what that means. Is this the "test image width"?

https://codereview.chromium.org/1374953003/diff/140001/third_party/qcms/src/t...
third_party/qcms/src/tests/qcms_test_main.c:44: printf("\t-n INT\t\tnumber of
iterations to run the test(s) on\n");
on? on what? Perhaps you want "number of iterations for each test" or some such?

https://codereview.chromium.org/1374953003/diff/140001/third_party/qcms/src/t...
third_party/qcms/src/tests/qcms_test_main.c:119: if (!run_all && enabled_tests
== 0) {
 if (!run_all && !enabled_tests) ?

https://codereview.chromium.org/1374953003/diff/140001/third_party/qcms/src/t...
File third_party/qcms/src/tests/qcms_test_tetra_clut_rgba.c (right):

https://codereview.chromium.org/1374953003/diff/140001/third_party/qcms/src/t...
third_party/qcms/src/tests/qcms_test_tetra_clut_rgba.c:41: static int
validate(unsigned char *dst0, unsigned char *dst1, size_t length, int limit,
const size_t pixel_size)
Space before this line.

[radu.velea, 2015-10-07 14:43:43]

https://codereview.chromium.org/1374953003/diff/140001/third_party/qcms/src/t...
File third_party/qcms/src/tests/qcms_test_main.c (right):

https://codereview.chromium.org/1374953003/diff/140001/third_party/qcms/src/t...
third_party/qcms/src/tests/qcms_test_main.c:42: printf("\t-w
INT\t\tauto-generated image width\n");
On 2015/10/07 14:03:06, noel gordon wrote:
> auto-generated?  No clue what that means. Is this the "test image width"?

Yes.
And the image is auto-generated. I'll rephrase it to avoid confusion.

https://codereview.chromium.org/1374953003/diff/140001/third_party/qcms/src/t...
third_party/qcms/src/tests/qcms_test_main.c:44: printf("\t-n INT\t\tnumber of
iterations to run the test(s) on\n");
On 2015/10/07 14:03:06, noel gordon wrote:
> on? on what? Perhaps you want "number of iterations for each test" or some
such?

Done.

https://codereview.chromium.org/1374953003/diff/140001/third_party/qcms/src/t...
third_party/qcms/src/tests/qcms_test_main.c:119: if (!run_all && enabled_tests
== 0) {
On 2015/10/07 14:03:06, noel gordon wrote:
>  if (!run_all && !enabled_tests) ?

Done.

https://codereview.chromium.org/1374953003/diff/140001/third_party/qcms/src/t...
File third_party/qcms/src/tests/qcms_test_tetra_clut_rgba.c (right):

https://codereview.chromium.org/1374953003/diff/140001/third_party/qcms/src/t...
third_party/qcms/src/tests/qcms_test_tetra_clut_rgba.c:41: static int
validate(unsigned char *dst0, unsigned char *dst1, size_t length, int limit,
const size_t pixel_size)
On 2015/10/07 14:03:06, noel gordon wrote:
> Space before this line.

Done.

[Noel Gordon, 2015-10-08 03:12:49]

https://codereview.chromium.org/1374953003/diff/160001/third_party/qcms/src/t...
File third_party/qcms/src/tests/qcms_test_main.c (right):

https://codereview.chromium.org/1374953003/diff/160001/third_party/qcms/src/t...
third_party/qcms/src/tests/qcms_test_main.c:30: printf("Available QCMS
tests:\n");
QCMS -> qcms

https://codereview.chromium.org/1374953003/diff/160001/third_party/qcms/src/t...
third_party/qcms/src/tests/qcms_test_main.c:59: for (i = 0; i < TEST_CASES; ++i)
{
% ./out/Release/qcms_tests -t
Segmentation fault

if (!args)
    return 0;

for (i ...

https://codereview.chromium.org/1374953003/diff/160001/third_party/qcms/src/t...
third_party/qcms/src/tests/qcms_test_main.c:107: enabled_tests +=
enable_test(argv[2]);
Use spaces (not tabs) here. (git apply complains).

https://codereview.chromium.org/1374953003/diff/160001/third_party/qcms/src/t...
third_party/qcms/src/tests/qcms_test_main.c:120: print_usage();
Use spaces (not tabs) here. (git apply complains).

https://codereview.chromium.org/1374953003/diff/160001/third_party/qcms/src/t...
third_party/qcms/src/tests/qcms_test_main.c:127: exit_status +=
qcms_test[i].test_fn(width, height, iterations, in, out, force_software);
Use spaces (not tabs) here. (git apply complains).

https://codereview.chromium.org/1374953003/diff/160001/third_party/qcms/src/t...
File third_party/qcms/src/tests/qcms_test_munsell.c (right):

https://codereview.chromium.org/1374953003/diff/160001/third_party/qcms/src/t...
third_party/qcms/src/tests/qcms_test_munsell.c:103: printf("Test qcms data
transforms accuracy\n");
printf("Test qcms data transforms accuracy using Munsell colors\n");

https://codereview.chromium.org/1374953003/diff/160001/third_party/qcms/src/t...
third_party/qcms/src/tests/qcms_test_munsell.c:104: printf("Test image size 1 x
1 pixels (using Munsell colors)\n");
Delete this printf.

https://codereview.chromium.org/1374953003/diff/160001/third_party/qcms/src/t...
third_party/qcms/src/tests/qcms_test_munsell.c:139: transform =
qcms_transform_create(in_profile, QCMS_DATA_RGBA_8, out_profile,
QCMS_DATA_RGBA_8, QCMS_INTENT_DEFAULT);
Check if the transform is NULL and bail with an error message if so.

https://codereview.chromium.org/1374953003/diff/160001/third_party/qcms/src/t...
third_party/qcms/src/tests/qcms_test_munsell.c:143: for (i = 0; i <
(sizeof(adobe_munsell) / sizeof(adobe_munsell[0])); i++) {
(sizeof(adobe_munsell) / sizeof(adobe_munsell[0]) -> 24.  There are only 24
Munsell colors.

https://codereview.chromium.org/1374953003/diff/160001/third_party/qcms/src/t...
third_party/qcms/src/tests/qcms_test_munsell.c:149: results[i] = sqrt(r_diff *
r_diff + g_diff * g_diff + b_diff * b_diff);
results[i] is not used other than in this loop.  Seems results[i] could be made
a local float variable (an array is needed).

https://codereview.chromium.org/1374953003/diff/160001/third_party/qcms/src/t...
third_party/qcms/src/tests/qcms_test_munsell.c:153: if (rmse != 0) {
remove the if, always write the RMS error.

https://codereview.chromium.org/1374953003/diff/160001/third_party/qcms/src/t...
third_party/qcms/src/tests/qcms_test_munsell.c:154: printf("Transform RMS
%.3f\n", rmse / 24);
rmse / 24 is computed multiple times below.  Why not just do:

   rmse = rmse / 24.0;

to compute it once, then use |rmse| instead of |rmse / 24| in the remaining
code?

https://codereview.chromium.org/1374953003/diff/160001/third_party/qcms/src/t...
third_party/qcms/src/tests/qcms_test_munsell.c:158: sprintf(file_name,
"qcms_test_munsell_%lu_RMS_%.3f.csv", time(NULL), rmse / 24);
Use "...munsell_%ld..." and cast the time(NULL) argument to that type: (long
int) time(NULL)

https://codereview.chromium.org/1374953003/diff/160001/third_party/qcms/src/t...
third_party/qcms/src/tests/qcms_test_munsell.c:160: // Print header.
Space before this line.

https://codereview.chromium.org/1374953003/diff/160001/third_party/qcms/src/t...
third_party/qcms/src/tests/qcms_test_munsell.c:166: fprintf(output, "color
name,Input %s[R],Input %s[G],Input %s[B],"
Remove the input color "Input %s[R],Input %s[G],Input %s[B]," from this header:
just print out the output and reference color results.

https://codereview.chromium.org/1374953003/diff/160001/third_party/qcms/src/t...
third_party/qcms/src/tests/qcms_test_munsell.c:172: // Print results.
Space before this line.

https://codereview.chromium.org/1374953003/diff/160001/third_party/qcms/src/t...
third_party/qcms/src/tests/qcms_test_munsell.c:173: for (i = 0; i <
(sizeof(adobe_munsell) / sizeof(adobe_munsell[0])); i++) {
(sizeof(adobe_munsell) / sizeof(adobe_munsell[0]) -> 24.  There are only 24
Munsell colors.

https://codereview.chromium.org/1374953003/diff/160001/third_party/qcms/src/t...
File third_party/qcms/src/tests/qcms_test_util.h (right):

https://codereview.chromium.org/1374953003/diff/160001/third_party/qcms/src/t...
third_party/qcms/src/tests/qcms_test_util.h:4: 
Globally use spaces instead of tabs in this file.

[Noel Gordon, 2015-10-08 03:14:59]

Also noticed, src/test/Makefile is not part of this CL.  It will need to be
updated to compile the new test harness, right?

[Noel Gordon, 2015-10-08 06:01:50]

https://codereview.chromium.org/1374953003/diff/160001/third_party/qcms/src/t...
third_party/qcms/src/tests/qcms_test_munsell.c:149: results[i] = sqrt(r_diff *
r_diff + g_diff * g_diff + b_diff * b_diff);

> results[i] is not used other than in this loop.  Seems results[i] could be
made
a local float variable (an array is needed).

Ahem, meant an array is _not_ needed, right?

[radu.velea, 2015-10-08 09:39:13]

https://codereview.chromium.org/1374953003/diff/160001/third_party/qcms/src/t...
File third_party/qcms/src/tests/qcms_test_main.c (right):

https://codereview.chromium.org/1374953003/diff/160001/third_party/qcms/src/t...
third_party/qcms/src/tests/qcms_test_main.c:30: printf("Available QCMS
tests:\n");
On 2015/10/08 03:12:49, noel gordon wrote:
> QCMS -> qcms

Done.

https://codereview.chromium.org/1374953003/diff/160001/third_party/qcms/src/t...
third_party/qcms/src/tests/qcms_test_main.c:59: for (i = 0; i < TEST_CASES; ++i)
{
On 2015/10/08 03:12:49, noel gordon wrote:
> % ./out/Release/qcms_tests -t
> Segmentation fault
> 
> if (!args)
>     return 0;
> 
> for (i ...

Done.

https://codereview.chromium.org/1374953003/diff/160001/third_party/qcms/src/t...
third_party/qcms/src/tests/qcms_test_main.c:107: enabled_tests +=
enable_test(argv[2]);
On 2015/10/08 03:12:49, noel gordon wrote:
> Use spaces (not tabs) here. (git apply complains).

Done.

https://codereview.chromium.org/1374953003/diff/160001/third_party/qcms/src/t...
third_party/qcms/src/tests/qcms_test_main.c:120: print_usage();
On 2015/10/08 03:12:49, noel gordon wrote:
> Use spaces (not tabs) here. (git apply complains).

Done.

https://codereview.chromium.org/1374953003/diff/160001/third_party/qcms/src/t...
third_party/qcms/src/tests/qcms_test_main.c:127: exit_status +=
qcms_test[i].test_fn(width, height, iterations, in, out, force_software);
On 2015/10/08 03:12:49, noel gordon wrote:
> Use spaces (not tabs) here. (git apply complains).

Done.

https://codereview.chromium.org/1374953003/diff/160001/third_party/qcms/src/t...
File third_party/qcms/src/tests/qcms_test_munsell.c (right):

https://codereview.chromium.org/1374953003/diff/160001/third_party/qcms/src/t...
third_party/qcms/src/tests/qcms_test_munsell.c:103: printf("Test qcms data
transforms accuracy\n");
On 2015/10/08 03:12:49, noel gordon wrote:
> printf("Test qcms data transforms accuracy using Munsell colors\n");

Done.

https://codereview.chromium.org/1374953003/diff/160001/third_party/qcms/src/t...
third_party/qcms/src/tests/qcms_test_munsell.c:104: printf("Test image size 1 x
1 pixels (using Munsell colors)\n");
On 2015/10/08 03:12:49, noel gordon wrote:
> Delete this printf.

Done.

https://codereview.chromium.org/1374953003/diff/160001/third_party/qcms/src/t...
third_party/qcms/src/tests/qcms_test_munsell.c:139: transform =
qcms_transform_create(in_profile, QCMS_DATA_RGBA_8, out_profile,
QCMS_DATA_RGBA_8, QCMS_INTENT_DEFAULT);
On 2015/10/08 03:12:49, noel gordon wrote:
> Check if the transform is NULL and bail with an error message if so.

Done.

https://codereview.chromium.org/1374953003/diff/160001/third_party/qcms/src/t...
third_party/qcms/src/tests/qcms_test_munsell.c:143: for (i = 0; i <
(sizeof(adobe_munsell) / sizeof(adobe_munsell[0])); i++) {
On 2015/10/08 03:12:49, noel gordon wrote:
> (sizeof(adobe_munsell) / sizeof(adobe_munsell[0]) -> 24.  There are only 24
> Munsell colors.

Done.

https://codereview.chromium.org/1374953003/diff/160001/third_party/qcms/src/t...
third_party/qcms/src/tests/qcms_test_munsell.c:149: results[i] = sqrt(r_diff *
r_diff + g_diff * g_diff + b_diff * b_diff);
On 2015/10/08 03:12:49, noel gordon wrote:
> results[i] is not used other than in this loop.  Seems results[i] could be
made
> a local float variable (an array is needed).

Done.

https://codereview.chromium.org/1374953003/diff/160001/third_party/qcms/src/t...
third_party/qcms/src/tests/qcms_test_munsell.c:153: if (rmse != 0) {
On 2015/10/08 03:12:49, noel gordon wrote:
> remove the if, always write the RMS error.

Done.

https://codereview.chromium.org/1374953003/diff/160001/third_party/qcms/src/t...
third_party/qcms/src/tests/qcms_test_munsell.c:154: printf("Transform RMS
%.3f\n", rmse / 24);
On 2015/10/08 03:12:49, noel gordon wrote:
> rmse / 24 is computed multiple times below.  Why not just do:
> 
>    rmse = rmse / 24.0;
> 
> to compute it once, then use |rmse| instead of |rmse / 24| in the remaining
> code?

Done.

https://codereview.chromium.org/1374953003/diff/160001/third_party/qcms/src/t...
third_party/qcms/src/tests/qcms_test_munsell.c:158: sprintf(file_name,
"qcms_test_munsell_%lu_RMS_%.3f.csv", time(NULL), rmse / 24);
On 2015/10/08 03:12:49, noel gordon wrote:
> Use "...munsell_%ld..." and cast the time(NULL) argument to that type: (long
> int) time(NULL)

Done.

https://codereview.chromium.org/1374953003/diff/160001/third_party/qcms/src/t...
third_party/qcms/src/tests/qcms_test_munsell.c:160: // Print header.
On 2015/10/08 03:12:49, noel gordon wrote:
> Space before this line.

Done.

https://codereview.chromium.org/1374953003/diff/160001/third_party/qcms/src/t...
third_party/qcms/src/tests/qcms_test_munsell.c:166: fprintf(output, "color
name,Input %s[R],Input %s[G],Input %s[B],"
On 2015/10/08 03:12:49, noel gordon wrote:
> Remove the input color "Input %s[R],Input %s[G],Input %s[B]," from this
header:
> just print out the output and reference color results.

Done.

https://codereview.chromium.org/1374953003/diff/160001/third_party/qcms/src/t...
third_party/qcms/src/tests/qcms_test_munsell.c:172: // Print results.
On 2015/10/08 03:12:49, noel gordon wrote:
> Space before this line.

Done.

https://codereview.chromium.org/1374953003/diff/160001/third_party/qcms/src/t...
third_party/qcms/src/tests/qcms_test_munsell.c:173: for (i = 0; i <
(sizeof(adobe_munsell) / sizeof(adobe_munsell[0])); i++) {
On 2015/10/08 03:12:49, noel gordon wrote:
> (sizeof(adobe_munsell) / sizeof(adobe_munsell[0]) -> 24.  There are only 24
> Munsell colors.

Done.

https://codereview.chromium.org/1374953003/diff/160001/third_party/qcms/src/t...
File third_party/qcms/src/tests/qcms_test_util.h (right):

https://codereview.chromium.org/1374953003/diff/160001/third_party/qcms/src/t...
third_party/qcms/src/tests/qcms_test_util.h:4: 
On 2015/10/08 03:12:49, noel gordon wrote:
> Globally use spaces instead of tabs in this file.

Done.

https://codereview.chromium.org/1374953003/diff/160001/third_party/qcms/src/t...
third_party/qcms/src/tests/qcms_test_util.h:4: 
On 2015/10/08 03:12:49, noel gordon wrote:
> Globally use spaces instead of tabs in this file.

Done.

[radu.velea, 2015-10-08 09:49:11]

On 2015/10/08 03:14:59, noel gordon wrote:
> Also noticed, src/test/Makefile is not part of this CL.  It will need to be
> updated to compile the new test harness, right?

I've added a Makefile for the tests. Let me know if you want me to use a
different compiler/flags or have each *.o built separately for qcms_tests.

[radu.velea, 2015-10-08 09:53:36]

On 2015/10/08 09:49:11, radu.velea wrote:
> On 2015/10/08 03:14:59, noel gordon wrote:
> > Also noticed, src/test/Makefile is not part of this CL.  It will need to be
> > updated to compile the new test harness, right?
> 
> I've added a Makefile for the tests. Let me know if you want me to use a
> different compiler/flags or have each *.o built separately for qcms_tests.

Sorry, meant to say "updated" the Makefile.

[Noel Gordon, 2015-10-08 15:40:39]

On 2015/10/08 09:53:36, radu.velea wrote:
> On 2015/10/08 09:49:11, radu.velea wrote:
> > On 2015/10/08 03:14:59, noel gordon wrote:
> > > Also noticed, src/test/Makefile is not part of this CL.  It will need to
be
> > > updated to compile the new test harness, right?
> > 
> > I've added a Makefile for the tests. Let me know if you want me to use a
> > different compiler/flags or have each *.o built separately for qcms_tests.

Seems ok for now.

[Noel Gordon, 2015-10-08 15:42:21]

LGTM

[radu.velea, 2015-10-09 06:37:45]

The CQ bit was checked by radu.velea@intel.com

[commit-bot: I haz the power, 2015-10-09 06:38:10]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1374953003/220001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1374953003/220001

[commit-bot: I haz the power, 2015-10-09 06:41:22]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-10-09 06:41:24]

Try jobs failed on following builders:
  chromeos_x86-generic_chromium_compile_only_ng on tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...)
  linux_chromium_chromeos_ozone_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_clobber_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Noel Gordon, 2015-10-09 07:35:00]

Radu, your git client is maybe out-of-date?  Resync your client to the tip of
the chromium tree, apply your patch to that, and upload again.  Should result in
a clean patch apply on the bots, and a successful CQ submit.

[Noel Gordon, 2015-10-09 08:05:48]

https://codereview.chromium.org/1374953003/diff/220001/third_party/qcms/src/t...
File third_party/qcms/src/tests/qcms_test_munsell.c (right):

https://codereview.chromium.org/1374953003/diff/220001/third_party/qcms/src/t...
third_party/qcms/src/tests/qcms_test_munsell.c:110: return -1;
Just noticed: because the driver does exit_status += return code from the test,
maybe return a positive value here instead of -1?  Alternative:
exit(EXIT_FAILURE).

[radu.velea, 2015-10-09 08:13:35]

https://codereview.chromium.org/1374953003/diff/220001/third_party/qcms/src/t...
File third_party/qcms/src/tests/qcms_test_munsell.c (right):

https://codereview.chromium.org/1374953003/diff/220001/third_party/qcms/src/t...
third_party/qcms/src/tests/qcms_test_munsell.c:110: return -1;
On 2015/10/09 08:05:47, noel gordon wrote:
> Just noticed: because the driver does exit_status += return code from the
test,
> maybe return a positive value here instead of -1?  Alternative:
> exit(EXIT_FAILURE).

Ok. I'll return 0. As an exit might affect the runtime of the rest of the qcms
tests.

[Noel Gordon, 2015-10-09 08:17:03]

Perhaps not 0, that's EXIT_SUCCESS right? Why not just exit?

[radu.velea, 2015-10-09 08:24:34]

On 2015/10/09 08:17:03, noel gordon wrote:
> Perhaps not 0, that's EXIT_SUCCESS right? Why not just exit?

If we run multiple tests, or all of them, exit might cause the whole program to
terminate even tough some tests have yet to run.
For example if we have tetra and munsell test and provide insufficient
parameters for munsell, the test will exit without having ran tetra (even though
there were enough params for tetra).
Is this the behavior we're looking for here?

[Noel Gordon, 2015-10-09 08:34:05]

On 2015/10/09 08:24:34, radu.velea wrote:
> On 2015/10/09 08:17:03, noel gordon wrote:
> > Perhaps not 0, that's EXIT_SUCCESS right? Why not just exit?
> 
> If we run multiple tests, or all of them, exit might cause the whole program
to
> terminate even tough some tests have yet to run.
> For example if we have tetra and munsell test and provide insufficient
> parameters for munsell, the test will exit without having ran tetra (even
though
> there were enough params for tetra).
> Is this the behavior we're looking for here?

Hmm ok, then return EXIT_SUCCESS everywhere?  Does that sound good?

[radu.velea, 2015-10-09 08:37:51]

The CQ bit was checked by radu.velea@intel.com to run a CQ dry run

[radu.velea, 2015-10-09 08:38:12]

On 2015/10/09 08:34:05, noel gordon wrote:
> On 2015/10/09 08:24:34, radu.velea wrote:
> > On 2015/10/09 08:17:03, noel gordon wrote:
> > > Perhaps not 0, that's EXIT_SUCCESS right? Why not just exit?
> > 
> > If we run multiple tests, or all of them, exit might cause the whole program
> to
> > terminate even tough some tests have yet to run.
> > For example if we have tetra and munsell test and provide insufficient
> > parameters for munsell, the test will exit without having ran tetra (even
> though
> > there were enough params for tetra).
> > Is this the behavior we're looking for here?
> 
> Hmm ok, then return EXIT_SUCCESS everywhere?  Does that sound good?

Yes. I've updated the patch.

[commit-bot: I haz the power, 2015-10-09 08:38:21]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1374953003/280001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1374953003/280001

[Noel Gordon, 2015-10-09 09:37:59]

On 2015/10/09 08:38:12, radu.velea wrote:
> On 2015/10/09 08:34:05, noel gordon wrote:
>
> > Hmm ok, then return EXIT_SUCCESS everywhere?  Does that sound good?
> 
> Yes. I've updated the patch.

Right, thanks.

[Noel Gordon, 2015-10-09 09:39:06]

LGTM - ChangeLog up-to-date, new tests, good stuff.

[commit-bot: I haz the power, 2015-10-09 09:44:20]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-10-09 09:44:21]

Dry run: This issue passed the CQ dry run.

[Noel Gordon, 2015-10-09 10:05:07]

The CQ bit was checked by noel@chromium.org

[commit-bot: I haz the power, 2015-10-09 10:05:36]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1374953003/280001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1374953003/280001

[commit-bot: I haz the power, 2015-10-09 10:11:03]

Committed patchset #15 (id:280001)

[commit-bot: I haz the power, 2015-10-09 10:12:05]

Patchset 15 (id:??) landed as
https://crrev.com/daae8f2139bd5681ff0bfbf620a86b645934cd34
Cr-Commit-Position: refs/heads/master@{#353255}