[v2,1/7] controls: rpi: Add a vendor rpi::ScalerCrops control
diff mbox series

Message ID 20240930141415.8857-2-naush@raspberrypi.com
State Superseded
Headers show
Series
  • Raspberry Pi: Add controls::rpi::ScalerCrops
Related show

Commit Message

Naushir Patuck Sept. 30, 2024, 2:14 p.m. UTC 
Add a vendor control rpi::ScalerCrops that is analogous to the current
core::ScalerCrop, but can apply a different crop to each configured
stream.

This control takes a span of Rectangle structures - the order of
rectangles must match the order of streams configured by the application.

Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
---
 src/ipa/rpi/common/ipa_base.cpp    | 14 ++++++++++++++
 src/libcamera/control_ids_rpi.yaml | 21 +++++++++++++++++++++
 2 files changed, 35 insertions(+)

Comments

Jacopo Mondi Oct. 1, 2024, 6:13 a.m. UTC | #1 
Hi Naush

On Mon, Sep 30, 2024 at 03:14:09PM GMT, Naushir Patuck wrote:
> Add a vendor control rpi::ScalerCrops that is analogous to the current
> core::ScalerCrop, but can apply a different crop to each configured
> stream.
>
> This control takes a span of Rectangle structures - the order of
> rectangles must match the order of streams configured by the application.
>
> Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
> ---
>  src/ipa/rpi/common/ipa_base.cpp    | 14 ++++++++++++++
>  src/libcamera/control_ids_rpi.yaml | 21 +++++++++++++++++++++
>  2 files changed, 35 insertions(+)
>
> diff --git a/src/ipa/rpi/common/ipa_base.cpp b/src/ipa/rpi/common/ipa_base.cpp
> index ee3848b54f21..d408cdb74255 100644
> --- a/src/ipa/rpi/common/ipa_base.cpp
> +++ b/src/ipa/rpi/common/ipa_base.cpp
> @@ -96,6 +96,15 @@ const ControlInfoMap::Map ipaAfControls{
>  	{ &controls::LensPosition, ControlInfo(0.0f, 32.0f, 1.0f) }
>  };
>
> +/* Platform specific controls */
> +const std::map<const std::string, ControlInfoMap::Map> platformControls {
> +	{ "pisp",
> +	  {
> +		{ &controls::rpi::ScalerCrops, ControlInfo(Rectangle{}, Rectangle(65535, 65535, 65535, 65535), Rectangle{}) }
> +	  }
> +	},
> +};

I wonder if this won't be better placed in the forthcoming pisp.c but
I don't see a ControlInfoMap in vc4.cpp, so maybe you plan to place
platform specific controls on ipa_base.cpp ?

> +
>  } /* namespace */
>
>  LOG_DEFINE_CATEGORY(IPARPI)
> @@ -159,6 +168,10 @@ int32_t IpaBase::init(const IPASettings &settings, const InitParams &params, Ini
>  	if (lensPresent_)
>  		ctrlMap.merge(ControlInfoMap::Map(ipaAfControls));
>
> +	auto platformCtrlsIt = platformControls.find(controller_.getTarget());
> +	if (platformCtrlsIt != platformControls.end())
> +		ctrlMap.merge(ControlInfoMap::Map(platformCtrlsIt->second));
> +
>  	monoSensor_ = params.sensorInfo.cfaPattern == properties::draft::ColorFilterArrangementEnum::MONO;
>  	if (!monoSensor_)
>  		ctrlMap.merge(ControlInfoMap::Map(ipaColourControls));
> @@ -1070,6 +1083,7 @@ void IpaBase::applyControls(const ControlList &controls)
>  			break;
>  		}
>
> +		case controls::rpi::SCALER_CROPS:
>  		case controls::SCALER_CROP: {
>  			/* We do nothing with this, but should avoid the warning below. */
>  			break;
> diff --git a/src/libcamera/control_ids_rpi.yaml b/src/libcamera/control_ids_rpi.yaml
> index cb097f887e16..a0fc1cd897b5 100644
> --- a/src/libcamera/control_ids_rpi.yaml
> +++ b/src/libcamera/control_ids_rpi.yaml
> @@ -26,4 +26,25 @@ controls:
>
>          \sa StatsOutputEnable
>
> +  - ScalerCrops:
> +      type: Rectangle
> +      size: [n]
> +      description: |
> +        An array of rectangles, where each singular value has identical
> +        functionality to the ScalerCrop control. This control allows the
> +        Raspberry Pi pipeline handler to control individual scaler crops per
> +        output stream.
> +
> +        The order of rectangles passed into the control must match the order of
> +        streams configured by the application. The pipeline handler will only
> +        configure crop retangles up-to the number of output streams configured.
> +        All subsequent rectangles passed into this control are ignored by the
> +        pipeline handler.
> +
> +        Note that using different crop rectangles for each output stream with
> +        this control is only applicable on the Pi5/PiSP platform. This control
> +        should also be considered temporary/draft and will be replaced with
> +        official libcamera API support for per-stream controls in the future.
> +
> +        \sa ScalerCrop

Fine to have a vendor control for this
Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>

Thanks
  j

>  ...
> --
> 2.34.1
>
Naushir Patuck Oct. 1, 2024, 11:05 a.m. UTC | #2 
Hi Jacopo,

Thanks for the feedback.

On Tue, 1 Oct 2024 at 07:13, Jacopo Mondi <jacopo.mondi@ideasonboard.com> wrote:
>
> Hi Naush
>
> On Mon, Sep 30, 2024 at 03:14:09PM GMT, Naushir Patuck wrote:
> > Add a vendor control rpi::ScalerCrops that is analogous to the current
> > core::ScalerCrop, but can apply a different crop to each configured
> > stream.
> >
> > This control takes a span of Rectangle structures - the order of
> > rectangles must match the order of streams configured by the application.
> >
> > Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
> > ---
> >  src/ipa/rpi/common/ipa_base.cpp    | 14 ++++++++++++++
> >  src/libcamera/control_ids_rpi.yaml | 21 +++++++++++++++++++++
> >  2 files changed, 35 insertions(+)
> >
> > diff --git a/src/ipa/rpi/common/ipa_base.cpp b/src/ipa/rpi/common/ipa_base.cpp
> > index ee3848b54f21..d408cdb74255 100644
> > --- a/src/ipa/rpi/common/ipa_base.cpp
> > +++ b/src/ipa/rpi/common/ipa_base.cpp
> > @@ -96,6 +96,15 @@ const ControlInfoMap::Map ipaAfControls{
> >       { &controls::LensPosition, ControlInfo(0.0f, 32.0f, 1.0f) }
> >  };
> >
> > +/* Platform specific controls */
> > +const std::map<const std::string, ControlInfoMap::Map> platformControls {
> > +     { "pisp",
> > +       {
> > +             { &controls::rpi::ScalerCrops, ControlInfo(Rectangle{}, Rectangle(65535, 65535, 65535, 65535), Rectangle{}) }
> > +       }
> > +     },
> > +};
>
> I wonder if this won't be better placed in the forthcoming pisp.c but
> I don't see a ControlInfoMap in vc4.cpp, so maybe you plan to place
> platform specific controls on ipa_base.cpp ?

Good question - I can do it if you think it's more appropriate?

The reason I did it in ipa_base.cpp was because I liked that all
general/hw specific controls were listed in the same place.

Regards,
Naush

>
> > +
> >  } /* namespace */
> >
> >  LOG_DEFINE_CATEGORY(IPARPI)
> > @@ -159,6 +168,10 @@ int32_t IpaBase::init(const IPASettings &settings, const InitParams &params, Ini
> >       if (lensPresent_)
> >               ctrlMap.merge(ControlInfoMap::Map(ipaAfControls));
> >
> > +     auto platformCtrlsIt = platformControls.find(controller_.getTarget());
> > +     if (platformCtrlsIt != platformControls.end())
> > +             ctrlMap.merge(ControlInfoMap::Map(platformCtrlsIt->second));
> > +
> >       monoSensor_ = params.sensorInfo.cfaPattern == properties::draft::ColorFilterArrangementEnum::MONO;
> >       if (!monoSensor_)
> >               ctrlMap.merge(ControlInfoMap::Map(ipaColourControls));
> > @@ -1070,6 +1083,7 @@ void IpaBase::applyControls(const ControlList &controls)
> >                       break;
> >               }
> >
> > +             case controls::rpi::SCALER_CROPS:
> >               case controls::SCALER_CROP: {
> >                       /* We do nothing with this, but should avoid the warning below. */
> >                       break;
> > diff --git a/src/libcamera/control_ids_rpi.yaml b/src/libcamera/control_ids_rpi.yaml
> > index cb097f887e16..a0fc1cd897b5 100644
> > --- a/src/libcamera/control_ids_rpi.yaml
> > +++ b/src/libcamera/control_ids_rpi.yaml
> > @@ -26,4 +26,25 @@ controls:
> >
> >          \sa StatsOutputEnable
> >
> > +  - ScalerCrops:
> > +      type: Rectangle
> > +      size: [n]
> > +      description: |
> > +        An array of rectangles, where each singular value has identical
> > +        functionality to the ScalerCrop control. This control allows the
> > +        Raspberry Pi pipeline handler to control individual scaler crops per
> > +        output stream.
> > +
> > +        The order of rectangles passed into the control must match the order of
> > +        streams configured by the application. The pipeline handler will only
> > +        configure crop retangles up-to the number of output streams configured.
> > +        All subsequent rectangles passed into this control are ignored by the
> > +        pipeline handler.
> > +
> > +        Note that using different crop rectangles for each output stream with
> > +        this control is only applicable on the Pi5/PiSP platform. This control
> > +        should also be considered temporary/draft and will be replaced with
> > +        official libcamera API support for per-stream controls in the future.
> > +
> > +        \sa ScalerCrop
>
> Fine to have a vendor control for this
> Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
>
> Thanks
>   j
>
> >  ...
> > --
> > 2.34.1
> >
Jacopo Mondi Oct. 1, 2024, 11:27 a.m. UTC | #3 
Hi Naush

On Tue, Oct 01, 2024 at 12:05:45PM GMT, Naushir Patuck wrote:
> Hi Jacopo,
>
> Thanks for the feedback.
>
> On Tue, 1 Oct 2024 at 07:13, Jacopo Mondi <jacopo.mondi@ideasonboard.com> wrote:
> >
> > Hi Naush
> >
> > On Mon, Sep 30, 2024 at 03:14:09PM GMT, Naushir Patuck wrote:
> > > Add a vendor control rpi::ScalerCrops that is analogous to the current
> > > core::ScalerCrop, but can apply a different crop to each configured
> > > stream.
> > >
> > > This control takes a span of Rectangle structures - the order of
> > > rectangles must match the order of streams configured by the application.
> > >
> > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
> > > ---
> > >  src/ipa/rpi/common/ipa_base.cpp    | 14 ++++++++++++++
> > >  src/libcamera/control_ids_rpi.yaml | 21 +++++++++++++++++++++
> > >  2 files changed, 35 insertions(+)
> > >
> > > diff --git a/src/ipa/rpi/common/ipa_base.cpp b/src/ipa/rpi/common/ipa_base.cpp
> > > index ee3848b54f21..d408cdb74255 100644
> > > --- a/src/ipa/rpi/common/ipa_base.cpp
> > > +++ b/src/ipa/rpi/common/ipa_base.cpp
> > > @@ -96,6 +96,15 @@ const ControlInfoMap::Map ipaAfControls{
> > >       { &controls::LensPosition, ControlInfo(0.0f, 32.0f, 1.0f) }
> > >  };
> > >
> > > +/* Platform specific controls */
> > > +const std::map<const std::string, ControlInfoMap::Map> platformControls {
> > > +     { "pisp",
> > > +       {
> > > +             { &controls::rpi::ScalerCrops, ControlInfo(Rectangle{}, Rectangle(65535, 65535, 65535, 65535), Rectangle{}) }
> > > +       }
> > > +     },
> > > +};
> >
> > I wonder if this won't be better placed in the forthcoming pisp.c but
> > I don't see a ControlInfoMap in vc4.cpp, so maybe you plan to place
> > platform specific controls on ipa_base.cpp ?
>
> Good question - I can do it if you think it's more appropriate?
>

It's up to you to decide whatever makes maintaining the two IPAs
simpler.

> The reason I did it in ipa_base.cpp was because I liked that all
> general/hw specific controls were listed in the same place.
>

That's fine then ;)

> Regards,
> Naush
>
> >
> > > +
> > >  } /* namespace */
> > >
> > >  LOG_DEFINE_CATEGORY(IPARPI)
> > > @@ -159,6 +168,10 @@ int32_t IpaBase::init(const IPASettings &settings, const InitParams &params, Ini
> > >       if (lensPresent_)
> > >               ctrlMap.merge(ControlInfoMap::Map(ipaAfControls));
> > >
> > > +     auto platformCtrlsIt = platformControls.find(controller_.getTarget());
> > > +     if (platformCtrlsIt != platformControls.end())
> > > +             ctrlMap.merge(ControlInfoMap::Map(platformCtrlsIt->second));
> > > +
> > >       monoSensor_ = params.sensorInfo.cfaPattern == properties::draft::ColorFilterArrangementEnum::MONO;
> > >       if (!monoSensor_)
> > >               ctrlMap.merge(ControlInfoMap::Map(ipaColourControls));
> > > @@ -1070,6 +1083,7 @@ void IpaBase::applyControls(const ControlList &controls)
> > >                       break;
> > >               }
> > >
> > > +             case controls::rpi::SCALER_CROPS:
> > >               case controls::SCALER_CROP: {
> > >                       /* We do nothing with this, but should avoid the warning below. */
> > >                       break;
> > > diff --git a/src/libcamera/control_ids_rpi.yaml b/src/libcamera/control_ids_rpi.yaml
> > > index cb097f887e16..a0fc1cd897b5 100644
> > > --- a/src/libcamera/control_ids_rpi.yaml
> > > +++ b/src/libcamera/control_ids_rpi.yaml
> > > @@ -26,4 +26,25 @@ controls:
> > >
> > >          \sa StatsOutputEnable
> > >
> > > +  - ScalerCrops:
> > > +      type: Rectangle
> > > +      size: [n]
> > > +      description: |
> > > +        An array of rectangles, where each singular value has identical
> > > +        functionality to the ScalerCrop control. This control allows the
> > > +        Raspberry Pi pipeline handler to control individual scaler crops per
> > > +        output stream.
> > > +
> > > +        The order of rectangles passed into the control must match the order of
> > > +        streams configured by the application. The pipeline handler will only
> > > +        configure crop retangles up-to the number of output streams configured.
> > > +        All subsequent rectangles passed into this control are ignored by the
> > > +        pipeline handler.
> > > +
> > > +        Note that using different crop rectangles for each output stream with
> > > +        this control is only applicable on the Pi5/PiSP platform. This control
> > > +        should also be considered temporary/draft and will be replaced with
> > > +        official libcamera API support for per-stream controls in the future.
> > > +
> > > +        \sa ScalerCrop
> >
> > Fine to have a vendor control for this
> > Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> >
> > Thanks
> >   j
> >
> > >  ...
> > > --
> > > 2.34.1
> > >

Patch
diff mbox series 

diff --git a/src/ipa/rpi/common/ipa_base.cpp b/src/ipa/rpi/common/ipa_base.cpp
index ee3848b54f21..d408cdb74255 100644
--- a/src/ipa/rpi/common/ipa_base.cpp
+++ b/src/ipa/rpi/common/ipa_base.cpp
@@ -96,6 +96,15 @@  const ControlInfoMap::Map ipaAfControls{
 	{ &controls::LensPosition, ControlInfo(0.0f, 32.0f, 1.0f) }
 };
 
+/* Platform specific controls */
+const std::map<const std::string, ControlInfoMap::Map> platformControls {
+	{ "pisp",
+	  {
+		{ &controls::rpi::ScalerCrops, ControlInfo(Rectangle{}, Rectangle(65535, 65535, 65535, 65535), Rectangle{}) }
+	  }
+	},
+};
+
 } /* namespace */
 
 LOG_DEFINE_CATEGORY(IPARPI)
@@ -159,6 +168,10 @@  int32_t IpaBase::init(const IPASettings &settings, const InitParams &params, Ini
 	if (lensPresent_)
 		ctrlMap.merge(ControlInfoMap::Map(ipaAfControls));
 
+	auto platformCtrlsIt = platformControls.find(controller_.getTarget());
+	if (platformCtrlsIt != platformControls.end())
+		ctrlMap.merge(ControlInfoMap::Map(platformCtrlsIt->second));
+
 	monoSensor_ = params.sensorInfo.cfaPattern == properties::draft::ColorFilterArrangementEnum::MONO;
 	if (!monoSensor_)
 		ctrlMap.merge(ControlInfoMap::Map(ipaColourControls));
@@ -1070,6 +1083,7 @@  void IpaBase::applyControls(const ControlList &controls)
 			break;
 		}
 
+		case controls::rpi::SCALER_CROPS:
 		case controls::SCALER_CROP: {
 			/* We do nothing with this, but should avoid the warning below. */
 			break;
diff --git a/src/libcamera/control_ids_rpi.yaml b/src/libcamera/control_ids_rpi.yaml
index cb097f887e16..a0fc1cd897b5 100644
--- a/src/libcamera/control_ids_rpi.yaml
+++ b/src/libcamera/control_ids_rpi.yaml
@@ -26,4 +26,25 @@  controls:
 
         \sa StatsOutputEnable
 
+  - ScalerCrops:
+      type: Rectangle
+      size: [n]
+      description: |
+        An array of rectangles, where each singular value has identical
+        functionality to the ScalerCrop control. This control allows the
+        Raspberry Pi pipeline handler to control individual scaler crops per
+        output stream.
+
+        The order of rectangles passed into the control must match the order of
+        streams configured by the application. The pipeline handler will only
+        configure crop retangles up-to the number of output streams configured.
+        All subsequent rectangles passed into this control are ignored by the
+        pipeline handler.
+
+        Note that using different crop rectangles for each output stream with
+        this control is only applicable on the Pi5/PiSP platform. This control
+        should also be considered temporary/draft and will be replaced with
+        official libcamera API support for per-stream controls in the future.
+
+        \sa ScalerCrop
 ...