7 ms·
I wrote a lot of the Skia color management code involved here, and I think maybe I can clear some things up? I'd be happy to answer any questions you might hav
by mtklein 6y ago
I wrote a lot of the Skia color management code involved here, and I think maybe I can clear some things up? I'd be happy to answer any questions you might have, either here or on https://bugs.chromium.org/p/skia/issues/detail?id=10301 https://bugs.chromium.org/p/skia/issues/detail?id=10301 where we've been trying to think about how much Skia's involved with the root cause of this bug.
There's no "Skia color profile" per se. When Skia's asked to write a JPEG from a pixel buffer tagged with a color profile (SkColorSpace), we auto-generate an ICC profile programatically from the parameters that describe the color gamut and transfer functions we use to represent those color profiles. That's what the code in this file is up to: https://source.chromium.org/chromium/chromium/src/+/master:third_party/skia/src/core/SkICC.cpp https://source.chromium.org/chromium/chromium/src/+/master:t...
So this profile is one of infinite possible profiles that'll have "Google/Skia/..big long MD5.." somewhere in them.
This profile in particular looks like it's probably describing the ProPhoto color space, which is very, very large and uses imaginary colors outside visible light to increase its gamut. While the JPEG itself is always storing values in range in the ProPhoto gamut, when you convert that to another gamut (say, to match the display, or to sRGB) it's very easy to end up with logical color channel values less than zero or greater than one, which do make a kind of sense, but not to code that expects colors are always sRGB bytes.
It's unclear to me what gamut the colors being read in that Java code are in,
and that'd very much affect what the appropriate values for that LUMINOSITY_MATRIX should be. Those are the correct values to get the luminosity for sRGB colors (though I think what's being calculated here is subtly-different gamma-encoded "luma" instead), but they're not likely a sound way to get luminosity if the image is in any different gamut. A typical strategy is to convert any image you load to the display gamut early, since you'll need to do that eventually to display it. And it's more and more common that phone display gamuts are not sRGB, usually something wider now like Display P3.
- wereHamster 6y agoFrom the Skia bug report: > out of bounds write in Java Isn't Java supposed to be a language safe from these kinds of bugs? At the very worst it throws an exception, which should be caught and dealt with accordingly… How can that crash the whole phone?
- cesarb 6y ago> At the very worst it throws an exception Yes, from what I have read that's exactly what was happening, it was throwing an IndexOutOfBoundsException. Apparently, nothing was catching that exception, so the whole process was terminated. > How can that crash the whole phone? The main problem is that the process which was crashing is the Android equivalent of the window manager or the X server. It recovered from that crash either by restarting the process (which immediately crashed again), or by rebooting the phone (and immediately crashing again).
- cannam 6y ago> Apparently, nothing was catching that exception, so the whole process was terminated. Apposite moment perhaps to refer to the mind-boggling finding in "Simple Testing Can Prevent Most Critical Failures: An Analysis of Production Failures in Distributed Data-Intensive Systems" (Yuan et al, Usenix 2014 https://www.usenix.org/conference/osdi14/technical-sessions/presentation/yuan https://www.usenix.org/conference/osdi14/technical-sessions/...): "almost all (92%) of the catastrophic system failures [in the real-world study] are the result of incorrect handling of non-fatal errors explicitly signaled in software"
- NikolaeVarius 6y agoI lends this leads credence to the go way. Verbose because than its hard to mess up
- Groxx 6y agoI mean... not really. This is the equivalent of `if err != nil { return nil, err }` which is so common it's a mindless default behavior worthy of memes: https://pbs.twimg.com/media/DCIF7-2W0AEAv9c.jpg https://pbs.twimg.com/media/DCIF7-2W0AEAv9c.jpg It absolutely results in the same kinds of failures.
- diamondo25 6y ago