[Bug 763026] dc1394: port to 1.X

[GStreamer (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=763026

Tim-Philipp Müller <t.i.m at zen.co.uk> changed:

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

--- Comment #3 from Tim-Philipp Müller <t.i.m at zen.co.uk> ---
Comment on attachment 327650
  --> https://bugzilla.gnome.org/attachment.cgi?id=327650
Initial patch, working well, but with some points to discuss

Thanks for the patch!

Here are some quick drive-by comments:

>  - (Ab)use `GST_ELEMENT_ERROR` and `GST_ELEMENT_WARNING`
>    until it is clear where they should be replaced by
>    `GST_OBJECT_ERROR` and `GST_OBJECT_WARNING`.

GST_ELEMENT_ERROR() is what should be used if a fatal error is encountered and
streaming stops. GST_ELEMENT_WARNING is used very rarely and is called for when
something bad happened that one might want to notify to the user, but it's not
fatal.

GST_OBJECT_ERROR/WARNING just log something in the debug log, it's not really
for error handling.


>  - Select the camera with a camera number property as in version 0.X,
>    although it would be better to use GUID and unit properties
>    (they uniquely identify the device, while the number depends on
>    the set of cameras currently detected by the system).
>    The GUID is 64bit identifier (same as MAC address), and could be
>    handled either with an unsigned 64 bit integer type property
>    or with a string property with the hexadecimal representation
>    of the value. For practicality a default value should be used
>    such that if the property is not set the first camera enumerated
>    by the library is used. Such a default value could be the
>    empty string `""` or the value `0xffffffffffffffff`
>    (which is not a valid value for a GUID).
>    The unit is a signed integer not commonly used, and the wildcard
>    value -1 can be used to match any camera (with matching GUID).

Right. Ideally one would also implement a GstDeviceProvider/DeviceMonitor for
these cameras, then applications can simply select the right camera that way
and don't have to know which property to set to what.


>  - Keep name `GstDc1394` and prefix `gst_dc1394` as in version 0.X,
>    although `GstDC1394Src` and `gst_dc1394_src` are more descriptive.

Might be worth renaming in a separate commit later.

>- * gst-launch -v dc1394 camera-number=0 ! xvimagesink
>+ * gst-launch -v dc1394 camera-number=0 ! videoconvert ! autovideosink

gst-launch -> gst-launch-1.0

> static GstStateChangeReturn
> gst_dc1394_change_state (GstElement * element, GstStateChange transition)
> {
>-  GstStateChangeReturn ret = GST_STATE_CHANGE_SUCCESS;
>-  GstDc1394 *src = GST_DC1394 (element);
>+  GstDc1394 *src;
>+  GstStateChangeReturn ret;
>...

The change_state func should really go away.

Use GstBaseSrc::start/stop() (or open/close) instead.

>+static void
>+gst_dc1394_set_prop_iso_speed (GstDc1394 * src, guint speed)
>+{
>+  switch (speed) {
>+    case 100:
>+    case 200:
>+    case 300:
>+    case 400:
>+    case 800:
>+    case 1600:
>+    case 3200:
>+      src->iso_speed = speed;
>       break;

Maybe the iso-speed property should be an enum property instead?

In any case, if it works then perhaps we should get it in and do anything else
as follow-up patches.

-- 
You are receiving this mail because:
You are the QA Contact for the bug.
You are the assignee for the bug.