Created attachment 200026 [details] [review]
dv1394src: Add support for older kernels

raw1394_read_cycle_timer is not working with all kernel versions.
When using the firewire module, the kernel needs to have defined
FW_CDEV_IOC_GET_CYCLE_TIMER (added in kernel 2.6.24) and when using
the ieee1394 module, the kernel version must be >= 2.6.21

Created attachment 200033 [details] [review]
dv1394src: Add support for older kernels

raw1394_read_cycle_timer is not working with all kernel versions.
When using the firewire module, the kernel needs to have defined
FW_CDEV_IOC_GET_CYCLE_TIMER (added in kernel 2.6.24) and when using
the ieee1394 module, the kernel version must be >= 2.6.21

Created attachment 200034 [details] [review]
dv1394src: check return value of raw1394_ready_cycle_timer

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

Review of attachment 200033 [details] [review]:

The patch does not add tests for the conditions your commit message mentions (2.6.24, 2.6.21),
should those be added to configure (or in the source) ?

::: ext/raw1394/gst1394clock.c
@@ +129,3 @@
+            CSR_REGISTER_BASE | CSR_CYCLE_TIME,
+            sizeof (guint32), cycle_timer) == 0) {
+      *cycle_timer = GUINT32_SWAP_LE_BE (*cycle_timer);

That looks a bit suspicious. The hardware is returning something in a fixed endianness presumably, this will yield a value in a fixed endianness as well, should it not be swapped only on BE or LE ?

@@ +186,3 @@
   clock->cycle_timer_hi = 0;
+
+  if (raw1394_read_cycle_timer (clock->handle, &cycle_timer, &local_time) != 0) {

That function is declared as returning a boolean, not an int.

Review of attachment 200034 [details] [review]:

Typo in Subject: raw1394_ready_cycle_timer
                             ^

::: ext/raw1394/gst1394clock.c
@@ +120,3 @@
+    if (err) {
+      GST_ERROR_OBJECT (clock, "Error reading cycle timer");
+      return GST_CLOCK_TIME_NONE;

The other use of _read_cycle_timer unsets have_rct on error, should this one do as well ?

Review of attachment 200033 [details] [review]:

::: ext/raw1394/gst1394clock.c
@@ +129,3 @@
+            CSR_REGISTER_BASE | CSR_CYCLE_TIME,
+            sizeof (guint32), cycle_timer) == 0) {
+    return TRUE;

The real operation that needs to be done here is bswap_32(), which is not portable.
From the documentation:
The bswap16(), bswap32(), and bswap64() functions return a byte order
swapped integer. On big endian systems, the number is converted to little
endian byte order. On little endian systems, the number is converted
to big endian byte order.

I though that GUINT_SWAP_LE_BE was meant for the same thing.

@@ +186,3 @@
   clock->cycle_timer_hi = 0;
+
+  if (raw1394_read_cycle_timer (clock->handle, &cycle_timer, &local_time) != 0) {

I might be wrong but from the docs, this functions returns an int: http://www.dennedy.org/libraw1394/API-raw1394-read-cycle-timer.html

Review of attachment 200033 [details] [review]:

::: ext/raw1394/gst1394clock.c
@@ +129,3 @@
+            CSR_REGISTER_BASE | CSR_CYCLE_TIME,
+            sizeof (guint32), cycle_timer) == 0) {
+    return TRUE;

The real operation that needs to be done here is bswap_32(), which is not portable.
From the documentation:
The bswap16(), bswap32(), and bswap64() functions return a byte order
swapped integer. On big endian systems, the number is converted to little
endian byte order. On little endian systems, the number is converted
to big endian byte order.

I though that GUINT_SWAP_LE_BE was meant for the same thing.

@@ +186,3 @@
   clock->cycle_timer_hi = 0;
+
+  if (raw1394_read_cycle_timer (clock->handle, &cycle_timer, &local_time) != 0) {

I might be wrong but from the docs, this functions returns an int: http://www.dennedy.org/libraw1394/API-raw1394-read-cycle-timer.html

Review of attachment 200034 [details] [review]:

::: ext/raw1394/gst1394clock.c
@@ +120,3 @@
+    if (err) {
+      GST_ERROR_OBJECT (clock, "Error reading cycle timer");
+      return GST_CLOCK_TIME_NONE;

_have_rct defines whether the kernel support this call, which is checked when setting the clock handle.

I see 3 scenarios here:
  * This call is not supported, returning always an error value,  and _have_rct is properly set to FALSE setting the clock handle.
  * This call is supported and _have_rct is properly set to TRUE in the first call, but we can still fail reading the timer from time to time, which should never happen and that's why _have_crt is not reset here
  * This call is supported and _have_crt is badly set to FALSE because the first call to the function returned with an error, which also should never happen

> The real operation that needs to be done here is bswap_32(), which is not
> portable.

It might be alright, I don't know what the hardware returns in detail, it just seems odd and thus worth pointing out in case it was an oversight.

> @@ +186,3 @@
>    clock->cycle_timer_hi = 0;
> +
> +  if (raw1394_read_cycle_timer (clock->handle, &cycle_timer, &local_time) !=
> 0) {
> 
> I might be wrong but from the docs, this functions returns an int:
> http://www.dennedy.org/libraw1394/API-raw1394-read-cycle-timer.html

Right, I mixed up raw1394_read_cycle_timer and gst_raw1394_read_cycle_timer, named very similarly but returning different types.

Any news on this?

Ping?

Is anyone really ever going to use 1.0 with a kernel older than 2.6.24 (released 24 January 2008) ?

I'm going to say no. If you're using firewire and machines running kernels from over 5 years ago you have bigger issues.

Closing as obsolete.