clamp before premul
When alpha is <1, it's possible to fit r,g,b >1 into premul unorms,
and that subverts the expectations users who consume the output of
skcms_Transform().
When working with unorm pixels, downstream consumers will assume the
r,g,b values are also within [0,1], i.e. are less than the alpha
channel. Put another way, they assume the colors are in gamut.
So clamp earlier, before premul. (The reordering has no effect on
force_opaque or swap_rb.)
Bug: chromium:867813
Bug: chromium:862908
Change-Id: I553b42df06482a4e3cd3a9377c2adddb8ff3d1bc
Reviewed-on: https://skia-review.googlesource.com/143710
Reviewed-by: Brian Osman <brianosman@google.com>
Commit-Queue: Brian Osman <brianosman@google.com>
Auto-Submit: Mike Klein <mtklein@google.com>
diff --git a/skcms.cc b/skcms.cc
index e81f949..0a82062 100644
--- a/skcms.cc
+++ b/skcms.cc
@@ -2285,6 +2285,15 @@
if (!is_identity_tf(&inv_dst_tf_b)) { *ops++ = Op_tf_b; *args++ = &inv_dst_tf_b; }
}
+ // Clamp here before premul to make sure we're clamping to fixed-point values _and_ gamut,
+ // not just to values that fit in the fixed point representation.
+ //
+ // E.g. r = 1.1, a = 0.5 would fit fine in fixed point after premul (ra=0.55,a=0.5),
+ // but would be carrying r > 1, which is really unexpected for downstream consumers.
+ // TODO(mtklein): add a unit test
+ if (dstFmt < skcms_PixelFormat_RGB_hhh) {
+ *ops++ = Op_clamp;
+ }
if (dstAlpha == skcms_AlphaFormat_Opaque) {
*ops++ = Op_force_opaque;
} else if (dstAlpha == skcms_AlphaFormat_PremulAsEncoded) {
@@ -2293,9 +2302,6 @@
if (dstFmt & 1) {
*ops++ = Op_swap_rb;
}
- if (dstFmt < skcms_PixelFormat_RGB_hhh) {
- *ops++ = Op_clamp;
- }
switch (dstFmt >> 1) {
default: return false;
case skcms_PixelFormat_A_8 >> 1: *ops++ = Op_store_a8; break;