Handling profiles as a gpointer rather than a GObject due to resource use issues seems like a weird reasoning. If something is a GObject then its very easy to share between e.g. different pixbufs. If something is a gpointer then such reuse is much harder. 

I also don't understand "without copying this into an object". There is nothing inherent about GObjects that means they can't refer to const data without copying it.

It would seem like a better approach to have some way to do implementation specific profile object implementations that can store pre-parsed (to some extent) profiles that can be easily shared between different users (like the default output profile) with easy memory management semantics.

Review of attachment 151354 [details] [review]:

Here is a first round of reviewing.

::: configure.in
@@ +1986,3 @@
+           [AC_HELP_STRING([--with-gtk-giomodule-dir=PATH],
+                           [Load gio modules from this directory [LIBDIR/gtk-2.0/${GTK_BINARY_VERSION}/giomodules]])],
+           [],

I'm not sure it makes sense to allow setting the giomodules directory like this. We don't do that for any of the other module directories, and there is no way to extract this value when building giomodule, while you can extract the $LIBDIR/gtk-2.0/ value using pkg-config.

::: gdk/gdk.h
@@ +47,3 @@
 #include <gdk/gdkpango.h>
 #include <gdk/gdkpixbuf.h>
+#include <gdk/gdkicctransform.h>

On second thought, this should probably be in gtk rather than gtk, since its not part of the windowing portability layer as such, but rather an implementation-independent object.

::: gdk/gdkicctransform.c
@@ +323,3 @@
+    case PROP_BLACK_POINT_COMPENSATION:
+      g_value_set_boolean (value, priv->black_point_compensation);
+      break;

I guess input and output profiles should be properties too

@@ +343,3 @@
+    {
+    case PROP_INTENT:
+      priv->intent = g_value_get_uint (value);

You need to emit notify when changing property values

@@ +369,3 @@
+    * GdkIccTransform:intent:
+    */
+  pspec = g_param_spec_uint ("intent", "rendering intent", NULL,

this should be a proper enum param, not a uint

::: gdk/gdkicctransform.h
@@ +62,3 @@
+{
+        GObjectClass         parent_class;
+        gboolean         (*overwrite)                           (GdkIccTransform      *icc_transform,

The virtual methods should have the same name as the functions

@@ +90,3 @@
+GType                    gdk_icc_transform_get_type             (void);
+GdkIccTransform         *gdk_icc_transform_new                  (GError              **error);
+gboolean                 gdk_icc_transform_clear_input          (GdkIccTransform      *icc_transform,

How is this different from set_input_profile(NULL), and why not clear_input_profile

::: gtk/gtkmain.c
@@ +823,3 @@
 }
 
+G_LOCK_DEFINE_STATIC (registered_extensions);

I don't think these locks are needed. Gtk+ is already protected by the gdk lock, isn't that taken always when these are called.

@@ +1015,3 @@
     return FALSE;
 
+  _gtk_io_modules_ensure_loaded ();

I think it makes more sense to call this on demand when we need to look up an extension, not for every app on startup.

::: modules/giomodules/lcms/gdkicctransformlcms.c
@@ +56,3 @@
+
+  /* no alpha channel */
+  if (G_LIKELY(!has_alpha))

Why is it unlikely that there is alpha?
In general, I'd prefer to avoid sprinkling G_LIKELY stuff around. General non-hinted conditionals are automatically handled pretty well. However if you hint on something then the cost for that hint being wrong goes up a lot more than with no hinting. So, only hint for likely in things like error cases and where performance actually matters.

@@ +369,3 @@
+{
+}
+

Ideally you want a g_io_module_query too so we can do lazy loading (and avoid loading lcms for every app on startup).
This also requires a call to gio-querymodules in the makefiles

New ideas:

<alex> gtk_color_engine_get_default() - uses giomodule to look up the default implementation of the GtkColorEngine interface
<alex> (interfaces are better than base classes, we can extend them without padding)
<alex> This interface has methods to create GtkColorProfile subclassed when fed icc data
<alex> additionally it has, either a) methods to transform pixbufs given two GtkColorProfile objects or, b) method to create GtkColorTransform object (in case the caching we can do with this for repeated calls is worth it)
<alex> gtk_color_engine_get_default() returns a singleton, so there is only one, similar to the GVfs object in gio

Created attachment 151415 [details] [review]
patch for comments

I've made all the changes you suggested, even the ones on IRC. I've kept GtkColorEngine a base-class for now, as I couldn't work out how gtk_color_engine_get_default() would work if it was a virtual interface.

Further review most welcome, thanks.

How is GtkColorEngine an interface in any way a problem for gtk_color_engine_get_default?

All it does is figure out the default gtype for the engine type and create an instance of that type and returns that.

Obviously there is a requirement that the gtype implements the interface just like the previous requirement was that it derived from the base class.

Review of attachment 151415 [details] [review]:

You don't have to put each class in its own file. The fallback ones are so small that it imho makes more sense to put them inside gtkcolorengine.c to avoid polluting the directory with small files. (Also, since these are not generic or public classes you can avoid a lot of the boilerplace in their definitions)

::: gtk/gtkcolorengine.c
@@ +58,3 @@
+
+  /* proxy */
+  return klass->create_transform ();

Why are you not passing in the GtkColorEngine? It might be needed for some global engine references.

@@ +66,3 @@
+ * @input_profile: a %GtkColorProfile, or %NULL
+ * @output_profile: a %GtkColorProfile, or %NULL
+ * @error: a %GError, or %NULL

There is no error argument anymore.

@@ +98,3 @@
+ * Loads an ICC file as an input engine.
+ *
+ * Return value: %TRUE if the engine was opened, otherwise %FALSE and @error is set.

This is not up-to-date

@@ +115,3 @@
+
+  /* create and set */
+  color_profile = klass->create_profile ();

We want the color engine passed to create profile too

@@ +116,3 @@
+  /* create and set */
+  color_profile = klass->create_profile ();
+  ret = gtk_color_profile_set_from_data (color_profile, data, length, error);

Doesn't it make more sense to pass the data + lenght to create_profile directly, so that you never get half-initialized profile objects without data.
It also means you don't have a public profile_set_from_data that can be used to re-set the data in a particular profile which may be complicated implementation-wise and unexpected for the user.

@@ +151,3 @@
+gtk_color_engine_finalize (GObject *object)
+{
+  G_OBJECT_CLASS (gtk_color_engine_parent_class)->finalize (object);

If you're not actually doing anything in finalize you need not override it.

@@ +193,3 @@
+  GType type;
+
+  _gtk_io_modules_ensure_loaded ();

As a minor optimization, you could move this to gtk_color_engine_get_default_cb so it will only be called once

@@ +206,3 @@
+  } else {
+    gtk_color_engine_singleton = g_object_new (type, NULL);
+    g_object_add_weak_pointer (gtk_color_engine_singleton, &gtk_color_engine_singleton);

Is it really efficient to recreate the singleton every time its used after its refcount goes to zero? Doesn't that mean you have to re-initialize lcms or whatever each time its used.
Other singletons, like gvfs live forever once they've been used once.
Consinder especially that once the last object from the module is unrefed the module will be unloaded, and lcms with it. 
Does lcms correctly handle being unloaded? 
When the module is used the next time it will be loaded again, does lcms and your initialization code of this handle that correctly?

::: gtk/gtkcolorenginefallback.c
@@ +60,3 @@
+gtk_color_engine_fallback_finalize (GObject *object)
+{
+  G_OBJECT_CLASS (gtk_color_engine_fallback_parent_class)->finalize (object);

Again, empty finalizers are not needed.

::: gtk/gtkcolorenginefallback.h
@@ +46,3 @@
+{
+  GtkColorEngineClass parent_class;
+  /* padding for future expansion */

No need for padding in private classes

::: gtk/gtkcolortransform.c
@@ +100,3 @@
+
+  /* set new profile */
+  g_object_set (color_transform, "input-profile", profile, NULL);

Its generally more common do implement the property setter in terms of direct c calls rather than the other way around, so that the direct C calls don't have to have all the overview of properties.
Also, if you do this and combine with the above unref you get a lot less code.

Additionally, you may want to call some vtable function after the input and output profiles have changed so that the implementation can reinitialize its state.

@@ +205,3 @@
+  /* check the pixbufs are compatible */
+  if (pixbuf_in != pixbuf_out)
+    {

It seems to me that there is a lot of work going on here to verify that the out pixbuf is set up exactly right. Things would be a lot smoother if we just allocated a correct out pixbuf ourselves and returned that. Since we overwrite the whole destination anyway there doesn't seem to be much use in having the out pixbuf passed in as an argument like this.

::: gtk/gtkcolortransform.h
@@ +81,3 @@
+                                                                   GtkColorProfile      *profile);
+void                     gtk_color_transform_set_output_profile   (GtkColorTransform    *color_transform,
+                                                                   GtkColorProfile      *profile);

Lots of setters and getters are missing here. All properties should also have direct c calls to get/set.

::: modules/giomodules/lcms/Makefile.am
@@ +31,3 @@
+	gtkcolorprofilelcms.h
+
+libcolorenginelcms_la_LDFLAGS =  -avoid-version -module $(no_undefined)

You should add "-export-symbols-regex '^g_io_module_(load|unload|query)'" here, to ensure that moduled don't export any unnecessary symbols.

::: modules/giomodules/lcms/gtkcolorenginelcms.h
@@ +49,3 @@
+  GtkColorEngineClass parent_class;
+  /* padding for future expansion */
+  void (*_gtk_reserved1) (void);

Padding again is for classes in public headers

::: modules/giomodules/lcms/gtkcolorprofilelcms.c
@@ +96,3 @@
+  color_profile_lcms->priv = GTK_COLOR_PROFILE_LCMS_GET_PRIVATE (color_profile_lcms);
+
+  lcms_helper_init ();

There is no need for this. All GtkColorProfileLcm objects are created from GtkColorEngingeLcms, so you only need to initialize lcms from the gtk_color_engine_lcsm_init().
And, any uninitialization you need should be in the lcms engine finalizer.

::: modules/giomodules/lcms/gtkcolorprofilelcms.h
@@ +42,3 @@
+{
+   GtkColorProfile parent;
+   GtkColorProfileLcmsPrivate *priv;

You don't necessarily need even a priv here, as this is an internal class. It can change size however it wants without affecting derived classes, nor can other code access the private data since the header file is not available.

::: modules/giomodules/lcms/gtkcolortransformlcms.c
@@ +40,3 @@
+struct _GtkColorTransformLcmsPrivate
+{
+  cmsHPROFILE   srgb_profile;

Why is this in profile? Seems like it should be in the engine so that it doesn't have to be one copy per transform.
In fact, maybe there should even be a public function for the engine g_color_engine_get_srgb_profile(), so that you can get such a profile for all backends.

@@ +87,3 @@
+  g_object_get (color_transform,
+                "black-point-compensation", &black_point_compensation,
+                NULL);

This and others below would be more efficient and easier to read with direct c getters.

@@ +177,3 @@
+    input_profile_handle = priv->srgb_profile;
+  else
+    input_profile_handle = _gtk_color_profile_lcms_get_handle (input_profile);

If you skip the private stuff for these internal classes you can just dereference the object to get the handle.

@@ +186,3 @@
+
+  /* create transform */
+  transform = cmsCreateTransform (input_profile_handle, format, output_profile_handle, format, intent, flags);

How heavyweight is an object like this? Shouldn't we create one when the properties are set and then keep it. (I mean, isn't that the sole reason of the existance of the GtkColorTransform class)
This does get slightly complicated by having the pixbuf format be part of it, but in practice there are only two possible values with gdk-pixbuf, RGB_8 and RGBA_8, so we could create both and keep around.

@@ +208,3 @@
+out:
+  if (input_profile != NULL)
+    g_object_unref (input_profile);

Here you are unrefing things you didn't ref

@@ +249,3 @@
+  color_transform_lcms->priv = GTK_COLOR_TRANSFORM_LCMS_GET_PRIVATE (color_transform_lcms);
+
+  lcms_helper_init ();

Again, this should be in the engine init

::: modules/giomodules/lcms/lcms-helper.c
@@ +43,3 @@
+lcms_helper_init (void)
+{
+  static gboolean inited = FALSE;

This should really be in the engine init.
Also, you can't use globals or statics like this in a module, that won't work when the module gets unloaded.

Created attachment 151470 [details] [review]
new patch

New patch, hopefully much better than the last one. I've not yet added any lcms optimisation for saving the transforms, I would like to profile it first. For a typical 10k profile generating the transform matrix is probably likely to be quicker than managing a cache.

In this patch I've also made GtkColorEngine a proper interface, thanks for your help with that. All the other points should be addressed now. Please review (again), thanks.

Review of attachment 151470 [details] [review]:

I think we want to have a at least some idea how color management applies in the cairo world. It would be wierd to build up a machinery that can deal with pixbufs here, and have nothing for the cairo side.

::: configure.in
@@ +1957,3 @@
+AC_ARG_ENABLE(lcms,
+              [AC_HELP_STRING([--disable-lcms]
+                              [disable lcms print backend])],,

Its not a print backend (I hope)

::: gtk/gtkcolorengine.h
@@ +64,3 @@
+};
+
+GType                   gtk_color_engine_get_type           (void);

Should have a G_GNUC_CONST annotation, like we do everywhere else.

::: gtk/gtkcolorenginefallback.c
@@ +38,3 @@
+/**
+ * gtk_color_engine_fallback_init:
+ **/

Please don't add empty (or non-empty) doc comments for internal api - only serves to confuse gtk-doc.

@@ +59,3 @@
+{
+  GtkColorTransformFallback *color_transform;
+  color_transform = g_object_new (GTK_TYPE_COLOR_TRANSFORM_FALLBACK, NULL);

g_object_new() just gives you a GObject*...

::: gtk/gtkcolorenginefallback.h
@@ +81,3 @@
+GType      gtk_color_engine_fallback_get_type    (void);
+GType      gtk_color_transform_fallback_get_type (void);
+GType      gtk_color_profile_fallback_get_type   (void);

G_GNUC_CONST here as well, please.

::: gtk/gtkcolortransform.c
@@ +35,3 @@
+#include <gdk/gdk.h>
+
+#include "gtktypeutils.h"

gtktypeutils.h ? Thats entirely deprecated and unnecessary.

@@ +186,3 @@
+  if (priv->input_profile == NULL)
+    return NULL;
+  return g_object_ref (priv->input_profile);

We don't have reffing getters in GTK+.

@@ +329,3 @@
+    case PROP_BLACK_POINT_COMPENSATION:
+      priv->black_point_compensation = g_value_get_boolean (value);
+      g_object_notify (object, "black-point-compensation");

Notification is automatic in set_property implementations. No need to do it manually here.

@@ +361,3 @@
+                             GTK_TYPE_COLOR_INTENT,
+                             GTK_COLOR_INTENT_PERCEPTUAL,
+                             G_PARAM_READWRITE);

Please try to match the format we use for property definitions in class_init elsewhere.

::: modules/giomodules/lcms/gtkcolortransformlcms.c
@@ +57,3 @@
+
+  /* alpha channel */
+  if (bits == 8)

Like for colorspaces, this is a lost cause. We are never going to introduce non-8-bits-per-sample pixbufs, since nobody ever checks. So don't bother.

@@ +72,3 @@
+  DWORD flags = 0;
+  if (gtk_color_transform_get_black_point_compensation (color_transform))
+    flags &= cmsFLAGS_BLACKPOINTCOMPENSATION;

This should probably be |= ?!

@@ +120,3 @@
+
+  /* CMYK not supported */
+  if (gdk_pixbuf_get_colorspace (pixbuf) != GDK_COLORSPACE_RGB)

This is a bit pointless. gdk-pixbuf never has, and in all likeliness never will, support anything but RGB.
And no code ever cares to check for it, which is a strong reason for us not to add CMYK or such a thing.

@@ +134,3 @@
+    {
+      if (error != NULL)
+        *error = g_error_new (1, 0, "format not supported");

I think g_propagate_error is the more elegant way to do this, rather than a manual error != NULL check.

@@ +167,3 @@
+  rowstride = gdk_pixbuf_get_rowstride (pixbuf);
+  p = gdk_pixbuf_get_pixels (pixbuf);
+  for (i=0; i<height; ++i)

we put spaces here: for (i = 0; i < height; i++)

@@ +198,3 @@
+
+  /* FIXME: we only need to allocate the space for the image and copy the
+   * headers, although the memcpy will be fast anyway */

Not sure what you mean here ? gdk_pixbuf_copy is essentially a memcpy...

::: modules/giomodules/lcms/lcms-helper.c
@@ +49,3 @@
+  cmsSetErrorHandler (lcms_helper_error_cb);
+  cmsErrorAction (LCMS_ERROR_SHOW);
+  cmsSetLanguage ("en", "US");

Is there a reason why we hardcode en_US here ?

Created attachment 151483 [details] [review]
test patch

New patch addressing all concerns except the cairo surface API. I think this would be pretty easy to add in the future, and I would need to read up on cairo internals some more before I felt comfortable adding API myself. The API we've got here already allows us to fix the primary motivator, a color managed GtkImage.

I would like to limit the initial commit to pretty much the scope of this patch, and then work on the cairo surface API and win32 engine bits after that.

Can we maybe start some discussion on the cairo list ? I think it would be good to know at least some outline of the possible cairo support before we finalize the api here.

    gtk_color_engine_singleton = g_object_new (type, NULL);
+    g_object_add_weak_pointer (gtk_color_engine_singleton, &gtk_color_engine_singleton);

I think alex' point about (not) unloading is still worth pondering here...

Also, the docs should probably some references to necessary background reading in color theory and the like. Also, it should point out that the 8-bit-per-sample
resolution of pixbufs means that you will generally receive better results when doing color correction in whatever source format you have, be it 16-bit or float, or something else, and only transform to pixbufs for display. 
That is the case, right ?

Created attachment 151529 [details] [review]
test patch

New patch which makes the lcms module never unload (just like gvfs) and cairo surface API additions. The cairo surface stuff was much simpler that I thought it would be, and was pretty simple to add. I'll work on the documentation updates over the weekend, but I thought I should show you guys the cairo and unload stuff first.

Created attachment 151533 [details] [review]
new patch, please review

New patch with all required gtk-doc love:

 configure.in                                    |   29 +
 demos/icc-profile-fish.png                      |binary
 demos/testpixbuf-color.c                        |   80 +++
 gtk/Makefile.am                                 |   10 
 gtk/gtk.h                                       |    5 
 gtk/gtkcolorengine.c                            |  212 ++++++++
 gtk/gtkcolorengine.h                            |   84 +++
 gtk/gtkcolorenginefallback.c                    |  138 +++++
 gtk/gtkcolorenginefallback.h                    |   88 +++
 gtk/gtkcolorprofile.c                           |   60 ++
 gtk/gtkcolorprofile.h                           |   66 ++
 gtk/gtkcolortransform.c                         |  570 ++++++++++++++++++++++++
 gtk/gtkcolortransform.h                         |  117 ++++
 gtk/gtkmain.c                                   |   48 ++
 gtk/gtkprivate.h                                |    2 
 modules/Makefile.am                             |    2 
 modules/giomodules/Makefile.am                  |   11 
 modules/giomodules/lcms/Makefile.am             |   34 +
 modules/giomodules/lcms/gtkcolorenginelcms.c    |  146 ++++++
 modules/giomodules/lcms/gtkcolorenginelcms.h    |   55 ++
 modules/giomodules/lcms/gtkcolorprofilelcms.c   |   84 +++
 modules/giomodules/lcms/gtkcolorprofilelcms.h   |   60 ++
 modules/giomodules/lcms/gtkcolortransformlcms.c |  306 ++++++++++++
 modules/giomodules/lcms/gtkcolortransformlcms.h |   54 ++
 24 files changed, 2258 insertions(+), 3 deletions(-)

Please review. Thanks again.

Review of attachment 151533 [details] [review]:

::: modules/giomodules/lcms/gtkcolorenginelcms.c
@@ +91,3 @@
+
+  /* this is the default fallback language */
+  cmsSetLanguage ("en", "US");

I don't think 'fallback' is true here. If I read the lcms code correctly, this selects which locale's data to extract for multiLocalizedUnicodeType properties in the profile. So shouldn't we use the current locale instead of 'en-US'?

::: modules/giomodules/lcms/gtkcolortransformlcms.c
@@ +197,3 @@
+    {
+      if (error != NULL)
+        *error = g_error_new (1, 0, "format not supported");

g_set_error[_literal](). Also, please use a real error domain, with a real error code from a public enum.

Review of attachment 151533 [details] [review]:

::: modules/giomodules/lcms/gtkcolortransformlcms.c
@@ +165,3 @@
+
+  if (format == CAIRO_FORMAT_ARGB32)
+    return TYPE_ARGB_32;

Cairo alpha mode differs from gdk-pixbuf in that its premultiplied. So a red color (r=0xff) at 50% opacity would be
r = 0x80, g=0, b=0, a=0x80.

How does lcms handle alpha? Maybe you have to unpremultiply before passing it in?

(In reply to comment #15)
> +  /* this is the default fallback language */
> +  cmsSetLanguage ("en", "US");
> 
> I don't think 'fallback' is true here. If I read the lcms code correctly, this
> selects which locale's data to extract for multiLocalizedUnicodeType properties
> in the profile. So shouldn't we use the current locale instead of 'en-US'?

No, any maybe the comment wasn't long enough. I'll explain:

A MLUC tag _can_ have translations. 99.99% of profiles don't, and just have the en-US locale string. In profiles where there is more than one entry, the en_US version of the string is usually first in the list. We're not actually returning any text data from the profile, and so we don't want the parser (lcms in this case) looking for the most-correct locale in the mluc tag.

Now, if we were returning text data from the GtkColorProfile object I would agree with you, but until them we just want the first entry for speed.

> ::: modules/giomodules/lcms/gtkcolortransformlcms.c
> @@ +197,3 @@
> +    {
> +      if (error != NULL)
> +        *error = g_error_new (1, 0, "format not supported");
> 
> g_set_error[_literal](). Also, please use a real error domain, with a real
> error code from a public enum.

Okay, thanks. I've fixed this locally and I'll post a new patch after I've fixed the cairo surface premultiply problem. Thanks for the review!

Created attachment 151682 [details] [review]
test patch

New patch, adding the pre-multiply[1] and error domain fixes. Thanks.

[1] Long term this should live in lcms as it's already tearing the formats apart. I'll talk to marti to get his view on this sort of API addition.

(In reply to comment #18)
> Created an attachment (id=151682) [details] [review]
> test patch

This patch is positively empty, only diffstat output.

Created attachment 151745 [details] [review]
new patch, please review

Gahh, sorry about that. Working late again...

Ping?

I've talked with lots of the color management dudes this weekend at FOSDEM (who know a lot more about color management than me) and they think this patch is the right thing to do. The first step for desktop color management is this API (early binding style) and then we can do full screen color management (late-binding style) when we have 3D drivers for all graphics cards (and compositing on by default), and wide gamut screens that can actually display more than just sRGB-based colors. Even with late binding we probably want to render from the input source to sRGB unless we want to track regions and profiles in the x server.

Late binding is probably the 10-year future, but this API is required for applications like shotwell to easily implement color management. Late binding requires us to do things on the GPU, and with a composited window manager (or X server) and even with this we need early-binding color management for doing non-display-output-proofing.

I hope this assures you this functionality is really required.

Richard.

We're moving to gitlab! As part of this move, we are closing bugs that haven't seen activity in more than 5 years. If this issue is still imporant to you and
still relevant with GTK+ 3.22 or master, please consider creating a gitlab issue
for it.