<!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.0 Transitional//EN" "http://www.w3.org/TR/xhtml1/DTD/xhtml1-transitional.dtd">
<html xmlns="http://www.w3.org/1999/xhtml">
<head>
<meta http-equiv="Content-Type" content="text/html; charset=utf-8" />
<meta http-equiv="Content-Style-Type" content="text/css" />
<meta name="generator" content="pandoc" />
<title></title>
<style type="text/css">code{white-space: pre;}</style>
</head>
<body>
<p>Hi Christian,</p>
<p>Even though the formatting of your <code>patch</code> 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 <code>git send-email</code> at a later time).</p>
<p>Keep up the good work!</p>
<p>Best Regards,<br />
Filip</p>
<p>On 2017-02-21 14:42, Christian Hoene wrote:</p>
<blockquote style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;color:#500050">
<pre><code> 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?</code></pre>
</blockquote>
<p>Did you consider/try using <em>evdev</em> instead of <code>linux/joystick.h</code>, and if so; is there a notable difference in precision and user-experience?</p>
<ul>
<li>https://en.wikipedia.org/wiki/Evdev</li>
</ul>
<blockquote style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;color:#500050">
<pre><code> With best regards,
Christian Hoene
-----Ursprüngliche Nachricht-----
Von: Christian Hoene [mailto:christian.hoene@symonics.com]
Gesendet: Dienstag, 21. Februar 2017 14:40
An: christian.hoene@symonics.com
Betreff: [PATCH] Added a Linux headtracker
---
bin/vlc.c | 3 +
modules/control/Makefile.am | 3 +
modules/control/headtracking.c | 248</code></pre>
</blockquote>
<p>Please do not forget to add an entry in <code>NEWS</code> where you mention this module (it is certainly worthy of such mention).</p>
<blockquote style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;color:#500050">
<pre><code> +++++++++++++++++++++++++++++++++++++++++
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</code></pre>
</blockquote>
<p>I am not sure that we would like to enable a controller module like this unconditionally, a user can enable extra interfaces with <code>--extraintf</code>.</p>
<blockquote style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;color:#500050">
<pre><code> #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@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;</code></pre>
</blockquote>
<p>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 (<code>intf_thread_t</code>).</p>
<p>It is however debatable whether you need this <em>data-member</em> at all, you could just as well pass the <code>intf_thread_t*</code> to your relevant functions (instead of <code>intf_sys_t*</code>).</p>
<blockquote style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;color:#500050">
<pre><code> +
+ vlc_timer_t discoveryTimer;
+ vlc_thread_t queryThread;
+
+ int jsDevice;
+
+ int running;</code></pre>
</blockquote>
<p>The threads will race to read/write from/to this variable, please introduce synchronization to prevent such.</p>
<blockquote style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;color:#500050">
<pre><code> +
+ 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 ()</code></pre>
</blockquote>
<p>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.</p>
<blockquote style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;color:#500050">
<pre><code> +
+static int jsDiscovery(intf_sys_t *p_sys) {
+ char filename[20];
+ int file,i;
+
+ for(i=0;i<32;i++) {</code></pre>
</blockquote>
<p>Please declare <code>i</code> as part of the <em>loop</em>, there is no need for it to clutter the sourrounding scope when it is only used within the <em>for-loop</em>.</p>
<blockquote style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;color:#500050">
<pre><code> + sprintf(filename,"/dev/input/js%d",i);
+ file = open(filename, O_RDONLY);
+ if(file >= 0)
+ break;
+ }
+ if(file == -1)
+ return 0;</code></pre>
</blockquote>
<p>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>
<blockquote style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;color:#500050">
<pre><code> +
+ 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;</code></pre>
</blockquote>
<p>Could there be a better name for this variable (<code>fire</code>)? Initially I was struggling to see the purpose of it (only judging by the name, which is not a good sign).</p>
<blockquote style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;color:#500050">
<pre><code> +
+ /* blocking read, TODO: check for thread cancel */
+ n = read(p_sys->jsDevice, (void*)event, sizeof(event));</code></pre>
</blockquote>
<p>Use <code>int n = read( ... )</code> instead of separate declaration + initialization.</p>
<blockquote style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;color:#500050">
<pre><code> + 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.);</code></pre>
</blockquote>
<p>Could there possibly be a comment to document why <code>32767.</code> is being used, or at least a named constant?</p>
<blockquote style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;color:#500050">
<pre><code> + 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</code></pre>
</blockquote>
<p><code>msg_Dbg</code> is, as the name implies, used for debugging-information.</p>
<blockquote style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;color:#500050">
<pre><code> +
+ 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); }</code></pre>
</blockquote>
<p>Though somewhat unlikely, do not assume that <code>vlc_clone</code> will always succeed.</p>
<blockquote style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;color:#500050">
<pre><code> +
+
+/**********************************************************************
+*******
+ * 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 ) );</code></pre>
</blockquote>
<p>There is no need to separate declaration and variable initialization, the above could be written as <code>intf_sys_t *p_sys = malloc( ... )</code>.</p>
<p>Same goes for <code>res</code> in this scope.</p>
<blockquote style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;color:#500050">
<pre><code> + 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;</code></pre>
</blockquote>
<p>The above return will leak <code>p_intf->p_sys</code>.</p>
<blockquote style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;color:#500050">
<pre><code> +
+ 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" );</code></pre>
</blockquote>
<p>The above is not worthy of <code>msg_Info</code>, if anything it should be <code>msg_Dbg</code> (the core will however log that the module was successfully loaded so I’d vote that you can remove it completely).</p>
<blockquote style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;color:#500050">
<pre><code> +
+ 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</code></pre>
</blockquote>
</body>
</html>