[gstreamer-bugs] [Bug 616814] Photography interface extension

[GStreamer (bugzilla.gnome.org)]

https://bugzilla.gnome.org/show_bug.cgi?id=616814
  GStreamer | gst-plugins-bad | git

Tim-Philipp Müller <t.i.m> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #159566|none                        |reviewed
             status|                            |

--- Comment #1 from Tim-Philipp Müller <t.i.m at zen.co.uk> 2010-04-26 09:41:03 UTC ---
Review of attachment 159566:
 --> (https://bugzilla.gnome.org/review?bug=616814&attachment=159566)

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.

-- 
Configure bugmail: https://bugzilla.gnome.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the QA contact for the bug.
You are the assignee for the bug.