[Bug 768110] new plugin: ahssrc (Android hardware sensor source)

[GStreamer (GNOME Bugzilla)]

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

Olivier Crête <olivier.crete at ocrete.ca> changed:

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

--- Comment #1 from Olivier Crête <olivier.crete at ocrete.ca> ---
Review of attachment 330460:
 --> (https://bugzilla.gnome.org/review?bug=768110&attachment=330460)

Using GStreamer for non-AV data is always quite cool. I'm really excited by
this, below are some comments on some details of the code:

::: sys/androidmedia/gstahscaps.h
@@ +47,3 @@
+
+#define GST_SENSOR_CAPS_MAKE(format)                             \
+    "sensor, " \

Caps should have a format like application/android-sensor or something like
that.

::: sys/androidmedia/gstahssrc.c
@@ +208,3 @@
+      G_PARAM_READWRITE | G_PARAM_CONSTRUCT | G_PARAM_STATIC_STRINGS);
+  g_object_class_install_property (gobject_class, PROP_EMIT_EVENTS,
+      properties[PROP_EMIT_EVENTS]);

This should be "emit-messages", not events. I'm also not sure what its for?

@@ +240,3 @@
+gst_ahs_src_init (GstAHSSrc * self)
+{
+  g_mutex_init (&self->mutex);

Why add another mutex instead of using the GST_OBJECT_LOCK() ?

@@ +256,3 @@
+
+  self->system_clock = g_object_new (GST_TYPE_SYSTEM_CLOCK,
+      "name", "GstSystemClock", "clock-type", GST_CLOCK_TYPE_REALTIME, NULL);

Why use the realtime clock which can be affected by timezone changes, etc
instead of the monotonic clock? Actually, why not just use the pipeline clock?

Also, why not use the timestamp in the "SensorEvent" object and somehow convert
it into the pipeline's clock.

@@ +670,3 @@
+  clock = GST_ELEMENT_CLOCK (self);
+  g_assert_nonnull (clock);
+  gst_object_ref (clock);

This is racy.. You want to take the GST_OBJECT_LOCK() while getting and reffing
the clock and the base time.

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