[{"id":37260,"web_url":"https://patchwork.libcamera.org/comment/37260/","msgid":"<3u36wmmvt4np2bslrb5hjb6caouuhyjh3fbc7ysf4mmvenrhlw@nsvma7gi2c6e>","date":"2025-12-10T16:04:49","subject":"Re: [RFC PATCH v1 2/2] libcamera: pipeline: uvcvideo: Handle\n\t`FrameDurationLimits`","submitter":{"id":143,"url":"https://patchwork.libcamera.org/api/people/143/","name":"Jacopo Mondi","email":"jacopo.mondi@ideasonboard.com"},"content":"Hi Barnabás\n\nOn Wed, Dec 10, 2025 at 02:37:04PM +0100, Barnabás Pőcze wrote:\n> Setting a frame rate is possible with UVC devices using `VIDIOC_S_PARM`.\n> However, compared to other platforms supported by libcamera, limitations\n> apply. Probably most crucially, the desired frame rate cannot be set\n> while the camera is streaming. Furthermore, it is only a single value\n> and not an allowed range.\n>\n> So this change only adds support for `FrameDurationLimits` in the control\n> list passed to `Camera::start()`, and the control is otherwise ignored in\n> requests.\n>\n> The kernel interface also only allows a single number and not a range,\n> so the midpoint of the desired range is used. Checking the supplied\n\nEverything else sounds fine, but I wonder if taking the midpoint is\nthe best solution here or we should simply use the shortest provided\nduration, warn the user, and let the kernel adjust it to the closest\nsupported value.\n\n> values is not necessary since the kernel will adjust the value if\n> it is not supported by the device.\n>\n> Initially the global min/max values are advertised in the `ControlInfo`\n> of the `FrameDurationLimits` control, which are then updated after\n> the camera is configured. Updating the control limits after configuration\n> matches the behaviour of other platforms.\n>\n> While the kernel interface differentiates three types of frame intervals\n> (discrete, continuous, stepwise), when querying the available frame intervals\n> for a given (pixel format, size) combination, all options are evaluated\n> and only the \"local\" minimum and maximum is used, as that is the only way\n> the limits can reasonably be advertised on the libcamera side.\n>\n> Closes: https://gitlab.freedesktop.org/camera/libcamera/-/issues/296\n> Signed-off-by: Barnabás Pőcze \n> ---\n> include/libcamera/internal/v4l2_videodevice.h | 3 +\n> src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 59 +++++++++\n> src/libcamera/v4l2_videodevice.cpp | 116 ++++++++++++++++++\n> 3 files changed, 178 insertions(+)\n>\n> diff --git a/include/libcamera/internal/v4l2_videodevice.h b/include/libcamera/internal/v4l2_videodevice.h\n> index 82d98184ed..e97c0f9bf8 100644\n> --- a/include/libcamera/internal/v4l2_videodevice.h\n> +++ b/include/libcamera/internal/v4l2_videodevice.h\n> @@ -209,6 +209,9 @@ public:\n> \tFormats formats(uint32_t code = 0);\n>\n> \tint getFrameInterval(std::chrono::microseconds *interval);\n> +\tint setFrameInterval(std::chrono::microseconds *interval);\n> +\tstd::optional>\n> +\tgetFrameIntervalLimits(V4L2PixelFormat pixelFormat, Size size);\n>\n> \tint getSelection(unsigned int target, Rectangle *rect);\n> \tint setSelection(unsigned int target, Rectangle *rect);\n> diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> index 3f98e8ece0..e4e9b8ab9b 100644\n> --- a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> +++ b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> @@ -57,6 +57,7 @@ public:\n> \tstd::unique_ptr video_;\n> \tStream stream_;\n> \tstd::map> formats_;\n> +\tstd::map, std::array> frameIntervals_;\n>\n> \tstd::optional autoExposureMode_;\n> \tstd::optional manualExposureMode_;\n> @@ -277,6 +278,22 @@ int PipelineHandlerUVC::configure(Camera *camera, CameraConfiguration *config)\n> \t format.fourcc != data->video_->toV4L2PixelFormat(cfg.pixelFormat))\n> \t\treturn -EINVAL;\n>\n> +\tauto it = data->controlInfo_.find(&controls::FrameDurationLimits);\n> +\tif (it != data->controlInfo_.end()) {\n> +\t\tauto it2 = data->frameIntervals_.find({ cfg.pixelFormat, cfg.size });\n> +\t\tif (it2 != data->frameIntervals_.end()) {\n> +\t\t\tstd::chrono::microseconds current;\n> +\n> +\t\t\tret = data->video_->getFrameInterval(¤t);\n> +\n> +\t\t\tit->second = ControlInfo{\n> +\t\t\t\tint64_t(it2->second[0].count()),\n> +\t\t\t\tint64_t(it2->second[1].count()),\n> +\t\t\t\tret == 0 ? ControlValue(int64_t(current.count())) : ControlValue()\n> +\t\t\t};\n> +\t\t}\n> +\t}\n> +\n> \tcfg.setStream(&data->stream_);\n>\n> \treturn 0;\n> @@ -306,6 +323,19 @@ int PipelineHandlerUVC::start(Camera *camera, const ControlList *controls)\n> \t\tret = processControls(data, *controls);\n> \t\tif (ret < 0)\n> \t\t\tgoto err_release_buffers;\n> +\n> +\t\t/* Can only be set before starting. */\n> +\t\tauto fdl = controls->get(controls::FrameDurationLimits);\n> +\t\tif (fdl) {\n> +\t\t\tconst auto wantMin = std::chrono::microseconds((*fdl)[0]);\n> +\t\t\tconst auto wantMax = std::chrono::microseconds((*fdl)[1]);\n> +\t\t\tauto want = (wantMin + wantMax) / 2;\n> +\n> +\t\t\t/* Let the kernel choose something close to the middle. */\n> +\t\t\tret = data->video_->setFrameInterval(&want);\n> +\t\t\tif (ret == 0)\n> +\t\t\t\tdata->timePerFrame_ = want;\n> +\t\t}\n> \t}\n>\n> \tret = data->video_->streamOn();\n> @@ -355,6 +385,8 @@ int PipelineHandlerUVC::processControl(const UVCCameraData *data, ControlList *c\n> \t\tcid = V4L2_CID_GAMMA;\n> \telse if (id == controls::AeEnable)\n> \t\treturn 0; /* Handled in `Camera::queueRequest()`. */\n> +\telse if (id == controls::FrameDurationLimits)\n> +\t\treturn 0; /* Handled in `start()` */\n> \telse\n> \t\treturn -EINVAL;\n>\n> @@ -555,6 +587,23 @@ int UVCCameraData::init(std::shared_ptr media)\n> \t * resolution from the largest size it advertises.\n> \t */\n> \tSize resolution;\n> +\tauto minFrameInterval = std::chrono::microseconds::max();\n> +\tauto maxFrameInterval = std::chrono::microseconds::min();\n> +\n> +\tconst auto processFrameIntervals = [&](PixelFormat pf, V4L2PixelFormat v4l2pf, SizeRange size) {\n> +\t\tif (size.min != size.max)\n> +\t\t\treturn;\n\nCan this happen ?\n\n> +\n> +\t\tauto frameIntervals = video_->getFrameIntervalLimits(v4l2pf, size.min);\n> +\t\tif (!frameIntervals)\n> +\t\t\treturn;\n> +\n> +\t\tminFrameInterval = std::min(minFrameInterval, (*frameIntervals)[0]);\n> +\t\tmaxFrameInterval = std::max(maxFrameInterval, (*frameIntervals)[1]);\n\nAren't you already taking the min/max in the video device helpers ?\n\n> +\n> +\t\tframeIntervals_.try_emplace({ pf, size.min }, *frameIntervals);\n> +\t};\n> +\n> \tfor (const auto &format : video_->formats()) {\n> \t\tPixelFormat pixelFormat = format.first.toPixelFormat();\n> \t\tif (!pixelFormat.isValid())\n> @@ -566,6 +615,8 @@ int UVCCameraData::init(std::shared_ptr media)\n> \t\tfor (const SizeRange &sizeRange : sizeRanges) {\n> \t\t\tif (sizeRange.max > resolution)\n> \t\t\t\tresolution = sizeRange.max;\n> +\n> +\t\t\tprocessFrameIntervals(pixelFormat, format.first, sizeRange);\n> \t\t}\n> \t}\n>\n> @@ -625,6 +676,14 @@ int UVCCameraData::init(std::shared_ptr media)\n> \t\tctrls[&controls::AeEnable] = ControlInfo(false, true, true);\n> \t}\n>\n> +\t/* Use the global min/max here, limits will be updated in `configure()`. */\n> +\tif (!frameIntervals_.empty()) {\n> +\t\tctrls[&controls::FrameDurationLimits] = ControlInfo{\n> +\t\t\tint64_t(minFrameInterval.count()),\n> +\t\t\tint64_t(maxFrameInterval.count()),\n> +\t\t};\n> +\t}\n> +\n> \tcontrolInfo_ = ControlInfoMap(std::move(ctrls), controls::controls);\n>\n> \t/*\n> diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp\n> index 3836dabef3..3db6e80aed 100644\n> --- a/src/libcamera/v4l2_videodevice.cpp\n> +++ b/src/libcamera/v4l2_videodevice.cpp\n> @@ -1202,6 +1202,122 @@ int V4L2VideoDevice::getFrameInterval(std::chrono::microseconds *interval)\n> \treturn 0;\n> }\n>\n> +/**\n> + * \\brief Configure the frame interval\n> + * \\param[inout] interval The frame interval\n\nunit would be nice here as well\n\n> + *\n> + * Apply the supplied \\a interval as the time-per-frame stream parameter\n> + * on the device, and return the actually applied value.\n> + *\n> + * \\return 0 on success or a negative error code otherwise\n> + */\n> +int V4L2VideoDevice::setFrameInterval(std::chrono::microseconds *interval)\n> +{\n> +\tv4l2_fract *frameInterval = nullptr;\n> +\tuint32_t *caps = nullptr;\n> +\tv4l2_streamparm sparm = {};\n> +\n> +\tsparm.type = bufferType_;\n> +\n> +\tswitch (sparm.type) {\n> +\tcase V4L2_BUF_TYPE_VIDEO_CAPTURE:\n> +\tcase V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE:\n> +\t\tframeInterval = &sparm.parm.capture.timeperframe;\n> +\t\tcaps = &sparm.parm.capture.capability;\n> +\t\tbreak;\n> +\tcase V4L2_BUF_TYPE_VIDEO_OUTPUT:\n> +\tcase V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE:\n> +\t\tframeInterval = &sparm.parm.output.timeperframe;\n> +\t\tcaps = &sparm.parm.output.capability;\n> +\t\tbreak;\n> +\t}\n> +\n> +\tif (!frameInterval)\n> +\t\treturn -EINVAL;\n> +\n> +\tconstexpr auto max = std::numeric_limitsnumerator)>::max();\n\nFeels quite a corner case, but checking doesn't hurt ?\n\n> +\tif (interval->count() <= 0 || interval->count() > max)\n> +\t\treturn -EINVAL;\n> +\n> +\tframeInterval->numerator = interval->count();\n> +\tframeInterval->denominator = std::chrono::microseconds(std::chrono::seconds(1)).count();\n> +\n> +\tint ret = ioctl(VIDIOC_S_PARM, &sparm);\n> +\tif (ret)\n> +\t\treturn ret;\n> +\n> +\tif (!(*caps & V4L2_CAP_TIMEPERFRAME))\n> +\t\treturn -ENOTSUP;\n\nIn this case, I' pretty sure you should check this flag before calling\nioctl()\n\n> +\n> +\t*interval = v4l2FractionToMs(*frameInterval);\n> +\n> +\treturn 0;\n> +}\n> +\n> +/**\n> + * \\brief Retrieve the frame interval limits\n> + * \\param[in] pixelFormat The pixel format\n> + * \\param[in] size The size\n> + *\n> + * Retrieve the minimum and maximum available frame interval for\n> + * the given \\a pixelFormat and \\a size.\n> + *\n> + * \\return The min and max frame interval or std::nullopt otherwise\n> + */\n> +std::optional>\n> +V4L2VideoDevice::getFrameIntervalLimits(V4L2PixelFormat pixelFormat, Size size)\n> +{\n> +\tauto min = std::chrono::microseconds::max();\n> +\tauto max = std::chrono::microseconds::min();\n> +\tunsigned int index = 0;\n> +\tint ret;\n> +\n> +\tfor (;; index++) {\n> +\t\tstruct v4l2_frmivalenum frameInterval = {};\n> +\t\tframeInterval.index = index;\n> +\t\tframeInterval.pixel_format = pixelFormat;\n> +\t\tframeInterval.width = size.width;\n> +\t\tframeInterval.height = size.height;\n> +\n> +\t\tret = ioctl(VIDIOC_ENUM_FRAMEINTERVALS, &frameInterval);\n> +\t\tif (ret)\n> +\t\t\tbreak;\n> +\n> +\t\tswitch (frameInterval.type) {\n> +\t\tcase V4L2_FRMIVAL_TYPE_DISCRETE: {\n> +\t\t\tauto ms = v4l2FractionToMs(frameInterval.discrete);\n> +\n> +\t\t\tmin = std::min(min, ms);\n> +\t\t\tmax = std::max(max, ms);\n> +\t\t\tbreak;\n> +\t\t}\n> +\t\tcase V4L2_FRMIVAL_TYPE_CONTINUOUS:\n> +\t\tcase V4L2_FRMIVAL_TYPE_STEPWISE: {\n> +\t\t\tmin = std::min(min, v4l2FractionToMs(frameInterval.stepwise.min));\n> +\t\t\tmax = std::max(max, v4l2FractionToMs(frameInterval.stepwise.max));\n> +\t\t\tbreak;\n> +\t\t}\n\nYou know, I'm still not sure if we should support anything != DISCRETE.\n\nIf I'm not mistaken UVC will always be DISCRETE. Every other device\nthat uses this function is likely something that we don't want to\nsupport (because we want the drivers not to use this API for complex\ndevices).\n\nThe same could be said for VIDEO_OUTPUT/OUTPUT_MPLANE, this will only\napply to codecs ?\n\n> +\t\tdefault:\n> +\t\t\tLOG(V4L2, Error)\n> +\t\t\t\t<< \"Unknown v4l2_frmsizetypes value \"\n> +\t\t\t\t<< frameInterval.type;\n> +\t\t\treturn {};\n> +\t\t}\n> +\t}\n> +\n> +\tif (ret && ret != -EINVAL) {\n\nI might have missed why -EINVAL is ok\n\n> +\t\tLOG(V4L2, Error)\n> +\t\t\t<< \"Unable to enumerate pixel formats: \"\n> +\t\t\t<< strerror(-ret);\n> +\t\treturn {};\n> +\t}\n> +\n> +\tif (index <= 0)\n\nHow can index be < 0 ?\n\n> +\t\treturn {};\n> +\n> +\treturn {{ min, max }};\n> +}\n> +\n> std::vector V4L2VideoDevice::enumPixelformats(uint32_t code)\n> {\n> \tstd::vector formats;\n> --\n> 2.52.0\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 12959BD1F1\n\tfor ;\n\tWed, 10 Dec 2025 16:05:00 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 4F20361491;\n\tWed, 10 Dec 2025 17:04:59 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 04A8961480\n\tfor ;\n\tWed, 10 Dec 2025 17:04:58 +0100 (CET)","from ideasonboard.com (mob-5-90-55-146.net.vodafone.it\n\t[5.90.55.146])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id B2EF5ED;\n\tWed, 10 Dec 2025 17:04:55 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"JyiY1yzD\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1765382695;\n\tbh=L8Vev63OT/go2lSrfcNiXi66FRR3otvPFY4cl33SYAc=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=JyiY1yzD1Q0krrhPIeXdyZydN65gob5dWLnLdDVktfgJiaiR70sNKPqxcPdcaqQ5V\n\tR1+kjJll6O8wgCa2xnpELjB1Oe9QPRbRUZFggsSjSheBZZs0VpIzbLFpAzIbG0bdUa\n\tiEF6WTEnpH1wLS/qFyD6jcYDwqiijus0991/j9y0=","Date":"Wed, 10 Dec 2025 17:04:49 +0100","From":"Jacopo Mondi ","To":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","Cc":"libcamera-devel@lists.libcamera.org","Subject":"Re: [RFC PATCH v1 2/2] libcamera: pipeline: uvcvideo: Handle\n\t`FrameDurationLimits`","Message-ID":"<3u36wmmvt4np2bslrb5hjb6caouuhyjh3fbc7ysf4mmvenrhlw@nsvma7gi2c6e>","References":"<20251210133704.2711629-1-barnabas.pocze@ideasonboard.com>\n\t<20251210133704.2711629-3-barnabas.pocze@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20251210133704.2711629-3-barnabas.pocze@ideasonboard.com>","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":37269,"web_url":"https://patchwork.libcamera.org/comment/37269/","msgid":"<58222843-07aa-45a8-9740-bd481b76c1e8@ideasonboard.com>","date":"2025-12-10T16:56:01","subject":"Re: [RFC PATCH v1 2/2] libcamera: pipeline: uvcvideo: Handle\n\t`FrameDurationLimits`","submitter":{"id":216,"url":"https://patchwork.libcamera.org/api/people/216/","name":"Barnabás Pőcze","email":"barnabas.pocze@ideasonboard.com"},"content":"Hi\n\n2025. 12. 10. 17:04 keltezéssel, Jacopo Mondi írta:\n> Hi Barnabás\n> \n> On Wed, Dec 10, 2025 at 02:37:04PM +0100, Barnabás Pőcze wrote:\n>> Setting a frame rate is possible with UVC devices using `VIDIOC_S_PARM`.\n>> However, compared to other platforms supported by libcamera, limitations\n>> apply. Probably most crucially, the desired frame rate cannot be set\n>> while the camera is streaming. Furthermore, it is only a single value\n>> and not an allowed range.\n>>\n>> So this change only adds support for `FrameDurationLimits` in the control\n>> list passed to `Camera::start()`, and the control is otherwise ignored in\n>> requests.\n>>\n>> The kernel interface also only allows a single number and not a range,\n>> so the midpoint of the desired range is used. Checking the supplied\n> \n> Everything else sounds fine, but I wonder if taking the midpoint is\n> the best solution here or we should simply use the shortest provided\n> duration, warn the user, and let the kernel adjust it to the closest\n> supported value.\n\nAs far as I understand, the kernel selects the closest valid frame duration\nvalue. Which means that using the midpoint should guarantee that if the device\nsupports at least one frame duration value in the desired range, then a value\nin the desired range will be selected on the device. This is not guaranteed if\neither the min or max is sent to the kernel. That's why I chose this behaviour;\nI believe it's probably best to select a frame duration from the desired range,\nif possible.\n\n\n> \n>> values is not necessary since the kernel will adjust the value if\n>> it is not supported by the device.\n>>\n>> Initially the global min/max values are advertised in the `ControlInfo`\n>> of the `FrameDurationLimits` control, which are then updated after\n>> the camera is configured. Updating the control limits after configuration\n>> matches the behaviour of other platforms.\n>>\n>> While the kernel interface differentiates three types of frame intervals\n>> (discrete, continuous, stepwise), when querying the available frame intervals\n>> for a given (pixel format, size) combination, all options are evaluated\n>> and only the \"local\" minimum and maximum is used, as that is the only way\n>> the limits can reasonably be advertised on the libcamera side.\n>>\n>> Closes: https://gitlab.freedesktop.org/camera/libcamera/-/issues/296\n>> Signed-off-by: Barnabás Pőcze \n>> ---\n>> include/libcamera/internal/v4l2_videodevice.h | 3 +\n>> src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 59 +++++++++\n>> src/libcamera/v4l2_videodevice.cpp | 116 ++++++++++++++++++\n>> 3 files changed, 178 insertions(+)\n>>\n>> diff --git a/include/libcamera/internal/v4l2_videodevice.h b/include/libcamera/internal/v4l2_videodevice.h\n>> index 82d98184ed..e97c0f9bf8 100644\n>> --- a/include/libcamera/internal/v4l2_videodevice.h\n>> +++ b/include/libcamera/internal/v4l2_videodevice.h\n>> @@ -209,6 +209,9 @@ public:\n>> \tFormats formats(uint32_t code = 0);\n>>\n>> \tint getFrameInterval(std::chrono::microseconds *interval);\n>> +\tint setFrameInterval(std::chrono::microseconds *interval);\n>> +\tstd::optional>\n>> +\tgetFrameIntervalLimits(V4L2PixelFormat pixelFormat, Size size);\n>>\n>> \tint getSelection(unsigned int target, Rectangle *rect);\n>> \tint setSelection(unsigned int target, Rectangle *rect);\n>> diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n>> index 3f98e8ece0..e4e9b8ab9b 100644\n>> --- a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n>> +++ b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n>> @@ -57,6 +57,7 @@ public:\n>> \tstd::unique_ptr video_;\n>> \tStream stream_;\n>> \tstd::map> formats_;\n>> +\tstd::map, std::array> frameIntervals_;\n>>\n>> \tstd::optional autoExposureMode_;\n>> \tstd::optional manualExposureMode_;\n>> @@ -277,6 +278,22 @@ int PipelineHandlerUVC::configure(Camera *camera, CameraConfiguration *config)\n>> \t format.fourcc != data->video_->toV4L2PixelFormat(cfg.pixelFormat))\n>> \t\treturn -EINVAL;\n>>\n>> +\tauto it = data->controlInfo_.find(&controls::FrameDurationLimits);\n>> +\tif (it != data->controlInfo_.end()) {\n>> +\t\tauto it2 = data->frameIntervals_.find({ cfg.pixelFormat, cfg.size });\n>> +\t\tif (it2 != data->frameIntervals_.end()) {\n>> +\t\t\tstd::chrono::microseconds current;\n>> +\n>> +\t\t\tret = data->video_->getFrameInterval(¤t);\n>> +\n>> +\t\t\tit->second = ControlInfo{\n>> +\t\t\t\tint64_t(it2->second[0].count()),\n>> +\t\t\t\tint64_t(it2->second[1].count()),\n>> +\t\t\t\tret == 0 ? ControlValue(int64_t(current.count())) : ControlValue()\n>> +\t\t\t};\n>> +\t\t}\n>> +\t}\n>> +\n>> \tcfg.setStream(&data->stream_);\n>>\n>> \treturn 0;\n>> @@ -306,6 +323,19 @@ int PipelineHandlerUVC::start(Camera *camera, const ControlList *controls)\n>> \t\tret = processControls(data, *controls);\n>> \t\tif (ret < 0)\n>> \t\t\tgoto err_release_buffers;\n>> +\n>> +\t\t/* Can only be set before starting. */\n>> +\t\tauto fdl = controls->get(controls::FrameDurationLimits);\n>> +\t\tif (fdl) {\n>> +\t\t\tconst auto wantMin = std::chrono::microseconds((*fdl)[0]);\n>> +\t\t\tconst auto wantMax = std::chrono::microseconds((*fdl)[1]);\n>> +\t\t\tauto want = (wantMin + wantMax) / 2;\n>> +\n>> +\t\t\t/* Let the kernel choose something close to the middle. */\n>> +\t\t\tret = data->video_->setFrameInterval(&want);\n>> +\t\t\tif (ret == 0)\n>> +\t\t\t\tdata->timePerFrame_ = want;\n>> +\t\t}\n>> \t}\n>>\n>> \tret = data->video_->streamOn();\n>> @@ -355,6 +385,8 @@ int PipelineHandlerUVC::processControl(const UVCCameraData *data, ControlList *c\n>> \t\tcid = V4L2_CID_GAMMA;\n>> \telse if (id == controls::AeEnable)\n>> \t\treturn 0; /* Handled in `Camera::queueRequest()`. */\n>> +\telse if (id == controls::FrameDurationLimits)\n>> +\t\treturn 0; /* Handled in `start()` */\n>> \telse\n>> \t\treturn -EINVAL;\n>>\n>> @@ -555,6 +587,23 @@ int UVCCameraData::init(std::shared_ptr media)\n>> \t * resolution from the largest size it advertises.\n>> \t */\n>> \tSize resolution;\n>> +\tauto minFrameInterval = std::chrono::microseconds::max();\n>> +\tauto maxFrameInterval = std::chrono::microseconds::min();\n>> +\n>> +\tconst auto processFrameIntervals = [&](PixelFormat pf, V4L2PixelFormat v4l2pf, SizeRange size) {\n>> +\t\tif (size.min != size.max)\n>> +\t\t\treturn;\n> \n> Can this happen ?\n\nI suppose not, I will replace it with an assertion.\n\n\n> \n>> +\n>> +\t\tauto frameIntervals = video_->getFrameIntervalLimits(v4l2pf, size.min);\n>> +\t\tif (!frameIntervals)\n>> +\t\t\treturn;\n>> +\n>> +\t\tminFrameInterval = std::min(minFrameInterval, (*frameIntervals)[0]);\n>> +\t\tmaxFrameInterval = std::max(maxFrameInterval, (*frameIntervals)[1]);\n> \n> Aren't you already taking the min/max in the video device helpers ?\n\nThis is the min/max among all pixel formats and sizes, to provide initial\nmin/max values in the `ControlInfo`. See below ...\n\n\n> \n>> +\n>> +\t\tframeIntervals_.try_emplace({ pf, size.min }, *frameIntervals);\n>> +\t};\n>> +\n>> \tfor (const auto &format : video_->formats()) {\n>> \t\tPixelFormat pixelFormat = format.first.toPixelFormat();\n>> \t\tif (!pixelFormat.isValid())\n>> @@ -566,6 +615,8 @@ int UVCCameraData::init(std::shared_ptr media)\n>> \t\tfor (const SizeRange &sizeRange : sizeRanges) {\n>> \t\t\tif (sizeRange.max > resolution)\n>> \t\t\t\tresolution = sizeRange.max;\n>> +\n>> +\t\t\tprocessFrameIntervals(pixelFormat, format.first, sizeRange);\n>> \t\t}\n>> \t}\n>>\n>> @@ -625,6 +676,14 @@ int UVCCameraData::init(std::shared_ptr media)\n>> \t\tctrls[&controls::AeEnable] = ControlInfo(false, true, true);\n>> \t}\n>>\n>> +\t/* Use the global min/max here, limits will be updated in `configure()`. */\n>> +\tif (!frameIntervals_.empty()) {\n>> +\t\tctrls[&controls::FrameDurationLimits] = ControlInfo{\n>> +\t\t\tint64_t(minFrameInterval.count()),\n>> +\t\t\tint64_t(maxFrameInterval.count()),\n>> +\t\t};\n>> +\t}\n\n... here the \"global\" limits are reported initially.\n\n\n>> +\n>> \tcontrolInfo_ = ControlInfoMap(std::move(ctrls), controls::controls);\n>>\n>> \t/*\n>> diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp\n>> index 3836dabef3..3db6e80aed 100644\n>> --- a/src/libcamera/v4l2_videodevice.cpp\n>> +++ b/src/libcamera/v4l2_videodevice.cpp\n>> @@ -1202,6 +1202,122 @@ int V4L2VideoDevice::getFrameInterval(std::chrono::microseconds *interval)\n>> \treturn 0;\n>> }\n>>\n>> +/**\n>> + * \\brief Configure the frame interval\n>> + * \\param[inout] interval The frame interval\n> \n> unit would be nice here as well\n\nOk.\n\n> \n>> + *\n>> + * Apply the supplied \\a interval as the time-per-frame stream parameter\n>> + * on the device, and return the actually applied value.\n>> + *\n>> + * \\return 0 on success or a negative error code otherwise\n>> + */\n>> +int V4L2VideoDevice::setFrameInterval(std::chrono::microseconds *interval)\n>> +{\n>> +\tv4l2_fract *frameInterval = nullptr;\n>> +\tuint32_t *caps = nullptr;\n>> +\tv4l2_streamparm sparm = {};\n>> +\n>> +\tsparm.type = bufferType_;\n>> +\n>> +\tswitch (sparm.type) {\n>> +\tcase V4L2_BUF_TYPE_VIDEO_CAPTURE:\n>> +\tcase V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE:\n>> +\t\tframeInterval = &sparm.parm.capture.timeperframe;\n>> +\t\tcaps = &sparm.parm.capture.capability;\n>> +\t\tbreak;\n>> +\tcase V4L2_BUF_TYPE_VIDEO_OUTPUT:\n>> +\tcase V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE:\n>> +\t\tframeInterval = &sparm.parm.output.timeperframe;\n>> +\t\tcaps = &sparm.parm.output.capability;\n>> +\t\tbreak;\n>> +\t}\n>> +\n>> +\tif (!frameInterval)\n>> +\t\treturn -EINVAL;\n>> +\n>> +\tconstexpr auto max = std::numeric_limitsnumerator)>::max();\n> \n> Feels quite a corner case, but checking doesn't hurt ?\n> \n>> +\tif (interval->count() <= 0 || interval->count() > max)\n>> +\t\treturn -EINVAL;\n>> +\n>> +\tframeInterval->numerator = interval->count();\n>> +\tframeInterval->denominator = std::chrono::microseconds(std::chrono::seconds(1)).count();\n>> +\n>> +\tint ret = ioctl(VIDIOC_S_PARM, &sparm);\n>> +\tif (ret)\n>> +\t\treturn ret;\n>> +\n>> +\tif (!(*caps & V4L2_CAP_TIMEPERFRAME))\n>> +\t\treturn -ENOTSUP;\n> \n> In this case, I' pretty sure you should check this flag before calling\n> ioctl()\n\nI think it's fine. If `V4L2_CAP_TIMEPERFRAME` is not present, then nothing\n(should) changes on the device and 0 is returned, in which case this condition\ndetects that; or the driver returns an error code, which is detected above.\n\n\n> \n>> +\n>> +\t*interval = v4l2FractionToMs(*frameInterval);\n>> +\n>> +\treturn 0;\n>> +}\n>> +\n>> +/**\n>> + * \\brief Retrieve the frame interval limits\n>> + * \\param[in] pixelFormat The pixel format\n>> + * \\param[in] size The size\n>> + *\n>> + * Retrieve the minimum and maximum available frame interval for\n>> + * the given \\a pixelFormat and \\a size.\n>> + *\n>> + * \\return The min and max frame interval or std::nullopt otherwise\n>> + */\n>> +std::optional>\n>> +V4L2VideoDevice::getFrameIntervalLimits(V4L2PixelFormat pixelFormat, Size size)\n>> +{\n>> +\tauto min = std::chrono::microseconds::max();\n>> +\tauto max = std::chrono::microseconds::min();\n>> +\tunsigned int index = 0;\n>> +\tint ret;\n>> +\n>> +\tfor (;; index++) {\n>> +\t\tstruct v4l2_frmivalenum frameInterval = {};\n>> +\t\tframeInterval.index = index;\n>> +\t\tframeInterval.pixel_format = pixelFormat;\n>> +\t\tframeInterval.width = size.width;\n>> +\t\tframeInterval.height = size.height;\n>> +\n>> +\t\tret = ioctl(VIDIOC_ENUM_FRAMEINTERVALS, &frameInterval);\n>> +\t\tif (ret)\n>> +\t\t\tbreak;\n>> +\n>> +\t\tswitch (frameInterval.type) {\n>> +\t\tcase V4L2_FRMIVAL_TYPE_DISCRETE: {\n>> +\t\t\tauto ms = v4l2FractionToMs(frameInterval.discrete);\n>> +\n>> +\t\t\tmin = std::min(min, ms);\n>> +\t\t\tmax = std::max(max, ms);\n>> +\t\t\tbreak;\n>> +\t\t}\n>> +\t\tcase V4L2_FRMIVAL_TYPE_CONTINUOUS:\n>> +\t\tcase V4L2_FRMIVAL_TYPE_STEPWISE: {\n>> +\t\t\tmin = std::min(min, v4l2FractionToMs(frameInterval.stepwise.min));\n>> +\t\t\tmax = std::max(max, v4l2FractionToMs(frameInterval.stepwise.max));\n>> +\t\t\tbreak;\n>> +\t\t}\n> \n> You know, I'm still not sure if we should support anything != DISCRETE.\n> \n> If I'm not mistaken UVC will always be DISCRETE. Every other device\n> that uses this function is likely something that we don't want to\n> support (because we want the drivers not to use this API for complex\n> devices).\n\nUVC can have stepwise: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/media/usb/uvc/uvc_v4l2.c?id=0048fbb4011ec55c32d3148b2cda56433f273375#n1001\n\n\n> \n> The same could be said for VIDEO_OUTPUT/OUTPUT_MPLANE, this will only\n> apply to codecs ?\n\nThat's probably true, but since it was easy to handle those cases, I saw\nno reason to intentionally omit them.\n\n\n> \n>> +\t\tdefault:\n>> +\t\t\tLOG(V4L2, Error)\n>> +\t\t\t\t<< \"Unknown v4l2_frmsizetypes value \"\n>> +\t\t\t\t<< frameInterval.type;\n>> +\t\t\treturn {};\n>> +\t\t}\n>> +\t}\n>> +\n>> +\tif (ret && ret != -EINVAL) {\n> \n> I might have missed why -EINVAL is ok\n\nWhen the index is one more than the index of the last available frame duration,\nthen EINVAL is returned, which breaks the loop, but it is not an error condition.\n\n\n> \n>> +\t\tLOG(V4L2, Error)\n>> +\t\t\t<< \"Unable to enumerate pixel formats: \"\n>> +\t\t\t<< strerror(-ret);\n>> +\t\treturn {};\n>> +\t}\n>> +\n>> +\tif (index <= 0)\n> \n> How can index be < 0 ?\n\nAdmittedly it can't... I just like to do `<= 0` regardless of type.\n\n\n> \n>> +\t\treturn {};\n>> +\n>> +\treturn {{ min, max }};\n>> +}\n>> +\n>> std::vector V4L2VideoDevice::enumPixelformats(uint32_t code)\n>> {\n>> \tstd::vector formats;\n>> --\n>> 2.52.0\n>>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 770B0C3257\n\tfor ;\n\tWed, 10 Dec 2025 16:56:07 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 2E59B614A6;\n\tWed, 10 Dec 2025 17:56:07 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id B9FA2614A6\n\tfor ;\n\tWed, 10 Dec 2025 17:56:05 +0100 (CET)","from [192.168.33.37] (185.221.143.114.nat.pool.zt.hu\n\t[185.221.143.114])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 93C081864;\n\tWed, 10 Dec 2025 17:56:03 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"kXTDA/F7\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1765385764;\n\tbh=CWG8xJVjUiK5F86ESE8PQLUA1effIQ5WBUBKETgoy3g=;\n\th=Date:Subject:To:Cc:References:From:In-Reply-To:From;\n\tb=kXTDA/F77R8O8SaCcLbBEnNUc6mMHcs3wVlwAkEPI6HQ9WovJf65Y4asLkYB+0VO7\n\tmgqBxi49kov+xSDk8rGSRM+0+Djr29BA/vNRSm4yXtaCUE738WIkSba9XwniuxM4vB\n\tV6kX/kQ8pOY7RkOIIlm7vt4fxr4lE9YjnNMjH5H4=","Message-ID":"<58222843-07aa-45a8-9740-bd481b76c1e8@ideasonboard.com>","Date":"Wed, 10 Dec 2025 17:56:01 +0100","MIME-Version":"1.0","User-Agent":"Mozilla Thunderbird","Subject":"Re: [RFC PATCH v1 2/2] libcamera: pipeline: uvcvideo: Handle\n\t`FrameDurationLimits`","To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","References":"<20251210133704.2711629-1-barnabas.pocze@ideasonboard.com>\n\t<20251210133704.2711629-3-barnabas.pocze@ideasonboard.com>\n\t<3u36wmmvt4np2bslrb5hjb6caouuhyjh3fbc7ysf4mmvenrhlw@nsvma7gi2c6e>","From":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","Content-Language":"en-US, hu-HU","In-Reply-To":"<3u36wmmvt4np2bslrb5hjb6caouuhyjh3fbc7ysf4mmvenrhlw@nsvma7gi2c6e>","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"8bit","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":37292,"web_url":"https://patchwork.libcamera.org/comment/37292/","msgid":"","date":"2025-12-11T09:15:01","subject":"Re: [RFC PATCH v1 2/2] libcamera: pipeline: uvcvideo: Handle\n\t`FrameDurationLimits`","submitter":{"id":143,"url":"https://patchwork.libcamera.org/api/people/143/","name":"Jacopo Mondi","email":"jacopo.mondi@ideasonboard.com"},"content":"Hi Barnabás\n\nOn Wed, Dec 10, 2025 at 05:56:01PM +0100, Barnabás Pőcze wrote:\n> Hi\n>\n> 2025. 12. 10. 17:04 keltezéssel, Jacopo Mondi írta:\n> > Hi Barnabás\n> >\n> > On Wed, Dec 10, 2025 at 02:37:04PM +0100, Barnabás Pőcze wrote:\n> > > Setting a frame rate is possible with UVC devices using `VIDIOC_S_PARM`.\n> > > However, compared to other platforms supported by libcamera, limitations\n> > > apply. Probably most crucially, the desired frame rate cannot be set\n> > > while the camera is streaming. Furthermore, it is only a single value\n> > > and not an allowed range.\n> > >\n> > > So this change only adds support for `FrameDurationLimits` in the control\n> > > list passed to `Camera::start()`, and the control is otherwise ignored in\n> > > requests.\n> > >\n> > > The kernel interface also only allows a single number and not a range,\n> > > so the midpoint of the desired range is used. Checking the supplied\n> >\n> > Everything else sounds fine, but I wonder if taking the midpoint is\n> > the best solution here or we should simply use the shortest provided\n> > duration, warn the user, and let the kernel adjust it to the closest\n> > supported value.\n>\n> As far as I understand, the kernel selects the closest valid frame duration\n> value. Which means that using the midpoint should guarantee that if the device\n> supports at least one frame duration value in the desired range, then a value\n> in the desired range will be selected on the device. This is not guaranteed if\n> either the min or max is sent to the kernel. That's why I chose this behaviour;\n> I believe it's probably best to select a frame duration from the desired range,\n> if possible.\n\nWell, you have a cache of those now, don't you ?\n\n>\n>\n> >\n> > > values is not necessary since the kernel will adjust the value if\n> > > it is not supported by the device.\n> > >\n> > > Initially the global min/max values are advertised in the `ControlInfo`\n> > > of the `FrameDurationLimits` control, which are then updated after\n> > > the camera is configured. Updating the control limits after configuration\n> > > matches the behaviour of other platforms.\n> > >\n> > > While the kernel interface differentiates three types of frame intervals\n> > > (discrete, continuous, stepwise), when querying the available frame intervals\n> > > for a given (pixel format, size) combination, all options are evaluated\n> > > and only the \"local\" minimum and maximum is used, as that is the only way\n> > > the limits can reasonably be advertised on the libcamera side.\n> > >\n> > > Closes: https://gitlab.freedesktop.org/camera/libcamera/-/issues/296\n> > > Signed-off-by: Barnabás Pőcze \n> > > ---\n> > > include/libcamera/internal/v4l2_videodevice.h | 3 +\n> > > src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 59 +++++++++\n> > > src/libcamera/v4l2_videodevice.cpp | 116 ++++++++++++++++++\n> > > 3 files changed, 178 insertions(+)\n> > >\n> > > diff --git a/include/libcamera/internal/v4l2_videodevice.h b/include/libcamera/internal/v4l2_videodevice.h\n> > > index 82d98184ed..e97c0f9bf8 100644\n> > > --- a/include/libcamera/internal/v4l2_videodevice.h\n> > > +++ b/include/libcamera/internal/v4l2_videodevice.h\n> > > @@ -209,6 +209,9 @@ public:\n> > > \tFormats formats(uint32_t code = 0);\n> > >\n> > > \tint getFrameInterval(std::chrono::microseconds *interval);\n> > > +\tint setFrameInterval(std::chrono::microseconds *interval);\n> > > +\tstd::optional>\n> > > +\tgetFrameIntervalLimits(V4L2PixelFormat pixelFormat, Size size);\n> > >\n> > > \tint getSelection(unsigned int target, Rectangle *rect);\n> > > \tint setSelection(unsigned int target, Rectangle *rect);\n> > > diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> > > index 3f98e8ece0..e4e9b8ab9b 100644\n> > > --- a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> > > +++ b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> > > @@ -57,6 +57,7 @@ public:\n> > > \tstd::unique_ptr video_;\n> > > \tStream stream_;\n> > > \tstd::map> formats_;\n> > > +\tstd::map, std::array> frameIntervals_;\n> > >\n> > > \tstd::optional autoExposureMode_;\n> > > \tstd::optional manualExposureMode_;\n> > > @@ -277,6 +278,22 @@ int PipelineHandlerUVC::configure(Camera *camera, CameraConfiguration *config)\n> > > \t format.fourcc != data->video_->toV4L2PixelFormat(cfg.pixelFormat))\n> > > \t\treturn -EINVAL;\n> > >\n> > > +\tauto it = data->controlInfo_.find(&controls::FrameDurationLimits);\n> > > +\tif (it != data->controlInfo_.end()) {\n> > > +\t\tauto it2 = data->frameIntervals_.find({ cfg.pixelFormat, cfg.size });\n> > > +\t\tif (it2 != data->frameIntervals_.end()) {\n> > > +\t\t\tstd::chrono::microseconds current;\n> > > +\n> > > +\t\t\tret = data->video_->getFrameInterval(¤t);\n> > > +\n> > > +\t\t\tit->second = ControlInfo{\n> > > +\t\t\t\tint64_t(it2->second[0].count()),\n> > > +\t\t\t\tint64_t(it2->second[1].count()),\n> > > +\t\t\t\tret == 0 ? ControlValue(int64_t(current.count())) : ControlValue()\n> > > +\t\t\t};\n> > > +\t\t}\n> > > +\t}\n> > > +\n> > > \tcfg.setStream(&data->stream_);\n> > >\n> > > \treturn 0;\n> > > @@ -306,6 +323,19 @@ int PipelineHandlerUVC::start(Camera *camera, const ControlList *controls)\n> > > \t\tret = processControls(data, *controls);\n> > > \t\tif (ret < 0)\n> > > \t\t\tgoto err_release_buffers;\n> > > +\n> > > +\t\t/* Can only be set before starting. */\n> > > +\t\tauto fdl = controls->get(controls::FrameDurationLimits);\n> > > +\t\tif (fdl) {\n> > > +\t\t\tconst auto wantMin = std::chrono::microseconds((*fdl)[0]);\n> > > +\t\t\tconst auto wantMax = std::chrono::microseconds((*fdl)[1]);\n> > > +\t\t\tauto want = (wantMin + wantMax) / 2;\n> > > +\n> > > +\t\t\t/* Let the kernel choose something close to the middle. */\n> > > +\t\t\tret = data->video_->setFrameInterval(&want);\n> > > +\t\t\tif (ret == 0)\n> > > +\t\t\t\tdata->timePerFrame_ = want;\n> > > +\t\t}\n> > > \t}\n> > >\n> > > \tret = data->video_->streamOn();\n> > > @@ -355,6 +385,8 @@ int PipelineHandlerUVC::processControl(const UVCCameraData *data, ControlList *c\n> > > \t\tcid = V4L2_CID_GAMMA;\n> > > \telse if (id == controls::AeEnable)\n> > > \t\treturn 0; /* Handled in `Camera::queueRequest()`. */\n> > > +\telse if (id == controls::FrameDurationLimits)\n> > > +\t\treturn 0; /* Handled in `start()` */\n> > > \telse\n> > > \t\treturn -EINVAL;\n> > >\n> > > @@ -555,6 +587,23 @@ int UVCCameraData::init(std::shared_ptr media)\n> > > \t * resolution from the largest size it advertises.\n> > > \t */\n> > > \tSize resolution;\n> > > +\tauto minFrameInterval = std::chrono::microseconds::max();\n> > > +\tauto maxFrameInterval = std::chrono::microseconds::min();\n> > > +\n> > > +\tconst auto processFrameIntervals = [&](PixelFormat pf, V4L2PixelFormat v4l2pf, SizeRange size) {\n> > > +\t\tif (size.min != size.max)\n> > > +\t\t\treturn;\n> >\n> > Can this happen ?\n>\n> I suppose not, I will replace it with an assertion.\n>\n>\n> >\n> > > +\n> > > +\t\tauto frameIntervals = video_->getFrameIntervalLimits(v4l2pf, size.min);\n> > > +\t\tif (!frameIntervals)\n> > > +\t\t\treturn;\n> > > +\n> > > +\t\tminFrameInterval = std::min(minFrameInterval, (*frameIntervals)[0]);\n> > > +\t\tmaxFrameInterval = std::max(maxFrameInterval, (*frameIntervals)[1]);\n> >\n> > Aren't you already taking the min/max in the video device helpers ?\n>\n> This is the min/max among all pixel formats and sizes, to provide initial\n> min/max values in the `ControlInfo`. See below ...\n>\n>\n> >\n> > > +\n> > > +\t\tframeIntervals_.try_emplace({ pf, size.min }, *frameIntervals);\n> > > +\t};\n> > > +\n> > > \tfor (const auto &format : video_->formats()) {\n> > > \t\tPixelFormat pixelFormat = format.first.toPixelFormat();\n> > > \t\tif (!pixelFormat.isValid())\n> > > @@ -566,6 +615,8 @@ int UVCCameraData::init(std::shared_ptr media)\n> > > \t\tfor (const SizeRange &sizeRange : sizeRanges) {\n> > > \t\t\tif (sizeRange.max > resolution)\n> > > \t\t\t\tresolution = sizeRange.max;\n> > > +\n> > > +\t\t\tprocessFrameIntervals(pixelFormat, format.first, sizeRange);\n> > > \t\t}\n> > > \t}\n> > >\n> > > @@ -625,6 +676,14 @@ int UVCCameraData::init(std::shared_ptr media)\n> > > \t\tctrls[&controls::AeEnable] = ControlInfo(false, true, true);\n> > > \t}\n> > >\n> > > +\t/* Use the global min/max here, limits will be updated in `configure()`. */\n> > > +\tif (!frameIntervals_.empty()) {\n> > > +\t\tctrls[&controls::FrameDurationLimits] = ControlInfo{\n> > > +\t\t\tint64_t(minFrameInterval.count()),\n> > > +\t\t\tint64_t(maxFrameInterval.count()),\n> > > +\t\t};\n> > > +\t}\n>\n> ... here the \"global\" limits are reported initially.\n>\n\nI see\n\n>\n> > > +\n> > > \tcontrolInfo_ = ControlInfoMap(std::move(ctrls), controls::controls);\n> > >\n> > > \t/*\n> > > diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp\n> > > index 3836dabef3..3db6e80aed 100644\n> > > --- a/src/libcamera/v4l2_videodevice.cpp\n> > > +++ b/src/libcamera/v4l2_videodevice.cpp\n> > > @@ -1202,6 +1202,122 @@ int V4L2VideoDevice::getFrameInterval(std::chrono::microseconds *interval)\n> > > \treturn 0;\n> > > }\n> > >\n> > > +/**\n> > > + * \\brief Configure the frame interval\n> > > + * \\param[inout] interval The frame interval\n> >\n> > unit would be nice here as well\n>\n> Ok.\n>\n> >\n> > > + *\n> > > + * Apply the supplied \\a interval as the time-per-frame stream parameter\n> > > + * on the device, and return the actually applied value.\n> > > + *\n> > > + * \\return 0 on success or a negative error code otherwise\n> > > + */\n> > > +int V4L2VideoDevice::setFrameInterval(std::chrono::microseconds *interval)\n> > > +{\n> > > +\tv4l2_fract *frameInterval = nullptr;\n> > > +\tuint32_t *caps = nullptr;\n> > > +\tv4l2_streamparm sparm = {};\n> > > +\n> > > +\tsparm.type = bufferType_;\n> > > +\n> > > +\tswitch (sparm.type) {\n> > > +\tcase V4L2_BUF_TYPE_VIDEO_CAPTURE:\n> > > +\tcase V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE:\n> > > +\t\tframeInterval = &sparm.parm.capture.timeperframe;\n> > > +\t\tcaps = &sparm.parm.capture.capability;\n> > > +\t\tbreak;\n> > > +\tcase V4L2_BUF_TYPE_VIDEO_OUTPUT:\n> > > +\tcase V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE:\n> > > +\t\tframeInterval = &sparm.parm.output.timeperframe;\n> > > +\t\tcaps = &sparm.parm.output.capability;\n> > > +\t\tbreak;\n> > > +\t}\n> > > +\n> > > +\tif (!frameInterval)\n> > > +\t\treturn -EINVAL;\n> > > +\n> > > +\tconstexpr auto max = std::numeric_limitsnumerator)>::max();\n> >\n> > Feels quite a corner case, but checking doesn't hurt ?\n> >\n> > > +\tif (interval->count() <= 0 || interval->count() > max)\n> > > +\t\treturn -EINVAL;\n> > > +\n> > > +\tframeInterval->numerator = interval->count();\n> > > +\tframeInterval->denominator = std::chrono::microseconds(std::chrono::seconds(1)).count();\n> > > +\n> > > +\tint ret = ioctl(VIDIOC_S_PARM, &sparm);\n> > > +\tif (ret)\n> > > +\t\treturn ret;\n> > > +\n> > > +\tif (!(*caps & V4L2_CAP_TIMEPERFRAME))\n> > > +\t\treturn -ENOTSUP;\n> >\n> > In this case, I' pretty sure you should check this flag before calling\n> > ioctl()\n>\n> I think it's fine. If `V4L2_CAP_TIMEPERFRAME` is not present, then nothing\n> (should) changes on the device and 0 is returned, in which case this condition\n> detects that; or the driver returns an error code, which is detected above.\n\nWhy going through the trouble of an ioctl if we know that it's not\nsupported ? Ah! or is caps only updated -after- the ioctl call ?\n\n>\n>\n> >\n> > > +\n> > > +\t*interval = v4l2FractionToMs(*frameInterval);\n> > > +\n> > > +\treturn 0;\n> > > +}\n> > > +\n> > > +/**\n> > > + * \\brief Retrieve the frame interval limits\n> > > + * \\param[in] pixelFormat The pixel format\n> > > + * \\param[in] size The size\n> > > + *\n> > > + * Retrieve the minimum and maximum available frame interval for\n> > > + * the given \\a pixelFormat and \\a size.\n> > > + *\n> > > + * \\return The min and max frame interval or std::nullopt otherwise\n> > > + */\n> > > +std::optional>\n> > > +V4L2VideoDevice::getFrameIntervalLimits(V4L2PixelFormat pixelFormat, Size size)\n> > > +{\n> > > +\tauto min = std::chrono::microseconds::max();\n> > > +\tauto max = std::chrono::microseconds::min();\n> > > +\tunsigned int index = 0;\n> > > +\tint ret;\n> > > +\n> > > +\tfor (;; index++) {\n> > > +\t\tstruct v4l2_frmivalenum frameInterval = {};\n> > > +\t\tframeInterval.index = index;\n> > > +\t\tframeInterval.pixel_format = pixelFormat;\n> > > +\t\tframeInterval.width = size.width;\n> > > +\t\tframeInterval.height = size.height;\n> > > +\n> > > +\t\tret = ioctl(VIDIOC_ENUM_FRAMEINTERVALS, &frameInterval);\n> > > +\t\tif (ret)\n> > > +\t\t\tbreak;\n> > > +\n> > > +\t\tswitch (frameInterval.type) {\n> > > +\t\tcase V4L2_FRMIVAL_TYPE_DISCRETE: {\n> > > +\t\t\tauto ms = v4l2FractionToMs(frameInterval.discrete);\n> > > +\n> > > +\t\t\tmin = std::min(min, ms);\n> > > +\t\t\tmax = std::max(max, ms);\n> > > +\t\t\tbreak;\n> > > +\t\t}\n> > > +\t\tcase V4L2_FRMIVAL_TYPE_CONTINUOUS:\n> > > +\t\tcase V4L2_FRMIVAL_TYPE_STEPWISE: {\n> > > +\t\t\tmin = std::min(min, v4l2FractionToMs(frameInterval.stepwise.min));\n> > > +\t\t\tmax = std::max(max, v4l2FractionToMs(frameInterval.stepwise.max));\n> > > +\t\t\tbreak;\n> > > +\t\t}\n> >\n> > You know, I'm still not sure if we should support anything != DISCRETE.\n> >\n> > If I'm not mistaken UVC will always be DISCRETE. Every other device\n> > that uses this function is likely something that we don't want to\n> > support (because we want the drivers not to use this API for complex\n> > devices).\n>\n> UVC can have stepwise: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/media/usb/uvc/uvc_v4l2.c?id=0048fbb4011ec55c32d3148b2cda56433f273375#n1001\n>\n\nArgh\n\n>\n> >\n> > The same could be said for VIDEO_OUTPUT/OUTPUT_MPLANE, this will only\n> > apply to codecs ?\n>\n> That's probably true, but since it was easy to handle those cases, I saw\n> no reason to intentionally omit them.\n>\n\nWell, I'm always in the \"the less code we have, the less code can\nbreak\" camp, but I guess that's not too drammatic\n\n>\n> >\n> > > +\t\tdefault:\n> > > +\t\t\tLOG(V4L2, Error)\n> > > +\t\t\t\t<< \"Unknown v4l2_frmsizetypes value \"\n> > > +\t\t\t\t<< frameInterval.type;\n> > > +\t\t\treturn {};\n> > > +\t\t}\n> > > +\t}\n> > > +\n> > > +\tif (ret && ret != -EINVAL) {\n> >\n> > I might have missed why -EINVAL is ok\n>\n> When the index is one more than the index of the last available frame duration,\n> then EINVAL is returned, which breaks the loop, but it is not an error condition.\n>\n\nAh, right right\n\n>\n> >\n> > > +\t\tLOG(V4L2, Error)\n> > > +\t\t\t<< \"Unable to enumerate pixel formats: \"\n> > > +\t\t\t<< strerror(-ret);\n> > > +\t\treturn {};\n> > > +\t}\n> > > +\n> > > +\tif (index <= 0)\n> >\n> > How can index be < 0 ?\n>\n> Admittedly it can't... I just like to do `<= 0` regardless of type.\n\nWhich creates confusion in the reader which might ask \"what am I\nmissing that could lead to index be < 0\" ?\n\n>\n>\n> >\n> > > +\t\treturn {};\n> > > +\n> > > +\treturn {{ min, max }};\n> > > +}\n> > > +\n> > > std::vector V4L2VideoDevice::enumPixelformats(uint32_t code)\n> > > {\n> > > \tstd::vector formats;\n> > > --\n> > > 2.52.0\n> > >\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id B890DC3257\n\tfor ;\n\tThu, 11 Dec 2025 09:15:07 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id B378461593;\n\tThu, 11 Dec 2025 10:15:06 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 182C7610A6\n\tfor ;\n\tThu, 11 Dec 2025 10:15:05 +0100 (CET)","from ideasonboard.com (93-46-82-201.ip106.fastwebnet.it\n\t[93.46.82.201])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 939801661;\n\tThu, 11 Dec 2025 10:15:02 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"SJMNlEG0\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1765444502;\n\tbh=Lbtb1qcmS0/xWs1E1aZ/GMQyzLfVhi8d1y5yOyTjkUs=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=SJMNlEG08NHxUAC6ujI5LcdEmLEvRxxqne0uvpEdszofQ1ScXqqjcIyg3xlafsLQQ\n\t1UxzlXUShjUzfml7kmhfv9/W7bdQtA58n2N5Sg+LqdRcFOIanpRmbkFQk4mK7gb77R\n\tiHt9IhMD7DZwdGFvM7bemmIZ4YdrYhHSs6zzNO7Q=","Date":"Thu, 11 Dec 2025 10:15:01 +0100","From":"Jacopo Mondi ","To":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","Cc":"Jacopo Mondi , \n\tlibcamera-devel@lists.libcamera.org","Subject":"Re: [RFC PATCH v1 2/2] libcamera: pipeline: uvcvideo: Handle\n\t`FrameDurationLimits`","Message-ID":"","References":"<20251210133704.2711629-1-barnabas.pocze@ideasonboard.com>\n\t<20251210133704.2711629-3-barnabas.pocze@ideasonboard.com>\n\t<3u36wmmvt4np2bslrb5hjb6caouuhyjh3fbc7ysf4mmvenrhlw@nsvma7gi2c6e>\n\t<58222843-07aa-45a8-9740-bd481b76c1e8@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<58222843-07aa-45a8-9740-bd481b76c1e8@ideasonboard.com>","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":37295,"web_url":"https://patchwork.libcamera.org/comment/37295/","msgid":"<7fa0d67c-96f6-4440-9055-d03fcb6938ea@ideasonboard.com>","date":"2025-12-11T09:38:19","subject":"Re: [RFC PATCH v1 2/2] libcamera: pipeline: uvcvideo: Handle\n\t`FrameDurationLimits`","submitter":{"id":216,"url":"https://patchwork.libcamera.org/api/people/216/","name":"Barnabás Pőcze","email":"barnabas.pocze@ideasonboard.com"},"content":"Hi\n\n2025. 12. 11. 10:15 keltezéssel, Jacopo Mondi írta:\n> Hi Barnabás\n> \n> On Wed, Dec 10, 2025 at 05:56:01PM +0100, Barnabás Pőcze wrote:\n>> Hi\n>>\n>> 2025. 12. 10. 17:04 keltezéssel, Jacopo Mondi írta:\n>>> Hi Barnabás\n>>>\n>>> On Wed, Dec 10, 2025 at 02:37:04PM +0100, Barnabás Pőcze wrote:\n>>>> Setting a frame rate is possible with UVC devices using `VIDIOC_S_PARM`.\n>>>> However, compared to other platforms supported by libcamera, limitations\n>>>> apply. Probably most crucially, the desired frame rate cannot be set\n>>>> while the camera is streaming. Furthermore, it is only a single value\n>>>> and not an allowed range.\n>>>>\n>>>> So this change only adds support for `FrameDurationLimits` in the control\n>>>> list passed to `Camera::start()`, and the control is otherwise ignored in\n>>>> requests.\n>>>>\n>>>> The kernel interface also only allows a single number and not a range,\n>>>> so the midpoint of the desired range is used. Checking the supplied\n>>>\n>>> Everything else sounds fine, but I wonder if taking the midpoint is\n>>> the best solution here or we should simply use the shortest provided\n>>> duration, warn the user, and let the kernel adjust it to the closest\n>>> supported value.\n>>\n>> As far as I understand, the kernel selects the closest valid frame duration\n>> value. Which means that using the midpoint should guarantee that if the device\n>> supports at least one frame duration value in the desired range, then a value\n>> in the desired range will be selected on the device. This is not guaranteed if\n>> either the min or max is sent to the kernel. That's why I chose this behaviour;\n>> I believe it's probably best to select a frame duration from the desired range,\n>> if possible.\n> \n> Well, you have a cache of those now, don't you ?\n\nOnly the min/max is stored for a particular (pixel format, size) combination,\nand that is only done to be able to report it in the `ControlInfo` when the\nconfiguration changes.\n\nEven if there all the frame interval options were stored, I still think selecting\nthe midpoint is the best that we can do. Unless I am missing something?\n\n\n> \n>>\n>>\n>>>\n>>>> values is not necessary since the kernel will adjust the value if\n>>>> it is not supported by the device.\n>>>>\n>>>> Initially the global min/max values are advertised in the `ControlInfo`\n>>>> of the `FrameDurationLimits` control, which are then updated after\n>>>> the camera is configured. Updating the control limits after configuration\n>>>> matches the behaviour of other platforms.\n>>>>\n>>>> While the kernel interface differentiates three types of frame intervals\n>>>> (discrete, continuous, stepwise), when querying the available frame intervals\n>>>> for a given (pixel format, size) combination, all options are evaluated\n>>>> and only the \"local\" minimum and maximum is used, as that is the only way\n>>>> the limits can reasonably be advertised on the libcamera side.\n>>>>\n>>>> Closes: https://gitlab.freedesktop.org/camera/libcamera/-/issues/296\n>>>> Signed-off-by: Barnabás Pőcze \n>>>> ---\n>>>> include/libcamera/internal/v4l2_videodevice.h | 3 +\n>>>> src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 59 +++++++++\n>>>> src/libcamera/v4l2_videodevice.cpp | 116 ++++++++++++++++++\n>>>> 3 files changed, 178 insertions(+)\n>>>>\n>>>> diff --git a/include/libcamera/internal/v4l2_videodevice.h b/include/libcamera/internal/v4l2_videodevice.h\n>>>> index 82d98184ed..e97c0f9bf8 100644\n>>>> --- a/include/libcamera/internal/v4l2_videodevice.h\n>>>> +++ b/include/libcamera/internal/v4l2_videodevice.h\n>>>> @@ -209,6 +209,9 @@ public:\n>>>> \tFormats formats(uint32_t code = 0);\n>>>>\n>>>> \tint getFrameInterval(std::chrono::microseconds *interval);\n>>>> +\tint setFrameInterval(std::chrono::microseconds *interval);\n>>>> +\tstd::optional>\n>>>> +\tgetFrameIntervalLimits(V4L2PixelFormat pixelFormat, Size size);\n>>>>\n>>>> \tint getSelection(unsigned int target, Rectangle *rect);\n>>>> \tint setSelection(unsigned int target, Rectangle *rect);\n>>>> diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n>>>> index 3f98e8ece0..e4e9b8ab9b 100644\n>>>> --- a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n>>>> +++ b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n>>>> @@ -57,6 +57,7 @@ public:\n>>>> \tstd::unique_ptr video_;\n>>>> \tStream stream_;\n>>>> \tstd::map> formats_;\n>>>> +\tstd::map, std::array> frameIntervals_;\n>>>>\n>>>> \tstd::optional autoExposureMode_;\n>>>> \tstd::optional manualExposureMode_;\n>>>> @@ -277,6 +278,22 @@ int PipelineHandlerUVC::configure(Camera *camera, CameraConfiguration *config)\n>>>> \t format.fourcc != data->video_->toV4L2PixelFormat(cfg.pixelFormat))\n>>>> \t\treturn -EINVAL;\n>>>>\n>>>> +\tauto it = data->controlInfo_.find(&controls::FrameDurationLimits);\n>>>> +\tif (it != data->controlInfo_.end()) {\n>>>> +\t\tauto it2 = data->frameIntervals_.find({ cfg.pixelFormat, cfg.size });\n>>>> +\t\tif (it2 != data->frameIntervals_.end()) {\n>>>> +\t\t\tstd::chrono::microseconds current;\n>>>> +\n>>>> +\t\t\tret = data->video_->getFrameInterval(¤t);\n>>>> +\n>>>> +\t\t\tit->second = ControlInfo{\n>>>> +\t\t\t\tint64_t(it2->second[0].count()),\n>>>> +\t\t\t\tint64_t(it2->second[1].count()),\n>>>> +\t\t\t\tret == 0 ? ControlValue(int64_t(current.count())) : ControlValue()\n>>>> +\t\t\t};\n>>>> +\t\t}\n>>>> +\t}\n>>>> +\n>>>> \tcfg.setStream(&data->stream_);\n>>>>\n>>>> \treturn 0;\n>>>> @@ -306,6 +323,19 @@ int PipelineHandlerUVC::start(Camera *camera, const ControlList *controls)\n>>>> \t\tret = processControls(data, *controls);\n>>>> \t\tif (ret < 0)\n>>>> \t\t\tgoto err_release_buffers;\n>>>> +\n>>>> +\t\t/* Can only be set before starting. */\n>>>> +\t\tauto fdl = controls->get(controls::FrameDurationLimits);\n>>>> +\t\tif (fdl) {\n>>>> +\t\t\tconst auto wantMin = std::chrono::microseconds((*fdl)[0]);\n>>>> +\t\t\tconst auto wantMax = std::chrono::microseconds((*fdl)[1]);\n>>>> +\t\t\tauto want = (wantMin + wantMax) / 2;\n>>>> +\n>>>> +\t\t\t/* Let the kernel choose something close to the middle. */\n>>>> +\t\t\tret = data->video_->setFrameInterval(&want);\n>>>> +\t\t\tif (ret == 0)\n>>>> +\t\t\t\tdata->timePerFrame_ = want;\n>>>> +\t\t}\n>>>> \t}\n>>>>\n>>>> \tret = data->video_->streamOn();\n>>>> @@ -355,6 +385,8 @@ int PipelineHandlerUVC::processControl(const UVCCameraData *data, ControlList *c\n>>>> \t\tcid = V4L2_CID_GAMMA;\n>>>> \telse if (id == controls::AeEnable)\n>>>> \t\treturn 0; /* Handled in `Camera::queueRequest()`. */\n>>>> +\telse if (id == controls::FrameDurationLimits)\n>>>> +\t\treturn 0; /* Handled in `start()` */\n>>>> \telse\n>>>> \t\treturn -EINVAL;\n>>>>\n>>>> @@ -555,6 +587,23 @@ int UVCCameraData::init(std::shared_ptr media)\n>>>> \t * resolution from the largest size it advertises.\n>>>> \t */\n>>>> \tSize resolution;\n>>>> +\tauto minFrameInterval = std::chrono::microseconds::max();\n>>>> +\tauto maxFrameInterval = std::chrono::microseconds::min();\n>>>> +\n>>>> +\tconst auto processFrameIntervals = [&](PixelFormat pf, V4L2PixelFormat v4l2pf, SizeRange size) {\n>>>> +\t\tif (size.min != size.max)\n>>>> +\t\t\treturn;\n>>>\n>>> Can this happen ?\n>>\n>> I suppose not, I will replace it with an assertion.\n>>\n>>\n>>>\n>>>> +\n>>>> +\t\tauto frameIntervals = video_->getFrameIntervalLimits(v4l2pf, size.min);\n>>>> +\t\tif (!frameIntervals)\n>>>> +\t\t\treturn;\n>>>> +\n>>>> +\t\tminFrameInterval = std::min(minFrameInterval, (*frameIntervals)[0]);\n>>>> +\t\tmaxFrameInterval = std::max(maxFrameInterval, (*frameIntervals)[1]);\n>>>\n>>> Aren't you already taking the min/max in the video device helpers ?\n>>\n>> This is the min/max among all pixel formats and sizes, to provide initial\n>> min/max values in the `ControlInfo`. See below ...\n>>\n>>\n>>>\n>>>> +\n>>>> +\t\tframeIntervals_.try_emplace({ pf, size.min }, *frameIntervals);\n>>>> +\t};\n>>>> +\n>>>> \tfor (const auto &format : video_->formats()) {\n>>>> \t\tPixelFormat pixelFormat = format.first.toPixelFormat();\n>>>> \t\tif (!pixelFormat.isValid())\n>>>> @@ -566,6 +615,8 @@ int UVCCameraData::init(std::shared_ptr media)\n>>>> \t\tfor (const SizeRange &sizeRange : sizeRanges) {\n>>>> \t\t\tif (sizeRange.max > resolution)\n>>>> \t\t\t\tresolution = sizeRange.max;\n>>>> +\n>>>> +\t\t\tprocessFrameIntervals(pixelFormat, format.first, sizeRange);\n>>>> \t\t}\n>>>> \t}\n>>>>\n>>>> @@ -625,6 +676,14 @@ int UVCCameraData::init(std::shared_ptr media)\n>>>> \t\tctrls[&controls::AeEnable] = ControlInfo(false, true, true);\n>>>> \t}\n>>>>\n>>>> +\t/* Use the global min/max here, limits will be updated in `configure()`. */\n>>>> +\tif (!frameIntervals_.empty()) {\n>>>> +\t\tctrls[&controls::FrameDurationLimits] = ControlInfo{\n>>>> +\t\t\tint64_t(minFrameInterval.count()),\n>>>> +\t\t\tint64_t(maxFrameInterval.count()),\n>>>> +\t\t};\n>>>> +\t}\n>>\n>> ... here the \"global\" limits are reported initially.\n>>\n> \n> I see\n> \n>>\n>>>> +\n>>>> \tcontrolInfo_ = ControlInfoMap(std::move(ctrls), controls::controls);\n>>>>\n>>>> \t/*\n>>>> diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp\n>>>> index 3836dabef3..3db6e80aed 100644\n>>>> --- a/src/libcamera/v4l2_videodevice.cpp\n>>>> +++ b/src/libcamera/v4l2_videodevice.cpp\n>>>> @@ -1202,6 +1202,122 @@ int V4L2VideoDevice::getFrameInterval(std::chrono::microseconds *interval)\n>>>> \treturn 0;\n>>>> }\n>>>>\n>>>> +/**\n>>>> + * \\brief Configure the frame interval\n>>>> + * \\param[inout] interval The frame interval\n>>>\n>>> unit would be nice here as well\n>>\n>> Ok.\n>>\n>>>\n>>>> + *\n>>>> + * Apply the supplied \\a interval as the time-per-frame stream parameter\n>>>> + * on the device, and return the actually applied value.\n>>>> + *\n>>>> + * \\return 0 on success or a negative error code otherwise\n>>>> + */\n>>>> +int V4L2VideoDevice::setFrameInterval(std::chrono::microseconds *interval)\n>>>> +{\n>>>> +\tv4l2_fract *frameInterval = nullptr;\n>>>> +\tuint32_t *caps = nullptr;\n>>>> +\tv4l2_streamparm sparm = {};\n>>>> +\n>>>> +\tsparm.type = bufferType_;\n>>>> +\n>>>> +\tswitch (sparm.type) {\n>>>> +\tcase V4L2_BUF_TYPE_VIDEO_CAPTURE:\n>>>> +\tcase V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE:\n>>>> +\t\tframeInterval = &sparm.parm.capture.timeperframe;\n>>>> +\t\tcaps = &sparm.parm.capture.capability;\n>>>> +\t\tbreak;\n>>>> +\tcase V4L2_BUF_TYPE_VIDEO_OUTPUT:\n>>>> +\tcase V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE:\n>>>> +\t\tframeInterval = &sparm.parm.output.timeperframe;\n>>>> +\t\tcaps = &sparm.parm.output.capability;\n>>>> +\t\tbreak;\n>>>> +\t}\n>>>> +\n>>>> +\tif (!frameInterval)\n>>>> +\t\treturn -EINVAL;\n>>>> +\n>>>> +\tconstexpr auto max = std::numeric_limitsnumerator)>::max();\n>>>\n>>> Feels quite a corner case, but checking doesn't hurt ?\n>>>\n>>>> +\tif (interval->count() <= 0 || interval->count() > max)\n>>>> +\t\treturn -EINVAL;\n>>>> +\n>>>> +\tframeInterval->numerator = interval->count();\n>>>> +\tframeInterval->denominator = std::chrono::microseconds(std::chrono::seconds(1)).count();\n>>>> +\n>>>> +\tint ret = ioctl(VIDIOC_S_PARM, &sparm);\n>>>> +\tif (ret)\n>>>> +\t\treturn ret;\n>>>> +\n>>>> +\tif (!(*caps & V4L2_CAP_TIMEPERFRAME))\n>>>> +\t\treturn -ENOTSUP;\n>>>\n>>> In this case, I' pretty sure you should check this flag before calling\n>>> ioctl()\n>>\n>> I think it's fine. If `V4L2_CAP_TIMEPERFRAME` is not present, then nothing\n>> (should) changes on the device and 0 is returned, in which case this condition\n>> detects that; or the driver returns an error code, which is detected above.\n> \n> Why going through the trouble of an ioctl if we know that it's not\n> supported ? Ah! or is caps only updated -after- the ioctl call ?\n\nThe ioctl updates the capabilities, yes. I think this could be checked in `open()` and remembered.\nWould that be better?\n\n\n> \n>>\n>>\n>>>\n>>>> +\n>>>> +\t*interval = v4l2FractionToMs(*frameInterval);\n>>>> +\n>>>> +\treturn 0;\n>>>> +}\n>>>> +\n>>>> +/**\n>>>> + * \\brief Retrieve the frame interval limits\n>>>> + * \\param[in] pixelFormat The pixel format\n>>>> + * \\param[in] size The size\n>>>> + *\n>>>> + * Retrieve the minimum and maximum available frame interval for\n>>>> + * the given \\a pixelFormat and \\a size.\n>>>> + *\n>>>> + * \\return The min and max frame interval or std::nullopt otherwise\n>>>> + */\n>>>> +std::optional>\n>>>> +V4L2VideoDevice::getFrameIntervalLimits(V4L2PixelFormat pixelFormat, Size size)\n>>>> +{\n>>>> +\tauto min = std::chrono::microseconds::max();\n>>>> +\tauto max = std::chrono::microseconds::min();\n>>>> +\tunsigned int index = 0;\n>>>> +\tint ret;\n>>>> +\n>>>> +\tfor (;; index++) {\n>>>> +\t\tstruct v4l2_frmivalenum frameInterval = {};\n>>>> +\t\tframeInterval.index = index;\n>>>> +\t\tframeInterval.pixel_format = pixelFormat;\n>>>> +\t\tframeInterval.width = size.width;\n>>>> +\t\tframeInterval.height = size.height;\n>>>> +\n>>>> +\t\tret = ioctl(VIDIOC_ENUM_FRAMEINTERVALS, &frameInterval);\n>>>> +\t\tif (ret)\n>>>> +\t\t\tbreak;\n>>>> +\n>>>> +\t\tswitch (frameInterval.type) {\n>>>> +\t\tcase V4L2_FRMIVAL_TYPE_DISCRETE: {\n>>>> +\t\t\tauto ms = v4l2FractionToMs(frameInterval.discrete);\n>>>> +\n>>>> +\t\t\tmin = std::min(min, ms);\n>>>> +\t\t\tmax = std::max(max, ms);\n>>>> +\t\t\tbreak;\n>>>> +\t\t}\n>>>> +\t\tcase V4L2_FRMIVAL_TYPE_CONTINUOUS:\n>>>> +\t\tcase V4L2_FRMIVAL_TYPE_STEPWISE: {\n>>>> +\t\t\tmin = std::min(min, v4l2FractionToMs(frameInterval.stepwise.min));\n>>>> +\t\t\tmax = std::max(max, v4l2FractionToMs(frameInterval.stepwise.max));\n>>>> +\t\t\tbreak;\n>>>> +\t\t}\n>>>\n>>> You know, I'm still not sure if we should support anything != DISCRETE.\n>>>\n>>> If I'm not mistaken UVC will always be DISCRETE. Every other device\n>>> that uses this function is likely something that we don't want to\n>>> support (because we want the drivers not to use this API for complex\n>>> devices).\n>>\n>> UVC can have stepwise: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/media/usb/uvc/uvc_v4l2.c?id=0048fbb4011ec55c32d3148b2cda56433f273375#n1001\n>>\n> \n> Argh\n> \n>>\n>>>\n>>> The same could be said for VIDEO_OUTPUT/OUTPUT_MPLANE, this will only\n>>> apply to codecs ?\n>>\n>> That's probably true, but since it was easy to handle those cases, I saw\n>> no reason to intentionally omit them.\n>>\n> \n> Well, I'm always in the \"the less code we have, the less code can\n> break\" camp, but I guess that's not too drammatic\n> \n>>\n>>>\n>>>> +\t\tdefault:\n>>>> +\t\t\tLOG(V4L2, Error)\n>>>> +\t\t\t\t<< \"Unknown v4l2_frmsizetypes value \"\n>>>> +\t\t\t\t<< frameInterval.type;\n>>>> +\t\t\treturn {};\n>>>> +\t\t}\n>>>> +\t}\n>>>> +\n>>>> +\tif (ret && ret != -EINVAL) {\n>>>\n>>> I might have missed why -EINVAL is ok\n>>\n>> When the index is one more than the index of the last available frame duration,\n>> then EINVAL is returned, which breaks the loop, but it is not an error condition.\n>>\n> \n> Ah, right right\n> \n>>\n>>>\n>>>> +\t\tLOG(V4L2, Error)\n>>>> +\t\t\t<< \"Unable to enumerate pixel formats: \"\n>>>> +\t\t\t<< strerror(-ret);\n>>>> +\t\treturn {};\n>>>> +\t}\n>>>> +\n>>>> +\tif (index <= 0)\n>>>\n>>> How can index be < 0 ?\n>>\n>> Admittedly it can't... I just like to do `<= 0` regardless of type.\n> \n> Which creates confusion in the reader which might ask \"what am I\n> missing that could lead to index be < 0\" ?\n\nOkay, I'll change it.\n\n\n> \n>>\n>>\n>>>\n>>>> +\t\treturn {};\n>>>> +\n>>>> +\treturn {{ min, max }};\n>>>> +}\n>>>> +\n>>>> std::vector V4L2VideoDevice::enumPixelformats(uint32_t code)\n>>>> {\n>>>> \tstd::vector formats;\n>>>> --\n>>>> 2.52.0\n>>>>\n>>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 1B0F4BD1F1\n\tfor ;\n\tThu, 11 Dec 2025 09:38:33 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 0ADD6615A7;\n\tThu, 11 Dec 2025 10:38:32 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id A4A0C610A6\n\tfor ;\n\tThu, 11 Dec 2025 10:38:23 +0100 (CET)","from [192.168.33.38] (185.221.143.114.nat.pool.zt.hu\n\t[185.221.143.114])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 078DA1661;\n\tThu, 11 Dec 2025 10:38:21 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"OMTJIART\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1765445901;\n\tbh=4mAAt/CjQPspaqNFu3gwoKBTiMB+c4l5RVb4D27q12Y=;\n\th=Date:Subject:To:Cc:References:From:In-Reply-To:From;\n\tb=OMTJIARTLJx5zlxcrpQ5MOjXPiMS8g1V/leveTbuIn6mysBdU0u8MJxHgH828XjHu\n\tKqEarJkcW9/TQg6dzqhjXZqpxooqGGg0PatkTYr7hTX3USWszrk8zYnEGAwqKFt4FY\n\ta5/c4vEPxYmOWBOm9j0wcIAxUKPUR89pwiPCTLPs=","Message-ID":"<7fa0d67c-96f6-4440-9055-d03fcb6938ea@ideasonboard.com>","Date":"Thu, 11 Dec 2025 10:38:19 +0100","MIME-Version":"1.0","User-Agent":"Mozilla Thunderbird","Subject":"Re: [RFC PATCH v1 2/2] libcamera: pipeline: uvcvideo: Handle\n\t`FrameDurationLimits`","To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","References":"<20251210133704.2711629-1-barnabas.pocze@ideasonboard.com>\n\t<20251210133704.2711629-3-barnabas.pocze@ideasonboard.com>\n\t<3u36wmmvt4np2bslrb5hjb6caouuhyjh3fbc7ysf4mmvenrhlw@nsvma7gi2c6e>\n\t<58222843-07aa-45a8-9740-bd481b76c1e8@ideasonboard.com>\n\t","From":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","Content-Language":"en-US, hu-HU","In-Reply-To":"","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"8bit","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":37301,"web_url":"https://patchwork.libcamera.org/comment/37301/","msgid":"","date":"2025-12-11T11:55:11","subject":"Re: [RFC PATCH v1 2/2] libcamera: pipeline: uvcvideo: Handle\n\t`FrameDurationLimits`","submitter":{"id":143,"url":"https://patchwork.libcamera.org/api/people/143/","name":"Jacopo Mondi","email":"jacopo.mondi@ideasonboard.com"},"content":"Hi Barnabás\n\nOn Thu, Dec 11, 2025 at 10:38:19AM +0100, Barnabás Pőcze wrote:\n> Hi\n>\n> 2025. 12. 11. 10:15 keltezéssel, Jacopo Mondi írta:\n> > Hi Barnabás\n> >\n> > On Wed, Dec 10, 2025 at 05:56:01PM +0100, Barnabás Pőcze wrote:\n> > > Hi\n> > >\n> > > 2025. 12. 10. 17:04 keltezéssel, Jacopo Mondi írta:\n> > > > Hi Barnabás\n> > > >\n> > > > On Wed, Dec 10, 2025 at 02:37:04PM +0100, Barnabás Pőcze wrote:\n> > > > > Setting a frame rate is possible with UVC devices using `VIDIOC_S_PARM`.\n> > > > > However, compared to other platforms supported by libcamera, limitations\n> > > > > apply. Probably most crucially, the desired frame rate cannot be set\n> > > > > while the camera is streaming. Furthermore, it is only a single value\n> > > > > and not an allowed range.\n> > > > >\n> > > > > So this change only adds support for `FrameDurationLimits` in the control\n> > > > > list passed to `Camera::start()`, and the control is otherwise ignored in\n> > > > > requests.\n> > > > >\n> > > > > The kernel interface also only allows a single number and not a range,\n> > > > > so the midpoint of the desired range is used. Checking the supplied\n> > > >\n> > > > Everything else sounds fine, but I wonder if taking the midpoint is\n> > > > the best solution here or we should simply use the shortest provided\n> > > > duration, warn the user, and let the kernel adjust it to the closest\n> > > > supported value.\n> > >\n> > > As far as I understand, the kernel selects the closest valid frame duration\n> > > value. Which means that using the midpoint should guarantee that if the device\n> > > supports at least one frame duration value in the desired range, then a value\n> > > in the desired range will be selected on the device. This is not guaranteed if\n> > > either the min or max is sent to the kernel. That's why I chose this behaviour;\n> > > I believe it's probably best to select a frame duration from the desired range,\n> > > if possible.\n> >\n> > Well, you have a cache of those now, don't you ?\n>\n> Only the min/max is stored for a particular (pixel format, size) combination,\n> and that is only done to be able to report it in the `ControlInfo` when the\n> configuration changes.\n\nRight, sorry, I've overlooked it\n\n>\n> Even if there all the frame interval options were stored, I still think selecting\n> the midpoint is the best that we can do. Unless I am missing something?\n>\n\nI'm in two minds here.\n\nI'm fine if we select the midpoint, but I would log the user about\nwhat we're doing (and about the fact uvc doesn't support frame\nduration ranges)\n\n>\n> >\n> > >\n> > >\n> > > >\n> > > > > values is not necessary since the kernel will adjust the value if\n> > > > > it is not supported by the device.\n> > > > >\n> > > > > Initially the global min/max values are advertised in the `ControlInfo`\n> > > > > of the `FrameDurationLimits` control, which are then updated after\n> > > > > the camera is configured. Updating the control limits after configuration\n> > > > > matches the behaviour of other platforms.\n> > > > >\n> > > > > While the kernel interface differentiates three types of frame intervals\n> > > > > (discrete, continuous, stepwise), when querying the available frame intervals\n> > > > > for a given (pixel format, size) combination, all options are evaluated\n> > > > > and only the \"local\" minimum and maximum is used, as that is the only way\n> > > > > the limits can reasonably be advertised on the libcamera side.\n> > > > >\n> > > > > Closes: https://gitlab.freedesktop.org/camera/libcamera/-/issues/296\n> > > > > Signed-off-by: Barnabás Pőcze \n> > > > > ---\n> > > > > include/libcamera/internal/v4l2_videodevice.h | 3 +\n> > > > > src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 59 +++++++++\n> > > > > src/libcamera/v4l2_videodevice.cpp | 116 ++++++++++++++++++\n> > > > > 3 files changed, 178 insertions(+)\n> > > > >\n> > > > > diff --git a/include/libcamera/internal/v4l2_videodevice.h b/include/libcamera/internal/v4l2_videodevice.h\n> > > > > index 82d98184ed..e97c0f9bf8 100644\n> > > > > --- a/include/libcamera/internal/v4l2_videodevice.h\n> > > > > +++ b/include/libcamera/internal/v4l2_videodevice.h\n> > > > > @@ -209,6 +209,9 @@ public:\n> > > > > \tFormats formats(uint32_t code = 0);\n> > > > >\n> > > > > \tint getFrameInterval(std::chrono::microseconds *interval);\n> > > > > +\tint setFrameInterval(std::chrono::microseconds *interval);\n> > > > > +\tstd::optional>\n> > > > > +\tgetFrameIntervalLimits(V4L2PixelFormat pixelFormat, Size size);\n> > > > >\n> > > > > \tint getSelection(unsigned int target, Rectangle *rect);\n> > > > > \tint setSelection(unsigned int target, Rectangle *rect);\n> > > > > diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> > > > > index 3f98e8ece0..e4e9b8ab9b 100644\n> > > > > --- a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> > > > > +++ b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> > > > > @@ -57,6 +57,7 @@ public:\n> > > > > \tstd::unique_ptr video_;\n> > > > > \tStream stream_;\n> > > > > \tstd::map> formats_;\n> > > > > +\tstd::map, std::array> frameIntervals_;\n> > > > >\n> > > > > \tstd::optional autoExposureMode_;\n> > > > > \tstd::optional manualExposureMode_;\n> > > > > @@ -277,6 +278,22 @@ int PipelineHandlerUVC::configure(Camera *camera, CameraConfiguration *config)\n> > > > > \t format.fourcc != data->video_->toV4L2PixelFormat(cfg.pixelFormat))\n> > > > > \t\treturn -EINVAL;\n> > > > >\n> > > > > +\tauto it = data->controlInfo_.find(&controls::FrameDurationLimits);\n> > > > > +\tif (it != data->controlInfo_.end()) {\n> > > > > +\t\tauto it2 = data->frameIntervals_.find({ cfg.pixelFormat, cfg.size });\n> > > > > +\t\tif (it2 != data->frameIntervals_.end()) {\n> > > > > +\t\t\tstd::chrono::microseconds current;\n> > > > > +\n> > > > > +\t\t\tret = data->video_->getFrameInterval(¤t);\n> > > > > +\n> > > > > +\t\t\tit->second = ControlInfo{\n> > > > > +\t\t\t\tint64_t(it2->second[0].count()),\n> > > > > +\t\t\t\tint64_t(it2->second[1].count()),\n> > > > > +\t\t\t\tret == 0 ? ControlValue(int64_t(current.count())) : ControlValue()\n> > > > > +\t\t\t};\n> > > > > +\t\t}\n> > > > > +\t}\n> > > > > +\n> > > > > \tcfg.setStream(&data->stream_);\n> > > > >\n> > > > > \treturn 0;\n> > > > > @@ -306,6 +323,19 @@ int PipelineHandlerUVC::start(Camera *camera, const ControlList *controls)\n> > > > > \t\tret = processControls(data, *controls);\n> > > > > \t\tif (ret < 0)\n> > > > > \t\t\tgoto err_release_buffers;\n> > > > > +\n> > > > > +\t\t/* Can only be set before starting. */\n> > > > > +\t\tauto fdl = controls->get(controls::FrameDurationLimits);\n> > > > > +\t\tif (fdl) {\n> > > > > +\t\t\tconst auto wantMin = std::chrono::microseconds((*fdl)[0]);\n> > > > > +\t\t\tconst auto wantMax = std::chrono::microseconds((*fdl)[1]);\n> > > > > +\t\t\tauto want = (wantMin + wantMax) / 2;\n> > > > > +\n> > > > > +\t\t\t/* Let the kernel choose something close to the middle. */\n> > > > > +\t\t\tret = data->video_->setFrameInterval(&want);\n> > > > > +\t\t\tif (ret == 0)\n> > > > > +\t\t\t\tdata->timePerFrame_ = want;\n> > > > > +\t\t}\n> > > > > \t}\n> > > > >\n> > > > > \tret = data->video_->streamOn();\n> > > > > @@ -355,6 +385,8 @@ int PipelineHandlerUVC::processControl(const UVCCameraData *data, ControlList *c\n> > > > > \t\tcid = V4L2_CID_GAMMA;\n> > > > > \telse if (id == controls::AeEnable)\n> > > > > \t\treturn 0; /* Handled in `Camera::queueRequest()`. */\n> > > > > +\telse if (id == controls::FrameDurationLimits)\n> > > > > +\t\treturn 0; /* Handled in `start()` */\n> > > > > \telse\n> > > > > \t\treturn -EINVAL;\n> > > > >\n> > > > > @@ -555,6 +587,23 @@ int UVCCameraData::init(std::shared_ptr media)\n> > > > > \t * resolution from the largest size it advertises.\n> > > > > \t */\n> > > > > \tSize resolution;\n> > > > > +\tauto minFrameInterval = std::chrono::microseconds::max();\n> > > > > +\tauto maxFrameInterval = std::chrono::microseconds::min();\n> > > > > +\n> > > > > +\tconst auto processFrameIntervals = [&](PixelFormat pf, V4L2PixelFormat v4l2pf, SizeRange size) {\n> > > > > +\t\tif (size.min != size.max)\n> > > > > +\t\t\treturn;\n> > > >\n> > > > Can this happen ?\n> > >\n> > > I suppose not, I will replace it with an assertion.\n> > >\n> > >\n> > > >\n> > > > > +\n> > > > > +\t\tauto frameIntervals = video_->getFrameIntervalLimits(v4l2pf, size.min);\n> > > > > +\t\tif (!frameIntervals)\n> > > > > +\t\t\treturn;\n> > > > > +\n> > > > > +\t\tminFrameInterval = std::min(minFrameInterval, (*frameIntervals)[0]);\n> > > > > +\t\tmaxFrameInterval = std::max(maxFrameInterval, (*frameIntervals)[1]);\n> > > >\n> > > > Aren't you already taking the min/max in the video device helpers ?\n> > >\n> > > This is the min/max among all pixel formats and sizes, to provide initial\n> > > min/max values in the `ControlInfo`. See below ...\n> > >\n> > >\n> > > >\n> > > > > +\n> > > > > +\t\tframeIntervals_.try_emplace({ pf, size.min }, *frameIntervals);\n> > > > > +\t};\n> > > > > +\n> > > > > \tfor (const auto &format : video_->formats()) {\n> > > > > \t\tPixelFormat pixelFormat = format.first.toPixelFormat();\n> > > > > \t\tif (!pixelFormat.isValid())\n> > > > > @@ -566,6 +615,8 @@ int UVCCameraData::init(std::shared_ptr media)\n> > > > > \t\tfor (const SizeRange &sizeRange : sizeRanges) {\n> > > > > \t\t\tif (sizeRange.max > resolution)\n> > > > > \t\t\t\tresolution = sizeRange.max;\n> > > > > +\n> > > > > +\t\t\tprocessFrameIntervals(pixelFormat, format.first, sizeRange);\n> > > > > \t\t}\n> > > > > \t}\n> > > > >\n> > > > > @@ -625,6 +676,14 @@ int UVCCameraData::init(std::shared_ptr media)\n> > > > > \t\tctrls[&controls::AeEnable] = ControlInfo(false, true, true);\n> > > > > \t}\n> > > > >\n> > > > > +\t/* Use the global min/max here, limits will be updated in `configure()`. */\n> > > > > +\tif (!frameIntervals_.empty()) {\n> > > > > +\t\tctrls[&controls::FrameDurationLimits] = ControlInfo{\n> > > > > +\t\t\tint64_t(minFrameInterval.count()),\n> > > > > +\t\t\tint64_t(maxFrameInterval.count()),\n> > > > > +\t\t};\n> > > > > +\t}\n> > >\n> > > ... here the \"global\" limits are reported initially.\n> > >\n> >\n> > I see\n> >\n> > >\n> > > > > +\n> > > > > \tcontrolInfo_ = ControlInfoMap(std::move(ctrls), controls::controls);\n> > > > >\n> > > > > \t/*\n> > > > > diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp\n> > > > > index 3836dabef3..3db6e80aed 100644\n> > > > > --- a/src/libcamera/v4l2_videodevice.cpp\n> > > > > +++ b/src/libcamera/v4l2_videodevice.cpp\n> > > > > @@ -1202,6 +1202,122 @@ int V4L2VideoDevice::getFrameInterval(std::chrono::microseconds *interval)\n> > > > > \treturn 0;\n> > > > > }\n> > > > >\n> > > > > +/**\n> > > > > + * \\brief Configure the frame interval\n> > > > > + * \\param[inout] interval The frame interval\n> > > >\n> > > > unit would be nice here as well\n> > >\n> > > Ok.\n> > >\n> > > >\n> > > > > + *\n> > > > > + * Apply the supplied \\a interval as the time-per-frame stream parameter\n> > > > > + * on the device, and return the actually applied value.\n> > > > > + *\n> > > > > + * \\return 0 on success or a negative error code otherwise\n> > > > > + */\n> > > > > +int V4L2VideoDevice::setFrameInterval(std::chrono::microseconds *interval)\n> > > > > +{\n> > > > > +\tv4l2_fract *frameInterval = nullptr;\n> > > > > +\tuint32_t *caps = nullptr;\n> > > > > +\tv4l2_streamparm sparm = {};\n> > > > > +\n> > > > > +\tsparm.type = bufferType_;\n> > > > > +\n> > > > > +\tswitch (sparm.type) {\n> > > > > +\tcase V4L2_BUF_TYPE_VIDEO_CAPTURE:\n> > > > > +\tcase V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE:\n> > > > > +\t\tframeInterval = &sparm.parm.capture.timeperframe;\n> > > > > +\t\tcaps = &sparm.parm.capture.capability;\n> > > > > +\t\tbreak;\n> > > > > +\tcase V4L2_BUF_TYPE_VIDEO_OUTPUT:\n> > > > > +\tcase V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE:\n> > > > > +\t\tframeInterval = &sparm.parm.output.timeperframe;\n> > > > > +\t\tcaps = &sparm.parm.output.capability;\n> > > > > +\t\tbreak;\n> > > > > +\t}\n> > > > > +\n> > > > > +\tif (!frameInterval)\n> > > > > +\t\treturn -EINVAL;\n> > > > > +\n> > > > > +\tconstexpr auto max = std::numeric_limitsnumerator)>::max();\n> > > >\n> > > > Feels quite a corner case, but checking doesn't hurt ?\n> > > >\n> > > > > +\tif (interval->count() <= 0 || interval->count() > max)\n> > > > > +\t\treturn -EINVAL;\n> > > > > +\n> > > > > +\tframeInterval->numerator = interval->count();\n> > > > > +\tframeInterval->denominator = std::chrono::microseconds(std::chrono::seconds(1)).count();\n> > > > > +\n> > > > > +\tint ret = ioctl(VIDIOC_S_PARM, &sparm);\n> > > > > +\tif (ret)\n> > > > > +\t\treturn ret;\n> > > > > +\n> > > > > +\tif (!(*caps & V4L2_CAP_TIMEPERFRAME))\n> > > > > +\t\treturn -ENOTSUP;\n> > > >\n> > > > In this case, I' pretty sure you should check this flag before calling\n> > > > ioctl()\n> > >\n> > > I think it's fine. If `V4L2_CAP_TIMEPERFRAME` is not present, then nothing\n> > > (should) changes on the device and 0 is returned, in which case this condition\n> > > detects that; or the driver returns an error code, which is detected above.\n> >\n> > Why going through the trouble of an ioctl if we know that it's not\n> > supported ? Ah! or is caps only updated -after- the ioctl call ?\n>\n> The ioctl updates the capabilities, yes. I think this could be checked in `open()` and remembered.\n> Would that be better?\n\nThe V4L2_CAP_TIMEPERFRAME cap is found in the v4l2_streamparm\nstructure. Can you get it from other caps at open() time or are you\nconsidering issueing a g_parm ioctl() to check if it is supported in\nopen() ?\n\n\n\n>\n>\n> >\n> > >\n> > >\n> > > >\n> > > > > +\n> > > > > +\t*interval = v4l2FractionToMs(*frameInterval);\n> > > > > +\n> > > > > +\treturn 0;\n> > > > > +}\n> > > > > +\n> > > > > +/**\n> > > > > + * \\brief Retrieve the frame interval limits\n> > > > > + * \\param[in] pixelFormat The pixel format\n> > > > > + * \\param[in] size The size\n> > > > > + *\n> > > > > + * Retrieve the minimum and maximum available frame interval for\n> > > > > + * the given \\a pixelFormat and \\a size.\n> > > > > + *\n> > > > > + * \\return The min and max frame interval or std::nullopt otherwise\n> > > > > + */\n> > > > > +std::optional>\n> > > > > +V4L2VideoDevice::getFrameIntervalLimits(V4L2PixelFormat pixelFormat, Size size)\n> > > > > +{\n> > > > > +\tauto min = std::chrono::microseconds::max();\n> > > > > +\tauto max = std::chrono::microseconds::min();\n> > > > > +\tunsigned int index = 0;\n> > > > > +\tint ret;\n> > > > > +\n> > > > > +\tfor (;; index++) {\n> > > > > +\t\tstruct v4l2_frmivalenum frameInterval = {};\n> > > > > +\t\tframeInterval.index = index;\n> > > > > +\t\tframeInterval.pixel_format = pixelFormat;\n> > > > > +\t\tframeInterval.width = size.width;\n> > > > > +\t\tframeInterval.height = size.height;\n> > > > > +\n> > > > > +\t\tret = ioctl(VIDIOC_ENUM_FRAMEINTERVALS, &frameInterval);\n> > > > > +\t\tif (ret)\n> > > > > +\t\t\tbreak;\n> > > > > +\n> > > > > +\t\tswitch (frameInterval.type) {\n> > > > > +\t\tcase V4L2_FRMIVAL_TYPE_DISCRETE: {\n> > > > > +\t\t\tauto ms = v4l2FractionToMs(frameInterval.discrete);\n> > > > > +\n> > > > > +\t\t\tmin = std::min(min, ms);\n> > > > > +\t\t\tmax = std::max(max, ms);\n> > > > > +\t\t\tbreak;\n> > > > > +\t\t}\n> > > > > +\t\tcase V4L2_FRMIVAL_TYPE_CONTINUOUS:\n> > > > > +\t\tcase V4L2_FRMIVAL_TYPE_STEPWISE: {\n> > > > > +\t\t\tmin = std::min(min, v4l2FractionToMs(frameInterval.stepwise.min));\n> > > > > +\t\t\tmax = std::max(max, v4l2FractionToMs(frameInterval.stepwise.max));\n> > > > > +\t\t\tbreak;\n> > > > > +\t\t}\n> > > >\n> > > > You know, I'm still not sure if we should support anything != DISCRETE.\n> > > >\n> > > > If I'm not mistaken UVC will always be DISCRETE. Every other device\n> > > > that uses this function is likely something that we don't want to\n> > > > support (because we want the drivers not to use this API for complex\n> > > > devices).\n> > >\n> > > UVC can have stepwise: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/media/usb/uvc/uvc_v4l2.c?id=0048fbb4011ec55c32d3148b2cda56433f273375#n1001\n> > >\n> >\n> > Argh\n> >\n> > >\n> > > >\n> > > > The same could be said for VIDEO_OUTPUT/OUTPUT_MPLANE, this will only\n> > > > apply to codecs ?\n> > >\n> > > That's probably true, but since it was easy to handle those cases, I saw\n> > > no reason to intentionally omit them.\n> > >\n> >\n> > Well, I'm always in the \"the less code we have, the less code can\n> > break\" camp, but I guess that's not too drammatic\n> >\n> > >\n> > > >\n> > > > > +\t\tdefault:\n> > > > > +\t\t\tLOG(V4L2, Error)\n> > > > > +\t\t\t\t<< \"Unknown v4l2_frmsizetypes value \"\n> > > > > +\t\t\t\t<< frameInterval.type;\n> > > > > +\t\t\treturn {};\n> > > > > +\t\t}\n> > > > > +\t}\n> > > > > +\n> > > > > +\tif (ret && ret != -EINVAL) {\n> > > >\n> > > > I might have missed why -EINVAL is ok\n> > >\n> > > When the index is one more than the index of the last available frame duration,\n> > > then EINVAL is returned, which breaks the loop, but it is not an error condition.\n> > >\n> >\n> > Ah, right right\n> >\n> > >\n> > > >\n> > > > > +\t\tLOG(V4L2, Error)\n> > > > > +\t\t\t<< \"Unable to enumerate pixel formats: \"\n> > > > > +\t\t\t<< strerror(-ret);\n> > > > > +\t\treturn {};\n> > > > > +\t}\n> > > > > +\n> > > > > +\tif (index <= 0)\n> > > >\n> > > > How can index be < 0 ?\n> > >\n> > > Admittedly it can't... I just like to do `<= 0` regardless of type.\n> >\n> > Which creates confusion in the reader which might ask \"what am I\n> > missing that could lead to index be < 0\" ?\n>\n> Okay, I'll change it.\n>\n\nThanks, it's a tiny detail anyway\n\nThanks\n j\n\n>\n> >\n> > >\n> > >\n> > > >\n> > > > > +\t\treturn {};\n> > > > > +\n> > > > > +\treturn {{ min, max }};\n> > > > > +}\n> > > > > +\n> > > > > std::vector V4L2VideoDevice::enumPixelformats(uint32_t code)\n> > > > > {\n> > > > > \tstd::vector formats;\n> > > > > --\n> > > > > 2.52.0\n> > > > >\n> > >\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id AE2E2BD1F1\n\tfor ;\n\tThu, 11 Dec 2025 11:55:16 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 720AF615D2;\n\tThu, 11 Dec 2025 12:55:15 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 3AE0A609D8\n\tfor ;\n\tThu, 11 Dec 2025 12:55:14 +0100 (CET)","from ideasonboard.com (93-46-82-201.ip106.fastwebnet.it\n\t[93.46.82.201])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 00014667;\n\tThu, 11 Dec 2025 12:55:11 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"OOl/dNvY\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1765454112;\n\tbh=wGYY7w3Q1xNAZhg/gbNLO8NXebbpjkBOouCMZg7THBM=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=OOl/dNvYOibQqnQitboFVWHHljyY9a2Kro3uQ3ienYLKY/m6xT+EP9fGLYe4ipola\n\tu4/0dirfHi22m8zcyNPFDTFhJfk62yaj+uv0bndNFQBS6qSs3scND9SaJT0T1nZgat\n\tUSdxoHMywYnXytw7taqjPUMtwrlsTX5pDqjAT49Q=","Date":"Thu, 11 Dec 2025 12:55:11 +0100","From":"Jacopo Mondi ","To":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","Cc":"Jacopo Mondi , \n\tlibcamera-devel@lists.libcamera.org","Subject":"Re: [RFC PATCH v1 2/2] libcamera: pipeline: uvcvideo: Handle\n\t`FrameDurationLimits`","Message-ID":"","References":"<20251210133704.2711629-1-barnabas.pocze@ideasonboard.com>\n\t<20251210133704.2711629-3-barnabas.pocze@ideasonboard.com>\n\t<3u36wmmvt4np2bslrb5hjb6caouuhyjh3fbc7ysf4mmvenrhlw@nsvma7gi2c6e>\n\t<58222843-07aa-45a8-9740-bd481b76c1e8@ideasonboard.com>\n\t\n\t<7fa0d67c-96f6-4440-9055-d03fcb6938ea@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<7fa0d67c-96f6-4440-9055-d03fcb6938ea@ideasonboard.com>","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":37306,"web_url":"https://patchwork.libcamera.org/comment/37306/","msgid":"<660ae4f3-c6ab-4a19-a5ec-5540c0a4c689@ideasonboard.com>","date":"2025-12-11T12:21:38","subject":"Re: [RFC PATCH v1 2/2] libcamera: pipeline: uvcvideo: Handle\n\t`FrameDurationLimits`","submitter":{"id":216,"url":"https://patchwork.libcamera.org/api/people/216/","name":"Barnabás Pőcze","email":"barnabas.pocze@ideasonboard.com"},"content":"2025. 12. 11. 12:55 keltezéssel, Jacopo Mondi írta:\n> Hi Barnabás\n> \n> On Thu, Dec 11, 2025 at 10:38:19AM +0100, Barnabás Pőcze wrote:\n>> Hi\n>>\n>> 2025. 12. 11. 10:15 keltezéssel, Jacopo Mondi írta:\n>>> Hi Barnabás\n>>>\n>>> On Wed, Dec 10, 2025 at 05:56:01PM +0100, Barnabás Pőcze wrote:\n>>>> Hi\n>>>>\n>>>> 2025. 12. 10. 17:04 keltezéssel, Jacopo Mondi írta:\n>>>>> Hi Barnabás\n>>>>>\n>>>>> On Wed, Dec 10, 2025 at 02:37:04PM +0100, Barnabás Pőcze wrote:\n>>>>>> Setting a frame rate is possible with UVC devices using `VIDIOC_S_PARM`.\n>>>>>> However, compared to other platforms supported by libcamera, limitations\n>>>>>> apply. Probably most crucially, the desired frame rate cannot be set\n>>>>>> while the camera is streaming. Furthermore, it is only a single value\n>>>>>> and not an allowed range.\n>>>>>>\n>>>>>> So this change only adds support for `FrameDurationLimits` in the control\n>>>>>> list passed to `Camera::start()`, and the control is otherwise ignored in\n>>>>>> requests.\n>>>>>>\n>>>>>> The kernel interface also only allows a single number and not a range,\n>>>>>> so the midpoint of the desired range is used. Checking the supplied\n>>>>>\n>>>>> Everything else sounds fine, but I wonder if taking the midpoint is\n>>>>> the best solution here or we should simply use the shortest provided\n>>>>> duration, warn the user, and let the kernel adjust it to the closest\n>>>>> supported value.\n>>>>\n>>>> As far as I understand, the kernel selects the closest valid frame duration\n>>>> value. Which means that using the midpoint should guarantee that if the device\n>>>> supports at least one frame duration value in the desired range, then a value\n>>>> in the desired range will be selected on the device. This is not guaranteed if\n>>>> either the min or max is sent to the kernel. That's why I chose this behaviour;\n>>>> I believe it's probably best to select a frame duration from the desired range,\n>>>> if possible.\n>>>\n>>> Well, you have a cache of those now, don't you ?\n>>\n>> Only the min/max is stored for a particular (pixel format, size) combination,\n>> and that is only done to be able to report it in the `ControlInfo` when the\n>> configuration changes.\n> \n> Right, sorry, I've overlooked it\n> \n>>\n>> Even if there all the frame interval options were stored, I still think selecting\n>> the midpoint is the best that we can do. Unless I am missing something?\n>>\n> \n> I'm in two minds here.\n> \n> I'm fine if we select the midpoint, but I would log the user about\n> what we're doing (and about the fact uvc doesn't support frame\n> duration ranges)\n\nI have already added a debug message to V4L2VideoDevice::setFrameInterval that shows\nthe wanted interval as well as the actual interval that was set on the device, for the\nnext version.\n\nDo you envision we need a \"warning\" or \"debug\" message in uvcvideo.cpp as well?\nMy thinking is that such a warning does not seem actionable for either the user\nor application developer. I believe we don't want application developers to have\nto special case uvc cameras just to avoid a warning, and in that case it's most\nlikely not actionable for the user either, unless they are in an app like camshark\nwhere they have direct control over such settings.\n\n\n> \n>>\n>>>\n>>>>\n>>>>\n>>>>>\n>>>>>> values is not necessary since the kernel will adjust the value if\n>>>>>> it is not supported by the device.\n>>>>>>\n>>>>>> Initially the global min/max values are advertised in the `ControlInfo`\n>>>>>> of the `FrameDurationLimits` control, which are then updated after\n>>>>>> the camera is configured. Updating the control limits after configuration\n>>>>>> matches the behaviour of other platforms.\n>>>>>>\n>>>>>> While the kernel interface differentiates three types of frame intervals\n>>>>>> (discrete, continuous, stepwise), when querying the available frame intervals\n>>>>>> for a given (pixel format, size) combination, all options are evaluated\n>>>>>> and only the \"local\" minimum and maximum is used, as that is the only way\n>>>>>> the limits can reasonably be advertised on the libcamera side.\n>>>>>>\n>>>>>> Closes: https://gitlab.freedesktop.org/camera/libcamera/-/issues/296\n>>>>>> Signed-off-by: Barnabás Pőcze \n>>>>>> ---\n>>>>>> include/libcamera/internal/v4l2_videodevice.h | 3 +\n>>>>>> src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 59 +++++++++\n>>>>>> src/libcamera/v4l2_videodevice.cpp | 116 ++++++++++++++++++\n>>>>>> 3 files changed, 178 insertions(+)\n>>>>>>\n>>>>>> diff --git a/include/libcamera/internal/v4l2_videodevice.h b/include/libcamera/internal/v4l2_videodevice.h\n>>>>>> index 82d98184ed..e97c0f9bf8 100644\n>>>>>> --- a/include/libcamera/internal/v4l2_videodevice.h\n>>>>>> +++ b/include/libcamera/internal/v4l2_videodevice.h\n>>>>>> @@ -209,6 +209,9 @@ public:\n> [...]\n>>>>>> diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n>>>>>> index 3f98e8ece0..e4e9b8ab9b 100644\n>>>>>> --- a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n>>>>>> +++ b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n>>>>>> @@ -57,6 +57,7 @@ public:\n> [...]\n>>>>>> diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp\n>>>>>> index 3836dabef3..3db6e80aed 100644\n>>>>>> --- a/src/libcamera/v4l2_videodevice.cpp\n>>>>>> +++ b/src/libcamera/v4l2_videodevice.cpp\n>>>>>> @@ -1202,6 +1202,122 @@ int V4L2VideoDevice::getFrameInterval(std::chrono::microseconds *interval)\n>>>>>> \treturn 0;\n>>>>>> }\n>>>>>>\n>>>>>> +/**\n>>>>>> + * \\brief Configure the frame interval\n>>>>>> + * \\param[inout] interval The frame interval\n>>>>>\n>>>>> unit would be nice here as well\n>>>>\n>>>> Ok.\n>>>>\n>>>>>\n>>>>>> + *\n>>>>>> + * Apply the supplied \\a interval as the time-per-frame stream parameter\n>>>>>> + * on the device, and return the actually applied value.\n>>>>>> + *\n>>>>>> + * \\return 0 on success or a negative error code otherwise\n>>>>>> + */\n>>>>>> +int V4L2VideoDevice::setFrameInterval(std::chrono::microseconds *interval)\n>>>>>> +{\n>>>>>> +\tv4l2_fract *frameInterval = nullptr;\n>>>>>> +\tuint32_t *caps = nullptr;\n>>>>>> +\tv4l2_streamparm sparm = {};\n>>>>>> +\n>>>>>> +\tsparm.type = bufferType_;\n>>>>>> +\n>>>>>> +\tswitch (sparm.type) {\n>>>>>> +\tcase V4L2_BUF_TYPE_VIDEO_CAPTURE:\n>>>>>> +\tcase V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE:\n>>>>>> +\t\tframeInterval = &sparm.parm.capture.timeperframe;\n>>>>>> +\t\tcaps = &sparm.parm.capture.capability;\n>>>>>> +\t\tbreak;\n>>>>>> +\tcase V4L2_BUF_TYPE_VIDEO_OUTPUT:\n>>>>>> +\tcase V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE:\n>>>>>> +\t\tframeInterval = &sparm.parm.output.timeperframe;\n>>>>>> +\t\tcaps = &sparm.parm.output.capability;\n>>>>>> +\t\tbreak;\n>>>>>> +\t}\n>>>>>> +\n>>>>>> +\tif (!frameInterval)\n>>>>>> +\t\treturn -EINVAL;\n>>>>>> +\n>>>>>> +\tconstexpr auto max = std::numeric_limitsnumerator)>::max();\n>>>>>\n>>>>> Feels quite a corner case, but checking doesn't hurt ?\n>>>>>\n>>>>>> +\tif (interval->count() <= 0 || interval->count() > max)\n>>>>>> +\t\treturn -EINVAL;\n>>>>>> +\n>>>>>> +\tframeInterval->numerator = interval->count();\n>>>>>> +\tframeInterval->denominator = std::chrono::microseconds(std::chrono::seconds(1)).count();\n>>>>>> +\n>>>>>> +\tint ret = ioctl(VIDIOC_S_PARM, &sparm);\n>>>>>> +\tif (ret)\n>>>>>> +\t\treturn ret;\n>>>>>> +\n>>>>>> +\tif (!(*caps & V4L2_CAP_TIMEPERFRAME))\n>>>>>> +\t\treturn -ENOTSUP;\n>>>>>\n>>>>> In this case, I' pretty sure you should check this flag before calling\n>>>>> ioctl()\n>>>>\n>>>> I think it's fine. If `V4L2_CAP_TIMEPERFRAME` is not present, then nothing\n>>>> (should) changes on the device and 0 is returned, in which case this condition\n>>>> detects that; or the driver returns an error code, which is detected above.\n>>>\n>>> Why going through the trouble of an ioctl if we know that it's not\n>>> supported ? Ah! or is caps only updated -after- the ioctl call ?\n>>\n>> The ioctl updates the capabilities, yes. I think this could be checked in `open()` and remembered.\n>> Would that be better?\n> \n> The V4L2_CAP_TIMEPERFRAME cap is found in the v4l2_streamparm\n> structure. Can you get it from other caps at open() time or are you\n> considering issueing a g_parm ioctl() to check if it is supported in\n> open() ?\n> \n\nI was thinking about doing a `VIDIOC_G_PARM` in `open()`, I'm not sure if\nthis capability can be determined in any other way.\n\n\n> [...]","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 6EB56C3257\n\tfor ;\n\tThu, 11 Dec 2025 12:21:45 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 8C1B9615FF;\n\tThu, 11 Dec 2025 13:21:44 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 7F132609D8\n\tfor ;\n\tThu, 11 Dec 2025 13:21:42 +0100 (CET)","from [192.168.33.38] (185.221.143.114.nat.pool.zt.hu\n\t[185.221.143.114])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 571E01352;\n\tThu, 11 Dec 2025 13:21:40 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"FAzt02VV\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1765455700;\n\tbh=hkcmBzF2p4nmleUHB8GV0lxTT9Uj4P+VUkqXqSrSWCk=;\n\th=Date:Subject:To:Cc:References:From:In-Reply-To:From;\n\tb=FAzt02VV+vgy1qMIlDuhYnWI6DmuR3zCtlntdtJFCAgacfqQTROKSBjTSXXWJoXxJ\n\todulkNo817o5+Nql8ivvb7NZXoITVCmuw47lshvyUKEEvKyORRdGpLkg0rF/YTYYkT\n\t0GEDf36xTfT5YjYvv3QK4cyY0HTZZwFIV4gPJGO4=","Message-ID":"<660ae4f3-c6ab-4a19-a5ec-5540c0a4c689@ideasonboard.com>","Date":"Thu, 11 Dec 2025 13:21:38 +0100","MIME-Version":"1.0","User-Agent":"Mozilla Thunderbird","Subject":"Re: [RFC PATCH v1 2/2] libcamera: pipeline: uvcvideo: Handle\n\t`FrameDurationLimits`","To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","References":"<20251210133704.2711629-1-barnabas.pocze@ideasonboard.com>\n\t<20251210133704.2711629-3-barnabas.pocze@ideasonboard.com>\n\t<3u36wmmvt4np2bslrb5hjb6caouuhyjh3fbc7ysf4mmvenrhlw@nsvma7gi2c6e>\n\t<58222843-07aa-45a8-9740-bd481b76c1e8@ideasonboard.com>\n\t\n\t<7fa0d67c-96f6-4440-9055-d03fcb6938ea@ideasonboard.com>\n\t","From":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","Content-Language":"en-US, hu-HU","In-Reply-To":"","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"8bit","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":37307,"web_url":"https://patchwork.libcamera.org/comment/37307/","msgid":"","date":"2025-12-11T13:35:50","subject":"Re: [RFC PATCH v1 2/2] libcamera: pipeline: uvcvideo: Handle\n\t`FrameDurationLimits`","submitter":{"id":143,"url":"https://patchwork.libcamera.org/api/people/143/","name":"Jacopo Mondi","email":"jacopo.mondi@ideasonboard.com"},"content":"Hi Barnabás\n\nOn Thu, Dec 11, 2025 at 01:21:38PM +0100, Barnabás Pőcze wrote:\n> 2025. 12. 11. 12:55 keltezéssel, Jacopo Mondi írta:\n> > Hi Barnabás\n> >\n> > On Thu, Dec 11, 2025 at 10:38:19AM +0100, Barnabás Pőcze wrote:\n> > > Hi\n> > >\n> > > 2025. 12. 11. 10:15 keltezéssel, Jacopo Mondi írta:\n> > > > Hi Barnabás\n> > > >\n> > > > On Wed, Dec 10, 2025 at 05:56:01PM +0100, Barnabás Pőcze wrote:\n> > > > > Hi\n> > > > >\n> > > > > 2025. 12. 10. 17:04 keltezéssel, Jacopo Mondi írta:\n> > > > > > Hi Barnabás\n> > > > > >\n> > > > > > On Wed, Dec 10, 2025 at 02:37:04PM +0100, Barnabás Pőcze wrote:\n> > > > > > > Setting a frame rate is possible with UVC devices using `VIDIOC_S_PARM`.\n> > > > > > > However, compared to other platforms supported by libcamera, limitations\n> > > > > > > apply. Probably most crucially, the desired frame rate cannot be set\n> > > > > > > while the camera is streaming. Furthermore, it is only a single value\n> > > > > > > and not an allowed range.\n> > > > > > >\n> > > > > > > So this change only adds support for `FrameDurationLimits` in the control\n> > > > > > > list passed to `Camera::start()`, and the control is otherwise ignored in\n> > > > > > > requests.\n> > > > > > >\n> > > > > > > The kernel interface also only allows a single number and not a range,\n> > > > > > > so the midpoint of the desired range is used. Checking the supplied\n> > > > > >\n> > > > > > Everything else sounds fine, but I wonder if taking the midpoint is\n> > > > > > the best solution here or we should simply use the shortest provided\n> > > > > > duration, warn the user, and let the kernel adjust it to the closest\n> > > > > > supported value.\n> > > > >\n> > > > > As far as I understand, the kernel selects the closest valid frame duration\n> > > > > value. Which means that using the midpoint should guarantee that if the device\n> > > > > supports at least one frame duration value in the desired range, then a value\n> > > > > in the desired range will be selected on the device. This is not guaranteed if\n> > > > > either the min or max is sent to the kernel. That's why I chose this behaviour;\n> > > > > I believe it's probably best to select a frame duration from the desired range,\n> > > > > if possible.\n> > > >\n> > > > Well, you have a cache of those now, don't you ?\n> > >\n> > > Only the min/max is stored for a particular (pixel format, size) combination,\n> > > and that is only done to be able to report it in the `ControlInfo` when the\n> > > configuration changes.\n> >\n> > Right, sorry, I've overlooked it\n> >\n> > >\n> > > Even if there all the frame interval options were stored, I still think selecting\n> > > the midpoint is the best that we can do. Unless I am missing something?\n> > >\n> >\n> > I'm in two minds here.\n> >\n> > I'm fine if we select the midpoint, but I would log the user about\n> > what we're doing (and about the fact uvc doesn't support frame\n> > duration ranges)\n>\n> I have already added a debug message to V4L2VideoDevice::setFrameInterval that shows\n> the wanted interval as well as the actual interval that was set on the device, for the\n> next version.\n>\n> Do you envision we need a \"warning\" or \"debug\" message in uvcvideo.cpp as well?\n\nThat's what I was thinking about\n\n> My thinking is that such a warning does not seem actionable for either the user\n> or application developer. I believe we don't want application developers to have\n> to special case uvc cameras just to avoid a warning, and in that case it's most\n> likely not actionable for the user either, unless they are in an app like camshark\n> where they have direct control over such settings.\n\nThat's true, it can't be actioned, but it will provide users an\nindication that what they are asking for (a ranging FPS) cannot be\nobtained.\n\nUp to you, I really don't have a strong opinion here\n\n>\n>\n> >\n> > >\n> > > >\n> > > > >\n> > > > >\n> > > > > >\n> > > > > > > values is not necessary since the kernel will adjust the value if\n> > > > > > > it is not supported by the device.\n> > > > > > >\n> > > > > > > Initially the global min/max values are advertised in the `ControlInfo`\n> > > > > > > of the `FrameDurationLimits` control, which are then updated after\n> > > > > > > the camera is configured. Updating the control limits after configuration\n> > > > > > > matches the behaviour of other platforms.\n> > > > > > >\n> > > > > > > While the kernel interface differentiates three types of frame intervals\n> > > > > > > (discrete, continuous, stepwise), when querying the available frame intervals\n> > > > > > > for a given (pixel format, size) combination, all options are evaluated\n> > > > > > > and only the \"local\" minimum and maximum is used, as that is the only way\n> > > > > > > the limits can reasonably be advertised on the libcamera side.\n> > > > > > >\n> > > > > > > Closes: https://gitlab.freedesktop.org/camera/libcamera/-/issues/296\n> > > > > > > Signed-off-by: Barnabás Pőcze \n> > > > > > > ---\n> > > > > > > include/libcamera/internal/v4l2_videodevice.h | 3 +\n> > > > > > > src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 59 +++++++++\n> > > > > > > src/libcamera/v4l2_videodevice.cpp | 116 ++++++++++++++++++\n> > > > > > > 3 files changed, 178 insertions(+)\n> > > > > > >\n> > > > > > > diff --git a/include/libcamera/internal/v4l2_videodevice.h b/include/libcamera/internal/v4l2_videodevice.h\n> > > > > > > index 82d98184ed..e97c0f9bf8 100644\n> > > > > > > --- a/include/libcamera/internal/v4l2_videodevice.h\n> > > > > > > +++ b/include/libcamera/internal/v4l2_videodevice.h\n> > > > > > > @@ -209,6 +209,9 @@ public:\n> > [...]\n> > > > > > > diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> > > > > > > index 3f98e8ece0..e4e9b8ab9b 100644\n> > > > > > > --- a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> > > > > > > +++ b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> > > > > > > @@ -57,6 +57,7 @@ public:\n> > [...]\n> > > > > > > diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp\n> > > > > > > index 3836dabef3..3db6e80aed 100644\n> > > > > > > --- a/src/libcamera/v4l2_videodevice.cpp\n> > > > > > > +++ b/src/libcamera/v4l2_videodevice.cpp\n> > > > > > > @@ -1202,6 +1202,122 @@ int V4L2VideoDevice::getFrameInterval(std::chrono::microseconds *interval)\n> > > > > > > \treturn 0;\n> > > > > > > }\n> > > > > > >\n> > > > > > > +/**\n> > > > > > > + * \\brief Configure the frame interval\n> > > > > > > + * \\param[inout] interval The frame interval\n> > > > > >\n> > > > > > unit would be nice here as well\n> > > > >\n> > > > > Ok.\n> > > > >\n> > > > > >\n> > > > > > > + *\n> > > > > > > + * Apply the supplied \\a interval as the time-per-frame stream parameter\n> > > > > > > + * on the device, and return the actually applied value.\n> > > > > > > + *\n> > > > > > > + * \\return 0 on success or a negative error code otherwise\n> > > > > > > + */\n> > > > > > > +int V4L2VideoDevice::setFrameInterval(std::chrono::microseconds *interval)\n> > > > > > > +{\n> > > > > > > +\tv4l2_fract *frameInterval = nullptr;\n> > > > > > > +\tuint32_t *caps = nullptr;\n> > > > > > > +\tv4l2_streamparm sparm = {};\n> > > > > > > +\n> > > > > > > +\tsparm.type = bufferType_;\n> > > > > > > +\n> > > > > > > +\tswitch (sparm.type) {\n> > > > > > > +\tcase V4L2_BUF_TYPE_VIDEO_CAPTURE:\n> > > > > > > +\tcase V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE:\n> > > > > > > +\t\tframeInterval = &sparm.parm.capture.timeperframe;\n> > > > > > > +\t\tcaps = &sparm.parm.capture.capability;\n> > > > > > > +\t\tbreak;\n> > > > > > > +\tcase V4L2_BUF_TYPE_VIDEO_OUTPUT:\n> > > > > > > +\tcase V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE:\n> > > > > > > +\t\tframeInterval = &sparm.parm.output.timeperframe;\n> > > > > > > +\t\tcaps = &sparm.parm.output.capability;\n> > > > > > > +\t\tbreak;\n> > > > > > > +\t}\n> > > > > > > +\n> > > > > > > +\tif (!frameInterval)\n> > > > > > > +\t\treturn -EINVAL;\n> > > > > > > +\n> > > > > > > +\tconstexpr auto max = std::numeric_limitsnumerator)>::max();\n> > > > > >\n> > > > > > Feels quite a corner case, but checking doesn't hurt ?\n> > > > > >\n> > > > > > > +\tif (interval->count() <= 0 || interval->count() > max)\n> > > > > > > +\t\treturn -EINVAL;\n> > > > > > > +\n> > > > > > > +\tframeInterval->numerator = interval->count();\n> > > > > > > +\tframeInterval->denominator = std::chrono::microseconds(std::chrono::seconds(1)).count();\n> > > > > > > +\n> > > > > > > +\tint ret = ioctl(VIDIOC_S_PARM, &sparm);\n> > > > > > > +\tif (ret)\n> > > > > > > +\t\treturn ret;\n> > > > > > > +\n> > > > > > > +\tif (!(*caps & V4L2_CAP_TIMEPERFRAME))\n> > > > > > > +\t\treturn -ENOTSUP;\n> > > > > >\n> > > > > > In this case, I' pretty sure you should check this flag before calling\n> > > > > > ioctl()\n> > > > >\n> > > > > I think it's fine. If `V4L2_CAP_TIMEPERFRAME` is not present, then nothing\n> > > > > (should) changes on the device and 0 is returned, in which case this condition\n> > > > > detects that; or the driver returns an error code, which is detected above.\n> > > >\n> > > > Why going through the trouble of an ioctl if we know that it's not\n> > > > supported ? Ah! or is caps only updated -after- the ioctl call ?\n> > >\n> > > The ioctl updates the capabilities, yes. I think this could be checked in `open()` and remembered.\n> > > Would that be better?\n> >\n> > The V4L2_CAP_TIMEPERFRAME cap is found in the v4l2_streamparm\n> > structure. Can you get it from other caps at open() time or are you\n> > considering issueing a g_parm ioctl() to check if it is supported in\n> > open() ?\n> >\n>\n> I was thinking about doing a `VIDIOC_G_PARM` in `open()`, I'm not sure if\n> this capability can be determined in any other way.\n\nConsidering that g/s_parm will only be called on uvc, I would spare\ncalling it at open() time for all other platforms.\n\nThanks\n j\n\n>\n>\n> > [...]\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 19494BD1F1\n\tfor ;\n\tThu, 11 Dec 2025 13:35:58 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 3E84B615FF;\n\tThu, 11 Dec 2025 14:35:57 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id DCB44609D8\n\tfor ;\n\tThu, 11 Dec 2025 14:35:54 +0100 (CET)","from ideasonboard.com (93-46-82-201.ip106.fastwebnet.it\n\t[93.46.82.201])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id B5E21667;\n\tThu, 11 Dec 2025 14:35:51 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"Q356bDYR\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1765460152;\n\tbh=0Gl2btN0yGkCt9fQfvPTykdppAV/iGdMDN2oMKBX9CE=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=Q356bDYRgv+aP3PvVwjV60BdTis3CAHULd7TyOEJAnAqcQ3+zZJavFWsyOX89UucZ\n\tX/orn+AW69LFQkETKgZO6qxwBZiW8jCHfXnhmeK9gPo8kpYIsLqsU4CwKDujed9di7\n\txM1iXN4fpwDItyGzBAzLwWL/nnA9z2Tt0sF2lYR4=","Date":"Thu, 11 Dec 2025 14:35:50 +0100","From":"Jacopo Mondi ","To":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","Cc":"Jacopo Mondi , \n\tlibcamera-devel@lists.libcamera.org","Subject":"Re: [RFC PATCH v1 2/2] libcamera: pipeline: uvcvideo: Handle\n\t`FrameDurationLimits`","Message-ID":"","References":"<20251210133704.2711629-1-barnabas.pocze@ideasonboard.com>\n\t<20251210133704.2711629-3-barnabas.pocze@ideasonboard.com>\n\t<3u36wmmvt4np2bslrb5hjb6caouuhyjh3fbc7ysf4mmvenrhlw@nsvma7gi2c6e>\n\t<58222843-07aa-45a8-9740-bd481b76c1e8@ideasonboard.com>\n\t\n\t<7fa0d67c-96f6-4440-9055-d03fcb6938ea@ideasonboard.com>\n\t\n\t<660ae4f3-c6ab-4a19-a5ec-5540c0a4c689@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<660ae4f3-c6ab-4a19-a5ec-5540c0a4c689@ideasonboard.com>","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]