Chromium Code Reviews Chromium Code Reviews
Issue
2436223002:
Revert of Use SkColorSpaceXform to handle color conversions in decoders (Closed)
DescriptionRevert of Use SkColorSpaceXform to handle color conversions in decoders (patchset #7 id:230001 of https://chromiumcodereview.appspot.com/2426723005/ )
Reason for revert:
I believe this is the (indirect) cause of a crash on Linux MSAN bot due to use of uninitialized variable:
https://build.chromium.org/p/chromium.webkit/builders/WebKit%20Linux%20Precise%20MSAN/builds/650/steps/webkit_tests/logs/stdio
I imagine this is merely exposing this bug in some existing not-currently-exercised code. But if the variable is really used uninitialized, this could result in test flakiness as well.
Original issue's description:
> Use SkColorSpaceXform to handle color conversions in decoders
>
> Note on matching color spaces:
> *** Roughly 90% of image profiles specify sRGB. Most
> dsts are also sRGB (and so is our default when we
> don't know the dst). In the sRGB->sRGB case (and in
> other cases where the src and dst match), we should
> skip the xform. QCMS checks for matching profiles,
> but the check is very weak (only works if profiles have
> the same text description). SkColorSpace has
> a robust check for color space equality.
>
> Performance Notes:
> ***Test Srcs: Averaged across 97 images pulled from the top
> 10k skps.
> ***Test Dsts: sRGB transfer function, 2.2, other.
>
> HP z620 Average Xform Speed-Up (QCMS / SkColorSpaceXform)
> sRGB 1.51x
> 2.2 1.61x
> Other 1.44x
> (SkColorSpaceXform is actually fastest for sRGB - I just
> had to drop a bunch of images from the sRGB average,
> including a few that tripped up QCMS. SkColorSpaceXform
> was recognizing sRGB->sRGB was a no-op, so including these
> images skewed the results in its favor - I saw like 17x)
>
> Nexus 6P Average Xform Speed-Up (QCMS / SkColorSpaceXform)
> sRGB 2.08x
> 2.2 1.42x
> Other 3.33x
> (interesting that the fallback code is actually faster than
> the special cases on Arm - likely this is because the
> sqrt instructions that we're using to model the curves are
> slow on this platform - would be interesting to investigate
> further to see if we can do better)
> *** Note that turning this on for Android is left to a
> later CL.
>
> Correctness Notes:
> ***QCMS (and the Skia fallback code path) use a LUT to
> convert linear floats to the dst transfer function.
> This results in a loss of accuracy since the float
> must be rounded to an int before being used as an
> index into the table. Both Skia and QCMS do not
> interpolate the table for performance reasons.
> Skia avoids table-based implementations for two
> common cases (sRGB, 2.2), guaranteeing a result
> within 1 (on a 0-255 scale) of the "true" value.
> QCMS may be off by as much as 5 on the steep 2.2
> curve.
> ***There are several fairly common sRGB profiles that
> represent the transfer function as a small LUT.
> QCMS does not recognize that it is sRGB, and
> interpolates this LUT to build a larger LUT that
> it then uses. This results in the conversion being
> off from the "true" value by as much as 9 (on a
> 0-255 scale).
>
> Other Behavior Changes:
> *** More lenient approval of ICC profiles. QCMS rejects
> xform matrices that are not "close enough to D50" by
> some arbitrary measure. We don't have enough data to
> know that this is or isn't necessary. QCMS also
> rejects profiles that it thinks are "too big". Again,
> this seems reasonable, but we'll wait until we
> understand the motivation better.
>
> What ICC profiles are supported?
> ***SkColorSpaceXform supports the same subset of profiles
> as QCMS. Adding support for A2B and Lab profiles is
> a WIP (We had support for a few of these, but we've
> dropped it as part of a refactor. THe plan is to add
> it back as a part of a more logical, complete design).
>
> How does this relate to color correct rendering for Chrome?
> ***This modifies the legacy path, not the color correct
> path.
> ***It's still somewhat interestingly related to color
> correct rendering, since the color correct renderer
> will also use SkColorSpaceXform in some cases
> (likely just to xform scary profiles into linear F16).
> ***It looks like the color correct rendering path will
> rely on tagged images being handled inside Skia, but
> it's worth mentioning that this change makes it simpler
> to do flexible xforms (ex: to F16) at decode time.
>
> BUG=
TBR=scroggo@chromium.org,ccameron@chromium.org,dpranke@chromium.org,msarett@google.com
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG=
Committed: https://crrev.com/aea9755923beadcff36271fb41c7cf58dab88456
Cr-Commit-Position: refs/heads/master@{#426678}
Patch Set 1 #Messages
Total messages: 16 (7 generated)
|
