[libcamera-devel,1/3] libcamera: control_ids: Introduce LensShadingEnable
diff mbox series

Message ID 20230308164028.235638-2-jacopo.mondi@ideasonboard.com
State Superseded
Headers show
Series
  • ipa: rkisp1: lsc: Enable/disable LSC
Related show

Commit Message

Jacopo Mondi March 8, 2023, 4:40 p.m. UTC 
From: Jacopo Mondi <jacopo@jmondi.org>

Introduce a control to enable and disable LSC and replace the
draft android control. The control is used to report the algorithm
current state when part of a completed Request metadata.

Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
---
 src/android/camera_capabilities.cpp |  4 ++--
 src/libcamera/control_ids.yaml      | 20 ++++++--------------
 2 files changed, 8 insertions(+), 16 deletions(-)

Comments

Laurent Pinchart March 12, 2023, 3:54 p.m. UTC | #1 
Hi Jacopo,

Thank you for the patch.

On Wed, Mar 08, 2023 at 05:40:26PM +0100, Jacopo Mondi via libcamera-devel wrote:
> From: Jacopo Mondi <jacopo@jmondi.org>
> 
> Introduce a control to enable and disable LSC and replace the
> draft android control. The control is used to report the algorithm
> current state when part of a completed Request metadata.

What's the use case for this ? Also, the new control isn't equivalent to
ANDROID_STATISTICS_LENS_SHADING_MAP_MODE, how will we support the latter
if you drop LensShadingMapMode ?

> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> ---
>  src/android/camera_capabilities.cpp |  4 ++--
>  src/libcamera/control_ids.yaml      | 20 ++++++--------------
>  2 files changed, 8 insertions(+), 16 deletions(-)
> 
> diff --git a/src/android/camera_capabilities.cpp b/src/android/camera_capabilities.cpp
> index 1bfeaea4b121..040ad7347773 100644
> --- a/src/android/camera_capabilities.cpp
> +++ b/src/android/camera_capabilities.cpp
> @@ -1187,10 +1187,10 @@ int CameraCapabilities::initializeStaticMetadata()
>  	{
>  		std::vector<uint8_t> data;
>  		data.reserve(2);
> -		const auto &infoMap = controlsInfo.find(&controls::draft::LensShadingMapMode);
> +		const auto &infoMap = controlsInfo.find(&controls::LensShadingEnable);
>  		if (infoMap != controlsInfo.end()) {
>  			for (const auto &value : infoMap->second.values())
> -				data.push_back(value.get<int32_t>());
> +				data.push_back(value.get<bool>());
>  		} else {
>  			data.push_back(ANDROID_STATISTICS_LENS_SHADING_MAP_MODE_OFF);
>  		}
> diff --git a/src/libcamera/control_ids.yaml b/src/libcamera/control_ids.yaml
> index adea5f90acc5..9457fb3de7b0 100644
> --- a/src/libcamera/control_ids.yaml
> +++ b/src/libcamera/control_ids.yaml
> @@ -694,6 +694,12 @@ controls:
>              Continuous AF is paused. No further state changes or lens movements
>              will occur until the AfPauseResume control is sent.
>  
> +  - LensShadingEnable:
> +      type: bool
> +      description: |
> +        Enable or disable the lens shading algorithm. When reported in the
> +        Request metadata the control reflects the algorithm's current state.
> +
>    # ----------------------------------------------------------------------------
>    # Draft controls section
>  
> @@ -829,20 +835,6 @@ controls:
>         row and the start of exposure of the last row. Currently identical to
>         ANDROID_SENSOR_ROLLING_SHUTTER_SKEW
>  
> -  - LensShadingMapMode:
> -      type: int32_t
> -      draft: true
> -      description: |
> -       Control to report if the lens shading map is available. Currently
> -       identical to ANDROID_STATISTICS_LENS_SHADING_MAP_MODE.
> -      enum:
> -        - name: LensShadingMapModeOff
> -          value: 0
> -          description: No lens shading map mode is available.
> -        - name: LensShadingMapModeOn
> -          value: 1
> -          description: The lens shading map mode is available.
> -
>    - SceneFlicker:
>        type: int32_t
>        draft: true
Jacopo Mondi March 13, 2023, 8:34 a.m. UTC | #2 
Hi Laurent

On Sun, Mar 12, 2023 at 05:54:12PM +0200, Laurent Pinchart via libcamera-devel wrote:
> Hi Jacopo,
>
> Thank you for the patch.
>
> On Wed, Mar 08, 2023 at 05:40:26PM +0100, Jacopo Mondi via libcamera-devel wrote:
> > From: Jacopo Mondi <jacopo@jmondi.org>
> >
> > Introduce a control to enable and disable LSC and replace the
> > draft android control. The control is used to report the algorithm
> > current state when part of a completed Request metadata.
>
> What's the use case for this ? Also, the new control isn't equivalent to
> ANDROID_STATISTICS_LENS_SHADING_MAP_MODE, how will we support the latter
> if you drop LensShadingMapMode ?
>

I'm not sure I understand your questions here...

1) What is the use case: to enable/disable LSC at runtime
2) Isn't it equivalent to ANDROID_STATISTICS_LENS_SHADING_MAP_MODE:
yes, and so far we had a draft control to support it
3) How we support Android: by translating the libcamera control to the
Android control as done in this patch for the static metadata


> > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> > ---
> >  src/android/camera_capabilities.cpp |  4 ++--
> >  src/libcamera/control_ids.yaml      | 20 ++++++--------------
> >  2 files changed, 8 insertions(+), 16 deletions(-)
> >
> > diff --git a/src/android/camera_capabilities.cpp b/src/android/camera_capabilities.cpp
> > index 1bfeaea4b121..040ad7347773 100644
> > --- a/src/android/camera_capabilities.cpp
> > +++ b/src/android/camera_capabilities.cpp
> > @@ -1187,10 +1187,10 @@ int CameraCapabilities::initializeStaticMetadata()
> >  	{
> >  		std::vector<uint8_t> data;
> >  		data.reserve(2);
> > -		const auto &infoMap = controlsInfo.find(&controls::draft::LensShadingMapMode);
> > +		const auto &infoMap = controlsInfo.find(&controls::LensShadingEnable);
> >  		if (infoMap != controlsInfo.end()) {
> >  			for (const auto &value : infoMap->second.values())
> > -				data.push_back(value.get<int32_t>());
> > +				data.push_back(value.get<bool>());
> >  		} else {
> >  			data.push_back(ANDROID_STATISTICS_LENS_SHADING_MAP_MODE_OFF);
> >  		}
> > diff --git a/src/libcamera/control_ids.yaml b/src/libcamera/control_ids.yaml
> > index adea5f90acc5..9457fb3de7b0 100644
> > --- a/src/libcamera/control_ids.yaml
> > +++ b/src/libcamera/control_ids.yaml
> > @@ -694,6 +694,12 @@ controls:
> >              Continuous AF is paused. No further state changes or lens movements
> >              will occur until the AfPauseResume control is sent.
> >
> > +  - LensShadingEnable:
> > +      type: bool
> > +      description: |
> > +        Enable or disable the lens shading algorithm. When reported in the
> > +        Request metadata the control reflects the algorithm's current state.
> > +
> >    # ----------------------------------------------------------------------------
> >    # Draft controls section
> >
> > @@ -829,20 +835,6 @@ controls:
> >         row and the start of exposure of the last row. Currently identical to
> >         ANDROID_SENSOR_ROLLING_SHUTTER_SKEW
> >
> > -  - LensShadingMapMode:
> > -      type: int32_t
> > -      draft: true
> > -      description: |
> > -       Control to report if the lens shading map is available. Currently
> > -       identical to ANDROID_STATISTICS_LENS_SHADING_MAP_MODE.
> > -      enum:
> > -        - name: LensShadingMapModeOff
> > -          value: 0
> > -          description: No lens shading map mode is available.
> > -        - name: LensShadingMapModeOn
> > -          value: 1
> > -          description: The lens shading map mode is available.
> > -
> >    - SceneFlicker:
> >        type: int32_t
> >        draft: true
>
> --
> Regards,
>
> Laurent Pinchart
Laurent Pinchart March 21, 2023, 12:25 a.m. UTC | #3 
Hi Jacopo,

On Mon, Mar 13, 2023 at 09:34:29AM +0100, Jacopo Mondi wrote:
> On Sun, Mar 12, 2023 at 05:54:12PM +0200, Laurent Pinchart via libcamera-devel wrote:
> > On Wed, Mar 08, 2023 at 05:40:26PM +0100, Jacopo Mondi via libcamera-devel wrote:
> > > From: Jacopo Mondi <jacopo@jmondi.org>
> > >
> > > Introduce a control to enable and disable LSC and replace the
> > > draft android control. The control is used to report the algorithm
> > > current state when part of a completed Request metadata.
> >
> > What's the use case for this ? Also, the new control isn't equivalent to
> > ANDROID_STATISTICS_LENS_SHADING_MAP_MODE, how will we support the latter
> > if you drop LensShadingMapMode ?
> 
> I'm not sure I understand your questions here...
> 
> 1) What is the use case: to enable/disable LSC at runtime

I meant what's the use case for enabling and disabling LSC dynamically ?

> 2) Isn't it equivalent to ANDROID_STATISTICS_LENS_SHADING_MAP_MODE:
> yes, and so far we had a draft control to support it

No, my point is that your new control is *not* equivalent to
ANDROID_STATISTICS_LENS_SHADING_MAP_MODE, which was supported through
the LensShadingMapMode control that you're removing.

> 3) How we support Android: by translating the libcamera control to the
> Android control as done in this patch for the static metadata
> 
> > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> > > ---
> > >  src/android/camera_capabilities.cpp |  4 ++--
> > >  src/libcamera/control_ids.yaml      | 20 ++++++--------------
> > >  2 files changed, 8 insertions(+), 16 deletions(-)
> > >
> > > diff --git a/src/android/camera_capabilities.cpp b/src/android/camera_capabilities.cpp
> > > index 1bfeaea4b121..040ad7347773 100644
> > > --- a/src/android/camera_capabilities.cpp
> > > +++ b/src/android/camera_capabilities.cpp
> > > @@ -1187,10 +1187,10 @@ int CameraCapabilities::initializeStaticMetadata()
> > >  	{
> > >  		std::vector<uint8_t> data;
> > >  		data.reserve(2);
> > > -		const auto &infoMap = controlsInfo.find(&controls::draft::LensShadingMapMode);
> > > +		const auto &infoMap = controlsInfo.find(&controls::LensShadingEnable);
> > >  		if (infoMap != controlsInfo.end()) {
> > >  			for (const auto &value : infoMap->second.values())
> > > -				data.push_back(value.get<int32_t>());
> > > +				data.push_back(value.get<bool>());
> > >  		} else {
> > >  			data.push_back(ANDROID_STATISTICS_LENS_SHADING_MAP_MODE_OFF);
> > >  		}
> > > diff --git a/src/libcamera/control_ids.yaml b/src/libcamera/control_ids.yaml
> > > index adea5f90acc5..9457fb3de7b0 100644
> > > --- a/src/libcamera/control_ids.yaml
> > > +++ b/src/libcamera/control_ids.yaml
> > > @@ -694,6 +694,12 @@ controls:
> > >              Continuous AF is paused. No further state changes or lens movements
> > >              will occur until the AfPauseResume control is sent.
> > >
> > > +  - LensShadingEnable:
> > > +      type: bool
> > > +      description: |
> > > +        Enable or disable the lens shading algorithm. When reported in the
> > > +        Request metadata the control reflects the algorithm's current state.
> > > +
> > >    # ----------------------------------------------------------------------------
> > >    # Draft controls section
> > >
> > > @@ -829,20 +835,6 @@ controls:
> > >         row and the start of exposure of the last row. Currently identical to
> > >         ANDROID_SENSOR_ROLLING_SHUTTER_SKEW
> > >
> > > -  - LensShadingMapMode:
> > > -      type: int32_t
> > > -      draft: true
> > > -      description: |
> > > -       Control to report if the lens shading map is available. Currently
> > > -       identical to ANDROID_STATISTICS_LENS_SHADING_MAP_MODE.
> > > -      enum:
> > > -        - name: LensShadingMapModeOff
> > > -          value: 0
> > > -          description: No lens shading map mode is available.
> > > -        - name: LensShadingMapModeOn
> > > -          value: 1
> > > -          description: The lens shading map mode is available.
> > > -
> > >    - SceneFlicker:
> > >        type: int32_t
> > >        draft: true

Patch
diff mbox series 

diff --git a/src/android/camera_capabilities.cpp b/src/android/camera_capabilities.cpp
index 1bfeaea4b121..040ad7347773 100644
--- a/src/android/camera_capabilities.cpp
+++ b/src/android/camera_capabilities.cpp
@@ -1187,10 +1187,10 @@  int CameraCapabilities::initializeStaticMetadata()
 	{
 		std::vector<uint8_t> data;
 		data.reserve(2);
-		const auto &infoMap = controlsInfo.find(&controls::draft::LensShadingMapMode);
+		const auto &infoMap = controlsInfo.find(&controls::LensShadingEnable);
 		if (infoMap != controlsInfo.end()) {
 			for (const auto &value : infoMap->second.values())
-				data.push_back(value.get<int32_t>());
+				data.push_back(value.get<bool>());
 		} else {
 			data.push_back(ANDROID_STATISTICS_LENS_SHADING_MAP_MODE_OFF);
 		}
diff --git a/src/libcamera/control_ids.yaml b/src/libcamera/control_ids.yaml
index adea5f90acc5..9457fb3de7b0 100644
--- a/src/libcamera/control_ids.yaml
+++ b/src/libcamera/control_ids.yaml
@@ -694,6 +694,12 @@  controls:
             Continuous AF is paused. No further state changes or lens movements
             will occur until the AfPauseResume control is sent.
 
+  - LensShadingEnable:
+      type: bool
+      description: |
+        Enable or disable the lens shading algorithm. When reported in the
+        Request metadata the control reflects the algorithm's current state.
+
   # ----------------------------------------------------------------------------
   # Draft controls section
 
@@ -829,20 +835,6 @@  controls:
        row and the start of exposure of the last row. Currently identical to
        ANDROID_SENSOR_ROLLING_SHUTTER_SKEW
 
-  - LensShadingMapMode:
-      type: int32_t
-      draft: true
-      description: |
-       Control to report if the lens shading map is available. Currently
-       identical to ANDROID_STATISTICS_LENS_SHADING_MAP_MODE.
-      enum:
-        - name: LensShadingMapModeOff
-          value: 0
-          description: No lens shading map mode is available.
-        - name: LensShadingMapModeOn
-          value: 1
-          description: The lens shading map mode is available.
-
   - SceneFlicker:
       type: int32_t
       draft: true