[qcms] qcms_transform_get_output_trc_rgba is not correct for non-precached transforms using parametric curves
Reported by
radu.ve...@intel.com,
Apr 4 2016
|
||||||||
Issue descriptionVersion: All OS: All What steps will reproduce the problem? (1) Create a transform object with output ICC profile containing a parametric curve (2.1) Call qcms_transform_get_output_trc_rgba or and save table (2.2) Alternativly call build_output_lut for the parametric curves of the output ICC profile (3) Compare output with similar transform after precaching output or LCMS equivalent code What is the expected output? The gamma tables should match. They do not. What do you see instead? build_output_lut does not invert the gamma curve when type is parametric. Please use labels and text to provide additional information.
,
Apr 6 2016
I've plotted the current values returned by build_output_lut: https://docs.google.com/spreadsheets/d/1H0LJSxgRNdKGyHcPjCoYUJOrHmTo0lJyo38Bd9Y7_Ew/ I've compared them with the values from LCMS and invert_lut: https://codereview.chromium.org/1862053002/ Attaching 3 custom icc profiles I used to gather this data.
,
Apr 26 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/a89370950f514a3df77ed84240d4b8e3abbee801 commit a89370950f514a3df77ed84240d4b8e3abbee801 Author: radu.velea <radu.velea@intel.com> Date: Tue Apr 26 12:36:52 2016 [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 Review URL: https://codereview.chromium.org/1862053002 Cr-Commit-Position: refs/heads/master@{#389754} [modify] https://crrev.com/a89370950f514a3df77ed84240d4b8e3abbee801/third_party/qcms/BUILD.gn [modify] https://crrev.com/a89370950f514a3df77ed84240d4b8e3abbee801/third_party/qcms/README.chromium [modify] https://crrev.com/a89370950f514a3df77ed84240d4b8e3abbee801/third_party/qcms/qcms.gyp [modify] https://crrev.com/a89370950f514a3df77ed84240d4b8e3abbee801/third_party/qcms/src/tests/Makefile [modify] https://crrev.com/a89370950f514a3df77ed84240d4b8e3abbee801/third_party/qcms/src/tests/qcms_test_main.c [add] https://crrev.com/a89370950f514a3df77ed84240d4b8e3abbee801/third_party/qcms/src/tests/qcms_test_output_trc.c [add] https://crrev.com/a89370950f514a3df77ed84240d4b8e3abbee801/third_party/qcms/src/tests/qcms_test_util.c [modify] https://crrev.com/a89370950f514a3df77ed84240d4b8e3abbee801/third_party/qcms/src/tests/qcms_test_util.h [modify] https://crrev.com/a89370950f514a3df77ed84240d4b8e3abbee801/third_party/qcms/src/transform_util.c
,
Apr 26 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/7b4905416ab871fe374f8879f5e13d4a50d0da6f commit 7b4905416ab871fe374f8879f5e13d4a50d0da6f Author: treib <treib@chromium.org> Date: Tue Apr 26 13:06:48 2016 Revert of [qcms] Fix build_output_lut to return correct data for parametric curves (patchset #13 id:240001 of https://codereview.chromium.org/1862053002/ ) Reason for revert: Breaks the build on Windows: https://build.chromium.org/p/chromium/builders/Win%20x64/builds/123 Original issue's 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} TBR=noel@chromium.org,radu.velea@intel.com # Skipping CQ checks because original CL landed less than 1 days ago. NOPRESUBMIT=true NOTREECHECKS=true NOTRY=true BUG= 600338 Review URL: https://codereview.chromium.org/1922113002 Cr-Commit-Position: refs/heads/master@{#389762} [modify] https://crrev.com/7b4905416ab871fe374f8879f5e13d4a50d0da6f/third_party/qcms/BUILD.gn [modify] https://crrev.com/7b4905416ab871fe374f8879f5e13d4a50d0da6f/third_party/qcms/README.chromium [modify] https://crrev.com/7b4905416ab871fe374f8879f5e13d4a50d0da6f/third_party/qcms/qcms.gyp [modify] https://crrev.com/7b4905416ab871fe374f8879f5e13d4a50d0da6f/third_party/qcms/src/tests/Makefile [modify] https://crrev.com/7b4905416ab871fe374f8879f5e13d4a50d0da6f/third_party/qcms/src/tests/qcms_test_main.c [delete] https://crrev.com/8473ba8f97e28a02529343c9e4d1934c9054b94d/third_party/qcms/src/tests/qcms_test_output_trc.c [delete] https://crrev.com/8473ba8f97e28a02529343c9e4d1934c9054b94d/third_party/qcms/src/tests/qcms_test_util.c [modify] https://crrev.com/7b4905416ab871fe374f8879f5e13d4a50d0da6f/third_party/qcms/src/tests/qcms_test_util.h [modify] https://crrev.com/7b4905416ab871fe374f8879f5e13d4a50d0da6f/third_party/qcms/src/transform_util.c
,
Apr 26 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/3a4a913c1de915ef228a35b4f85603662719c49f commit 3a4a913c1de915ef228a35b4f85603662719c49f Author: radu.velea <radu.velea@intel.com> Date: Tue Apr 26 20:11:34 2016 [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} Review URL: https://codereview.chromium.org/1862053002 Cr-Commit-Position: refs/heads/master@{#389866} [modify] https://crrev.com/3a4a913c1de915ef228a35b4f85603662719c49f/third_party/qcms/BUILD.gn [modify] https://crrev.com/3a4a913c1de915ef228a35b4f85603662719c49f/third_party/qcms/README.chromium [modify] https://crrev.com/3a4a913c1de915ef228a35b4f85603662719c49f/third_party/qcms/qcms.gyp [modify] https://crrev.com/3a4a913c1de915ef228a35b4f85603662719c49f/third_party/qcms/src/tests/Makefile [modify] https://crrev.com/3a4a913c1de915ef228a35b4f85603662719c49f/third_party/qcms/src/tests/qcms_test_main.c [add] https://crrev.com/3a4a913c1de915ef228a35b4f85603662719c49f/third_party/qcms/src/tests/qcms_test_output_trc.c [add] https://crrev.com/3a4a913c1de915ef228a35b4f85603662719c49f/third_party/qcms/src/tests/qcms_test_util.c [modify] https://crrev.com/3a4a913c1de915ef228a35b4f85603662719c49f/third_party/qcms/src/tests/qcms_test_util.h [modify] https://crrev.com/3a4a913c1de915ef228a35b4f85603662719c49f/third_party/qcms/src/transform_util.c
,
Apr 26 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/517ca6467545ddd4e0281f3302d65871f4164603 commit 517ca6467545ddd4e0281f3302d65871f4164603 Author: creis <creis@chromium.org> Date: Tue Apr 26 20:55:55 2016 Revert of Reland: [qcms] Fix build_output_lut to return correct data for parametric curves (patchset #14 id:260001 of https://codereview.chromium.org/1862053002/ ) Reason for revert: This broke the Windows compile (again). https://build.chromium.org/p/chromium/builders/Win/builds/42772 Original issue's 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} TBR=noel@chromium.org,radu.velea@intel.com # Skipping CQ checks because original CL landed less than 1 days ago. NOPRESUBMIT=true NOTREECHECKS=true NOTRY=true BUG= 600338 Review URL: https://codereview.chromium.org/1920253003 Cr-Commit-Position: refs/heads/master@{#389877} [modify] https://crrev.com/517ca6467545ddd4e0281f3302d65871f4164603/third_party/qcms/BUILD.gn [modify] https://crrev.com/517ca6467545ddd4e0281f3302d65871f4164603/third_party/qcms/README.chromium [modify] https://crrev.com/517ca6467545ddd4e0281f3302d65871f4164603/third_party/qcms/qcms.gyp [modify] https://crrev.com/517ca6467545ddd4e0281f3302d65871f4164603/third_party/qcms/src/tests/Makefile [modify] https://crrev.com/517ca6467545ddd4e0281f3302d65871f4164603/third_party/qcms/src/tests/qcms_test_main.c [delete] https://crrev.com/ac7bdb6c9ea312abc3abc3c251c44598a9a158d1/third_party/qcms/src/tests/qcms_test_output_trc.c [delete] https://crrev.com/ac7bdb6c9ea312abc3abc3c251c44598a9a158d1/third_party/qcms/src/tests/qcms_test_util.c [modify] https://crrev.com/517ca6467545ddd4e0281f3302d65871f4164603/third_party/qcms/src/tests/qcms_test_util.h [modify] https://crrev.com/517ca6467545ddd4e0281f3302d65871f4164603/third_party/qcms/src/transform_util.c
,
Apr 27 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/555dfdfcd56ae8ef91637e4b0827970be7609059 commit 555dfdfcd56ae8ef91637e4b0827970be7609059 Author: radu.velea <radu.velea@intel.com> Date: Wed Apr 27 01:32:39 2016 [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} Review URL: https://codereview.chromium.org/1862053002 Cr-Commit-Position: refs/heads/master@{#389964} [modify] https://crrev.com/555dfdfcd56ae8ef91637e4b0827970be7609059/third_party/qcms/BUILD.gn [modify] https://crrev.com/555dfdfcd56ae8ef91637e4b0827970be7609059/third_party/qcms/README.chromium [modify] https://crrev.com/555dfdfcd56ae8ef91637e4b0827970be7609059/third_party/qcms/qcms.gyp [modify] https://crrev.com/555dfdfcd56ae8ef91637e4b0827970be7609059/third_party/qcms/src/tests/Makefile [modify] https://crrev.com/555dfdfcd56ae8ef91637e4b0827970be7609059/third_party/qcms/src/tests/qcms_test_main.c [add] https://crrev.com/555dfdfcd56ae8ef91637e4b0827970be7609059/third_party/qcms/src/tests/qcms_test_output_trc.c [add] https://crrev.com/555dfdfcd56ae8ef91637e4b0827970be7609059/third_party/qcms/src/tests/qcms_test_util.c [modify] https://crrev.com/555dfdfcd56ae8ef91637e4b0827970be7609059/third_party/qcms/src/tests/qcms_test_util.h [modify] https://crrev.com/555dfdfcd56ae8ef91637e4b0827970be7609059/third_party/qcms/src/transform_util.c
,
Apr 27 2016
#7 win build https://build.chromium.org/p/chromium/builders/Win/builds/42784 looks good this time.
,
Apr 27 2016
#4 win build https://build.chromium.org/p/chromium/builders/Win%20x64/builds/145 x64 win looks good this time.
,
Apr 27 2016
Also synced my client to ToT, and built locally in Windows: no errors.
,
Apr 27 2016
I noticed that for some profiles, the assert(size == 256) goes off, example attached. How should that be dealt with?
,
Apr 27 2016
Right, the test code calls get_output_gamma_table for all profiles. If a profile does not have a parametric curve the LUT size is anything between 256 and 4096. I think it would be safe to remove the assert there since we have 2 others down the line that check size == 256 for parametric curves.
,
Apr 27 2016
Ok, wanna try to remove the asserts?
,
Apr 27 2016
I could remove just the assert from get_output_gamma_table alone (which is causing the problem) or from get_input_gamma_table as well (that way we would have consistency between the 2 functions) and have asserts only inside the parametric "if" clause.
,
Apr 27 2016
Sound about right, try that.
,
Apr 28 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/4b0e97ee4cb19560b5857a53fcd19018846ec697 commit 4b0e97ee4cb19560b5857a53fcd19018846ec697 Author: radu.velea <radu.velea@intel.com> Date: Thu Apr 28 16:21:44 2016 [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 Review-Url: https://codereview.chromium.org/1923873002 Cr-Commit-Position: refs/heads/master@{#390402} [modify] https://crrev.com/4b0e97ee4cb19560b5857a53fcd19018846ec697/third_party/qcms/README.chromium [modify] https://crrev.com/4b0e97ee4cb19560b5857a53fcd19018846ec697/third_party/qcms/src/tests/qcms_test_output_trc.c [modify] https://crrev.com/4b0e97ee4cb19560b5857a53fcd19018846ec697/third_party/qcms/src/transform_util.c
,
Apr 29 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/df1527ec51ed0079b792b260340918c9abfe551c commit df1527ec51ed0079b792b260340918c9abfe551c Author: noel <noel@chromium.org> Date: Fri Apr 29 02:48:34 2016 [qcms] Use a static table in build_output_lut to invert para curves Instead of allocating the uint16_t output table, use a static table gamma_table_uint[256], to match the code in compute_precache(). Minor: adjust the fprintf text in the qcms_tests_output_trc test to clarify the test output. TBR=radu.velea@intel.com BUG= 600338 Review-Url: https://codereview.chromium.org/1929143003 Cr-Commit-Position: refs/heads/master@{#390583} [modify] https://crrev.com/df1527ec51ed0079b792b260340918c9abfe551c/third_party/qcms/README.chromium [modify] https://crrev.com/df1527ec51ed0079b792b260340918c9abfe551c/third_party/qcms/src/tests/qcms_test_output_trc.c [modify] https://crrev.com/df1527ec51ed0079b792b260340918c9abfe551c/third_party/qcms/src/transform_util.c
,
Apr 29 2016
To complete the testing story, we should update qcms_test_output_trc to handle non-para curves as well ...
,
Apr 29 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/3a7538e8c639f83e82f29c632b0a4ecf550670ee commit 3a7538e8c639f83e82f29c632b0a4ecf550670ee Author: noel <noel@chromium.org> Date: Fri Apr 29 12:05:35 2016 [qcms] Make qcms_test_output_trc handle normal gamma curves Update qcms_test_output_trc to read and output normal gamma curves (viz., non-parametric gamma curves). BUG= 600338 Review-Url: https://codereview.chromium.org/1933613002 Cr-Commit-Position: refs/heads/master@{#390624} [modify] https://crrev.com/3a7538e8c639f83e82f29c632b0a4ecf550670ee/third_party/qcms/README.chromium [modify] https://crrev.com/3a7538e8c639f83e82f29c632b0a4ecf550670ee/third_party/qcms/src/tests/qcms_test_output_trc.c
,
Apr 29 2016
|
||||||||
►
Sign in to add a comment |
||||||||
Comment 1 by noel@chromium.org
, Apr 4 2016