[Bug 763026] dc1394: port to 1.X
GStreamer (GNOME Bugzilla)
bugzilla at gnome.org
Fri May 27 14:33:41 UTC 2016
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.
More information about the gstreamer-bugs
mailing list