| Message ID | 20220215114222.115790-1-galof.nejc@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
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@gmail.com> > Signed-off-by: Kieran Bingham <kieran.bingham@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@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);
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@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@gmail.com> > > Signed-off-by: Kieran Bingham <kieran.bingham@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@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 >
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@gmail.com> > > > Signed-off-by: Kieran Bingham <kieran.bingham@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@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);
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> + #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_; } + 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)); + + int64_t uDuration = frameDuration.get<std::micro>(); + vcam_->controls().set(controls::FrameDurationLimits, { uDuration, uDuration }); + + return 0; +} + 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)); + 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);