Re: [PATCH 1/1 v2] Add plugin support to libv4l
Hi, Thanks for the new patch, it looks much better. Unfortunately I found what I can only describe as a bug in the plugin rfc (which as you probasbly know I wrote). The problem is 2 fold: 1) The existence of v4l2_fd_open (and its active use by multiple applications) means that we cannot let he plugin open the fd, since it may already be opened by the app. So the plugin function dev_open should be replaced by a dev_init taking an already open fd. Note we could probably change the API and remove v4l2_fd_open, but ... The reason for actually passing the open call with a file path (ie /dev/video0) to the plugin was to allow creating a fully fake device (which would be created by a plugin when an open tries to open fakevideo0 for example). However I now realize that doing fake (or userspace originating) device is best left to a loopback v4l2 kernel driver, because: 2) The fd needs to be a real file descriptor, which also really links to the v4l2 buffer for the device, as the application may use poll or select on the fd. Note that 2 also means when working with subdevices that the application actually needs to open the subdevice which will be the exit node for the format which the apps want (raw bayer for example has a different exit node then YUV data). This is part of the unsolved enumeration problem which we discussed in Helsinki last year. So summarizing, I believe that: 1) dev_open should be replaced by a dev_init which gets passed in an already open fd for the main / exit /dev/video node for the device 2) dev_close should be replaced by dev_cleanup, which frees allocated resources but does not close the fd (for consistency) This means this patch will need to be reworked a bit more, sorry about this. ### Looking at / thinking about the mmap / munmap code for plugins, I'm wondering if we should allow plugins to intercept mmap at all (given the above where we have given up on fully fake devices). Intercepting mmap is only useful for creating fakebuffers, which in turn is only necessary for format conversion which libv4l2 already handles. So I think it would be best to remove mmap interception, if we ever get a use case scenario where plugins need to modify frame data, they can create and manage their own mapping of the buffers and modify the data inline (after a dqbuf), or we can add a separate plugin mechanism to libv4lconvert for registering new format conversion plugins. Given that the main use case is for plugins which control settings inside the webcam bridge part of soc's related to 3a I don't think that dropping mmap interception will cause any problems and it will significantly simplify the code, removing some nasty corner cases. Which are hard to get right (and the current code does not get right). ### More detailed comments inline. Note quite a few remarks have to do with mmap / munmap. If we decide to drop this, they can be ignored :) On 01/14/2011 06:18 PM, Yordan Kamenov wrote: A libv4l2 plugin will sit in between libv4l2 itself and the actual /dev/video device node a fd refers to. It will be called each time libv4l2 wants to do an operation (read/write/ioctl/mmap/munmap) on the actual /dev/video node in question. Signed-off-by: Yordan Kamenovykame...@mm-sol.com --- lib/include/libv4l2-plugin.h | 43 +++ lib/include/libv4l2.h |4 +- lib/include/libv4lconvert.h|5 +- lib/libv4l1/libv4l1.c |2 +- lib/libv4l2/Makefile |4 +- lib/libv4l2/libv4l2-priv.h | 24 ++ lib/libv4l2/libv4l2.c | 128 +++--- lib/libv4l2/v4l2-plugin.c | 344 lib/libv4l2/v4l2convert.c | 23 ++- lib/libv4lconvert/control/libv4lcontrol-priv.h |3 + lib/libv4lconvert/control/libv4lcontrol.c | 26 +- lib/libv4lconvert/control/libv4lcontrol.h |5 +- lib/libv4lconvert/libv4lconvert-priv.h |1 + lib/libv4lconvert/libv4lconvert.c | 25 +- 14 files changed, 573 insertions(+), 64 deletions(-) create mode 100644 lib/include/libv4l2-plugin.h create mode 100644 lib/libv4l2/v4l2-plugin.c diff --git a/lib/include/libv4l2-plugin.h b/lib/include/libv4l2-plugin.h new file mode 100644 index 000..3971735 --- /dev/null +++ b/lib/include/libv4l2-plugin.h @@ -0,0 +1,43 @@ +/* +* Copyright (C) 2010 Nokia Corporationmultime...@maemo.org + +* This program is free software; you can redistribute it and/or modify +* it under the terms of the GNU Lesser General Public License as published by +* the Free Software Foundation; either version 2.1 of the License, or +* (at your option) any later version. +* +* This program 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 +*
[PATCH 1/1 v2] Add plugin support to libv4l
A libv4l2 plugin will sit in between libv4l2 itself and the
actual /dev/video device node a fd refers to. It will be called each time
libv4l2 wants to do an operation (read/write/ioctl/mmap/munmap) on the
actual /dev/video node in question.
Signed-off-by: Yordan Kamenov ykame...@mm-sol.com
---
lib/include/libv4l2-plugin.h | 43 +++
lib/include/libv4l2.h |4 +-
lib/include/libv4lconvert.h|5 +-
lib/libv4l1/libv4l1.c |2 +-
lib/libv4l2/Makefile |4 +-
lib/libv4l2/libv4l2-priv.h | 24 ++
lib/libv4l2/libv4l2.c | 128 +++---
lib/libv4l2/v4l2-plugin.c | 344
lib/libv4l2/v4l2convert.c | 23 ++-
lib/libv4lconvert/control/libv4lcontrol-priv.h |3 +
lib/libv4lconvert/control/libv4lcontrol.c | 26 +-
lib/libv4lconvert/control/libv4lcontrol.h |5 +-
lib/libv4lconvert/libv4lconvert-priv.h |1 +
lib/libv4lconvert/libv4lconvert.c | 25 +-
14 files changed, 573 insertions(+), 64 deletions(-)
create mode 100644 lib/include/libv4l2-plugin.h
create mode 100644 lib/libv4l2/v4l2-plugin.c
diff --git a/lib/include/libv4l2-plugin.h b/lib/include/libv4l2-plugin.h
new file mode 100644
index 000..3971735
--- /dev/null
+++ b/lib/include/libv4l2-plugin.h
@@ -0,0 +1,43 @@
+/*
+* Copyright (C) 2010 Nokia Corporation multime...@maemo.org
+
+* This program is free software; you can redistribute it and/or modify
+* it under the terms of the GNU Lesser General Public License as published by
+* the Free Software Foundation; either version 2.1 of the License, or
+* (at your option) any later version.
+*
+* This program 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 program; if not, write to the Free Software
+* Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
+*/
+
+#ifndef __LIBV4L2_PLUGIN_H
+#define __LIBV4L2_PLUGIN_H
+
+#include sys/types.h
+
+/* Structure libv4l2_dev_ops holds the calls from libv4ls to video nodes.
+ They can be normal open/clode/ioctl etc. or any of them may be replaced
+ with a callback by a loaded plugin.
+*/
+
+struct libv4l2_dev_ops {
+ int (*open)(const char *file, int oflag, mode_t mode);
+ int (*close)(int fd);
+ int (*ioctl)(int fd, unsigned long int request, void *arg);
+ ssize_t (*read)(int fd, void *buffer, size_t n);
+ void *(*mmap)(void *start, size_t length, int prot, int flags,
+ int fd, int64_t offset);
+ int (*munmap)(void *_start, size_t length);
+};
+
+/* Plugin utility functions */
+void libv4l2_set_plugindata(int fd, void *plugin_data);
+void *libv4l2_get_plugindata(int fd);
+
+#endif
diff --git a/lib/include/libv4l2.h b/lib/include/libv4l2.h
index cc0ab4a..5db000f 100644
--- a/lib/include/libv4l2.h
+++ b/lib/include/libv4l2.h
@@ -22,6 +22,7 @@
#include stdio.h
#include unistd.h
#include stdint.h
+#include libv4l2-plugin.h
#ifdef __cplusplus
extern C {
@@ -106,7 +107,8 @@ LIBV4L_PUBLIC int v4l2_get_control(int fd, int cid);
Returns fd on success, -1 if the fd is not suitable for use through libv4l2
(note the fd is left open in this case). */
-LIBV4L_PUBLIC int v4l2_fd_open(int fd, int v4l2_flags);
+LIBV4L_PUBLIC int v4l2_fd_open(int fd, struct libv4l2_dev_ops *dev_ops,
+ int v4l2_flags);
#ifdef __cplusplus
}
diff --git a/lib/include/libv4lconvert.h b/lib/include/libv4lconvert.h
index 0264290..f210b2d 100644
--- a/lib/include/libv4lconvert.h
+++ b/lib/include/libv4lconvert.h
@@ -38,6 +38,8 @@
#include linux/videodev2.h
+#include libv4l2-plugin.h
+
#ifdef __cplusplus
extern C {
#endif /* __cplusplus */
@@ -50,7 +52,8 @@ extern C {
struct v4lconvert_data;
-LIBV4L_PUBLIC struct v4lconvert_data *v4lconvert_create(int fd);
+LIBV4L_PUBLIC struct v4lconvert_data *v4lconvert_create(int fd,
+ struct libv4l2_dev_ops *dev_ops);
LIBV4L_PUBLIC void v4lconvert_destroy(struct v4lconvert_data *data);
/* When doing flipping / rotating / video-processing, only supported
diff --git a/lib/libv4l1/libv4l1.c b/lib/libv4l1/libv4l1.c
index fee0fb7..180db0e 100644
--- a/lib/libv4l1/libv4l1.c
+++ b/lib/libv4l1/libv4l1.c
@@ -342,7 +342,7 @@ int v4l1_open(const char *file, int oflag, ...)
/* Register with libv4l2, as we use that todo format conversion and
read()
emulation for us */
- if (v4l2_fd_open(fd, 0) == -1) {
+ if (v4l2_fd_open(fd, NULL, 0) == -1) {
int saved_err = errno;
SYS_CLOSE(fd);
diff --git
