GNOME Bugzilla – Bug 768110
new plugin: ahssrc (Android hardware sensor source)
Last modified: 2016-10-31 14:28:07 UTC
Created attachment 330460 [details] [review] new plugin: Android hardware sensor source ahssrc is a new plugin that enables Gstreamer to read from the android.hardware.sensor Android sensors. These sensors are treated as buffers and can be passed through and manipulated by the pipeline. To my knowledge, this is the first GStreamer plugin that handles analog sensor data. Treating sensors as buffers in the pipeline is useful because sensor data fits naturally as a data stream, and it can be very high rate, requiring high performance (e.g. accelerometers can generate over 1000Hz). If sensors are just another data type, then they can be multiplexed alongside audio and video, allowing per-frame correlation. For example, you could have a video frame with corresponding accelerometer, gyroscope, barometer, and light level measurements, enabling very precise correlation of multiple data types together. In addition, sensors as streams can be throttled, buffered, and aggregated, just as you might with video or audio. I would like some feedback regarding a few aspects of the implementation: - My current buffer format is just binary, mapping to some simple structs that correspond to the equivalent types in the android.hardware.Sensor API. I intend the structs to be in an exported header file. Although this works well for Android sensor data, it's possible that this won't map well to non-Android sensors, if other platforms expose a slightly different schema for their data. However, without knowing the universe of possibilities, I figured that starting with the Android API was reasonable. What do you think? - Currently, the file gstahscaps.h is not exported anywhere, but it should be exported so that other plugins can handle the sensor data coming from ahssrc. Ideally, this should be exported somewhere that is not Android-specific (e.g. not in androidmedia), as sensor data should be able to be generated and handled by non-Android platforms too. What is the best way to accomplish this? Thanks!
Review of attachment 330460 [details] [review]: 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.
The timestamp in the event seems to match System.nanoTime(), which I think always matched clock_gettime(CLOCK_MONOTONIC) which is what you get from the system clock with GST_CLOCK_TYPE_MONOTONIC, so you should be able to create one of those and slave it to the pipeline clock.
Review of attachment 330460 [details] [review]: This seems very useful indeed :) ::: sys/androidmedia/gstahssrc.c @@ +719,3 @@ + buffer = gst_buffer_new_wrapped (data, self->buffer_size); + GST_BUFFER_DURATION (buffer) = duration; + GST_BUFFER_PTS (buffer) = buffer_ts; As Olivier said, you really want to timestamp with the pipeline clock here. Just use GST_ELEMENT_CLOCK() instead of using your own system clock. Alternatively if there is indeed a timestamp provided by the Android APIs (is there?) you need to know which clock it comes from (probably the monotonic system clock). You can then either really slave that to the pipeline clock to transform timestamps (timestamps must always be according to the pipeline clock), or go the easy way and assume there is not much drift between the two: capture_time = whatever android gives you current_time = gst_clock_get_time(clock android uses) pipeline_time = gst_clock_get_time(element clock) buffer_time = pipeline_time - (current_time - capture_time) // check for wraparound
Thanks for all the comments! It seems like the main aspect of this to figure out is the clocking and the messages vs buffers. Once we agree on the right solution forward there, I will release an updated patch addressing all the comments. The reason why I created the optional "emit-events" (I'll change it to "emit-messages") is that, currently, there is no sink for handling sensor data. Thus sending messages out on the event bus is what I'm currently using for getting the sensor data and for debugging it. Once a sensor sink exists, this property should probably be dropped, as the sink could do a better job. Because messages are not synchronized nicely with buffers, I gave them system time rather than pipeline time, as an app processing the events doesn't know how to handle pipeline time. I could instead just use pipeline time for the messages, but then the app would have to somehow translate to system time. My intention was to keep the buffers using the pipeline clock, so please let me know if you noticed a bug and that's not what's happening. Thus the current division is: - messages use system time so that apps can understand them out-of-band with the pipeline - buffers use pipeline (GST_ELEMENT_CLOCK) time so they're synchronized with the pipeline Now, it's true that Android supplies a timestamp in the SensorEvent object as part of the sensor data callback. Last time I checked, this was monotically increasing time since Android system boot. Right now, I'm ignoring that timestamp and just using the element clock when I receive the callback to compute the buffer duration. However, you're right that it would be better to use the timestamps given in the events and slave them to the element clock. I will make that change. It seems there's a few main issues to figure out: - Should we keep the "emit-messages/emit-events" property? If we don't keep it, there's no easy way to see these events until we add a sensor sink. If we do keep it, then we have an inelegant out-of-band channel for events. - If we keep the "emit-messages" property, should it use system time, or should it use pipeline time and let the app convert to system time? - Where should the headers for the sensor caps be exported, and what's the correct way to handle that in Makefile.am? Once we resolve these issues, I'll issue an updated patch. Thanks again for the helpful feedback!
(ping) Do you have opinions on the issues I highlighted above?
- You could use appsink instead of emitting messages, no? Although I'm not much against the emit-messages stuff. - I don't have much opinion on which timestamps to use.. Maybe have something based on a specific gstreamer clock (it could just be what gst_system_clock_obtain() returns?) - I'm not sure what's the best way to export the header, maybe in gst-libs/gst/interfaces .. We don't really have a good place for platform specific headers like that.
Thanks Olivier. Now that I think of it, I agree that appsink is a better way to do this. In addition, if the appsink wants system time, it can handle translating pipeline time to system time without forcing ahssrc to do that directly. Who do you think would know where to export the header? Should we get slomo's opinion?
I think at some point we should have platform specific integration libraries, like a libgstandroid in this case. There are other things that could make sense in there, e.g. the Dalvik/ART integration.
(In reply to Sebastian Dröge (slomo) from comment #8) > I think at some point we should have platform specific integration > libraries, like a libgstandroid in this case. There are other things that > could make sense in there, e.g. the Dalvik/ART integration. If we do that, we should move the init stuff we do in Java in C, so we can easily support QT for Android and other native stacks.
Although platform specific integration libraries seem like a good idea, I actually hope for the sensor caps that I created to be usable by non-Android platforms (for example, a more standard embedded Linux device). In addition, the caps header needs to be accessible by apps that use ahssrc. What is the best location to install such a header? Should it go in gst-libs inside gst-plugins-bad? For example, I could put it in gst-libs/gst/sensor/gstsensorcaps.h, or similar.
(In reply to Nicolas Dufresne (stormer) from comment #9) > If we do that, we should move the init stuff we do in Java in C, so we can > easily support QT for Android and other native stacks. Put that in another bug please :) But I don't see how you would do that more than what we already do, it's already callable from C. (In reply to Martin Kelly from comment #10) > Although platform specific integration libraries seem like a good idea, I > actually hope for the sensor caps that I created to be usable by non-Android > platforms (for example, a more standard embedded Linux device). In addition, > the caps header needs to be accessible by apps that use ahssrc. > > What is the best location to install such a header? Should it go in gst-libs > inside gst-plugins-bad? For example, I could put it in > gst-libs/gst/sensor/gstsensorcaps.h, or similar. That would also work, but it's not only caps, right? It should probably come with something that parses the data into proper C structs
Alright, attached is a new version of the plugin addressing all the feedback so far. There's only one outstanding issue that I know, which is that I'm not sure where to install the file listing caps and and data structs, as well as how to write the correct Makefile rules to make that happen. I have removed the emit-events property and removed system time entirely, so now the plugin is passing only buffers, using pipeline time. I experimented with using the timestamp in the actual Android SensorEvent. However, it appears that those timestamps are not reliable, so I'm just using pipeline time when we receive the callback. If you're curious, see here for more details on Android timestamps: https://code.google.com/p/android/issues/detail?id=7981 In addition, I added a UNION_CAST macro to avoid a type-punning issue. This macro is from the following website: http://www.cocoawithlove.com/2008/04/using-pointers-to-recast-in-c-is-bad.html The website allows use, as long as you provide attribution (http://www.cocoawithlove.com/about/), so I added attribution at the top of the file. However, someone may want to check that I did this correctly before the patch is accepted. (In reply to Sebastian Dröge (slomo) from comment #11) > (In reply to Martin Kelly from comment #10) > > Although platform specific integration libraries seem like a good idea, I > > actually hope for the sensor caps that I created to be usable by non-Android > > platforms (for example, a more standard embedded Linux device). In addition, > > the caps header needs to be accessible by apps that use ahssrc. > > > > What is the best location to install such a header? Should it go in gst-libs > > inside gst-plugins-bad? For example, I could put it in > > gst-libs/gst/sensor/gstsensorcaps.h, or similar. > > That would also work, but it's not only caps, right? It should probably come > with something that parses the data into proper C structs Yes, agreed. Right now, I have just merged the struct and caps definitions into one file, gstsensors.h. I'm not sure where this file should be installed and how to setup the Makefile rules to install it. I welcome advice on how best to do this. Thanks again!
Created attachment 331379 [details] [review] [PATCH v2] new plugin: Android hardware sensor source
Review of attachment 331379 [details] [review]: ::: sys/androidmedia/gstahssrc.c @@ +613,3 @@ + /* If the clock is NULL, the pipeline is not yet set to PLAYING. */ + if (pipeline_clock == NULL) + goto done; You should also provide the system clock as a backup by implementing the (see how rtpjitterbuffer does it). If you have ahssrc ! appsink, there will be no element to provide one. @@ +629,3 @@ + gst_object_ref (pipeline_clock); + buffer_time = gst_clock_get_time (pipeline_clock); + gst_object_unref (pipeline_clock); No need to ref & unref the clock, you're still holding the element's object lock. @@ +636,3 @@ + else + buffer_duration = GST_CLOCK_TIME_NONE; + self->previous_time = buffer_time; I would just skip the duration entirely and always set it to NONE. Those events don't really have a duration.
(In reply to Olivier Crête from comment #15) > Review of attachment 331379 [details] [review] [review]: > > ::: sys/androidmedia/gstahssrc.c > @@ +613,3 @@ > + /* If the clock is NULL, the pipeline is not yet set to PLAYING. */ > + if (pipeline_clock == NULL) > + goto done; > > You should also provide the system clock as a backup by implementing the > (see how rtpjitterbuffer does it). If you have ahssrc ! appsink, there will > be no element to provide one. Could you explain this part more? I see what rtpjitterbuffer is doing but I don't fully understand why. In my current test app, I'm using a bunch of ahssrc elements going into a funnel going into an appsink. I'm seeing GST_ELEMENT_CLOCK return a valid clock that looks to be monotonic with some base. In addition, if my element provides a clock, why not choose a monotonic one rather than the system clock? It seems like that would be more reliable, but I'm probably missing something here, as I don't fully understand how all the clocks work in gstreamer. > @@ +629,3 @@ > + gst_object_ref (pipeline_clock); > + buffer_time = gst_clock_get_time (pipeline_clock); > + gst_object_unref (pipeline_clock); > > No need to ref & unref the clock, you're still holding the element's object > lock. Agreed; I'll fix that. > > @@ +636,3 @@ > + else > + buffer_duration = GST_CLOCK_TIME_NONE; > + self->previous_time = buffer_time; > > I would just skip the duration entirely and always set it to NONE. Those > events don't really have a duration. Agreed; I'll fix that.
(In reply to Martin Kelly from comment #16) > (In reply to Olivier Crête from comment #15) > Could you explain this part more? I see what rtpjitterbuffer is doing but I > don't fully understand why. In my current test app, I'm using a bunch of > ahssrc elements going into a funnel going into an appsink. I'm seeing > GST_ELEMENT_CLOCK return a valid clock that looks to be monotonic with some > base. > > In addition, if my element provides a clock, why not choose a monotonic one > rather than the system clock? It seems like that would be more reliable, but > I'm probably missing something here, as I don't fully understand how all the > clocks work in gstreamer. Actually forget that, it seems that the pipeline default to the system clock, which should be fine as the default GStreamer system clock is the Linux monotonic clock.
OK. In that case, I'll issue a revised patch with your two most recent bits of feedback (duration and clock ref/unref). There is one remaining issue, which is that I don't know where and how to install the gstsensors.h file so that apps and other plugins outside of androidmedia can access it. Could someone help advise on this part?
Created attachment 331449 [details] [review] [PATCH v3] new plugin: Android hardware sensor source
Attached is a patch v3 that addresses Olivier's most recent feedback, so all that is left is where to install and how to install gstsensors.h. I will need some guidance with that.
Merged, I don't have a great location where to put the header file, so let's leave it there for now. commit a04e6b0cb2863b960c54ad5decc6d3a9a3a2bd42 Author: Martin Kelly <martin@surround.io> Date: Tue Jul 12 14:51:47 2016 -0700 new plugin: Android hardware sensor source ahssrc is a new plugin that enables Gstreamer to read from the android.hardware.Sensor Android sensors. These sensors are treated as buffers and can be passed through and manipulated by the pipeline. https://bugzilla.gnome.org/show_bug.cgi?id=768110
Cool, thanks! Sounds like it may be best to bring up the header question on gstreamer-devel.