GNOME Bugzilla – Bug 616814
Photography interface extension: colour tone mode and noise reduction settings
Last modified: 2011-02-10 12:12:27 UTC
Created attachment 159566 [details] [review] Patch file for the photography extension Current gstreamer photography interface supports a limited image effects setting. Some features such as noise reduction, some colur effects are available in some digital camera device and will be supported by the ISP on Intel Atom platforms. These featue doesn't support by photography interface. The patch in the attachment will extend following two features. 1. extend the “GstColourToneMode “ structure to support sky blue, grass green, skin whiten, emboss image effect and sketch image effect in the photography interface when do the image capture through gstreamer. The color effects are listed as in below, we are proposing to extend “GstColourToneMode” to support them. GST_PHOTOGRAPHY_COLOUR_TONE_MODE_SKY_BLUE: make sky looks more blue GST_PHOTOGRAPHY_COLOUR_TONE_MODE_GRASS_GREEN : make grass looks more green GST_PHOTOGRAPHY_COLOUR_TONE_MODE_SKIN_ WHITEN: make skin looks more whiten. GST_PHOTOGRAPHY_COLOUR_TONE_MODE_ EMBOSS: emboss image effect GST_PHOTOGRAPHY_COLOUR_TONE_MODE_ SKETECH: sketch image effect 2. support the noise reduction setting in the photography interface. We are proposing to add an entry “nr_mode” to GstPhotoSettings to support the following noise reduction setting. #define GST_PHOTOGRAPHY_BAYER_NR_MODE 1<< 0 #define GST_PHOTOGRAPHY_YCC_NR_MODE 1<< 1 #define GST_PHOTOGRAPHY_TEMPORAL_NR_MODE 1<< 2 #define GST_PHOTOGRAPHY_FPN_NR_MODE 1<< 3 #define GST_PHOTOGRAPHY_EXTRA_NR_MODE 1<< 4
Review of attachment 159566 [details] [review]: The common submodule changes shouldn't be part of the patch, maybe you need to do 'git submodule update'? ::: gst-libs/gst/interfaces/photography.h @@ +80,3 @@ +#define GST_PHOTOGRAPHY_TEMPORAL_NR_MODE ( 1<< 2) +#define GST_PHOTOGRAPHY_FPN_NR_MODE (1 << 3) +#define GST_PHOTOGRAPHY_EXTRA_NR_MODE (1 << 4) Are these really flags? Ie. can these modes be combined, or is it always one mode only that's active? Also, shouldn't there be also a NONE value, or UNKNOWN value? They should probably be wrapped in a typedef enum { ... } GstFooBarSomething; (This is abusing enums a bit, but not uncommon, and makes things easier to bind automatically via glib-mkenums etc.). Please not that the order is wrong too, it should be GST_PHOTOGRAPHY_NR_MODE_XYZ (see the colour tone mode below). Lastly, I think I would prefer NOISE_REDUCTION_MODE instead of NR_MODE, even if it's considerably longer (same everywhere else). @@ +257,3 @@ gboolean (*get_flash_mode) (GstPhotography * photo, GstFlashMode * flash_mode); + gboolean (*get_nr_mode) (GstPhotography * photo, Same as above: I think {get|set}_noise_reduction_mode would be nicer.
Created attachment 159592 [details] [review] Updated photography extension patch according to Tim's comments This patch extend the photography interfance and camerabin to support the noise reduction settings and more color effect settings.
Tim Thanks for your quick reply. The noise reduction are flags which can be combined together. There is no none mode. Each of them can be controlled separated. I put them together because all of them are related to noise reduction in the camera imaging. I have updated the patch according to your comments. Thanks.
GST_PHOTOGRAPHY_COLOUR_TONE_MODE_ EMBOSS: emboss image effect GST_PHOTOGRAPHY_COLOUR_TONE_MODE_ SKETECH: sketch image effect are not color modes, they are effects. I don't think it makes much sense to apply them at the time of taking the image. I am a bit worried regarding the noise reduction. This is something applied as a postprocessing (which actually is also true for the color modes). The problem of adding all those possible image post processing function to the photography interface is that the interface get semantically diluted. The purpose of the photography interface is to specify capture settings. Ideally posprocessing effects are separate gstreamer plugins. If the post processing is done one the fly by the source, we need a better way to express it. Another problem of using the enums/flags here is, that there is no way to enumerate all possible noise filtering algorithms. If the noisefiltering would be presented as a separate gstreamer element, it can have its own api.
Emboss and sketech are not related to the color tone mode. I will delete them from the color tone mode. The noise reduction functions are not posprocessing effects. They can be turned on/off in the ISP and take effect during the image capture. I think it will be better to add them to the photography interface as they are the settings to the image capture.
(In reply to comment #5) > Emboss and sketech are not related to the color tone mode. I will delete them > from the color tone mode. Thanks. > > The noise reduction functions are not posprocessing effects. They can be turned > on/off in the ISP and take effect during the image capture. I think it will be > better to add them to the photography interface as they are the settings to the > image capture. I understand. The question here is how to agree on the canonical algorithm names. I am just a bit worried that the specific noise reduction algorithms don't map easily accross ISP. Also please reconsider the names: typedef enum { GST_PHOTOGRAPHY_NOISE_REDUCTION_MODE_BAYER = ( 1<<0 ), GST_PHOTOGRAPHY_NOISE_REDUCTION_MODE_YCC = ( 1<<1 ), GST_PHOTOGRAPHY_NOISE_REDUCTION_MODE_TEMPORAL= ( 1<< 2), GST_PHOTOGRAPHY_NOISE_REDUCTION_MODE_FPN = (1 << 3), GST_PHOTOGRAPHY_NOISE_REDUCTION_MODE_EXTRA = (1 << 4) } GstNoiseReductionMode; what about: typedef enum { GST_PHOTOGRAPHY_NOISE_REDUCTION_BAYER = ( 1<<0 ), GST_PHOTOGRAPHY_NOISE_REDUCTION_YCC = ( 1<<1 ), GST_PHOTOGRAPHY_NOISE_REDUCTION_TEMPORAL= ( 1<< 2), GST_PHOTOGRAPHY_NOISE_REDUCTION_FPN = (1 << 3), GST_PHOTOGRAPHY_NOISE_REDUCTION_EXTRA = (1 << 4) } GstPhotographyNoiseReduction; (the typename should match the enum names) Please also add a doc blob describing it briefly each one. E.g. FPN and EXTRA sounds unclear (do you have better names for them).
Created attachment 159682 [details] [review] Updated patch according to Stefan's comments Add GstPhotographyNoiseReduction to photography interface to support noise reduction settings in ISP. Add "sky blue", "grass green" ans "skin whitten" to the GstColourToneMode to support more color effect in photography interface.
Created attachment 159683 [details] [review] Replace the noise_reduction_mode with noise_reduction
This looks good now.
Review of attachment 159683 [details] [review]: Hu, sorry I forgot about the patch. I can push it for next gst-plugin-bad release though, if you could update it according to my comments. ::: gst-libs/gst/interfaces/photography.c @@ +300,3 @@ + * + * Returns: %TRUE if setting the value succeeded, %FALSE otherwise + */ Please add * Since: 0.10.21 @@ +309,3 @@ + * + * Returns: %TRUE if getting the value succeeded, %FALSE otherwise + */ Please add * Since: 0.10.21 ::: gst-libs/gst/interfaces/photography.h @@ +92,3 @@ + * of high-ISO capturing, some noise remains after YCC NR. XNR reduces this + * remaining noise. + */ Please add * Since: 0.10.21 ::: gst/camerabin/gstcamerabin.c @@ +3194,3 @@ + g_object_class_override_property (gobject_class, ARG_NOISE_REDUCTION, + GST_PHOTOGRAPHY_PROP_NOISE_REDUCTION); + we don't proxy the photo iface anymore in camerabin. applications can get the camera-src and make the changes there. Thus there is no gstcamerabinphotography.c any more. Please remove this part of the patch.
Created attachment 171073 [details] [review] Noise reduction and color effect extension to photography interface Update according to Stefan's comments. Remove the camerabin part.
I have committed this now. Please read http://gstreamer.freedesktop.org/wiki/SubmittingPatches for future patch submissions. Also please apply all suggested change (you forgot fixing the doc blobs) or explain why you leave them out. commit 8f26b414fa7e718ca65ebd6be42b4babde4039f6 Author: Hu Gang <gang.a.hu@intel.com> Date: Tue Sep 28 11:35:53 2010 +0300 photography: extend photography iface Add more color tone modes and add NoseReduction settings. Fixes #616814.
Stefan: that's a lousy commit message: the summary line should have contained the color tone mode / noise reduction stuff. 'extend iface' is about as good a summary as 'fix some stuff' ;-) Also, please add API: markers to the commit message when adding API. You may want to read http://gstreamer.freedesktop.org/wiki/GitDeveloperGuidelines again. (Looking forward to the NoseReduction feature though!) On a different note, our API uses US American English spelling, not British English spelling, so please fix up COLOUR => COLOR. Re-opening bug for that.
> On a different note, our API uses US American English spelling, not British > English spelling, so please fix up COLOUR => COLOR. Re-opening bug for that. Oh nevermind, it's just an extension, it's British spelling already *sigh*.
I added the COLOUR -> COLOR cleanup to bug #622482.
This commit broke ABI btw (adding structure members / interface methods in the middle of a public struct), for no good reason, as far as I can tell. The API is marked as unstable of course, but it would still be nice to avoid unneeded ABI breaks ;)
commit e3d120d725b56d6367e69d754e383f9e28fbfa77 Author: Thiago Santos <thiago.sousa.santos@collabora.co.uk> Date: Thu Oct 14 13:41:00 2010 -0300 photography: Avoid breaking ABI Move the newly added functions/fields to the end of the structs
Created attachment 174310 [details] [review] Noise reduction capability and property to photography interface This patch is small bug fix to bug 171073 to adding the following mssing features to photography interface. Add GST_PHOTOGRAPHY_CAPS_NOISE_REDUCTION to GstPhotoCaps. Add GST_PHOTOGRAPHY_PROP_NOISE_REDUCTION property.
This patch has nothing to do with bug 171073. It is the patch number. Add patch to add the following mssing features to photography interface. Add GST_PHOTOGRAPHY_CAPS_NOISE_REDUCTION to GstPhotoCaps. Add GST_PHOTOGRAPHY_PROP_NOISE_REDUCTION property.
Stefan, can you update this patch?
Comment on attachment 171073 [details] [review] Noise reduction and color effect extension to photography interface commit 8f26b414fa7e718ca65ebd6be42b4babde4039f6 Author: Hu Gang <gang.a.hu@intel.com> Date: Tue Sep 28 11:35:53 2010 +0300 photography: extend photography iface Add more color tone modes and add NoseReduction settings. Fixes #616814.
Comment on attachment 174310 [details] [review] Noise reduction capability and property to photography interface commit 279cda20d7ad636fba911e818b88456f8e3f7100 Author: Hu Gang <gang.a.hu@intel.com> Date: Mon Dec 20 11:14:49 2010 +0200 photography: add missing property and cabability flag for noise reduction
Gang, I pushed the patches, but wonder if we can actually set a default here as none of the noise reductions are mandatory. One should probably just set defaults in the specific implementation and do a g_object_notify() to signal the changed parameters. ALso having BAYER|YCC would sugest that a two pass noise reduction is done. Is that recommended?
Yes. They are the suggeted settings and is ON by default in atom ISP. And use the g_object_notify is a good idea for the the vendor specific settings. Thanks.
I switched the property to a GFlags type and set the defaults to 0 with another commit.
Created attachment 180547 [details] [review] Fix noise_reduction type mismatch with the property spec in photography interface Stefan, Your last update "use a flags type instead of the uint" doesn't update all the uint type. This patch update all the noise_reduction type to flags.