[libcamera-devel] [PATCH] v4l2: Support setting frame rate in the V4L2 Adaptation layer
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Fri Feb 25 15:25:22 CET 2022
Hi Nejc,
On Fri, Feb 25, 2022 at 03:19:18PM +0100, Nejc Galof wrote:
> Hi Laurent.
>
> I something messed up with patches (I think), and it is added like a new
> patch.
It should indeed have been a single patch. If the first patch had been
merged already, a separate patch to fix issues would have been the right
thing to do, but as that's not the case, the fixes should have been
squashed. "git commit --amend" and "git rebase -i" are you friends there.
> Can you review what I do wrong, for the next update in the feature?
> Some notes about review:
> - GetParams I recommended to add in the new patch.
> - setControls is not named like that, because there is no getControls (yet).
> - All other recommendations are added and fixed.
Sure, I'll review v2.
> V V pet., 25. feb. 2022 ob 09:47 je oseba Laurent Pinchart napisala:
> > On Tue, Feb 15, 2022 at 12:42:22PM +0100, Nejc Galof wrote:
> > > The V4L2 adaptation layer can already support streaming with components
> > > such as OpenCV, however it is not accepting, or handling any requests to
> > > configure the frame rate.
> > >
> > > In V4L2 the frame rate is set by configuring the timeperframe component
> > > of the v4l2_streamparm structure through the VIDIOC_S_PARM ioctl.
> > >
> > > Extend the V4L2 compatibility layer to accept the VIDIOC_S_PARM ioctls
> > > and provide an interface for setting controls on the V4L2Camera class to
> > > set the requested rate when starting the camera.
> > >
> > > Signed-off-by: Nejc Galof <galof.nejc at gmail.com>
> > > Signed-off-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> > > ---
> > > src/v4l2/v4l2_camera.cpp | 12 +++++++++---
> > > src/v4l2/v4l2_camera.h | 7 +++++++
> > > src/v4l2/v4l2_camera_proxy.cpp | 25 +++++++++++++++++++++++++
> > > src/v4l2/v4l2_camera_proxy.h | 1 +
> > > 4 files changed, 42 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/src/v4l2/v4l2_camera.cpp b/src/v4l2/v4l2_camera.cpp
> > > index e922b9e6..e4eb3a2b 100644
> > > --- a/src/v4l2/v4l2_camera.cpp
> > > +++ b/src/v4l2/v4l2_camera.cpp
> > > @@ -12,13 +12,15 @@
> > >
> > > #include <libcamera/base/log.h>
> > >
> > > +#include <libcamera/control_ids.h>
> > > +
> > > using namespace libcamera;
> > >
> > > LOG_DECLARE_CATEGORY(V4L2Compat)
> > >
> > > V4L2Camera::V4L2Camera(std::shared_ptr<Camera> camera)
> > > - : camera_(camera), isRunning_(false), bufferAllocator_(nullptr),
> > > - efd_(-1), bufferAvailableCount_(0)
> > > + : camera_(camera), controls_(controls::controls), isRunning_(false),
> > > + bufferAllocator_(nullptr), efd_(-1), bufferAvailableCount_(0)
> > > {
> > > camera_->requestCompleted.connect(this, &V4L2Camera::requestComplete);
> > > }
> > > @@ -203,10 +205,12 @@ int V4L2Camera::streamOn()
> > > if (isRunning_)
> > > return 0;
> > >
> > > - int ret = camera_->start();
> > > + int ret = camera_->start(&controls_);
> > > if (ret < 0)
> > > return ret == -EACCES ? -EBUSY : ret;
> > >
> > > + controls_.clear();
> > > +
> > > isRunning_ = true;
> > >
> > > for (Request *req : pendingRequests_) {
> > > @@ -266,6 +270,8 @@ int V4L2Camera::qbuf(unsigned int index)
> > > return 0;
> > > }
> > >
> > > + request->controls().merge(std::move(controls_));
> > > +
> > > ret = camera_->queueRequest(request);
> > > if (ret < 0) {
> > > LOG(V4L2Compat, Error) << "Can't queue request";
> > > diff --git a/src/v4l2/v4l2_camera.h b/src/v4l2/v4l2_camera.h
> > > index 03e74118..9e6c895a 100644
> > > --- a/src/v4l2/v4l2_camera.h
> > > +++ b/src/v4l2/v4l2_camera.h
> > > @@ -10,11 +10,14 @@
> > > #include <deque>
> > > #include <utility>
> > >
> > > +#include <linux/videodev2.h>
> >
> > I don't think you need to include this here.
> >
> > > +
> > > #include <libcamera/base/mutex.h>
> > > #include <libcamera/base/semaphore.h>
> > > #include <libcamera/base/shared_fd.h>
> > >
> > > #include <libcamera/camera.h>
> > > +#include <libcamera/controls.h>
> > > #include <libcamera/framebuffer.h>
> > > #include <libcamera/framebuffer_allocator.h>
> > >
> > > @@ -49,6 +52,8 @@ public:
> > > const libcamera::Size &size,
> > > libcamera::StreamConfiguration *streamConfigOut);
> > >
> > > + libcamera::ControlList &controls() { return controls_; }
> >
> > I may have gone for a setControls() function, but this works too.
> >
> > > +
> > > int allocBuffers(unsigned int count);
> > > void freeBuffers();
> > > int getBufferFd(unsigned int index);
> > > @@ -69,6 +74,8 @@ private:
> > > std::shared_ptr<libcamera::Camera> camera_;
> > > std::unique_ptr<libcamera::CameraConfiguration> config_;
> > >
> > > + libcamera::ControlList controls_;
> > > +
> > > bool isRunning_;
> > >
> > > libcamera::Mutex bufferLock_;
> > > diff --git a/src/v4l2/v4l2_camera_proxy.cpp b/src/v4l2/v4l2_camera_proxy.cpp
> > > index e114d09f..f005b21c 100644
> > > --- a/src/v4l2/v4l2_camera_proxy.cpp
> > > +++ b/src/v4l2/v4l2_camera_proxy.cpp
> > > @@ -18,6 +18,8 @@
> > > #include <unistd.h>
> > >
> > > #include <libcamera/camera.h>
> > > +#include <libcamera/controls.h>
> > > +#include <libcamera/control_ids.h>
> > > #include <libcamera/formats.h>
> > >
> > > #include <libcamera/base/log.h>
> > > @@ -33,6 +35,7 @@
> > > #define KERNEL_VERSION(a, b, c) (((a) << 16) + ((b) << 8) + (c))
> > >
> > > using namespace libcamera;
> > > +using namespace std::literals::chrono_literals;
> > >
> > > LOG_DECLARE_CATEGORY(V4L2Compat)
> > >
> > > @@ -755,6 +758,24 @@ int V4L2CameraProxy::vidioc_streamoff(V4L2CameraFile *file, int *arg)
> > > return ret;
> > > }
> > >
> > > +int V4L2CameraProxy::vidioc_s_param(V4L2CameraFile *file, struct v4l2_streamparm *arg)
> > > +{
> > > + LOG(V4L2Compat, Debug)
> > > + << "[" << file->description() << "] " << __func__ << "()";
> > > +
> > > + if (!validateBufferType(arg->type))
> > > + return -EINVAL;
> > > +
> > > + struct v4l2_fract *timeperframe = &arg->parm.capture.timeperframe;
> > > + utils::Duration frameDuration = 1s * (static_cast<double>(timeperframe->numerator) /
> > > + static_cast<double>(timeperframe->denominator));
> >
> > Does the following work ?
> >
> > utils::Duration frameDuration = 1.0s * timeperframe->numerator
> > / timeperframe->denominator;
> >
> > > +
> > > + int64_t uDuration = frameDuration.get<std::micro>();
> > > + vcam_->controls().set(controls::FrameDurationLimits, { uDuration, uDuration });
> >
> > Looks like we should move the FrameDurationLimits control to using the
> > Duration type. Not a candidate for this patch of course.
> >
> > > +
> > > + return 0;
> > > +}
> >
> > Getting the frame rate back from the camera to implement vidioc_g_param
> > is a more difficult task, but would it make sense to cache the frame
> > rate in the V4L2CameraProxy and implement vidioc_g_param based on that
> > already ? The question of what frame rate to expose by default will be
> > an interesting one, but I think you can start with a hardcoded default
> > value of 1/30. This could be done in this patch if easy, or in a
> > separate patch.
> >
> > > +
> > > const std::set<unsigned long> V4L2CameraProxy::supportedIoctls_ = {
> > > VIDIOC_QUERYCAP,
> > > VIDIOC_ENUM_FRAMESIZES,
> > > @@ -775,6 +796,7 @@ const std::set<unsigned long> V4L2CameraProxy::supportedIoctls_ = {
> > > VIDIOC_EXPBUF,
> > > VIDIOC_STREAMON,
> > > VIDIOC_STREAMOFF,
> > > + VIDIOC_S_PARM,
> > > };
> > >
> > > int V4L2CameraProxy::ioctl(V4L2CameraFile *file, unsigned long request, void *arg)
> > > @@ -852,6 +874,9 @@ int V4L2CameraProxy::ioctl(V4L2CameraFile *file, unsigned long request, void *ar
> > > case VIDIOC_STREAMOFF:
> > > ret = vidioc_streamoff(file, static_cast<int *>(arg));
> > > break;
> > > + case VIDIOC_S_PARM:
> > > + ret = vidioc_s_param(file, static_cast<struct v4l2_streamparm *>(arg));
> >
> > vidioc_s_parm, to match the ioctl name.
> >
> > All in all it looks fairly good. With the two small changes requested
> > above,
> >
> > Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> >
> > Will you send a v2 ?
> >
> > > + break;
> > > default:
> > > ret = -ENOTTY;
> > > break;
> > > diff --git a/src/v4l2/v4l2_camera_proxy.h b/src/v4l2/v4l2_camera_proxy.h
> > > index 76ca2d8a..30a3f492 100644
> > > --- a/src/v4l2/v4l2_camera_proxy.h
> > > +++ b/src/v4l2/v4l2_camera_proxy.h
> > > @@ -65,6 +65,7 @@ private:
> > > int vidioc_expbuf(V4L2CameraFile *file, struct v4l2_exportbuffer
> > *arg);
> > > int vidioc_streamon(V4L2CameraFile *file, int *arg);
> > > int vidioc_streamoff(V4L2CameraFile *file, int *arg);
> > > + int vidioc_s_param(V4L2CameraFile *file, struct v4l2_streamparm
> > *arg);
> > >
> > > bool hasOwnership(V4L2CameraFile *file);
> > > int acquire(V4L2CameraFile *file);
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list