[RFC,v1,2/2] libcamera: pipeline: uvcvideo: Handle `FrameDurationLimits`

25456 diff mbox series

Setting a frame rate is possible with UVC devices using `VIDIOC_S_PARM`.
However, compared to other platforms supported by libcamera, limitations
apply. Probably most crucially, the desired frame rate cannot be set
while the camera is streaming. Furthermore, it is only a single value
and not an allowed range.

So this change only adds support for `FrameDurationLimits` in the control
list passed to `Camera::start()`, and the control is otherwise ignored in
requests.

The kernel interface also only allows a single number and not a range,
so the midpoint of the desired range is used. Checking the supplied
values is not necessary since the kernel will adjust the value if
it is not supported by the device.

Initially the global min/max values are advertised in the `ControlInfo`
of the `FrameDurationLimits` control, which are then updated after
the camera is configured. Updating the control limits after configuration
matches the behaviour of other platforms.

While the kernel interface differentiates three types of frame intervals
(discrete, continuous, stepwise), when querying the available frame intervals
for a given (pixel format, size) combination, all options are evaluated
and only the "local" minimum and maximum is used, as that is the only way
the limits can reasonably be advertised on the libcamera side.

Closes: https://gitlab.freedesktop.org/camera/libcamera/-/issues/296
Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
---
 include/libcamera/internal/v4l2_videodevice.h |   3 +
 src/libcamera/pipeline/uvcvideo/uvcvideo.cpp  |  59 +++++++++
 src/libcamera/v4l2_videodevice.cpp            | 116 ++++++++++++++++++
 3 files changed, 178 insertions(+)

Hi Barnabás

On Wed, Dec 10, 2025 at 02:37:04PM +0100, Barnabás Pőcze wrote:
> Setting a frame rate is possible with UVC devices using `VIDIOC_S_PARM`.
> However, compared to other platforms supported by libcamera, limitations
> apply. Probably most crucially, the desired frame rate cannot be set
> while the camera is streaming. Furthermore, it is only a single value
> and not an allowed range.
>
> So this change only adds support for `FrameDurationLimits` in the control
> list passed to `Camera::start()`, and the control is otherwise ignored in
> requests.
>
> The kernel interface also only allows a single number and not a range,
> so the midpoint of the desired range is used. Checking the supplied

Everything else sounds fine, but I wonder if taking the midpoint is
the best solution here or we should simply use the shortest provided
duration, warn the user, and let the kernel adjust it to the closest
supported value.

> values is not necessary since the kernel will adjust the value if
> it is not supported by the device.
>
> Initially the global min/max values are advertised in the `ControlInfo`
> of the `FrameDurationLimits` control, which are then updated after
> the camera is configured. Updating the control limits after configuration
> matches the behaviour of other platforms.
>
> While the kernel interface differentiates three types of frame intervals
> (discrete, continuous, stepwise), when querying the available frame intervals
> for a given (pixel format, size) combination, all options are evaluated
> and only the "local" minimum and maximum is used, as that is the only way
> the limits can reasonably be advertised on the libcamera side.
>
> Closes: https://gitlab.freedesktop.org/camera/libcamera/-/issues/296
> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
> ---
>  include/libcamera/internal/v4l2_videodevice.h |   3 +
>  src/libcamera/pipeline/uvcvideo/uvcvideo.cpp  |  59 +++++++++
>  src/libcamera/v4l2_videodevice.cpp            | 116 ++++++++++++++++++
>  3 files changed, 178 insertions(+)
>
> diff --git a/include/libcamera/internal/v4l2_videodevice.h b/include/libcamera/internal/v4l2_videodevice.h
> index 82d98184ed..e97c0f9bf8 100644
> --- a/include/libcamera/internal/v4l2_videodevice.h
> +++ b/include/libcamera/internal/v4l2_videodevice.h
> @@ -209,6 +209,9 @@ public:
>  	Formats formats(uint32_t code = 0);
>
>  	int getFrameInterval(std::chrono::microseconds *interval);
> +	int setFrameInterval(std::chrono::microseconds *interval);
> +	std::optional<std::array<std::chrono::microseconds, 2>>
> +	getFrameIntervalLimits(V4L2PixelFormat pixelFormat, Size size);
>
>  	int getSelection(unsigned int target, Rectangle *rect);
>  	int setSelection(unsigned int target, Rectangle *rect);
> diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
> index 3f98e8ece0..e4e9b8ab9b 100644
> --- a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
> +++ b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
> @@ -57,6 +57,7 @@ public:
>  	std::unique_ptr<V4L2VideoDevice> video_;
>  	Stream stream_;
>  	std::map<PixelFormat, std::vector<SizeRange>> formats_;
> +	std::map<std::pair<PixelFormat, Size>, std::array<std::chrono::microseconds, 2>> frameIntervals_;
>
>  	std::optional<v4l2_exposure_auto_type> autoExposureMode_;
>  	std::optional<v4l2_exposure_auto_type> manualExposureMode_;
> @@ -277,6 +278,22 @@ int PipelineHandlerUVC::configure(Camera *camera, CameraConfiguration *config)
>  	    format.fourcc != data->video_->toV4L2PixelFormat(cfg.pixelFormat))
>  		return -EINVAL;
>
> +	auto it = data->controlInfo_.find(&controls::FrameDurationLimits);
> +	if (it != data->controlInfo_.end()) {
> +		auto it2 = data->frameIntervals_.find({ cfg.pixelFormat, cfg.size });
> +		if (it2 != data->frameIntervals_.end()) {
> +			std::chrono::microseconds current;
> +
> +			ret = data->video_->getFrameInterval(&current);
> +
> +			it->second = ControlInfo{
> +				int64_t(it2->second[0].count()),
> +				int64_t(it2->second[1].count()),
> +				ret == 0 ? ControlValue(int64_t(current.count())) : ControlValue()
> +			};
> +		}
> +	}
> +
>  	cfg.setStream(&data->stream_);
>
>  	return 0;
> @@ -306,6 +323,19 @@ int PipelineHandlerUVC::start(Camera *camera, const ControlList *controls)
>  		ret = processControls(data, *controls);
>  		if (ret < 0)
>  			goto err_release_buffers;
> +
> +		/* Can only be set before starting. */
> +		auto fdl = controls->get(controls::FrameDurationLimits);
> +		if (fdl) {
> +			const auto wantMin = std::chrono::microseconds((*fdl)[0]);
> +			const auto wantMax = std::chrono::microseconds((*fdl)[1]);
> +			auto want = (wantMin + wantMax) / 2;
> +
> +			/* Let the kernel choose something close to the middle. */
> +			ret = data->video_->setFrameInterval(&want);
> +			if (ret == 0)
> +				data->timePerFrame_ = want;
> +		}
>  	}
>
>  	ret = data->video_->streamOn();
> @@ -355,6 +385,8 @@ int PipelineHandlerUVC::processControl(const UVCCameraData *data, ControlList *c
>  		cid = V4L2_CID_GAMMA;
>  	else if (id == controls::AeEnable)
>  		return 0; /* Handled in `Camera::queueRequest()`. */
> +	else if (id == controls::FrameDurationLimits)
> +		return 0; /* Handled in `start()` */
>  	else
>  		return -EINVAL;
>
> @@ -555,6 +587,23 @@ int UVCCameraData::init(std::shared_ptr<MediaDevice> media)
>  	 * resolution from the largest size it advertises.
>  	 */
>  	Size resolution;
> +	auto minFrameInterval = std::chrono::microseconds::max();
> +	auto maxFrameInterval = std::chrono::microseconds::min();
> +
> +	const auto processFrameIntervals = [&](PixelFormat pf, V4L2PixelFormat v4l2pf, SizeRange size) {
> +		if (size.min != size.max)
> +			return;

Can this happen ?

> +
> +		auto frameIntervals = video_->getFrameIntervalLimits(v4l2pf, size.min);
> +		if (!frameIntervals)
> +			return;
> +
> +		minFrameInterval = std::min(minFrameInterval, (*frameIntervals)[0]);
> +		maxFrameInterval = std::max(maxFrameInterval, (*frameIntervals)[1]);

Aren't you already taking the min/max in the video device helpers ?

> +
> +		frameIntervals_.try_emplace({ pf, size.min }, *frameIntervals);
> +	};
> +
>  	for (const auto &format : video_->formats()) {
>  		PixelFormat pixelFormat = format.first.toPixelFormat();
>  		if (!pixelFormat.isValid())
> @@ -566,6 +615,8 @@ int UVCCameraData::init(std::shared_ptr<MediaDevice> media)
>  		for (const SizeRange &sizeRange : sizeRanges) {
>  			if (sizeRange.max > resolution)
>  				resolution = sizeRange.max;
> +
> +			processFrameIntervals(pixelFormat, format.first, sizeRange);
>  		}
>  	}
>
> @@ -625,6 +676,14 @@ int UVCCameraData::init(std::shared_ptr<MediaDevice> media)
>  		ctrls[&controls::AeEnable] = ControlInfo(false, true, true);
>  	}
>
> +	/* Use the global min/max here, limits will be updated in `configure()`. */
> +	if (!frameIntervals_.empty()) {
> +		ctrls[&controls::FrameDurationLimits] = ControlInfo{
> +			int64_t(minFrameInterval.count()),
> +			int64_t(maxFrameInterval.count()),
> +		};
> +	}
> +
>  	controlInfo_ = ControlInfoMap(std::move(ctrls), controls::controls);
>
>  	/*
> diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp
> index 3836dabef3..3db6e80aed 100644
> --- a/src/libcamera/v4l2_videodevice.cpp
> +++ b/src/libcamera/v4l2_videodevice.cpp
> @@ -1202,6 +1202,122 @@ int V4L2VideoDevice::getFrameInterval(std::chrono::microseconds *interval)
>  	return 0;
>  }
>
> +/**
> + * \brief Configure the frame interval
> + * \param[inout] interval The frame interval

unit would be nice here as well

> + *
> + * Apply the supplied \a interval as the time-per-frame stream parameter
> + * on the device, and return the actually applied value.
> + *
> + * \return 0 on success or a negative error code otherwise
> + */
> +int V4L2VideoDevice::setFrameInterval(std::chrono::microseconds *interval)
> +{
> +	v4l2_fract *frameInterval = nullptr;
> +	uint32_t *caps = nullptr;
> +	v4l2_streamparm sparm = {};
> +
> +	sparm.type = bufferType_;
> +
> +	switch (sparm.type) {
> +	case V4L2_BUF_TYPE_VIDEO_CAPTURE:
> +	case V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE:
> +		frameInterval = &sparm.parm.capture.timeperframe;
> +		caps = &sparm.parm.capture.capability;
> +		break;
> +	case V4L2_BUF_TYPE_VIDEO_OUTPUT:
> +	case V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE:
> +		frameInterval = &sparm.parm.output.timeperframe;
> +		caps = &sparm.parm.output.capability;
> +		break;
> +	}
> +
> +	if (!frameInterval)
> +		return -EINVAL;
> +
> +	constexpr auto max = std::numeric_limits<decltype(frameInterval->numerator)>::max();

Feels quite a corner case, but checking doesn't hurt ?

> +	if (interval->count() <= 0 || interval->count() > max)
> +		return -EINVAL;
> +
> +	frameInterval->numerator = interval->count();
> +	frameInterval->denominator = std::chrono::microseconds(std::chrono::seconds(1)).count();
> +
> +	int ret = ioctl(VIDIOC_S_PARM, &sparm);
> +	if (ret)
> +		return ret;
> +
> +	if (!(*caps & V4L2_CAP_TIMEPERFRAME))
> +		return -ENOTSUP;

In this case, I' pretty sure you should check this flag before calling
ioctl()

> +
> +	*interval = v4l2FractionToMs(*frameInterval);
> +
> +	return 0;
> +}
> +
> +/**
> + * \brief Retrieve the frame interval limits
> + * \param[in] pixelFormat The pixel format
> + * \param[in] size The size
> + *
> + * Retrieve the minimum and maximum available frame interval for
> + * the given \a pixelFormat and \a size.
> + *
> + * \return The min and max frame interval or std::nullopt otherwise
> + */
> +std::optional<std::array<std::chrono::microseconds, 2>>
> +V4L2VideoDevice::getFrameIntervalLimits(V4L2PixelFormat pixelFormat, Size size)
> +{
> +	auto min = std::chrono::microseconds::max();
> +	auto max = std::chrono::microseconds::min();
> +	unsigned int index = 0;
> +	int ret;
> +
> +	for (;; index++) {
> +		struct v4l2_frmivalenum frameInterval = {};
> +		frameInterval.index = index;
> +		frameInterval.pixel_format = pixelFormat;
> +		frameInterval.width = size.width;
> +		frameInterval.height = size.height;
> +
> +		ret = ioctl(VIDIOC_ENUM_FRAMEINTERVALS, &frameInterval);
> +		if (ret)
> +			break;
> +
> +		switch (frameInterval.type) {
> +		case V4L2_FRMIVAL_TYPE_DISCRETE: {
> +			auto ms = v4l2FractionToMs(frameInterval.discrete);
> +
> +			min = std::min(min, ms);
> +			max = std::max(max, ms);
> +			break;
> +		}
> +		case V4L2_FRMIVAL_TYPE_CONTINUOUS:
> +		case V4L2_FRMIVAL_TYPE_STEPWISE: {
> +			min = std::min(min, v4l2FractionToMs(frameInterval.stepwise.min));
> +			max = std::max(max, v4l2FractionToMs(frameInterval.stepwise.max));
> +			break;
> +		}

You know, I'm still not sure if we should support anything != DISCRETE.

If I'm not mistaken UVC will always be DISCRETE. Every other device
that uses this function is likely something that we don't want to
support (because we want the drivers not to use this API for complex
devices).

The same could be said for VIDEO_OUTPUT/OUTPUT_MPLANE, this will only
apply to codecs ?

> +		default:
> +			LOG(V4L2, Error)
> +				<< "Unknown v4l2_frmsizetypes value "
> +				<< frameInterval.type;
> +			return {};
> +		}
> +	}
> +
> +	if (ret && ret != -EINVAL) {

I might have missed why -EINVAL is ok

> +		LOG(V4L2, Error)
> +			<< "Unable to enumerate pixel formats: "
> +			<< strerror(-ret);
> +		return {};
> +	}
> +
> +	if (index <= 0)

How can index be < 0 ?

> +		return {};
> +
> +	return {{ min, max }};
> +}
> +
>  std::vector<V4L2PixelFormat> V4L2VideoDevice::enumPixelformats(uint32_t code)
>  {
>  	std::vector<V4L2PixelFormat> formats;
> --
> 2.52.0
>

Hi

2025. 12. 10. 17:04 keltezéssel, Jacopo Mondi írta:
> Hi Barnabás
> 
> On Wed, Dec 10, 2025 at 02:37:04PM +0100, Barnabás Pőcze wrote:
>> Setting a frame rate is possible with UVC devices using `VIDIOC_S_PARM`.
>> However, compared to other platforms supported by libcamera, limitations
>> apply. Probably most crucially, the desired frame rate cannot be set
>> while the camera is streaming. Furthermore, it is only a single value
>> and not an allowed range.
>>
>> So this change only adds support for `FrameDurationLimits` in the control
>> list passed to `Camera::start()`, and the control is otherwise ignored in
>> requests.
>>
>> The kernel interface also only allows a single number and not a range,
>> so the midpoint of the desired range is used. Checking the supplied
> 
> Everything else sounds fine, but I wonder if taking the midpoint is
> the best solution here or we should simply use the shortest provided
> duration, warn the user, and let the kernel adjust it to the closest
> supported value.

As far as I understand, the kernel selects the closest valid frame duration
value. Which means that using the midpoint should guarantee that if the device
supports at least one frame duration value in the desired range, then a value
in the desired range will be selected on the device. This is not guaranteed if
either the min or max is sent to the kernel. That's why I chose this behaviour;
I believe it's probably best to select a frame duration from the desired range,
if possible.


> 
>> values is not necessary since the kernel will adjust the value if
>> it is not supported by the device.
>>
>> Initially the global min/max values are advertised in the `ControlInfo`
>> of the `FrameDurationLimits` control, which are then updated after
>> the camera is configured. Updating the control limits after configuration
>> matches the behaviour of other platforms.
>>
>> While the kernel interface differentiates three types of frame intervals
>> (discrete, continuous, stepwise), when querying the available frame intervals
>> for a given (pixel format, size) combination, all options are evaluated
>> and only the "local" minimum and maximum is used, as that is the only way
>> the limits can reasonably be advertised on the libcamera side.
>>
>> Closes: https://gitlab.freedesktop.org/camera/libcamera/-/issues/296
>> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
>> ---
>>   include/libcamera/internal/v4l2_videodevice.h |   3 +
>>   src/libcamera/pipeline/uvcvideo/uvcvideo.cpp  |  59 +++++++++
>>   src/libcamera/v4l2_videodevice.cpp            | 116 ++++++++++++++++++
>>   3 files changed, 178 insertions(+)
>>
>> diff --git a/include/libcamera/internal/v4l2_videodevice.h b/include/libcamera/internal/v4l2_videodevice.h
>> index 82d98184ed..e97c0f9bf8 100644
>> --- a/include/libcamera/internal/v4l2_videodevice.h
>> +++ b/include/libcamera/internal/v4l2_videodevice.h
>> @@ -209,6 +209,9 @@ public:
>>   	Formats formats(uint32_t code = 0);
>>
>>   	int getFrameInterval(std::chrono::microseconds *interval);
>> +	int setFrameInterval(std::chrono::microseconds *interval);
>> +	std::optional<std::array<std::chrono::microseconds, 2>>
>> +	getFrameIntervalLimits(V4L2PixelFormat pixelFormat, Size size);
>>
>>   	int getSelection(unsigned int target, Rectangle *rect);
>>   	int setSelection(unsigned int target, Rectangle *rect);
>> diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
>> index 3f98e8ece0..e4e9b8ab9b 100644
>> --- a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
>> +++ b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
>> @@ -57,6 +57,7 @@ public:
>>   	std::unique_ptr<V4L2VideoDevice> video_;
>>   	Stream stream_;
>>   	std::map<PixelFormat, std::vector<SizeRange>> formats_;
>> +	std::map<std::pair<PixelFormat, Size>, std::array<std::chrono::microseconds, 2>> frameIntervals_;
>>
>>   	std::optional<v4l2_exposure_auto_type> autoExposureMode_;
>>   	std::optional<v4l2_exposure_auto_type> manualExposureMode_;
>> @@ -277,6 +278,22 @@ int PipelineHandlerUVC::configure(Camera *camera, CameraConfiguration *config)
>>   	    format.fourcc != data->video_->toV4L2PixelFormat(cfg.pixelFormat))
>>   		return -EINVAL;
>>
>> +	auto it = data->controlInfo_.find(&controls::FrameDurationLimits);
>> +	if (it != data->controlInfo_.end()) {
>> +		auto it2 = data->frameIntervals_.find({ cfg.pixelFormat, cfg.size });
>> +		if (it2 != data->frameIntervals_.end()) {
>> +			std::chrono::microseconds current;
>> +
>> +			ret = data->video_->getFrameInterval(&current);
>> +
>> +			it->second = ControlInfo{
>> +				int64_t(it2->second[0].count()),
>> +				int64_t(it2->second[1].count()),
>> +				ret == 0 ? ControlValue(int64_t(current.count())) : ControlValue()
>> +			};
>> +		}
>> +	}
>> +
>>   	cfg.setStream(&data->stream_);
>>
>>   	return 0;
>> @@ -306,6 +323,19 @@ int PipelineHandlerUVC::start(Camera *camera, const ControlList *controls)
>>   		ret = processControls(data, *controls);
>>   		if (ret < 0)
>>   			goto err_release_buffers;
>> +
>> +		/* Can only be set before starting. */
>> +		auto fdl = controls->get(controls::FrameDurationLimits);
>> +		if (fdl) {
>> +			const auto wantMin = std::chrono::microseconds((*fdl)[0]);
>> +			const auto wantMax = std::chrono::microseconds((*fdl)[1]);
>> +			auto want = (wantMin + wantMax) / 2;
>> +
>> +			/* Let the kernel choose something close to the middle. */
>> +			ret = data->video_->setFrameInterval(&want);
>> +			if (ret == 0)
>> +				data->timePerFrame_ = want;
>> +		}
>>   	}
>>
>>   	ret = data->video_->streamOn();
>> @@ -355,6 +385,8 @@ int PipelineHandlerUVC::processControl(const UVCCameraData *data, ControlList *c
>>   		cid = V4L2_CID_GAMMA;
>>   	else if (id == controls::AeEnable)
>>   		return 0; /* Handled in `Camera::queueRequest()`. */
>> +	else if (id == controls::FrameDurationLimits)
>> +		return 0; /* Handled in `start()` */
>>   	else
>>   		return -EINVAL;
>>
>> @@ -555,6 +587,23 @@ int UVCCameraData::init(std::shared_ptr<MediaDevice> media)
>>   	 * resolution from the largest size it advertises.
>>   	 */
>>   	Size resolution;
>> +	auto minFrameInterval = std::chrono::microseconds::max();
>> +	auto maxFrameInterval = std::chrono::microseconds::min();
>> +
>> +	const auto processFrameIntervals = [&](PixelFormat pf, V4L2PixelFormat v4l2pf, SizeRange size) {
>> +		if (size.min != size.max)
>> +			return;
> 
> Can this happen ?

I suppose not, I will replace it with an assertion.


> 
>> +
>> +		auto frameIntervals = video_->getFrameIntervalLimits(v4l2pf, size.min);
>> +		if (!frameIntervals)
>> +			return;
>> +
>> +		minFrameInterval = std::min(minFrameInterval, (*frameIntervals)[0]);
>> +		maxFrameInterval = std::max(maxFrameInterval, (*frameIntervals)[1]);
> 
> Aren't you already taking the min/max in the video device helpers ?

This is the min/max among all pixel formats and sizes, to provide initial
min/max values in the `ControlInfo`. See below ...


> 
>> +
>> +		frameIntervals_.try_emplace({ pf, size.min }, *frameIntervals);
>> +	};
>> +
>>   	for (const auto &format : video_->formats()) {
>>   		PixelFormat pixelFormat = format.first.toPixelFormat();
>>   		if (!pixelFormat.isValid())
>> @@ -566,6 +615,8 @@ int UVCCameraData::init(std::shared_ptr<MediaDevice> media)
>>   		for (const SizeRange &sizeRange : sizeRanges) {
>>   			if (sizeRange.max > resolution)
>>   				resolution = sizeRange.max;
>> +
>> +			processFrameIntervals(pixelFormat, format.first, sizeRange);
>>   		}
>>   	}
>>
>> @@ -625,6 +676,14 @@ int UVCCameraData::init(std::shared_ptr<MediaDevice> media)
>>   		ctrls[&controls::AeEnable] = ControlInfo(false, true, true);
>>   	}
>>
>> +	/* Use the global min/max here, limits will be updated in `configure()`. */
>> +	if (!frameIntervals_.empty()) {
>> +		ctrls[&controls::FrameDurationLimits] = ControlInfo{
>> +			int64_t(minFrameInterval.count()),
>> +			int64_t(maxFrameInterval.count()),
>> +		};
>> +	}

... here the "global" limits are reported initially.


>> +
>>   	controlInfo_ = ControlInfoMap(std::move(ctrls), controls::controls);
>>
>>   	/*
>> diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp
>> index 3836dabef3..3db6e80aed 100644
>> --- a/src/libcamera/v4l2_videodevice.cpp
>> +++ b/src/libcamera/v4l2_videodevice.cpp
>> @@ -1202,6 +1202,122 @@ int V4L2VideoDevice::getFrameInterval(std::chrono::microseconds *interval)
>>   	return 0;
>>   }
>>
>> +/**
>> + * \brief Configure the frame interval
>> + * \param[inout] interval The frame interval
> 
> unit would be nice here as well

Ok.

> 
>> + *
>> + * Apply the supplied \a interval as the time-per-frame stream parameter
>> + * on the device, and return the actually applied value.
>> + *
>> + * \return 0 on success or a negative error code otherwise
>> + */
>> +int V4L2VideoDevice::setFrameInterval(std::chrono::microseconds *interval)
>> +{
>> +	v4l2_fract *frameInterval = nullptr;
>> +	uint32_t *caps = nullptr;
>> +	v4l2_streamparm sparm = {};
>> +
>> +	sparm.type = bufferType_;
>> +
>> +	switch (sparm.type) {
>> +	case V4L2_BUF_TYPE_VIDEO_CAPTURE:
>> +	case V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE:
>> +		frameInterval = &sparm.parm.capture.timeperframe;
>> +		caps = &sparm.parm.capture.capability;
>> +		break;
>> +	case V4L2_BUF_TYPE_VIDEO_OUTPUT:
>> +	case V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE:
>> +		frameInterval = &sparm.parm.output.timeperframe;
>> +		caps = &sparm.parm.output.capability;
>> +		break;
>> +	}
>> +
>> +	if (!frameInterval)
>> +		return -EINVAL;
>> +
>> +	constexpr auto max = std::numeric_limits<decltype(frameInterval->numerator)>::max();
> 
> Feels quite a corner case, but checking doesn't hurt ?
> 
>> +	if (interval->count() <= 0 || interval->count() > max)
>> +		return -EINVAL;
>> +
>> +	frameInterval->numerator = interval->count();
>> +	frameInterval->denominator = std::chrono::microseconds(std::chrono::seconds(1)).count();
>> +
>> +	int ret = ioctl(VIDIOC_S_PARM, &sparm);
>> +	if (ret)
>> +		return ret;
>> +
>> +	if (!(*caps & V4L2_CAP_TIMEPERFRAME))
>> +		return -ENOTSUP;
> 
> In this case, I' pretty sure you should check this flag before calling
> ioctl()

I think it's fine. If `V4L2_CAP_TIMEPERFRAME` is not present, then nothing
(should) changes on the device and 0 is returned, in which case this condition
detects that; or the driver returns an error code, which is detected above.


> 
>> +
>> +	*interval = v4l2FractionToMs(*frameInterval);
>> +
>> +	return 0;
>> +}
>> +
>> +/**
>> + * \brief Retrieve the frame interval limits
>> + * \param[in] pixelFormat The pixel format
>> + * \param[in] size The size
>> + *
>> + * Retrieve the minimum and maximum available frame interval for
>> + * the given \a pixelFormat and \a size.
>> + *
>> + * \return The min and max frame interval or std::nullopt otherwise
>> + */
>> +std::optional<std::array<std::chrono::microseconds, 2>>
>> +V4L2VideoDevice::getFrameIntervalLimits(V4L2PixelFormat pixelFormat, Size size)
>> +{
>> +	auto min = std::chrono::microseconds::max();
>> +	auto max = std::chrono::microseconds::min();
>> +	unsigned int index = 0;
>> +	int ret;
>> +
>> +	for (;; index++) {
>> +		struct v4l2_frmivalenum frameInterval = {};
>> +		frameInterval.index = index;
>> +		frameInterval.pixel_format = pixelFormat;
>> +		frameInterval.width = size.width;
>> +		frameInterval.height = size.height;
>> +
>> +		ret = ioctl(VIDIOC_ENUM_FRAMEINTERVALS, &frameInterval);
>> +		if (ret)
>> +			break;
>> +
>> +		switch (frameInterval.type) {
>> +		case V4L2_FRMIVAL_TYPE_DISCRETE: {
>> +			auto ms = v4l2FractionToMs(frameInterval.discrete);
>> +
>> +			min = std::min(min, ms);
>> +			max = std::max(max, ms);
>> +			break;
>> +		}
>> +		case V4L2_FRMIVAL_TYPE_CONTINUOUS:
>> +		case V4L2_FRMIVAL_TYPE_STEPWISE: {
>> +			min = std::min(min, v4l2FractionToMs(frameInterval.stepwise.min));
>> +			max = std::max(max, v4l2FractionToMs(frameInterval.stepwise.max));
>> +			break;
>> +		}
> 
> You know, I'm still not sure if we should support anything != DISCRETE.
> 
> If I'm not mistaken UVC will always be DISCRETE. Every other device
> that uses this function is likely something that we don't want to
> support (because we want the drivers not to use this API for complex
> devices).

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


> 
> The same could be said for VIDEO_OUTPUT/OUTPUT_MPLANE, this will only
> apply to codecs ?

That's probably true, but since it was easy to handle those cases, I saw
no reason to intentionally omit them.


> 
>> +		default:
>> +			LOG(V4L2, Error)
>> +				<< "Unknown v4l2_frmsizetypes value "
>> +				<< frameInterval.type;
>> +			return {};
>> +		}
>> +	}
>> +
>> +	if (ret && ret != -EINVAL) {
> 
> I might have missed why -EINVAL is ok

When the index is one more than the index of the last available frame duration,
then EINVAL is returned, which breaks the loop, but it is not an error condition.


> 
>> +		LOG(V4L2, Error)
>> +			<< "Unable to enumerate pixel formats: "
>> +			<< strerror(-ret);
>> +		return {};
>> +	}
>> +
>> +	if (index <= 0)
> 
> How can index be < 0 ?

Admittedly it can't... I just like to do `<= 0` regardless of type.


> 
>> +		return {};
>> +
>> +	return {{ min, max }};
>> +}
>> +
>>   std::vector<V4L2PixelFormat> V4L2VideoDevice::enumPixelformats(uint32_t code)
>>   {
>>   	std::vector<V4L2PixelFormat> formats;
>> --
>> 2.52.0
>>

Hi Barnabás

On Wed, Dec 10, 2025 at 05:56:01PM +0100, Barnabás Pőcze wrote:
> Hi
>
> 2025. 12. 10. 17:04 keltezéssel, Jacopo Mondi írta:
> > Hi Barnabás
> >
> > On Wed, Dec 10, 2025 at 02:37:04PM +0100, Barnabás Pőcze wrote:
> > > Setting a frame rate is possible with UVC devices using `VIDIOC_S_PARM`.
> > > However, compared to other platforms supported by libcamera, limitations
> > > apply. Probably most crucially, the desired frame rate cannot be set
> > > while the camera is streaming. Furthermore, it is only a single value
> > > and not an allowed range.
> > >
> > > So this change only adds support for `FrameDurationLimits` in the control
> > > list passed to `Camera::start()`, and the control is otherwise ignored in
> > > requests.
> > >
> > > The kernel interface also only allows a single number and not a range,
> > > so the midpoint of the desired range is used. Checking the supplied
> >
> > Everything else sounds fine, but I wonder if taking the midpoint is
> > the best solution here or we should simply use the shortest provided
> > duration, warn the user, and let the kernel adjust it to the closest
> > supported value.
>
> As far as I understand, the kernel selects the closest valid frame duration
> value. Which means that using the midpoint should guarantee that if the device
> supports at least one frame duration value in the desired range, then a value
> in the desired range will be selected on the device. This is not guaranteed if
> either the min or max is sent to the kernel. That's why I chose this behaviour;
> I believe it's probably best to select a frame duration from the desired range,
> if possible.

Well, you have a cache of those now, don't you ?

>
>
> >
> > > values is not necessary since the kernel will adjust the value if
> > > it is not supported by the device.
> > >
> > > Initially the global min/max values are advertised in the `ControlInfo`
> > > of the `FrameDurationLimits` control, which are then updated after
> > > the camera is configured. Updating the control limits after configuration
> > > matches the behaviour of other platforms.
> > >
> > > While the kernel interface differentiates three types of frame intervals
> > > (discrete, continuous, stepwise), when querying the available frame intervals
> > > for a given (pixel format, size) combination, all options are evaluated
> > > and only the "local" minimum and maximum is used, as that is the only way
> > > the limits can reasonably be advertised on the libcamera side.
> > >
> > > Closes: https://gitlab.freedesktop.org/camera/libcamera/-/issues/296
> > > Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
> > > ---
> > >   include/libcamera/internal/v4l2_videodevice.h |   3 +
> > >   src/libcamera/pipeline/uvcvideo/uvcvideo.cpp  |  59 +++++++++
> > >   src/libcamera/v4l2_videodevice.cpp            | 116 ++++++++++++++++++
> > >   3 files changed, 178 insertions(+)
> > >
> > > diff --git a/include/libcamera/internal/v4l2_videodevice.h b/include/libcamera/internal/v4l2_videodevice.h
> > > index 82d98184ed..e97c0f9bf8 100644
> > > --- a/include/libcamera/internal/v4l2_videodevice.h
> > > +++ b/include/libcamera/internal/v4l2_videodevice.h
> > > @@ -209,6 +209,9 @@ public:
> > >   	Formats formats(uint32_t code = 0);
> > >
> > >   	int getFrameInterval(std::chrono::microseconds *interval);
> > > +	int setFrameInterval(std::chrono::microseconds *interval);
> > > +	std::optional<std::array<std::chrono::microseconds, 2>>
> > > +	getFrameIntervalLimits(V4L2PixelFormat pixelFormat, Size size);
> > >
> > >   	int getSelection(unsigned int target, Rectangle *rect);
> > >   	int setSelection(unsigned int target, Rectangle *rect);
> > > diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
> > > index 3f98e8ece0..e4e9b8ab9b 100644
> > > --- a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
> > > +++ b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
> > > @@ -57,6 +57,7 @@ public:
> > >   	std::unique_ptr<V4L2VideoDevice> video_;
> > >   	Stream stream_;
> > >   	std::map<PixelFormat, std::vector<SizeRange>> formats_;
> > > +	std::map<std::pair<PixelFormat, Size>, std::array<std::chrono::microseconds, 2>> frameIntervals_;
> > >
> > >   	std::optional<v4l2_exposure_auto_type> autoExposureMode_;
> > >   	std::optional<v4l2_exposure_auto_type> manualExposureMode_;
> > > @@ -277,6 +278,22 @@ int PipelineHandlerUVC::configure(Camera *camera, CameraConfiguration *config)
> > >   	    format.fourcc != data->video_->toV4L2PixelFormat(cfg.pixelFormat))
> > >   		return -EINVAL;
> > >
> > > +	auto it = data->controlInfo_.find(&controls::FrameDurationLimits);
> > > +	if (it != data->controlInfo_.end()) {
> > > +		auto it2 = data->frameIntervals_.find({ cfg.pixelFormat, cfg.size });
> > > +		if (it2 != data->frameIntervals_.end()) {
> > > +			std::chrono::microseconds current;
> > > +
> > > +			ret = data->video_->getFrameInterval(&current);
> > > +
> > > +			it->second = ControlInfo{
> > > +				int64_t(it2->second[0].count()),
> > > +				int64_t(it2->second[1].count()),
> > > +				ret == 0 ? ControlValue(int64_t(current.count())) : ControlValue()
> > > +			};
> > > +		}
> > > +	}
> > > +
> > >   	cfg.setStream(&data->stream_);
> > >
> > >   	return 0;
> > > @@ -306,6 +323,19 @@ int PipelineHandlerUVC::start(Camera *camera, const ControlList *controls)
> > >   		ret = processControls(data, *controls);
> > >   		if (ret < 0)
> > >   			goto err_release_buffers;
> > > +
> > > +		/* Can only be set before starting. */
> > > +		auto fdl = controls->get(controls::FrameDurationLimits);
> > > +		if (fdl) {
> > > +			const auto wantMin = std::chrono::microseconds((*fdl)[0]);
> > > +			const auto wantMax = std::chrono::microseconds((*fdl)[1]);
> > > +			auto want = (wantMin + wantMax) / 2;
> > > +
> > > +			/* Let the kernel choose something close to the middle. */
> > > +			ret = data->video_->setFrameInterval(&want);
> > > +			if (ret == 0)
> > > +				data->timePerFrame_ = want;
> > > +		}
> > >   	}
> > >
> > >   	ret = data->video_->streamOn();
> > > @@ -355,6 +385,8 @@ int PipelineHandlerUVC::processControl(const UVCCameraData *data, ControlList *c
> > >   		cid = V4L2_CID_GAMMA;
> > >   	else if (id == controls::AeEnable)
> > >   		return 0; /* Handled in `Camera::queueRequest()`. */
> > > +	else if (id == controls::FrameDurationLimits)
> > > +		return 0; /* Handled in `start()` */
> > >   	else
> > >   		return -EINVAL;
> > >
> > > @@ -555,6 +587,23 @@ int UVCCameraData::init(std::shared_ptr<MediaDevice> media)
> > >   	 * resolution from the largest size it advertises.
> > >   	 */
> > >   	Size resolution;
> > > +	auto minFrameInterval = std::chrono::microseconds::max();
> > > +	auto maxFrameInterval = std::chrono::microseconds::min();
> > > +
> > > +	const auto processFrameIntervals = [&](PixelFormat pf, V4L2PixelFormat v4l2pf, SizeRange size) {
> > > +		if (size.min != size.max)
> > > +			return;
> >
> > Can this happen ?
>
> I suppose not, I will replace it with an assertion.
>
>
> >
> > > +
> > > +		auto frameIntervals = video_->getFrameIntervalLimits(v4l2pf, size.min);
> > > +		if (!frameIntervals)
> > > +			return;
> > > +
> > > +		minFrameInterval = std::min(minFrameInterval, (*frameIntervals)[0]);
> > > +		maxFrameInterval = std::max(maxFrameInterval, (*frameIntervals)[1]);
> >
> > Aren't you already taking the min/max in the video device helpers ?
>
> This is the min/max among all pixel formats and sizes, to provide initial
> min/max values in the `ControlInfo`. See below ...
>
>
> >
> > > +
> > > +		frameIntervals_.try_emplace({ pf, size.min }, *frameIntervals);
> > > +	};
> > > +
> > >   	for (const auto &format : video_->formats()) {
> > >   		PixelFormat pixelFormat = format.first.toPixelFormat();
> > >   		if (!pixelFormat.isValid())
> > > @@ -566,6 +615,8 @@ int UVCCameraData::init(std::shared_ptr<MediaDevice> media)
> > >   		for (const SizeRange &sizeRange : sizeRanges) {
> > >   			if (sizeRange.max > resolution)
> > >   				resolution = sizeRange.max;
> > > +
> > > +			processFrameIntervals(pixelFormat, format.first, sizeRange);
> > >   		}
> > >   	}
> > >
> > > @@ -625,6 +676,14 @@ int UVCCameraData::init(std::shared_ptr<MediaDevice> media)
> > >   		ctrls[&controls::AeEnable] = ControlInfo(false, true, true);
> > >   	}
> > >
> > > +	/* Use the global min/max here, limits will be updated in `configure()`. */
> > > +	if (!frameIntervals_.empty()) {
> > > +		ctrls[&controls::FrameDurationLimits] = ControlInfo{
> > > +			int64_t(minFrameInterval.count()),
> > > +			int64_t(maxFrameInterval.count()),
> > > +		};
> > > +	}
>
> ... here the "global" limits are reported initially.
>

I see

>
> > > +
> > >   	controlInfo_ = ControlInfoMap(std::move(ctrls), controls::controls);
> > >
> > >   	/*
> > > diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp
> > > index 3836dabef3..3db6e80aed 100644
> > > --- a/src/libcamera/v4l2_videodevice.cpp
> > > +++ b/src/libcamera/v4l2_videodevice.cpp
> > > @@ -1202,6 +1202,122 @@ int V4L2VideoDevice::getFrameInterval(std::chrono::microseconds *interval)
> > >   	return 0;
> > >   }
> > >
> > > +/**
> > > + * \brief Configure the frame interval
> > > + * \param[inout] interval The frame interval
> >
> > unit would be nice here as well
>
> Ok.
>
> >
> > > + *
> > > + * Apply the supplied \a interval as the time-per-frame stream parameter
> > > + * on the device, and return the actually applied value.
> > > + *
> > > + * \return 0 on success or a negative error code otherwise
> > > + */
> > > +int V4L2VideoDevice::setFrameInterval(std::chrono::microseconds *interval)
> > > +{
> > > +	v4l2_fract *frameInterval = nullptr;
> > > +	uint32_t *caps = nullptr;
> > > +	v4l2_streamparm sparm = {};
> > > +
> > > +	sparm.type = bufferType_;
> > > +
> > > +	switch (sparm.type) {
> > > +	case V4L2_BUF_TYPE_VIDEO_CAPTURE:
> > > +	case V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE:
> > > +		frameInterval = &sparm.parm.capture.timeperframe;
> > > +		caps = &sparm.parm.capture.capability;
> > > +		break;
> > > +	case V4L2_BUF_TYPE_VIDEO_OUTPUT:
> > > +	case V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE:
> > > +		frameInterval = &sparm.parm.output.timeperframe;
> > > +		caps = &sparm.parm.output.capability;
> > > +		break;
> > > +	}
> > > +
> > > +	if (!frameInterval)
> > > +		return -EINVAL;
> > > +
> > > +	constexpr auto max = std::numeric_limits<decltype(frameInterval->numerator)>::max();
> >
> > Feels quite a corner case, but checking doesn't hurt ?
> >
> > > +	if (interval->count() <= 0 || interval->count() > max)
> > > +		return -EINVAL;
> > > +
> > > +	frameInterval->numerator = interval->count();
> > > +	frameInterval->denominator = std::chrono::microseconds(std::chrono::seconds(1)).count();
> > > +
> > > +	int ret = ioctl(VIDIOC_S_PARM, &sparm);
> > > +	if (ret)
> > > +		return ret;
> > > +
> > > +	if (!(*caps & V4L2_CAP_TIMEPERFRAME))
> > > +		return -ENOTSUP;
> >
> > In this case, I' pretty sure you should check this flag before calling
> > ioctl()
>
> I think it's fine. If `V4L2_CAP_TIMEPERFRAME` is not present, then nothing
> (should) changes on the device and 0 is returned, in which case this condition
> detects that; or the driver returns an error code, which is detected above.

Why going through the trouble of an ioctl if we know that it's not
supported ? Ah! or is caps only updated -after- the ioctl call ?

>
>
> >
> > > +
> > > +	*interval = v4l2FractionToMs(*frameInterval);
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +/**
> > > + * \brief Retrieve the frame interval limits
> > > + * \param[in] pixelFormat The pixel format
> > > + * \param[in] size The size
> > > + *
> > > + * Retrieve the minimum and maximum available frame interval for
> > > + * the given \a pixelFormat and \a size.
> > > + *
> > > + * \return The min and max frame interval or std::nullopt otherwise
> > > + */
> > > +std::optional<std::array<std::chrono::microseconds, 2>>
> > > +V4L2VideoDevice::getFrameIntervalLimits(V4L2PixelFormat pixelFormat, Size size)
> > > +{
> > > +	auto min = std::chrono::microseconds::max();
> > > +	auto max = std::chrono::microseconds::min();
> > > +	unsigned int index = 0;
> > > +	int ret;
> > > +
> > > +	for (;; index++) {
> > > +		struct v4l2_frmivalenum frameInterval = {};
> > > +		frameInterval.index = index;
> > > +		frameInterval.pixel_format = pixelFormat;
> > > +		frameInterval.width = size.width;
> > > +		frameInterval.height = size.height;
> > > +
> > > +		ret = ioctl(VIDIOC_ENUM_FRAMEINTERVALS, &frameInterval);
> > > +		if (ret)
> > > +			break;
> > > +
> > > +		switch (frameInterval.type) {
> > > +		case V4L2_FRMIVAL_TYPE_DISCRETE: {
> > > +			auto ms = v4l2FractionToMs(frameInterval.discrete);
> > > +
> > > +			min = std::min(min, ms);
> > > +			max = std::max(max, ms);
> > > +			break;
> > > +		}
> > > +		case V4L2_FRMIVAL_TYPE_CONTINUOUS:
> > > +		case V4L2_FRMIVAL_TYPE_STEPWISE: {
> > > +			min = std::min(min, v4l2FractionToMs(frameInterval.stepwise.min));
> > > +			max = std::max(max, v4l2FractionToMs(frameInterval.stepwise.max));
> > > +			break;
> > > +		}
> >
> > You know, I'm still not sure if we should support anything != DISCRETE.
> >
> > If I'm not mistaken UVC will always be DISCRETE. Every other device
> > that uses this function is likely something that we don't want to
> > support (because we want the drivers not to use this API for complex
> > devices).
>
> 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
>

Argh

>
> >
> > The same could be said for VIDEO_OUTPUT/OUTPUT_MPLANE, this will only
> > apply to codecs ?
>
> That's probably true, but since it was easy to handle those cases, I saw
> no reason to intentionally omit them.
>

Well, I'm always in the "the less code we have, the less code can
break" camp, but I guess that's not too drammatic

>
> >
> > > +		default:
> > > +			LOG(V4L2, Error)
> > > +				<< "Unknown v4l2_frmsizetypes value "
> > > +				<< frameInterval.type;
> > > +			return {};
> > > +		}
> > > +	}
> > > +
> > > +	if (ret && ret != -EINVAL) {
> >
> > I might have missed why -EINVAL is ok
>
> When the index is one more than the index of the last available frame duration,
> then EINVAL is returned, which breaks the loop, but it is not an error condition.
>

Ah, right right

>
> >
> > > +		LOG(V4L2, Error)
> > > +			<< "Unable to enumerate pixel formats: "
> > > +			<< strerror(-ret);
> > > +		return {};
> > > +	}
> > > +
> > > +	if (index <= 0)
> >
> > How can index be < 0 ?
>
> Admittedly it can't... I just like to do `<= 0` regardless of type.

Which creates confusion in the reader which might ask "what am I
missing that could lead to index be < 0" ?

>
>
> >
> > > +		return {};
> > > +
> > > +	return {{ min, max }};
> > > +}
> > > +
> > >   std::vector<V4L2PixelFormat> V4L2VideoDevice::enumPixelformats(uint32_t code)
> > >   {
> > >   	std::vector<V4L2PixelFormat> formats;
> > > --
> > > 2.52.0
> > >
>

Hi

2025. 12. 11. 10:15 keltezéssel, Jacopo Mondi írta:
> Hi Barnabás
> 
> On Wed, Dec 10, 2025 at 05:56:01PM +0100, Barnabás Pőcze wrote:
>> Hi
>>
>> 2025. 12. 10. 17:04 keltezéssel, Jacopo Mondi írta:
>>> Hi Barnabás
>>>
>>> On Wed, Dec 10, 2025 at 02:37:04PM +0100, Barnabás Pőcze wrote:
>>>> Setting a frame rate is possible with UVC devices using `VIDIOC_S_PARM`.
>>>> However, compared to other platforms supported by libcamera, limitations
>>>> apply. Probably most crucially, the desired frame rate cannot be set
>>>> while the camera is streaming. Furthermore, it is only a single value
>>>> and not an allowed range.
>>>>
>>>> So this change only adds support for `FrameDurationLimits` in the control
>>>> list passed to `Camera::start()`, and the control is otherwise ignored in
>>>> requests.
>>>>
>>>> The kernel interface also only allows a single number and not a range,
>>>> so the midpoint of the desired range is used. Checking the supplied
>>>
>>> Everything else sounds fine, but I wonder if taking the midpoint is
>>> the best solution here or we should simply use the shortest provided
>>> duration, warn the user, and let the kernel adjust it to the closest
>>> supported value.
>>
>> As far as I understand, the kernel selects the closest valid frame duration
>> value. Which means that using the midpoint should guarantee that if the device
>> supports at least one frame duration value in the desired range, then a value
>> in the desired range will be selected on the device. This is not guaranteed if
>> either the min or max is sent to the kernel. That's why I chose this behaviour;
>> I believe it's probably best to select a frame duration from the desired range,
>> if possible.
> 
> Well, you have a cache of those now, don't you ?

Only the min/max is stored for a particular (pixel format, size) combination,
and that is only done to be able to report it in the `ControlInfo` when the
configuration changes.

Even if there all the frame interval options were stored, I still think selecting
the midpoint is the best that we can do. Unless I am missing something?


> 
>>
>>
>>>
>>>> values is not necessary since the kernel will adjust the value if
>>>> it is not supported by the device.
>>>>
>>>> Initially the global min/max values are advertised in the `ControlInfo`
>>>> of the `FrameDurationLimits` control, which are then updated after
>>>> the camera is configured. Updating the control limits after configuration
>>>> matches the behaviour of other platforms.
>>>>
>>>> While the kernel interface differentiates three types of frame intervals
>>>> (discrete, continuous, stepwise), when querying the available frame intervals
>>>> for a given (pixel format, size) combination, all options are evaluated
>>>> and only the "local" minimum and maximum is used, as that is the only way
>>>> the limits can reasonably be advertised on the libcamera side.
>>>>
>>>> Closes: https://gitlab.freedesktop.org/camera/libcamera/-/issues/296
>>>> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
>>>> ---
>>>>    include/libcamera/internal/v4l2_videodevice.h |   3 +
>>>>    src/libcamera/pipeline/uvcvideo/uvcvideo.cpp  |  59 +++++++++
>>>>    src/libcamera/v4l2_videodevice.cpp            | 116 ++++++++++++++++++
>>>>    3 files changed, 178 insertions(+)
>>>>
>>>> diff --git a/include/libcamera/internal/v4l2_videodevice.h b/include/libcamera/internal/v4l2_videodevice.h
>>>> index 82d98184ed..e97c0f9bf8 100644
>>>> --- a/include/libcamera/internal/v4l2_videodevice.h
>>>> +++ b/include/libcamera/internal/v4l2_videodevice.h
>>>> @@ -209,6 +209,9 @@ public:
>>>>    	Formats formats(uint32_t code = 0);
>>>>
>>>>    	int getFrameInterval(std::chrono::microseconds *interval);
>>>> +	int setFrameInterval(std::chrono::microseconds *interval);
>>>> +	std::optional<std::array<std::chrono::microseconds, 2>>
>>>> +	getFrameIntervalLimits(V4L2PixelFormat pixelFormat, Size size);
>>>>
>>>>    	int getSelection(unsigned int target, Rectangle *rect);
>>>>    	int setSelection(unsigned int target, Rectangle *rect);
>>>> diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
>>>> index 3f98e8ece0..e4e9b8ab9b 100644
>>>> --- a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
>>>> +++ b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
>>>> @@ -57,6 +57,7 @@ public:
>>>>    	std::unique_ptr<V4L2VideoDevice> video_;
>>>>    	Stream stream_;
>>>>    	std::map<PixelFormat, std::vector<SizeRange>> formats_;
>>>> +	std::map<std::pair<PixelFormat, Size>, std::array<std::chrono::microseconds, 2>> frameIntervals_;
>>>>
>>>>    	std::optional<v4l2_exposure_auto_type> autoExposureMode_;
>>>>    	std::optional<v4l2_exposure_auto_type> manualExposureMode_;
>>>> @@ -277,6 +278,22 @@ int PipelineHandlerUVC::configure(Camera *camera, CameraConfiguration *config)
>>>>    	    format.fourcc != data->video_->toV4L2PixelFormat(cfg.pixelFormat))
>>>>    		return -EINVAL;
>>>>
>>>> +	auto it = data->controlInfo_.find(&controls::FrameDurationLimits);
>>>> +	if (it != data->controlInfo_.end()) {
>>>> +		auto it2 = data->frameIntervals_.find({ cfg.pixelFormat, cfg.size });
>>>> +		if (it2 != data->frameIntervals_.end()) {
>>>> +			std::chrono::microseconds current;
>>>> +
>>>> +			ret = data->video_->getFrameInterval(&current);
>>>> +
>>>> +			it->second = ControlInfo{
>>>> +				int64_t(it2->second[0].count()),
>>>> +				int64_t(it2->second[1].count()),
>>>> +				ret == 0 ? ControlValue(int64_t(current.count())) : ControlValue()
>>>> +			};
>>>> +		}
>>>> +	}
>>>> +
>>>>    	cfg.setStream(&data->stream_);
>>>>
>>>>    	return 0;
>>>> @@ -306,6 +323,19 @@ int PipelineHandlerUVC::start(Camera *camera, const ControlList *controls)
>>>>    		ret = processControls(data, *controls);
>>>>    		if (ret < 0)
>>>>    			goto err_release_buffers;
>>>> +
>>>> +		/* Can only be set before starting. */
>>>> +		auto fdl = controls->get(controls::FrameDurationLimits);
>>>> +		if (fdl) {
>>>> +			const auto wantMin = std::chrono::microseconds((*fdl)[0]);
>>>> +			const auto wantMax = std::chrono::microseconds((*fdl)[1]);
>>>> +			auto want = (wantMin + wantMax) / 2;
>>>> +
>>>> +			/* Let the kernel choose something close to the middle. */
>>>> +			ret = data->video_->setFrameInterval(&want);
>>>> +			if (ret == 0)
>>>> +				data->timePerFrame_ = want;
>>>> +		}
>>>>    	}
>>>>
>>>>    	ret = data->video_->streamOn();
>>>> @@ -355,6 +385,8 @@ int PipelineHandlerUVC::processControl(const UVCCameraData *data, ControlList *c
>>>>    		cid = V4L2_CID_GAMMA;
>>>>    	else if (id == controls::AeEnable)
>>>>    		return 0; /* Handled in `Camera::queueRequest()`. */
>>>> +	else if (id == controls::FrameDurationLimits)
>>>> +		return 0; /* Handled in `start()` */
>>>>    	else
>>>>    		return -EINVAL;
>>>>
>>>> @@ -555,6 +587,23 @@ int UVCCameraData::init(std::shared_ptr<MediaDevice> media)
>>>>    	 * resolution from the largest size it advertises.
>>>>    	 */
>>>>    	Size resolution;
>>>> +	auto minFrameInterval = std::chrono::microseconds::max();
>>>> +	auto maxFrameInterval = std::chrono::microseconds::min();
>>>> +
>>>> +	const auto processFrameIntervals = [&](PixelFormat pf, V4L2PixelFormat v4l2pf, SizeRange size) {
>>>> +		if (size.min != size.max)
>>>> +			return;
>>>
>>> Can this happen ?
>>
>> I suppose not, I will replace it with an assertion.
>>
>>
>>>
>>>> +
>>>> +		auto frameIntervals = video_->getFrameIntervalLimits(v4l2pf, size.min);
>>>> +		if (!frameIntervals)
>>>> +			return;
>>>> +
>>>> +		minFrameInterval = std::min(minFrameInterval, (*frameIntervals)[0]);
>>>> +		maxFrameInterval = std::max(maxFrameInterval, (*frameIntervals)[1]);
>>>
>>> Aren't you already taking the min/max in the video device helpers ?
>>
>> This is the min/max among all pixel formats and sizes, to provide initial
>> min/max values in the `ControlInfo`. See below ...
>>
>>
>>>
>>>> +
>>>> +		frameIntervals_.try_emplace({ pf, size.min }, *frameIntervals);
>>>> +	};
>>>> +
>>>>    	for (const auto &format : video_->formats()) {
>>>>    		PixelFormat pixelFormat = format.first.toPixelFormat();
>>>>    		if (!pixelFormat.isValid())
>>>> @@ -566,6 +615,8 @@ int UVCCameraData::init(std::shared_ptr<MediaDevice> media)
>>>>    		for (const SizeRange &sizeRange : sizeRanges) {
>>>>    			if (sizeRange.max > resolution)
>>>>    				resolution = sizeRange.max;
>>>> +
>>>> +			processFrameIntervals(pixelFormat, format.first, sizeRange);
>>>>    		}
>>>>    	}
>>>>
>>>> @@ -625,6 +676,14 @@ int UVCCameraData::init(std::shared_ptr<MediaDevice> media)
>>>>    		ctrls[&controls::AeEnable] = ControlInfo(false, true, true);
>>>>    	}
>>>>
>>>> +	/* Use the global min/max here, limits will be updated in `configure()`. */
>>>> +	if (!frameIntervals_.empty()) {
>>>> +		ctrls[&controls::FrameDurationLimits] = ControlInfo{
>>>> +			int64_t(minFrameInterval.count()),
>>>> +			int64_t(maxFrameInterval.count()),
>>>> +		};
>>>> +	}
>>
>> ... here the "global" limits are reported initially.
>>
> 
> I see
> 
>>
>>>> +
>>>>    	controlInfo_ = ControlInfoMap(std::move(ctrls), controls::controls);
>>>>
>>>>    	/*
>>>> diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp
>>>> index 3836dabef3..3db6e80aed 100644
>>>> --- a/src/libcamera/v4l2_videodevice.cpp
>>>> +++ b/src/libcamera/v4l2_videodevice.cpp
>>>> @@ -1202,6 +1202,122 @@ int V4L2VideoDevice::getFrameInterval(std::chrono::microseconds *interval)
>>>>    	return 0;
>>>>    }
>>>>
>>>> +/**
>>>> + * \brief Configure the frame interval
>>>> + * \param[inout] interval The frame interval
>>>
>>> unit would be nice here as well
>>
>> Ok.
>>
>>>
>>>> + *
>>>> + * Apply the supplied \a interval as the time-per-frame stream parameter
>>>> + * on the device, and return the actually applied value.
>>>> + *
>>>> + * \return 0 on success or a negative error code otherwise
>>>> + */
>>>> +int V4L2VideoDevice::setFrameInterval(std::chrono::microseconds *interval)
>>>> +{
>>>> +	v4l2_fract *frameInterval = nullptr;
>>>> +	uint32_t *caps = nullptr;
>>>> +	v4l2_streamparm sparm = {};
>>>> +
>>>> +	sparm.type = bufferType_;
>>>> +
>>>> +	switch (sparm.type) {
>>>> +	case V4L2_BUF_TYPE_VIDEO_CAPTURE:
>>>> +	case V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE:
>>>> +		frameInterval = &sparm.parm.capture.timeperframe;
>>>> +		caps = &sparm.parm.capture.capability;
>>>> +		break;
>>>> +	case V4L2_BUF_TYPE_VIDEO_OUTPUT:
>>>> +	case V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE:
>>>> +		frameInterval = &sparm.parm.output.timeperframe;
>>>> +		caps = &sparm.parm.output.capability;
>>>> +		break;
>>>> +	}
>>>> +
>>>> +	if (!frameInterval)
>>>> +		return -EINVAL;
>>>> +
>>>> +	constexpr auto max = std::numeric_limits<decltype(frameInterval->numerator)>::max();
>>>
>>> Feels quite a corner case, but checking doesn't hurt ?
>>>
>>>> +	if (interval->count() <= 0 || interval->count() > max)
>>>> +		return -EINVAL;
>>>> +
>>>> +	frameInterval->numerator = interval->count();
>>>> +	frameInterval->denominator = std::chrono::microseconds(std::chrono::seconds(1)).count();
>>>> +
>>>> +	int ret = ioctl(VIDIOC_S_PARM, &sparm);
>>>> +	if (ret)
>>>> +		return ret;
>>>> +
>>>> +	if (!(*caps & V4L2_CAP_TIMEPERFRAME))
>>>> +		return -ENOTSUP;
>>>
>>> In this case, I' pretty sure you should check this flag before calling
>>> ioctl()
>>
>> I think it's fine. If `V4L2_CAP_TIMEPERFRAME` is not present, then nothing
>> (should) changes on the device and 0 is returned, in which case this condition
>> detects that; or the driver returns an error code, which is detected above.
> 
> Why going through the trouble of an ioctl if we know that it's not
> supported ? Ah! or is caps only updated -after- the ioctl call ?

The ioctl updates the capabilities, yes. I think this could be checked in `open()` and remembered.
Would that be better?


> 
>>
>>
>>>
>>>> +
>>>> +	*interval = v4l2FractionToMs(*frameInterval);
>>>> +
>>>> +	return 0;
>>>> +}
>>>> +
>>>> +/**
>>>> + * \brief Retrieve the frame interval limits
>>>> + * \param[in] pixelFormat The pixel format
>>>> + * \param[in] size The size
>>>> + *
>>>> + * Retrieve the minimum and maximum available frame interval for
>>>> + * the given \a pixelFormat and \a size.
>>>> + *
>>>> + * \return The min and max frame interval or std::nullopt otherwise
>>>> + */
>>>> +std::optional<std::array<std::chrono::microseconds, 2>>
>>>> +V4L2VideoDevice::getFrameIntervalLimits(V4L2PixelFormat pixelFormat, Size size)
>>>> +{
>>>> +	auto min = std::chrono::microseconds::max();
>>>> +	auto max = std::chrono::microseconds::min();
>>>> +	unsigned int index = 0;
>>>> +	int ret;
>>>> +
>>>> +	for (;; index++) {
>>>> +		struct v4l2_frmivalenum frameInterval = {};
>>>> +		frameInterval.index = index;
>>>> +		frameInterval.pixel_format = pixelFormat;
>>>> +		frameInterval.width = size.width;
>>>> +		frameInterval.height = size.height;
>>>> +
>>>> +		ret = ioctl(VIDIOC_ENUM_FRAMEINTERVALS, &frameInterval);
>>>> +		if (ret)
>>>> +			break;
>>>> +
>>>> +		switch (frameInterval.type) {
>>>> +		case V4L2_FRMIVAL_TYPE_DISCRETE: {
>>>> +			auto ms = v4l2FractionToMs(frameInterval.discrete);
>>>> +
>>>> +			min = std::min(min, ms);
>>>> +			max = std::max(max, ms);
>>>> +			break;
>>>> +		}
>>>> +		case V4L2_FRMIVAL_TYPE_CONTINUOUS:
>>>> +		case V4L2_FRMIVAL_TYPE_STEPWISE: {
>>>> +			min = std::min(min, v4l2FractionToMs(frameInterval.stepwise.min));
>>>> +			max = std::max(max, v4l2FractionToMs(frameInterval.stepwise.max));
>>>> +			break;
>>>> +		}
>>>
>>> You know, I'm still not sure if we should support anything != DISCRETE.
>>>
>>> If I'm not mistaken UVC will always be DISCRETE. Every other device
>>> that uses this function is likely something that we don't want to
>>> support (because we want the drivers not to use this API for complex
>>> devices).
>>
>> 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
>>
> 
> Argh
> 
>>
>>>
>>> The same could be said for VIDEO_OUTPUT/OUTPUT_MPLANE, this will only
>>> apply to codecs ?
>>
>> That's probably true, but since it was easy to handle those cases, I saw
>> no reason to intentionally omit them.
>>
> 
> Well, I'm always in the "the less code we have, the less code can
> break" camp, but I guess that's not too drammatic
> 
>>
>>>
>>>> +		default:
>>>> +			LOG(V4L2, Error)
>>>> +				<< "Unknown v4l2_frmsizetypes value "
>>>> +				<< frameInterval.type;
>>>> +			return {};
>>>> +		}
>>>> +	}
>>>> +
>>>> +	if (ret && ret != -EINVAL) {
>>>
>>> I might have missed why -EINVAL is ok
>>
>> When the index is one more than the index of the last available frame duration,
>> then EINVAL is returned, which breaks the loop, but it is not an error condition.
>>
> 
> Ah, right right
> 
>>
>>>
>>>> +		LOG(V4L2, Error)
>>>> +			<< "Unable to enumerate pixel formats: "
>>>> +			<< strerror(-ret);
>>>> +		return {};
>>>> +	}
>>>> +
>>>> +	if (index <= 0)
>>>
>>> How can index be < 0 ?
>>
>> Admittedly it can't... I just like to do `<= 0` regardless of type.
> 
> Which creates confusion in the reader which might ask "what am I
> missing that could lead to index be < 0" ?

Okay, I'll change it.


> 
>>
>>
>>>
>>>> +		return {};
>>>> +
>>>> +	return {{ min, max }};
>>>> +}
>>>> +
>>>>    std::vector<V4L2PixelFormat> V4L2VideoDevice::enumPixelformats(uint32_t code)
>>>>    {
>>>>    	std::vector<V4L2PixelFormat> formats;
>>>> --
>>>> 2.52.0
>>>>
>>

Hi Barnabás

On Thu, Dec 11, 2025 at 10:38:19AM +0100, Barnabás Pőcze wrote:
> Hi
>
> 2025. 12. 11. 10:15 keltezéssel, Jacopo Mondi írta:
> > Hi Barnabás
> >
> > On Wed, Dec 10, 2025 at 05:56:01PM +0100, Barnabás Pőcze wrote:
> > > Hi
> > >
> > > 2025. 12. 10. 17:04 keltezéssel, Jacopo Mondi írta:
> > > > Hi Barnabás
> > > >
> > > > On Wed, Dec 10, 2025 at 02:37:04PM +0100, Barnabás Pőcze wrote:
> > > > > Setting a frame rate is possible with UVC devices using `VIDIOC_S_PARM`.
> > > > > However, compared to other platforms supported by libcamera, limitations
> > > > > apply. Probably most crucially, the desired frame rate cannot be set
> > > > > while the camera is streaming. Furthermore, it is only a single value
> > > > > and not an allowed range.
> > > > >
> > > > > So this change only adds support for `FrameDurationLimits` in the control
> > > > > list passed to `Camera::start()`, and the control is otherwise ignored in
> > > > > requests.
> > > > >
> > > > > The kernel interface also only allows a single number and not a range,
> > > > > so the midpoint of the desired range is used. Checking the supplied
> > > >
> > > > Everything else sounds fine, but I wonder if taking the midpoint is
> > > > the best solution here or we should simply use the shortest provided
> > > > duration, warn the user, and let the kernel adjust it to the closest
> > > > supported value.
> > >
> > > As far as I understand, the kernel selects the closest valid frame duration
> > > value. Which means that using the midpoint should guarantee that if the device
> > > supports at least one frame duration value in the desired range, then a value
> > > in the desired range will be selected on the device. This is not guaranteed if
> > > either the min or max is sent to the kernel. That's why I chose this behaviour;
> > > I believe it's probably best to select a frame duration from the desired range,
> > > if possible.
> >
> > Well, you have a cache of those now, don't you ?
>
> Only the min/max is stored for a particular (pixel format, size) combination,
> and that is only done to be able to report it in the `ControlInfo` when the
> configuration changes.

Right, sorry, I've overlooked it

>
> Even if there all the frame interval options were stored, I still think selecting
> the midpoint is the best that we can do. Unless I am missing something?
>

I'm in two minds here.

I'm fine if we select the midpoint, but I would log the user about
what we're doing (and about the fact uvc doesn't support frame
duration ranges)

>
> >
> > >
> > >
> > > >
> > > > > values is not necessary since the kernel will adjust the value if
> > > > > it is not supported by the device.
> > > > >
> > > > > Initially the global min/max values are advertised in the `ControlInfo`
> > > > > of the `FrameDurationLimits` control, which are then updated after
> > > > > the camera is configured. Updating the control limits after configuration
> > > > > matches the behaviour of other platforms.
> > > > >
> > > > > While the kernel interface differentiates three types of frame intervals
> > > > > (discrete, continuous, stepwise), when querying the available frame intervals
> > > > > for a given (pixel format, size) combination, all options are evaluated
> > > > > and only the "local" minimum and maximum is used, as that is the only way
> > > > > the limits can reasonably be advertised on the libcamera side.
> > > > >
> > > > > Closes: https://gitlab.freedesktop.org/camera/libcamera/-/issues/296
> > > > > Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
> > > > > ---
> > > > >    include/libcamera/internal/v4l2_videodevice.h |   3 +
> > > > >    src/libcamera/pipeline/uvcvideo/uvcvideo.cpp  |  59 +++++++++
> > > > >    src/libcamera/v4l2_videodevice.cpp            | 116 ++++++++++++++++++
> > > > >    3 files changed, 178 insertions(+)
> > > > >
> > > > > diff --git a/include/libcamera/internal/v4l2_videodevice.h b/include/libcamera/internal/v4l2_videodevice.h
> > > > > index 82d98184ed..e97c0f9bf8 100644
> > > > > --- a/include/libcamera/internal/v4l2_videodevice.h
> > > > > +++ b/include/libcamera/internal/v4l2_videodevice.h
> > > > > @@ -209,6 +209,9 @@ public:
> > > > >    	Formats formats(uint32_t code = 0);
> > > > >
> > > > >    	int getFrameInterval(std::chrono::microseconds *interval);
> > > > > +	int setFrameInterval(std::chrono::microseconds *interval);
> > > > > +	std::optional<std::array<std::chrono::microseconds, 2>>
> > > > > +	getFrameIntervalLimits(V4L2PixelFormat pixelFormat, Size size);
> > > > >
> > > > >    	int getSelection(unsigned int target, Rectangle *rect);
> > > > >    	int setSelection(unsigned int target, Rectangle *rect);
> > > > > diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
> > > > > index 3f98e8ece0..e4e9b8ab9b 100644
> > > > > --- a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
> > > > > +++ b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
> > > > > @@ -57,6 +57,7 @@ public:
> > > > >    	std::unique_ptr<V4L2VideoDevice> video_;
> > > > >    	Stream stream_;
> > > > >    	std::map<PixelFormat, std::vector<SizeRange>> formats_;
> > > > > +	std::map<std::pair<PixelFormat, Size>, std::array<std::chrono::microseconds, 2>> frameIntervals_;
> > > > >
> > > > >    	std::optional<v4l2_exposure_auto_type> autoExposureMode_;
> > > > >    	std::optional<v4l2_exposure_auto_type> manualExposureMode_;
> > > > > @@ -277,6 +278,22 @@ int PipelineHandlerUVC::configure(Camera *camera, CameraConfiguration *config)
> > > > >    	    format.fourcc != data->video_->toV4L2PixelFormat(cfg.pixelFormat))
> > > > >    		return -EINVAL;
> > > > >
> > > > > +	auto it = data->controlInfo_.find(&controls::FrameDurationLimits);
> > > > > +	if (it != data->controlInfo_.end()) {
> > > > > +		auto it2 = data->frameIntervals_.find({ cfg.pixelFormat, cfg.size });
> > > > > +		if (it2 != data->frameIntervals_.end()) {
> > > > > +			std::chrono::microseconds current;
> > > > > +
> > > > > +			ret = data->video_->getFrameInterval(&current);
> > > > > +
> > > > > +			it->second = ControlInfo{
> > > > > +				int64_t(it2->second[0].count()),
> > > > > +				int64_t(it2->second[1].count()),
> > > > > +				ret == 0 ? ControlValue(int64_t(current.count())) : ControlValue()
> > > > > +			};
> > > > > +		}
> > > > > +	}
> > > > > +
> > > > >    	cfg.setStream(&data->stream_);
> > > > >
> > > > >    	return 0;
> > > > > @@ -306,6 +323,19 @@ int PipelineHandlerUVC::start(Camera *camera, const ControlList *controls)
> > > > >    		ret = processControls(data, *controls);
> > > > >    		if (ret < 0)
> > > > >    			goto err_release_buffers;
> > > > > +
> > > > > +		/* Can only be set before starting. */
> > > > > +		auto fdl = controls->get(controls::FrameDurationLimits);
> > > > > +		if (fdl) {
> > > > > +			const auto wantMin = std::chrono::microseconds((*fdl)[0]);
> > > > > +			const auto wantMax = std::chrono::microseconds((*fdl)[1]);
> > > > > +			auto want = (wantMin + wantMax) / 2;
> > > > > +
> > > > > +			/* Let the kernel choose something close to the middle. */
> > > > > +			ret = data->video_->setFrameInterval(&want);
> > > > > +			if (ret == 0)
> > > > > +				data->timePerFrame_ = want;
> > > > > +		}
> > > > >    	}
> > > > >
> > > > >    	ret = data->video_->streamOn();
> > > > > @@ -355,6 +385,8 @@ int PipelineHandlerUVC::processControl(const UVCCameraData *data, ControlList *c
> > > > >    		cid = V4L2_CID_GAMMA;
> > > > >    	else if (id == controls::AeEnable)
> > > > >    		return 0; /* Handled in `Camera::queueRequest()`. */
> > > > > +	else if (id == controls::FrameDurationLimits)
> > > > > +		return 0; /* Handled in `start()` */
> > > > >    	else
> > > > >    		return -EINVAL;
> > > > >
> > > > > @@ -555,6 +587,23 @@ int UVCCameraData::init(std::shared_ptr<MediaDevice> media)
> > > > >    	 * resolution from the largest size it advertises.
> > > > >    	 */
> > > > >    	Size resolution;
> > > > > +	auto minFrameInterval = std::chrono::microseconds::max();
> > > > > +	auto maxFrameInterval = std::chrono::microseconds::min();
> > > > > +
> > > > > +	const auto processFrameIntervals = [&](PixelFormat pf, V4L2PixelFormat v4l2pf, SizeRange size) {
> > > > > +		if (size.min != size.max)
> > > > > +			return;
> > > >
> > > > Can this happen ?
> > >
> > > I suppose not, I will replace it with an assertion.
> > >
> > >
> > > >
> > > > > +
> > > > > +		auto frameIntervals = video_->getFrameIntervalLimits(v4l2pf, size.min);
> > > > > +		if (!frameIntervals)
> > > > > +			return;
> > > > > +
> > > > > +		minFrameInterval = std::min(minFrameInterval, (*frameIntervals)[0]);
> > > > > +		maxFrameInterval = std::max(maxFrameInterval, (*frameIntervals)[1]);
> > > >
> > > > Aren't you already taking the min/max in the video device helpers ?
> > >
> > > This is the min/max among all pixel formats and sizes, to provide initial
> > > min/max values in the `ControlInfo`. See below ...
> > >
> > >
> > > >
> > > > > +
> > > > > +		frameIntervals_.try_emplace({ pf, size.min }, *frameIntervals);
> > > > > +	};
> > > > > +
> > > > >    	for (const auto &format : video_->formats()) {
> > > > >    		PixelFormat pixelFormat = format.first.toPixelFormat();
> > > > >    		if (!pixelFormat.isValid())
> > > > > @@ -566,6 +615,8 @@ int UVCCameraData::init(std::shared_ptr<MediaDevice> media)
> > > > >    		for (const SizeRange &sizeRange : sizeRanges) {
> > > > >    			if (sizeRange.max > resolution)
> > > > >    				resolution = sizeRange.max;
> > > > > +
> > > > > +			processFrameIntervals(pixelFormat, format.first, sizeRange);
> > > > >    		}
> > > > >    	}
> > > > >
> > > > > @@ -625,6 +676,14 @@ int UVCCameraData::init(std::shared_ptr<MediaDevice> media)
> > > > >    		ctrls[&controls::AeEnable] = ControlInfo(false, true, true);
> > > > >    	}
> > > > >
> > > > > +	/* Use the global min/max here, limits will be updated in `configure()`. */
> > > > > +	if (!frameIntervals_.empty()) {
> > > > > +		ctrls[&controls::FrameDurationLimits] = ControlInfo{
> > > > > +			int64_t(minFrameInterval.count()),
> > > > > +			int64_t(maxFrameInterval.count()),
> > > > > +		};
> > > > > +	}
> > >
> > > ... here the "global" limits are reported initially.
> > >
> >
> > I see
> >
> > >
> > > > > +
> > > > >    	controlInfo_ = ControlInfoMap(std::move(ctrls), controls::controls);
> > > > >
> > > > >    	/*
> > > > > diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp
> > > > > index 3836dabef3..3db6e80aed 100644
> > > > > --- a/src/libcamera/v4l2_videodevice.cpp
> > > > > +++ b/src/libcamera/v4l2_videodevice.cpp
> > > > > @@ -1202,6 +1202,122 @@ int V4L2VideoDevice::getFrameInterval(std::chrono::microseconds *interval)
> > > > >    	return 0;
> > > > >    }
> > > > >
> > > > > +/**
> > > > > + * \brief Configure the frame interval
> > > > > + * \param[inout] interval The frame interval
> > > >
> > > > unit would be nice here as well
> > >
> > > Ok.
> > >
> > > >
> > > > > + *
> > > > > + * Apply the supplied \a interval as the time-per-frame stream parameter
> > > > > + * on the device, and return the actually applied value.
> > > > > + *
> > > > > + * \return 0 on success or a negative error code otherwise
> > > > > + */
> > > > > +int V4L2VideoDevice::setFrameInterval(std::chrono::microseconds *interval)
> > > > > +{
> > > > > +	v4l2_fract *frameInterval = nullptr;
> > > > > +	uint32_t *caps = nullptr;
> > > > > +	v4l2_streamparm sparm = {};
> > > > > +
> > > > > +	sparm.type = bufferType_;
> > > > > +
> > > > > +	switch (sparm.type) {
> > > > > +	case V4L2_BUF_TYPE_VIDEO_CAPTURE:
> > > > > +	case V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE:
> > > > > +		frameInterval = &sparm.parm.capture.timeperframe;
> > > > > +		caps = &sparm.parm.capture.capability;
> > > > > +		break;
> > > > > +	case V4L2_BUF_TYPE_VIDEO_OUTPUT:
> > > > > +	case V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE:
> > > > > +		frameInterval = &sparm.parm.output.timeperframe;
> > > > > +		caps = &sparm.parm.output.capability;
> > > > > +		break;
> > > > > +	}
> > > > > +
> > > > > +	if (!frameInterval)
> > > > > +		return -EINVAL;
> > > > > +
> > > > > +	constexpr auto max = std::numeric_limits<decltype(frameInterval->numerator)>::max();
> > > >
> > > > Feels quite a corner case, but checking doesn't hurt ?
> > > >
> > > > > +	if (interval->count() <= 0 || interval->count() > max)
> > > > > +		return -EINVAL;
> > > > > +
> > > > > +	frameInterval->numerator = interval->count();
> > > > > +	frameInterval->denominator = std::chrono::microseconds(std::chrono::seconds(1)).count();
> > > > > +
> > > > > +	int ret = ioctl(VIDIOC_S_PARM, &sparm);
> > > > > +	if (ret)
> > > > > +		return ret;
> > > > > +
> > > > > +	if (!(*caps & V4L2_CAP_TIMEPERFRAME))
> > > > > +		return -ENOTSUP;
> > > >
> > > > In this case, I' pretty sure you should check this flag before calling
> > > > ioctl()
> > >
> > > I think it's fine. If `V4L2_CAP_TIMEPERFRAME` is not present, then nothing
> > > (should) changes on the device and 0 is returned, in which case this condition
> > > detects that; or the driver returns an error code, which is detected above.
> >
> > Why going through the trouble of an ioctl if we know that it's not
> > supported ? Ah! or is caps only updated -after- the ioctl call ?
>
> The ioctl updates the capabilities, yes. I think this could be checked in `open()` and remembered.
> Would that be better?

The V4L2_CAP_TIMEPERFRAME cap is found in the v4l2_streamparm
structure. Can you get it from other caps at open() time or are you
considering issueing a g_parm ioctl() to check if it is supported in
open() ?



>
>
> >
> > >
> > >
> > > >
> > > > > +
> > > > > +	*interval = v4l2FractionToMs(*frameInterval);
> > > > > +
> > > > > +	return 0;
> > > > > +}
> > > > > +
> > > > > +/**
> > > > > + * \brief Retrieve the frame interval limits
> > > > > + * \param[in] pixelFormat The pixel format
> > > > > + * \param[in] size The size
> > > > > + *
> > > > > + * Retrieve the minimum and maximum available frame interval for
> > > > > + * the given \a pixelFormat and \a size.
> > > > > + *
> > > > > + * \return The min and max frame interval or std::nullopt otherwise
> > > > > + */
> > > > > +std::optional<std::array<std::chrono::microseconds, 2>>
> > > > > +V4L2VideoDevice::getFrameIntervalLimits(V4L2PixelFormat pixelFormat, Size size)
> > > > > +{
> > > > > +	auto min = std::chrono::microseconds::max();
> > > > > +	auto max = std::chrono::microseconds::min();
> > > > > +	unsigned int index = 0;
> > > > > +	int ret;
> > > > > +
> > > > > +	for (;; index++) {
> > > > > +		struct v4l2_frmivalenum frameInterval = {};
> > > > > +		frameInterval.index = index;
> > > > > +		frameInterval.pixel_format = pixelFormat;
> > > > > +		frameInterval.width = size.width;
> > > > > +		frameInterval.height = size.height;
> > > > > +
> > > > > +		ret = ioctl(VIDIOC_ENUM_FRAMEINTERVALS, &frameInterval);
> > > > > +		if (ret)
> > > > > +			break;
> > > > > +
> > > > > +		switch (frameInterval.type) {
> > > > > +		case V4L2_FRMIVAL_TYPE_DISCRETE: {
> > > > > +			auto ms = v4l2FractionToMs(frameInterval.discrete);
> > > > > +
> > > > > +			min = std::min(min, ms);
> > > > > +			max = std::max(max, ms);
> > > > > +			break;
> > > > > +		}
> > > > > +		case V4L2_FRMIVAL_TYPE_CONTINUOUS:
> > > > > +		case V4L2_FRMIVAL_TYPE_STEPWISE: {
> > > > > +			min = std::min(min, v4l2FractionToMs(frameInterval.stepwise.min));
> > > > > +			max = std::max(max, v4l2FractionToMs(frameInterval.stepwise.max));
> > > > > +			break;
> > > > > +		}
> > > >
> > > > You know, I'm still not sure if we should support anything != DISCRETE.
> > > >
> > > > If I'm not mistaken UVC will always be DISCRETE. Every other device
> > > > that uses this function is likely something that we don't want to
> > > > support (because we want the drivers not to use this API for complex
> > > > devices).
> > >
> > > 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
> > >
> >
> > Argh
> >
> > >
> > > >
> > > > The same could be said for VIDEO_OUTPUT/OUTPUT_MPLANE, this will only
> > > > apply to codecs ?
> > >
> > > That's probably true, but since it was easy to handle those cases, I saw
> > > no reason to intentionally omit them.
> > >
> >
> > Well, I'm always in the "the less code we have, the less code can
> > break" camp, but I guess that's not too drammatic
> >
> > >
> > > >
> > > > > +		default:
> > > > > +			LOG(V4L2, Error)
> > > > > +				<< "Unknown v4l2_frmsizetypes value "
> > > > > +				<< frameInterval.type;
> > > > > +			return {};
> > > > > +		}
> > > > > +	}
> > > > > +
> > > > > +	if (ret && ret != -EINVAL) {
> > > >
> > > > I might have missed why -EINVAL is ok
> > >
> > > When the index is one more than the index of the last available frame duration,
> > > then EINVAL is returned, which breaks the loop, but it is not an error condition.
> > >
> >
> > Ah, right right
> >
> > >
> > > >
> > > > > +		LOG(V4L2, Error)
> > > > > +			<< "Unable to enumerate pixel formats: "
> > > > > +			<< strerror(-ret);
> > > > > +		return {};
> > > > > +	}
> > > > > +
> > > > > +	if (index <= 0)
> > > >
> > > > How can index be < 0 ?
> > >
> > > Admittedly it can't... I just like to do `<= 0` regardless of type.
> >
> > Which creates confusion in the reader which might ask "what am I
> > missing that could lead to index be < 0" ?
>
> Okay, I'll change it.
>

Thanks, it's a tiny detail anyway

Thanks
  j

>
> >
> > >
> > >
> > > >
> > > > > +		return {};
> > > > > +
> > > > > +	return {{ min, max }};
> > > > > +}
> > > > > +
> > > > >    std::vector<V4L2PixelFormat> V4L2VideoDevice::enumPixelformats(uint32_t code)
> > > > >    {
> > > > >    	std::vector<V4L2PixelFormat> formats;
> > > > > --
> > > > > 2.52.0
> > > > >
> > >
>

2025. 12. 11. 12:55 keltezéssel, Jacopo Mondi írta:
> Hi Barnabás
> 
> On Thu, Dec 11, 2025 at 10:38:19AM +0100, Barnabás Pőcze wrote:
>> Hi
>>
>> 2025. 12. 11. 10:15 keltezéssel, Jacopo Mondi írta:
>>> Hi Barnabás
>>>
>>> On Wed, Dec 10, 2025 at 05:56:01PM +0100, Barnabás Pőcze wrote:
>>>> Hi
>>>>
>>>> 2025. 12. 10. 17:04 keltezéssel, Jacopo Mondi írta:
>>>>> Hi Barnabás
>>>>>
>>>>> On Wed, Dec 10, 2025 at 02:37:04PM +0100, Barnabás Pőcze wrote:
>>>>>> Setting a frame rate is possible with UVC devices using `VIDIOC_S_PARM`.
>>>>>> However, compared to other platforms supported by libcamera, limitations
>>>>>> apply. Probably most crucially, the desired frame rate cannot be set
>>>>>> while the camera is streaming. Furthermore, it is only a single value
>>>>>> and not an allowed range.
>>>>>>
>>>>>> So this change only adds support for `FrameDurationLimits` in the control
>>>>>> list passed to `Camera::start()`, and the control is otherwise ignored in
>>>>>> requests.
>>>>>>
>>>>>> The kernel interface also only allows a single number and not a range,
>>>>>> so the midpoint of the desired range is used. Checking the supplied
>>>>>
>>>>> Everything else sounds fine, but I wonder if taking the midpoint is
>>>>> the best solution here or we should simply use the shortest provided
>>>>> duration, warn the user, and let the kernel adjust it to the closest
>>>>> supported value.
>>>>
>>>> As far as I understand, the kernel selects the closest valid frame duration
>>>> value. Which means that using the midpoint should guarantee that if the device
>>>> supports at least one frame duration value in the desired range, then a value
>>>> in the desired range will be selected on the device. This is not guaranteed if
>>>> either the min or max is sent to the kernel. That's why I chose this behaviour;
>>>> I believe it's probably best to select a frame duration from the desired range,
>>>> if possible.
>>>
>>> Well, you have a cache of those now, don't you ?
>>
>> Only the min/max is stored for a particular (pixel format, size) combination,
>> and that is only done to be able to report it in the `ControlInfo` when the
>> configuration changes.
> 
> Right, sorry, I've overlooked it
> 
>>
>> Even if there all the frame interval options were stored, I still think selecting
>> the midpoint is the best that we can do. Unless I am missing something?
>>
> 
> I'm in two minds here.
> 
> I'm fine if we select the midpoint, but I would log the user about
> what we're doing (and about the fact uvc doesn't support frame
> duration ranges)

I have already added a debug message to V4L2VideoDevice::setFrameInterval that shows
the wanted interval as well as the actual interval that was set on the device, for the
next version.

Do you envision we need a "warning" or "debug" message in uvcvideo.cpp as well?
My thinking is that such a warning does not seem actionable for either the user
or application developer. I believe we don't want application developers to have
to special case uvc cameras just to avoid a warning, and in that case it's most
likely not actionable for the user either, unless they are in an app like camshark
where they have direct control over such settings.


> 
>>
>>>
>>>>
>>>>
>>>>>
>>>>>> values is not necessary since the kernel will adjust the value if
>>>>>> it is not supported by the device.
>>>>>>
>>>>>> Initially the global min/max values are advertised in the `ControlInfo`
>>>>>> of the `FrameDurationLimits` control, which are then updated after
>>>>>> the camera is configured. Updating the control limits after configuration
>>>>>> matches the behaviour of other platforms.
>>>>>>
>>>>>> While the kernel interface differentiates three types of frame intervals
>>>>>> (discrete, continuous, stepwise), when querying the available frame intervals
>>>>>> for a given (pixel format, size) combination, all options are evaluated
>>>>>> and only the "local" minimum and maximum is used, as that is the only way
>>>>>> the limits can reasonably be advertised on the libcamera side.
>>>>>>
>>>>>> Closes: https://gitlab.freedesktop.org/camera/libcamera/-/issues/296
>>>>>> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
>>>>>> ---
>>>>>>     include/libcamera/internal/v4l2_videodevice.h |   3 +
>>>>>>     src/libcamera/pipeline/uvcvideo/uvcvideo.cpp  |  59 +++++++++
>>>>>>     src/libcamera/v4l2_videodevice.cpp            | 116 ++++++++++++++++++
>>>>>>     3 files changed, 178 insertions(+)
>>>>>>
>>>>>> diff --git a/include/libcamera/internal/v4l2_videodevice.h b/include/libcamera/internal/v4l2_videodevice.h
>>>>>> index 82d98184ed..e97c0f9bf8 100644
>>>>>> --- a/include/libcamera/internal/v4l2_videodevice.h
>>>>>> +++ b/include/libcamera/internal/v4l2_videodevice.h
>>>>>> @@ -209,6 +209,9 @@ public:
> [...]
>>>>>> diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
>>>>>> index 3f98e8ece0..e4e9b8ab9b 100644
>>>>>> --- a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
>>>>>> +++ b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
>>>>>> @@ -57,6 +57,7 @@ public:
> [...]
>>>>>> diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp
>>>>>> index 3836dabef3..3db6e80aed 100644
>>>>>> --- a/src/libcamera/v4l2_videodevice.cpp
>>>>>> +++ b/src/libcamera/v4l2_videodevice.cpp
>>>>>> @@ -1202,6 +1202,122 @@ int V4L2VideoDevice::getFrameInterval(std::chrono::microseconds *interval)
>>>>>>     	return 0;
>>>>>>     }
>>>>>>
>>>>>> +/**
>>>>>> + * \brief Configure the frame interval
>>>>>> + * \param[inout] interval The frame interval
>>>>>
>>>>> unit would be nice here as well
>>>>
>>>> Ok.
>>>>
>>>>>
>>>>>> + *
>>>>>> + * Apply the supplied \a interval as the time-per-frame stream parameter
>>>>>> + * on the device, and return the actually applied value.
>>>>>> + *
>>>>>> + * \return 0 on success or a negative error code otherwise
>>>>>> + */
>>>>>> +int V4L2VideoDevice::setFrameInterval(std::chrono::microseconds *interval)
>>>>>> +{
>>>>>> +	v4l2_fract *frameInterval = nullptr;
>>>>>> +	uint32_t *caps = nullptr;
>>>>>> +	v4l2_streamparm sparm = {};
>>>>>> +
>>>>>> +	sparm.type = bufferType_;
>>>>>> +
>>>>>> +	switch (sparm.type) {
>>>>>> +	case V4L2_BUF_TYPE_VIDEO_CAPTURE:
>>>>>> +	case V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE:
>>>>>> +		frameInterval = &sparm.parm.capture.timeperframe;
>>>>>> +		caps = &sparm.parm.capture.capability;
>>>>>> +		break;
>>>>>> +	case V4L2_BUF_TYPE_VIDEO_OUTPUT:
>>>>>> +	case V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE:
>>>>>> +		frameInterval = &sparm.parm.output.timeperframe;
>>>>>> +		caps = &sparm.parm.output.capability;
>>>>>> +		break;
>>>>>> +	}
>>>>>> +
>>>>>> +	if (!frameInterval)
>>>>>> +		return -EINVAL;
>>>>>> +
>>>>>> +	constexpr auto max = std::numeric_limits<decltype(frameInterval->numerator)>::max();
>>>>>
>>>>> Feels quite a corner case, but checking doesn't hurt ?
>>>>>
>>>>>> +	if (interval->count() <= 0 || interval->count() > max)
>>>>>> +		return -EINVAL;
>>>>>> +
>>>>>> +	frameInterval->numerator = interval->count();
>>>>>> +	frameInterval->denominator = std::chrono::microseconds(std::chrono::seconds(1)).count();
>>>>>> +
>>>>>> +	int ret = ioctl(VIDIOC_S_PARM, &sparm);
>>>>>> +	if (ret)
>>>>>> +		return ret;
>>>>>> +
>>>>>> +	if (!(*caps & V4L2_CAP_TIMEPERFRAME))
>>>>>> +		return -ENOTSUP;
>>>>>
>>>>> In this case, I' pretty sure you should check this flag before calling
>>>>> ioctl()
>>>>
>>>> I think it's fine. If `V4L2_CAP_TIMEPERFRAME` is not present, then nothing
>>>> (should) changes on the device and 0 is returned, in which case this condition
>>>> detects that; or the driver returns an error code, which is detected above.
>>>
>>> Why going through the trouble of an ioctl if we know that it's not
>>> supported ? Ah! or is caps only updated -after- the ioctl call ?
>>
>> The ioctl updates the capabilities, yes. I think this could be checked in `open()` and remembered.
>> Would that be better?
> 
> The V4L2_CAP_TIMEPERFRAME cap is found in the v4l2_streamparm
> structure. Can you get it from other caps at open() time or are you
> considering issueing a g_parm ioctl() to check if it is supported in
> open() ?
> 

I was thinking about doing a `VIDIOC_G_PARM` in `open()`, I'm not sure if
this capability can be determined in any other way.


> [...]

Hi Barnabás

On Thu, Dec 11, 2025 at 01:21:38PM +0100, Barnabás Pőcze wrote:
> 2025. 12. 11. 12:55 keltezéssel, Jacopo Mondi írta:
> > Hi Barnabás
> >
> > On Thu, Dec 11, 2025 at 10:38:19AM +0100, Barnabás Pőcze wrote:
> > > Hi
> > >
> > > 2025. 12. 11. 10:15 keltezéssel, Jacopo Mondi írta:
> > > > Hi Barnabás
> > > >
> > > > On Wed, Dec 10, 2025 at 05:56:01PM +0100, Barnabás Pőcze wrote:
> > > > > Hi
> > > > >
> > > > > 2025. 12. 10. 17:04 keltezéssel, Jacopo Mondi írta:
> > > > > > Hi Barnabás
> > > > > >
> > > > > > On Wed, Dec 10, 2025 at 02:37:04PM +0100, Barnabás Pőcze wrote:
> > > > > > > Setting a frame rate is possible with UVC devices using `VIDIOC_S_PARM`.
> > > > > > > However, compared to other platforms supported by libcamera, limitations
> > > > > > > apply. Probably most crucially, the desired frame rate cannot be set
> > > > > > > while the camera is streaming. Furthermore, it is only a single value
> > > > > > > and not an allowed range.
> > > > > > >
> > > > > > > So this change only adds support for `FrameDurationLimits` in the control
> > > > > > > list passed to `Camera::start()`, and the control is otherwise ignored in
> > > > > > > requests.
> > > > > > >
> > > > > > > The kernel interface also only allows a single number and not a range,
> > > > > > > so the midpoint of the desired range is used. Checking the supplied
> > > > > >
> > > > > > Everything else sounds fine, but I wonder if taking the midpoint is
> > > > > > the best solution here or we should simply use the shortest provided
> > > > > > duration, warn the user, and let the kernel adjust it to the closest
> > > > > > supported value.
> > > > >
> > > > > As far as I understand, the kernel selects the closest valid frame duration
> > > > > value. Which means that using the midpoint should guarantee that if the device
> > > > > supports at least one frame duration value in the desired range, then a value
> > > > > in the desired range will be selected on the device. This is not guaranteed if
> > > > > either the min or max is sent to the kernel. That's why I chose this behaviour;
> > > > > I believe it's probably best to select a frame duration from the desired range,
> > > > > if possible.
> > > >
> > > > Well, you have a cache of those now, don't you ?
> > >
> > > Only the min/max is stored for a particular (pixel format, size) combination,
> > > and that is only done to be able to report it in the `ControlInfo` when the
> > > configuration changes.
> >
> > Right, sorry, I've overlooked it
> >
> > >
> > > Even if there all the frame interval options were stored, I still think selecting
> > > the midpoint is the best that we can do. Unless I am missing something?
> > >
> >
> > I'm in two minds here.
> >
> > I'm fine if we select the midpoint, but I would log the user about
> > what we're doing (and about the fact uvc doesn't support frame
> > duration ranges)
>
> I have already added a debug message to V4L2VideoDevice::setFrameInterval that shows
> the wanted interval as well as the actual interval that was set on the device, for the
> next version.
>
> Do you envision we need a "warning" or "debug" message in uvcvideo.cpp as well?

That's what I was thinking about

> My thinking is that such a warning does not seem actionable for either the user
> or application developer. I believe we don't want application developers to have
> to special case uvc cameras just to avoid a warning, and in that case it's most
> likely not actionable for the user either, unless they are in an app like camshark
> where they have direct control over such settings.

That's true, it can't be actioned, but it will provide users an
indication that what they are asking for (a ranging FPS) cannot be
obtained.

Up to you, I really don't have a strong opinion here

>
>
> >
> > >
> > > >
> > > > >
> > > > >
> > > > > >
> > > > > > > values is not necessary since the kernel will adjust the value if
> > > > > > > it is not supported by the device.
> > > > > > >
> > > > > > > Initially the global min/max values are advertised in the `ControlInfo`
> > > > > > > of the `FrameDurationLimits` control, which are then updated after
> > > > > > > the camera is configured. Updating the control limits after configuration
> > > > > > > matches the behaviour of other platforms.
> > > > > > >
> > > > > > > While the kernel interface differentiates three types of frame intervals
> > > > > > > (discrete, continuous, stepwise), when querying the available frame intervals
> > > > > > > for a given (pixel format, size) combination, all options are evaluated
> > > > > > > and only the "local" minimum and maximum is used, as that is the only way
> > > > > > > the limits can reasonably be advertised on the libcamera side.
> > > > > > >
> > > > > > > Closes: https://gitlab.freedesktop.org/camera/libcamera/-/issues/296
> > > > > > > Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
> > > > > > > ---
> > > > > > >     include/libcamera/internal/v4l2_videodevice.h |   3 +
> > > > > > >     src/libcamera/pipeline/uvcvideo/uvcvideo.cpp  |  59 +++++++++
> > > > > > >     src/libcamera/v4l2_videodevice.cpp            | 116 ++++++++++++++++++
> > > > > > >     3 files changed, 178 insertions(+)
> > > > > > >
> > > > > > > diff --git a/include/libcamera/internal/v4l2_videodevice.h b/include/libcamera/internal/v4l2_videodevice.h
> > > > > > > index 82d98184ed..e97c0f9bf8 100644
> > > > > > > --- a/include/libcamera/internal/v4l2_videodevice.h
> > > > > > > +++ b/include/libcamera/internal/v4l2_videodevice.h
> > > > > > > @@ -209,6 +209,9 @@ public:
> > [...]
> > > > > > > diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
> > > > > > > index 3f98e8ece0..e4e9b8ab9b 100644
> > > > > > > --- a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
> > > > > > > +++ b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
> > > > > > > @@ -57,6 +57,7 @@ public:
> > [...]
> > > > > > > diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp
> > > > > > > index 3836dabef3..3db6e80aed 100644
> > > > > > > --- a/src/libcamera/v4l2_videodevice.cpp
> > > > > > > +++ b/src/libcamera/v4l2_videodevice.cpp
> > > > > > > @@ -1202,6 +1202,122 @@ int V4L2VideoDevice::getFrameInterval(std::chrono::microseconds *interval)
> > > > > > >     	return 0;
> > > > > > >     }
> > > > > > >
> > > > > > > +/**
> > > > > > > + * \brief Configure the frame interval
> > > > > > > + * \param[inout] interval The frame interval
> > > > > >
> > > > > > unit would be nice here as well
> > > > >
> > > > > Ok.
> > > > >
> > > > > >
> > > > > > > + *
> > > > > > > + * Apply the supplied \a interval as the time-per-frame stream parameter
> > > > > > > + * on the device, and return the actually applied value.
> > > > > > > + *
> > > > > > > + * \return 0 on success or a negative error code otherwise
> > > > > > > + */
> > > > > > > +int V4L2VideoDevice::setFrameInterval(std::chrono::microseconds *interval)
> > > > > > > +{
> > > > > > > +	v4l2_fract *frameInterval = nullptr;
> > > > > > > +	uint32_t *caps = nullptr;
> > > > > > > +	v4l2_streamparm sparm = {};
> > > > > > > +
> > > > > > > +	sparm.type = bufferType_;
> > > > > > > +
> > > > > > > +	switch (sparm.type) {
> > > > > > > +	case V4L2_BUF_TYPE_VIDEO_CAPTURE:
> > > > > > > +	case V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE:
> > > > > > > +		frameInterval = &sparm.parm.capture.timeperframe;
> > > > > > > +		caps = &sparm.parm.capture.capability;
> > > > > > > +		break;
> > > > > > > +	case V4L2_BUF_TYPE_VIDEO_OUTPUT:
> > > > > > > +	case V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE:
> > > > > > > +		frameInterval = &sparm.parm.output.timeperframe;
> > > > > > > +		caps = &sparm.parm.output.capability;
> > > > > > > +		break;
> > > > > > > +	}
> > > > > > > +
> > > > > > > +	if (!frameInterval)
> > > > > > > +		return -EINVAL;
> > > > > > > +
> > > > > > > +	constexpr auto max = std::numeric_limits<decltype(frameInterval->numerator)>::max();
> > > > > >
> > > > > > Feels quite a corner case, but checking doesn't hurt ?
> > > > > >
> > > > > > > +	if (interval->count() <= 0 || interval->count() > max)
> > > > > > > +		return -EINVAL;
> > > > > > > +
> > > > > > > +	frameInterval->numerator = interval->count();
> > > > > > > +	frameInterval->denominator = std::chrono::microseconds(std::chrono::seconds(1)).count();
> > > > > > > +
> > > > > > > +	int ret = ioctl(VIDIOC_S_PARM, &sparm);
> > > > > > > +	if (ret)
> > > > > > > +		return ret;
> > > > > > > +
> > > > > > > +	if (!(*caps & V4L2_CAP_TIMEPERFRAME))
> > > > > > > +		return -ENOTSUP;
> > > > > >
> > > > > > In this case, I' pretty sure you should check this flag before calling
> > > > > > ioctl()
> > > > >
> > > > > I think it's fine. If `V4L2_CAP_TIMEPERFRAME` is not present, then nothing
> > > > > (should) changes on the device and 0 is returned, in which case this condition
> > > > > detects that; or the driver returns an error code, which is detected above.
> > > >
> > > > Why going through the trouble of an ioctl if we know that it's not
> > > > supported ? Ah! or is caps only updated -after- the ioctl call ?
> > >
> > > The ioctl updates the capabilities, yes. I think this could be checked in `open()` and remembered.
> > > Would that be better?
> >
> > The V4L2_CAP_TIMEPERFRAME cap is found in the v4l2_streamparm
> > structure. Can you get it from other caps at open() time or are you
> > considering issueing a g_parm ioctl() to check if it is supported in
> > open() ?
> >
>
> I was thinking about doing a `VIDIOC_G_PARM` in `open()`, I'm not sure if
> this capability can be determined in any other way.

Considering that g/s_parm will only be called on uvc, I would spare
calling it at open() time for all other platforms.

Thanks
  j

>
>
> > [...]
>