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