[Bug 765927] new plugin: ICC (International Color Consortium) correction plugin

[GStreamer (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=765927

Sebastian Dröge (slomo) <slomo at coaxion.net> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #328042|none                        |needs-work
             status|                            |

--- Comment #11 from Sebastian Dröge (slomo) <slomo at coaxion.net> ---
Review of attachment 328042:
 --> (https://bugzilla.gnome.org/review?bug=765927&attachment=328042)

::: configure.ac
@@ +3684,3 @@
 ext/gsm/Makefile
 ext/hls/Makefile
+ext/colormanagement/Makefile

I would call the plugin just lcms

::: ext/colormanagement/gstlcms.c
@@ +135,3 @@
+    );
+
+#define FLOAT_CLAMP(x) (((x) > (1.0)) ? (1.0) : (((x) < (0.0)) ? (0.0) : (x)))

#define FLOAT_CLAMP(x) (CLAMP(x, 0.0, 1.0))

@@ +226,3 @@
+      gst_static_pad_template_get (&gst_lcms_src_template));
+
+  element_class->change_state = GST_DEBUG_FUNCPTR (gst_lcms_change_state);

Why not GstBaseTransform::start/stop() instead of this?

@@ +250,3 @@
+  GstLcms *lcms = GST_LCMS (object);
+  if (lcms->color_lut)
+    free (lcms->color_lut);

g_free() and use g_malloc/g_new below for allocation

@@ +252,3 @@
+    free (lcms->color_lut);
+  g_free (lcms->inp_profile_filename);
+  g_free (lcms->dst_profile_filename);

dispose can be called multiple times, you need to guard against that...
otherwise you get double frees.
Better use finalize instead :)

@@ +253,3 @@
+  g_free (lcms->inp_profile_filename);
+  g_free (lcms->dst_profile_filename);
+  GST_LOG_OBJECT (lcms, "disposed");

Need to chain up to the parent class' dispose

@@ +257,3 @@
+
+void
+gst_lcms_set_intent (GstLcms * lcms, GstLcmsIntent intent)

Why not static?

@@ +281,3 @@
+
+GstLcmsIntent
+gst_lcms_get_intent (GstLcms * lcms)

Why not static?

@@ +288,3 @@
+
+void
+gst_lcms_set_lookup_method (GstLcms * lcms, GstLcmsLookupMethod method)

Why not static?

@@ +333,3 @@
+          && g_file_test (filename,
+              (GFileTest) (G_FILE_TEST_EXISTS | G_FILE_TEST_IS_REGULAR)))
+        lcms->inp_profile_filename = g_strdup (filename);

You need to free inp_profile_filename if it was set before

@@ +346,3 @@
+      if (g_file_test (filename,
+              (GFileTest) (G_FILE_TEST_EXISTS | G_FILE_TEST_IS_REGULAR)))
+        lcms->dst_profile_filename = g_strdup (filename);

and here

@@ +480,3 @@
+      GST_WARNING_OBJECT (lcms,
+          "Couldn't parse destination ICC profile '%s'",
+          lcms->dst_profile_filename);

That's probably a real error, no?

@@ +501,3 @@
+    cmsCloseProfile (lcms->cms_dst_profile);
+  if (lcms->cms_transform)
+    cmsDeleteTransform (lcms->cms_transform);

Probably want to set those to NULL afterwards

@@ +510,3 @@
+
+  color_max = 0x01000000;
+  lcms->color_lut = (guint32 *) malloc (color_max * sizeof (guint32));

g_new(guint32, color_max), and check if it exists before (and if it does,
g_free() if needed)

@@ +523,3 @@
+        cmsCreateTransform (lcms->cms_inp_profile, TYPE_RGB_8,
+        lcms->cms_dst_profile, TYPE_RGB_8, lcms->intent, 0);
+    //!!FIXME use cmsFLAGS_COPY_ALPHA when new lcms2 2.8 release is available

No C99/C++ comments

@@ +671,3 @@
+      const gchar *icc_name;
+      icc_name = gst_structure_get_string (structure, "icc_name");
+      //     g_mutex_lock (&lcms->ass_mutex);

Comment to go away :)

@@ +690,3 @@
+          "disregarding embedded ICC profile because input profile file was
explicitly specified");
+  } else
+    GST_DEBUG_OBJECT (lcms, "attachment is not an ICC profile");

Curly braces please

@@ +802,3 @@
+          "GST_LCMS_LOOKUP_METHOD_UNCACHED WITHOUT alpha AND WITHOUT
preserve-black -> picture-at-once transformation!");
+      cmsDoTransformStride (lcms->cms_transform, in_data, out_data,
+          height * width, out_pixel_stride);

Shouldn't this use the row stride / row wrap somewhere?

::: ext/colormanagement/gstlcms.h
@@ +30,3 @@
+#include <lcms2.h>
+
+G_BEGIN_DECLS typedef enum lcms_intent

Should get a newline before the typedef, and no name for the enum
("lcms_intent"). Same for the other enums

@@ +100,3 @@
+GType gst_lcms_lookup_method_get_type (void);
+void gst_lcms_set_lookup_method (GstLcms * lcms, GstLcmsLookupMethod method);
+GstLcmsLookupMethod gst_lcms_get_lookup_method (GstLcms * lcms);

For bonus points, mark them all G_GNUC_INTERNAL

-- 
You are receiving this mail because:
You are the QA Contact for the bug.
You are the assignee for the bug.