[libcamera-devel] [PATCH v2 2/2] v4l2: v4l2_compat: Add V4L2 compatibility layer
Paul Elder
paul.elder at ideasonboard.com
Mon Dec 16 07:18:53 CET 2019
Hi Laurent,
Thank you for the review.
On Mon, Dec 16, 2019 at 02:18:44AM +0200, Laurent Pinchart wrote:
> Hi Paul,
>
> Thank you for the patch.
>
> On Mon, Dec 09, 2019 at 11:30:13AM +0100, Jacopo Mondi wrote:
> > On Sun, Dec 08, 2019 at 11:56:03PM -0500, Paul Elder wrote:
> > > Add libcamera V4L2 compatibility layer.
> > >
> > > This initial implementation supports the minimal set of V4L2 operations,
> > > which allows getting, setting, and enumerating formats, and streaming
> > > frames from a video device. Some data about the wrapped V4L2 video
> > > device are hardcoded.
> > >
> > > Add a build option named 'v4l2' and adjust the build system to
> > > selectively compile the V4L2 compatibility layer.
> > >
> > > Note that until we have a way of mapping V4L2 device nodes to libcamera
> > > cameras, the V4L2 compatibility layer will always selects and use the
>
> s/selects/select/
This paragraph got removed after I replaced it with the short paragraph
describing the temporary (?) mapping using sysfs names.
> > > first enumerated libcamera camera.
> > >
> > > Signed-off-by: Paul Elder <paul.elder at ideasonboard.com>
> > >
> > > ---
> > > Changes in v2:
> > > - move all errno acrobatics to V4L2CameraProxy
> > > - remove all mentions of dmabuf
> > > - make V4L2CompatManager::getCamera return pointer rather than
> > > shared_ptr
> > > - match V4L2 device nodes to libcamera cameras using Camera::name()
> > > compared to /sys/dev/char/maj:min/name (only works for UVC cameras)
> > > - in V4L2CompatManager::getCameraIndex()
> > > - add -fvisibility=hidden to v4l2 compat
> > > - cache the results of V4L2CompatManager::imageSize() within V4L2Camera
> > > (where V4L2Camera is interested)
> > > - remove V4L2CompatManager::valid[fd|mmap], and where those methods were
> > > used, check V4L2CompatManager::getCamera() != nullptr instead
> > > - fixed V4L2CompatManager::drmToV4L2() mappings for DRM_FORMAT_BGR888
> > > and DRM_FORMAT_RGB888
> > > - other cosmetic changes
> > > ---
> > > meson_options.txt | 5 +
> > > src/meson.build | 4 +
> > > src/v4l2/meson.build | 30 ++
> > > src/v4l2/v4l2_camera.cpp | 299 ++++++++++++++++++++
> > > src/v4l2/v4l2_camera.h | 67 +++++
> > > src/v4l2/v4l2_camera_proxy.cpp | 452 +++++++++++++++++++++++++++++++
> > > src/v4l2/v4l2_camera_proxy.h | 63 +++++
> > > src/v4l2/v4l2_compat.cpp | 81 ++++++
> > > src/v4l2/v4l2_compat_manager.cpp | 382 ++++++++++++++++++++++++++
> > > src/v4l2/v4l2_compat_manager.h | 86 ++++++
> > > 10 files changed, 1469 insertions(+)
> > > create mode 100644 src/v4l2/meson.build
> > > create mode 100644 src/v4l2/v4l2_camera.cpp
> > > create mode 100644 src/v4l2/v4l2_camera.h
> > > create mode 100644 src/v4l2/v4l2_camera_proxy.cpp
> > > create mode 100644 src/v4l2/v4l2_camera_proxy.h
> > > create mode 100644 src/v4l2/v4l2_compat.cpp
> > > create mode 100644 src/v4l2/v4l2_compat_manager.cpp
> > > create mode 100644 src/v4l2/v4l2_compat_manager.h
> > >
> > > diff --git a/meson_options.txt b/meson_options.txt
> > > index 1a328045..b06dd494 100644
> > > --- a/meson_options.txt
> > > +++ b/meson_options.txt
> > > @@ -10,3 +10,8 @@ option('documentation',
> > > option('test',
> > > type : 'boolean',
> > > description: 'Compile and include the tests')
> > > +
> > > +option('v4l2',
> > > + type : 'boolean',
> > > + value : false,
> > > + description : 'Compile libcamera with V4L2 compatibility layer')
>
> How about 'Compile the V4L2 adaptation layer', as it's not part of
> libcamera.so ?
ack
Although, I wrote "compatibility" instead of "adaptation" everywhere
because that's what the documentation on libcamera.org says (V4L2
Compatibility Layer).
> > > diff --git a/src/meson.build b/src/meson.build
> > > index 67ad20aa..5adcd61f 100644
> > > --- a/src/meson.build
> > > +++ b/src/meson.build
> > > @@ -6,3 +6,7 @@ subdir('libcamera')
> > > subdir('ipa')
> > > subdir('cam')
> > > subdir('qcam')
> > > +
> > > +if get_option('v4l2')
> > > + subdir('v4l2')
> > > +endif
> > > diff --git a/src/v4l2/meson.build b/src/v4l2/meson.build
> > > new file mode 100644
> > > index 00000000..1650048e
> > > --- /dev/null
> > > +++ b/src/v4l2/meson.build
> > > @@ -0,0 +1,30 @@
> > > +v4l2_compat_sources = files([
> > > + 'v4l2_camera.cpp',
> > > + 'v4l2_camera_proxy.cpp',
> > > + 'v4l2_compat.cpp',
> > > + 'v4l2_compat_manager.cpp',
> > > +])
> > > +
> > > +v4l2_compat_includes = [
> > > + libcamera_includes,
> > > + libcamera_internal_includes,
> > > +]
> > > +
> > > +v4l2_compat_cpp_args = [
> > > + # Meson enables large file support unconditionally, which redirect file
> > > + # operations to 64-bit versions. This results in some symbols being
> > > + # renamed, for instance open() being renamed to open64(). As the V4L2
> > > + # adaptation wrapper needs to provide both 32-bit and 64-bit versions of
> > > + # file operations, disable transparent large file support.
> > > + '-U_FILE_OFFSET_BITS',
>
> As explained in my review of v1, I would prefer also defining
> _FILE_OFFSET_BITS explicitly.
ack
> > > + '-fvisibility=hidden',
> > > +]
> > > +
> > > +v4l2_compat = shared_library('v4l2-compat',
> > > + v4l2_compat_sources,
> > > + name_prefix : '',
> >
> > Do you need this ? I tried removing it and it succesfully compiled...
>
> But does it then generate a shared object without the 'lib' prefix ?
>
> > > + install : true,
> > > + link_with : libcamera,
> > > + include_directories : v4l2_compat_includes,
> > > + dependencies : libcamera_deps,
> > > + cpp_args : v4l2_compat_cpp_args)
>
> [snip]
>
> Skipping over V4L2Camera and V4L2CameraProxy, I'll get to those
> tomorrow.
>
> > > diff --git a/src/v4l2/v4l2_compat.cpp b/src/v4l2/v4l2_compat.cpp
> > > new file mode 100644
> > > index 00000000..3330e7bc
> > > --- /dev/null
> > > +++ b/src/v4l2/v4l2_compat.cpp
> > > @@ -0,0 +1,81 @@
> > > +/* SPDX-License-Identifier: GPL-2.0-or-later */
> > > +/*
> > > + * Copyright (C) 2019, Google Inc.
> > > + *
> > > + * v4l2_compat.cpp - V4L2 compatibility layer
>
> "V4L2 adaptation layer" ?
The documentation page on libcamera.org says V4L2 Compatibility Layer :)
> > > + */
> > > +
> > > +#include "v4l2_compat_manager.h"
> > > +
> > > +#include <iostream>
>
> You don't need this header.
ack
> > > +
> > > +#include <errno.h>
> > > +#include <fcntl.h>
> > > +#include <linux/videodev2.h>
>
> I don't think you need this header either.
ack
> > > +#include <stdarg.h>
> > > +#include <sys/mman.h>
> > > +#include <sys/stat.h>
> > > +#include <sys/types.h>
> > > +
> > > +#define LIBCAMERA_PUBLIC __attribute__((visibility("default")))
> > > +
> > > +using namespace libcamera;
> > > +
> > > +#define extract_va_arg(type, arg, last) \
> > > +{ \
> > > + va_list ap; \
> > > + va_start(ap, last); \
> > > + arg = va_arg(ap, type); \
> > > + va_end(ap); \
> > > +}
> > > +
> > > +extern "C" {
>
> I would add a blank line here, as you add a blank line before the
> corresponding }.
ack
> > > +LIBCAMERA_PUBLIC int open(const char *path, int oflag, ...)
> > > +{
> > > + mode_t mode = 0;
> > > + if (oflag & O_CREAT || oflag & O_TMPFILE)
> > > + extract_va_arg(mode_t, mode, oflag);
> > > +
> > > + return openat(AT_FDCWD, path, oflag, mode);
>
> You could call
>
> return V4L2CompatManager::instance()->openat(AT_FDCWD, path, oflag,
> mode);
>
> to avoid one layer of indirection.
ack
> > > +}
> > > +
> > > +LIBCAMERA_PUBLIC int openat(int dirfd, const char *path, int oflag, ...)
> > > +{
> > > + mode_t mode = 0;
> > > + if (oflag & O_CREAT || oflag & O_TMPFILE)
> > > + extract_va_arg(mode_t, mode, oflag);
> > > +
> > > + return V4L2CompatManager::instance()->openat(dirfd, path, oflag, mode);
> > > +}
> > > +
> > > +LIBCAMERA_PUBLIC int dup(int oldfd)
> > > +{
> > > + return V4L2CompatManager::instance()->dup(oldfd);
> > > +}
> > > +
> > > +LIBCAMERA_PUBLIC int close(int fd)
> > > +{
> > > + return V4L2CompatManager::instance()->close(fd);
> > > +}
> > > +
> > > +LIBCAMERA_PUBLIC void *mmap(void *addr, size_t length, int prot, int flags,
> > > + int fd, off_t offset)
> > > +{
> > > + void *val = V4L2CompatManager::instance()->mmap(addr, length, prot, flags, fd, offset);
> > > + return val;
>
> Anything wrong with
>
> return V4L2CompatManager::instance()->mmap(addr, length, prot, flags,
> fd, offset);
>
> ?
ack
> > > +}
> > > +
> > > +LIBCAMERA_PUBLIC int munmap(void *addr, size_t length)
> > > +{
> > > + return V4L2CompatManager::instance()->munmap(addr, length);
> > > +}
> > > +
> > > +LIBCAMERA_PUBLIC int ioctl(int fd, unsigned long request, ...)
> > > +{
> > > + void *arg;
> > > + extract_va_arg(void *, arg, request);
> > > +
> > > + return V4L2CompatManager::instance()->ioctl(fd, request, arg);
> > > +}
> > > +
> > > +}
> > > diff --git a/src/v4l2/v4l2_compat_manager.cpp b/src/v4l2/v4l2_compat_manager.cpp
> > > new file mode 100644
> > > index 00000000..90416b35
> > > --- /dev/null
> > > +++ b/src/v4l2/v4l2_compat_manager.cpp
> > > @@ -0,0 +1,382 @@
> > > +/* SPDX-License-Identifier: GPL-2.0-or-later */
> > > +/*
> > > + * Copyright (C) 2019, Google Inc.
> > > + *
> > > + * v4l2_compat_manager.cpp - V4L2 compatibility manager
> > > + */
>
> Missing blank line.
ack
...wait most of my files are like this; gotta fix 'em all
> > > +#include "v4l2_compat_manager.h"
> > > +
> > > +#include <dlfcn.h>
> > > +#include <fcntl.h>
> > > +#include <fstream>
> > > +#include <iostream>
>
> I don't think you need iostream here either.
ack
> > > +#include <linux/drm_fourcc.h>
> > > +#include <linux/videodev2.h>
> > > +#include <map>
> > > +#include <stdarg.h>
> > > +#include <string.h>
> > > +#include <sys/eventfd.h>
> > > +#include <sys/mman.h>
> > > +#include <sys/stat.h>
> > > +#include <sys/sysmacros.h>
> > > +#include <sys/types.h>
> > > +#include <unistd.h>
> > > +
> > > +#include <libcamera/camera.h>
> > > +#include <libcamera/camera_manager.h>
> > > +#include <libcamera/stream.h>
> > > +
> > > +#include "log.h"
> > > +
> > > +using namespace libcamera;
> > > +
> > > +LOG_DEFINE_CATEGORY(V4L2Compat)
> > > +
> > > +V4L2CompatManager::V4L2CompatManager()
> > > + : cm_(nullptr)
> > > +{
> > > + openat_func_ = (openat_func_t )dlsym(RTLD_NEXT, "openat");
> > > + dup_func_ = (dup_func_t )dlsym(RTLD_NEXT, "dup");
> > > + close_func_ = (close_func_t )dlsym(RTLD_NEXT, "close");
> > > + ioctl_func_ = (ioctl_func_t )dlsym(RTLD_NEXT, "ioctl");
> > > + mmap_func_ = (mmap_func_t )dlsym(RTLD_NEXT, "mmap");
> > > + munmap_func_ = (munmap_func_t )dlsym(RTLD_NEXT, "munmap");
>
> You can remove one space before the ) in each of those lines.
ack
> > > +}
> > > +
> > > +V4L2CompatManager::~V4L2CompatManager()
> > > +{
> > > + devices_.clear();
> > > +
> > > + if (isRunning()) {
> > > + exit(0);
> > > + /* \todo Wait with a timeout, just in case. */
> > > + wait();
> > > + }
> > > +}
> > > +
> > > +int V4L2CompatManager::init()
> > > +{
> > > + start();
> > > +
> > > + MutexLocker locker(mutex_);
> > > + cv_.wait(locker);
>
> There's a risk of a race condition here, where the new thread could
> notify the condition variable before you start waiting on it. The wait
> would then never complete. You need to add an initialized_ member to the
> class, initialize it to false in the constructor, set it to true before
> calling cv_.notify_one() in run(), and call
>
> cv_.wait(locker, []{ return initialized_; });
>
> here.
ack
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +void V4L2CompatManager::run()
> > > +{
> > > + cm_ = new CameraManager();
> > > +
> > > + int ret = cm_->start();
> > > + if (ret) {
> > > + LOG(V4L2Compat, Error) << "Failed to start camera manager: "
> > > + << strerror(-ret);
> > > + return;
> > > + }
> > > +
> > > + LOG(V4L2Compat, Debug) << "Started camera manager";
> > > +
> > > + /*
> > > + * For each Camera registered in the system, a V4L2CameraProxy
> > > + * gets created here to wraps a camera device.
>
> s/wraps/wrap/
This was copied from CameraHalManager::run(), which means that that has
the same typo :)
> and you can go up to 80 columns before wrapping.
ack
> > > + */
> > > + unsigned int index = 0;
> > > + for (auto &camera : cm_->cameras()) {
> > > + V4L2CameraProxy *proxy = new V4L2CameraProxy(index, camera);
> > > + proxies_.emplace_back(proxy);
> > > + ++index;
> > > + }
> > > +
> > > + /*
> > > + * libcamera has been initialized. Unlock the init() caller
> > > + * as we're now ready to handle calls from the framework.
>
> You can go up to 80 columns before wrapping here too.
ack
> > > + */
>
> mutex_.lock();
> initialized_ = true;
> mutex_.unlock();
ack
> > > + cv_.notify_one();
> > > +
> > > + /* Now start processing events and messages. */
> > > + exec();
> > > +
> > > + cm_->stop();
> > > + delete cm_;
> > > + cm_ = nullptr;
> > > +}
> > > +
> > > +V4L2CompatManager *V4L2CompatManager::instance()
> > > +{
> > > + static V4L2CompatManager v4l2CompatManager;
> > > + return &v4l2CompatManager;
>
> s/v4l2CompatManager/instance/ ?
ack
> > > +}
> > > +
> > > +V4L2CameraProxy *V4L2CompatManager::getCamera(int fd)
> > > +{
> > > + auto device = devices_.find(fd);
> > > + if (device == devices_.end())
> > > + return nullptr;
> > > +
> > > + return device->second.get();
> > > +}
> > > +
> > > +V4L2CameraProxy *V4L2CompatManager::getCamera(void *addr)
> > > +{
> > > + auto map = mmaps_.find(addr);
> > > + if (map == mmaps_.end())
> > > + return nullptr;
> > > +
> > > + return devices_.at(map->second).get();
> > > +}
> > > +
> > > +int V4L2CompatManager::getCameraIndex(int fd)
> > > +{
> > > + struct stat statbuf;
> > > + fstat(fd, &statbuf);
> > > + unsigned int dev_major = major(statbuf.st_rdev);
> > > + unsigned int dev_minor = minor(statbuf.st_rdev);
>
> devMajor and devMinor.
ack
> > > +
> > > + std::string name;
> > > + std::ifstream nameFile;
> > > + nameFile.open("/sys/dev/char/" + std::to_string(dev_major) + ":" +
> > > + std::to_string(dev_minor) + "/name");
> > > + if (!nameFile)
> > > + return -1;
> > > +
> > > + std::getline(nameFile, name);
> > > + nameFile.close();
> > > +
> > > + unsigned int index = 0;
> > > + for (auto &camera : cm_->cameras()) {
> > > + if (!camera->name().compare(name))
> > > + break;
> > > + ++index;
> > > + }
>
> As commented on v1, matching on the video node name isn't a good idea,
> as we want to support all cameras, and not all cameras are named based
> on the video node name. I wrote
>
> "One option would be to add a method to the camera manager to retrieve a
> camera by device node path. This wouldn't expose any V4L2-specific
> information towards applications, the map of device nodes to cameras
> would be private."
>
> but I don't think that's right, we should instead retrieve a camera by
> major:minor, as multiple device nodes in the system could
Doesn't the major:minor devnum map uniquely to device node paths?
What do we do when different device nodes/devnums map to the same
libcamera camera but different stream etc? Simply map to the libcamera
camera at the simplest setting?
> > > +
> > > + if (index >= cm_->cameras().size())
> > > + return -1;
> > > +
> > > + return index;
> > > +}
> > > +
> > > +int V4L2CompatManager::openat(int dirfd, const char *path, int oflag, mode_t mode)
> > > +{
> > > + int fd = openat_func_(dirfd, path, oflag, mode);
> > > + if (fd < 0)
> > > + return fd;
> > > +
> > > + if (std::string(path).find("video") == std::string::npos)
> > > + return fd;
>
> So opening "/home/user/myvideo.mp4" will match :-)
Oh, oops :p
> You can't match by name here, you need to fstat() and check that the
> file is a character device node whose major is equal to the V4L2 major.
I thought I did that a long time ago... I guess it disappeared.
> Or, if you add a method to the camera manager to retrieve a camera by
> major:minor, I think you could even skip this check completely as it
> would be handled in CameraManager (but the fstat is needed nonetheless).
ack
> > > +
> > > + if (!isRunning())
> > > + init();
> > > +
> > > + int ret = getCameraIndex(fd);
> > > + if (ret < 0) {
> > > + LOG(V4L2Compat, Error) << "No camera found for " << path;
>
> I'd turn that into an Info or Debug message as it's not necessarily an
> error, not all cameras are supported by libcamera.
True.
> > > + return fd;
> > > + }
>
> You need to close_func_(fd) here.
Why? The application has called open() on a V4L2 device node that we
don't support, so shouldn't we just let it through? Should we block it
instead?
> > > +
> > > + unsigned int camera_index = static_cast<unsigned int>(ret);
> > > +
> > > + std::shared_ptr<V4L2CameraProxy> proxy = proxies_[camera_index];
> >
> > You here share the ownership of the managed object owned by
> > proxies_[camera_index] with a local variable, increasing its
> > reference count
> >
> > > + ret = proxy->open(mode & O_NONBLOCK);
> > > + if (ret < 0)
> > > + return ret;
> > > +
> > > + int efd = eventfd(0, (mode & O_CLOEXEC) | (mode & O_NONBLOCK));
> > > + if (efd < 0)
> > > + return efd;
> >
> > I'm still not convinced about this, I'll go read your reply to v1
> > again.
>
> What's wrong with it ? We need an fd that applications can use in a
> select() or poll() call, so we can't return the fd for the video node,
> we need an eventfd that we can signal when a request completes.
>
> > > +
> > > + devices_.emplace(efd, proxy);
> >
> > And here you store it permanently in a map, making its reference count
> > permanently = 2 (it goes up to 3, then the local variable goes out of
> > scope and gest back to 2).
>
> I don't think this is a big issue, and reference counting is needed to
> support hot-unplug. As we don't support it yet we could turn proxies_
> into a std::vector<std::unique_ptr<V4L2CameraProxy>> and devices_ into a
> std::map<int, V4L2CameraProxy *>>, knowing that the device won't be
> deleted until the V4L2CompatManager itself is deleted. The above lookup
> in proxies_ would then become
>
> V4L2CameraProxy *proxy = proxies_[camera_index].get();
>
> I think the mmaps_ could then map from void * to V4L2CameraProxy *,
> which would avoid double look-ups in munmap().
Good idea.
> > > +
> > > + return efd;
> > > +}
> > > +
> > > +int V4L2CompatManager::dup(int oldfd)
> > > +{
> > > + int newfd = dup_func_(oldfd);
> > > + if (getCamera(oldfd))
> > > + devices_[newfd] = devices_[oldfd];
> >
> > Then here you create a new copy, with reference count = 3
> >
> > At delete time you clear all the entries in devices_, causing the
> > shared pointer stored in proxies_ to get back to 1, then proxies_ goes
> > out of scope, and the managed object gets deleted.
> >
> > Why all this dance ? Is the ownership of the CameraProxy shared
> > between different componentes? Is it created or destoryed outside of
> > this class ? I would save all this complexity and store
> > V4L2CameraProxy instances in a vector, and point to them in the map at
> > openat() time.
> >
> > The only thing you care about is to increase the reference count to
> > the Camera, but you have a shared_ptr<> class instance in V4L2Camera,
> > so that's fine.
>
> To support hot-unplug we'll likely need shared pointers, for now we can
> do without them and use unique_ptr as explained above.
>
> > > +
> > > + return newfd;
> > > +}
> > > +
> > > +int V4L2CompatManager::close(int fd)
> > > +{
> > > + V4L2CameraProxy *proxy = getCamera(fd);
> > > + if (proxy)
> > > + return proxy->close();
>
> This will break if the fd has been dup'ed. Closing one of the two fds
> will close the proxy, the second fd will then be unusable. You need to
> refcount opens in V4L2CameraProxy.
Oh yeah...
> You also need to remove fd from the devices_ map here.
>
> > > +
> > > + int ret = close_func_(fd);
> > > + return ret;
>
> return close_func_(fd);
>
> > > +}
> > > +
> > > +void *V4L2CompatManager::mmap(void *addr, size_t length, int prot, int flags,
> > > + int fd, off_t offset)
> >
> > Align to open ( please
>
> it is aligned :-)
>
> > Mostly stylistic comments, so I think we're almost there, at least
> > from my side!
> >
> > > +{
> > > + V4L2CameraProxy *proxy = getCamera(fd);
> > > + if (!proxy)
> > > + return mmap_func_(addr, length, prot, flags, fd, offset);
> > > +
> > > + void *map = proxy->mmap(addr, length, prot, flags, offset);
> > > + if (map == MAP_FAILED)
> > > + return map;
> > > +
> > > + mmaps_[addr] = fd;
>
> addr is a hint, there's no guarantee that the buffer will be mapped at
> addr, and it is often set to NULL by the caller. The correct index for
> mmaps_ is map, not addr.
ack
> > > + return map;
> > > +}
> > > +
> > > +int V4L2CompatManager::munmap(void *addr, size_t length)
> > > +{
> > > + V4L2CameraProxy *proxy = getCamera(addr);
> > > + if (!proxy)
> > > + return munmap_func_(addr, length);
>
> As getCamera(void *) is only called in this method, I would inline it:
>
> auto map = mmaps_.find(addr);
> if (map == mmaps_.end())
> return munmap_func_(addr, length);
>
> V4L2CameraProxy *proxy = devices_.at(map->second).get();
>
> > > +
> > > + int ret = proxy->munmap(addr, length);
> > > + if (ret < 0)
> > > + return ret;
> > > +
> > > + mmaps_.erase(addr);
>
> And here you could then call
>
> mmaps_.erase(map);
>
> > > + addr = nullptr;
>
> This isn't needed.
ack
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +int V4L2CompatManager::ioctl(int fd, unsigned long request, void *arg)
> > > +{
> > > + V4L2CameraProxy *proxy = getCamera(fd);
> > > + if (!proxy)
> > > + return ioctl_func_(fd, request, arg);
> > > +
> > > + return proxy->ioctl(request, arg);
> > > +}
> > > +
> > > +/* \todo make libcamera export these */
> > > +int V4L2CompatManager::bplMultiplier(unsigned int format)
> > > +{
> > > + switch (format) {
> > > + case V4L2_PIX_FMT_NV12:
> > > + case V4L2_PIX_FMT_NV21:
> > > + case V4L2_PIX_FMT_NV16:
> > > + case V4L2_PIX_FMT_NV61:
> > > + case V4L2_PIX_FMT_NV24:
> > > + case V4L2_PIX_FMT_NV42:
> > > + return 1;
> > > + case V4L2_PIX_FMT_BGR24:
> > > + case V4L2_PIX_FMT_RGB24:
> > > + return 3;
> > > + case V4L2_PIX_FMT_ARGB32:
> > > + return 4;
> > > + case V4L2_PIX_FMT_VYUY:
> > > + case V4L2_PIX_FMT_YVYU:
> > > + case V4L2_PIX_FMT_UYVY:
> > > + case V4L2_PIX_FMT_YUYV:
> > > + return 2;
> > > + default:
> > > + return 0;
> > > + };
> > > +}
> > > +
> > > +int V4L2CompatManager::imageSize(unsigned int format,
> > > + unsigned int width, unsigned int height)
> > > +{
> > > + switch (format) {
> > > + case V4L2_PIX_FMT_NV12:
> > > + case V4L2_PIX_FMT_NV21:
> > > + return width * height + width * height / 2;
> > > + case V4L2_PIX_FMT_NV16:
> > > + case V4L2_PIX_FMT_NV61:
> > > + return width * height * 2;
> > > + case V4L2_PIX_FMT_NV24:
> > > + case V4L2_PIX_FMT_NV42:
> > > + return width * height * 3;
> > > + case V4L2_PIX_FMT_BGR24:
> > > + case V4L2_PIX_FMT_RGB24:
> > > + return width * height * 3;
> > > + case V4L2_PIX_FMT_ARGB32:
> > > + return width * height * 4;
> > > + case V4L2_PIX_FMT_VYUY:
> > > + case V4L2_PIX_FMT_YVYU:
> > > + case V4L2_PIX_FMT_UYVY:
> > > + case V4L2_PIX_FMT_YUYV:
> > > + return width * height * 2;
> > > + default:
> > > + return 0;
> > > + };
> > > +}
> > > +
> > > +unsigned int V4L2CompatManager::v4l2ToDrm(unsigned int pixelformat)
> > > +{
> > > + switch (pixelformat) {
> > > + /* RGB formats. */
> > > + case V4L2_PIX_FMT_RGB24:
> > > + return DRM_FORMAT_BGR888;
> > > + case V4L2_PIX_FMT_BGR24:
> > > + return DRM_FORMAT_RGB888;
> > > + case V4L2_PIX_FMT_ARGB32:
> > > + return DRM_FORMAT_BGRA8888;
> > > +
> > > + /* YUV packed formats. */
> > > + case V4L2_PIX_FMT_YUYV:
> > > + return DRM_FORMAT_YUYV;
> > > + case V4L2_PIX_FMT_YVYU:
> > > + return DRM_FORMAT_YVYU;
> > > + case V4L2_PIX_FMT_UYVY:
> > > + return DRM_FORMAT_UYVY;
> > > + case V4L2_PIX_FMT_VYUY:
> > > + return DRM_FORMAT_VYUY;
> > > +
> > > + /* YUY planar formats. */
> > > + case V4L2_PIX_FMT_NV16:
> > > + return DRM_FORMAT_NV16;
> > > + case V4L2_PIX_FMT_NV61:
> > > + return DRM_FORMAT_NV61;
> > > + case V4L2_PIX_FMT_NV12:
> > > + return DRM_FORMAT_NV12;
> > > + case V4L2_PIX_FMT_NV21:
> > > + return DRM_FORMAT_NV21;
> > > + case V4L2_PIX_FMT_NV24:
> > > + return DRM_FORMAT_NV24;
> > > + case V4L2_PIX_FMT_NV42:
> > > + return DRM_FORMAT_NV42;
> > > + default:
> > > + return pixelformat;
> > > + };
> > > +}
> > > +
> > > +unsigned int V4L2CompatManager::drmToV4L2(unsigned int pixelformat)
> > > +{
> > > + switch (pixelformat) {
> > > + /* RGB formats. */
> > > + case DRM_FORMAT_BGR888:
> > > + return V4L2_PIX_FMT_RGB24;
> > > + case DRM_FORMAT_RGB888:
> > > + return V4L2_PIX_FMT_BGR24;
> > > + case DRM_FORMAT_BGRA8888:
> > > + return V4L2_PIX_FMT_ARGB32;
> > > +
> > > + /* YUV packed formats. */
> > > + case DRM_FORMAT_YUYV:
> > > + return V4L2_PIX_FMT_YUYV;
> > > + case DRM_FORMAT_YVYU:
> > > + return V4L2_PIX_FMT_YVYU;
> > > + case DRM_FORMAT_UYVY:
> > > + return V4L2_PIX_FMT_UYVY;
> > > + case DRM_FORMAT_VYUY:
> > > + return V4L2_PIX_FMT_VYUY;
> > > +
> > > + /* YUY planar formats. */
> > > + case DRM_FORMAT_NV16:
> > > + return V4L2_PIX_FMT_NV16;
> > > + case DRM_FORMAT_NV61:
> > > + return V4L2_PIX_FMT_NV61;
> > > + case DRM_FORMAT_NV12:
> > > + return V4L2_PIX_FMT_NV12;
> > > + case DRM_FORMAT_NV21:
> > > + return V4L2_PIX_FMT_NV21;
> > > + case DRM_FORMAT_NV24:
> > > + return V4L2_PIX_FMT_NV24;
> > > + case DRM_FORMAT_NV42:
> > > + return V4L2_PIX_FMT_NV42;
> > > + default:
> > > + return pixelformat;
> > > + }
> > > +}
> > > diff --git a/src/v4l2/v4l2_compat_manager.h b/src/v4l2/v4l2_compat_manager.h
> > > new file mode 100644
> > > index 00000000..d5ae7810
> > > --- /dev/null
> > > +++ b/src/v4l2/v4l2_compat_manager.h
> > > @@ -0,0 +1,86 @@
> > > +/* SPDX-License-Identifier: GPL-2.0-or-later */
> > > +/*
> > > + * Copyright (C) 2019, Google Inc.
> > > + *
> > > + * v4l2_compat_manager.h - V4L2 compatibility manager
> > > + */
> > > +#ifndef __V4L2_COMPAT_MANAGER_H__
> > > +#define __V4L2_COMPAT_MANAGER_H__
> > > +
> > > +#include <condition_variable>
> > > +#include <linux/videodev2.h>
>
> videodev2.h isn't needed as far as I can tell.
>
> > > +#include <map>
> > > +#include <mutex>
> > > +#include <queue>
>
> I don't think you use std::queue here.
>
> > > +#include <sys/syscall.h>
>
> Is this needed here ?
>
> > > +#include <unistd.h>
>
> I don't think unistd.h is needed either, but you need fcntl.h for
> mode_t, and sys/types.h for off_t and size_t.
ack
> > > +#include <vector>
> > > +
> > > +#include <libcamera/camera.h>
> > > +#include <libcamera/camera_manager.h>
> > > +#include <libcamera/stream.h>
> > > +
> > > +#include "thread.h"
> > > +#include "v4l2_camera_proxy.h"
> > > +
> > > +using namespace libcamera;
> > > +
> > > +class V4L2CompatManager : public Thread
> > > +{
> > > +public:
> > > + static V4L2CompatManager *instance();
> > > +
> > > + int init();
> > > +
> > > + int openat(int dirfd, const char *path, int oflag, mode_t mode);
> > > +
> > > + V4L2CameraProxy *getCamera(int fd);
> > > + V4L2CameraProxy *getCamera(void *addr);
>
> I would move those two methods either before openat() or after ioctl()
> in order to keep all the implementations of the C functions you
> intercept grouped together.
ack
> > > +
> > > + int dup(int oldfd);
> > > + int close(int fd);
> > > + void *mmap(void *addr, size_t length, int prot, int flags,
> > > + int fd, off_t offset);
> > > + int munmap(void *addr, size_t length);
> > > + int ioctl(int fd, unsigned long request, void *arg);
> > > +
> > > + static int bplMultiplier(unsigned int format);
> > > + static int imageSize(unsigned int format, unsigned int width,
> > > + unsigned int height);
> > > +
> > > + static unsigned int v4l2ToDrm(unsigned int pixelformat);
> > > + static unsigned int drmToV4L2(unsigned int pixelformat);
> > > +
> > > +private:
> > > + V4L2CompatManager();
> > > + ~V4L2CompatManager();
> > > +
> > > + void run() override;
> > > + int getCameraIndex(int fd);
> > > +
> > > + typedef int (*openat_func_t)(int dirfd, const char *path, int oflag, ...);
> > > + typedef int (*dup_func_t)(int oldfd);
> > > + typedef int (*close_func_t)(int fd);
> > > + typedef int (*ioctl_func_t)(int fd, unsigned long request, ...);
> > > + typedef void *(*mmap_func_t)(void *addr, size_t length, int prot,
> > > + int flags, int fd, off_t offset);
> > > + typedef int (*munmap_func_t)(void *addr, size_t length);
> > > +
> > > + openat_func_t openat_func_;
> > > + dup_func_t dup_func_;
> > > + close_func_t close_func_;
> > > + ioctl_func_t ioctl_func_;
> > > + mmap_func_t mmap_func_;
> > > + munmap_func_t munmap_func_;
> > > +
> > > + CameraManager *cm_;
> > > +
> > > + std::mutex mutex_;
> > > + std::condition_variable cv_;
> > > +
> > > + std::vector<std::shared_ptr<V4L2CameraProxy>> proxies_;
> > > + std::map<int, std::shared_ptr<V4L2CameraProxy>> devices_;
> > > + std::map<void *, int> mmaps_;
> > > +};
> > > +
> > > +#endif /* __V4L2_COMPAT_MANAGER_H__ */
Thanks,
Paul
More information about the libcamera-devel
mailing list