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 763026 - dc1394: port to 1.X
dc1394: port to 1.X
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-bad
unspecified
Other Linux
: Normal enhancement
: 1.9.1
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2016-03-03 09:07 UTC by Joan Pau
Modified: 2016-06-20 20:47 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Initial patch, working well, but with some points to discuss (80.92 KB, patch)
2016-05-11 16:06 UTC, Joan Pau
none Details | Review
Add support for the IYU2 format. (5.96 KB, patch)
2016-05-30 14:49 UTC, Joan Pau
committed Details | Review
docs/design: add IYU2 raw video format description (1.31 KB, patch)
2016-06-01 14:00 UTC, Joan Pau
committed Details | Review
dc1394src: port to 1.X (with so far proposed modifications) (82.08 KB, patch)
2016-06-01 15:26 UTC, Joan Pau
none Details | Review
dc1394src: port to 1.X (with so far proposed modifications and better documentation) (83.27 KB, patch)
2016-06-07 14:40 UTC, Joan Pau
none Details | Review
dc1394src: internal type, prefix and file renaming to adhere to conventions (86.92 KB, patch)
2016-06-07 14:50 UTC, Joan Pau
none Details | Review
dc1394src: port to 1.X (84.21 KB, patch)
2016-06-09 13:44 UTC, Joan Pau
committed Details | Review
dc1394src: internal type, prefix and file renaming to adhere to conventions (87.10 KB, patch)
2016-06-09 13:46 UTC, Joan Pau
committed Details | Review
dc1394src: check for disabled transmission in _stop_cam (1.91 KB, patch)
2016-06-17 12:46 UTC, Joan Pau
committed Details | Review

Description Joan Pau 2016-03-03 09:07:06 UTC
dc1394src needs to be ported to gstreamer version 1.x.
WIP on the required patch, hopefully ready soon.
Comment 1 Tim-Philipp Müller 2016-03-03 09:25:01 UTC
Did you forget to attach the patch by any chance? :)
Comment 2 Joan Pau 2016-05-11 16:06:28 UTC
Created attachment 327650 [details] [review]
Initial patch, working well, but with some points to discuss

This is a working proposal, but includes several points which need discussion,
and, depending on the conclusions, minor changes.
The points to discuss are marked with TODO comments in the code,
and also explained in the patch description, which I copy below for reference.
I can provide another patch with the required modifications
after the discussion, and those TODO comments hopefully removed.
This is the description of the patch:

The dc1394src is a PushSrc element for IIDC cameras based on libdc1394.
The implementation from the 0.x series is deffective:
caps negotiation does not work, and some video formats
provided by the camera are not supported.

Refactor the code to port it to 1.X and enhance the support
for the full set of video options of IIDC cameras:

  - The IIDC specification includes a set of camera video modes
    (video format, frame size, and frame rates).
    They do not map perfectly to Gstreamer formats, but those that
    do not match are very rare (if used at all by any camera).
    In addition, although the specification includes a raw format,
    some cameras use mono video formats to capture in Bayer format.
    Map corresponding video modes to Gstreamer formats in capabilities,
    allowing both gray raw and Bayer video formats for mono video modes.

    Version 0.X maps DC1394_COLOR_CODING_YUV444 to IYU2 fourcc format,
    which is not defined in 1.X. Map it to Y444 format instead.

  - The specification includes scalable video modes (Format7),
    where the frame size and rate can be set to arbitrary values
    (within the limits of the camera and the bus transport).
    Allow the use of such mode, using the frame size and rate
    from the negotiatied caps, and set the camera frame rate
    adjusting the packet size as in:
    <http://damien.douxchamps.net/ieee1394/libdc1394/faq/#How_do_I_set_the_frame_rate>

    The scalable modes also allow for a custom ROI offset.
    Support for it can be easily added later using properties.

  - Camera operation using libdc1394 is as follows:

      1. Enumerate cameras on the system and open the camera
         identified the enumeration index or by a GUID (64bit hex code).

      2. Query the video formats supported by the camera.

      3. Configure the camera for the desired video format.

      4. Setup the capture resources for the configured video format
         and start the camera transmission.

      5. Capture frames from the camera and release them when not used.

      6. Stop the camera transmission and clear the capture resources.

      7. Close the camera freeing its resources.

    Steps 2 and 3 are done when getting and setting the caps respectively.
    Ideally 4 and 6 would be done when going from PAUSED to PLAYING
    and viceversa, but since caps might not be set yet, the video mode
    is not properly configured leaving the camera in a broken state.
    Setup capture and start transmission in the set caps method,
    and consequently clear the capture and stop the transmission
    when going from PAUSED to READY (instead of PLAYING to PAUSED).

    If caps can be (re)set while in PLAYING, the capture should
    be cleared and the transmission stopped before configuring
    the new video mode and enabling the capture again.

  - Create buffers copying the bytes of the captured frames.
    Alternatively, the buffers could just wrap the bytes of the frames,
    releasing the frame in the buffer's destroy notify function,
    if all buffers were destroyed before going from PLAYING to PAUSED.

  - No timestamp nor offset is set when creating buffers.
    Timestamping is delegated to the parent class BaseSrc,
    setting `gst_base_src_set_live` TRUE, `gst_base_src_set_format`
    with GST_FORMAT_TIME and `gst_base_src_set_do_timestamp`.
    Captured frames have a timestamp field with the system time
    at the completion of the transmission of the frame,
    but it is not sure that this comes from a monotonic clock,
    and it seems to be left NULL in Windows.

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

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

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

  - Adjust build files to reenable the compilation of the plugin.

    Remove dc1394 from the list of unported plugins in configure.ac.

    Add the missing flags and libraries to Makefile.
    Use `$()` for variable substitution, as many plugins do,
    although other plugins use `@@` instead.
Comment 3 Tim-Philipp Müller 2016-05-19 15:03:04 UTC
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.


>  - 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.
Comment 4 Joan Pau 2016-05-27 14:33:41 UTC
(In reply to Tim-Philipp Müller from comment #3)
> Comment on attachment 327650 [details] [review] [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?
Comment 5 Tim-Philipp Müller 2016-05-27 15:40:06 UTC
> 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)?

Not really. There isn't really a good way to communicate a set_property() failure to the caller other than to just not change the property (they can check with get_property() if it worked if they want to). This would be a programmer error however, and it would be unusual to post an error message for that.


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

Yes :)


> What type of property do you think is more suitable: string or 64 bit
> integral?

I would go for a GUID string. Presumably this is also something one might find somewhere in some system tool that shows connected devices, no?



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

I think so. I don't see what else you can do really, and it's a live source. It sounds fine. We can still improve it later if we can think of a better way.


 
> > Maybe the iso-speed property should be an enum property instead?
> > 
> Would something like the following be ok?

Looks good to me, yes. I trust this means you agree it makes sense..


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

As far as I can tell IYU2 is a packed 4:4:4 format, but with component ordering like U Y V U Y V whereas Y444 is for planar 4:4:4, so the closest would be the v308 format. I think we just have to add a new video format for IYU2. Shouldn't be too difficult, there's IYU1 already :)


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

Please amend your original patch. Then once it's merged we can do follow-up patches separately. Thanks!
Comment 6 Joan Pau 2016-05-30 14:49:00 UTC
Created attachment 328731 [details] [review]
Add support for the IYU2 format.

Regarding the format IYU2, this patch implements it.
I am not very familiar with the internals of video formats,
so I am monkey patching here, but I think is correct.
I am not sure if I should include these changes in the dc1394 patch
or create a new bug and attach the patch there.
Comment 7 Joan Pau 2016-05-30 15:43:08 UTC
(In reply to Joan Pau from comment #6)
> Created attachment 328731 [details] [review] [review]
> Add support for the IYU2 format.
> 
> Regarding the format IYU2, this patch implements it.
> I am not very familiar with the internals of video formats,
> so I am monkey patching here, but I think is correct.
> I am not sure if I should include these changes in the dc1394 patch
> or create a new bug and attach the patch there.

I mean donkey patching, review highly needed!
Comment 8 Tim-Philipp Müller 2016-06-01 11:09:40 UTC
Comment on attachment 328731 [details] [review]
Add support for the IYU2 format.

Thanks, pushed this with some minor changes/additions (most importantly, the new enum value needs to be added at the end in order to maintain ABI backwards compatibility):

commit abb88801e79bde97523c0ad4dba30a463b5d922d
Author: Joan Pau Beltran <joanpau.beltran@socib.cat>
Date:   Mon May 30 16:40:26 2016 +0200

    video: add IYU2 format
    
    This existed in 0.10 and is needed by dc1394src.
    
    IYU2 format is a YUV fully-sampled packed format similar to v308
    but with different component order (U-Y-V instead of Y-U-V).
    
    http://www.fourcc.org/yuv.php#IYU2
    
    https://bugzilla.gnome.org/show_bug.cgi?id=763026#c5
Comment 9 Joan Pau 2016-06-01 14:00:17 UTC
Created attachment 328890 [details] [review]
docs/design: add IYU2 raw video format description

For completeness, this patch updates the list of raw video types
in design documents with the description of the IYU2 video format just added.
Comment 10 Joan Pau 2016-06-01 15:26:13 UTC
Created attachment 328900 [details] [review]
dc1394src: port to 1.X (with so far proposed modifications)

This is a modified version of the initial patch
with the modifications discussed so far, namely:

  - Replace the camera number integer property to indentify the camera
    by a pair of GUID and unit properties (string and integer).
    Operate on the first camera available if GUID is null (default).
    Match any camera unit number if unit is -1.

  - Redefine the ISO speed property as enum type (instead of integer).

  - Map DC1394_COLOR_CODING_YUV444 to GST_VIDEO_FORMAT_IYU2 introduced
    in commit: abb88801e79bde97523c0ad4dba30a463b5d922d.

  - Replace `GstElement` state change function by start` and stop functions
    from `GstBaseSrc`.

  - Stop the camera in set caps method before setting new caps and
    restarting again (it has no effect if the camera is not started).

Hence, the only 3 points left to discuss from the initial proposal are:

  - The buffer creation method (copy instead of wrapping)

  - The buffer timestamping (delegated to the GstBaseSrc parent class I think)

  - The replacement of prefix `gst_dc1394` by `gst_dc1394_src`
    (it only affects the code,the name of the element is already `dc1394src`)
Comment 11 Tim-Philipp Müller 2016-06-01 16:50:36 UTC
Thanks, I'm not sure that's really reviewable like that tbh, so I think if you say it works fine for you we should just push it and take it from there ;)

- buffer copy is fine for now

- letting basesrc do timestamping should be fine for starters as well

- prefix: doesn't matter, can do later or now.

One thing though: does the example launch pipeline in the docs need updating for the new properties?
Comment 12 Tim-Philipp Müller 2016-06-07 13:01:03 UTC
What's up with this now? Is this okay to go in as an initial port or were there more you wanted to fix?
Comment 13 Joan Pau 2016-06-07 14:40:14 UTC
Created attachment 329283 [details] [review]
dc1394src: port to 1.X (with so far proposed modifications and better documentation)

(In reply to Tim-Philipp Müller from comment #11)
No, this patch updates that comment with more information
and proper example pipelines.
Comment 14 Joan Pau 2016-06-07 14:50:14 UTC
Created attachment 329284 [details] [review]
dc1394src: internal type, prefix and file renaming to adhere to conventions

(In reply to Tim-Philipp Müller from comment #12)
> What's up with this now? Is this okay to go in as an initial port or were
> there more you wanted to fix?

I attach a last patch including the type name, function prefix,
and file name replacement to adhere to the conventions.
I send it as a separate patch so that it can be applied in different commits.

Note that this does not rename the element (it already was dc1394src),
changes just affect the internal code and and the name of the files
(I do not know if replacing the GST_TYPE can have any side effect though).
Comment 15 Tim-Philipp Müller 2016-06-08 17:55:02 UTC
Great, thanks. There are no side-effects from renaming and such.

Could I ask you to fix up some minor coding style issues in the first commit?

1) install GNU indent and run gst-indent (https://cgit.freedesktop.org/gstreamer/gstreamer/tree/tools/gst-indent) on the .c file (just the .c file not the header file). If that's a problem I can re-indent it for you.

In _build_caps() you might want to split the variable declarations like this to avoid the indenter doing weird-looking things:

  GValue format = { 0 };
  GValue formats = { 0 };
  GValue width = { 0 };
  GValue widths = { 0 };
  GValue height = { 0 };
  GValue heights = { 0 };
  GValue framerate = { 0 };
  GValue framerates = { 0 };

2) convert C++ style comments (// hello) to C-style comments (/* hello */)

3) don't use C99 style struct initialisers like = { .num = 7, .foo = { xyz }}

Thanks for your patience ! :)
Comment 16 Joan Pau 2016-06-09 13:44:23 UTC
Created attachment 329465 [details] [review]
dc1394src: port to 1.X

(In reply to Tim-Philipp Müller from comment #15)
This patch fixes the style problems and passes the pre-commit hook.
Comment 17 Joan Pau 2016-06-09 13:46:44 UTC
Created attachment 329466 [details] [review]
dc1394src: internal type, prefix and file renaming to adhere to conventions

And this is the patch corresponding to the subsequent commit performing the renaming.
Comment 18 Tim-Philipp Müller 2016-06-09 22:03:40 UTC
Superb, thanks! Let's get this in. Please file any follow-up patches in new bugs, thanks!

commit faf6e5a1eb757815d3e6ff49765e79b9895d4e48
Author: Tim-Philipp Müller <tim@centricular.com>
Date:   Thu Jun 9 22:01:45 2016 +0100

    dc1394src: minor clean-up
    
    We always call _parse_caps() with non-NULL out vars.

commit 09737d1874467558f51313fdf1d089edeccccdf8
Author: Tim-Philipp Müller <tim@centricular.com>
Date:   Thu Jun 9 22:01:13 2016 +0100

    dc1394src: fix some more c99-isms

commit c8b7585dce782518d0b6f96a1cef4f7a85939820
Author: Tim-Philipp Müller <tim@centricular.com>
Date:   Thu Jun 9 21:47:05 2016 +0100

    docs: fix for renamed dc1394 source file
    
    https://bugzilla.gnome.org/show_bug.cgi?id=763026

commit 3355f5b3ab2357844e1bd6baded88c7a286184cd
Author: Joan Pau Beltran <joanpau.beltran@socib.cat>
Date:   Tue Jun 7 15:50:50 2016 +0200

    dc1394src: prefix and file names according to Gstreamer conventions
    
    Replace the type and function prefix to follow the conventions:
    
      - Use `GST_TYPE_DC1394_SRC` instead of `GST_TYPE_DC1394`.
    
      - Use `GstDC1394Src` and `GstDC1394SrcClass` instead of
        `GstDc1394` and `GstDc1394Class`.
    
      - Use `gst_dc1394_src` instead of `gst_dc1394`.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=763026

commit e28b1236084a134007ae69f4dc94f2d98a2d16e1
Author: Joan Pau Beltran <joanpau.beltran@socib.cat>
Date:   Tue May 10 18:30:35 2016 +0200

    dc1394src: port to 1.X
    
    The dc1394src is a PushSrc element for IIDC cameras based on libdc1394.
    The implementation from the 0.x series is deffective:
    caps negotiation does not work, and some video formats
    provided by the camera are not supported.
    
    Refactor the code to port it to 1.X and enhance the support
    for the full set of video options of IIDC cameras:
    
      - The IIDC specification includes a set of camera video modes
        (video format, frame size, and frame rates).
        They do not map perfectly to Gstreamer formats, but those that
        do not match are very rare (if used at all by any camera).
        In addition, although the specification includes a raw format,
        some cameras use mono video formats to capture in Bayer format.
        Map corresponding video modes to Gstreamer formats in capabilities,
        allowing both gray raw and Bayer video formats for mono video modes.
    
      - The specification includes scalable video modes (Format7),
        where the frame size and rate can be set to arbitrary values
        (within the limits of the camera and the bus transport).
        Allow the use of such mode, using the frame size and rate
        from the negotiatied caps, and set the camera frame rate
        adjusting the packet size as in:
        <http://damien.douxchamps.net/ieee1394/libdc1394/faq/#How_do_I_set_the_frame_rate>
    
        The scalable modes also allow for a custom ROI offset.
        Support for it can be easily added later using properties.
    
      - Camera operation using libdc1394 is as follows:
    
          1. Enumerate cameras on the system and open the camera
             identified the enumeration index or by a GUID (64bit hex code).
    
          2. Query the video formats supported by the camera.
    
          3. Configure the camera for the desired video format.
    
          4. Setup the capture resources for the configured video format
             and start the camera transmission.
    
          5. Capture frames from the camera and release them when not used.
    
          6. Stop the camera transmission and clear the capture resources.
    
          7. Close the camera freeing its resources.
    
        Do steps 2 and 3 when getting and setting the caps respectively.
        Ideally 4 and 6 would be done when going from PAUSED to PLAYING
        and viceversa, but since caps might not be set yet, the video mode
        is not properly configured leaving the camera in a broken state.
        Hence, setup capture and start transmission in the set caps method,
        and consequently clear the capture and stop the transmission
        when going from PAUSED to READY (instead of PLAYING to PAUSED).
        Symmetrycally, open the camera when going from READY to PAUSED,
        allowing to probe the camera caps in the negotiation stage.
        Implement that using the `start` and `stop` methods of `GstBaseSrc`,
        instead of the `change-state` method of `GstElement`.
        Stop the camera before setting new caps and restarting it again
        to handle caps reconfiguration while in PLAYING (it has no effect
        if the camera is not started).
    
      - Create buffers copying the bytes of the captured frames.
        Alternatively, the buffers could just wrap the bytes of the frames,
        releasing the frame in the buffer's destroy notify function,
        if all buffers were destroyed before going from PLAYING to PAUSED.
    
      - No timestamp nor offset is set when creating buffers.
        Timestamping is delegated to the parent class BaseSrc,
        setting `gst_base_src_set_live` TRUE, `gst_base_src_set_format`
        with GST_FORMAT_TIME and `gst_base_src_set_do_timestamp`.
        Captured frames have a timestamp field with the system time
        at the completion of the transmission of the frame,
        but it is not sure that this comes from a monotonic clock,
        and it seems to be left NULL in Windows.
    
      - Use GUID and unit properties to select the camera to operate on.
        The camera number used in version 0.X does not uniquely identify
        the device (it depends on the set of cameras currently detected).
        Since the GUID is 64bit identifier (same as MAC address),
        handle it with a string property with its hexadecimal representation.
        For practicality, operate on the first camera available if the GUID
        is null (default) and match any camera unit number if unit is -1.
        Alternatively, the GUID could be handed with an unsigned 64 bit
        integer type property, using `0xffffffffffffffff` as default value
        to select the first camera available (it is not a valid GUID value).
    
      - Keep name `GstDc1394` and prefix `gst_dc1394` as in version 0.X,
        although `GstDC1394Src` and `gst_dc1394_src` are more descriptive.
    
      - Adjust build files to reenable the compilation of the plugin.
    
        Remove dc1394 from the list of unported plugins in configure.ac.
    
        Add the missing flags and libraries to Makefile.
        Use `$()` for variable substitution, as many plugins do,
        although other plugins use `@@` instead.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=763026
Comment 19 Joan Pau 2016-06-17 12:46:46 UTC
Created attachment 329952 [details] [review]
dc1394src: check for disabled transmission in _stop_cam

I missed this small bit in my previous commits.
It should not make any difference though,
except for some not-well-behaving cameras.
Comment 20 Tim-Philipp Müller 2016-06-20 20:47:31 UTC
Comment on attachment 329952 [details] [review]
dc1394src: check for disabled transmission in _stop_cam

Not exactly very nice, but here goes:

commit dc762166f39799bf28f8293c161546f64006efb2
Author: Joan Pau Beltran <joanpau.beltran@socib.cat>
Date:   Fri Jun 17 14:31:42 2016 +0200

    dc1394src: check for disabled transmission in _stop_cam
    
    For symetry with _start_cam, check that the transmission
    is effectively disabled in _stop_cam.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=763026