[libcamera-devel,v2,6/6] android: Add CONTROL_MODE_OFF in template and static metadata
diff mbox series

Message ID 20211220232629.1485890-7-paul.elder@ideasonboard.com
State Changes Requested
Headers show
Series
  • android: Miscellaneous fixes
Related show

Commit Message

Paul Elder Dec. 20, 2021, 11:26 p.m. UTC 
Add CONTROL_MODE_OFF to the available control modes in static metadata,
if both AE off and AWB off are available. Also set CONTROL_MODE_OFF in
the manual template.

Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
---
 src/android/camera_capabilities.cpp | 18 +++++++++++++++++-
 1 file changed, 17 insertions(+), 1 deletion(-)

Comments

Jacopo Mondi Dec. 21, 2021, 9 a.m. UTC | #1 
Hi Paul,

On Mon, Dec 20, 2021 at 05:26:29PM -0600, Paul Elder wrote:
> Add CONTROL_MODE_OFF to the available control modes in static metadata,
> if both AE off and AWB off are available. Also set CONTROL_MODE_OFF in
> the manual template.
>
> Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> ---
>  src/android/camera_capabilities.cpp | 18 +++++++++++++++++-
>  1 file changed, 17 insertions(+), 1 deletion(-)
>
> diff --git a/src/android/camera_capabilities.cpp b/src/android/camera_capabilities.cpp
> index ea2aaf58..a2e42a5c 100644
> --- a/src/android/camera_capabilities.cpp
> +++ b/src/android/camera_capabilities.cpp
> @@ -996,7 +996,17 @@ int CameraCapabilities::initializeStaticMetadata()
>  	staticMetadata_->addEntry(ANDROID_CONTROL_AWB_LOCK_AVAILABLE,
>  				  awbLockAvailable);
>
> -	char availableControlModes = ANDROID_CONTROL_MODE_AUTO;
> +	/*
> +	 * \todo Get this from some combination of the available AE and AWB
> +	 * modes
> +	 */
> +	std::vector<uint8_t> availableControlModes = { ANDROID_CONTROL_MODE_AUTO };
> +	if (staticMetadata_->entryContains<uint8_t>(ANDROID_CONTROL_AE_AVAILABLE_MODES,
> +						    ANDROID_CONTROL_AE_MODE_OFF) &&
> +	    staticMetadata_->entryContains<uint8_t>(ANDROID_CONTROL_AWB_AVAILABLE_MODES,
> +						    ANDROID_CONTROL_AWB_MODE_OFF)) {
> +		availableControlModes.push_back(ANDROID_CONTROL_MODE_OFF);
> +	}

This makes sense

>  	staticMetadata_->addEntry(ANDROID_CONTROL_AVAILABLE_MODES,
>  				  availableControlModes);
>
> @@ -1452,6 +1462,12 @@ std::unique_ptr<CameraMetadata> CameraCapabilities::requestTemplateManual() cons
>  	if (!manualTemplate)
>  		return nullptr;
>
> +	if (staticMetadata_->entryContains<uint8_t>(ANDROID_CONTROL_AVAILABLE_MODES,
> +						    ANDROID_CONTROL_MODE_OFF)) {
> +		uint8_t mode = ANDROID_CONTROL_MODE_OFF;
> +		manualTemplate->appendEntry(ANDROID_CONTROL_MODE, mode);
> +	}
> +

I would wait. The manual template is broken, it is created with the
preview one (which has AE on and AWB set to auto). I wonder if this
creates more confusion than anything. We'll have to rewrite this part
anyway.

What I would be more interested in is knowing if reporting
CONTROL_MODE in dynamic metadata is required, now that we potentially
advertise CONTROL_MODE_OFF too.

Thanks
  j

>  	return manualTemplate;
>  }
>
> --
> 2.27.0
>
Paul Elder Dec. 21, 2021, 10:58 p.m. UTC | #2 
Hi Jacopo,

On Tue, Dec 21, 2021 at 10:00:39AM +0100, Jacopo Mondi wrote:
> Hi Paul,
> 
> On Mon, Dec 20, 2021 at 05:26:29PM -0600, Paul Elder wrote:
> > Add CONTROL_MODE_OFF to the available control modes in static metadata,
> > if both AE off and AWB off are available. Also set CONTROL_MODE_OFF in
> > the manual template.
> >
> > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> > ---
> >  src/android/camera_capabilities.cpp | 18 +++++++++++++++++-
> >  1 file changed, 17 insertions(+), 1 deletion(-)
> >
> > diff --git a/src/android/camera_capabilities.cpp b/src/android/camera_capabilities.cpp
> > index ea2aaf58..a2e42a5c 100644
> > --- a/src/android/camera_capabilities.cpp
> > +++ b/src/android/camera_capabilities.cpp
> > @@ -996,7 +996,17 @@ int CameraCapabilities::initializeStaticMetadata()
> >  	staticMetadata_->addEntry(ANDROID_CONTROL_AWB_LOCK_AVAILABLE,
> >  				  awbLockAvailable);
> >
> > -	char availableControlModes = ANDROID_CONTROL_MODE_AUTO;
> > +	/*
> > +	 * \todo Get this from some combination of the available AE and AWB
> > +	 * modes
> > +	 */
> > +	std::vector<uint8_t> availableControlModes = { ANDROID_CONTROL_MODE_AUTO };
> > +	if (staticMetadata_->entryContains<uint8_t>(ANDROID_CONTROL_AE_AVAILABLE_MODES,
> > +						    ANDROID_CONTROL_AE_MODE_OFF) &&
> > +	    staticMetadata_->entryContains<uint8_t>(ANDROID_CONTROL_AWB_AVAILABLE_MODES,
> > +						    ANDROID_CONTROL_AWB_MODE_OFF)) {
> > +		availableControlModes.push_back(ANDROID_CONTROL_MODE_OFF);
> > +	}
> 
> This makes sense
> 
> >  	staticMetadata_->addEntry(ANDROID_CONTROL_AVAILABLE_MODES,
> >  				  availableControlModes);
> >
> > @@ -1452,6 +1462,12 @@ std::unique_ptr<CameraMetadata> CameraCapabilities::requestTemplateManual() cons
> >  	if (!manualTemplate)
> >  		return nullptr;
> >
> > +	if (staticMetadata_->entryContains<uint8_t>(ANDROID_CONTROL_AVAILABLE_MODES,
> > +						    ANDROID_CONTROL_MODE_OFF)) {
> > +		uint8_t mode = ANDROID_CONTROL_MODE_OFF;
> > +		manualTemplate->appendEntry(ANDROID_CONTROL_MODE, mode);
> > +	}
> > +
> 
> I would wait. The manual template is broken, it is created with the
> preview one (which has AE on and AWB set to auto). I wonder if this
> creates more confusion than anything. We'll have to rewrite this part
> anyway.

It's not really broken. There's a guard against requesting a manual
template when manual sensor isn't supported, and our manual sensor
validator currently never returns true. It will at the very end once
everything for FULL is merged in.

So I think it's okay to put this in now, since it ought to be sufficient
(perhaps with the exception of AF), so later when we do enable FULL (and
the required capability validators), it'll Just Work.

> 
> What I would be more interested in is knowing if reporting
> CONTROL_MODE in dynamic metadata is required, now that we potentially
> advertise CONTROL_MODE_OFF too.

We always report it as an available request key.

Actually, I just realized I forgot to plumb this into the result
metadata. So this patch will be moved out of this series anyway.


Paul

> 
> >  	return manualTemplate;
> >  }
> >
> > --
> > 2.27.0
> >

Patch
diff mbox series 

diff --git a/src/android/camera_capabilities.cpp b/src/android/camera_capabilities.cpp
index ea2aaf58..a2e42a5c 100644
--- a/src/android/camera_capabilities.cpp
+++ b/src/android/camera_capabilities.cpp
@@ -996,7 +996,17 @@  int CameraCapabilities::initializeStaticMetadata()
 	staticMetadata_->addEntry(ANDROID_CONTROL_AWB_LOCK_AVAILABLE,
 				  awbLockAvailable);
 
-	char availableControlModes = ANDROID_CONTROL_MODE_AUTO;
+	/*
+	 * \todo Get this from some combination of the available AE and AWB
+	 * modes
+	 */
+	std::vector<uint8_t> availableControlModes = { ANDROID_CONTROL_MODE_AUTO };
+	if (staticMetadata_->entryContains<uint8_t>(ANDROID_CONTROL_AE_AVAILABLE_MODES,
+						    ANDROID_CONTROL_AE_MODE_OFF) &&
+	    staticMetadata_->entryContains<uint8_t>(ANDROID_CONTROL_AWB_AVAILABLE_MODES,
+						    ANDROID_CONTROL_AWB_MODE_OFF)) {
+		availableControlModes.push_back(ANDROID_CONTROL_MODE_OFF);
+	}
 	staticMetadata_->addEntry(ANDROID_CONTROL_AVAILABLE_MODES,
 				  availableControlModes);
 
@@ -1452,6 +1462,12 @@  std::unique_ptr<CameraMetadata> CameraCapabilities::requestTemplateManual() cons
 	if (!manualTemplate)
 		return nullptr;
 
+	if (staticMetadata_->entryContains<uint8_t>(ANDROID_CONTROL_AVAILABLE_MODES,
+						    ANDROID_CONTROL_MODE_OFF)) {
+		uint8_t mode = ANDROID_CONTROL_MODE_OFF;
+		manualTemplate->appendEntry(ANDROID_CONTROL_MODE, mode);
+	}
+
 	return manualTemplate;
 }