[v1,2/2] libcamera: controls: Use nanoseconds units for FrameWallClock
diff mbox series

Message ID 20250708085147.752248-3-naush@raspberrypi.com
State Accepted
Commit 29a88d85b730baed52a2f2e5fde2a927474ce41c
Headers show
Series
  • FrameWallClock units
Related show

Commit Message

Naushir Patuck July 8, 2025, 8:49 a.m. UTC
Use nanoseconds for the FrameWallClock control to match the units for
other timestamp controls, including SensorTimestamp.

Update the RPi pipeline handlers to match the new nanoseconds units when
converting from SensorTimestamp to FrameWallClock.

Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
---
 src/libcamera/control_ids_core.yaml      | 3 ++-
 src/libcamera/pipeline/rpi/pisp/pisp.cpp | 2 +-
 src/libcamera/pipeline/rpi/vc4/vc4.cpp   | 2 +-
 3 files changed, 4 insertions(+), 3 deletions(-)

Comments

David Plowman July 8, 2025, 9:08 a.m. UTC | #1
Hi Naush

Thanks for the patch.

On Tue, 8 Jul 2025 at 09:52, Naushir Patuck <naush@raspberrypi.com> wrote:
>
> Use nanoseconds for the FrameWallClock control to match the units for
> other timestamp controls, including SensorTimestamp.
>
> Update the RPi pipeline handlers to match the new nanoseconds units when
> converting from SensorTimestamp to FrameWallClock.
>
> Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
> ---
>  src/libcamera/control_ids_core.yaml      | 3 ++-
>  src/libcamera/pipeline/rpi/pisp/pisp.cpp | 2 +-
>  src/libcamera/pipeline/rpi/vc4/vc4.cpp   | 2 +-
>  3 files changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/src/libcamera/control_ids_core.yaml b/src/libcamera/control_ids_core.yaml
> index 566e1533702f..eec4b4f937ee 100644
> --- a/src/libcamera/control_ids_core.yaml
> +++ b/src/libcamera/control_ids_core.yaml
> @@ -1274,7 +1274,8 @@ controls:
>        description: |
>          This timestamp corresponds to the same moment in time as the
>          SensorTimestamp, but is represented as a wall clock time as measured by
> -        the CLOCK_REALTIME clock.
> +        the CLOCK_REALTIME clock. Like SensorTimestamp, the timestamp value is
> +        expressed in nanoseconds.
>
>          Being a wall clock measurement, it can be used to synchronise timing
>          across different devices.
> diff --git a/src/libcamera/pipeline/rpi/pisp/pisp.cpp b/src/libcamera/pipeline/rpi/pisp/pisp.cpp
> index 2df91bacf3be..92b9070c1bc2 100644
> --- a/src/libcamera/pipeline/rpi/pisp/pisp.cpp
> +++ b/src/libcamera/pipeline/rpi/pisp/pisp.cpp
> @@ -1760,7 +1760,7 @@ void PiSPCameraData::cfeBufferDequeue(FrameBuffer *buffer)
>                  */
>                 wallClockRecovery_.addSample();
>                 uint64_t sensorTimestamp = buffer->metadata().timestamp;
> -               uint64_t wallClockTimestamp = wallClockRecovery_.getOutput(sensorTimestamp / 1000);
> +               uint64_t wallClockTimestamp = wallClockRecovery_.getOutput(sensorTimestamp);
>
>                 ctrl.set(controls::SensorTimestamp, sensorTimestamp);
>                 ctrl.set(controls::FrameWallClock, wallClockTimestamp);
> diff --git a/src/libcamera/pipeline/rpi/vc4/vc4.cpp b/src/libcamera/pipeline/rpi/vc4/vc4.cpp
> index e99a7edf809c..5cadef52712f 100644
> --- a/src/libcamera/pipeline/rpi/vc4/vc4.cpp
> +++ b/src/libcamera/pipeline/rpi/vc4/vc4.cpp
> @@ -778,7 +778,7 @@ void Vc4CameraData::unicamBufferDequeue(FrameBuffer *buffer)
>                  */
>                 wallClockRecovery_.addSample();
>                 uint64_t sensorTimestamp = buffer->metadata().timestamp;
> -               uint64_t wallClockTimestamp = wallClockRecovery_.getOutput(sensorTimestamp / 1000);
> +               uint64_t wallClockTimestamp = wallClockRecovery_.getOutput(sensorTimestamp);

Yes, looks good to me.

Reviewed-by: David Plowman <david.plowman@raspberrypi.com>

Thanks!
David

>
>                 ctrl.set(controls::SensorTimestamp, sensorTimestamp);
>                 ctrl.set(controls::FrameWallClock, wallClockTimestamp);
> --
> 2.43.0
>
Kieran Bingham July 8, 2025, 9:37 a.m. UTC | #2
Quoting Naushir Patuck (2025-07-08 09:49:15)
> Use nanoseconds for the FrameWallClock control to match the units for
> other timestamp controls, including SensorTimestamp.
> 
> Update the RPi pipeline handlers to match the new nanoseconds units when
> converting from SensorTimestamp to FrameWallClock.

Thanks, I think this removes the ambiguity/inconsistency.

I noted that the ROS discussion was also worried that FrameWallClock is
only applicable on Raspberry Pi at the moment - I know we went round in
circles on that before too - I might try to see how we can make it
easily work for all pipelines (they only noticed the lack of
FrameWallClock in UVC ... but anyway - that's not related/blocking this
patch)

Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

> Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
> ---
>  src/libcamera/control_ids_core.yaml      | 3 ++-
>  src/libcamera/pipeline/rpi/pisp/pisp.cpp | 2 +-
>  src/libcamera/pipeline/rpi/vc4/vc4.cpp   | 2 +-
>  3 files changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/src/libcamera/control_ids_core.yaml b/src/libcamera/control_ids_core.yaml
> index 566e1533702f..eec4b4f937ee 100644
> --- a/src/libcamera/control_ids_core.yaml
> +++ b/src/libcamera/control_ids_core.yaml
> @@ -1274,7 +1274,8 @@ controls:
>        description: |
>          This timestamp corresponds to the same moment in time as the
>          SensorTimestamp, but is represented as a wall clock time as measured by
> -        the CLOCK_REALTIME clock.
> +        the CLOCK_REALTIME clock. Like SensorTimestamp, the timestamp value is
> +        expressed in nanoseconds.
>  
>          Being a wall clock measurement, it can be used to synchronise timing
>          across different devices.
> diff --git a/src/libcamera/pipeline/rpi/pisp/pisp.cpp b/src/libcamera/pipeline/rpi/pisp/pisp.cpp
> index 2df91bacf3be..92b9070c1bc2 100644
> --- a/src/libcamera/pipeline/rpi/pisp/pisp.cpp
> +++ b/src/libcamera/pipeline/rpi/pisp/pisp.cpp
> @@ -1760,7 +1760,7 @@ void PiSPCameraData::cfeBufferDequeue(FrameBuffer *buffer)
>                  */
>                 wallClockRecovery_.addSample();
>                 uint64_t sensorTimestamp = buffer->metadata().timestamp;
> -               uint64_t wallClockTimestamp = wallClockRecovery_.getOutput(sensorTimestamp / 1000);
> +               uint64_t wallClockTimestamp = wallClockRecovery_.getOutput(sensorTimestamp);
>  
>                 ctrl.set(controls::SensorTimestamp, sensorTimestamp);
>                 ctrl.set(controls::FrameWallClock, wallClockTimestamp);
> diff --git a/src/libcamera/pipeline/rpi/vc4/vc4.cpp b/src/libcamera/pipeline/rpi/vc4/vc4.cpp
> index e99a7edf809c..5cadef52712f 100644
> --- a/src/libcamera/pipeline/rpi/vc4/vc4.cpp
> +++ b/src/libcamera/pipeline/rpi/vc4/vc4.cpp
> @@ -778,7 +778,7 @@ void Vc4CameraData::unicamBufferDequeue(FrameBuffer *buffer)
>                  */
>                 wallClockRecovery_.addSample();
>                 uint64_t sensorTimestamp = buffer->metadata().timestamp;
> -               uint64_t wallClockTimestamp = wallClockRecovery_.getOutput(sensorTimestamp / 1000);
> +               uint64_t wallClockTimestamp = wallClockRecovery_.getOutput(sensorTimestamp);
>  
>                 ctrl.set(controls::SensorTimestamp, sensorTimestamp);
>                 ctrl.set(controls::FrameWallClock, wallClockTimestamp);
> -- 
> 2.43.0
>

Patch
diff mbox series

diff --git a/src/libcamera/control_ids_core.yaml b/src/libcamera/control_ids_core.yaml
index 566e1533702f..eec4b4f937ee 100644
--- a/src/libcamera/control_ids_core.yaml
+++ b/src/libcamera/control_ids_core.yaml
@@ -1274,7 +1274,8 @@  controls:
       description: |
         This timestamp corresponds to the same moment in time as the
         SensorTimestamp, but is represented as a wall clock time as measured by
-        the CLOCK_REALTIME clock.
+        the CLOCK_REALTIME clock. Like SensorTimestamp, the timestamp value is
+        expressed in nanoseconds.
 
         Being a wall clock measurement, it can be used to synchronise timing
         across different devices.
diff --git a/src/libcamera/pipeline/rpi/pisp/pisp.cpp b/src/libcamera/pipeline/rpi/pisp/pisp.cpp
index 2df91bacf3be..92b9070c1bc2 100644
--- a/src/libcamera/pipeline/rpi/pisp/pisp.cpp
+++ b/src/libcamera/pipeline/rpi/pisp/pisp.cpp
@@ -1760,7 +1760,7 @@  void PiSPCameraData::cfeBufferDequeue(FrameBuffer *buffer)
 		 */
 		wallClockRecovery_.addSample();
 		uint64_t sensorTimestamp = buffer->metadata().timestamp;
-		uint64_t wallClockTimestamp = wallClockRecovery_.getOutput(sensorTimestamp / 1000);
+		uint64_t wallClockTimestamp = wallClockRecovery_.getOutput(sensorTimestamp);
 
 		ctrl.set(controls::SensorTimestamp, sensorTimestamp);
 		ctrl.set(controls::FrameWallClock, wallClockTimestamp);
diff --git a/src/libcamera/pipeline/rpi/vc4/vc4.cpp b/src/libcamera/pipeline/rpi/vc4/vc4.cpp
index e99a7edf809c..5cadef52712f 100644
--- a/src/libcamera/pipeline/rpi/vc4/vc4.cpp
+++ b/src/libcamera/pipeline/rpi/vc4/vc4.cpp
@@ -778,7 +778,7 @@  void Vc4CameraData::unicamBufferDequeue(FrameBuffer *buffer)
 		 */
 		wallClockRecovery_.addSample();
 		uint64_t sensorTimestamp = buffer->metadata().timestamp;
-		uint64_t wallClockTimestamp = wallClockRecovery_.getOutput(sensorTimestamp / 1000);
+		uint64_t wallClockTimestamp = wallClockRecovery_.getOutput(sensorTimestamp);
 
 		ctrl.set(controls::SensorTimestamp, sensorTimestamp);
 		ctrl.set(controls::FrameWallClock, wallClockTimestamp);