[Bug 763026] dc1394: port to 1.X

[GStreamer (GNOME Bugzilla)]

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

--- Comment #4 from Joan Pau <joanpau.beltran at socib.cat> ---
(In reply to Tim-Philipp Müller from comment #3)
> Comment on attachment 327650 [details] [review]
> 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.

All usages seem correct to me then.
Should GST_ELEMENT_ERROR be used in the set_property method (or some callee)
if a property can not be set to the given value (e.g. set_prop_iso_speed)?

> >  - 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.

Ok, I think that the device provider/monitor can be added after merging this
in,
in a different bug/request.

What type of property do you think is more suitable: string or 64 bit integral?
I did not find any similar case in the plugin suite to inspire me.
If using a 64 integer type, how would the property definition look like?
And what format will the user have to use in gst-launch pipeline syntax?:
  'dc1394src guid=0123456789abcdef   ! autovideosink' (hex without prefix)
  'dc1394src guid=0x0123456789abcdef ! autovideosink' (hex with 0x prefix)
  'dc1394src guid=81985529216486895  ! autovideosink' (decimal)
The third would be quite annoying actually,
because the GUID is usually provided in hex code.
The second one just a little bit (two extra key strokes for the prefix).

> >  - 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

Ok, and in fact that line is wrong. The name of the element is dc1394src:

  gst-launch -v dc1394src camera-number=0 ! videoconvert ! autovideosink

> > 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.

Ok, does that mean that we agree on my point about the camera operation
and the states transitions? It seems odd to me to start the transmission
in the set_caps method, but I can not figure out any other way that would work.

> >+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.

Would something like the following be ok?
(note that we are not using the enum values of `dc1394speed_t` here,
to allow setting the property by enum value with intuitive numbers):

#define GST_TYPE_DC1394_ISO_SPEED (gst_dc1394_iso_speed_get_type ())
static GType
gst_dc1394_iso_speed_get_type (void)
{
  static const GEnumValue iso_speeds[] = {
    {100, "DC1394 ISO speed 100", "100"},
    {200, "DC1394 ISO speed 200", "200"},
    {400, "DC1394 ISO speed 400", "400"},
    {800, "DC1394 ISO speed 800", "800"},
    {1600, "DC1394 ISO speed 1600", "1600"},
    {3200, "DC1394 ISO speed 3200", "3200"},
    {0, NULL, NULL}
  };
  static GType type = 0;

  if (!type) {
    type = g_enum_register_static ("GstDC1394ISOSpeed", iso_speeds);
  }
  return type;
}

With this one of the switch statements in open_cam and in set_prop_iso_speed
are redundant and one could be removed.

Could you also confirm the doubt about the IYU2 format in my first point:
Version 0.X maps DC1394_COLOR_CODING_YUV444 to IYU2 fourcc format,
which is no longer defined in 1.X. It is Y444 format the right equivalent?

And a final question about how should I proceed to make the merging easier.
Should I amend the last commit to produce a patch with the new changes,
or create new commits and provide consecutive patches?

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