Avoid divisions creating sample points in the float cube LUT builder

third_party/qcms/src/transform.c

 /* vim: set ts=8 sw=8 noexpandtab: */
 //  qcms
 //  Copyright (C) 2009 Mozilla Corporation
 //  Copyright (C) 1998-2007 Marti Maria
 //
 // Permission is hereby granted, free of charge, to any person obtaining 
 // a copy of this software and associated documentation files (the "Software"), 
 // to deal in the Software without restriction, including without limitation 
 // the rights to use, copy, modify, merge, publish, distribute, sublicense, 
 // and/or sell copies of the Software, and to permit persons to whom the Software 
@@ skipping 1100 matching lines @@
 qcms_transform* qcms_transform_precacheLUT_float(qcms_transform *transform, qcms_profile *in, qcms_profile *out, 
                                                  int samples, qcms_data_type in_type)
 {
         /* The range between which 2 consecutive sample points can be used to interpolate */
         uint16_t x,y,z;
         uint32_t l;
         uint32_t lutSize = 3 * samples * samples * samples;
         float* src = NULL;
         float* dest = NULL;
         float* lut = NULL;
+        float inverse;
 
         src = malloc(lutSize*sizeof(float));
         dest = malloc(lutSize*sizeof(float));
 
         if (src && dest) {
-                /* Prepare a list of points we want to sample */
+                /* Prepare a list of points we want to sample: x, y, z */
                 l = 0;
+                inverse = 1 / (float)(samples-1);
                 for (x = 0; x < samples; x++) {
                         for (y = 0; y < samples; y++) {
                                 for (z = 0; z < samples; z++) {
-                                        src[l++] = x / (float)(samples-1);
-                                        src[l++] = y / (float)(samples-1);
-                                        src[l++] = z / (float)(samples-1);
+                                        src[l++] = x * inverse; // r
+                                        src[l++] = y * inverse; // g
+                                        src[l++] = z * inverse; // b

[sugoi1, 2015/01/27 15:42:58]

You're going to have "samples * samples * samples * 3" number of multiplications
here and you're paying for int to float conversions.
You could reduce that to only "samples" number of multiplications by doing:

float* multLut = malloc(samples*sizeof(float)); // Should fit in cache
for (x = 0; x < samples; ++x) {
  multLut = x * inverse;
}

for (x = 0; x < samples; ++x) {
  for (y = 0; y < samples; ++y) {
    for (z = 0; z < samples; ++z) {
      src[l++] = multLut[x];
      src[l++] = multLut[y];
      src[l++] = multLut[z];
    }
  }
}

free(multLut);

Also, is "samples" likely to change between calls? If not, then multLut could be
static, and only be reallocated and recomputed when "samples" changes.

[Noel Gordon, 2015/01/28 05:57:29]

yes, "samples" change b/w calls.

[Noel Gordon, 2015/01/28 05:57:29]

And what followed seems fine, but the cost of this function would still be 8ms I
believe since the sample creation costs here are tiny overall.

Perhaps I could better explain; someone asked me a question. The point of this
patch is not really the hoist, but to present a diff that 1) shows how the
sampling code works so we all look at it, 2) describes where time goes, 3)
documents sample order (x,y,z) [exactly like ColorSync, Adobe, LCMS, etc] and 4)
documents output LUT format order (RGB[A]).

This to help resolve other questions: Why does Skia sample (z,y,x) order (viz.,
backwards compared to libs)? Why does it use BGR output order?  And should we
care about this in Skia, or should we fix sample order and output format to
match the library user user-case? (just imagine I want to swap out qcms for
ColorSync on mac, say).

Bug is 443863, make it easier to use for users.  I don't personally care what
the sample order / output format is, but Skia might and color-correction library
users of Skia might, so I thought I'd better advise via this bug - thoughts
skia-peeps?

[sugoi1, 2015/01/28 16:44:40]

I understand that it *can* change, but if that represents the size of the cube
of the 3D lut, why would we change this number frequently? Do we use multiple
sizes of cubes? Is "samples" used to define something else than the size of a
cube?

[sugoi1, 2015/01/28 16:44:40]

On 2015/01/28 05:57:29, noel gordon wrote:

The 443863 bug is described as an SkColorCube issue but I don't think it can be
limited to that. If we're using an SkFilter tree for the GPU case and libqcms as
a software fallback, then image would need to be in a different format for each
case. If you look at JPEGImageDecoder, you'll see (you probably wrote this)
that, in order to use libqcms, there's some temporary swizzling happening in the
decoder. JPEGImageDecoder, and every other decoder, will output to the format
skia uses and everything that touches pixels in Blink/Chromium will expect
pixels to be stored that way. Wouldn't it be much easier to get the fairly small
libqcms to use Skia/Blink/Chrome's color format than to change all of the rest
of the code? If we eventually use another library, then we'd do what we do
currently in JPEGImageDecoder when using libqcms, which is to swizzle in and out
of the library when the formats don't match.

[Noel Gordon, 2015/01/30 03:04:51]

I might use a smaller or larger cube depending on circumstances (image size,
pre-rendering, printing ...) but have not experimented with that as yet. At
least for now, I expect this number won't change frequently. We could optimize
speed around here sure, but as we've noted here and off-line, our time might
better spent elsewhere in this code.
Nope - "samples" and "cubeSize" are the same thing, and we've used both terms
interchangeably in discussions / code without confusion thus far (which is ok
with me), sorry if there was any confusion.

[Noel Gordon, 2015/01/30 18:52:21]

443863 is about cube data format I think.  Some code might help, so I changed
SkColorCube as follows:

https://gist.github.com/anonymous/1d62b4ead69bbebb5814

Didn't test the software path. The GPU path seems to color-correct properly when
I provide (x,y,z) sampled, RGBA order cube data from qcms.
We must decode / handle images in Skia platform format always: rule #1, no
exceptions :)
Yeap. That temporary swizzling happens when we bake profiles into decoded image
frames.  If decoded frames need qcms-ing, the decoders swizzle (at no cost mind)
to RGBA for input to qcms and we make qcms output platform format (rule #1).
The only thing that'd change is the color cube data format.
One thing to note: all libraries support RGB input, so swapping libraries means
no code changes at ImageDecoder level. That said, the swizzle code there will go
away once we stop baking color profiles into image frames at decode time.

In summary, I'm not proposing to change image frame format, only the cube data
format (gist above).  Point of this patch was to explore the issues. I'm also
fine with leaving things as is, and when I create cube data in qcms for Skia
use, I use (z,y,x) sampling, BGRA order. If that's still your preference, then
let's do that.

(good questions btw, thanks for those).

                                 }
                         }
                 }
 
                 lut = qcms_chain_transform(in, out, src, dest, lutSize);
+
                 if (lut) {
-                        transform->r_clut = &lut[0];
-                        transform->g_clut = &lut[1];
-                        transform->b_clut = &lut[2];
+                        transform->r_clut = &lut[0]; // r
+                        transform->g_clut = &lut[1]; // g
+                        transform->b_clut = &lut[2]; // b
                         transform->grid_size = samples;
                         if (in_type == QCMS_DATA_RGBA_8) {
                                 transform->transform_fn = qcms_transform_data_tetra_clut_rgba;
                         } else {
                                 transform->transform_fn = qcms_transform_data_tetra_clut;
                         }
                 }
         }
 
-
-        //XXX: qcms_modular_transform_data may return either the src or dest buffer. If so it must not be free-ed
+        // XXX: qcms_modular_transform_data may return the lut in either the src or the
+        // dest buffer. If so, it must not be free-ed.
         if (src && lut != src) {
                 free(src);
         }
         if (dest && lut != dest) {
                 free(dest);
         }
 
         if (lut == NULL) {
                 return NULL;
         }
@@ skipping 196 matching lines @@
         static const struct _qcms_format_type output_bgrx = { 2, 0 };
 
         transform->transform_fn(transform, src, dest, length, type == QCMS_OUTPUT_BGRX ? output_bgrx : output_rgbx);
 }
 
 qcms_bool qcms_supports_iccv4;
 void qcms_enable_iccv4()
 {
         qcms_supports_iccv4 = true;
 }