[libcamera-devel] [PATCH] v4l2: Support setting frame rate in the V4L2 Adaptation layer
Nejc Galof
galof.nejc at gmail.com
Fri Feb 25 15:19:18 CET 2022
Hi Laurent.
I something messed up with patches (I think), and it is added like a new
patch. 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.
Regards,
Nejc Galof
V V pet., 25. feb. 2022 ob 09:47 je oseba Laurent Pinchart <
laurent.pinchart at ideasonboard.com> napisala:
> Hi Nejc,
>
> Thank you for the patch.
>
> 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
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20220225/bde490e4/attachment-0001.htm>
More information about the libcamera-devel
mailing list