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

23774 diff mbox series

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(-)

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
>

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
>