Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2715453002:
Add UMAs for numerical approximation of table-based transfer functions. (Closed)
|
Created:
3 years, 10 months ago by ccameron Modified:
3 years, 10 months ago CC:
ajuma+watch_chromium.org, asvitkine+watch_chromium.org, blink-reviews, blink-reviews-platform-graphics_chromium.org, Rik, chromium-reviews, danakj+watch_chromium.org, dshwang, drott+blinkwatch_chromium.org, krit, fmalita+watch_chromium.org, jbroman, Justin Novosad, kinuko+watch, pdr+graphicswatchlist_chromium.org, rwlbuis, Stephen Chennney Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd UMAs for numerical approximation of table-based transfer functions.
Track the following values.
The range of the table-based transfer functions. If almost all cover the
full range from 0 to 1, then we can bake this assumption into the
numerical approxmation. This is a temporary histogram that will be
removed once we gather enough data to make an informed decision.
Whether or not the nonlinear numerical approximation converged (or if
we had to fall back to a linear approximation).
The accuracy of the final numerical approximation.
Note that none of the numerical code in this patch is used by anything
execept histograms (yet).
BUG=667966
Review-Url: https://codereview.chromium.org/2715453002
Cr-Commit-Position: refs/heads/master@{#452701}
Committed: https://chromium.googlesource.com/chromium/src/+/41d5c6041911282c00f3946aa3e4759ba602434b
Patch Set 1 #Patch Set 2 : Fix error computation #
Total comments: 17
Patch Set 3 : Review feedback #Patch Set 4 : Oops, fix compile #
Total comments: 19
Patch Set 5 : Review feedback #
Total comments: 2
Messages
Total messages: 27 (15 generated)
ccameron@chromium.org changed reviewers: + isherman@chromium.org, msarett@chromium.org
ptal: msarett@: for the numerics. there are lots of unit tests which show that this works for synthetic data -- we'll see from here how it works in the real world. isherman@: histograms
https://codereview.chromium.org/2715453002/diff/20001/ui/gfx/skia_color_space... File ui/gfx/skia_color_space_util.cc (right): https://codereview.chromium.org/2715453002/diff/20001/ui/gfx/skia_color_space... ui/gfx/skia_color_space_util.cc:169: If we find that we can assume that TrFn(1)=1, then this code will be able to be removed. In particular, we'll know that (A+B)^G+E=1, so we can just set E=1-(A+B)^G, and not have to solve for it explicitly. (also in that case our normal equations will be 3x3).
Cool! Thanks for the fun review! Only nits and questions. https://codereview.chromium.org/2715453002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/graphics/ColorBehavior.cpp (right): https://codereview.chromium.org/2715453002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/ColorBehavior.cpp:67: if (channel->fCount < 0) Instead you could say: DCHECK(channel->fCount >= 2); It's impossible to specify a table in an ICC profile that is smaller than 2 entries - so if you get one, it's a bug. Perhaps the comments in SkICC.h should make this guarantee. https://codereview.chromium.org/2715453002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/ColorBehavior.cpp:69: const float* data = reinterpret_cast<const float*>( I see that my "convenience" helpers red(), green(), and blue() are pretty worthless. Perhaps I'll add getters by index. https://codereview.chromium.org/2715453002/diff/20001/ui/gfx/color_space_unit... File ui/gfx/color_space_unittest.cc (right): https://codereview.chromium.org/2715453002/diff/20001/ui/gfx/color_space_unit... ui/gfx/color_space_unittest.cc:110: EXPECT_NEAR(t[i], fn_approx_of_x, 2.f / 256.f); Really encouraging that we pass this test :). https://codereview.chromium.org/2715453002/diff/20001/ui/gfx/skia_color_space... File ui/gfx/skia_color_space_util.cc (right): https://codereview.chromium.org/2715453002/diff/20001/ui/gfx/skia_color_space... ui/gfx/skia_color_space_util.cc:27: bool SkTransferFnSolveLinear(SkColorSpaceTransferFn* fn, I imagined this function being simpler. Ex: "draw a line between (0, 0) and the last point on the non-linear segment" - there's only way to do that... I guess this isn't feasible until UMA convinces us that it is ok to assume (0, 0). Still, is it possible for this to cause discontinuities in our approximation function? Which might be visible on a smooth gradient etc. https://codereview.chromium.org/2715453002/diff/20001/ui/gfx/skia_color_space... ui/gfx/skia_color_space_util.cc:97: return false; Logically, when can this fail? Sure, matrix inverses can fail - but shouldn't we always be able to get a linear fit when we have multiple x-values? https://codereview.chromium.org/2715453002/diff/20001/ui/gfx/skia_color_space... ui/gfx/skia_color_space_util.cc:169: On 2017/02/22 10:21:30, ccameron wrote: > If we find that we can assume that TrFn(1)=1, then this code will be able to be > removed. > > In particular, we'll know that (A+B)^G+E=1, so we can just set E=1-(A+B)^G, and > not have to solve for it explicitly. > > (also in that case our normal equations will be 3x3). Acknowledged, really hope this is the case. https://codereview.chromium.org/2715453002/diff/20001/ui/gfx/skia_color_space... ui/gfx/skia_color_space_util.cc:218: const float kConvergedEarlyEpsilon = 1.f / 32768.f; How important is it to have this case? Are we worried about the performance of this code? https://codereview.chromium.org/2715453002/diff/20001/ui/gfx/skia_color_space... ui/gfx/skia_color_space_util.cc:325: fn->fD = 1.1f; I think you choose 1.1f here because the linear range is defined as strictly less than fD? This is maybe more clear as: 1.0f + <some epsilon> https://codereview.chromium.org/2715453002/diff/20001/ui/gfx/skia_color_space... ui/gfx/skia_color_space_util.cc:333: if (*linear_fit_succeeded && !*nonlinear_fit_converged) { I'd like to think this will always be: A = 1.0f B = 0.0f That way we always map [0, 1] to [0, 1]. But your histograms should be informative about this.
The CQ bit was checked by ccameron@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thanks! We'll still need fallback for pure-A2B profiles, but it'll be less accurate or more expensive. If we find that [tMin, tMax] is [0, 1] for >99% of non-numerical profiles, then I'll be content to take the remaining 1% and throw them onto the A2B path for simplicity. https://codereview.chromium.org/2715453002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/graphics/ColorBehavior.cpp (right): https://codereview.chromium.org/2715453002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/ColorBehavior.cpp:67: if (channel->fCount < 0) On 2017/02/22 19:53:18, msarett1 wrote: > Instead you could say: > DCHECK(channel->fCount >= 2); > > It's impossible to specify a table in an ICC profile that is smaller than 2 > entries - so if you get one, it's a bug. > > Perhaps the comments in SkICC.h should make this guarantee. Done. https://codereview.chromium.org/2715453002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/ColorBehavior.cpp:69: const float* data = reinterpret_cast<const float*>( On 2017/02/22 19:53:18, msarett1 wrote: > I see that my "convenience" helpers red(), green(), and blue() are pretty > worthless. Perhaps I'll add getters by index. Yeah, index is better, cause I'll be iterating over them. But this is fine for now. https://codereview.chromium.org/2715453002/diff/20001/ui/gfx/skia_color_space... File ui/gfx/skia_color_space_util.cc (right): https://codereview.chromium.org/2715453002/diff/20001/ui/gfx/skia_color_space... ui/gfx/skia_color_space_util.cc:27: bool SkTransferFnSolveLinear(SkColorSpaceTransferFn* fn, On 2017/02/22 19:53:18, msarett1 wrote: > I imagined this function being simpler. Ex: "draw a line between (0, 0) and the > last point on the non-linear segment" - there's only way to do that... > > I guess this isn't feasible until UMA convinces us that it is ok to assume (0, > 0). > > Still, is it possible for this to cause discontinuities in our approximation > function? Which might be visible on a smooth gradient etc. Yes, this is a bit scary at the moment -- it can have discontinuities at fD. Drawing a line from 0,0 to fD would certainly work. Another scheme that I was considering to avoid that problem would be - compute the best-fit line for the linear segment (hopefully assuming 0 maps to 0) - compute fD to be the intersection between the nonlinear segment and the linear segment I'd like to start the investigation using this scheme, because it's guaranteed to have the smallest error -- if we're seeing good results, we can then add more constraints about continuity (and compare the results to this). https://codereview.chromium.org/2715453002/diff/20001/ui/gfx/skia_color_space... ui/gfx/skia_color_space_util.cc:97: return false; On 2017/02/22 19:53:18, msarett1 wrote: > Logically, when can this fail? > > Sure, matrix inverses can fail - but shouldn't we always be able to get a linear > fit when we have multiple x-values? True, if we have at least 1 data point, then, up to machine epsilon, this shouldn't fail. Changed this to a DCHECK. I separated out the histograms to "error when the nonlinear fit converged" and "error when the nonlinear fit did not converge". https://codereview.chromium.org/2715453002/diff/20001/ui/gfx/skia_color_space... ui/gfx/skia_color_space_util.cc:218: const float kConvergedEarlyEpsilon = 1.f / 32768.f; On 2017/02/22 19:53:18, msarett1 wrote: > How important is it to have this case? Are we worried about the performance of > this code? In the final version I'll want to have some early-convergence checks, but for now this isn't really needed. Removed it for now. https://codereview.chromium.org/2715453002/diff/20001/ui/gfx/skia_color_space... ui/gfx/skia_color_space_util.cc:325: fn->fD = 1.1f; On 2017/02/22 19:53:18, msarett1 wrote: > I think you choose 1.1f here because the linear range is defined as strictly > less than fD? This is maybe more clear as: > 1.0f + <some epsilon> Done. https://codereview.chromium.org/2715453002/diff/20001/ui/gfx/skia_color_space... ui/gfx/skia_color_space_util.cc:333: if (*linear_fit_succeeded && !*nonlinear_fit_converged) { On 2017/02/22 19:53:18, msarett1 wrote: > I'd like to think this will always be: > A = 1.0f > B = 0.0f > > That way we always map [0, 1] to [0, 1]. But your histograms should be > informative about this. Yeah, it'll be nice to not have to worry about these cases.
lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...) ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
The CQ bit was checked by ccameron@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2715453002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/graphics/ColorBehavior.cpp (right): https://codereview.chromium.org/2715453002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/ColorBehavior.cpp:65: 1023, 32); Just to double-check: Are you taking into account that buckets are exponentially sized? It looks like you might want linearly-spaced buckets, which UMA_HISTOGRAM_CUSTOM_COUNTS will not provide (but UMA_HISTOGRAM_LINEAR will). https://codereview.chromium.org/2715453002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/ColorBehavior.cpp:87: 256); Please don't pre-allocate space for 256 buckets -- across many histograms, that adds up to significant allocated-but-unused memory. Instead, you could use a sparse histogram (UMA_HISTOGRAM_SPARSE_SLOWLY), with custom clamping code. (Actually, if you want to go the extra mile, it would be great to have a wrapper for sparse histograms that includes clamping values.) https://codereview.chromium.org/2715453002/diff/60001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2715453002/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:4419: +</histogram> You mentioned in the CL description that at least some of these histograms are intended to be temporary. Please indicate this within the <summary> or <details>, including a target milestone for removal. It's easy to lose track of cleanup tasks, so it's helpful to document the intent. https://codereview.chromium.org/2715453002/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:4431: + units="ColorSpaceNonlinearFitConverged"> nit: s/units/enum https://codereview.chromium.org/2715453002/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:4457: +<histogram name="Blink.ColorSpace.Destination.RawData" nit: Perhaps name this "ExtractedRawData", to clarify from the name that it's a boolean histogram? https://codereview.chromium.org/2715453002/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:4458: + units="ColorSpaceRawDataResult"> nit: s/units/enum https://codereview.chromium.org/2715453002/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:4466: +<histogram name="Blink.ColorSpace.Destination.TableSize"> Please add a units attribute. https://codereview.chromium.org/2715453002/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:4481: + The minimum value of table-based ICC profile transfer functions. It looks like these values are actually multiplied by 255; please mention that here in the histogram description. Perhaps something similar to the description of the Error histograms? (And ditto for tMax.) https://codereview.chromium.org/2715453002/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:82826: + <int value="1" label="Nonlinear fit converged."/> nit: I'd shorten these labels to "Did not converge" and "Converged". https://codereview.chromium.org/2715453002/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:82836: + <int value="1" label="Successfully extracted raw transfer function tables."/> nit: Likewise, I'd shorten these labels to "Failed to extract" and "Successfully extracted".
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Patchset #5 (id:80001) has been deleted
Thanks -- updated https://codereview.chromium.org/2715453002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/graphics/ColorBehavior.cpp (right): https://codereview.chromium.org/2715453002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/ColorBehavior.cpp:65: 1023, 32); On 2017/02/22 22:48:48, Ilya Sherman wrote: > Just to double-check: Are you taking into account that buckets are exponentially > sized? It looks like you might want linearly-spaced buckets, which > UMA_HISTOGRAM_CUSTOM_COUNTS will not provide (but UMA_HISTOGRAM_LINEAR will). Oh, I didn't know that. Thanks! https://codereview.chromium.org/2715453002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/ColorBehavior.cpp:87: 256); On 2017/02/22 22:48:48, Ilya Sherman wrote: > Please don't pre-allocate space for 256 buckets -- across many histograms, that > adds up to significant allocated-but-unused memory. Instead, you could use a > sparse histogram (UMA_HISTOGRAM_SPARSE_SLOWLY), with custom clamping code. > (Actually, if you want to go the extra mile, it would be great to have a wrapper > for sparse histograms that includes clamping values.) Actually, with exponentially-sized buckets, having 16 buckets for the range of 0 through 127 is good enough for error. For tMin/tMax, I need even less precision (a value of 32 is huge), so I've changed this to an 8-bucket with a max of 32. I also changed TMax to OneMinusTMax. https://codereview.chromium.org/2715453002/diff/60001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2715453002/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:4431: + units="ColorSpaceNonlinearFitConverged"> On 2017/02/22 22:48:49, Ilya Sherman wrote: > nit: s/units/enum Done. https://codereview.chromium.org/2715453002/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:4457: +<histogram name="Blink.ColorSpace.Destination.RawData" On 2017/02/22 22:48:48, Ilya Sherman wrote: > nit: Perhaps name this "ExtractedRawData", to clarify from the name that it's a > boolean histogram? Done. https://codereview.chromium.org/2715453002/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:4458: + units="ColorSpaceRawDataResult"> On 2017/02/22 22:48:48, Ilya Sherman wrote: > nit: s/units/enum Done. https://codereview.chromium.org/2715453002/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:4466: +<histogram name="Blink.ColorSpace.Destination.TableSize"> On 2017/02/22 22:48:49, Ilya Sherman wrote: > Please add a units attribute. Removed this one entirely, since it's less actionable (for the moment). https://codereview.chromium.org/2715453002/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:4481: + The minimum value of table-based ICC profile transfer functions. On 2017/02/22 22:48:48, Ilya Sherman wrote: > It looks like these values are actually multiplied by 255; please mention that > here in the histogram description. Perhaps something similar to the description > of the Error histograms? (And ditto for tMax.) Done (8-bit fixed-point). https://codereview.chromium.org/2715453002/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:82826: + <int value="1" label="Nonlinear fit converged."/> On 2017/02/22 22:48:49, Ilya Sherman wrote: > nit: I'd shorten these labels to "Did not converge" and "Converged". Done. https://codereview.chromium.org/2715453002/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:82836: + <int value="1" label="Successfully extracted raw transfer function tables."/> On 2017/02/22 22:48:49, Ilya Sherman wrote: > nit: Likewise, I'd shorten these labels to "Failed to extract" and "Successfully > extracted". Done.
Metrics LGTM, thanks.
pdr@chromium.org changed reviewers: + pdr@chromium.org
LGTM https://codereview.chromium.org/2715453002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/graphics/ColorBehavior.cpp (right): https://codereview.chromium.org/2715453002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/graphics/ColorBehavior.cpp:100: if (nonlinearFitConverged) { These counters are in what looks like an expensive loop. Just double-checking... is that okay for your usecase?
Thanks! https://codereview.chromium.org/2715453002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/graphics/ColorBehavior.cpp (right): https://codereview.chromium.org/2715453002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/graphics/ColorBehavior.cpp:100: if (nonlinearFitConverged) { On 2017/02/23 21:24:57, pdr. wrote: > These counters are in what looks like an expensive loop. Just double-checking... > is that okay for your usecase? Yes -- this should only happen once in a renderer's lifetime (though it does happen for all 3 channels).
The CQ bit was checked by ccameron@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from msarett@chromium.org Link to the patchset: https://codereview.chromium.org/2715453002/#ps100001 (title: "Review feedback")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 100001, "attempt_start_ts": 1487891938681120,
"parent_rev": "65fad2795b571f868c559941187f785aaa0414a0", "commit_rev":
"41d5c6041911282c00f3946aa3e4759ba602434b"}
Message was sent while issue was closed.
Description was changed from ========== Add UMAs for numerical approximation of table-based transfer functions. Track the following values. The range of the table-based transfer functions. If almost all cover the full range from 0 to 1, then we can bake this assumption into the numerical approxmation. This is a temporary histogram that will be removed once we gather enough data to make an informed decision. Whether or not the nonlinear numerical approximation converged (or if we had to fall back to a linear approximation). The accuracy of the final numerical approximation. Note that none of the numerical code in this patch is used by anything execept histograms (yet). BUG=667966 ========== to ========== Add UMAs for numerical approximation of table-based transfer functions. Track the following values. The range of the table-based transfer functions. If almost all cover the full range from 0 to 1, then we can bake this assumption into the numerical approxmation. This is a temporary histogram that will be removed once we gather enough data to make an informed decision. Whether or not the nonlinear numerical approximation converged (or if we had to fall back to a linear approximation). The accuracy of the final numerical approximation. Note that none of the numerical code in this patch is used by anything execept histograms (yet). BUG=667966 Review-Url: https://codereview.chromium.org/2715453002 Cr-Commit-Position: refs/heads/master@{#452701} Committed: https://chromium.googlesource.com/chromium/src/+/41d5c6041911282c00f3946aa3e4... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:100001) as https://chromium.googlesource.com/chromium/src/+/41d5c6041911282c00f3946aa3e4... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
