[vlc-devel] WG: [PATCH] Added a Linux headtracker
Filip Roséen
filip at atch.se
Tue Feb 21 20:37:35 CET 2017
Hi Christian,
Even though the formatting of your `patch` is not sufficient enough
for potential merge, I thought I do a quick review of your
implementation (so that you can submit a revised implementation with
`git send-email` at a later time).
Keep up the good work!
Best Regards,\
Filip
On 2017-02-21 14:42, Christian Hoene wrote:
> Hi,
>
> I added a headtracker interface to VLC, which works with Linux.
> It can be used for 3d audio or 360° videos.
>
> Any comments to this first patch?
Did you consider/try using *evdev* instead of `linux/joystick.h`, and
if so; is there a notable difference in precision and user-experience?
- https://en.wikipedia.org/wiki/Evdev
>
> With best regards,
>
> Christian Hoene
>
>
> -----Ursprüngliche Nachricht-----
> Von: Christian Hoene [mailto:christian.hoene at symonics.com]
> Gesendet: Dienstag, 21. Februar 2017 14:40
> An: christian.hoene at symonics.com
> Betreff: [PATCH] Added a Linux headtracker
>
> ---
> bin/vlc.c | 3 +
> modules/control/Makefile.am | 3 +
> modules/control/headtracking.c | 248
Please do not forget to add an entry in `NEWS` where you mention this
module (it is certainly worthy of such mention).
> +++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 254 insertions(+)
> create mode 100644 modules/control/headtracking.c
>
> diff --git a/bin/vlc.c b/bin/vlc.c
> index 0a33a0f..6c7913f 100644
> --- a/bin/vlc.c
> +++ b/bin/vlc.c
> @@ -235,6 +235,9 @@ int main( int i_argc, const char *ppsz_argv[] )
> libvlc_set_user_agent (vlc, "VLC media player", "VLC/"PACKAGE_VERSION);
>
> libvlc_add_intf (vlc, "hotkeys,none");
> +#if !defined (_WIN32) && !defined (__OS2__) && !defined (__APPLE__)
>
> + libvlc_add_intf (vlc, "headtracking,none"); #endif
I am not sure that we would like to enable a controller module like
this unconditionally, a user can enable extra interfaces with
`--extraintf`.
>
> #if !defined (__OS2__)
> libvlc_add_intf (vlc, "globalhotkeys,none"); #endif diff --git
> a/modules/control/Makefile.am b/modules/control/Makefile.am index
> 2469a86..56ebf03 100644
> --- a/modules/control/Makefile.am
> +++ b/modules/control/Makefile.am
> @@ -8,11 +8,14 @@ libnetsync_plugin_la_SOURCES = control/netsync.c
> libnetsync_plugin_la_LIBADD = $(SOCKET_LIBS) liboldrc_plugin_la_SOURCES =
> control/oldrc.c control/intromsg.h liboldrc_plugin_la_LIBADD =
> $(SOCKET_LIBS) $(LIBM)
> +libheadtracking_plugin_la_SOURCES = control/headtracking.c
> +libheadtracking_plugin_la_LIBADD = $(LIBM)
>
> control_LTLIBRARIES = \
> libdummy_plugin.la \
> libgestures_plugin.la \
> libhotkeys_plugin.la \
> + libheadtracking_plugin.la \
> libnetsync_plugin.la \
> liboldrc_plugin.la
>
> diff --git a/modules/control/headtracking.c b/modules/control/headtracking.c
> new file mode 100644 index 0000000..49da670
> --- /dev/null
> +++ b/modules/control/headtracking.c
> @@ -0,0 +1,248 @@
> +/**********************************************************************
> +*******
> + * headtracking.c: headtracking interface plugin
> +
> +***********************************************************************
> +******
> + * Copyright (C) 2016 Christian Hoene, Symonics GmbH
> + * $Id$
> + *
> + * Authors: Christian Hoene <christian.hoene at symonics.com>
> + *
> + * This library is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Lesser General Public
> + * License version 2.1 as published by the Free Software Foundation.
> + *
> + * This library is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> + * Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public
> + * License along with this library; if not, write to the Free Software
> + * Foundation Inc. 59 Temple Place, Suite 330, Boston MA 02111-1307
> +USA */
> +
> +/**********************************************************************
> +*******
> + * Preamble
> +
> +***********************************************************************
> +******/
> +
> +#ifdef HAVE_CONFIG_H
> +# include "config.h"
> +#endif
> +
> +#define VLC_MODULE_LICENSE VLC_LICENSE_GPL_2_PLUS #include
> +<vlc_common.h> #include <vlc_plugin.h> #include <vlc_interface.h>
> +#include <sys/types.h> #include <sys/stat.h> #include <fcntl.h>
> +#include <errno.h> #include <linux/joystick.h> #include <string.h>
> +
> +/**********************************************************************
> +*******
> + * intf_sys_t: description and status of FB interface
> +***********************************************************************
> +******/
> +struct intf_sys_t
> +{
> + libvlc_int_t* libvlc;
I do not see a reason for storing a pointer to the internal
libvlc_int_t, and given that you seem to only use it for logging
purposes you would be better storing a pointer to the modules handle
(`intf_thread_t`).
It is however debatable whether you need this *data-member* at all,
you could just as well pass the `intf_thread_t*` to your relevant
functions (instead of `intf_sys_t*`).
> +
> + vlc_timer_t discoveryTimer;
> + vlc_thread_t queryThread;
> +
> + int jsDevice;
> +
> + int running;
The threads will race to read/write from/to this variable, please
introduce synchronization to prevent such.
> +
> + double axes[3];
> +};
> +
> +/**********************************************************************
> +*******
> + * Local prototypes
> +
> ****************************************************************************
> */
> +static int Open ( vlc_object_t * );
> +static void Close ( vlc_object_t * );
> +static int ActionEvent( vlc_object_t *, char const *,
> + vlc_value_t, vlc_value_t, void * );
> +
> +
> +/**********************************************************************
> +*******
> + * Module descriptor
> +
> +***********************************************************************
> +******/
> +
> +vlc_module_begin ()
> + set_shortname( N_("Headtracking") )
> + set_description( N_("Headtracking interface") )
> + set_capability( "interface", 0 )
> + set_callbacks( Open, Close )
> + set_category( CAT_INTERFACE )
> + set_subcategory( SUBCAT_INTERFACE_CONTROL ) vlc_module_end ()
It would be neat if the user could select an explicit joystick device
through module configuration, instead of it defaulting to the first
one it can find.
> +
> +static int jsDiscovery(intf_sys_t *p_sys) {
> + char filename[20];
> + int file,i;
> +
> + for(i=0;i<32;i++) {
Please declare `i` as part of the *loop*, there is no need for it to
clutter the sourrounding scope when it is only used within the
*for-loop*.
> + sprintf(filename,"/dev/input/js%d",i);
> + file = open(filename, O_RDONLY);
> + if(file >= 0)
> + break;
> + }
> + if(file == -1)
> + return 0;
We have error-codes within the code-base, and it is encouraged to use
them (but strictly speaking it is, afaik, not a requirement for
internal functions).
> +
> + p_sys->jsDevice = file;
> + msg_Info( p_sys->libvlc, "found joystick at %s", filename);
> +
> + return 1;
> +}
> +
> +static int jsQuery(intf_sys_t *p_sys)
> +{
> + if(p_sys->jsDevice >= 0) {
> +
> + /* at start up, all events are fired */
> + struct js_event event[10];
> + int n;
> + int fire=0;
Could there be a better name for this variable (`fire`)? Initially I
was struggling to see the purpose of it (only judging by the name,
which is not a good sign).
> +
> + /* blocking read, TODO: check for thread cancel */
> + n = read(p_sys->jsDevice, (void*)event, sizeof(event));
Use `int n = read( ... )` instead of separate declaration +
initialization.
> + if(n < 0) {
> + msg_Info(p_sys->libvlc, "joystick error %d
> %s",errno,strerror(errno));
> + close(p_sys->jsDevice);
> + p_sys->jsDevice = -1;
> + return -1;
> + }
> +
> + for(struct js_event *e = event;n > 0; e++,n-=sizeof(*e)) {
> + if((e->type & ~JS_EVENT_INIT) == JS_EVENT_AXIS &&
> e->number<3) {
> + p_sys->axes[e->number] = e->value * (180. /
> 32767.);
Could there possibly be a comment to document why `32767.` is being
used, or at least a named constant?
> + fire++;
> + }
> +#ifdef DEBUG
> + else
> + msg_Info(p_sys->libvlc, "unknown js event
> %9d %04X %02X %d %p\n",
> +e->time, e->value, e->type, e->number, p_sys->axes); #endif
> + }
> + if(fire) {
> + var_SetAddress(p_sys->libvlc, "head-rotation",
> p_sys->axes);
> + }
> + }
> + return 1;
> +}
> +
> +static void* query(void *p)
> +{
> + intf_sys_t *p_sys = (intf_sys_t*)p;
> +#ifdef DEBUG
> + msg_Info( p_sys->libvlc, "query thread started"); #endif
`msg_Dbg` is, as the name implies, used for debugging-information.
> +
> + while(p_sys->running) {
> + vlc_testcancel();
> +
> + if(jsQuery(p_sys)<0)
> + break;
> +
> + }
> +
> + p_sys->running = 0;
> +
> + return NULL;
> +}
> +
> +static void discovery(void *p)
> +{
> + intf_sys_t *p_sys = (intf_sys_t*)p;
> +
> + if(p_sys->running)
> + return;
> +
> + if(!jsDiscovery(p_sys))
> + return;
> +
> + p_sys->running = 1;
> + vlc_clone(&p_sys->queryThread, &query, p_sys,
> +VLC_THREAD_PRIORITY_INPUT); }
Though somewhat unlikely, do not assume that `vlc_clone` will always
succeed.
> +
> +
> +/**********************************************************************
> +*******
> + * Open: initialize headtracking interface
> +***********************************************************************
> +******/ static int Open( vlc_object_t *p_this ) {
> + intf_thread_t *p_intf = (intf_thread_t *)p_this;
> + intf_sys_t *p_sys;
> + int res;
> +
> + msg_Info( p_intf, "using the headtracking interface module..." );
> +
> + /* get memory for structure */
> + p_sys = malloc( sizeof( intf_sys_t ) );
There is no need to separate declaration and variable initialization,
the above could be written as `intf_sys_t *p_sys = malloc( ... )`.
Same goes for `res` in this scope.
> + if( !p_sys )
> + return VLC_ENOMEM;
> + p_intf->p_sys = p_sys;
> +
> + p_sys->libvlc = p_this->obj.libvlc;
> + p_sys->jsDevice = -1;
> + p_sys->running = 0;
> + p_sys->axes[0] = p_sys->axes[1] = p_sys->axes[2] = 0;
> +
> + /* start HID discovery timer */
> + res = vlc_timer_create(&p_sys->discoveryTimer, &discovery, p_sys);
> + if(res != 0)
> + return VLC_EGENERIC;
The above return will leak `p_intf->p_sys`.
> +
> + vlc_timer_schedule(p_sys->discoveryTimer, false, 1000000, 5000000);
> +
> + /* create notification object */
> + var_Create (p_sys->libvlc, "head-rotation", VLC_VAR_ADDRESS);
> + var_AddCallback(p_sys->libvlc, "head-rotation", ActionEvent, p_sys
> +);
> +
> + msg_Info( p_intf, "done" );
The above is not worthy of `msg_Info`, if anything it should be
`msg_Dbg` (the core will however log that the module was successfully
loaded so I'd vote that you can remove it completely).
> +
> + return VLC_SUCCESS;
> +}
> +
> +/**********************************************************************
> +*******
> + * Close: destroy interface
> +
> +***********************************************************************
> +******/ static void Close( vlc_object_t *p_this ) {
> + intf_thread_t *p_intf = (intf_thread_t *)p_this;
> + intf_sys_t *p_sys = p_intf->p_sys;
> +
> + var_DelCallback( p_intf->obj.libvlc, "head-rotation", ActionEvent,
> + p_intf );
> +
> + /* destroy thread */
> + if(p_sys->running) {
> + vlc_cancel(p_sys->queryThread);
> + vlc_join(p_sys->queryThread, NULL);
> + }
> +
> + /* destroy timer */
> + vlc_timer_destroy(p_sys->discoveryTimer);
> +
> + /* Destroy structure */
> + free( p_sys );
> +
> +}
> +
> +/**********************************************************************
> +*******
> + * ActionEvent: callback for hotkey actions
> +***********************************************************************
> +******/
> +
> +static int ActionEvent( vlc_object_t *libvlc, char const *psz_var,
> + vlc_value_t oldval, vlc_value_t newval, void
> +*pdata) {
> + VLC_UNUSED(oldval);
> + VLC_UNUSED(pdata);
> +
> + double *axes = (double*) newval.p_address;
> + msg_Info( libvlc, "%s yaw %4.0f pitch %4.0f roll %4.0f", psz_var,
> axes[0], axes[1], axes[2]);
> + return VLC_SUCCESS;
> +}
> +
> +
> --
> 2.1.4
>
>
>
> _______________________________________________
> vlc-devel mailing list
> To unsubscribe or modify your subscription options:
> https://mailman.videolan.org/listinfo/vlc-devel
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mailman.videolan.org/pipermail/vlc-devel/attachments/20170221/1f09596b/attachment.html>
More information about the vlc-devel
mailing list