Color transform unpremult logic changes.

After some deep thinking and discussion with people who know more color
than me, I have become convinced that you *almost always* want to make
sure to un-premultiply colors (i.e. divide by alpha) before doing color
space transformations with OCIO, and then re-multiply by alpha (to get
back to "associated alpha") immediately after the color transformation.

To support this,

* For the ImageBufAlgo color transformation functions, change the default
  value of the `unpremult` parameter from `false` to `true`.

* `oiiotool --autocc` will do the unpremult/premult steps surrounding any
  automatic color transformations that it does upon input and output.

* `oiiotool --colorconvert` and the various `--ocio` commands now take
  an optional modifier `:unpremult=1` which will cause the conversion to
  be internally bracked by a unpremult/premult step (when you do this,
  you no longer need an explicit --unpremult and --premult on the
  command line).

* oiiotool now issues warnings for cases where it has a good reason to
  think you might be double-unpremulting (for example, if you do both
  --unpremult and follow it by a --colorconvert:unpremult=1).

Later, we'll stage a second part of this where we change the default
value for unpremult= to be 1, so that the modifier is not needed (at which
point you REALLY don't want the explicit --unpremult/--premult, or it will
be wrong).

On the whole, I think this makes the situation less error prone for
casual users. Most oiiotool command lines should just use --autocc and
then not need to worry about any explicit use of `--colorconvert` or
`--unpremult/--premult`. For the rare case when more control is needed,
the individual commands and modifiers can be used.

Note that for oiiotool, `--autocc` is not on by default. If you want
images auto-converted to linear space and ouputs auto-converted from
linear to the desired output space, you need to use `--autocc`.

Author: lgritz

CHANGES.md

@@ -43,6 +43,10 @@ Public API changes:
   only be used to pass into certain IBA functions). The color space names
   "rgb" and "default" are now understood to be synonyms for the default
   "linear" color space. #1788 (1.9.0)
+* `ImageBufAlgo::colorconvert` and various `ocio` transformations have
+  changed the default value of their `unpremult` parameter from `false` to
+  `true`, reflecting the fact that we believe this is almost always the more
+  correct choice. (1.9.2)
 * Remove long-deprecated API calls:
     * ImageBuf::get_pixels/get_pixel_channels varieties deprecated since 1.6.
     * ImageBuf::set_deep_value_uint, deprecated since 1.7.
@@ -68,6 +72,14 @@ Fixes, minor enhancements, and performance improvements:
        look good converted to greyscale, and usable by people with color
        blindness. #1820 (1.9.2)
     * oiiotool no longer enables autotile by default. #1856 (1.9.2)
+    * `--colorconvert`, `--tocolorspace`, and all of the `--ocio` commands
+      now by default will bracket the transformation unpremult/premult
+      steps, i.e., divide the RGB values by alpha (if present and nonzero)
+      before the color operation, then immediately re-multiply by alpha.
+      This can be disabled using a new optional modifier `:unpremult=0`.
+      (1.9.2)
+    * `--autocc` will also cause unpremult/premult to bracket any color
+      transformations it does automatically for read and write. (1.9.2)
 * ImageBufAlgo:
     * `color_map()` new  maps "inferno", "magma", "plasma", "viridis".
        #1820 (1.9.2)

src/doc/oiiotool.tex

@@ -1019,7 +1019,7 @@ \subsection*{Reading images}
 \apiend
 
 \apiitem{\ce --autocc}
-Turns on automatic color conversion: Every input image file will be
+Turns on automatic color space conversion: Every input image file will be
 immediately converted to a scene-referred linear color space, and every file
 written will be first transformed to an appropriate output color space based
 on the filename or type.   Additionally, if the name of an output file
@@ -2903,6 +2903,11 @@ \section{\oiiotool commands for color management}
  & {\cf value=}\emph{str} & Adds a key/value pair to the ``context'' that
   OpenColorIO will used when applying the look. Multiple key/value pairs
   may be specified by making each one a comma-separated list. \\
+ & {\cf unpremult=}\emph{val} & If the numeric \emph{val} is nonzero, the
+     pixel values will be ``un-premultipled'' (divided by alpha) prior to
+     the actual color conversion, and then re-multipled by alpha afterwards.
+     The default is 0, meaning the color transformation not will be
+     automatically bracketed by divide-by-alpha / mult-by-alpha operations. \\
  & {\cf strict=}\emph{val} & When nonzero (the default), an
      inability to perform the color transform will be a hard error. If
      strict is 0, inability to find the transformation will just print a
@@ -2936,6 +2941,11 @@ \section{\oiiotool commands for color management}
  & {\cf value=}\emph{str} & Adds a key/value pair to the ``context'' that
   OpenColorIO will used when applying the look. Multiple key/value pairs
   may be specified by making each one a comma-separated list. \\
+ & {\cf unpremult=}\emph{val} & If the numeric \emph{val} is nonzero, the
+     pixel values will be ``un-premultipled'' (divided by alpha) prior to
+     the actual color conversion, and then re-multipled by alpha afterwards.
+     The default is 0, meaning the color transformation not will be
+     automatically bracketed by divide-by-alpha / mult-by-alpha operations. \\
 \end{tabular}
 
 This command is only meaningful if OIIO was compiled with OCIO support
@@ -2966,6 +2976,11 @@ \section{\oiiotool commands for color management}
  & {\cf value=}\emph{str} & Adds a key/value pair to the ``context'' that
   OpenColorIO will used when applying the display transform. Multiple key/value pairs
   may be specified by making each one a comma-separated list. \\
+ & {\cf unpremult=}\emph{val} & If the numeric \emph{val} is nonzero, the
+     pixel values will be ``un-premultipled'' (divided by alpha) prior to
+     the actual color conversion, and then re-multipled by alpha afterwards.
+     The default is 0, meaning the color transformation not will be
+     automatically bracketed by divide-by-alpha / mult-by-alpha operations. \\
 \end{tabular}
 
 This command is only meaningful if OIIO was compiled with OCIO support
@@ -2989,6 +3004,11 @@ \section{\oiiotool commands for color management}
 \begin{tabular}{p{10pt} p{1in} p{3.75in}}
  & {\cf inverse=}\emph{val} & If \emph{val} is nonzero, inverts the 
   color transformation. \\
+ & {\cf unpremult=}\emph{val} & If the numeric \emph{val} is nonzero, the
+     pixel values will be ``un-premultipled'' (divided by alpha) prior to
+     the actual color conversion, and then re-multipled by alpha afterwards.
+     The default is 0, meaning the color transformation not will be
+     automatically bracketed by divide-by-alpha / mult-by-alpha operations. \\
 \end{tabular}
 
 This command is only meaningful if OIIO was compiled with OCIO support

src/doc/openimageio.tex

@@ -94,7 +94,7 @@
  \bigskip \\
 }
 \date{{\large
-Date: 5 Dec 2017
+Date: 3 Feb 2018
 \\ (with corrections, 4 Feb 2018)
 }}
 

src/include/OpenImageIO/imagebufalgo.h

@@ -970,15 +970,17 @@ bool OIIO_API rangeexpand (ImageBuf &dst, const ImageBuf &src,
 /// size as specified by roi.  If roi is not defined it will be all
 /// of dst, if dst is defined, or all of src, if dst is not yet defined.
 ///
-/// If unpremult is true, unpremultiply before color conversion, then
-/// premultiply after the color conversion.  You may want to use this
-/// flag if your image contains an alpha channel.
+/// If unpremult is true, divide the RGB channels by alpha (if it exists and
+/// is nonzero) before color conversion, then re-multiply by alpha after the
+/// after the color conversion. Passing unpremult=false skips this step,
+/// which may be desirable if you know that the image is "unassociated alpha"
+/// (a.k.a. "not pre-multiplied colors").
 ///
 /// Return true on success, false on error (with an appropriate error
 /// message set in dst).
 bool OIIO_API colorconvert (ImageBuf &dst, const ImageBuf &src,
                             string_view from, string_view to,
-                            bool unpremult=false,
+                            bool unpremult=true,
                             string_view context_key="",
                             string_view context_value="",
                             ColorConfig *colorconfig=NULL,
@@ -991,9 +993,11 @@ bool OIIO_API colorconvert (ImageBuf &dst, const ImageBuf &src,
 /// size as specified by roi.  If roi is not defined it will be all
 /// of dst, if dst is defined, or all of src, if dst is not yet defined.
 ///
-/// If unpremult is true, unpremultiply before color conversion, then
-/// premultiply after the color conversion.  You may want to use this
-/// flag if your image contains an alpha channel.
+/// If unpremult is true, divide the RGB channels by alpha (if it exists and
+/// is nonzero) before color conversion, then re-multiply by alpha after the
+/// after the color conversion. Passing unpremult=false skips this step,
+/// which may be desirable if you know that the image is "unassociated alpha"
+/// (a.k.a. "not pre-multiplied colors").
 ///
 /// Return true on success, false on error (with an appropriate error
 /// message set in dst).
@@ -1029,7 +1033,7 @@ bool OIIO_API colorconvert (float *color, int nchannels,
 /// message set in dst).
 bool OIIO_API ociolook (ImageBuf &dst, const ImageBuf &src,
                         string_view looks, string_view from, string_view to,
-                        bool unpremult=false, bool inverse=false,
+                        bool unpremult=true, bool inverse=false,
                         string_view key="", string_view value="",
                         ColorConfig *colorconfig=NULL,
                         ROI roi=ROI::All(), int nthreads=0);
@@ -1038,23 +1042,25 @@ bool OIIO_API ociolook (ImageBuf &dst, const ImageBuf &src,
 /// "display" transform.  If from or looks are NULL, it will not
 /// override the look or source color space (subtly different than
 /// passing "", the empty string, which means to use no look or source
-/// space).
+/// space). If inverse is true, it will reverse the color transformation.
 ///
 /// If dst is not yet initialized, it will be allocated to the same
 /// size as specified by roi.  If roi is not defined it will be all
 /// of dst, if dst is defined, or all of src, if dst is not yet defined.
 /// In-place operations (dst == src) are supported.
 ///
-/// If unpremult is true, unpremultiply before color conversion, then
-/// premultiply after the color conversion.  You may want to use this
-/// flag if your image contains an alpha channel.
+/// If unpremult is true, divide the RGB channels by alpha (if it exists and
+/// is nonzero) before color conversion, then re-multiply by alpha after the
+/// after the color conversion. Passing unpremult=false skips this step,
+/// which may be desirable if you know that the image is "unassociated alpha"
+/// (a.k.a. "not pre-multiplied colors").
 ///
 /// Return true on success, false on error (with an appropriate error
 /// message set in dst).
 bool OIIO_API ociodisplay (ImageBuf &dst, const ImageBuf &src,
                         string_view display, string_view view,
                         string_view from="", string_view looks="",
-                        bool unpremult=false,
+                        bool unpremult=true,
                         string_view key="", string_view value="",
                         ColorConfig *colorconfig=NULL,
                         ROI roi=ROI::All(), int nthreads=0);
@@ -1067,15 +1073,17 @@ bool OIIO_API ociodisplay (ImageBuf &dst, const ImageBuf &src,
 /// size as specified by roi.  If roi is not defined it will be all
 /// of dst, if dst is defined, or all of src, if dst is not yet defined.
 ///
-/// If unpremult is true, unpremultiply before color conversion, then
-/// premultiply after the color conversion.  You may want to use this
-/// flag if your image contains an alpha channel. 
+/// If unpremult is true, divide the RGB channels by alpha (if it exists and
+/// is nonzero) before color conversion, then re-multiply by alpha after the
+/// after the color conversion. Passing unpremult=false skips this step,
+/// which may be desirable if you know that the image is "unassociated alpha"
+/// (a.k.a. "not pre-multiplied colors").
 ///
 /// Return true on success, false on error (with an appropriate error
 /// message set in dst).
 bool OIIO_API ociofiletransform (ImageBuf &dst, const ImageBuf &src,
                                  string_view name,
-                                 bool unpremult=false, bool inverse=false,
+                                 bool unpremult=true, bool inverse=false,
                                  ColorConfig *colorconfig=NULL,
                                  ROI roi=ROI::All(), int nthreads=0);
 

src/libOpenImageIO/imagebufalgo_pixelmath.cpp

@@ -562,7 +562,11 @@ ImageBufAlgo::unpremult (ImageBuf &dst, const ImageBuf &src,
 {
     if (! IBAprep (roi, &dst, &src, IBAprep_CLAMP_MUTUAL_NCHANNELS))
         return false;
-    if (src.spec().alpha_channel < 0) {
+    if (src.spec().alpha_channel < 0 
+        // Wise?  || src.spec().get_int_attribute("oiio:UnassociatedAlpha") != 0
+        ) {
+        // If there is no alpha channel, just *copy* instead of dividing
+        // by alpha.
         if (&dst != &src)
             return paste (dst, src.spec().x, src.spec().y, src.spec().z,
                           roi.chbegin, src, roi, nthreads);
@@ -571,6 +575,8 @@ ImageBufAlgo::unpremult (ImageBuf &dst, const ImageBuf &src,
     bool ok;
     OIIO_DISPATCH_COMMON_TYPES2 (ok, "unpremult", unpremult_, dst.spec().format,
                           src.spec().format, dst, src, roi, nthreads);
+    // Mark the output as having unassociated alpha
+    dst.specmod().attribute ("oiio:UnassociatedAlpha", 1);
     return ok;
 }
 
@@ -624,6 +630,8 @@ ImageBufAlgo::premult (ImageBuf &dst, const ImageBuf &src,
     bool ok;
     OIIO_DISPATCH_COMMON_TYPES2 (ok, "premult", premult_, dst.spec().format,
                           src.spec().format, dst, src, roi, nthreads);
+    // Clear the output of any prior marking of associated alpha
+    dst.specmod().erase_attribute ("oiio:UnassociatedAlpha");
     return ok;
 }
 

src/oiiotool/oiiotool.cpp

@@ -1761,6 +1761,7 @@ class OpColorConvert : public OiiotoolOp {
         }
     virtual void option_defaults () {
         options["strict"] = "1";
+        options["unpremult"] = "0";
     }
     virtual bool setup () {
         if (fromspace == tospace) {
@@ -1777,8 +1778,12 @@ class OpColorConvert : public OiiotoolOp {
         string_view contextkey = options["key"];
         string_view contextvalue = options["value"];
         bool strict = Strutil::from_string<int>(options["strict"]);
+        bool unpremult = Strutil::from_string<int>(options["unpremult"]);
+        if (unpremult && img[1]->spec().get_int_attribute("oiio:UnassociatedAlpha") && img[1]->spec().alpha_channel >= 0) {
+            ot.warning (opname(), "Image appears to already be unassociated alpha (un-premultiplied color), beware double unpremult. Don't use --unpremult and also --colorconvert:unpremult=1.");
+        }
         bool ok = ImageBufAlgo::colorconvert (*img[0], *img[1],
-                                              fromspace, tospace, false,
+                                              fromspace, tospace, unpremult,
                                               contextkey, contextvalue,
                                               &ot.colorconfig);
         if (!ok && !strict) {
@@ -1822,6 +1827,7 @@ class OpOcioLook : public OiiotoolOp {
     virtual void option_defaults () {
         options["from"] = "current";
         options["to"] = "current";
+        options["unpremult"] = "0";
     }
     virtual int impl (ImageBuf **img) {
         string_view lookname = args[1];
@@ -1830,12 +1836,13 @@ class OpOcioLook : public OiiotoolOp {
         string_view contextkey = options["key"];
         string_view contextvalue = options["value"];
         bool inverse = Strutil::from_string<int> (options["inverse"]);
+        bool unpremult = Strutil::from_string<int>(options["unpremult"]);
         if (fromspace == "current" || fromspace == "")
             fromspace = img[1]->spec().get_string_attribute ("oiio:Colorspace", "Linear");
         if (tospace == "current" || tospace == "")
             tospace = img[1]->spec().get_string_attribute ("oiio:Colorspace", "Linear");
         return ImageBufAlgo::ociolook (*img[0], *img[1], lookname,
-                                       fromspace, tospace, false, inverse,
+                                       fromspace, tospace, unpremult, inverse,
                                        contextkey, contextvalue,
                                        &ot.colorconfig);
     }
@@ -1851,6 +1858,7 @@ class OpOcioDisplay : public OiiotoolOp {
         : OiiotoolOp (ot, opname, argc, argv, 1) { }
     virtual void option_defaults () {
         options["from"] = "current";
+        options["unpremult"] = "0";
     }
     virtual int impl (ImageBuf **img) {
         string_view displayname  = args[1];
@@ -1859,12 +1867,13 @@ class OpOcioDisplay : public OiiotoolOp {
         string_view contextkey   = options["key"];
         string_view contextvalue = options["value"];
         bool override_looks = options.find("looks") != options.end();
+        bool unpremult = Strutil::from_string<int>(options["unpremult"]);
         if (fromspace == "current" || fromspace == "")
             fromspace = img[1]->spec().get_string_attribute ("oiio:Colorspace", "Linear");
         return ImageBufAlgo::ociodisplay (*img[0], *img[1], displayname,
                              viewname, fromspace,
                              override_looks ? options["looks"] : std::string(""),
-                             false, contextkey, contextvalue, &ot.colorconfig);
+                             unpremult, contextkey, contextvalue, &ot.colorconfig);
     }
 };
 
@@ -1876,12 +1885,15 @@ class OpOcioFileTransform : public OiiotoolOp {
 public:
     OpOcioFileTransform (Oiiotool &ot, string_view opname, int argc, const char *argv[])
         : OiiotoolOp (ot, opname, argc, argv, 1) { }
-    virtual void option_defaults () { }
+    virtual void option_defaults () {
+        options["unpremult"] = "0";
+    }
     virtual int impl (ImageBuf **img) {
         string_view name = args[1];
         bool inverse = Strutil::from_string<int> (options["inverse"]);
+        bool unpremult = Strutil::from_string<int>(options["unpremult"]);
         return ImageBufAlgo::ociofiletransform (*img[0], *img[1], name,
-                                       false, inverse, &ot.colorconfig);
+                                                inverse, unpremult, &ot.colorconfig);
     }
 };
 
@@ -2504,8 +2516,33 @@ BINARY_IMAGE_COLOR_OP (absdiffc, ImageBufAlgo::absdiff, 0);
 BINARY_IMAGE_COLOR_OP (powc, ImageBufAlgo::pow, 1.0f);
 
 UNARY_IMAGE_OP (abs, ImageBufAlgo::abs);
-UNARY_IMAGE_OP (unpremult, ImageBufAlgo::unpremult);
-UNARY_IMAGE_OP (premult, ImageBufAlgo::premult);
+
+
+
+class OpPremult : public OiiotoolOp {
+public:
+    OpPremult (Oiiotool &ot, string_view opname, int argc, const char *argv[])
+        : OiiotoolOp (ot, opname, argc, argv, 1) {}
+    virtual int impl (ImageBuf **img) {
+        return ImageBufAlgo::premult (*img[0], *img[1]);
+    }
+};
+OP_CUSTOMCLASS (premult, OpPremult, 1);
+
+
+
+class OpUnpremult : public OiiotoolOp {
+public:
+    OpUnpremult (Oiiotool &ot, string_view opname, int argc, const char *argv[])
+        : OiiotoolOp (ot, opname, argc, argv, 1) {}
+    virtual int impl (ImageBuf **img) {
+        if (img[1]->spec().get_int_attribute("oiio:UnassociatedAlpha") && img[1]->spec().alpha_channel >= 0) {
+            ot.warning (opname(), "Image appears to already be unassociated alpha (un-premultiplied color), beware double unpremult.");
+        }
+        return ImageBufAlgo::unpremult (*img[0], *img[1]);
+    }
+};
+OP_CUSTOMCLASS (unpremult, OpUnpremult, 1);
 
 
 
@@ -5289,13 +5326,13 @@ getargs (int argc, char *argv[])
                 "--tocolorspace %@ %s", action_tocolorspace, NULL,
                     "Convert the current image's pixels to a named color space",
                 "--colorconvert %@ %s %s", action_colorconvert, NULL, NULL,
-                    "Convert pixels from 'src' to 'dst' color space (options: key=, value=)",
+                    "Convert pixels from 'src' to 'dst' color space (options: key=, value=, unpremult=)",
                 "--ociolook %@ %s", action_ociolook, NULL,
-                    "Apply the named OCIO look (options: from=, to=, inverse=, key=, value=)",
+                    "Apply the named OCIO look (options: from=, to=, inverse=, key=, value=, unpremult=)",
                 "--ociodisplay %@ %s %s", action_ociodisplay, NULL, NULL,
-                    "Apply the named OCIO display and view (options: from=, looks=, key=, value=)",
+                    "Apply the named OCIO display and view (options: from=, looks=, key=, value=, unpremult=)",
                 "--ociofiletransform %@ %s", action_ociofiletransform, NULL,
-                    "Apply the named OCIO filetransform (options: inverse=)",
+                    "Apply the named OCIO filetransform (options: inverse=, unpremult=)",
                 "--unpremult %@", action_unpremult, NULL,
                     "Divide all color channels of the current image by the alpha to \"un-premultiply\"",
                 "--premult %@", action_premult, NULL,

testsuite/oiiotool-spi/ref/out.txt

@@ -1,5 +1,5 @@
-Reading iffout_vd8.1001.iff
-iffout_vd8.1001.iff  : 1998 x 1080, 4 channel, uint8 iff
+Reading iff_vd8.1001.iff
+iff_vd8.1001.iff     : 1998 x 1080, 4 channel, uint8 iff
     SHA-1: 8DCC7F60EEA962A70ACE8E443BFA73D899123332
     channel list: R, G, B, A
     tile size: 64 x 64
@@ -16,3 +16,5 @@ Comparing "ep0400-v2_bg1_v101_1kalxog_vd8.1001.jpg" and "../../../../../spi-oiio
 PASS
 Comparing "os0225_110_lightingfix_v002.0101.png" and "../../../../../spi-oiio-tests/ref/os0225_110_lightingfix_v002.0101.png"
 PASS
+Comparing "iff_vd8.1001.iff" and "../../../../../spi-oiio-tests/ref/iff_vd8.1001.iff"
+PASS