[vlc-devel] [PATCH] Support for alternative libv4l2 library (userspace drivers)

[Markus Rechberger]

On Fri, Aug 1, 2014 at 5:30 PM, Rémi Denis-Courmont <remi at remlab.net> wrote:
>    Lo,
>
> Le 2014-07-31 22:01, Markus Rechberger a écrit :
>
>> Attached patch adds support to access our userspace USB TV Tuner
>> drivers (the analogTV part - S-Video / Composite and AnalogTV)
>> directly via our interface library.
>> That way VLC would also be able to access our driver interface on
>> MacOSX, FreeBSD and Solaris.
>
>
> Hmm? FreeBSD perhaps, but I don't think VLC has the V4L plugin on MacOS nor
> Solaris.
>
>

it's a start, those devices have been around for 5 years now and there
are many users out there.
Next step would be to enable it on MacOSX and voila
analogTV/Composite/S-Video will work with it.

We also have an Android userspace port which does not require a rooted
system, however it still needs some more
work. The system will just if our driver should be able to access the
external USB device.
I'd like to follow up with that once the basic things are done.

http://sundtek.de/images/android3.jpg

http://sundtek.de/images/nvidia-tvtime.jpg


>> It is fully compatible with the official V4L2 API, the patch will have
>> no effect on existing V4L2 devices.
>
>
> If it were fully compatible, then it would intrinscally not need such a
> patch, so that is a bit misleading statement. In fact, I do not surmise why
> your library is not hooked as a proper libv4l2 plugin, which would *then* be
> fully compatible.
>

libv4l2 accesses device nodes, our one communicates via unix domain
sockets to a local server the v4l2 API is fully compatible but the
interface is different of course. I just mean that the API calls are
fully compatible. The underlying architecture is completely different.

> Further...
>
> diff --git a/modules/access/v4l2/lib.c b/modules/access/v4l2/lib.c
> index cf2b480..46057c5 100644
> --- a/modules/access/v4l2/lib.c
> +++ b/modules/access/v4l2/lib.c
> @@ -25,6 +25,8 @@
>  #include <pthread.h>
>  #include <dlfcn.h>
>  #include <unistd.h>
> +#include <fcntl.h>
> +#include <sys/stat.h>
>  #include <sys/ioctl.h>
>  #include <sys/mman.h>
>
> @@ -34,6 +36,7 @@
>
>  static void *v4l2_handle = NULL;
>  static int (*v4l2_fd_open_) (int, int);
> +static int (*v4l2_open_direct) (__const char *__file, int __oflag, ...);
>
> Identifiers with leading underscore are reserved.
>

ok.

>  int (*v4l2_close) (int);
>  int (*v4l2_ioctl) (int, unsigned long int, ...);
>  ssize_t (*v4l2_read) (int, void *, size_t);
> @@ -46,6 +49,87 @@ static int fd_open (int fd, int flags)
>      return fd;
>  }
>
> +static int v4l2_check_socket() {
>
> Incomplete prototype. This is not C++.
>

ok.

> +        int fd;
> +        char *buf=NULL;
> +        int buflen=0;
> +        int nread;
> +        char rbuf[10240];
> +        int rv;
> +        struct stat statbuf;
> +
> +        const char *sockpath="@/de/sundtek/mediasocket";
> +
> +        /* FreeBSD, Solaris, MacOSX */
> +        rv = stat("/tmp/.mediasocket", &statbuf);
> +        if (rv == 0)
> +                return 0;
>
> Looks very much like a TOCTOU race.
>
> Ditto for the procfs check.
>
> +
> +        fd = open("/proc/net/unix", O_RDONLY);
>
> Missing close-on-exec.
>

close-on-exec is handled in the library already.

> +        if (fd >= 0) {
> +                while ((nread=read(fd, rbuf, 10240))>0) {
> +                        buf = (char*)realloc(buf, buflen+nread+1);
>
> Unchecked NULL return.
>

ok.

> +                        memcpy(&buf[buflen], rbuf, nread);
> +                        buflen+=nread;
> +                }
> +                buf[buflen]=0;
> +                if (strstr(buf, sockpath)) {
>
> Reinventing the square wheel. There are fopen() and getline() for this.
>

ok.

> +                        free(buf);
> +                       close(fd);
> +                        return 0;
> +                }
> +               close(fd);
> +                free(buf);
> +        }
> +        return -1;
> +}
> +
> +static int v4l2_net_open (char *path) {
> +    void *h;
> +    int fd;
> +
> +    h = dlopen ("libmcsimple.so", RTLD_LAZY | RTLD_LOCAL);
> +    if (h == NULL) {
> +       /* this is the default driver location */
> +       h = dlopen("/opt/lib/libmcsimple.so", RTLD_LAZY | RTLD_LOCAL);
>
> From a quick glance, that library is not reentrant.
>

the library has locking inside to avoid race conditions.

> +       if (h == NULL)
> +            goto fallback;
> +    }
> +    /* no need for that one */
> +    v4l2_open_direct = dlsym (h, "net_open");
> +    v4l2_close = dlsym (h, "net_close");
> +    v4l2_ioctl = dlsym (h, "net_ioctl");
> +    v4l2_read = dlsym (h, "__net_read");
> +    v4l2_mmap = dlsym (h, "net_mmap");
> +    v4l2_munmap = dlsym (h, "net_munmap");
>
> This breaks reentrancy and the memory model.
> The reast of the assumes that the back-end library is constant throughout
> the lifetime of the process.
>

I see what you mean here, I'll do some further tests.
But I guess it should be more dynamic and just be checked when the
user tries to open a node.

Aside of that I was wondering about the pthread_once inside the
original source, can you elaborate what the purpose
of that is instead of doing those things directly without pthread_once?

> +
> +    if (v4l2_open_direct != NULL && v4l2_close != NULL && v4l2_ioctl !=
> NULL
> +     && v4l2_read != NULL && v4l2_mmap != NULL && v4l2_munmap != NULL)
> +    {
> +        v4l2_handle = h;
> +        fd = v4l2_open_direct(path, O_RDWR);
> +        if (fd == -1) {
> +                v4l2_handle = NULL;
> +                dlclose(h);
> +                return -1;
> +        } else {
> +                return fd;
> +        }
> +    }
> +
> +    dlclose (h);
> +fallback:
> +    return -1;
> +}
> +
> +int v4l2_check_device(char *path) {
> +        int sk;
> +        sk = v4l2_check_socket();
> +        if (sk == 0)
> +                return v4l2_net_open (path);
> +        return -1;
> +}
> +
>  static void v4l2_lib_load (void)
>  {
>      void *h = dlopen ("libv4l2.so.0", RTLD_LAZY | RTLD_LOCAL);
> diff --git a/modules/access/v4l2/v4l2.c b/modules/access/v4l2/v4l2.c
> index 7807a8a..beb95d9 100644
> --- a/modules/access/v4l2/v4l2.c
> +++ b/modules/access/v4l2/v4l2.c
> @@ -466,21 +466,29 @@ int OpenDevice (vlc_object_t *obj, const char *path,
> uint32_t *restrict caps)
>  {
>      msg_Dbg (obj, "opening device '%s'", path);
>
> -    int rawfd = vlc_open (path, O_RDWR);
> -    if (rawfd == -1)
> -    {
> -        msg_Err (obj, "cannot open device '%s': %s", path,
> -                 vlc_strerror_c(errno));
> -        return -1;
> -    }
> -
> -    int fd = v4l2_fd_open (rawfd, 0);
> -    if (fd == -1)
> -    {
> -        msg_Warn (obj, "cannot initialize user-space library: %s",
> -                  vlc_strerror_c(errno));
> -        /* fallback to direct kernel mode anyway */
> -        fd = rawfd;
> +    int rawfd;
> +    int fd;
> +
> +    fd = v4l2_check_device(path);
>
> I don't really get why this should override even the normal existing V4L
> devices.
>

it just checks if the server is available if not it will just do the
regular thing.
It's a different multimedia subsystem so either an extra plugin or
some hook on the existing one will be needed
to access the functionality directly.

> +
> +    if (fd == -1) {
> +
> +           rawfd = vlc_open (path, O_RDWR);
> +           if (rawfd == -1)
> +           {
> +               msg_Err (obj, "cannot open device '%s': %s", path,
> +                        vlc_strerror_c(errno));
> +               return -1;
> +           }
> +
> +           fd = v4l2_fd_open (rawfd, 0);
> +           if (fd == -1)
> +           {
> +               msg_Warn (obj, "cannot initialize user-space library: %s",
> +                         vlc_strerror_c(errno));
> +               /* fallback to direct kernel mode anyway */
> +               fd = rawfd;
> +           }
>      }
>
>      /* Get device capabilites */
> diff --git a/modules/access/v4l2/v4l2.h b/modules/access/v4l2/v4l2.h
> index 9888785..da28519 100644
> --- a/modules/access/v4l2/v4l2.h
> +++ b/modules/access/v4l2/v4l2.h
> @@ -22,6 +22,7 @@
>
>  /* libv4l2 functions */
>  extern int v4l2_fd_open (int, int);
> +extern int v4l2_check_device(char *path);
>  extern int (*v4l2_close) (int);
>  extern int (*v4l2_ioctl) (int, unsigned long int, ...);
>  extern ssize_t (*v4l2_read) (int, void *, size_t);
>
>


thank you for the review!

Markus

> --
> Rémi Denis-Courmont
>