[libcamera-devel,v2,10/13] android: camera_device: Report PIPELINE_MAX_DEPTH
diff mbox series

Message ID 20201020180534.36855-11-jacopo@jmondi.org
State Accepted
Headers show
Series
  • libcamera: Introduce draft controls
Related show

Commit Message

Jacopo Mondi Oct. 20, 2020, 6:05 p.m. UTC
Register the ANDROID_REQUEST_PIPELINE_MAX_DEPTH static property
inspecting the value reported by the pipeline handler.

If the Camera does not report any suitable value, default the
static property to 2.

Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
---
 src/android/camera_device.cpp | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

Comments

Kieran Bingham Oct. 21, 2020, 1:10 p.m. UTC | #1
On 20/10/2020 19:05, Jacopo Mondi wrote:
> Register the ANDROID_REQUEST_PIPELINE_MAX_DEPTH static property
> inspecting the value reported by the pipeline handler.
> 
> If the Camera does not report any suitable value, default the
> static property to 2.
> 
> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> ---
>  src/android/camera_device.cpp | 15 ++++++++++++---
>  1 file changed, 12 insertions(+), 3 deletions(-)
> 
> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> index e9404c2f3004..8166b09bb69a 100644
> --- a/src/android/camera_device.cpp
> +++ b/src/android/camera_device.cpp
> @@ -13,6 +13,7 @@
>  #include <tuple>
>  #include <vector>
>  
> +#include <libcamera/control_ids.h>
>  #include <libcamera/controls.h>
>  #include <libcamera/formats.h>
>  #include <libcamera/property_ids.h>
> @@ -591,6 +592,8 @@ const camera_metadata_t *CameraDevice::getStaticMetadata()
>  		return nullptr;
>  	}
>  
> +	const ControlInfoMap &controlsInfo = camera_->controls();
> +
>  	/* Color correction static metadata. */
>  	std::vector<uint8_t> aberrationModes = {
>  		ANDROID_COLOR_CORRECTION_ABERRATION_MODE_OFF,
> @@ -882,9 +885,15 @@ const camera_metadata_t *CameraDevice::getStaticMetadata()
>  	staticMetadata_->addEntry(ANDROID_REQUEST_PARTIAL_RESULT_COUNT,
>  				  &partialResultCount, 1);
>  
> -	uint8_t maxPipelineDepth = 2;
> -	staticMetadata_->addEntry(ANDROID_REQUEST_PIPELINE_MAX_DEPTH,
> -				  &maxPipelineDepth, 1);
> +	{
> +		/* Default the value to 2 if not reported by the camera. */
> +		uint8_t maxPipelineDepth = 2;
> +		const auto &infoMap = controlsInfo.find(&controls::draft::PipelineDepth);
> +		if (infoMap != controlsInfo.end())
> +			maxPipelineDepth = infoMap->second.max().get<int32_t>();

I would certainly put a new line here, and possibly/probably before the
if statement too. Otherwise it's disguising the scope of the condition IMO.


Should we print any kind of a warning if the pipeline handler has not
reported this control?

I could foresee this being a repeated pattern. Not sure how to best
handle that yet though.

But for now,

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

> +		staticMetadata_->addEntry(ANDROID_REQUEST_PIPELINE_MAX_DEPTH,
> +					  &maxPipelineDepth, 1);
> +	}
>  
>  	/* LIMITED does not support reprocessing. */
>  	uint32_t maxNumInputStreams = 0;
>
Jacopo Mondi Oct. 21, 2020, 1:56 p.m. UTC | #2
Hi Kieran

On Wed, Oct 21, 2020 at 02:10:19PM +0100, Kieran Bingham wrote:
> On 20/10/2020 19:05, Jacopo Mondi wrote:
> > Register the ANDROID_REQUEST_PIPELINE_MAX_DEPTH static property
> > inspecting the value reported by the pipeline handler.
> >
> > If the Camera does not report any suitable value, default the
> > static property to 2.
> >
> > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> > ---
> >  src/android/camera_device.cpp | 15 ++++++++++++---
> >  1 file changed, 12 insertions(+), 3 deletions(-)
> >
> > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> > index e9404c2f3004..8166b09bb69a 100644
> > --- a/src/android/camera_device.cpp
> > +++ b/src/android/camera_device.cpp
> > @@ -13,6 +13,7 @@
> >  #include <tuple>
> >  #include <vector>
> >
> > +#include <libcamera/control_ids.h>
> >  #include <libcamera/controls.h>
> >  #include <libcamera/formats.h>
> >  #include <libcamera/property_ids.h>
> > @@ -591,6 +592,8 @@ const camera_metadata_t *CameraDevice::getStaticMetadata()
> >  		return nullptr;
> >  	}
> >
> > +	const ControlInfoMap &controlsInfo = camera_->controls();
> > +
> >  	/* Color correction static metadata. */
> >  	std::vector<uint8_t> aberrationModes = {
> >  		ANDROID_COLOR_CORRECTION_ABERRATION_MODE_OFF,
> > @@ -882,9 +885,15 @@ const camera_metadata_t *CameraDevice::getStaticMetadata()
> >  	staticMetadata_->addEntry(ANDROID_REQUEST_PARTIAL_RESULT_COUNT,
> >  				  &partialResultCount, 1);
> >
> > -	uint8_t maxPipelineDepth = 2;
> > -	staticMetadata_->addEntry(ANDROID_REQUEST_PIPELINE_MAX_DEPTH,
> > -				  &maxPipelineDepth, 1);
> > +	{
> > +		/* Default the value to 2 if not reported by the camera. */
> > +		uint8_t maxPipelineDepth = 2;
> > +		const auto &infoMap = controlsInfo.find(&controls::draft::PipelineDepth);
> > +		if (infoMap != controlsInfo.end())
> > +			maxPipelineDepth = infoMap->second.max().get<int32_t>();
>
> I would certainly put a new line here, and possibly/probably before the
> if statement too. Otherwise it's disguising the scope of the condition IMO.
>
>
> Should we print any kind of a warning if the pipeline handler has not
> reported this control?

I don't think so. According to the discussion we had on the previous
series pipeline handler should not be concerned with android
specificities.

>
> I could foresee this being a repeated pattern. Not sure how to best
> handle that yet though.
>
> But for now,
>
> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
>
> > +		staticMetadata_->addEntry(ANDROID_REQUEST_PIPELINE_MAX_DEPTH,
> > +					  &maxPipelineDepth, 1);
> > +	}
> >
> >  	/* LIMITED does not support reprocessing. */
> >  	uint32_t maxNumInputStreams = 0;
> >
>
> --
> Regards
> --
> Kieran

Patch
diff mbox series

diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
index e9404c2f3004..8166b09bb69a 100644
--- a/src/android/camera_device.cpp
+++ b/src/android/camera_device.cpp
@@ -13,6 +13,7 @@ 
 #include <tuple>
 #include <vector>
 
+#include <libcamera/control_ids.h>
 #include <libcamera/controls.h>
 #include <libcamera/formats.h>
 #include <libcamera/property_ids.h>
@@ -591,6 +592,8 @@  const camera_metadata_t *CameraDevice::getStaticMetadata()
 		return nullptr;
 	}
 
+	const ControlInfoMap &controlsInfo = camera_->controls();
+
 	/* Color correction static metadata. */
 	std::vector<uint8_t> aberrationModes = {
 		ANDROID_COLOR_CORRECTION_ABERRATION_MODE_OFF,
@@ -882,9 +885,15 @@  const camera_metadata_t *CameraDevice::getStaticMetadata()
 	staticMetadata_->addEntry(ANDROID_REQUEST_PARTIAL_RESULT_COUNT,
 				  &partialResultCount, 1);
 
-	uint8_t maxPipelineDepth = 2;
-	staticMetadata_->addEntry(ANDROID_REQUEST_PIPELINE_MAX_DEPTH,
-				  &maxPipelineDepth, 1);
+	{
+		/* Default the value to 2 if not reported by the camera. */
+		uint8_t maxPipelineDepth = 2;
+		const auto &infoMap = controlsInfo.find(&controls::draft::PipelineDepth);
+		if (infoMap != controlsInfo.end())
+			maxPipelineDepth = infoMap->second.max().get<int32_t>();
+		staticMetadata_->addEntry(ANDROID_REQUEST_PIPELINE_MAX_DEPTH,
+					  &maxPipelineDepth, 1);
+	}
 
 	/* LIMITED does not support reprocessing. */
 	uint32_t maxNumInputStreams = 0;