[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 #361846|none                        |needs-work
             status|                            |

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

Needs meson.build integration too ideally now :)

Generally looks good, just some cosmetics

::: ext/colormanagement/gstlcms.c
@@ +3,3 @@
+ * Copyright (C) 2016 Andreas Frisch <fraxinas at dreambox.guru>
+ *
+ * gstlcms.cpp

We have a .c file here, not .cpp :)

@@ +103,3 @@
+          "Calculate and cache color replacement values on first occurence",
+        "cached"},
+//     {GST_LCMS_LOOKUP_METHOD_FILE,          "Read lookup table from file,
precalculate if non-existant",       "file"},

No // comments

@@ +204,3 @@
+          "Select whether purely black pixels should be preserved",
+          DEFAULT_PRESERVE_BLACK,
+          (GParamFlags) (G_PARAM_CONSTRUCT | G_PARAM_READWRITE |

These casts are not needed for C code, only C++

@@ +208,3 @@
+
+  g_object_class_install_property (gobject_class, PROP_EMBEDDED_PROFILE,
+      g_param_spec_boolean ("embeddedprofile", "Embedded Profile",

embedded dash profile

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

Maybe use ::start() and ::stop() here instead? But not important

@@ +252,3 @@
+  g_free (lcms->dst_profile_filename);
+  G_OBJECT_CLASS (gst_lcms_parent_class)->finalize (object);
+  GST_LOG_OBJECT (lcms, "finalized");

Don't log after chaining up, the object is gone

@@ +337,3 @@
+      } else
+        GST_WARNING_OBJECT (lcms, "Input profile file '%s' not found!",
+            filename);

Some more {}

@@ +352,3 @@
+      } else
+        GST_WARNING_OBJECT (lcms, "Destination profile file '%s' not found!",
+            filename);

Some more {}

@@ +485,3 @@
+  }
+
+  return GST_STATE_CHANGE_SUCCESS;

You never return anything else, so just return nothing :)

@@ +510,3 @@
+  guint32 p, color_max;
+
+  color_max = 0x01000000;

Make this a const maybe

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