|
|
Chromium Code Reviews|
Created:
4 years, 7 months ago by radu.velea Modified:
4 years, 7 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] Make build_output_lut output 4096 points for parametric curves
build_output_lut inverts a gamma table. Make it return 4096
points for para curves to provide better accuracy.
Update the test to down-sample the output curve data to the
size of the input gamma table. Write the input and output
curve data results to a CSV output file.
Update the gamma table size asserts(): the input and output
gamma table sizes of para curves might not be the same. The
most we know is they are >= 256 points (assert that).
BUG=600338
Committed: https://crrev.com/4b0e97ee4cb19560b5857a53fcd19018846ec697
Cr-Commit-Position: refs/heads/master@{#390402}
Patch Set 1 #Patch Set 2 : Expand input and output tables to 4096 #Patch Set 3 : Updated output table and sampled output data #Patch Set 4 : Readme #Patch Set 5 : Reordered declarations #
Total comments: 20
Patch Set 6 : Added asserts and renamed variables #
Total comments: 4
Patch Set 7 : #
Total comments: 4
Patch Set 8 : Nits #
Messages
Total messages: 31 (20 generated)
Description was changed from ========== [qcms] Remove assertion in get_output_gamma_table BUG=600338 ========== to ========== [qcms] Remove assertion in get_output_gamma_table The assertion was valid only for parametric curves and broke the test for other profiles. Removing it should have no side effects since we do double-check for the correct size later in the test for profiles that contain a parametric curve. BUG=600338 ==========
radu.velea@intel.com changed reviewers: + noel@chromium.org
Description was changed from ========== [qcms] Remove assertion in get_output_gamma_table The assertion was valid only for parametric curves and broke the test for other profiles. Removing it should have no side effects since we do double-check for the correct size later in the test for profiles that contain a parametric curve. BUG=600338 ========== to ========== [qcms] Changed size of output gamma LUT for parametric curves & updated tests The assertion was valid only for parametric curves and broke the test for other profiles. Removing it should have no side effects since we do double-check for the correct size later in the test for profiles that contain a parametric curve. BUG=600338 ==========
Description was changed from ========== [qcms] Changed size of output gamma LUT for parametric curves & updated tests The assertion was valid only for parametric curves and broke the test for other profiles. Removing it should have no side effects since we do double-check for the correct size later in the test for profiles that contain a parametric curve. BUG=600338 ========== to ========== [qcms] Changed size of output gamma LUT for parametric curves & updated tests build_output_lut now inverts a gamma table and returns 4096 values. This provides better accuracy for parametric curves. BUG=600338 ==========
Description was changed from ========== [qcms] Changed size of output gamma LUT for parametric curves & updated tests build_output_lut now inverts a gamma table and returns 4096 values. This provides better accuracy for parametric curves. BUG=600338 ========== to ========== [qcms] Changed size of output gamma LUT for parametric curves & updated tests build_output_lut now inverts a gamma table and returns 4096 values. This provides better accuracy for parametric curves. Test has been updated to sample output data down to input size and write it into a .csv file. Removed asserts as input and output sizes for parametric curves are no longer expected to be equal. BUG=600338 ==========
https://codereview.chromium.org/1923873002/diff/80001/third_party/qcms/README... File third_party/qcms/README.chromium (right): https://codereview.chromium.org/1923873002/diff/80001/third_party/qcms/README... third_party/qcms/README.chromium:154: - Changed size of output gamma LUT for parametric curves & updated tests - Make build_output_lut output 4096 points for parametric curves and then fix the Change Description to match. https://codereview.chromium.org/1923873002/diff/80001/third_party/qcms/src/te... File third_party/qcms/src/tests/qcms_test_output_trc.c (right): https://codereview.chromium.org/1923873002/diff/80001/third_party/qcms/src/te... third_party/qcms/src/tests/qcms_test_output_trc.c:42: *size = qcms_transform_get_output_trc_rgba(transform, target, QCMS_TRC_USHORT, NULL); Good, now optional: should we assert(*size >= 256) here now? https://codereview.chromium.org/1923873002/diff/80001/third_party/qcms/src/te... third_party/qcms/src/tests/qcms_test_output_trc.c:76: *size = qcms_transform_get_input_trc_rgba(transform, source, QCMS_TRC_USHORT, NULL); Good, now optional: should we assert(*size >= 256) here now? https://codereview.chromium.org/1923873002/diff/80001/third_party/qcms/src/te... third_party/qcms/src/tests/qcms_test_output_trc.c:131: FILE *gamma_file; gamma_file -> output_file. https://codereview.chromium.org/1923873002/diff/80001/third_party/qcms/src/te... third_party/qcms/src/tests/qcms_test_output_trc.c:132: char output_file_name[256] = {0,}; char output_file_name[1024]; https://codereview.chromium.org/1923873002/diff/80001/third_party/qcms/src/te... third_party/qcms/src/tests/qcms_test_output_trc.c:171: float p = out_index / (output_size * 1.0); What is the maximum value of |out_index| (assume curves are 256 input, 4096 output as an example). Call this maximum M. What is the value of |p| when out_index is M? https://codereview.chromium.org/1923873002/diff/80001/third_party/qcms/src/te... third_party/qcms/src/tests/qcms_test_output_trc.c:172: float reference_out = clamp_float(evaluate_parametric_curve(type, profile->redTRC->parameter, p)); the "_out"'s are maybe not needed, perhaps use eference_out -> reference https://codereview.chromium.org/1923873002/diff/80001/third_party/qcms/src/te... third_party/qcms/src/tests/qcms_test_output_trc.c:173: float actual_out = gamma_table_out[out_index * 4] * inverse65535; actual_out -> actual https://codereview.chromium.org/1923873002/diff/80001/third_party/qcms/src/te... third_party/qcms/src/tests/qcms_test_output_trc.c:174: float error_out = fabs(actual_out - reference_out); error_out -> difference https://codereview.chromium.org/1923873002/diff/80001/third_party/qcms/src/tr... File third_party/qcms/src/transform_util.c (right): https://codereview.chromium.org/1923873002/diff/80001/third_party/qcms/src/tr... third_party/qcms/src/transform_util.c:618: inverted_size = 4096; Do this at line 604 diff-right. int inverted_size = 4096; Restore the comment (from diff-left), the comment that was here before.
Description was changed from ========== [qcms] Changed size of output gamma LUT for parametric curves & updated tests build_output_lut now inverts a gamma table and returns 4096 values. This provides better accuracy for parametric curves. Test has been updated to sample output data down to input size and write it into a .csv file. Removed asserts as input and output sizes for parametric curves are no longer expected to be equal. BUG=600338 ========== to ========== [qcms] Make build_output_lut output 4096 points for parametric curves build_output_lut now inverts a gamma table and returns 4096 values. This provides better accuracy for parametric curves. Test has been updated to sample output data down to input size and write it into a .csv file. Removed asserts as input and output sizes for parametric curves are no longer expected to be equal. BUG=600338 ==========
https://codereview.chromium.org/1923873002/diff/80001/third_party/qcms/README... File third_party/qcms/README.chromium (right): https://codereview.chromium.org/1923873002/diff/80001/third_party/qcms/README... third_party/qcms/README.chromium:154: - Changed size of output gamma LUT for parametric curves & updated tests On 2016/04/28 07:30:27, noel gordon wrote: > - Make build_output_lut output 4096 points for parametric curves > > and then fix the Change Description to match. > Done. https://codereview.chromium.org/1923873002/diff/80001/third_party/qcms/src/te... File third_party/qcms/src/tests/qcms_test_output_trc.c (right): https://codereview.chromium.org/1923873002/diff/80001/third_party/qcms/src/te... third_party/qcms/src/tests/qcms_test_output_trc.c:42: *size = qcms_transform_get_output_trc_rgba(transform, target, QCMS_TRC_USHORT, NULL); On 2016/04/28 07:30:27, noel gordon wrote: > Good, now optional: should we assert(*size >= 256) here now? I guess it couldn't hurt. https://codereview.chromium.org/1923873002/diff/80001/third_party/qcms/src/te... third_party/qcms/src/tests/qcms_test_output_trc.c:76: *size = qcms_transform_get_input_trc_rgba(transform, source, QCMS_TRC_USHORT, NULL); On 2016/04/28 07:30:27, noel gordon wrote: > Good, now optional: should we assert(*size >= 256) here now? Done. https://codereview.chromium.org/1923873002/diff/80001/third_party/qcms/src/te... third_party/qcms/src/tests/qcms_test_output_trc.c:131: FILE *gamma_file; On 2016/04/28 07:30:27, noel gordon wrote: > gamma_file -> output_file. Done. https://codereview.chromium.org/1923873002/diff/80001/third_party/qcms/src/te... third_party/qcms/src/tests/qcms_test_output_trc.c:132: char output_file_name[256] = {0,}; On 2016/04/28 07:30:27, noel gordon wrote: > char output_file_name[1024]; Done. https://codereview.chromium.org/1923873002/diff/80001/third_party/qcms/src/te... third_party/qcms/src/tests/qcms_test_output_trc.c:171: float p = out_index / (output_size * 1.0); On 2016/04/28 07:30:27, noel gordon wrote: > What is the maximum value of |out_index| (assume curves are 256 input, 4096 > output as an example). Call this maximum M. What is the value of |p| when > out_index is M? 4095 / 4096 * 1.0, which comes short of 1.0 (which may explain why lcms final value is not 1.0 in our tables). we could change this to out_index / ((output_size - 1) * 1.0), since we adjusted the scale to (float)(output_size - 1) / (input_size - 1); https://codereview.chromium.org/1923873002/diff/80001/third_party/qcms/src/te... third_party/qcms/src/tests/qcms_test_output_trc.c:172: float reference_out = clamp_float(evaluate_parametric_curve(type, profile->redTRC->parameter, p)); On 2016/04/28 07:30:27, noel gordon wrote: > the "_out"'s are maybe not needed, perhaps use > > eference_out -> reference Done. https://codereview.chromium.org/1923873002/diff/80001/third_party/qcms/src/te... third_party/qcms/src/tests/qcms_test_output_trc.c:173: float actual_out = gamma_table_out[out_index * 4] * inverse65535; On 2016/04/28 07:30:27, noel gordon wrote: > actual_out -> actual Done. https://codereview.chromium.org/1923873002/diff/80001/third_party/qcms/src/te... third_party/qcms/src/tests/qcms_test_output_trc.c:174: float error_out = fabs(actual_out - reference_out); On 2016/04/28 07:30:27, noel gordon wrote: > error_out -> difference Done. https://codereview.chromium.org/1923873002/diff/80001/third_party/qcms/src/tr... File third_party/qcms/src/transform_util.c (right): https://codereview.chromium.org/1923873002/diff/80001/third_party/qcms/src/tr... third_party/qcms/src/transform_util.c:618: inverted_size = 4096; On 2016/04/28 07:30:27, noel gordon wrote: > Do this at line 604 diff-right. int inverted_size = 4096; > > Restore the comment (from diff-left), the comment that was here before. Done.
https://codereview.chromium.org/1923873002/diff/100001/third_party/qcms/src/t... File third_party/qcms/src/tests/qcms_test_output_trc.c (right): https://codereview.chromium.org/1923873002/diff/100001/third_party/qcms/src/t... third_party/qcms/src/tests/qcms_test_output_trc.c:172: size_t out_index = (size_t)floor( i * scale_factor + 0.5); floor( i * -> floor(i * https://codereview.chromium.org/1923873002/diff/100001/third_party/qcms/src/t... third_party/qcms/src/tests/qcms_test_output_trc.c:173: float p = out_index / ((output_size - 1) * 1.0); (output_size - 1.0); here? Please check locally that the LCMS is sampled at 1.0 when the maximum out_index is reached, update the spreadsheet with the results please.
https://codereview.chromium.org/1923873002/diff/100001/third_party/qcms/src/t... File third_party/qcms/src/tests/qcms_test_output_trc.c (right): https://codereview.chromium.org/1923873002/diff/100001/third_party/qcms/src/t... third_party/qcms/src/tests/qcms_test_output_trc.c:172: size_t out_index = (size_t)floor( i * scale_factor + 0.5); On 2016/04/28 08:26:45, noel gordon wrote: > floor( i * -> floor(i * Done. https://codereview.chromium.org/1923873002/diff/100001/third_party/qcms/src/t... third_party/qcms/src/tests/qcms_test_output_trc.c:173: float p = out_index / ((output_size - 1) * 1.0); On 2016/04/28 08:26:45, noel gordon wrote: > (output_size - 1.0); here? > > Please check locally that the LCMS is sampled at 1.0 when the maximum out_index > is reached, update the spreadsheet with the results please. Done.
LGTM with nits. Also, I will dry-run this. https://codereview.chromium.org/1923873002/diff/120001/third_party/qcms/src/t... File third_party/qcms/src/tests/qcms_test_output_trc.c (right): https://codereview.chromium.org/1923873002/diff/120001/third_party/qcms/src/t... third_party/qcms/src/tests/qcms_test_output_trc.c:118: printf("LUT size = %zu\n", output_size); nit: "Output gamma LUT size ..." https://codereview.chromium.org/1923873002/diff/120001/third_party/qcms/src/t... third_party/qcms/src/tests/qcms_test_output_trc.c:173: float p = out_index / ((output_size - 1) * 1.0); nit: float p = out_index / (float)(output_size - 1);
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/1923873002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1923873002/120001
Description was changed from ========== [qcms] Make build_output_lut output 4096 points for parametric curves build_output_lut now inverts a gamma table and returns 4096 values. This provides better accuracy for parametric curves. Test has been updated to sample output data down to input size and write it into a .csv file. Removed asserts as input and output sizes for parametric curves are no longer expected to be equal. BUG=600338 ========== to ========== [qcms] Make build_output_lut output 4096 points for parametric curves build_output_lut inverts a gamma table; make it return 4096 values for parametric curves to provide better accuracy. Update the test to down-sample the output curve data to the size of the input gamma table and write the results to a CSV file. Removed asserts since the input and output sizes for parametric curves are no longer expected to be equal. BUG=600338 ==========
Description was changed from ========== [qcms] Make build_output_lut output 4096 points for parametric curves build_output_lut inverts a gamma table; make it return 4096 values for parametric curves to provide better accuracy. Update the test to down-sample the output curve data to the size of the input gamma table and write the results to a CSV file. Removed asserts since the input and output sizes for parametric curves are no longer expected to be equal. BUG=600338 ========== to ========== [qcms] Make build_output_lut output 4096 points for parametric curves build_output_lut inverts a gamma table. Make it return 4096 values for parametric curves to provide better accuracy. Update the test to down-sample the output curve data to the size of the input gamma table and write the results to a CSV file. Removed asserts since the input and output sizes for parametric curves are no longer expected to be equal. BUG=600338 ==========
Description was changed from ========== [qcms] Make build_output_lut output 4096 points for parametric curves build_output_lut inverts a gamma table. Make it return 4096 values for parametric curves to provide better accuracy. Update the test to down-sample the output curve data to the size of the input gamma table and write the results to a CSV file. Removed asserts since the input and output sizes for parametric curves are no longer expected to be equal. BUG=600338 ========== to ========== [qcms] Make build_output_lut output 4096 points for parametric curves build_output_lut inverts a gamma table. Make it return 4096 values for parametric curves to provide better accuracy. Update the test to down-sample the output curve data to the size of the input gamma table and write the results to a CSV output file. Remove gamma table size asserts since the input and output sizes for para curves are no longer expected to be equal. BUG=600338 ==========
Description was changed from ========== [qcms] Make build_output_lut output 4096 points for parametric curves build_output_lut inverts a gamma table. Make it return 4096 values for parametric curves to provide better accuracy. Update the test to down-sample the output curve data to the size of the input gamma table and write the results to a CSV output file. Remove gamma table size asserts since the input and output sizes for para curves are no longer expected to be equal. BUG=600338 ========== to ========== [qcms] Make build_output_lut output 4096 points for parametric curves build_output_lut inverts a gamma table. Make it return 4096 points for parametric curves to provide better accuracy. Update the test to down-sample the output curve data to the size of the input gamma table and write the results to a CSV output file. Remove gamma table size asserts() since the input and output sizes of para curves are no longer expected to be equal. BUG=600338 ==========
Description was changed from ========== [qcms] Make build_output_lut output 4096 points for parametric curves build_output_lut inverts a gamma table. Make it return 4096 points for parametric curves to provide better accuracy. Update the test to down-sample the output curve data to the size of the input gamma table and write the results to a CSV output file. Remove gamma table size asserts() since the input and output sizes of para curves are no longer expected to be equal. BUG=600338 ========== to ========== [qcms] Make build_output_lut output 4096 points for parametric curves build_output_lut inverts a gamma table. Make it return 4096 points for parametric curves to provide better accuracy. Update the test to down-sample the output curve data to the size of the input gamma table. Write the input and output curve data results to a CSV output file. Remove gamma table size asserts since the input and output table sizes of para curves might not be equal. BUG=600338 ==========
Description was changed from ========== [qcms] Make build_output_lut output 4096 points for parametric curves build_output_lut inverts a gamma table. Make it return 4096 points for parametric curves to provide better accuracy. Update the test to down-sample the output curve data to the size of the input gamma table. Write the input and output curve data results to a CSV output file. Remove gamma table size asserts since the input and output table sizes of para curves might not be equal. BUG=600338 ========== to ========== [qcms] Make build_output_lut output 4096 points for parametric curves build_output_lut inverts a gamma table. Make it return 4096 points for parametric curves to provide better accuracy. Update the test to down-sample the output curve data to the size of the input gamma table. Write the input and output curve data results to a CSV output file for debug. Remove gamma table size asserts since the input and output table sizes of para curves might not be equal. BUG=600338 ==========
Description was changed from ========== [qcms] Make build_output_lut output 4096 points for parametric curves build_output_lut inverts a gamma table. Make it return 4096 points for parametric curves to provide better accuracy. Update the test to down-sample the output curve data to the size of the input gamma table. Write the input and output curve data results to a CSV output file for debug. Remove gamma table size asserts since the input and output table sizes of para curves might not be equal. BUG=600338 ========== to ========== [qcms] Make build_output_lut output 4096 points for parametric curves build_output_lut inverts a gamma table. Make it return 4096 points for parametric curves to provide better accuracy. Update the test to down-sample the output curve data to the size of the input gamma table. Write the input and output curve data results to a CSV output file. Remove the gamma table size asserts(): the input and output gamma table sizes of para curves might not be equal. BUG=600338 ==========
Description was changed from ========== [qcms] Make build_output_lut output 4096 points for parametric curves build_output_lut inverts a gamma table. Make it return 4096 points for parametric curves to provide better accuracy. Update the test to down-sample the output curve data to the size of the input gamma table. Write the input and output curve data results to a CSV output file. Remove the gamma table size asserts(): the input and output gamma table sizes of para curves might not be equal. BUG=600338 ========== to ========== [qcms] Make build_output_lut output 4096 points for parametric curves build_output_lut inverts a gamma table. Make it return 4096 points for para curves to provide better accuracy. Update the test to down-sample the output curve data to the size of the input gamma table. Write the input and output curve data results to a CSV output file. Remove the gamma table size asserts(): the input and output gamma table sizes of para curves might not be equal. BUG=600338 ==========
https://codereview.chromium.org/1923873002/diff/120001/third_party/qcms/src/t... File third_party/qcms/src/tests/qcms_test_output_trc.c (right): https://codereview.chromium.org/1923873002/diff/120001/third_party/qcms/src/t... third_party/qcms/src/tests/qcms_test_output_trc.c:118: printf("LUT size = %zu\n", output_size); On 2016/04/28 10:28:40, noel gordon wrote: > nit: "Output gamma LUT size ..." Done. https://codereview.chromium.org/1923873002/diff/120001/third_party/qcms/src/t... third_party/qcms/src/tests/qcms_test_output_trc.c:173: float p = out_index / ((output_size - 1) * 1.0); On 2016/04/28 10:28:40, noel gordon wrote: > nit: float p = out_index / (float)(output_size - 1); Done.
LGTM. Due to the current GN/GYP compiler checking issues on landing (trys green, but main Win builders fail), I patched this CL in locally on Windows (since you do not have a windows machine) and built. No errors, should be fine to land.
The CQ bit was checked by noel@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1923873002/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1923873002/140001
Description was changed from ========== [qcms] Make build_output_lut output 4096 points for parametric curves build_output_lut inverts a gamma table. Make it return 4096 points for para curves to provide better accuracy. Update the test to down-sample the output curve data to the size of the input gamma table. Write the input and output curve data results to a CSV output file. Remove the gamma table size asserts(): the input and output gamma table sizes of para curves might not be equal. BUG=600338 ========== to ========== [qcms] Make build_output_lut output 4096 points for parametric curves build_output_lut inverts a gamma table. Make it return 4096 points for para curves to provide better accuracy. Update the test to down-sample the output curve data to the size of the input gamma table. Write the input and output curve data results to a CSV output file. Update the gamma table size asserts(): the input and output gamma table sizes of para curves might not be equal. BUG=600338 ==========
Description was changed from ========== [qcms] Make build_output_lut output 4096 points for parametric curves build_output_lut inverts a gamma table. Make it return 4096 points for para curves to provide better accuracy. Update the test to down-sample the output curve data to the size of the input gamma table. Write the input and output curve data results to a CSV output file. Update the gamma table size asserts(): the input and output gamma table sizes of para curves might not be equal. BUG=600338 ========== to ========== [qcms] Make build_output_lut output 4096 points for parametric curves build_output_lut inverts a gamma table. Make it return 4096 points for para curves to provide better accuracy. Update the test to down-sample the output curve data to the size of the input gamma table. Write the input and output curve data results to a CSV output file. Update the gamma table size asserts(): the input and output gamma table sizes of para curves might not be the same. The most we know is they are >= 256 points (assert that). BUG=600338 ==========
Message was sent while issue was closed.
Description was changed from ========== [qcms] Make build_output_lut output 4096 points for parametric curves build_output_lut inverts a gamma table. Make it return 4096 points for para curves to provide better accuracy. Update the test to down-sample the output curve data to the size of the input gamma table. Write the input and output curve data results to a CSV output file. Update the gamma table size asserts(): the input and output gamma table sizes of para curves might not be the same. The most we know is they are >= 256 points (assert that). BUG=600338 ========== to ========== [qcms] Make build_output_lut output 4096 points for parametric curves build_output_lut inverts a gamma table. Make it return 4096 points for para curves to provide better accuracy. Update the test to down-sample the output curve data to the size of the input gamma table. Write the input and output curve data results to a CSV output file. Update the gamma table size asserts(): the input and output gamma table sizes of para curves might not be the same. The most we know is they are >= 256 points (assert that). BUG=600338 ==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001)
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/4b0e97ee4cb19560b5857a53fcd19018846ec697 Cr-Commit-Position: refs/heads/master@{#390402}
Message was sent while issue was closed.
Description was changed from ========== [qcms] Make build_output_lut output 4096 points for parametric curves build_output_lut inverts a gamma table. Make it return 4096 points for para curves to provide better accuracy. Update the test to down-sample the output curve data to the size of the input gamma table. Write the input and output curve data results to a CSV output file. Update the gamma table size asserts(): the input and output gamma table sizes of para curves might not be the same. The most we know is they are >= 256 points (assert that). BUG=600338 ========== to ========== [qcms] Make build_output_lut output 4096 points for parametric curves build_output_lut inverts a gamma table. Make it return 4096 points for para curves to provide better accuracy. Update the test to down-sample the output curve data to the size of the input gamma table. Write the input and output curve data results to a CSV output file. Update the gamma table size asserts(): the input and output gamma table sizes of para curves might not be the same. The most we know is they are >= 256 points (assert that). BUG=600338 Committed: https://crrev.com/4b0e97ee4cb19560b5857a53fcd19018846ec697 Cr-Commit-Position: refs/heads/master@{#390402} ========== |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
