Re: [PATCH 1/1] Add plugin support to libv4l

[Yordan Kamenov <ykamenov@mm-sol.com>]

Hi Hans,

Thanks for your comments.

Hans de Goede wrote:
> Hi,
>
> First of all many thanks for working on this! I've several remarks
> which I would like to see addressed before merging this.
>
> Since most remarks are rather high level remarks I've opted to
> just make a bulleted list of them rather then inserting them inline.
>
> * The biggest problem with your current implementation is that for
> each existing libv4l2_foo function you check if there is a plugin 
> attached
> to the fd passed in and if that plugin wants to handle the call. Now lets
> assume that there is a plugin and that it wants to handle all calls. That
> means that you've now effectively replaced all libv4l2_foo calls
> with calling the corresponding foo function from the plugin and returning
> its result. This means that for this fd / device you've achieved the
> same result as completely replacing libv4l2.so.0 with a new library
> containing the plugin code.
>
> IOW you've not placed then plugin between libv4l2 and the device (as
> intended) but completely short-circuited / replaced libv4l2. This means
> for example for a device which only supports yuv output, that libv4l2 
> will
> no longer do format emulation and conversion and an app which only 
> supports
> devices which deliver rgb data will no longer work.
>
> To actually place the plugin between libv4l2 (and libv4lconvert) and the
> device, you should replace all the SYS_FOO calls in libv4l2. The SYS_FOO
> calls are the calls to the actual device, so be replacing those with 
> calls
> to the plugin you actual place the plugin between libv4l and the 
> device as
> intended.
I agree with that, currently the plugin can replace the libv4l2, but if
we replace SYS_FOO calls it will actually sit between library and the
video node. I will do that.
>
> * Currently you add a loop much like the one in the v4l2_get_index
> function to each libv4l2_plugin function. Basically you add an array of
> v4l2_plugin_info structs in libv4l2-plugin. Which gets searched by fd,
> much like the v4l2_dev_info struct array. Including needing similar
> locking. I would like you to instead just store the plugin info for
> a certain fd directly into the v4l2_dev_info struct. This way the
> separate array, looping and locking can go away.
I have put separate array, because the array of v4l2_dev_info is declared
static in libv4l2.c and is not visible to v4l2-plugin.c (I did't want to
change it's declaration). But with changes that you suggest below, there
is no problem to add plugin data to v4l2_dev_info.
>
> * Next I would also like to see all the libv4l2_plugin_foo functions
> except for libv4l2_plugin_open go away. Instead libv4l2.c can call
> the plugin functions directly. Let me try to explain what I have in
> mind. Lets say we store the struct libv4l2_plugin_data pointer to the
> active plugin in the v4l2_dev_info struct and name it dev_ops
> (short for device operations).
>
> Then we can replace all SYS_FOO calls inside libv4l2 (except the ones
> were v4l2_get_index returns -1), with a call to the relevant devop
> functions, for example:
>                 result = SYS_IOCTL(devices[index].fd, VIDIOC_REQBUFS, 
> req);
> Would become:
>                 result = devices[index].dev_ops->v4l2_plugin_ioctl(
>                                     devices[index].fd, VIDIOC_REQBUFS, 
> req);
>
> Note that the plugin_used parameter of the v4l2_plugin_ioctl is gone,
> it should simply do a normal SYS_IOCTL and return the return value
> of that if it is not interested in intercepting the ioctl (you could move
> the definition of the SYS_FOO macros to libv4l2-plugin.h to make them
> availables to plugins).
>
> Also I think it would be better to rename the function pointers inside
> the libv4l2_plugin_data struct from v4l2_plugin_foo to just foo, so
> that the above code would become:
>                 result = devices[index].dev_ops->v4l2_plugin_ioctl(
>                                     devices[index].fd, VIDIOC_REQBUFS, 
> req);
>
> * The above means that need to always have a dev_ops pointer, so we
> need to have a default_dev_ops struct to use when no plugin wants to
> talk to the device.
Ok, I will replace SYS_FOO calls with dev_ops structure.
>
> * You've put the v4l2_plugin_foo functions (of which only
> v4l2_plugin_foo will remain in my vision) in lib/include/libv4l2.h
> I don't think these functions should be public, their prototypes should
> be moved to lib/libv4l2/libv4l2-priv.h, and they should not be declared
> LIBV4L_PUBLIC.
>
> * There is one special case in all this, files under libv4lconvert also
> use SYS_IOCTL in various places. Since this now need to go through the
> plugin we need to take some special measures here. There are 2 options:
> 1) Break the libv4lconvert ABI (very few programs use it) and pass a
>    struct libv4l2_plugin_data pointer to the v4lconvert_create function.
>    *And* export the default_dev_ops struct from libv4l2.
> 2) Add a libv4l2_raw_ioctl method, which just gets the index and then
>    does devices[index].dev_ops->v4l2_plugin_ioctl
>    Except that this is not really an option as libv4lconvert should not
>    depend on libv4l2
> My vote personally goes to 1.
>
> * I think that once we do 1) from above it would be good to rename
> libv4l2_plugin_data to libv4l2_dev_ops, as that makes the public API
> more clear and dev_ops is in essence what a plugin provides.
>
> * Note that were I wrote: "like to see all the libv4l2_plugin_foo
> functions except for libv4l2_plugin_open go away" I did so for
> simplicity, in reality the wrappers around mmap and munmap need to
> stay too, but they should use data directly stored inside the
> v4l2_dev_info struct. This means that we need to either:
> mv the mmap and munmap code to libv4l2.c; or export the v4l2_dev_info
> struct array. I vote for exporting the v4l2_dev_info struct array
> (through libv4l2-priv.h, so it won't be visible to the outside
> world, but it will be usable outside libv4l2.c).
Ok, I will make 1) and export v4l2_dev_info.
>
> Thanks & Regards,
>
> Hans
>
Best Regards
Yordan