Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1862053002:
Reland: [qcms] Fix build_output_lut to return correct data for parametric curves (Closed)
|
Created:
4 years, 8 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] Fix build_output_lut to return correct data for parametric curves
build_output_lut() does not invert a parametric gamma curve when
computing the output curve data. The effect has not been visible
since Chrome only uses the output gamma values from the precache
table (which is inverted).
Make build_output_lut() return inverted data for parametric gamma
curves. Add a test to write the inverted data, and the output of
LCMS function DefaultEvalParametricFn, to a file for comparison.
For now the size of input and output gamma table for parametric
curves is hard-coded to 256; assert this in the test code. See
compute_curve_gamma_table_type_parametric for details. Future
implementations might return an arbitrary-sized table.
BUG=600338
Committed: https://crrev.com/a89370950f514a3df77ed84240d4b8e3abbee801
Cr-Commit-Position: refs/heads/master@{#389754}
Committed: https://crrev.com/3a4a913c1de915ef228a35b4f85603662719c49f
Cr-Commit-Position: refs/heads/master@{#389866}
Committed: https://crrev.com/555dfdfcd56ae8ef91637e4b0827970be7609059
Cr-Commit-Position: refs/heads/master@{#389964}
Patch Set 1 #Patch Set 2 : Use invert_lut on the values returned by build_output_lut for parametric curves #Patch Set 3 : whitespace #Patch Set 4 : Retrieved input gamma for parametric curves. Compute best gamma from data set #
Total comments: 1
Patch Set 5 : Whitespace and comments #
Total comments: 2
Patch Set 6 : Rebase and remove WebKit/LayoutTests/whitespace.txt #
Total comments: 33
Patch Set 7 : Updated Makefile & comments. Cleanedup qcms_test_output_trc.c #
Total comments: 14
Patch Set 8 : Removed gamma estimator #
Total comments: 12
Patch Set 9 : Removed precached code and added assertions #
Total comments: 4
Patch Set 10 : Removed unused variable #Patch Set 11 : Removed redundant comment #
Total comments: 4
Patch Set 12 : Nits #Patch Set 13 : Changed output message #Patch Set 14 : Cast unsigned to int before negating to remove warning on windows #Patch Set 15 : Remove more warnings #
Total comments: 1
Messages
Total messages: 73 (33 generated)
Description was changed from ========== [qcms] [wip] Fix build_output_lut to return correct data for parametric curves build_output_lut does not correctly invert a parametric gamma curve. The effect has not been visible so far because Chrome only used gamma values from the precache table (which is inverted). qcms_transform_get_output_trc_rgba, nevertheless returns incorrect data. The current CL adds a test to verify this behaviour and attempts to fix the problem. So far I have considered as valid output the values returned from lcms function DefaultEvalParametricFn. This is still work in progress. BUG=600338 ========== to ========== [qcms] Fix build_output_lut to return correct data for parametric curves build_output_lut does not correctly invert a parametric gamma curve. The effect has not been visible so far because Chrome only used gamma values from the precache table (which is inverted). qcms_transform_get_output_trc_rgba, nevertheless returns incorrect data. The current CL adds a test to verify this behavior and attempts to fix the problem. So far I have considered as valid output the values returned from lcms function DefaultEvalParametricFn. For parametric curves both input and output gamma is written to a csv file and approximated using the formula from: http://www.brucelindbloom.com/index.html?Eqn_BestGamma.html BUG=600338 ==========
radu.velea@intel.com changed reviewers: + noel@chromium.org
https://codereview.chromium.org/1862053002/diff/60001/third_party/qcms/src/te... File third_party/qcms/src/tests/qcms_test_main.c (left): https://codereview.chromium.org/1862053002/diff/60001/third_party/qcms/src/te... third_party/qcms/src/tests/qcms_test_main.c:76: void generate_source_uint8_t(unsigned char *src, const size_t length, const size_t pixel_size) Removed this and moved it into a new "util" file so it can be easily called by other tests.
https://codereview.chromium.org/1862053002/diff/80001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/whitespace.txt (right): https://codereview.chromium.org/1862053002/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/whitespace.txt:19: Still the data is inacurate! After https://bugs.chromium.org/p/chromium/issues/detail?id=597181 we don't need to update this file any more.
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/1862053002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1862053002/80001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios_dbg_simulator_gn on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...) 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_...)
rebased https://codereview.chromium.org/1862053002/diff/80001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/whitespace.txt (right): https://codereview.chromium.org/1862053002/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/whitespace.txt:19: Still the data is inacurate! On 2016/04/18 03:27:36, noel gordon wrote: > After https://bugs.chromium.org/p/chromium/issues/detail?id=597181 we don't need > to update this file any more. Done.
https://codereview.chromium.org/1862053002/diff/100001/third_party/qcms/src/t... File third_party/qcms/src/tests/Makefile (right): https://codereview.chromium.org/1862053002/diff/100001/third_party/qcms/src/t... third_party/qcms/src/tests/Makefile:12: qcms_tests: qcms_test_main.c qcms_test_munsell.c qcms_test_tetra_clut_rgba.c qcms_test_internal_srgb.c qcms_test_ntsc_gamut.c qcms_test_output_trc.c \ Would qcms_tests: qcms_test_*.c $(OBJS) work? Seems simpler if so?
https://codereview.chromium.org/1862053002/diff/100001/third_party/qcms/src/t... File third_party/qcms/src/tests/qcms_test_util.c (right): https://codereview.chromium.org/1862053002/diff/100001/third_party/qcms/src/t... third_party/qcms/src/tests/qcms_test_util.c:12: void generate_source_uint8_t(unsigned char *src, const size_t length, const size_t pixel_size) Perhaps prefix this helper with a comment ... // Store random pixel data in the source. https://codereview.chromium.org/1862053002/diff/100001/third_party/qcms/src/t... third_party/qcms/src/tests/qcms_test_util.c:22: // Parametric Fn using floating point <from lcms>: DefaultEvalParametricFn DefaultEvalParametricFn() is in which LCMS file? Perhaps mention that here to provide a cross reference.
https://codereview.chromium.org/1862053002/diff/100001/third_party/qcms/src/t... File third_party/qcms/src/tests/qcms_test_output_trc.c (right): https://codereview.chromium.org/1862053002/diff/100001/third_party/qcms/src/t... third_party/qcms/src/tests/qcms_test_output_trc.c:22: qcms_format_type format = {0, 2}; // RGBA format: where is this used? https://codereview.chromium.org/1862053002/diff/100001/third_party/qcms/src/t... third_party/qcms/src/tests/qcms_test_output_trc.c:24: qcms_profile *profile, *sRGB; qcms_profile *sRGB; qcms_profile *target; https://codereview.chromium.org/1862053002/diff/100001/third_party/qcms/src/t... third_party/qcms/src/tests/qcms_test_output_trc.c:27: profile = qcms_profile_from_path(profile_path); target = qcms_profile_from_path(profile_path); https://codereview.chromium.org/1862053002/diff/100001/third_party/qcms/src/t... third_party/qcms/src/tests/qcms_test_output_trc.c:33: sRGB = qcms_profile_sRGB(); Move this, place it after the precache step. https://codereview.chromium.org/1862053002/diff/100001/third_party/qcms/src/t... third_party/qcms/src/tests/qcms_test_output_trc.c:42: ret = 1; clean up right here: no real need for the goto, no need for the |ret| var. https://codereview.chromium.org/1862053002/diff/100001/third_party/qcms/src/t... third_party/qcms/src/tests/qcms_test_output_trc.c:61: qcms_format_type format = {0, 2}; // RGBA format: where is this used? https://codereview.chromium.org/1862053002/diff/100001/third_party/qcms/src/t... third_party/qcms/src/tests/qcms_test_output_trc.c:63: qcms_profile *profile, *sRGB; qcms_profile *source; qcms_profile *sRGB; https://codereview.chromium.org/1862053002/diff/100001/third_party/qcms/src/t... third_party/qcms/src/tests/qcms_test_output_trc.c:78: ret = 1; again, clean up right here (no need for the goto, or the |ret| var). https://codereview.chromium.org/1862053002/diff/100001/third_party/qcms/src/t... third_party/qcms/src/tests/qcms_test_output_trc.c:115: char file_name[256] = {0,}; Move this down to where used. https://codereview.chromium.org/1862053002/diff/100001/third_party/qcms/src/t... third_party/qcms/src/tests/qcms_test_output_trc.c:158: extra line: remove. https://codereview.chromium.org/1862053002/diff/100001/third_party/qcms/src/t... third_party/qcms/src/tests/qcms_test_output_trc.c:161: extra line: remove. https://codereview.chromium.org/1862053002/diff/100001/third_party/qcms/src/t... third_party/qcms/src/tests/qcms_test_output_trc.c:168: extra line: remove. https://codereview.chromium.org/1862053002/diff/100001/third_party/qcms/src/t... third_party/qcms/src/tests/qcms_test_output_trc.c:171: extra line: remove.
https://codereview.chromium.org/1862053002/diff/100001/third_party/qcms/src/t... File third_party/qcms/src/tests/Makefile (right): https://codereview.chromium.org/1862053002/diff/100001/third_party/qcms/src/t... third_party/qcms/src/tests/Makefile:12: qcms_tests: qcms_test_main.c qcms_test_munsell.c qcms_test_tetra_clut_rgba.c qcms_test_internal_srgb.c qcms_test_ntsc_gamut.c qcms_test_output_trc.c \ On 2016/04/19 00:57:45, noel gordon wrote: > Would qcms_tests: qcms_test_*.c $(OBJS) work? Seems simpler if so? Done. https://codereview.chromium.org/1862053002/diff/100001/third_party/qcms/src/t... File third_party/qcms/src/tests/qcms_test_output_trc.c (right): https://codereview.chromium.org/1862053002/diff/100001/third_party/qcms/src/t... third_party/qcms/src/tests/qcms_test_output_trc.c:22: qcms_format_type format = {0, 2}; // RGBA On 2016/04/19 01:26:05, noel gordon wrote: > format: where is this used? Done. https://codereview.chromium.org/1862053002/diff/100001/third_party/qcms/src/t... third_party/qcms/src/tests/qcms_test_output_trc.c:24: qcms_profile *profile, *sRGB; On 2016/04/19 01:26:05, noel gordon wrote: > qcms_profile *sRGB; > qcms_profile *target; Done. https://codereview.chromium.org/1862053002/diff/100001/third_party/qcms/src/t... third_party/qcms/src/tests/qcms_test_output_trc.c:27: profile = qcms_profile_from_path(profile_path); On 2016/04/19 01:26:05, noel gordon wrote: > target = qcms_profile_from_path(profile_path); Done. https://codereview.chromium.org/1862053002/diff/100001/third_party/qcms/src/t... third_party/qcms/src/tests/qcms_test_output_trc.c:33: sRGB = qcms_profile_sRGB(); On 2016/04/19 01:26:05, noel gordon wrote: > Move this, place it after the precache step. Done. https://codereview.chromium.org/1862053002/diff/100001/third_party/qcms/src/t... third_party/qcms/src/tests/qcms_test_output_trc.c:42: ret = 1; On 2016/04/19 01:26:06, noel gordon wrote: > clean up right here: no real need for the goto, no need for the |ret| var. Done. https://codereview.chromium.org/1862053002/diff/100001/third_party/qcms/src/t... third_party/qcms/src/tests/qcms_test_output_trc.c:61: qcms_format_type format = {0, 2}; // RGBA On 2016/04/19 01:26:06, noel gordon wrote: > format: where is this used? Done. https://codereview.chromium.org/1862053002/diff/100001/third_party/qcms/src/t... third_party/qcms/src/tests/qcms_test_output_trc.c:63: qcms_profile *profile, *sRGB; On 2016/04/19 01:26:05, noel gordon wrote: > qcms_profile *source; > qcms_profile *sRGB; Done. https://codereview.chromium.org/1862053002/diff/100001/third_party/qcms/src/t... third_party/qcms/src/tests/qcms_test_output_trc.c:78: ret = 1; On 2016/04/19 01:26:05, noel gordon wrote: > again, clean up right here (no need for the goto, or the |ret| var). Done. https://codereview.chromium.org/1862053002/diff/100001/third_party/qcms/src/t... third_party/qcms/src/tests/qcms_test_output_trc.c:115: char file_name[256] = {0,}; On 2016/04/19 01:26:05, noel gordon wrote: > Move this down to where used. Done. https://codereview.chromium.org/1862053002/diff/100001/third_party/qcms/src/t... third_party/qcms/src/tests/qcms_test_output_trc.c:158: On 2016/04/19 01:26:05, noel gordon wrote: > extra line: remove. Done. https://codereview.chromium.org/1862053002/diff/100001/third_party/qcms/src/t... third_party/qcms/src/tests/qcms_test_output_trc.c:161: On 2016/04/19 01:26:05, noel gordon wrote: > extra line: remove. Done. https://codereview.chromium.org/1862053002/diff/100001/third_party/qcms/src/t... third_party/qcms/src/tests/qcms_test_output_trc.c:168: On 2016/04/19 01:26:05, noel gordon wrote: > extra line: remove. Done. https://codereview.chromium.org/1862053002/diff/100001/third_party/qcms/src/t... third_party/qcms/src/tests/qcms_test_output_trc.c:171: On 2016/04/19 01:26:05, noel gordon wrote: > extra line: remove. Done. https://codereview.chromium.org/1862053002/diff/100001/third_party/qcms/src/t... third_party/qcms/src/tests/qcms_test_output_trc.c:232: printf("Computed gamma from LUTs:\nInput = %.6lf\nOutput = %.6lf\nAccuracy? = %.6lf\n", I didn't know how exactly to refer to the aproxInput * aproxOutput so I put Accuracy? Any thoughts on this one? https://codereview.chromium.org/1862053002/diff/100001/third_party/qcms/src/t... File third_party/qcms/src/tests/qcms_test_util.c (right): https://codereview.chromium.org/1862053002/diff/100001/third_party/qcms/src/t... third_party/qcms/src/tests/qcms_test_util.c:12: void generate_source_uint8_t(unsigned char *src, const size_t length, const size_t pixel_size) On 2016/04/19 01:06:00, noel gordon wrote: > Perhaps prefix this helper with a comment ... > > // Store random pixel data in the source. Done. https://codereview.chromium.org/1862053002/diff/100001/third_party/qcms/src/t... third_party/qcms/src/tests/qcms_test_util.c:22: // Parametric Fn using floating point <from lcms>: DefaultEvalParametricFn On 2016/04/19 01:06:01, noel gordon wrote: > DefaultEvalParametricFn() is in which LCMS file? Perhaps mention that here to > provide a cross reference. Done.
On 2016/04/19 08:42:07, radu.velea wrote: > https://codereview.chromium.org/1862053002/diff/100001/third_party/qcms/src/t... > third_party/qcms/src/tests/qcms_test_output_trc.c:232: printf("Computed gamma > from LUTs:\nInput = %.6lf\nOutput = %.6lf\nAccuracy? = %.6lf\n", > I didn't know how exactly to refer to the aproxInput * aproxOutput so I put > Accuracy? Any thoughts on this one? Hmm, I don't think the gamma estimator is really helping here, and it certainly makes the test much harder to read. Perhaps we should remove it.
https://codereview.chromium.org/1862053002/diff/120001/third_party/qcms/src/t... File third_party/qcms/src/tests/qcms_test_output_trc.c (right): https://codereview.chromium.org/1862053002/diff/120001/third_party/qcms/src/t... third_party/qcms/src/tests/qcms_test_output_trc.c:32: if (precache) { Why is the precache relevant to this test? The curve we extract is created and lives inside the transform struct, which is independent of pre-caching best I can tell. https://codereview.chromium.org/1862053002/diff/120001/third_party/qcms/src/t... third_party/qcms/src/tests/qcms_test_output_trc.c:125: if (get_output_gamma_table(in_path, &gamma_table_out, &gamma_table_size, true) != 0) { true => precache, but the results are stored in gamma_table_* ... https://codereview.chromium.org/1862053002/diff/120001/third_party/qcms/src/t... third_party/qcms/src/tests/qcms_test_output_trc.c:132: if (get_output_gamma_table(in_path, &gamma_table_out_precache, &gamma_table_size_precache, false) != 0) { ... false => no pre-cache but the results are stored in gamma_table_*_precache vars., (which is a little confusing) ... https://codereview.chromium.org/1862053002/diff/120001/third_party/qcms/src/t... third_party/qcms/src/tests/qcms_test_output_trc.c:138: if (gamma_table_size != gamma_table_size_precache) { but as mentioned above, why do the precache vs non-precache cases need to be separately tested. https://codereview.chromium.org/1862053002/diff/120001/third_party/qcms/src/t... third_party/qcms/src/tests/qcms_test_output_trc.c:217: I don't think the gamma estimator adds to the test, so on second thoughts, let's remove it? https://codereview.chromium.org/1862053002/diff/120001/third_party/qcms/src/t... File third_party/qcms/src/transform_util.c (right): https://codereview.chromium.org/1862053002/diff/120001/third_party/qcms/src/t... third_party/qcms/src/transform_util.c:610: *output_gamma_lut_length = 256; This line needs to be made more obvious. Perhaps place it after the for loop and somehow make it better refer to inverted_size, since that is the value that is eventually stored there, right? Maybe *output_gamma_lut_length = inverted_size near line 628 diff-right would makes the code a lot clearer? https://codereview.chromium.org/1862053002/diff/120001/third_party/qcms/src/t... third_party/qcms/src/transform_util.c:616: int inverted_size = 256; These two variables need to be declared just after line 602 diff-right.
https://codereview.chromium.org/1862053002/diff/120001/third_party/qcms/src/t... File third_party/qcms/src/tests/qcms_test_output_trc.c (right): https://codereview.chromium.org/1862053002/diff/120001/third_party/qcms/src/t... third_party/qcms/src/tests/qcms_test_output_trc.c:32: if (precache) { On 2016/04/21 06:01:53, noel gordon wrote: > Why is the precache relevant to this test? The curve we extract is created and > lives inside the transform struct, which is independent of pre-caching best I > can tell. Yes, but the same calculations are done during precache stage. Maybe in the future we'll save them somewhere so we don't have to do it twice. I just thought that regardless how the 2 code paths evolve it would be nice to check for consistency. If you think it is redundant I can remove it. https://codereview.chromium.org/1862053002/diff/120001/third_party/qcms/src/t... third_party/qcms/src/tests/qcms_test_output_trc.c:125: if (get_output_gamma_table(in_path, &gamma_table_out, &gamma_table_size, true) != 0) { On 2016/04/21 06:01:53, noel gordon wrote: > true => precache, but the results are stored in gamma_table_* ... See comment above. https://codereview.chromium.org/1862053002/diff/120001/third_party/qcms/src/t... third_party/qcms/src/tests/qcms_test_output_trc.c:132: if (get_output_gamma_table(in_path, &gamma_table_out_precache, &gamma_table_size_precache, false) != 0) { On 2016/04/21 06:01:53, noel gordon wrote: > ... false => no pre-cache but the results are stored in gamma_table_*_precache > vars., (which is a little confusing) ... Same as above. https://codereview.chromium.org/1862053002/diff/120001/third_party/qcms/src/t... third_party/qcms/src/tests/qcms_test_output_trc.c:217: On 2016/04/21 06:01:53, noel gordon wrote: > I don't think the gamma estimator adds to the test, so on second thoughts, let's > remove it? Ok. https://codereview.chromium.org/1862053002/diff/120001/third_party/qcms/src/t... File third_party/qcms/src/transform_util.c (right): https://codereview.chromium.org/1862053002/diff/120001/third_party/qcms/src/t... third_party/qcms/src/transform_util.c:610: *output_gamma_lut_length = 256; On 2016/04/21 06:01:54, noel gordon wrote: > This line needs to be made more obvious. Perhaps place it after the for loop > and somehow make it better refer to inverted_size, since that is the value that > is eventually stored there, right? > > Maybe *output_gamma_lut_length = inverted_size near line 628 diff-right would > makes the code a lot clearer? Done. https://codereview.chromium.org/1862053002/diff/120001/third_party/qcms/src/t... third_party/qcms/src/transform_util.c:616: int inverted_size = 256; On 2016/04/21 06:01:54, noel gordon wrote: > These two variables need to be declared just after line 602 diff-right. Done.
On 2016/04/21 07:13:45, radu.velea wrote: > https://codereview.chromium.org/1862053002/diff/120001/third_party/qcms/src/t... > File third_party/qcms/src/tests/qcms_test_output_trc.c (right): > > https://codereview.chromium.org/1862053002/diff/120001/third_party/qcms/src/t... > third_party/qcms/src/tests/qcms_test_output_trc.c:32: if (precache) { > On 2016/04/21 06:01:53, noel gordon wrote: > > Why is the precache relevant to this test? The curve we extract is created > and > > lives inside the transform struct, which is independent of pre-caching best I > > can tell. > > Yes, but the same calculations are done during precache stage. Maybe in the > future we'll save them somewhere so we don't have to do it twice. Maybe we will provide a API for reading from the precache, we'll see. > I just thought > that regardless how the 2 code paths evolve it would be nice to check for > consistency. If you think it is redundant I can remove it. Yeah, I think redundant for now, let's remove it.
https://codereview.chromium.org/1862053002/diff/140001/third_party/qcms/src/t... File third_party/qcms/src/tests/qcms_test_output_trc.c (right): https://codereview.chromium.org/1862053002/diff/140001/third_party/qcms/src/t... third_party/qcms/src/tests/qcms_test_output_trc.c:14: #define MAX_FLOAT_ERROR 0.000001f MAX_FLOAT_ERROR is no longer used, right? https://codereview.chromium.org/1862053002/diff/140001/third_party/qcms/src/t... third_party/qcms/src/tests/qcms_test_output_trc.c:46: *size = qcms_transform_get_output_trc_rgba(transform, target, QCMS_TRC_USHORT, NULL); As suggested for the input table, how about we assert(size == 256) here? https://codereview.chromium.org/1862053002/diff/140001/third_party/qcms/src/t... third_party/qcms/src/tests/qcms_test_output_trc.c:51: Remove this line, close up the space. https://codereview.chromium.org/1862053002/diff/140001/third_party/qcms/src/t... third_party/qcms/src/tests/qcms_test_output_trc.c:84: size = qcms_transform_get_input_trc_rgba(transform, source, QCMS_TRC_USHORT, NULL); This comment also applies to the output gamma table (it has a fixed size), but no comment there. Maybe put the comments about the fixed size input and output tables in the the change description? https://codereview.chromium.org/1862053002/diff/140001/third_party/qcms/src/t... third_party/qcms/src/tests/qcms_test_output_trc.c:86: fprintf(stderr, "Expected input table size (%zu) to match output size (%zu)\n", size, expected_size); How about we assert(size == 256); https://codereview.chromium.org/1862053002/diff/140001/third_party/qcms/src/t... third_party/qcms/src/tests/qcms_test_output_trc.c:94: Remove this line, close up the space.
Description was changed from ========== [qcms] Fix build_output_lut to return correct data for parametric curves build_output_lut does not correctly invert a parametric gamma curve. The effect has not been visible so far because Chrome only used gamma values from the precache table (which is inverted). qcms_transform_get_output_trc_rgba, nevertheless returns incorrect data. The current CL adds a test to verify this behavior and attempts to fix the problem. So far I have considered as valid output the values returned from lcms function DefaultEvalParametricFn. For parametric curves both input and output gamma is written to a csv file and approximated using the formula from: http://www.brucelindbloom.com/index.html?Eqn_BestGamma.html BUG=600338 ========== to ========== [qcms] Fix build_output_lut to return correct data for parametric curves build_output_lut() does not correctly invert a parametric gamma curve when computing the output gamma curve data. The effect has not been visible so far because Chrome only uses the output gamma values from the precache table (which is inverted). So qcms_transform_get_output_trc_rgba, which uses build_output_lut(), returns incorrect data for the output curve. Fix that by returning inverted data. Add a test to write the inverted output data and the values from LCMS function DefaultEvalParametricFn, to a CSV file for comparison. BUG=600338 ==========
Description was changed from ========== [qcms] Fix build_output_lut to return correct data for parametric curves build_output_lut() does not correctly invert a parametric gamma curve when computing the output gamma curve data. The effect has not been visible so far because Chrome only uses the output gamma values from the precache table (which is inverted). So qcms_transform_get_output_trc_rgba, which uses build_output_lut(), returns incorrect data for the output curve. Fix that by returning inverted data. Add a test to write the inverted output data and the values from LCMS function DefaultEvalParametricFn, to a CSV file for comparison. BUG=600338 ========== to ========== [qcms] Fix build_output_lut to return correct data for parametric curves build_output_lut() does not correctly invert a parametric gamma curve when computing the output gamma curve data. The effect has not been visible so far because Chrome only uses the output gamma values from the precache table (which is inverted). So qcms_transform_get_output_trc_rgba, which uses build_output_lut(), returns incorrect data for the output curve. Fix that by returning inverted data. Add a test to write the inverted output data and the values from LCMS function DefaultEvalParametricFn, to a CSV file for comparison. BUG=600338 ==========
Description was changed from ========== [qcms] Fix build_output_lut to return correct data for parametric curves build_output_lut() does not correctly invert a parametric gamma curve when computing the output gamma curve data. The effect has not been visible so far because Chrome only uses the output gamma values from the precache table (which is inverted). So qcms_transform_get_output_trc_rgba, which uses build_output_lut(), returns incorrect data for the output curve. Fix that by returning inverted data. Add a test to write the inverted output data and the values from LCMS function DefaultEvalParametricFn, to a CSV file for comparison. BUG=600338 ========== to ========== [qcms] Fix build_output_lut to return correct data for parametric curves build_output_lut() does not correctly invert a parametric gamma curve when computing the output gamma curve data. The effect has not been visible so far because Chrome only uses the output gamma values from the precache table (which is inverted). qcms_transform_get_output_trc_rgba(), which uses build_output_lut(), returns incorrect data for the output curve. Fix that by returning inverted data from build_output_lut(). Add a test to write the inverted output data and the values from LCMS function DefaultEvalParametricFn, to a CSV file for comparison. BUG=600338 ==========
Description was changed from ========== [qcms] Fix build_output_lut to return correct data for parametric curves build_output_lut() does not correctly invert a parametric gamma curve when computing the output gamma curve data. The effect has not been visible so far because Chrome only uses the output gamma values from the precache table (which is inverted). qcms_transform_get_output_trc_rgba(), which uses build_output_lut(), returns incorrect data for the output curve. Fix that by returning inverted data from build_output_lut(). Add a test to write the inverted output data and the values from LCMS function DefaultEvalParametricFn, to a CSV file for comparison. BUG=600338 ========== to ========== [qcms] Fix build_output_lut to return correct data for parametric curves build_output_lut() does not correctly invert a parametric gamma curve when computing the output gamma curve data. The effect has not been visible so far because Chrome only uses the output gamma values from the precache table (which is inverted). Fix that by returning inverted data from build_output_lut(). Add a test to write the inverted output data and the values output from LCMS function DefaultEvalParametricFn to a CSV file for comparison. BUG=600338 ==========
Description was changed from ========== [qcms] Fix build_output_lut to return correct data for parametric curves build_output_lut() does not correctly invert a parametric gamma curve when computing the output gamma curve data. The effect has not been visible so far because Chrome only uses the output gamma values from the precache table (which is inverted). Fix that by returning inverted data from build_output_lut(). Add a test to write the inverted output data and the values output from LCMS function DefaultEvalParametricFn to a CSV file for comparison. BUG=600338 ========== to ========== [qcms] Fix build_output_lut to return correct data for parametric curves build_output_lut() does not correctly invert a parametric gamma curve when computing the output gamma curve data. The effect has not been visible so far because Chrome only uses the output gamma values from the precache table (which is inverted). Fix that by returning inverted data from build_output_lut(). Add a test to write the inverted output data and the values output from LCMS function DefaultEvalParametricFn to a CSV file for comparison. BUG=600338 ==========
Description was changed from ========== [qcms] Fix build_output_lut to return correct data for parametric curves build_output_lut() does not correctly invert a parametric gamma curve when computing the output gamma curve data. The effect has not been visible so far because Chrome only uses the output gamma values from the precache table (which is inverted). Fix that by returning inverted data from build_output_lut(). Add a test to write the inverted output data and the values output from LCMS function DefaultEvalParametricFn to a CSV file for comparison. BUG=600338 ========== to ========== [qcms] Fix build_output_lut to return correct data for parametric curves build_output_lut() does not correctly invert a parametric gamma curve when computing the output gamma curve data. The effect has not been visible so far because Chrome only uses the output gamma values from the precache table (which is inverted). Fix that by returning inverted data from build_output_lut(). Add a test to write the inverted output data and the values output from LCMS function DefaultEvalParametricFn to a file for comparison. BUG=600338 ==========
Description was changed from ========== [qcms] Fix build_output_lut to return correct data for parametric curves build_output_lut() does not correctly invert a parametric gamma curve when computing the output gamma curve data. The effect has not been visible so far because Chrome only uses the output gamma values from the precache table (which is inverted). Fix that by returning inverted data from build_output_lut(). Add a test to write the inverted output data and the values output from LCMS function DefaultEvalParametricFn to a file for comparison. BUG=600338 ========== to ========== [qcms] Fix build_output_lut to return correct data for parametric curves build_output_lut() does not invert a parametric gamma curve when computing the output curve data. The effect has not been visible since Chrome only uses the output gamma values from the precache table (which is inverted). Make build_output_lut() return inverted data for parmatric gamma curves. Add a test to write the inverted data, and the output of LCMS function DefaultEvalParametricFn, to a file for comparison. BUG=600338 ==========
Description was changed from ========== [qcms] Fix build_output_lut to return correct data for parametric curves build_output_lut() does not invert a parametric gamma curve when computing the output curve data. The effect has not been visible since Chrome only uses the output gamma values from the precache table (which is inverted). Make build_output_lut() return inverted data for parmatric gamma curves. Add a test to write the inverted data, and the output of LCMS function DefaultEvalParametricFn, to a file for comparison. BUG=600338 ========== to ========== [qcms] Fix build_output_lut to return correct data for parametric curves build_output_lut() does not invert a parametric gamma curve when computing the output curve data. The effect has not been visible since Chrome only uses the output gamma values from the precache table (which is inverted). Make build_output_lut() return inverted data for parametric gamma curves. Add a test to write the inverted data, and the output of LCMS function DefaultEvalParametricFn, to a file for comparison. BUG=600338 ==========
Description was changed from ========== [qcms] Fix build_output_lut to return correct data for parametric curves build_output_lut() does not invert a parametric gamma curve when computing the output curve data. The effect has not been visible since Chrome only uses the output gamma values from the precache table (which is inverted). Make build_output_lut() return inverted data for parametric gamma curves. Add a test to write the inverted data, and the output of LCMS function DefaultEvalParametricFn, to a file for comparison. BUG=600338 ========== to ========== [qcms] Fix build_output_lut to return correct data for parametric curves build_output_lut() does not invert a parametric gamma curve when computing the output curve data. The effect has not been visible since Chrome only uses the output gamma values from the precache table (which is inverted). Make build_output_lut() return inverted data for parametric gamma curves. Add a test to write the inverted data, and the output of LCMS function DefaultEvalParametricFn, to a file for comparison. For now the size of input/output gamma table for parametric curves is hardcoded to 256. We are asserting this throughout the test code. See compute_curve_gamma_table_type_parametric for details. Future implementations might decide to return an arbitrary-sized table. BUG=600338 ==========
Description was changed from ========== [qcms] Fix build_output_lut to return correct data for parametric curves build_output_lut() does not invert a parametric gamma curve when computing the output curve data. The effect has not been visible since Chrome only uses the output gamma values from the precache table (which is inverted). Make build_output_lut() return inverted data for parametric gamma curves. Add a test to write the inverted data, and the output of LCMS function DefaultEvalParametricFn, to a file for comparison. For now the size of input/output gamma table for parametric curves is hardcoded to 256. We are asserting this throughout the test code. See compute_curve_gamma_table_type_parametric for details. Future implementations might decide to return an arbitrary-sized table. BUG=600338 ========== to ========== [qcms] Fix build_output_lut to return correct data for parametric curves build_output_lut() does not invert a parametric gamma curve when computing the output curve data. The effect has not been visible since Chrome only uses the output gamma values from the precache table (which is inverted). Make build_output_lut() return inverted data for parametric gamma curves. Add a test to write the inverted data, and the output of LCMS function DefaultEvalParametricFn, to a file for comparison. For now the size of input/output gamma table for parametric curves is hardcoded to 256. We are asserting this throughout the test code. See compute_curve_gamma_table_type_parametric for details. Future implementations might decide to return an arbitrary-sized table. BUG=600338 ==========
https://codereview.chromium.org/1862053002/diff/120001/third_party/qcms/src/t... File third_party/qcms/src/tests/qcms_test_output_trc.c (right): https://codereview.chromium.org/1862053002/diff/120001/third_party/qcms/src/t... third_party/qcms/src/tests/qcms_test_output_trc.c:138: if (gamma_table_size != gamma_table_size_precache) { On 2016/04/21 06:01:53, noel gordon wrote: > but as mentioned above, why do the precache vs non-precache cases need to be > separately tested. Done. https://codereview.chromium.org/1862053002/diff/140001/third_party/qcms/src/t... File third_party/qcms/src/tests/qcms_test_output_trc.c (right): https://codereview.chromium.org/1862053002/diff/140001/third_party/qcms/src/t... third_party/qcms/src/tests/qcms_test_output_trc.c:14: #define MAX_FLOAT_ERROR 0.000001f On 2016/04/26 02:46:39, noel gordon wrote: > MAX_FLOAT_ERROR is no longer used, right? Done. https://codereview.chromium.org/1862053002/diff/140001/third_party/qcms/src/t... third_party/qcms/src/tests/qcms_test_output_trc.c:46: *size = qcms_transform_get_output_trc_rgba(transform, target, QCMS_TRC_USHORT, NULL); On 2016/04/26 02:46:39, noel gordon wrote: > As suggested for the input table, how about we assert(size == 256) here? Done. https://codereview.chromium.org/1862053002/diff/140001/third_party/qcms/src/t... third_party/qcms/src/tests/qcms_test_output_trc.c:51: On 2016/04/26 02:46:39, noel gordon wrote: > Remove this line, close up the space. Done. https://codereview.chromium.org/1862053002/diff/140001/third_party/qcms/src/t... third_party/qcms/src/tests/qcms_test_output_trc.c:84: size = qcms_transform_get_input_trc_rgba(transform, source, QCMS_TRC_USHORT, NULL); On 2016/04/26 02:46:39, noel gordon wrote: > This comment also applies to the output gamma table (it has a fixed size), but > no comment there. Maybe put the comments about the fixed size input and output > tables in the the change description? Done. https://codereview.chromium.org/1862053002/diff/140001/third_party/qcms/src/t... third_party/qcms/src/tests/qcms_test_output_trc.c:86: fprintf(stderr, "Expected input table size (%zu) to match output size (%zu)\n", size, expected_size); On 2016/04/26 02:46:39, noel gordon wrote: > How about we assert(size == 256); Done. https://codereview.chromium.org/1862053002/diff/140001/third_party/qcms/src/t... third_party/qcms/src/tests/qcms_test_output_trc.c:94: On 2016/04/26 02:46:39, noel gordon wrote: > Remove this line, close up the space. Done.
Description was changed from ========== [qcms] Fix build_output_lut to return correct data for parametric curves build_output_lut() does not invert a parametric gamma curve when computing the output curve data. The effect has not been visible since Chrome only uses the output gamma values from the precache table (which is inverted). Make build_output_lut() return inverted data for parametric gamma curves. Add a test to write the inverted data, and the output of LCMS function DefaultEvalParametricFn, to a file for comparison. For now the size of input/output gamma table for parametric curves is hardcoded to 256. We are asserting this throughout the test code. See compute_curve_gamma_table_type_parametric for details. Future implementations might decide to return an arbitrary-sized table. BUG=600338 ========== to ========== [qcms] Fix build_output_lut to return correct data for parametric curves build_output_lut() does not invert a parametric gamma curve when computing the output curve data. The effect has not been visible since Chrome only uses the output gamma values from the precache table (which is inverted). Make build_output_lut() return inverted data for parametric gamma curves. Add a test to write the inverted data, and the output of LCMS function DefaultEvalParametricFn, to a file for comparison. For now the size of input/output gamma table for parametric curves is hardcoded to 256, and assert this limit in the test code. See compute_curve_gamma_table_type_parametric for details. Future implementations might decide to return an arbitrary-sized table. BUG=600338 ==========
https://codereview.chromium.org/1862053002/diff/160001/third_party/qcms/src/t... File third_party/qcms/src/tests/qcms_test_output_trc.c (right): https://codereview.chromium.org/1862053002/diff/160001/third_party/qcms/src/t... third_party/qcms/src/tests/qcms_test_output_trc.c:123: if (err != 0) { You can remove this too I think, it's never anything but 0. https://codereview.chromium.org/1862053002/diff/160001/third_party/qcms/src/t... third_party/qcms/src/tests/qcms_test_output_trc.c:200: return err; return 0? here ?
https://codereview.chromium.org/1862053002/diff/160001/third_party/qcms/src/t... File third_party/qcms/src/tests/qcms_test_output_trc.c (right): https://codereview.chromium.org/1862053002/diff/160001/third_party/qcms/src/t... third_party/qcms/src/tests/qcms_test_output_trc.c:123: if (err != 0) { On 2016/04/26 10:32:15, noel gordon wrote: > You can remove this too I think, it's never anything but 0. Done. https://codereview.chromium.org/1862053002/diff/160001/third_party/qcms/src/t... third_party/qcms/src/tests/qcms_test_output_trc.c:200: return err; On 2016/04/26 10:32:15, noel gordon wrote: > return 0? here ? Done.
Final nits. https://codereview.chromium.org/1862053002/diff/200001/third_party/qcms/src/t... File third_party/qcms/src/tests/qcms_test_output_trc.c (right): https://codereview.chromium.org/1862053002/diff/200001/third_party/qcms/src/t... third_party/qcms/src/tests/qcms_test_output_trc.c:44: *table = malloc(*size * sizeof(uint16_t) * 4); Add a line before this to match what you did in get_input_gamma_tabl(). https://codereview.chromium.org/1862053002/diff/200001/third_party/qcms/src/t... third_party/qcms/src/tests/qcms_test_output_trc.c:187: Nit: remove this line.
Description was changed from ========== [qcms] Fix build_output_lut to return correct data for parametric curves build_output_lut() does not invert a parametric gamma curve when computing the output curve data. The effect has not been visible since Chrome only uses the output gamma values from the precache table (which is inverted). Make build_output_lut() return inverted data for parametric gamma curves. Add a test to write the inverted data, and the output of LCMS function DefaultEvalParametricFn, to a file for comparison. For now the size of input/output gamma table for parametric curves is hardcoded to 256, and assert this limit in the test code. See compute_curve_gamma_table_type_parametric for details. Future implementations might decide to return an arbitrary-sized table. BUG=600338 ========== to ========== [qcms] Fix build_output_lut to return correct data for parametric curves build_output_lut() does not invert a parametric gamma curve when computing the output curve data. The effect has not been visible since Chrome only uses the output gamma values from the precache table (which is inverted). Make build_output_lut() return inverted data for parametric gamma curves. Add a test to write the inverted data, and the output of LCMS function DefaultEvalParametricFn, to a file for comparison. For now the size of input and output gamma table for parametric curves is hard-coded to 256; assert this in the test code. See compute_curve_gamma_table_type_parametric for details. Future implementations might decide to return an arbitrary-sized table. BUG=600338 ==========
https://codereview.chromium.org/1862053002/diff/200001/third_party/qcms/src/t... File third_party/qcms/src/tests/qcms_test_output_trc.c (right): https://codereview.chromium.org/1862053002/diff/200001/third_party/qcms/src/t... third_party/qcms/src/tests/qcms_test_output_trc.c:44: *table = malloc(*size * sizeof(uint16_t) * 4); On 2016/04/26 10:44:19, noel gordon wrote: > Add a line before this to match what you did in get_input_gamma_tabl(). Done. https://codereview.chromium.org/1862053002/diff/200001/third_party/qcms/src/t... third_party/qcms/src/tests/qcms_test_output_trc.c:187: On 2016/04/26 10:44:19, noel gordon wrote: > Nit: remove this line. Done.
Description was changed from ========== [qcms] Fix build_output_lut to return correct data for parametric curves build_output_lut() does not invert a parametric gamma curve when computing the output curve data. The effect has not been visible since Chrome only uses the output gamma values from the precache table (which is inverted). Make build_output_lut() return inverted data for parametric gamma curves. Add a test to write the inverted data, and the output of LCMS function DefaultEvalParametricFn, to a file for comparison. For now the size of input and output gamma table for parametric curves is hard-coded to 256; assert this in the test code. See compute_curve_gamma_table_type_parametric for details. Future implementations might decide to return an arbitrary-sized table. BUG=600338 ========== to ========== [qcms] Fix build_output_lut to return correct data for parametric curves build_output_lut() does not invert a parametric gamma curve when computing the output curve data. The effect has not been visible since Chrome only uses the output gamma values from the precache table (which is inverted). Make build_output_lut() return inverted data for parametric gamma curves. Add a test to write the inverted data, and the output of LCMS function DefaultEvalParametricFn, to a file for comparison. For now the size of input and output gamma table for parametric curves is hard-coded to 256; assert this in the test code. See compute_curve_gamma_table_type_parametric for details. Future implementations might return an arbitrary-sized table. BUG=600338 ==========
LGTM.
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/1862053002/240001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1862053002/240001
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
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1862053002/240001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1862053002/240001
Message was sent while issue was closed.
Description was changed from ========== [qcms] Fix build_output_lut to return correct data for parametric curves build_output_lut() does not invert a parametric gamma curve when computing the output curve data. The effect has not been visible since Chrome only uses the output gamma values from the precache table (which is inverted). Make build_output_lut() return inverted data for parametric gamma curves. Add a test to write the inverted data, and the output of LCMS function DefaultEvalParametricFn, to a file for comparison. For now the size of input and output gamma table for parametric curves is hard-coded to 256; assert this in the test code. See compute_curve_gamma_table_type_parametric for details. Future implementations might return an arbitrary-sized table. BUG=600338 ========== to ========== [qcms] Fix build_output_lut to return correct data for parametric curves build_output_lut() does not invert a parametric gamma curve when computing the output curve data. The effect has not been visible since Chrome only uses the output gamma values from the precache table (which is inverted). Make build_output_lut() return inverted data for parametric gamma curves. Add a test to write the inverted data, and the output of LCMS function DefaultEvalParametricFn, to a file for comparison. For now the size of input and output gamma table for parametric curves is hard-coded to 256; assert this in the test code. See compute_curve_gamma_table_type_parametric for details. Future implementations might return an arbitrary-sized table. BUG=600338 ==========
Message was sent while issue was closed.
Committed patchset #13 (id:240001)
Message was sent while issue was closed.
Description was changed from ========== [qcms] Fix build_output_lut to return correct data for parametric curves build_output_lut() does not invert a parametric gamma curve when computing the output curve data. The effect has not been visible since Chrome only uses the output gamma values from the precache table (which is inverted). Make build_output_lut() return inverted data for parametric gamma curves. Add a test to write the inverted data, and the output of LCMS function DefaultEvalParametricFn, to a file for comparison. For now the size of input and output gamma table for parametric curves is hard-coded to 256; assert this in the test code. See compute_curve_gamma_table_type_parametric for details. Future implementations might return an arbitrary-sized table. BUG=600338 ========== to ========== [qcms] Fix build_output_lut to return correct data for parametric curves build_output_lut() does not invert a parametric gamma curve when computing the output curve data. The effect has not been visible since Chrome only uses the output gamma values from the precache table (which is inverted). Make build_output_lut() return inverted data for parametric gamma curves. Add a test to write the inverted data, and the output of LCMS function DefaultEvalParametricFn, to a file for comparison. For now the size of input and output gamma table for parametric curves is hard-coded to 256; assert this in the test code. See compute_curve_gamma_table_type_parametric for details. Future implementations might return an arbitrary-sized table. BUG=600338 Committed: https://crrev.com/a89370950f514a3df77ed84240d4b8e3abbee801 Cr-Commit-Position: refs/heads/master@{#389754} ==========
Message was sent while issue was closed.
Patchset 13 (id:??) landed as https://crrev.com/a89370950f514a3df77ed84240d4b8e3abbee801 Cr-Commit-Position: refs/heads/master@{#389754}
Message was sent while issue was closed.
A revert of this CL (patchset #13 id:240001) has been created in https://codereview.chromium.org/1922113002/ by treib@chromium.org. The reason for reverting is: Breaks the build on Windows: https://build.chromium.org/p/chromium/builders/Win%20x64/builds/123.
Message was sent while issue was closed.
On 2016/04/26 13:06:06, Marc Treib wrote: > A revert of this CL (patchset #13 id:240001) has been created in > https://codereview.chromium.org/1922113002/ by mailto:treib@chromium.org. > > The reason for reverting is: Breaks the build on Windows: > https://build.chromium.org/p/chromium/builders/Win%20x64/builds/123. c:\b\build\slave\win_x64\build\src\third_party\qcms\src\tests\qcms_test_output_trc.c(129): warning C4146: unary minus operator applied to unsigned type, result still unsigned ninja: build stopped: subcommand failed. Radu can you fix please?
Message was sent while issue was closed.
Description was changed from ========== [qcms] Fix build_output_lut to return correct data for parametric curves build_output_lut() does not invert a parametric gamma curve when computing the output curve data. The effect has not been visible since Chrome only uses the output gamma values from the precache table (which is inverted). Make build_output_lut() return inverted data for parametric gamma curves. Add a test to write the inverted data, and the output of LCMS function DefaultEvalParametricFn, to a file for comparison. For now the size of input and output gamma table for parametric curves is hard-coded to 256; assert this in the test code. See compute_curve_gamma_table_type_parametric for details. Future implementations might return an arbitrary-sized table. BUG=600338 Committed: https://crrev.com/a89370950f514a3df77ed84240d4b8e3abbee801 Cr-Commit-Position: refs/heads/master@{#389754} ========== to ========== [qcms] Fix build_output_lut to return correct data for parametric curves build_output_lut() does not invert a parametric gamma curve when computing the output curve data. The effect has not been visible since Chrome only uses the output gamma values from the precache table (which is inverted). Make build_output_lut() return inverted data for parametric gamma curves. Add a test to write the inverted data, and the output of LCMS function DefaultEvalParametricFn, to a file for comparison. For now the size of input and output gamma table for parametric curves is hard-coded to 256; assert this in the test code. See compute_curve_gamma_table_type_parametric for details. Future implementations might return an arbitrary-sized table. BUG=600338 Committed: https://crrev.com/a89370950f514a3df77ed84240d4b8e3abbee801 Cr-Commit-Position: refs/heads/master@{#389754} ==========
On 2016/04/26 14:15:20, noel gordon wrote: > On 2016/04/26 13:06:06, Marc Treib wrote: > > A revert of this CL (patchset #13 id:240001) has been created in > > https://codereview.chromium.org/1922113002/ by mailto:treib@chromium.org. > > > > The reason for reverting is: Breaks the build on Windows: > > https://build.chromium.org/p/chromium/builders/Win%20x64/builds/123. > > c:\b\build\slave\win_x64\build\src\third_party\qcms\src\tests\qcms_test_output_trc.c(129): > warning C4146: unary minus operator applied to unsigned type, result still > unsigned > ninja: build stopped: subcommand failed. > > Radu can you fix please? Done
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/1862053002/#ps260001 (title: "Cast unsigned to int before negating to remove warning on windows")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1862053002/260001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1862053002/260001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_android_rel_ng on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by radu.velea@intel.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1862053002/260001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1862053002/260001
Message was sent while issue was closed.
Description was changed from ========== [qcms] Fix build_output_lut to return correct data for parametric curves build_output_lut() does not invert a parametric gamma curve when computing the output curve data. The effect has not been visible since Chrome only uses the output gamma values from the precache table (which is inverted). Make build_output_lut() return inverted data for parametric gamma curves. Add a test to write the inverted data, and the output of LCMS function DefaultEvalParametricFn, to a file for comparison. For now the size of input and output gamma table for parametric curves is hard-coded to 256; assert this in the test code. See compute_curve_gamma_table_type_parametric for details. Future implementations might return an arbitrary-sized table. BUG=600338 Committed: https://crrev.com/a89370950f514a3df77ed84240d4b8e3abbee801 Cr-Commit-Position: refs/heads/master@{#389754} ========== to ========== [qcms] Fix build_output_lut to return correct data for parametric curves build_output_lut() does not invert a parametric gamma curve when computing the output curve data. The effect has not been visible since Chrome only uses the output gamma values from the precache table (which is inverted). Make build_output_lut() return inverted data for parametric gamma curves. Add a test to write the inverted data, and the output of LCMS function DefaultEvalParametricFn, to a file for comparison. For now the size of input and output gamma table for parametric curves is hard-coded to 256; assert this in the test code. See compute_curve_gamma_table_type_parametric for details. Future implementations might return an arbitrary-sized table. BUG=600338 Committed: https://crrev.com/a89370950f514a3df77ed84240d4b8e3abbee801 Cr-Commit-Position: refs/heads/master@{#389754} ==========
Message was sent while issue was closed.
Committed patchset #14 (id:260001)
Message was sent while issue was closed.
Description was changed from ========== [qcms] Fix build_output_lut to return correct data for parametric curves build_output_lut() does not invert a parametric gamma curve when computing the output curve data. The effect has not been visible since Chrome only uses the output gamma values from the precache table (which is inverted). Make build_output_lut() return inverted data for parametric gamma curves. Add a test to write the inverted data, and the output of LCMS function DefaultEvalParametricFn, to a file for comparison. For now the size of input and output gamma table for parametric curves is hard-coded to 256; assert this in the test code. See compute_curve_gamma_table_type_parametric for details. Future implementations might return an arbitrary-sized table. BUG=600338 Committed: https://crrev.com/a89370950f514a3df77ed84240d4b8e3abbee801 Cr-Commit-Position: refs/heads/master@{#389754} ========== to ========== [qcms] Fix build_output_lut to return correct data for parametric curves build_output_lut() does not invert a parametric gamma curve when computing the output curve data. The effect has not been visible since Chrome only uses the output gamma values from the precache table (which is inverted). Make build_output_lut() return inverted data for parametric gamma curves. Add a test to write the inverted data, and the output of LCMS function DefaultEvalParametricFn, to a file for comparison. For now the size of input and output gamma table for parametric curves is hard-coded to 256; assert this in the test code. See compute_curve_gamma_table_type_parametric for details. Future implementations might return an arbitrary-sized table. BUG=600338 Committed: https://crrev.com/a89370950f514a3df77ed84240d4b8e3abbee801 Cr-Commit-Position: refs/heads/master@{#389754} Committed: https://crrev.com/3a4a913c1de915ef228a35b4f85603662719c49f Cr-Commit-Position: refs/heads/master@{#389866} ==========
Message was sent while issue was closed.
Patchset 14 (id:??) landed as https://crrev.com/3a4a913c1de915ef228a35b4f85603662719c49f Cr-Commit-Position: refs/heads/master@{#389866}
Message was sent while issue was closed.
A revert of this CL (patchset #14 id:260001) has been created in https://codereview.chromium.org/1920253003/ by creis@chromium.org. The reason for reverting is: This broke the Windows compile (again). https://build.chromium.org/p/chromium/builders/Win/builds/42772.
Message was sent while issue was closed.
On 2016/04/26 20:54:58, Charlie Reis wrote: > A revert of this CL (patchset #14 id:260001) has been created in > https://codereview.chromium.org/1920253003/ by mailto:creis@chromium.org. > > The reason for reverting is: This broke the Windows compile (again). > https://build.chromium.org/p/chromium/builders/Win/builds/42772. This time the failure is: c:\b\build\slave\win\build\src\third_party\qcms\src\tests\qcms_test_output_trc.c(170): warning C4018: '<': signed/unsigned mismatch ninja: build stopped: subcommand failed. Unclear to me why the try bots aren't catching these compile errors.
Message was sent while issue was closed.
> Unclear to me why the try bots aren't catching these compile errors. Unclear to me also. Anyway, I patched in patchset #14 locally and built target qcms_tests on Windows and reproduced the error reported in #64 ... c:\src\chrome\src\third_party\qcms\src\tests\qcms_test_output_trc.c(170): warning C4018: '<': signed/unsigned mismatch ninja: build stopped: subcommand failed. Next, I patched in the the fix in patchset #15 and built locally again. No errors.
Message was sent while issue was closed.
Description was changed from ========== [qcms] Fix build_output_lut to return correct data for parametric curves build_output_lut() does not invert a parametric gamma curve when computing the output curve data. The effect has not been visible since Chrome only uses the output gamma values from the precache table (which is inverted). Make build_output_lut() return inverted data for parametric gamma curves. Add a test to write the inverted data, and the output of LCMS function DefaultEvalParametricFn, to a file for comparison. For now the size of input and output gamma table for parametric curves is hard-coded to 256; assert this in the test code. See compute_curve_gamma_table_type_parametric for details. Future implementations might return an arbitrary-sized table. BUG=600338 Committed: https://crrev.com/a89370950f514a3df77ed84240d4b8e3abbee801 Cr-Commit-Position: refs/heads/master@{#389754} Committed: https://crrev.com/3a4a913c1de915ef228a35b4f85603662719c49f Cr-Commit-Position: refs/heads/master@{#389866} ========== to ========== [qcms] Fix build_output_lut to return correct data for parametric curves build_output_lut() does not invert a parametric gamma curve when computing the output curve data. The effect has not been visible since Chrome only uses the output gamma values from the precache table (which is inverted). Make build_output_lut() return inverted data for parametric gamma curves. Add a test to write the inverted data, and the output of LCMS function DefaultEvalParametricFn, to a file for comparison. For now the size of input and output gamma table for parametric curves is hard-coded to 256; assert this in the test code. See compute_curve_gamma_table_type_parametric for details. Future implementations might return an arbitrary-sized table. BUG=600338 Committed: https://crrev.com/a89370950f514a3df77ed84240d4b8e3abbee801 Cr-Commit-Position: refs/heads/master@{#389754} Committed: https://crrev.com/3a4a913c1de915ef228a35b4f85603662719c49f Cr-Commit-Position: refs/heads/master@{#389866} ==========
https://codereview.chromium.org/1862053002/diff/280001/third_party/qcms/src/t... File third_party/qcms/src/tests/qcms_test_output_trc.c (right): https://codereview.chromium.org/1862053002/diff/280001/third_party/qcms/src/t... third_party/qcms/src/tests/qcms_test_output_trc.c:100: size_t i; LGTM based on my local windows compile testing.
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/1862053002/280001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1862053002/280001
Message was sent while issue was closed.
Description was changed from ========== [qcms] Fix build_output_lut to return correct data for parametric curves build_output_lut() does not invert a parametric gamma curve when computing the output curve data. The effect has not been visible since Chrome only uses the output gamma values from the precache table (which is inverted). Make build_output_lut() return inverted data for parametric gamma curves. Add a test to write the inverted data, and the output of LCMS function DefaultEvalParametricFn, to a file for comparison. For now the size of input and output gamma table for parametric curves is hard-coded to 256; assert this in the test code. See compute_curve_gamma_table_type_parametric for details. Future implementations might return an arbitrary-sized table. BUG=600338 Committed: https://crrev.com/a89370950f514a3df77ed84240d4b8e3abbee801 Cr-Commit-Position: refs/heads/master@{#389754} Committed: https://crrev.com/3a4a913c1de915ef228a35b4f85603662719c49f Cr-Commit-Position: refs/heads/master@{#389866} ========== to ========== [qcms] Fix build_output_lut to return correct data for parametric curves build_output_lut() does not invert a parametric gamma curve when computing the output curve data. The effect has not been visible since Chrome only uses the output gamma values from the precache table (which is inverted). Make build_output_lut() return inverted data for parametric gamma curves. Add a test to write the inverted data, and the output of LCMS function DefaultEvalParametricFn, to a file for comparison. For now the size of input and output gamma table for parametric curves is hard-coded to 256; assert this in the test code. See compute_curve_gamma_table_type_parametric for details. Future implementations might return an arbitrary-sized table. BUG=600338 Committed: https://crrev.com/a89370950f514a3df77ed84240d4b8e3abbee801 Cr-Commit-Position: refs/heads/master@{#389754} Committed: https://crrev.com/3a4a913c1de915ef228a35b4f85603662719c49f Cr-Commit-Position: refs/heads/master@{#389866} ==========
Message was sent while issue was closed.
Committed patchset #15 (id:280001)
Message was sent while issue was closed.
Description was changed from ========== [qcms] Fix build_output_lut to return correct data for parametric curves build_output_lut() does not invert a parametric gamma curve when computing the output curve data. The effect has not been visible since Chrome only uses the output gamma values from the precache table (which is inverted). Make build_output_lut() return inverted data for parametric gamma curves. Add a test to write the inverted data, and the output of LCMS function DefaultEvalParametricFn, to a file for comparison. For now the size of input and output gamma table for parametric curves is hard-coded to 256; assert this in the test code. See compute_curve_gamma_table_type_parametric for details. Future implementations might return an arbitrary-sized table. BUG=600338 Committed: https://crrev.com/a89370950f514a3df77ed84240d4b8e3abbee801 Cr-Commit-Position: refs/heads/master@{#389754} Committed: https://crrev.com/3a4a913c1de915ef228a35b4f85603662719c49f Cr-Commit-Position: refs/heads/master@{#389866} ========== to ========== [qcms] Fix build_output_lut to return correct data for parametric curves build_output_lut() does not invert a parametric gamma curve when computing the output curve data. The effect has not been visible since Chrome only uses the output gamma values from the precache table (which is inverted). Make build_output_lut() return inverted data for parametric gamma curves. Add a test to write the inverted data, and the output of LCMS function DefaultEvalParametricFn, to a file for comparison. For now the size of input and output gamma table for parametric curves is hard-coded to 256; assert this in the test code. See compute_curve_gamma_table_type_parametric for details. Future implementations might return an arbitrary-sized table. BUG=600338 Committed: https://crrev.com/a89370950f514a3df77ed84240d4b8e3abbee801 Cr-Commit-Position: refs/heads/master@{#389754} Committed: https://crrev.com/3a4a913c1de915ef228a35b4f85603662719c49f Cr-Commit-Position: refs/heads/master@{#389866} Committed: https://crrev.com/555dfdfcd56ae8ef91637e4b0827970be7609059 Cr-Commit-Position: refs/heads/master@{#389964} ==========
Message was sent while issue was closed.
Patchset 15 (id:??) landed as https://crrev.com/555dfdfcd56ae8ef91637e4b0827970be7609059 Cr-Commit-Position: refs/heads/master@{#389964} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
