From: Eli Zaretskii <eliz@gnu.org>
To: Mark Oteiza <mvoteiza@udel.edu>
Cc: 28400@debbugs.gnu.org
Subject: bug#28400: 26.0.50; lcms2 bindings
Date: Tue, 12 Sep 2017 18:53:00 +0300 [thread overview]
Message-ID: <834ls7rk0z.fsf@gnu.org> (raw)
In-Reply-To: <20170911231006.GA5455@holos.localdomain> (message from Mark Oteiza on Mon, 11 Sep 2017 19:10:06 -0400)
> Date: Mon, 11 Sep 2017 19:10:06 -0400
> From: Mark Oteiza <mvoteiza@udel.edu>
> Cc: 28400@debbugs.gnu.org
>
> > Oh, I see. But in that case, I think adding a new file and the
> > configure.ac business is actually quite easy.
>
> Humble beginnings attached.
Thanks, it looks good. A few comments:
> diff --git a/src/lcms.c b/src/lcms.c
> new file mode 100644
> index 0000000000..560b262818
> --- /dev/null
> +++ b/src/lcms.c
> @@ -0,0 +1,157 @@
> +/* Interface to Little CMS
> + Copyright (C) 2017 Mark Oteiza <mvoteiza@udel.edu>
> +
> +This file is NOT part of GNU Emacs.
This will have to change to our standard preamble, of course.
> +DEFUN ("lcms-cie-de2000", Flcms_cie_de2000, Slcms_cie_de2000, 2, 2, 0,
> + doc: /* Compute CIEDE2000 metric distance between COLOR1 and COLOR2.
> +Each color is a list of L*a*b* coordinates, where the L* channel ranges from
> +0 to 100, and the a* and b* channels range from -128 to 128.
> +Optional arguments KL, KC, KH are weighting parameters for lightness,
> +chroma, and hue, respectively. */)
> + (Lisp_Object color1, Lisp_Object color2)
I don't see any optional arguments.
> +DEFUN ("lcms-cam02-ucs", Flcms-cam02-ucs, Slcms_cam02_ucs, 2, 2, 0,
> + doc: /* Compute CAM02-UCS metric distance between COLOR1 and COLOR2.
> +Each color is a list of XYZ coordinates.
> +Optional argument is the XYZ white point, which defaults to D65. */)
> + (Lisp_Object color1, Lisp_Object color2)
Likewise.
> + k = 1.0f / (1.0f + (5.0f * vc.La));
Any reason why you use float constants, as opposed to double?
> + FL = pow(k, 4) * vc.La +
> + 0.1f * pow(1 - pow(k, 4), 2) * pow(5.0f * vc.La, 1.0f / 3.0f);
> + Mp1 = 43.86f * log(1.0f + 0.0228f * (jch1.C * pow(FL, 0.25)));
> + Mp2 = 43.86f * log(1.0f + 0.0228f * (jch2.C * pow(FL, 0.25)));
> + Jp1 = 1.7f * jch1.J / (1.0f + (0.007f * jch1.J));
> + Jp2 = 1.7f * jch2.J / (1.0f + (0.007f * jch2.J));
> + ap1 = Mp1 * cos(jch1.h);
> + ap2 = Mp2 * cos(jch2.h);
> + bp1 = Mp1 * sin(jch1.h);
> + bp2 = Mp2 * sin(jch2.h);
> + return make_float(sqrt(pow(Jp2 - Jp1, 2) +
> + pow(ap2 - ap1, 2) +
> + pow(bp2 - bp1, 2)));
I generally dislike 'pow', where simpler functions will do. 'pow' is
expensive and less accurate numerically than the alternatives (where
they exist); it also makes the code slightly harder to read and
understand. Granted, this code is unlikely to run in the inner-most
loops of some Lisp program, or to require last-ulp accuracy, so some
of these arguments are admittedly weak. But still...
So I'd replace:
pow (SOMETHING, 2) with SOMETHING*SOMETHING
pow (SOMETHING, 1./3.) with cubrt (SOMETHING)
pow (k, 4) with k * k * k * k
pow (FL, 0.25) with sqrt (sqrt (FL))
etc.
Also, a nit: we leave a blank between the function name and the
following left paren.
I think this will also need a NEWS entry, under "Installation
Changes".
Thanks again for working on this.
next prev parent reply other threads:[~2017-09-12 15:53 UTC|newest]
Thread overview: 9+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-09-09 15:50 bug#28400: 26.0.50; lcms2 bindings Mark Oteiza
2017-09-09 17:37 ` Eli Zaretskii
2017-09-10 22:04 ` Mark Oteiza
2017-09-11 15:01 ` Eli Zaretskii
2017-09-11 15:25 ` Mark Oteiza
2017-09-11 15:35 ` Eli Zaretskii
2017-09-11 23:10 ` Mark Oteiza
2017-09-12 15:53 ` Eli Zaretskii [this message]
2017-09-12 21:06 ` Mark Oteiza
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: https://www.gnu.org/software/emacs/
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=834ls7rk0z.fsf@gnu.org \
--to=eliz@gnu.org \
--cc=28400@debbugs.gnu.org \
--cc=mvoteiza@udel.edu \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://git.savannah.gnu.org/cgit/emacs.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).