[v5] virtual: Implement framerate handling
diff mbox series

Message ID 20261015084539.215459-1-robert.mader@collabora.com
State Superseded
Headers show
Series
  • [v5] virtual: Implement framerate handling
Related show

Commit Message

Robert Mader Oct. 15, 2026, 8:45 a.m. UTC 
Throttle requests to the minimal frame duration in order to honor the
configured framerates.

Signed-off-by: Robert Mader <robert.mader@collabora.com>

---

Changes in v5:
 - Also clamp frame duration limit to highest one allowed by the config
   so clients can't set a unsupported frame rate.
 - Added comments for that calculations using frame rate logic, as that is
   what the config files use.
 - Successfully tested the patch against the following Pipewire MR:
   https://gitlab.freedesktop.org/pipewire/pipewire/-/merge_requests/3059

Changes in v4:
 - Calculate minFrameDuration_ once in start() using FDLs from camera
   and, if present, from client provided controls. The later is not yet
   tested.
 - Assert that the camera has FDLs set by the config parser.
 - Set FrameDuration in request metadata.

Changes in v3:
 - Removed a leftover debug change

Changes in v2:
 - Remove debug log in order to avoid CI issues.
 - Simplify code in queueRequestDevice(), hopefully making it easier to
   follow.

Changes in v1 (from RFC version):
 - Use std::this_thread::sleep_for() instead usleep() (Barnabás)
 - Sleep only once per request, after generating all frames
 - Remove control handling in start() (defer that for later)
 - Reset state in stop() instead of start()
---
 src/libcamera/pipeline/virtual/virtual.cpp | 59 +++++++++++++++++++---
 src/libcamera/pipeline/virtual/virtual.h   |  3 ++
 2 files changed, 54 insertions(+), 8 deletions(-)

Comments

Robert Mader Oct. 15, 2026, 10:03 a.m. UTC | #1 
On 15.10.26 11:16, Kieran Bingham wrote:
> Quoting Robert Mader (2026-10-15 09:45:39)
>> Throttle requests to the minimal frame duration in order to honor the
>> configured framerates.
>>
>> Signed-off-by: Robert Mader <robert.mader@collabora.com>
>>
>> ---
>>
>> Changes in v5:
>>   - Also clamp frame duration limit to highest one allowed by the config
>>     so clients can't set a unsupported frame rate.
>>   - Added comments for that calculations using frame rate logic, as that is
>>     what the config files use.
>>   - Successfully tested the patch against the following Pipewire MR:
>>     https://gitlab.freedesktop.org/pipewire/pipewire/-/merge_requests/3059
>>
>> Changes in v4:
>>   - Calculate minFrameDuration_ once in start() using FDLs from camera
>>     and, if present, from client provided controls. The later is not yet
>>     tested.
>>   - Assert that the camera has FDLs set by the config parser.
>>   - Set FrameDuration in request metadata.
>>
>> Changes in v3:
>>   - Removed a leftover debug change
>>
>> Changes in v2:
>>   - Remove debug log in order to avoid CI issues.
>>   - Simplify code in queueRequestDevice(), hopefully making it easier to
>>     follow.
>>
>> Changes in v1 (from RFC version):
>>   - Use std::this_thread::sleep_for() instead usleep() (Barnabás)
>>   - Sleep only once per request, after generating all frames
>>   - Remove control handling in start() (defer that for later)
>>   - Reset state in stop() instead of start()
>> ---
>>   src/libcamera/pipeline/virtual/virtual.cpp | 59 +++++++++++++++++++---
>>   src/libcamera/pipeline/virtual/virtual.h   |  3 ++
>>   2 files changed, 54 insertions(+), 8 deletions(-)
>>
>> diff --git a/src/libcamera/pipeline/virtual/virtual.cpp b/src/libcamera/pipeline/virtual/virtual.cpp
>> index e25dd85a7391..9c1ecb8584af 100644
>> --- a/src/libcamera/pipeline/virtual/virtual.cpp
>> +++ b/src/libcamera/pipeline/virtual/virtual.cpp
>> @@ -130,6 +130,9 @@ VirtualCameraData::VirtualCameraData(PipelineHandler *pipe,
>>   
>>   void VirtualCameraData::processRequest(Request *request)
>>   {
>> +       int64_t sensorTimestamp = *request->metadata().get(controls::SensorTimestamp);
>> +       std::vector<FrameBuffer *> buffers;
>> +
>>          for (const auto &[stream, buffer] : request->buffers()) {
>>                  bool found = false;
>>                  /* map buffer and fill test patterns */
>> @@ -139,7 +142,7 @@ void VirtualCameraData::processRequest(Request *request)
>>   
>>                                  fmd.status = FrameMetadata::Status::FrameSuccess;
>>                                  fmd.sequence = streamConfig.seq++;
>> -                               fmd.timestamp = currentTimestamp();
>> +                               fmd.timestamp = sensorTimestamp;
>>   
>>                                  std::span<const FrameBuffer::Plane> planes = buffer->planes();
>>                                  for (const auto [i, p] : utils::enumerate(planes))
>> @@ -151,12 +154,21 @@ void VirtualCameraData::processRequest(Request *request)
>>                                              stream->configuration().size, buffer))
>>                                          fmd.status = FrameMetadata::Status::FrameError;
>>   
>> -                               bufferCompleted.emit(buffer);
>> +                               buffers.push_back(buffer);
>>                                  break;
>>                          }
>>                  }
>>                  ASSERT(found);
>>          }
>> +
>> +       const uint64_t now = currentTimestamp();
>> +       if (now < static_cast<uint64_t>(sensorTimestamp)) {
>> +               std::chrono::nanoseconds duration(sensorTimestamp - now);
>> +               std::this_thread::sleep_for(duration);
>> +       }
>> +
>> +       for (auto buffer : buffers)
>> +               bufferCompleted.emit(buffer);
>>   }
>>   
>>   VirtualCameraConfiguration::VirtualCameraConfiguration(VirtualCameraData *data)
>> @@ -323,11 +335,30 @@ int PipelineHandlerVirtual::exportFrameBuffers([[maybe_unused]] Camera *camera,
>>          return dmaBufAllocator_.exportBuffers(config.bufferCount, planeSizes, buffers);
>>   }
>>   
>> -int PipelineHandlerVirtual::start([[maybe_unused]] Camera *camera,
>> -                                 [[maybe_unused]] const ControlList *controls)
>> +int PipelineHandlerVirtual::start(Camera *camera, const ControlList *controls)
>>   {
>>          VirtualCameraData *data = cameraData(camera);
>>   
>> +       auto cameraFrameDurationLimits =
>> +               camera->controls().find(&controls::FrameDurationLimits);
>> +       ASSERT(cameraFrameDurationLimits != camera->controls().end());
>> +
>> +       /* By default target the highest frame rate allowed by the config */
>> +       data->minFrameDuration_ =
>> +               cameraFrameDurationLimits->second.min().get<int64_t>();
>> +
>> +       if (controls) {
>> +               if (auto clientFrameDurationLimits =
>> +                           controls->get(controls::FrameDurationLimits)) {
>> +                       /* Clamp frame rate to the highest one allowed by the client */
>> +                       data->minFrameDuration_ = std::max(data->minFrameDuration_,
>> +                                                          clientFrameDurationLimits->front());
>> +                       /* Clamp frame rate to the lowest one allowed by the config */
>> +                       data->minFrameDuration_ = std::min(data->minFrameDuration_,
>> +                                                          cameraFrameDurationLimits->second.max().get<int64_t>());
> I'm surprised this isn't using std::clamp(v, lo, hi);
>
> 		auto limits = controls->get(controls::FrameDurationLimits);
> 		if (limits) {
> 			data->minFrameDuration_ = std::clamp(limits->front(),
> 							     cameraFrameDurationLimits->second.min().get<int64_t>(),
> 							     cameraFrameDurationLimits->second.max().get<int64_t>());
> 		}
>
Oh, right - happy to send out a v6 doing that if you prefer.
> but yours isn't wrong as far as I can tell so
>
> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
>
> I'd love to revisit a 'Interval/Bounds/Range/Limits' type[0] to be able
> to better convey a pair of min/max values sometime. It's used so
> frequently throughout the stack it would be nice to keep them tightly
> together, but that's not something for this patch.
>
>   - [0] https://git.ideasonboard.com/kbingham/libcamera/pulls/23/commits
>
> Or I could imagine some wrapper on the control info maps though that we
> could just clamp a value to a ControlInfo directly.
Sounds good indeed.
>
> Anyway, I don't think that's needed here right now.
>
> --
> Kieran
>
>
>
>
>> +               }
>> +       }
>> +
>>          for (auto &s : data->streamConfigs_)
>>                  s.seq = 0;
>>   
>> @@ -349,18 +380,30 @@ void PipelineHandlerVirtual::stopDevice(Camera *camera)
>>          /* Process pending `bufferCompleted` signals. */
>>          thread()->dispatchMessages(Message::Type::InvokeMessage, this);
>>          data->bufferCompleted.disconnect(this);
>> +       data->minFrameDuration_ = 0;
>> +       data->lastTimestamp_ = 0;
>>   
>>          while (!data->queuedRequests_.empty())
>>                  cancelRequest(data->queuedRequests_.front());
>>   }
>>   
>> -int PipelineHandlerVirtual::queueRequestDevice([[maybe_unused]] Camera *camera,
>> -                                              Request *request)
>> +int PipelineHandlerVirtual::queueRequestDevice(Camera *camera, Request *request)
>>   {
>>          VirtualCameraData *data = cameraData(camera);
>> -       const auto timestamp = currentTimestamp();
>>   
>> -       request->_d()->metadata().set(controls::SensorTimestamp, timestamp);
>> +       const uint64_t now = currentTimestamp();
>> +       uint64_t targetTimestamp =
>> +               data->lastTimestamp_ + data->minFrameDuration_ * 1000;
>> +
>> +       if (targetTimestamp < now)
>> +               targetTimestamp = now;
>> +       data->lastTimestamp_ = targetTimestamp;
>> +
>> +       request->_d()->metadata().set(controls::SensorTimestamp,
>> +                                     targetTimestamp);
>> +       request->_d()->metadata().set(controls::FrameDuration,
>> +                                     data->minFrameDuration_);
>> +
>>          data->invokeMethod(&VirtualCameraData::processRequest,
>>                             ConnectionTypeQueued, request);
>>   
>> diff --git a/src/libcamera/pipeline/virtual/virtual.h b/src/libcamera/pipeline/virtual/virtual.h
>> index 215e56fa3b9d..477f5b15a88f 100644
>> --- a/src/libcamera/pipeline/virtual/virtual.h
>> +++ b/src/libcamera/pipeline/virtual/virtual.h
>> @@ -66,6 +66,9 @@ public:
>>   
>>          std::vector<StreamConfig> streamConfigs_;
>>          Signal<FrameBuffer *> bufferCompleted;
>> +
>> +       int64_t minFrameDuration_ = 0;
>> +       uint64_t lastTimestamp_ = 0;
>>   };
>>   
>>   } /* namespace libcamera */
>> -- 
>> 2.56.0
>>

Patch
diff mbox series 

diff --git a/src/libcamera/pipeline/virtual/virtual.cpp b/src/libcamera/pipeline/virtual/virtual.cpp
index e25dd85a7391..9c1ecb8584af 100644
--- a/src/libcamera/pipeline/virtual/virtual.cpp
+++ b/src/libcamera/pipeline/virtual/virtual.cpp
@@ -130,6 +130,9 @@  VirtualCameraData::VirtualCameraData(PipelineHandler *pipe,
 
 void VirtualCameraData::processRequest(Request *request)
 {
+	int64_t sensorTimestamp = *request->metadata().get(controls::SensorTimestamp);
+	std::vector<FrameBuffer *> buffers;
+
 	for (const auto &[stream, buffer] : request->buffers()) {
 		bool found = false;
 		/* map buffer and fill test patterns */
@@ -139,7 +142,7 @@  void VirtualCameraData::processRequest(Request *request)
 
 				fmd.status = FrameMetadata::Status::FrameSuccess;
 				fmd.sequence = streamConfig.seq++;
-				fmd.timestamp = currentTimestamp();
+				fmd.timestamp = sensorTimestamp;
 
 				std::span<const FrameBuffer::Plane> planes = buffer->planes();
 				for (const auto [i, p] : utils::enumerate(planes))
@@ -151,12 +154,21 @@  void VirtualCameraData::processRequest(Request *request)
 					    stream->configuration().size, buffer))
 					fmd.status = FrameMetadata::Status::FrameError;
 
-				bufferCompleted.emit(buffer);
+				buffers.push_back(buffer);
 				break;
 			}
 		}
 		ASSERT(found);
 	}
+
+	const uint64_t now = currentTimestamp();
+	if (now < static_cast<uint64_t>(sensorTimestamp)) {
+		std::chrono::nanoseconds duration(sensorTimestamp - now);
+		std::this_thread::sleep_for(duration);
+	}
+
+	for (auto buffer : buffers)
+		bufferCompleted.emit(buffer);
 }
 
 VirtualCameraConfiguration::VirtualCameraConfiguration(VirtualCameraData *data)
@@ -323,11 +335,30 @@  int PipelineHandlerVirtual::exportFrameBuffers([[maybe_unused]] Camera *camera,
 	return dmaBufAllocator_.exportBuffers(config.bufferCount, planeSizes, buffers);
 }
 
-int PipelineHandlerVirtual::start([[maybe_unused]] Camera *camera,
-				  [[maybe_unused]] const ControlList *controls)
+int PipelineHandlerVirtual::start(Camera *camera, const ControlList *controls)
 {
 	VirtualCameraData *data = cameraData(camera);
 
+	auto cameraFrameDurationLimits =
+		camera->controls().find(&controls::FrameDurationLimits);
+	ASSERT(cameraFrameDurationLimits != camera->controls().end());
+
+	/* By default target the highest frame rate allowed by the config */
+	data->minFrameDuration_ =
+		cameraFrameDurationLimits->second.min().get<int64_t>();
+
+	if (controls) {
+		if (auto clientFrameDurationLimits =
+			    controls->get(controls::FrameDurationLimits)) {
+			/* Clamp frame rate to the highest one allowed by the client */
+			data->minFrameDuration_ = std::max(data->minFrameDuration_,
+							   clientFrameDurationLimits->front());
+			/* Clamp frame rate to the lowest one allowed by the config */
+			data->minFrameDuration_ = std::min(data->minFrameDuration_,
+							   cameraFrameDurationLimits->second.max().get<int64_t>());
+		}
+	}
+
 	for (auto &s : data->streamConfigs_)
 		s.seq = 0;
 
@@ -349,18 +380,30 @@  void PipelineHandlerVirtual::stopDevice(Camera *camera)
 	/* Process pending `bufferCompleted` signals. */
 	thread()->dispatchMessages(Message::Type::InvokeMessage, this);
 	data->bufferCompleted.disconnect(this);
+	data->minFrameDuration_ = 0;
+	data->lastTimestamp_ = 0;
 
 	while (!data->queuedRequests_.empty())
 		cancelRequest(data->queuedRequests_.front());
 }
 
-int PipelineHandlerVirtual::queueRequestDevice([[maybe_unused]] Camera *camera,
-					       Request *request)
+int PipelineHandlerVirtual::queueRequestDevice(Camera *camera, Request *request)
 {
 	VirtualCameraData *data = cameraData(camera);
-	const auto timestamp = currentTimestamp();
 
-	request->_d()->metadata().set(controls::SensorTimestamp, timestamp);
+	const uint64_t now = currentTimestamp();
+	uint64_t targetTimestamp =
+		data->lastTimestamp_ + data->minFrameDuration_ * 1000;
+
+	if (targetTimestamp < now)
+		targetTimestamp = now;
+	data->lastTimestamp_ = targetTimestamp;
+
+	request->_d()->metadata().set(controls::SensorTimestamp,
+				      targetTimestamp);
+	request->_d()->metadata().set(controls::FrameDuration,
+				      data->minFrameDuration_);
+
 	data->invokeMethod(&VirtualCameraData::processRequest,
 			   ConnectionTypeQueued, request);
 
diff --git a/src/libcamera/pipeline/virtual/virtual.h b/src/libcamera/pipeline/virtual/virtual.h
index 215e56fa3b9d..477f5b15a88f 100644
--- a/src/libcamera/pipeline/virtual/virtual.h
+++ b/src/libcamera/pipeline/virtual/virtual.h
@@ -66,6 +66,9 @@  public:
 
 	std::vector<StreamConfig> streamConfigs_;
 	Signal<FrameBuffer *> bufferCompleted;
+
+	int64_t minFrameDuration_ = 0;
+	uint64_t lastTimestamp_ = 0;
 };
 
 } /* namespace libcamera */