GNOME Bugzilla – Bug 765927
new plugin: ICC (International Color Consortium) correction plugin
Last modified: 2017-10-20 09:48:19 UTC
Created attachment 327203 [details] [review] initial add ICC (International Color Consortium) correction plugin This is my initial proposal for a plugin that does a color correction correspondingly to the given source (and optionally destination) ICC profile file with the desired rendering intent (Perceptual, Relative Colorimetric, Saturation, Absolute Colorimetric) This conversion uses the library SampleICC which is the ICC's reference implementation written in c++- Since the algorithm is very complex (e.g. needs multiple cubic root operations per pixel) it runs unsuitably slow for video (on my i7 it needs ~2s for every full hd frame). Therefore i came up with differend methods to speed up the lookup while still keeping it 100% correct: 1) uncached: will calculate every pixel on the fly with using SampleICC's CIccCmm::Apply method. 2) precalculated: Precalculate lookup table (of size 2**24) with every given color->replacement color combination in the entire color space. 3) cached: Allocate an empty LUT, calculate colors using method 1) as they appear for the first time, then write them to the LUT and next time, check if they've already been converted 1) may be suited best for single frames 2) may be suited if you have a lot of time before going into PLAYING available and is going to be the fastest running for high frame rates and resolutions 3) is a compromise between delay before getting READY and conversion time needed I could think of another method 4) which would make us of a pre-calculated binary lookup table that is specific to the selected combination of source and destination profile and the rendering intent. The conversion is aware of different RGB component orders, stride and alpha and should work with all 24bpp x-video/raw RGB subformats and their permutations. A thinkable issue could be conversion on big endian systems, I haven't tested that. Another thing i haven't considered is what happens to pixels that are out of gamut. Implementation of YCbCr/YUV or non-24bpp-RGB formats should be possible.
Review of attachment 327203 [details] [review]: ::: configure.ac @@ +3021,3 @@ +translit(dnm, m, l) AM_CONDITIONAL(USE_SAMPLEICC, true) +AG_GST_CHECK_FEATURE(SAMPLEICC, [ICC plugin], sampleicc, [ + PKG_CHECK_MODULES(SAMPLEICC, sampleicc >= 1.6.8, HAVE_SAMPLEICC="yes", [ It might be better to use lcms, which is actually packaged by distributions :) ::: ext/icc/gsticc.cpp @@ +254,3 @@ + char buf[64]; + + g_print ("Profile: '%s'\n", filename); Use GST_MEMDUMP_OBJECT or something here @@ +565,3 @@ + g_error_new (GST_RESOURCE_ERROR, GST_RESOURCE_ERROR_NO_SPACE_LEFT, + "Unable to open allocate memory for lookup table!"); + GstMessage *msg = gst_message_new_error (GST_OBJECT (icc), err, NULL); Use the GST_ELEMENT_ERROR() macro @@ +680,3 @@ + +static void +gst_icc_before_transform (GstBaseTransform * transform, GstBuffer * outbuf) Doesn't GstVideoFilter or basetransform do that already? If not, why not? :) @@ +838,3 @@ +plugin_init (GstPlugin * plugin) +{ + return (gst_element_register (plugin, "icc", GST_RANK_NONE, GST_TYPE_ICC)); icccorrect maybe? Something that better gives the intent
Created attachment 327238 [details] [review] initial add colormanagement plugins with lcms icc correction element Thanks for your quick review, slomo! I have incoporated all of your remarks and made a whole new approach. lcms is a factor 3 faster than the sampleicc reference implementation, so that was a very good suggestion. - renamed plugin to "colormanagement" and element to "lcms" - got rid of the profile dump function without replacement for right now, it wasn't needed for operation. - fixed GST_ELEMENT_ERROR - removed surplus before_transform vfunc
Created attachment 327276 [details] [review] initial add colormanagement plugin with lcms icc correction element V2 this is my next attempt the last version had some major issues - lcms would segfault for missing icc profiles, therefore now making sure they are specified, otherwise fall back to built-in sRGB - alpha wasn't copied properly - lcms corrupts the subsequent data when doing in-place transformation (which the doc say should work). i haven't debugged the lib yet but i sent a mail to lcms-user and worked around this by always disabling in-place transformation - added pure black preservation
(In reply to Andreas Frisch from comment #3) > Created attachment 327276 [details] [review] [review] > initial add colormanagement plugin with lcms icc correction element V2 > > this is my next attempt > the last version had some major issues > - lcms would segfault for missing icc profiles, therefore now making sure > they are specified, otherwise fall back to built-in sRGB Shouldn't it simply passthrough in that case ? (or is this what you mean ?)
yes, it does passthrough now in case there is no profile specified (no input and no output). if only one is specified, it will assume the missing one to be sRGB
Created attachment 327365 [details] [review] initial add colormanagement plugin with lcms icc correction element V3 in-place-transformation works now
Created attachment 327419 [details] [review] implement using icc profiles from attachment tags, handed down from decoder elements which extract embedded profiles from image files
Created attachment 327420 [details] [review] extract icc profiles from images and send them downstreams in an attachment tag for colormanagement plugins to use them this is a patch for pngdec from gst-plugins-good in conjunction with the addition to lcms, this enables extraction and utilization of icc profiles that are embedded in png files. profile is handed downstreams as an attachment tag.
Created attachment 328042 [details] [review] initial add colormanagement plugin with lcms icc correction (with embedded profile support) - combined embedded profile handling into inital add patch - solved sticky tag misordering issue by using transformation base class instead of sink pad event handler
Review of attachment 327420 [details] [review]: This should probably be a GstMeta or custom sticky event, GstMeta seems more correct ::: ext/libpng/gstpngdec.c @@ +358,3 @@ + + if (ret & PNG_INFO_iCCP) { + icc_result = malloc (icc_proflen); g_malloc() or even better g_new() :) @@ +387,3 @@ + taglist = gst_tag_list_new_empty (); + gst_tag_list_add (taglist, GST_TAG_MERGE_APPEND, GST_TAG_ATTACHMENT, + tagsample, NULL); gst_video_decoder_merge_tags() if you use tags, otherwise you override other tags. But this does not seem like the job for a tag.
Review of attachment 328042 [details] [review]: ::: 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
Created attachment 330493 [details] [review] initial add colormanagement plugin with lcms icc correction (with embedded profile support, v4) incorporate slomo's remarks remarks/exceptions: -left plugin collection name as "colormanagement" since it's a lot more descriptive than lcms by itself -had to keep GST_STATE_CHANGE_PAUSED_TO_PLAYING because basetransform->start() is too early (profiles not set a that point) -left pngdec to pass extracted embedded icc profiles downstreams by attachment
Created attachment 330494 [details] [review] extract icc profiles from images and send them downstreams in an attachment tag for colormanagement plugins to use them fix needless malloc
is there still any interest to add this to the plugins-bad collection and if yes, what's missing?
Created attachment 361846 [details] [review] initial add colormanagement plugin with lcms icc correction (with embedded profile support, v5) updated patch, properly applies to current git master 1.13.0.1
Review of attachment 330494 [details] [review]: ::: ext/libpng/gstpngdec.c @@ +356,3 @@ + &icc_compression_type, &icc_profile, &icc_proflen); + + if (ret & PNG_INFO_iCCP) { Put this into double parenthesis, some compilers complain otherwise @@ +358,3 @@ + if (ret & PNG_INFO_iCCP) { + gpointer gst_icc_prof = g_memdup (icc_profile, icc_proflen); + if (gst_icc_prof != NULL) { This will never be NULL @@ +375,3 @@ + + if (icc_name) + gst_structure_set (info, "icc_name", G_TYPE_STRING, icc_name, NULL); We usually use dash and not underscore as separator @@ +388,3 @@ + + tag_event = gst_event_new_tag (gst_tag_list_copy (taglist)); + gst_pad_push_event (GST_VIDEO_DECODER_SRC_PAD (pngdec), tag_event); gst_video_decoder_merge_tags() would be better than manual tags handling here. Also for any other tags handling pngdec might already do. You're otherwise here currently overriding any other tags with yours.
Created attachment 361869 [details] [review] initial add colormanagement plugin with lcms icc correction (with embedded profile support, v6) corrected passthrough handling, renamed "icc-name" structure according to standard
Created attachment 361870 [details] [review] extract icc profiles and send them downstreams for colormanagement plugin v3 incorporate slomo's remarks
Review of attachment 361846 [details] [review]: 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@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
Review of attachment 361870 [details] [review]: ::: ext/libpng/gstpngdec.c @@ +372,3 @@ + gst_structure_set (info, "icc-name", G_TYPE_STRING, icc_name, NULL); + + tagsample = gst_sample_new (tagbuffer, caps, NULL, info); I think the info struct is leaked
Created attachment 361873 [details] [review] initial add colormanagement plugin with lcms icc correction (with embedded profile support, v7) cosmetics
Created attachment 361875 [details] [review] extract icc profiles and send them downstreams for colormanagement plugin v4 don't leak taglist
Comment on attachment 361873 [details] [review] initial add colormanagement plugin with lcms icc correction (with embedded profile support, v7) commit 07d6b7f56d68a6465f34fa85accc1db0238f4440 (HEAD -> master) Author: Andreas Frisch <fraxinas@dreambox.guru> Date: Thu Oct 19 16:01:46 2017 +0200 lcms: Add LCMS ICC color correction element https://bugzilla.gnome.org/show_bug.cgi?id=765927
commit 9fd988170ac4631565434d91ee8f4b4e3eccf30d (HEAD -> master) Author: Andreas Frisch <fraxinas@dreambox.guru> Date: Thu Oct 19 16:17:45 2017 +0200 pngdec: Extract icc profiles and send them downstreams for colormanagement elements https://bugzilla.gnome.org/show_bug.cgi?id=765927
We're getting compilation regression since last commit on systems with libpng12-dev (version 1.2.50): gstpngdec.c: In function 'gst_pngdec_caps_create_and_set': gstpngdec.c:353:9: error: passing argument 5 of 'png_get_iCCP' from incompatible pointer type [-Werror] &icc_compression_type, &icc_profile, &icc_proflen); ^ In file included from /usr/include/libpng12/png.h:540:0, from gstpngdec.h:28, from gstpngdec.c:31: /usr/include/libpng12/png.h:2490:31: note: expected 'png_charpp' but argument is of type 'png_byte **' extern PNG_EXPORT(png_uint_32,png_get_iCCP) PNGARG((png_structp png_ptr, ^ /usr/include/libpng12/pngconf.h:1523:58: note: in definition of macro 'PNG_EXPORT' # define PNG_EXPORT(type,symbol) PNG_IMPEXP type PNGAPI symbol ^ cc1: all warnings being treated as errors make[3]: *** [libgstpng_la-gstpngdec.lo] Error 1
... and systems with libpng12-dev version 1.2.54
configure.ac has: dnl *** libpng *** translit(dnm, m, l) AM_CONDITIONAL(USE_LIBPNG, true) AG_GST_CHECK_FEATURE(LIBPNG, [Portable Network Graphics library], png, [ AG_GST_PKG_CHECK_MODULES(LIBPNG, libpng >= 1.2) ]) ... so either need to bump up required version in configure.ac or make last patch compatible with libpng 1.2.
Created attachment 361889 [details] [review] pngdec: fix build with libpng versions between 1.2 and 1.5.1 i checked libpng's changelog: version 1.5.1beta01 [January 8, 2011] Added a note in the manual that the type of the iCCP profile was changed from png_charpp to png_bytepp in png_get_iCCP(). This change happened in version 1.5.0beta36 but is not noted in the CHANGES. Similarly, it was changed from png_charpp to png_const_bytepp in png_set_iCCP(). so the legacy versions between 1.2 and 1.5.1 should now be built without the icc feature
Review of attachment 361889 [details] [review]: hmmm... this still does not fix compilation for libpng 1.2.x. I see HAVE_LIBPNG_1_5 still gets defined in config.h on my end for libpng 1.2.x. (`pkg-config --modversion libpng` returns 1.2.50).
Review of attachment 361889 [details] [review]: Using a different prefix in the second pkg_check_modules will fix it.
Attachment 361889 [details] pushed as 5615ec5 - pngdec: fix build with libpng versions between 1.2 and 1.5.1
Created attachment 361921 [details] [review] pngdec: fix build with libpng versions between 1.2 and 1.5.1 (revised)
Comment on attachment 361921 [details] [review] pngdec: fix build with libpng versions between 1.2 and 1.5.1 (revised) Attachment 361921 [details] pushed as 39785d9 - pngdec: fix build with libpng versions between 1.2 and 1.5.1 (revised)