[libcamera-devel,v3,1/3] controls: Split FrameDurations into FrameDuration and FrameDurationLimits
diff mbox series

Message ID 20210525091812.1225580-1-paul.elder@ideasonboard.com
State Superseded
Headers show
Series
  • [libcamera-devel,v3,1/3] controls: Split FrameDurations into FrameDuration and FrameDurationLimits
Related show

Commit Message

Paul Elder May 25, 2021, 9:18 a.m. UTC 
We need a separate control to report the nominal frame duration, but
it's also useful to report the min/max frame duration values that will
be used. Split the FrameDurations control into FrameDuration and
FrameDurationLimits respectively to support both of these.

Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

---
Changes in v3:
- mention that the control is meant to be returned in metadata
- s/nominal/instantaneous
---
 include/libcamera/ipa/raspberrypi.h  | 2 +-
 src/android/camera_device.cpp        | 2 +-
 src/ipa/raspberrypi/raspberrypi.cpp  | 4 ++--
 src/libcamera/control_ids.yaml       | 9 ++++++++-
 src/libcamera/pipeline/ipu3/ipu3.cpp | 6 +++---
 5 files changed, 15 insertions(+), 8 deletions(-)

Comments

Jacopo Mondi May 25, 2021, 10:23 a.m. UTC | #1 
Hi Paul,

On Tue, May 25, 2021 at 06:18:10PM +0900, Paul Elder wrote:
> We need a separate control to report the nominal frame duration, but
> it's also useful to report the min/max frame duration values that will
> be used. Split the FrameDurations control into FrameDuration and
> FrameDurationLimits respectively to support both of these.
>
> Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>
> ---
> Changes in v3:
> - mention that the control is meant to be returned in metadata
> - s/nominal/instantaneous
> ---
>  include/libcamera/ipa/raspberrypi.h  | 2 +-
>  src/android/camera_device.cpp        | 2 +-
>  src/ipa/raspberrypi/raspberrypi.cpp  | 4 ++--
>  src/libcamera/control_ids.yaml       | 9 ++++++++-
>  src/libcamera/pipeline/ipu3/ipu3.cpp | 6 +++---
>  5 files changed, 15 insertions(+), 8 deletions(-)
>
> diff --git a/include/libcamera/ipa/raspberrypi.h b/include/libcamera/ipa/raspberrypi.h
> index d10c1733..a8790000 100644
> --- a/include/libcamera/ipa/raspberrypi.h
> +++ b/include/libcamera/ipa/raspberrypi.h
> @@ -44,7 +44,7 @@ static const ControlInfoMap Controls = {
>  	{ &controls::Sharpness, ControlInfo(0.0f, 16.0f, 1.0f) },
>  	{ &controls::ColourCorrectionMatrix, ControlInfo(-16.0f, 16.0f) },
>  	{ &controls::ScalerCrop, ControlInfo(Rectangle{}, Rectangle(65535, 65535, 65535, 65535), Rectangle{}) },
> -	{ &controls::FrameDurations, ControlInfo(INT64_C(1000), INT64_C(1000000000)) },
> +	{ &controls::FrameDurationLimits, ControlInfo(INT64_C(1000), INT64_C(1000000000)) },
>  	{ &controls::draft::NoiseReductionMode, ControlInfo(controls::draft::NoiseReductionModeValues) },
>  };
>
> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> index b32e8be5..0eea2b95 100644
> --- a/src/android/camera_device.cpp
> +++ b/src/android/camera_device.cpp
> @@ -824,7 +824,7 @@ const camera_metadata_t *CameraDevice::getStaticMetadata()
>
>  	int64_t minFrameDurationNsec = -1;
>  	int64_t maxFrameDurationNsec = -1;
> -	const auto frameDurationsInfo = controlsInfo.find(&controls::FrameDurations);
> +	const auto frameDurationsInfo = controlsInfo.find(&controls::FrameDurationLimits);
>  	if (frameDurationsInfo != controlsInfo.end()) {
>  		minFrameDurationNsec = frameDurationsInfo->second.min().get<int64_t>() * 1000;
>  		maxFrameDurationNsec = frameDurationsInfo->second.max().get<int64_t>() * 1000;
> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp
> index e5bb8159..0c4752ec 100644
> --- a/src/ipa/raspberrypi/raspberrypi.cpp
> +++ b/src/ipa/raspberrypi/raspberrypi.cpp
> @@ -859,7 +859,7 @@ void IPARPi::queueRequest(const ControlList &controls)
>  			break;
>  		}
>
> -		case controls::FRAME_DURATIONS: {
> +		case controls::FRAME_DURATION_LIMITS: {
>  			auto frameDurations = ctrl.second.get<Span<const int64_t>>();
>  			applyFrameDurations(frameDurations[0], frameDurations[1]);
>  			break;
> @@ -1074,7 +1074,7 @@ void IPARPi::applyFrameDurations(double minFrameDuration, double maxFrameDuratio
>  	maxFrameDuration_ = std::max(maxFrameDuration_, minFrameDuration_);
>
>  	/* Return the validated limits via metadata. */
> -	libcameraMetadata_.set(controls::FrameDurations,
> +	libcameraMetadata_.set(controls::FrameDurationLimits,
>  			       { static_cast<int64_t>(minFrameDuration_),
>  				 static_cast<int64_t>(maxFrameDuration_) });
>
> diff --git a/src/libcamera/control_ids.yaml b/src/libcamera/control_ids.yaml
> index 88d81ac4..f62ade48 100644
> --- a/src/libcamera/control_ids.yaml
> +++ b/src/libcamera/control_ids.yaml
> @@ -323,7 +323,14 @@ controls:
>          step to respect the received gain factor and shall report
>          their total value in the request metadata.
>
> -  - FrameDurations:
> +  - FrameDuration:
> +      type: int64_t
> +      description: |
> +        The instantaneous frame duration from start of frame exposure to start
> +        of next exposure, expressed in microseconds. This control is meant to
> +        be returned in metadata.
> +
> +  - FrameDurationLimits:
>        type: int64_t
>        description: |
>          The minimum and maximum (in that order) frame duration,
> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> index 25203256..15ba5cf8 100644
> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> @@ -983,9 +983,9 @@ int PipelineHandlerIPU3::initControls(IPU3CameraData *data)
>  		frameDurations[i] = frameSize / (sensorInfo.pixelRate / 1000000U);
>  	}
>
> -	controls[&controls::FrameDurations] = ControlInfo(frameDurations[0],
> -							  frameDurations[1],
> -							  frameDurations[2]);
> +	controls[&controls::FrameDuration] = ControlInfo(frameDurations[0],
> +							 frameDurations[1],
> +							 frameDurations[2]);

Should we register both FrameDuration and FrameDurationLimits ?

FrameDuration has to be returned, but also the limits are (or will be
supported)....

>
>  	/*
>  	 * Compute the scaler crop limits.
> --
> 2.27.0
>
Laurent Pinchart May 25, 2021, 11:05 a.m. UTC | #2 
On Tue, May 25, 2021 at 12:23:37PM +0200, Jacopo Mondi wrote:
> On Tue, May 25, 2021 at 06:18:10PM +0900, Paul Elder wrote:
> > We need a separate control to report the nominal frame duration, but
> > it's also useful to report the min/max frame duration values that will
> > be used. Split the FrameDurations control into FrameDuration and
> > FrameDurationLimits respectively to support both of these.
> >
> > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> >
> > ---
> > Changes in v3:
> > - mention that the control is meant to be returned in metadata
> > - s/nominal/instantaneous
> > ---
> >  include/libcamera/ipa/raspberrypi.h  | 2 +-
> >  src/android/camera_device.cpp        | 2 +-
> >  src/ipa/raspberrypi/raspberrypi.cpp  | 4 ++--
> >  src/libcamera/control_ids.yaml       | 9 ++++++++-
> >  src/libcamera/pipeline/ipu3/ipu3.cpp | 6 +++---
> >  5 files changed, 15 insertions(+), 8 deletions(-)
> >
> > diff --git a/include/libcamera/ipa/raspberrypi.h b/include/libcamera/ipa/raspberrypi.h
> > index d10c1733..a8790000 100644
> > --- a/include/libcamera/ipa/raspberrypi.h
> > +++ b/include/libcamera/ipa/raspberrypi.h
> > @@ -44,7 +44,7 @@ static const ControlInfoMap Controls = {
> >  	{ &controls::Sharpness, ControlInfo(0.0f, 16.0f, 1.0f) },
> >  	{ &controls::ColourCorrectionMatrix, ControlInfo(-16.0f, 16.0f) },
> >  	{ &controls::ScalerCrop, ControlInfo(Rectangle{}, Rectangle(65535, 65535, 65535, 65535), Rectangle{}) },
> > -	{ &controls::FrameDurations, ControlInfo(INT64_C(1000), INT64_C(1000000000)) },
> > +	{ &controls::FrameDurationLimits, ControlInfo(INT64_C(1000), INT64_C(1000000000)) },
> >  	{ &controls::draft::NoiseReductionMode, ControlInfo(controls::draft::NoiseReductionModeValues) },
> >  };
> >
> > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> > index b32e8be5..0eea2b95 100644
> > --- a/src/android/camera_device.cpp
> > +++ b/src/android/camera_device.cpp
> > @@ -824,7 +824,7 @@ const camera_metadata_t *CameraDevice::getStaticMetadata()
> >
> >  	int64_t minFrameDurationNsec = -1;
> >  	int64_t maxFrameDurationNsec = -1;
> > -	const auto frameDurationsInfo = controlsInfo.find(&controls::FrameDurations);
> > +	const auto frameDurationsInfo = controlsInfo.find(&controls::FrameDurationLimits);
> >  	if (frameDurationsInfo != controlsInfo.end()) {
> >  		minFrameDurationNsec = frameDurationsInfo->second.min().get<int64_t>() * 1000;
> >  		maxFrameDurationNsec = frameDurationsInfo->second.max().get<int64_t>() * 1000;
> > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp
> > index e5bb8159..0c4752ec 100644
> > --- a/src/ipa/raspberrypi/raspberrypi.cpp
> > +++ b/src/ipa/raspberrypi/raspberrypi.cpp
> > @@ -859,7 +859,7 @@ void IPARPi::queueRequest(const ControlList &controls)
> >  			break;
> >  		}
> >
> > -		case controls::FRAME_DURATIONS: {
> > +		case controls::FRAME_DURATION_LIMITS: {
> >  			auto frameDurations = ctrl.second.get<Span<const int64_t>>();
> >  			applyFrameDurations(frameDurations[0], frameDurations[1]);
> >  			break;
> > @@ -1074,7 +1074,7 @@ void IPARPi::applyFrameDurations(double minFrameDuration, double maxFrameDuratio
> >  	maxFrameDuration_ = std::max(maxFrameDuration_, minFrameDuration_);
> >
> >  	/* Return the validated limits via metadata. */
> > -	libcameraMetadata_.set(controls::FrameDurations,
> > +	libcameraMetadata_.set(controls::FrameDurationLimits,
> >  			       { static_cast<int64_t>(minFrameDuration_),
> >  				 static_cast<int64_t>(maxFrameDuration_) });
> >
> > diff --git a/src/libcamera/control_ids.yaml b/src/libcamera/control_ids.yaml
> > index 88d81ac4..f62ade48 100644
> > --- a/src/libcamera/control_ids.yaml
> > +++ b/src/libcamera/control_ids.yaml
> > @@ -323,7 +323,14 @@ controls:
> >          step to respect the received gain factor and shall report
> >          their total value in the request metadata.
> >
> > -  - FrameDurations:
> > +  - FrameDuration:
> > +      type: int64_t
> > +      description: |
> > +        The instantaneous frame duration from start of frame exposure to start
> > +        of next exposure, expressed in microseconds. This control is meant to
> > +        be returned in metadata.
> > +
> > +  - FrameDurationLimits:
> >        type: int64_t
> >        description: |
> >          The minimum and maximum (in that order) frame duration,
> > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > index 25203256..15ba5cf8 100644
> > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > @@ -983,9 +983,9 @@ int PipelineHandlerIPU3::initControls(IPU3CameraData *data)
> >  		frameDurations[i] = frameSize / (sensorInfo.pixelRate / 1000000U);
> >  	}
> >
> > -	controls[&controls::FrameDurations] = ControlInfo(frameDurations[0],
> > -							  frameDurations[1],
> > -							  frameDurations[2]);
> > +	controls[&controls::FrameDuration] = ControlInfo(frameDurations[0],
> > +							 frameDurations[1],
> > +							 frameDurations[2]);
> 
> Should we register both FrameDuration and FrameDurationLimits ?
> 
> FrameDuration has to be returned, but also the limits are (or will be
> supported)....

I have missed this one. This patch should use FrameDurationLimits to
replace FrameDurations in the whole code base, adding support for
FrameDuration in selected pipeline handlers should be done in a
subsequent patch.

> >
> >  	/*
> >  	 * Compute the scaler crop limits.

Patch
diff mbox series 

diff --git a/include/libcamera/ipa/raspberrypi.h b/include/libcamera/ipa/raspberrypi.h
index d10c1733..a8790000 100644
--- a/include/libcamera/ipa/raspberrypi.h
+++ b/include/libcamera/ipa/raspberrypi.h
@@ -44,7 +44,7 @@  static const ControlInfoMap Controls = {
 	{ &controls::Sharpness, ControlInfo(0.0f, 16.0f, 1.0f) },
 	{ &controls::ColourCorrectionMatrix, ControlInfo(-16.0f, 16.0f) },
 	{ &controls::ScalerCrop, ControlInfo(Rectangle{}, Rectangle(65535, 65535, 65535, 65535), Rectangle{}) },
-	{ &controls::FrameDurations, ControlInfo(INT64_C(1000), INT64_C(1000000000)) },
+	{ &controls::FrameDurationLimits, ControlInfo(INT64_C(1000), INT64_C(1000000000)) },
 	{ &controls::draft::NoiseReductionMode, ControlInfo(controls::draft::NoiseReductionModeValues) },
 };
 
diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
index b32e8be5..0eea2b95 100644
--- a/src/android/camera_device.cpp
+++ b/src/android/camera_device.cpp
@@ -824,7 +824,7 @@  const camera_metadata_t *CameraDevice::getStaticMetadata()
 
 	int64_t minFrameDurationNsec = -1;
 	int64_t maxFrameDurationNsec = -1;
-	const auto frameDurationsInfo = controlsInfo.find(&controls::FrameDurations);
+	const auto frameDurationsInfo = controlsInfo.find(&controls::FrameDurationLimits);
 	if (frameDurationsInfo != controlsInfo.end()) {
 		minFrameDurationNsec = frameDurationsInfo->second.min().get<int64_t>() * 1000;
 		maxFrameDurationNsec = frameDurationsInfo->second.max().get<int64_t>() * 1000;
diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp
index e5bb8159..0c4752ec 100644
--- a/src/ipa/raspberrypi/raspberrypi.cpp
+++ b/src/ipa/raspberrypi/raspberrypi.cpp
@@ -859,7 +859,7 @@  void IPARPi::queueRequest(const ControlList &controls)
 			break;
 		}
 
-		case controls::FRAME_DURATIONS: {
+		case controls::FRAME_DURATION_LIMITS: {
 			auto frameDurations = ctrl.second.get<Span<const int64_t>>();
 			applyFrameDurations(frameDurations[0], frameDurations[1]);
 			break;
@@ -1074,7 +1074,7 @@  void IPARPi::applyFrameDurations(double minFrameDuration, double maxFrameDuratio
 	maxFrameDuration_ = std::max(maxFrameDuration_, minFrameDuration_);
 
 	/* Return the validated limits via metadata. */
-	libcameraMetadata_.set(controls::FrameDurations,
+	libcameraMetadata_.set(controls::FrameDurationLimits,
 			       { static_cast<int64_t>(minFrameDuration_),
 				 static_cast<int64_t>(maxFrameDuration_) });
 
diff --git a/src/libcamera/control_ids.yaml b/src/libcamera/control_ids.yaml
index 88d81ac4..f62ade48 100644
--- a/src/libcamera/control_ids.yaml
+++ b/src/libcamera/control_ids.yaml
@@ -323,7 +323,14 @@  controls:
         step to respect the received gain factor and shall report
         their total value in the request metadata.
 
-  - FrameDurations:
+  - FrameDuration:
+      type: int64_t
+      description: |
+        The instantaneous frame duration from start of frame exposure to start
+        of next exposure, expressed in microseconds. This control is meant to
+        be returned in metadata.
+
+  - FrameDurationLimits:
       type: int64_t
       description: |
         The minimum and maximum (in that order) frame duration,
diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
index 25203256..15ba5cf8 100644
--- a/src/libcamera/pipeline/ipu3/ipu3.cpp
+++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
@@ -983,9 +983,9 @@  int PipelineHandlerIPU3::initControls(IPU3CameraData *data)
 		frameDurations[i] = frameSize / (sensorInfo.pixelRate / 1000000U);
 	}
 
-	controls[&controls::FrameDurations] = ControlInfo(frameDurations[0],
-							  frameDurations[1],
-							  frameDurations[2]);
+	controls[&controls::FrameDuration] = ControlInfo(frameDurations[0],
+							 frameDurations[1],
+							 frameDurations[2]);
 
 	/*
 	 * Compute the scaler crop limits.