[Bug 765927] new plugin: ICC (International Color Consortium) correction plugin
GStreamer (GNOME Bugzilla)
bugzilla at gnome.org
Tue Jun 21 18:40:59 UTC 2016
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.
More information about the gstreamer-bugs
mailing list