Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2629723006:
Add histograms for ICC profile parsing (Closed)
|
Created:
3 years, 11 months ago by ccameron Modified:
3 years, 11 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, f(malita), jbroman, Justin Novosad, kinuko+watch, pdr+graphicswatchlist_chromium.org, rwlbuis, Stephen Chennney Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd histograms for ICC profile parsing
This will inform if we can restrict our color space support.
BUG=634102
Review-Url: https://codereview.chromium.org/2629723006
Cr-Commit-Position: refs/heads/master@{#444880}
Committed: https://chromium.googlesource.com/chromium/src/+/c99fb6148dfdadf8f335462db33f2a520394c097
Patch Set 1 #Patch Set 2 : Use boolean #
Total comments: 2
Patch Set 3 : Incorporate review feedback #
Dependent Patchsets: Messages
Total messages: 32 (20 generated)
ccameron@chromium.org changed reviewers: + chrishtr@chromium.org, isherman@chromium.org
isherman: histograms.xml chrishtr: blink/... msarett: Fingers crossed this will come back at >99.9%
On 2017/01/13 22:55:26, ccameron wrote: > isherman: histograms.xml > chrishtr: blink/... > > msarett: Fingers crossed this will come back at >99.9% I'll predict that the matrix metric will be encouraging, but I don't think the transfer fn will be. There's just a lot of tables... Having said that, I don't think we should give up on the approximateTransferFn(). What would be really useful would be if we could get the ICC data for FALSE values - then we would have test cases.
On 2017/01/13 23:01:26, msarett1 wrote: > On 2017/01/13 22:55:26, ccameron wrote: > > isherman: histograms.xml > > chrishtr: blink/... > > > > msarett: Fingers crossed this will come back at >99.9% > > I'll predict that the matrix metric will be encouraging, but I don't think the > transfer fn will be. There's just a lot of tables... > > Having said that, I don't think we should give up on the > approximateTransferFn(). What would be really useful would be if we could get > the ICC data for FALSE values - then we would have test cases. Oh, yeah, I really want to do approximateTransferFn -- I'd have included it, except it didn't link (no implementation). I may take a stab at writing a approximateTransferFn implementation, just cause it looks fun (and it's the end of the week).
OWNERs (chrishtr & isherman) ping. msarett and I are discussing the approximation part offline -- that's going to be a slightly longer project.
lgtm
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...
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/2629723006/diff/20001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2629723006/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:4278: +<histogram name="Blink.ColorSpace.Destination.Matrix" units="Boolean"> nit: s/units/enum (ditto below) https://codereview.chromium.org/2629723006/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:4278: +<histogram name="Blink.ColorSpace.Destination.Matrix" units="Boolean"> Please use a custom enum (just within this file), with labels like "No matrix" and "Extracted to-XYZD50 matrix"; similarly below.
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 ccameron@chromium.org to run a CQ dry run
Updated - thanks!
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Metrics LGTM, thanks.
The CQ bit was checked by ccameron@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from chrishtr@chromium.org Link to the patchset: https://codereview.chromium.org/2629723006/#ps40001 (title: "Incorporate review feedback")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by ccameron@chromium.org
The CQ bit was unchecked by ccameron@chromium.org
The CQ bit was unchecked by ccameron@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by ccameron@chromium.org
The CQ bit was checked by ccameron@chromium.org
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": 40001, "attempt_start_ts": 1484866992604560,
"parent_rev": "29f7cd135146bf1ac2e4020d04014f69fce261ac", "commit_rev":
"c99fb6148dfdadf8f335462db33f2a520394c097"}
Message was sent while issue was closed.
Description was changed from ========== Add histograms for ICC profile parsing This will inform if we can restrict our color space support. BUG=634102 ========== to ========== Add histograms for ICC profile parsing This will inform if we can restrict our color space support. BUG=634102 Review-Url: https://codereview.chromium.org/2629723006 Cr-Commit-Position: refs/heads/master@{#444880} Committed: https://chromium.googlesource.com/chromium/src/+/c99fb6148dfdadf8f335462db33f... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/c99fb6148dfdadf8f335462db33f... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
