[v7] virtual: Implement frame rate handling
diff mbox series

Message ID 20261020132409.37650-1-robert.mader@collabora.com
State Superseded
Headers show
Series
  • [v7] virtual: Implement frame rate handling
Related show

Commit Message

Robert Mader Oct. 20, 2026, 1:24 p.m. UTC 
Throttle requests to the minimal frame duration in order to honor the
configured frame rates.

Signed-off-by: Robert Mader <robert.mader@collabora.com>
Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

---

Changes in v7:
 - Address feedback from Laurent
 - Replace lastTimestamp_ with targetTimestamp_ and simplify calculation

Changes in v6:
 - Use std::clamp (Kieran) and minor code reshuffle in start()
 - Use "frame rate" consistently
 - Added R-B from Kieran (assuming the changes are ok)

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 | 58 ++++++++++++++++++----
 src/libcamera/pipeline/virtual/virtual.h   |  5 +-
 2 files changed, 53 insertions(+), 10 deletions(-)

Comments

Barnabás Pőcze Oct. 22, 2026, 4:46 p.m. UTC | #1 
Hi

2026. 10. 20. 15:24 keltezéssel, Robert Mader írta:
> Throttle requests to the minimal frame duration in order to honor the
> configured frame rates.
> 
> Signed-off-by: Robert Mader <robert.mader@collabora.com>
> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> 
> ---
> 
> Changes in v7:
>   - Address feedback from Laurent
>   - Replace lastTimestamp_ with targetTimestamp_ and simplify calculation
> 
> Changes in v6:
>   - Use std::clamp (Kieran) and minor code reshuffle in start()
>   - Use "frame rate" consistently
>   - Added R-B from Kieran (assuming the changes are ok)
> 
> 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 | 58 ++++++++++++++++++----
>   src/libcamera/pipeline/virtual/virtual.h   |  5 +-
>   2 files changed, 53 insertions(+), 10 deletions(-)
> 
> diff --git a/src/libcamera/pipeline/virtual/virtual.cpp b/src/libcamera/pipeline/virtual/virtual.cpp
> index f35af97b103d..13a96e6a9412 100644
> --- a/src/libcamera/pipeline/virtual/virtual.cpp
> +++ b/src/libcamera/pipeline/virtual/virtual.cpp
> @@ -135,8 +135,14 @@ VirtualCameraData::VirtualCameraData(PipelineHandler *pipe,
>   	moveToThread(this);
>   }
>   
> -void VirtualCameraData::processRequest(Request *request)
> +void VirtualCameraData::processRequest(Request *request,
> +				       uint64_t targetTimestamp)
>   {
> +	const auto targetTime = std::chrono::steady_clock::time_point{
> +		std::chrono::nanoseconds(targetTimestamp)
> +	};
> +	std::this_thread::sleep_until(targetTime);
> +
>   	for (const auto &[stream, buffer] : request->buffers()) {
>   		bool found = false;
>   		/* map buffer and fill test patterns */
> @@ -146,7 +152,7 @@ void VirtualCameraData::processRequest(Request *request)
>   
>   				fmd.status = FrameMetadata::Status::FrameSuccess;
>   				fmd.sequence = streamConfig.seq++;
> -				fmd.timestamp = currentTimestamp();
> +				fmd.timestamp = targetTimestamp;
>   
>   				std::span<const FrameBuffer::Plane> planes = buffer->planes();
>   				for (const auto [i, p] : utils::enumerate(planes))
> @@ -348,11 +354,34 @@ 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)) {
> +			/*
> +			 * Target the highest frame rate allowed by the client,
> +			 * clamped to the values allowed by the config.
> +			 */
> +			data->minFrameDuration_ = std::clamp(
> +				clientFrameDurationLimits->front(),
> +				data->minFrameDuration_,
> +				cameraFrameDurationLimits->second.max().get<int64_t>());
> +		}
> +	}
> +
> +	data->targetTimestamp_ = 0;
> +
>   	for (auto &s : data->streamConfigs_)
>   		s.seq = 0;
>   
> @@ -379,15 +408,26 @@ void PipelineHandlerVirtual::stopDevice(Camera *camera)
>   		cancelRequest(data->queuedRequests_.front());
>   }
>   
> -int PipelineHandlerVirtual::queueRequestDevice([[maybe_unused]] Camera *camera,
> -					       Request *request)
> +int PipelineHandlerVirtual::queueRequestDevice(Camera *camera, Request *request)
>   {

Is the lack of handling in `queueRequest()` intentional? Unless there are some
very compelling reasons, I'd like to see that implemented. E.g. when `FrameDurationLimits`
is initialized in `ConfigParser::parseConfigFile()`, you could set a new member like

   std::array<int64_t, 2> frameDurationLimits_;

(possibly making `ConfigParser` and `friend` of `VirtualCameraData`)

and then use that to clamp everywhere. Or am I missing something?




>   	VirtualCameraData *data = cameraData(camera);
> -	const auto timestamp = currentTimestamp();
>   
> -	request->_d()->metadata().set(controls::SensorTimestamp, timestamp);
> +	/*
> +	 * TODO: Skip frames if we can't keep up with the maximum frame
> +	 * duration.
> +	 */
> +	data->targetTimestamp_ =
> +		std::max(currentTimestamp(),
> +			 data->targetTimestamp_ + data->minFrameDuration_ * 1000);
> +
> +	request->_d()->metadata().set(controls::SensorTimestamp,
> +				      data->targetTimestamp_);
> +	request->_d()->metadata().set(controls::FrameDuration,
> +				      data->minFrameDuration_);
> +
>   	data->invokeMethod(&VirtualCameraData::processRequest,
> -			   ConnectionTypeQueued, request);
> +			   ConnectionTypeQueued, request,
> +			   data->targetTimestamp_);
>   
>   	return 0;
>   }
> diff --git a/src/libcamera/pipeline/virtual/virtual.h b/src/libcamera/pipeline/virtual/virtual.h
> index 215e56fa3b9d..40e9095c0c4f 100644
> --- a/src/libcamera/pipeline/virtual/virtual.h
> +++ b/src/libcamera/pipeline/virtual/virtual.h
> @@ -60,12 +60,15 @@ public:
>   
>   	~VirtualCameraData() = default;
>   
> -	void processRequest(Request *request);
> +	void processRequest(Request *request, uint64_t targetTimestamp);
>   
>   	Configuration config_;
>   
>   	std::vector<StreamConfig> streamConfigs_;
>   	Signal<FrameBuffer *> bufferCompleted;
> +
> +	int64_t minFrameDuration_;
> +	uint64_t targetTimestamp_;
>   };
>   
>   } /* namespace libcamera */
Barnabás Pőcze Oct. 22, 2026, 5:20 p.m. UTC | #2 
2026. 10. 22. 18:46 keltezéssel, Barnabás Pőcze írta:
> Hi
> 
> 2026. 10. 20. 15:24 keltezéssel, Robert Mader írta:
>> Throttle requests to the minimal frame duration in order to honor the
>> configured frame rates.
>>
>> Signed-off-by: Robert Mader <robert.mader@collabora.com>
>> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
>>
>> ---
>>
>> Changes in v7:
>>   - Address feedback from Laurent
>>   - Replace lastTimestamp_ with targetTimestamp_ and simplify calculation
>>
>> Changes in v6:
>>   - Use std::clamp (Kieran) and minor code reshuffle in start()
>>   - Use "frame rate" consistently
>>   - Added R-B from Kieran (assuming the changes are ok)
>>
>> 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 | 58 ++++++++++++++++++----
>>   src/libcamera/pipeline/virtual/virtual.h   |  5 +-
>>   2 files changed, 53 insertions(+), 10 deletions(-)
>>
>> diff --git a/src/libcamera/pipeline/virtual/virtual.cpp b/src/libcamera/pipeline/virtual/virtual.cpp
>> index f35af97b103d..13a96e6a9412 100644
>> --- a/src/libcamera/pipeline/virtual/virtual.cpp
>> +++ b/src/libcamera/pipeline/virtual/virtual.cpp
>> @@ -135,8 +135,14 @@ VirtualCameraData::VirtualCameraData(PipelineHandler *pipe,
>>       moveToThread(this);
>>   }
>> -void VirtualCameraData::processRequest(Request *request)
>> +void VirtualCameraData::processRequest(Request *request,
>> +                       uint64_t targetTimestamp)
>>   {
>> +    const auto targetTime = std::chrono::steady_clock::time_point{
>> +        std::chrono::nanoseconds(targetTimestamp)
>> +    };
>> +    std::this_thread::sleep_until(targetTime);
>> +
>>       for (const auto &[stream, buffer] : request->buffers()) {
>>           bool found = false;
>>           /* map buffer and fill test patterns */
>> @@ -146,7 +152,7 @@ void VirtualCameraData::processRequest(Request *request)
>>                   fmd.status = FrameMetadata::Status::FrameSuccess;
>>                   fmd.sequence = streamConfig.seq++;
>> -                fmd.timestamp = currentTimestamp();
>> +                fmd.timestamp = targetTimestamp;
>>                   std::span<const FrameBuffer::Plane> planes = buffer->planes();
>>                   for (const auto [i, p] : utils::enumerate(planes))
>> @@ -348,11 +354,34 @@ 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)) {
>> +            /*
>> +             * Target the highest frame rate allowed by the client,
>> +             * clamped to the values allowed by the config.
>> +             */
>> +            data->minFrameDuration_ = std::clamp(
>> +                clientFrameDurationLimits->front(),
>> +                data->minFrameDuration_,
>> +                cameraFrameDurationLimits->second.max().get<int64_t>());
>> +        }
>> +    }
>> +
>> +    data->targetTimestamp_ = 0;
>> +
>>       for (auto &s : data->streamConfigs_)
>>           s.seq = 0;
>> @@ -379,15 +408,26 @@ void PipelineHandlerVirtual::stopDevice(Camera *camera)
>>           cancelRequest(data->queuedRequests_.front());
>>   }
>> -int PipelineHandlerVirtual::queueRequestDevice([[maybe_unused]] Camera *camera,
>> -                           Request *request)
>> +int PipelineHandlerVirtual::queueRequestDevice(Camera *camera, Request *request)
>>   {
> 
> Is the lack of handling in `queueRequest()` intentional? Unless there are some
> very compelling reasons, I'd like to see that implemented. E.g. when `FrameDurationLimits`
> is initialized in `ConfigParser::parseConfigFile()`, you could set a new member like
> 
>    std::array<int64_t, 2> frameDurationLimits_;
> 
> (possibly making `ConfigParser` and `friend` of `VirtualCameraData`)
> 
> and then use that to clamp everywhere. Or am I missing something?

I have made an attempt, seems to work. Capture script:

```
properties:
   - loop: 500
frames:
   - 0:
       FrameDurationLimits: [ 111111, 111111 ]
   - 50:
       FrameDurationLimits: [ 99999, 99999 ]
   - 100:
       FrameDurationLimits: [ 88888, 88888 ]
   - 150:
       FrameDurationLimits: [ 77777, 77777 ]
   - 200:
       FrameDurationLimits: [ 66666, 66666 ]
   - 250:
       FrameDurationLimits: [ 55555, 55555 ]
   - 300:
       FrameDurationLimits: [ 44444, 44444 ]
   - 350:
       FrameDurationLimits: [ 33333, 33333 ]
   - 400:
       FrameDurationLimits: [ 22222, 22222 ]
   - 450:
       FrameDurationLimits: [ 11111, 11111 ]
```

Diff compared to v7:


```diff
diff --git a/src/libcamera/pipeline/virtual/config_parser.cpp b/src/libcamera/pipeline/virtual/config_parser.cpp
index 5169fd39bc..ec89158c7c 100644
--- a/src/libcamera/pipeline/virtual/config_parser.cpp
+++ b/src/libcamera/pipeline/virtual/config_parser.cpp
@@ -52,12 +52,17 @@ ConfigParser::parseConfigFile(File &file, PipelineHandler *pipe)
  			continue;
  		}
  
-		data->config_.id = cameraId;
  		ControlInfoMap::Map controls;
+
+		data->config_.id = cameraId;
+
  		/* \todo Check which resolution's frame rate to be reported */
+		data->frameDurationLimits_ = {
+			1000000 / data->config_.resolutions[0].frameRates[1],
+			1000000 / data->config_.resolutions[0].frameRates[0],
+		};
  		controls[&controls::FrameDurationLimits] =
-			ControlInfo(1000000 / data->config_.resolutions[0].frameRates[1],
-				    1000000 / data->config_.resolutions[0].frameRates[0]);
+			ControlInfo(data->frameDurationLimits_[0], data->frameDurationLimits_[1]);
  
  		std::vector<ControlValue> supportedFaceDetectModes{
  			static_cast<int32_t>(controls::draft::FaceDetectModeOff),
diff --git a/src/libcamera/pipeline/virtual/data/virtual.yaml b/src/libcamera/pipeline/virtual/data/virtual.yaml
index 20471bb94b..959e83d790 100644
--- a/src/libcamera/pipeline/virtual/data/virtual.yaml
+++ b/src/libcamera/pipeline/virtual/data/virtual.yaml
@@ -6,8 +6,8 @@
    - width: 1920
      height: 1080
      frame_rates:
-    - 30
-    - 60
+    - 1
+    - 120
    - width: 1680
      height: 1050
      frame_rates:
diff --git a/src/libcamera/pipeline/virtual/virtual.cpp b/src/libcamera/pipeline/virtual/virtual.cpp
index 13a96e6a94..8298be6e91 100644
--- a/src/libcamera/pipeline/virtual/virtual.cpp
+++ b/src/libcamera/pipeline/virtual/virtual.cpp
@@ -172,6 +172,22 @@ void VirtualCameraData::processRequest(Request *request,
  	}
  }
  
+void VirtualCameraData::updateFrameDuration(const ControlList &ctrls)
+{
+	auto fdl = ctrls.get(controls::FrameDurationLimits);
+	if (!fdl)
+		return;
+
+	/*
+	 * Target the highest frame rate allowed by the client,
+	 * clamped to the values allowed by the config.
+	 */
+	minFrameDuration_ = std::clamp(
+		fdl->front(),
+		frameDurationLimits_[0],
+		frameDurationLimits_[1]);
+}
+
  VirtualCameraConfiguration::VirtualCameraConfiguration(VirtualCameraData *data)
  	: CameraConfiguration(), data_(data)
  {
@@ -358,27 +374,12 @@ 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>();
+	data->minFrameDuration_ = data->frameDurationLimits_[0];
+	ASSERT(data->minFrameDuration_ > 0);
  
-	if (controls) {
-		if (auto clientFrameDurationLimits =
-			    controls->get(controls::FrameDurationLimits)) {
-			/*
-			 * Target the highest frame rate allowed by the client,
-			 * clamped to the values allowed by the config.
-			 */
-			data->minFrameDuration_ = std::clamp(
-				clientFrameDurationLimits->front(),
-				data->minFrameDuration_,
-				cameraFrameDurationLimits->second.max().get<int64_t>());
-		}
-	}
+	if (controls)
+		data->updateFrameDuration(*controls);
  
  	data->targetTimestamp_ = 0;
  
@@ -412,6 +413,8 @@ int PipelineHandlerVirtual::queueRequestDevice(Camera *camera, Request *request)
  {
  	VirtualCameraData *data = cameraData(camera);
  
+	data->updateFrameDuration(request->controls());
+
  	/*
  	 * TODO: Skip frames if we can't keep up with the maximum frame
  	 * duration.
diff --git a/src/libcamera/pipeline/virtual/virtual.h b/src/libcamera/pipeline/virtual/virtual.h
index 40e9095c0c..48ceda395b 100644
--- a/src/libcamera/pipeline/virtual/virtual.h
+++ b/src/libcamera/pipeline/virtual/virtual.h
@@ -61,12 +61,14 @@ public:
  	~VirtualCameraData() = default;
  
  	void processRequest(Request *request, uint64_t targetTimestamp);
+	void updateFrameDuration(const ControlList &ctrls);
  
  	Configuration config_;
  
  	std::vector<StreamConfig> streamConfigs_;
  	Signal<FrameBuffer *> bufferCompleted;
  
+	std::array<int64_t, 2> frameDurationLimits_ = {};
  	int64_t minFrameDuration_;
  	uint64_t targetTimestamp_;
  };
```


> 
> 
> 
> 
>>       VirtualCameraData *data = cameraData(camera);
>> -    const auto timestamp = currentTimestamp();
>> -    request->_d()->metadata().set(controls::SensorTimestamp, timestamp);
>> +    /*
>> +     * TODO: Skip frames if we can't keep up with the maximum frame
>> +     * duration.
>> +     */
>> +    data->targetTimestamp_ =
>> +        std::max(currentTimestamp(),
>> +             data->targetTimestamp_ + data->minFrameDuration_ * 1000);
>> +
>> +    request->_d()->metadata().set(controls::SensorTimestamp,
>> +                      data->targetTimestamp_);
>> +    request->_d()->metadata().set(controls::FrameDuration,
>> +                      data->minFrameDuration_);
>> +
>>       data->invokeMethod(&VirtualCameraData::processRequest,
>> -               ConnectionTypeQueued, request);
>> +               ConnectionTypeQueued, request,
>> +               data->targetTimestamp_);
>>       return 0;
>>   }
>> diff --git a/src/libcamera/pipeline/virtual/virtual.h b/src/libcamera/pipeline/virtual/virtual.h
>> index 215e56fa3b9d..40e9095c0c4f 100644
>> --- a/src/libcamera/pipeline/virtual/virtual.h
>> +++ b/src/libcamera/pipeline/virtual/virtual.h
>> @@ -60,12 +60,15 @@ public:
>>       ~VirtualCameraData() = default;
>> -    void processRequest(Request *request);
>> +    void processRequest(Request *request, uint64_t targetTimestamp);
>>       Configuration config_;
>>       std::vector<StreamConfig> streamConfigs_;
>>       Signal<FrameBuffer *> bufferCompleted;
>> +
>> +    int64_t minFrameDuration_;
>> +    uint64_t targetTimestamp_;
>>   };
>>   } /* namespace libcamera */
>
Robert Mader Oct. 22, 2026, 5:38 p.m. UTC | #3 
Hi

On 22.10.26 19:20, Barnabás Pőcze wrote:
> 2026. 10. 22. 18:46 keltezéssel, Barnabás Pőcze írta:
>> Hi
>>
>> 2026. 10. 20. 15:24 keltezéssel, Robert Mader írta:
>>> Throttle requests to the minimal frame duration in order to honor the
>>> configured frame rates.
>>>
>>> Signed-off-by: Robert Mader <robert.mader@collabora.com>
>>> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
>>>
>>> ---
>>>
>>> Changes in v7:
>>>   - Address feedback from Laurent
>>>   - Replace lastTimestamp_ with targetTimestamp_ and simplify 
>>> calculation
>>>
>>> Changes in v6:
>>>   - Use std::clamp (Kieran) and minor code reshuffle in start()
>>>   - Use "frame rate" consistently
>>>   - Added R-B from Kieran (assuming the changes are ok)
>>>
>>> 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 | 58 
>>> ++++++++++++++++++----
>>>   src/libcamera/pipeline/virtual/virtual.h   |  5 +-
>>>   2 files changed, 53 insertions(+), 10 deletions(-)
>>>
>>> diff --git a/src/libcamera/pipeline/virtual/virtual.cpp 
>>> b/src/libcamera/pipeline/virtual/virtual.cpp
>>> index f35af97b103d..13a96e6a9412 100644
>>> --- a/src/libcamera/pipeline/virtual/virtual.cpp
>>> +++ b/src/libcamera/pipeline/virtual/virtual.cpp
>>> @@ -135,8 +135,14 @@ 
>>> VirtualCameraData::VirtualCameraData(PipelineHandler *pipe,
>>>       moveToThread(this);
>>>   }
>>> -void VirtualCameraData::processRequest(Request *request)
>>> +void VirtualCameraData::processRequest(Request *request,
>>> +                       uint64_t targetTimestamp)
>>>   {
>>> +    const auto targetTime = std::chrono::steady_clock::time_point{
>>> +        std::chrono::nanoseconds(targetTimestamp)
>>> +    };
>>> +    std::this_thread::sleep_until(targetTime);
>>> +
>>>       for (const auto &[stream, buffer] : request->buffers()) {
>>>           bool found = false;
>>>           /* map buffer and fill test patterns */
>>> @@ -146,7 +152,7 @@ void VirtualCameraData::processRequest(Request 
>>> *request)
>>>                   fmd.status = FrameMetadata::Status::FrameSuccess;
>>>                   fmd.sequence = streamConfig.seq++;
>>> -                fmd.timestamp = currentTimestamp();
>>> +                fmd.timestamp = targetTimestamp;
>>>                   std::span<const FrameBuffer::Plane> planes = 
>>> buffer->planes();
>>>                   for (const auto [i, p] : utils::enumerate(planes))
>>> @@ -348,11 +354,34 @@ 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)) {
>>> +            /*
>>> +             * Target the highest frame rate allowed by the client,
>>> +             * clamped to the values allowed by the config.
>>> +             */
>>> +            data->minFrameDuration_ = std::clamp(
>>> +                clientFrameDurationLimits->front(),
>>> +                data->minFrameDuration_,
>>> + cameraFrameDurationLimits->second.max().get<int64_t>());
>>> +        }
>>> +    }
>>> +
>>> +    data->targetTimestamp_ = 0;
>>> +
>>>       for (auto &s : data->streamConfigs_)
>>>           s.seq = 0;
>>> @@ -379,15 +408,26 @@ void PipelineHandlerVirtual::stopDevice(Camera 
>>> *camera)
>>>           cancelRequest(data->queuedRequests_.front());
>>>   }
>>> -int PipelineHandlerVirtual::queueRequestDevice([[maybe_unused]] 
>>> Camera *camera,
>>> -                           Request *request)
>>> +int PipelineHandlerVirtual::queueRequestDevice(Camera *camera, 
>>> Request *request)
>>>   {
>>
>> Is the lack of handling in `queueRequest()` intentional? Unless there 
>> are some
>> very compelling reasons, I'd like to see that implemented. E.g. when 
>> `FrameDurationLimits`
>> is initialized in `ConfigParser::parseConfigFile()`, you could set a 
>> new member like
>>
>>    std::array<int64_t, 2> frameDurationLimits_;
>>
>> (possibly making `ConfigParser` and `friend` of `VirtualCameraData`)
>>
>> and then use that to clamp everywhere. Or am I missing something?
>
> I have made an attempt, seems to work. Capture script:
Look great - do you mind just taking over the patch and sending your 
version as v8?
>
> ```
> properties:
>   - loop: 500
> frames:
>   - 0:
>       FrameDurationLimits: [ 111111, 111111 ]
>   - 50:
>       FrameDurationLimits: [ 99999, 99999 ]
>   - 100:
>       FrameDurationLimits: [ 88888, 88888 ]
>   - 150:
>       FrameDurationLimits: [ 77777, 77777 ]
>   - 200:
>       FrameDurationLimits: [ 66666, 66666 ]
>   - 250:
>       FrameDurationLimits: [ 55555, 55555 ]
>   - 300:
>       FrameDurationLimits: [ 44444, 44444 ]
>   - 350:
>       FrameDurationLimits: [ 33333, 33333 ]
>   - 400:
>       FrameDurationLimits: [ 22222, 22222 ]
>   - 450:
>       FrameDurationLimits: [ 11111, 11111 ]
> ```
>
> Diff compared to v7:
>
>
> ```diff
> diff --git a/src/libcamera/pipeline/virtual/config_parser.cpp 
> b/src/libcamera/pipeline/virtual/config_parser.cpp
> index 5169fd39bc..ec89158c7c 100644
> --- a/src/libcamera/pipeline/virtual/config_parser.cpp
> +++ b/src/libcamera/pipeline/virtual/config_parser.cpp
> @@ -52,12 +52,17 @@ ConfigParser::parseConfigFile(File &file, 
> PipelineHandler *pipe)
>              continue;
>          }
>
> -        data->config_.id = cameraId;
>          ControlInfoMap::Map controls;
> +
> +        data->config_.id = cameraId;
> +
>          /* \todo Check which resolution's frame rate to be reported */
> +        data->frameDurationLimits_ = {
> +            1000000 / data->config_.resolutions[0].frameRates[1],
> +            1000000 / data->config_.resolutions[0].frameRates[0],
> +        };
>          controls[&controls::FrameDurationLimits] =
> -            ControlInfo(1000000 / 
> data->config_.resolutions[0].frameRates[1],
> -                    1000000 / 
> data->config_.resolutions[0].frameRates[0]);
> +            ControlInfo(data->frameDurationLimits_[0], 
> data->frameDurationLimits_[1]);
>
>          std::vector<ControlValue> supportedFaceDetectModes{
> static_cast<int32_t>(controls::draft::FaceDetectModeOff),
> diff --git a/src/libcamera/pipeline/virtual/data/virtual.yaml 
> b/src/libcamera/pipeline/virtual/data/virtual.yaml
> index 20471bb94b..959e83d790 100644
> --- a/src/libcamera/pipeline/virtual/data/virtual.yaml
> +++ b/src/libcamera/pipeline/virtual/data/virtual.yaml
> @@ -6,8 +6,8 @@
>    - width: 1920
>      height: 1080
>      frame_rates:
> -    - 30
> -    - 60
> +    - 1
> +    - 120
>    - width: 1680
>      height: 1050
>      frame_rates:
> diff --git a/src/libcamera/pipeline/virtual/virtual.cpp 
> b/src/libcamera/pipeline/virtual/virtual.cpp
> index 13a96e6a94..8298be6e91 100644
> --- a/src/libcamera/pipeline/virtual/virtual.cpp
> +++ b/src/libcamera/pipeline/virtual/virtual.cpp
> @@ -172,6 +172,22 @@ void VirtualCameraData::processRequest(Request 
> *request,
>      }
>  }
>
> +void VirtualCameraData::updateFrameDuration(const ControlList &ctrls)
> +{
> +    auto fdl = ctrls.get(controls::FrameDurationLimits);
> +    if (!fdl)
> +        return;
> +
> +    /*
> +     * Target the highest frame rate allowed by the client,
> +     * clamped to the values allowed by the config.
> +     */
> +    minFrameDuration_ = std::clamp(
> +        fdl->front(),
> +        frameDurationLimits_[0],
> +        frameDurationLimits_[1]);
> +}
> +
>  VirtualCameraConfiguration::VirtualCameraConfiguration(VirtualCameraData 
> *data)
>      : CameraConfiguration(), data_(data)
>  {
> @@ -358,27 +374,12 @@ 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>();
> +    data->minFrameDuration_ = data->frameDurationLimits_[0];
> +    ASSERT(data->minFrameDuration_ > 0);
>
> -    if (controls) {
> -        if (auto clientFrameDurationLimits =
> -                controls->get(controls::FrameDurationLimits)) {
> -            /*
> -             * Target the highest frame rate allowed by the client,
> -             * clamped to the values allowed by the config.
> -             */
> -            data->minFrameDuration_ = std::clamp(
> -                clientFrameDurationLimits->front(),
> -                data->minFrameDuration_,
> - cameraFrameDurationLimits->second.max().get<int64_t>());
> -        }
> -    }
> +    if (controls)
> +        data->updateFrameDuration(*controls);
>
>      data->targetTimestamp_ = 0;
>
> @@ -412,6 +413,8 @@ int 
> PipelineHandlerVirtual::queueRequestDevice(Camera *camera, Request 
> *request)
>  {
>      VirtualCameraData *data = cameraData(camera);
>
> +    data->updateFrameDuration(request->controls());
> +
>      /*
>       * TODO: Skip frames if we can't keep up with the maximum frame
>       * duration.
> diff --git a/src/libcamera/pipeline/virtual/virtual.h 
> b/src/libcamera/pipeline/virtual/virtual.h
> index 40e9095c0c..48ceda395b 100644
> --- a/src/libcamera/pipeline/virtual/virtual.h
> +++ b/src/libcamera/pipeline/virtual/virtual.h
> @@ -61,12 +61,14 @@ public:
>      ~VirtualCameraData() = default;
>
>      void processRequest(Request *request, uint64_t targetTimestamp);
> +    void updateFrameDuration(const ControlList &ctrls);
>
>      Configuration config_;
>
>      std::vector<StreamConfig> streamConfigs_;
>      Signal<FrameBuffer *> bufferCompleted;
>
> +    std::array<int64_t, 2> frameDurationLimits_ = {};
>      int64_t minFrameDuration_;
>      uint64_t targetTimestamp_;
>  };
> ```
>
>
>>
>>
>>
>>
>>>       VirtualCameraData *data = cameraData(camera);
>>> -    const auto timestamp = currentTimestamp();
>>> - request->_d()->metadata().set(controls::SensorTimestamp, timestamp);
>>> +    /*
>>> +     * TODO: Skip frames if we can't keep up with the maximum frame
>>> +     * duration.
>>> +     */
>>> +    data->targetTimestamp_ =
>>> +        std::max(currentTimestamp(),
>>> +             data->targetTimestamp_ + data->minFrameDuration_ * 1000);
>>> +
>>> + request->_d()->metadata().set(controls::SensorTimestamp,
>>> +                      data->targetTimestamp_);
>>> + request->_d()->metadata().set(controls::FrameDuration,
>>> +                      data->minFrameDuration_);
>>> +
>>> data->invokeMethod(&VirtualCameraData::processRequest,
>>> -               ConnectionTypeQueued, request);
>>> +               ConnectionTypeQueued, request,
>>> +               data->targetTimestamp_);
>>>       return 0;
>>>   }
>>> diff --git a/src/libcamera/pipeline/virtual/virtual.h 
>>> b/src/libcamera/pipeline/virtual/virtual.h
>>> index 215e56fa3b9d..40e9095c0c4f 100644
>>> --- a/src/libcamera/pipeline/virtual/virtual.h
>>> +++ b/src/libcamera/pipeline/virtual/virtual.h
>>> @@ -60,12 +60,15 @@ public:
>>>       ~VirtualCameraData() = default;
>>> -    void processRequest(Request *request);
>>> +    void processRequest(Request *request, uint64_t targetTimestamp);
>>>       Configuration config_;
>>>       std::vector<StreamConfig> streamConfigs_;
>>>       Signal<FrameBuffer *> bufferCompleted;
>>> +
>>> +    int64_t minFrameDuration_;
>>> +    uint64_t targetTimestamp_;
>>>   };
>>>   } /* namespace libcamera */
>>
>

Patch
diff mbox series 

diff --git a/src/libcamera/pipeline/virtual/virtual.cpp b/src/libcamera/pipeline/virtual/virtual.cpp
index f35af97b103d..13a96e6a9412 100644
--- a/src/libcamera/pipeline/virtual/virtual.cpp
+++ b/src/libcamera/pipeline/virtual/virtual.cpp
@@ -135,8 +135,14 @@  VirtualCameraData::VirtualCameraData(PipelineHandler *pipe,
 	moveToThread(this);
 }
 
-void VirtualCameraData::processRequest(Request *request)
+void VirtualCameraData::processRequest(Request *request,
+				       uint64_t targetTimestamp)
 {
+	const auto targetTime = std::chrono::steady_clock::time_point{
+		std::chrono::nanoseconds(targetTimestamp)
+	};
+	std::this_thread::sleep_until(targetTime);
+
 	for (const auto &[stream, buffer] : request->buffers()) {
 		bool found = false;
 		/* map buffer and fill test patterns */
@@ -146,7 +152,7 @@  void VirtualCameraData::processRequest(Request *request)
 
 				fmd.status = FrameMetadata::Status::FrameSuccess;
 				fmd.sequence = streamConfig.seq++;
-				fmd.timestamp = currentTimestamp();
+				fmd.timestamp = targetTimestamp;
 
 				std::span<const FrameBuffer::Plane> planes = buffer->planes();
 				for (const auto [i, p] : utils::enumerate(planes))
@@ -348,11 +354,34 @@  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)) {
+			/*
+			 * Target the highest frame rate allowed by the client,
+			 * clamped to the values allowed by the config.
+			 */
+			data->minFrameDuration_ = std::clamp(
+				clientFrameDurationLimits->front(),
+				data->minFrameDuration_,
+				cameraFrameDurationLimits->second.max().get<int64_t>());
+		}
+	}
+
+	data->targetTimestamp_ = 0;
+
 	for (auto &s : data->streamConfigs_)
 		s.seq = 0;
 
@@ -379,15 +408,26 @@  void PipelineHandlerVirtual::stopDevice(Camera *camera)
 		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);
+	/*
+	 * TODO: Skip frames if we can't keep up with the maximum frame
+	 * duration.
+	 */
+	data->targetTimestamp_ =
+		std::max(currentTimestamp(),
+			 data->targetTimestamp_ + data->minFrameDuration_ * 1000);
+
+	request->_d()->metadata().set(controls::SensorTimestamp,
+				      data->targetTimestamp_);
+	request->_d()->metadata().set(controls::FrameDuration,
+				      data->minFrameDuration_);
+
 	data->invokeMethod(&VirtualCameraData::processRequest,
-			   ConnectionTypeQueued, request);
+			   ConnectionTypeQueued, request,
+			   data->targetTimestamp_);
 
 	return 0;
 }
diff --git a/src/libcamera/pipeline/virtual/virtual.h b/src/libcamera/pipeline/virtual/virtual.h
index 215e56fa3b9d..40e9095c0c4f 100644
--- a/src/libcamera/pipeline/virtual/virtual.h
+++ b/src/libcamera/pipeline/virtual/virtual.h
@@ -60,12 +60,15 @@  public:
 
 	~VirtualCameraData() = default;
 
-	void processRequest(Request *request);
+	void processRequest(Request *request, uint64_t targetTimestamp);
 
 	Configuration config_;
 
 	std::vector<StreamConfig> streamConfigs_;
 	Signal<FrameBuffer *> bufferCompleted;
+
+	int64_t minFrameDuration_;
+	uint64_t targetTimestamp_;
 };
 
 } /* namespace libcamera */