Bug 17473 - grDevices::convertColor Chromatic Adaptation Is Broken
Summary: grDevices::convertColor Chromatic Adaptation Is Broken
Status: UNCONFIRMED
Alias: None
Product: R
Classification: Unclassified
Component: Add-ons (show other bugs)
Version: R 3.5.0
Hardware: All All
: P5 minor
Assignee: R-core
URL:
Depends on:
Blocks:
 
Reported: 2018-09-18 13:17 UTC by brodie.gaslam
Modified: 2018-09-22 19:11 UTC (History)
1 user (show)

See Also:


Attachments
patch to fix reported bug (1.72 KB, patch)
2018-09-18 13:17 UTC, brodie.gaslam
Details | Diff
Example of chromatic adaptation effect (image) (54.69 KB, image/png)
2018-09-18 13:18 UTC, brodie.gaslam
Details
patch that illustrates latest round of comments (2.23 KB, patch)
2018-09-22 19:11 UTC, brodie.gaslam
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description brodie.gaslam 2018-09-18 13:17:49 UTC
Created attachment 2369 [details]
patch to fix reported bug

I attempted to post this to r-devel to confirm it is a bug prior to posting here, but my plain-text e-mail sending capabilities are not good and what I sent got mangled by mailman, so I repost here as I'm reasonably confident this is a bug.

It appears that the chromatic adaptation feature of `grDevices::convertColor`is broken, and likely has been for many years.  While a little surprising, it is an obscure enough feature that there is some possibility this is actually broken, as opposed to user error on my part.

Consider:

    rgb.in <- c("#CCCCCC", "#EEEEEE")
    clr <- t(col2rgb(rgb.in)) / 255
    clr.out <- convertColor(clr, "sRGB", "sRGB")
    rgb(clr.out)
    ## [1] "#CCCCCC" "#EEEEEE"

Instead, if we try to change the whitepoint with chromatic adaptation:

    convertColor(clr, "sRGB", "sRGB", "D65", "D50")
    ## Error in match.arg(from, nWhite) :
    ##   'arg' must be NULL or a character vector

This appears to be because `grDevices:::chromaticAdaptation` expects the whitepoints to be provided in the character format (e.g. "D65"), but they are already converted by `convertColor`.  After applying the attached patch, we get:

    clr.out <- convertColor(clr, "sRGB", "sRGB", "D65", "D50")
    rgb(clr.out)
    ## [1] "#DACAB0" "#FEECCE"

I do not have a great way of confirming that the conversion is correct with my changes, but I did verify that the `M` matrix computed within`grDevics:::chromaticAdaptation` for the "D65"->"D50" conversion (approximately) matches the corresponding matrix from brucelindbloom.com chromatic adaptation page:

http://www.brucelindbloom.com/Eqn_ChromAdapt.html

Additionally visual inspection via:

    scales::show_col(c(rgb.in, rgb(clr.out)))

is consistent with a shift from bluer indirect daylight ("D65") to yellower direct daylight ("D50") illuminant.

It is worth noting that the adaption method currently in`grDevices:::chromaticAdaptation` appears to be the "Von Kries" method, not the "Bradford" method as documented in `?convertColor` and in the comments of the sources.  I base this on comparing the cone response domain matrices on the aforementioned brucelindbloom.com page to the `Ma` matrix defined in`grDevics:::chromaticAdaptation`.  If I am correct about this `?convertColor` should also be updated, or the adaptation method should be changed to "Bradford".

The attached patch is intended to minimize the required changes, but as a result it does a bit more computation than ideal (e.g. double transposition).  However, the additional computation time is small compared to the overall execution time of the function (aside: I am working on a patch to improve the performance of this function).

sessionInfo:

> sessionInfo()
R version 3.5.1 (2018-07-02)
Platform: x86_64-apple-darwin15.6.0 (64-bit)
Running under: macOS High Sierra 10.13.6

Matrix products: default
BLAS: /Library/Frameworks/R.framework/Versions/3.5/Resources/lib/libRblas.0.dylib
LAPACK: /Library/Frameworks/R.framework/Versions/3.5/Resources/lib/libRlapack.dylib

locale:
[1] en_US.UTF-8/en_US.UTF-8/en_US.UTF-8/C/en_US.UTF-8/en_US.UTF-8

attached base packages:
[1] stats     graphics  grDevices utils     datasets  methods   base     

loaded via a namespace (and not attached):
[1] compiler_3.5.1
Comment 1 brodie.gaslam 2018-09-18 13:18:59 UTC
Created attachment 2370 [details]
Example of chromatic adaptation effect (image)
Comment 2 brodie.gaslam 2018-09-20 14:24:15 UTC
Upon further examination of the sources it is now apparent to me that `convertColor` should enforce that `from.white.ref` and `to.white.ref` are length one, otherwise code like `c2to3(white.points[, from.ref.white])` will produce non-sensical results because it implicitly assumes its input to be length 2:

c2to3 <- function(col) c(col[1L]/col[2L], 1, (1 - sum(col))/col[2L])

The problem is `sum(col)`.  Two options come to mind:

1. Use `sum(col[, 1])` to silently ignore additional white points
2. Stop if `from.white.ref` and `to.white.ref` are longer than one

Incidentally, this also affects `make.rgb`.
Comment 3 Paul Murrell 2018-09-20 23:24:42 UTC
I have committed (r75340) a variation on your patch/suggestions ...

1.  Changed comment to refer to Von Kries scaling algorithm

2.  Removed the c2to3() calls within chromaticAdaptation() (because they have already happened outside chromaticAdaptation() as you pointed out)

3.  Changed references to 'from$white' to 'from$reference.white' within convertColor() (because the former is NULL), to enforce the intended semantics so that your example ...

convertColor(clr, "sRGB", "sRGB", "D65", "D50")

... is actually illegal (because sRGB has an implicit white point), but something like this actually works ...

xyzD65 <- convertColor(clr, "sRGB", "XYZ")
xyzD65
xyzD50 <- convertColor(xyzD65, "XYZ", "XYZ", "D65", "D50")
xyzD50

4.  Added the double-transposition in the call to chromaticAdaptation() within convertColor()

5.  Changed c2to3() to use ...

sum(col[1L:2L])

... to only use first two values.  There could be a whole lot more input checking added to these functions, but there are more thorough implementations of this sort of thing in packages 'colorspace' and 'colorscience' (at least), so I do not think these 'grDevices' functions are receiving heavy enough use to warrant a lot of extra work at this stage.

Confirmation that these changes produce sensible results for you would be welcome.
Comment 4 brodie.gaslam 2018-09-21 13:45:47 UTC
I will review and run tests today.
Comment 5 brodie.gaslam 2018-09-22 19:10:19 UTC
I ran some tests and the function appears to behave as expected, although there
is one change in behavior I note below in 4.

For reference the tests I ran are on github:

https://github.com/brodieG/ggbg/blob/r-devel-17473/tests/bugzilla-17473/test-convert-color.R

Additional comments (the attached patch helps to illustrate them):

1. Documentation should also be updated to reflect the correct chromatic
   adaptation.

2. Good catch on your #3 `$white` vs `$reference.white`; I missed that.
   Unfortunately this means I missed other related problems (see below).

3. I believe the likely reason for the above bug is the inconsistency between
   `make.rgb` (@63) and `colorConverter` (@96).  Both create `colorConverter` S3
   objects, but the former does it with a `reference.white` element and the
   latter with a `white` element.  If `make.rgb` used `colorConverter` to
   instantiate its `colorConverter` object this inconsistency would have been
   avoided.  As it stands the inconsistency remains and anyone using
   `colorConverter` directly with set reference whites will not get the expected
   results.

4. The bug fixed by #3 unfortunately caused incorrect conversions for color
   spaces with a non-"D65" white point.  Effectively, all RGB color spaces were
   implicitly assumed to be "D65".  Fortunately this only affects the "CIE RGB"
   color space, which I'm guessing is not broadly used.  It does mean that
   behavior of the function will change for "CIE RGB" and any other `make.rgb`
   user created color spaces with "non-D65" reference white points.
Comment 6 brodie.gaslam 2018-09-22 19:11:50 UTC
Created attachment 2371 [details]
patch that illustrates latest round of comments