After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 616814 - Photography interface extension: colour tone mode and noise reduction settings
Photography interface extension: colour tone mode and noise reduction settings
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-bad
git master
Other All
: Normal enhancement
: 0.10.22
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks: 622482
 
 
Reported: 2010-04-26 03:15 UTC by Hu Gang
Modified: 2011-02-10 12:12 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Patch file for the photography extension (7.22 KB, patch)
2010-04-26 03:15 UTC, Hu Gang
reviewed Details | Review
Updated photography extension patch according to Tim's comments (10.34 KB, patch)
2010-04-26 12:45 UTC, Hu Gang
none Details | Review
Updated patch according to Stefan's comments (10.95 KB, patch)
2010-04-27 11:45 UTC, Hu Gang
none Details | Review
Replace the noise_reduction_mode with noise_reduction (10.90 KB, patch)
2010-04-27 11:59 UTC, Hu Gang
needs-work Details | Review
Noise reduction and color effect extension to photography interface (7.42 KB, patch)
2010-09-25 02:39 UTC, Hu Gang
committed Details | Review
Noise reduction capability and property to photography interface (1.43 KB, patch)
2010-11-12 08:17 UTC, Hu Gang
committed Details | Review
Fix noise_reduction type mismatch with the property spec in photography interface (2.38 KB, patch)
2011-02-10 02:44 UTC, Hu Gang
committed Details | Review

Description Hu Gang 2010-04-26 03:15:04 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
Comment 1 Tim-Philipp Müller 2010-04-26 09:41:03 UTC
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.
Comment 2 Hu Gang 2010-04-26 12:45:52 UTC
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.
Comment 3 Hu Gang 2010-04-26 12:49:19 UTC
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.
Comment 4 Stefan Sauer (gstreamer, gtkdoc dev) 2010-04-26 19:41:20 UTC
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.
Comment 5 Hu Gang 2010-04-27 09:47:57 UTC
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.
Comment 6 Stefan Sauer (gstreamer, gtkdoc dev) 2010-04-27 10:23:48 UTC
(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).
Comment 7 Hu Gang 2010-04-27 11:45:15 UTC
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.
Comment 8 Hu Gang 2010-04-27 11:59:47 UTC
Created attachment 159683 [details] [review]
Replace the noise_reduction_mode with noise_reduction
Comment 9 Stefan Sauer (gstreamer, gtkdoc dev) 2010-05-07 14:08:23 UTC
This looks good now.
Comment 10 Stefan Sauer (gstreamer, gtkdoc dev) 2010-09-24 09:41:39 UTC
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.
Comment 11 Hu Gang 2010-09-25 02:39:47 UTC
Created attachment 171073 [details] [review]
Noise reduction and color effect extension to photography interface

Update according to Stefan's comments. Remove the camerabin part.
Comment 12 Stefan Sauer (gstreamer, gtkdoc dev) 2010-09-28 08:38:31 UTC
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.
Comment 13 Tim-Philipp Müller 2010-09-28 09:05:17 UTC
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.
Comment 14 Tim-Philipp Müller 2010-09-28 09:06:25 UTC
> 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*.
Comment 15 Stefan Sauer (gstreamer, gtkdoc dev) 2010-09-28 09:49:32 UTC
I added the COLOUR -> COLOR cleanup to bug #622482.
Comment 16 Tim-Philipp Müller 2010-10-14 18:40:51 UTC
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 ;)
Comment 17 Stefan Sauer (gstreamer, gtkdoc dev) 2010-10-14 20:07:37 UTC
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
Comment 18 Hu Gang 2010-11-12 08:17:54 UTC
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.
Comment 19 Hu Gang 2010-11-12 08:23:01 UTC
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.
Comment 20 Hu Gang 2010-12-09 03:14:51 UTC
Stefan, can you update this patch?
Comment 21 Stefan Sauer (gstreamer, gtkdoc dev) 2010-12-20 09:08:20 UTC
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 22 Stefan Sauer (gstreamer, gtkdoc dev) 2010-12-20 09:15:54 UTC
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
Comment 23 Stefan Sauer (gstreamer, gtkdoc dev) 2010-12-20 09:20:00 UTC
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?
Comment 24 Hu Gang 2010-12-20 11:38:55 UTC
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.
Comment 25 Stefan Sauer (gstreamer, gtkdoc dev) 2010-12-20 11:51:36 UTC
I switched the property to a GFlags type and set the defaults to 0 with another commit.
Comment 26 Hu Gang 2011-02-10 02:44:30 UTC
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.