[v1,5/7] pipeline: rpi: Track which ISP output is configured for a stream
diff mbox series

Message ID 20240808102346.13065-6-naush@raspberrypi.com
State Superseded, archived
Headers show
Series
  • Raspberry Pi: Add controls::rpi::ScalerCrops
Related show

Commit Message

Naushir Patuck Aug. 8, 2024, 10:23 a.m. UTC 
Add a ispIndex field to CropParams that is used to track
which ISP output (0/1) will be used for a given stream during
configuration.

Tracking this information is required for an upcoming change where crop
rectangles can be specified for each configured stream. Currently, the
value is fixed to 0.

Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
---
 src/libcamera/pipeline/rpi/common/pipeline_base.h | 6 ++++--
 src/libcamera/pipeline/rpi/vc4/vc4.cpp            | 2 +-
 2 files changed, 5 insertions(+), 3 deletions(-)

Comments

Jacopo Mondi Aug. 28, 2024, 9:50 a.m. UTC | #1 
Hi Naush

On Thu, Aug 08, 2024 at 11:23:44AM GMT, Naushir Patuck wrote:
> Add a ispIndex field to CropParams that is used to track
> which ISP output (0/1) will be used for a given stream during
> configuration.
>
> Tracking this information is required for an upcoming change where crop
> rectangles can be specified for each configured stream. Currently, the
> value is fixed to 0.

Maybe I'm missing something, but isn't this the same value used to
index the cropParams_ map ?

>
> Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
> ---
>  src/libcamera/pipeline/rpi/common/pipeline_base.h | 6 ++++--
>  src/libcamera/pipeline/rpi/vc4/vc4.cpp            | 2 +-
>  2 files changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/src/libcamera/pipeline/rpi/common/pipeline_base.h b/src/libcamera/pipeline/rpi/common/pipeline_base.h
> index 2bed905178bc..0ccfa4e82e86 100644
> --- a/src/libcamera/pipeline/rpi/common/pipeline_base.h
> +++ b/src/libcamera/pipeline/rpi/common/pipeline_base.h
> @@ -135,8 +135,8 @@ public:
>  	IPACameraSensorInfo sensorInfo_;
>
>  	struct CropParams {
> -		CropParams(Rectangle ispCrop_, Size ispMinCropSize_)
> -			: ispCrop(ispCrop_), ispMinCropSize(ispMinCropSize_)
> +		CropParams(Rectangle ispCrop_, Size ispMinCropSize_, unsigned int ispIndex_)
> +			: ispCrop(ispCrop_), ispMinCropSize(ispMinCropSize_), ispIndex(ispIndex_)
>  		{
>  		}
>
> @@ -148,6 +148,8 @@ public:
>  		Rectangle ispCrop;
>  		/* Minimum crop size in ISP output pixels */
>  		Size ispMinCropSize;
> +		/* Index of the ISP output channel for this crop */
> +		unsigned int ispIndex;
>  	};
>
>  	/* Mapping of CropParams keyed by the stream index in CameraConfiguration */
> diff --git a/src/libcamera/pipeline/rpi/vc4/vc4.cpp b/src/libcamera/pipeline/rpi/vc4/vc4.cpp
> index d118252f02dd..ffc45653daf1 100644
> --- a/src/libcamera/pipeline/rpi/vc4/vc4.cpp
> +++ b/src/libcamera/pipeline/rpi/vc4/vc4.cpp
> @@ -715,7 +715,7 @@ int Vc4CameraData::platformConfigure(const RPi::RPiCameraConfiguration *rpiConfi
>  	cropParams_.clear();
>  	cropParams_.emplace(std::piecewise_construct,
>  			    std::forward_as_tuple(0),
> -			    std::forward_as_tuple(scaleIspCrop(ispCrop), testCrop.size()));
> +			    std::forward_as_tuple(scaleIspCrop(ispCrop), testCrop.size(), 0));
>
>  	return 0;
>  }
> --
> 2.34.1
>
Naushir Patuck Aug. 28, 2024, 10:03 a.m. UTC | #2 
Hi Jacopo,

On Wed, 28 Aug 2024 at 10:50, Jacopo Mondi
<jacopo.mondi@ideasonboard.com> wrote:
>
> Hi Naush
>
> On Thu, Aug 08, 2024 at 11:23:44AM GMT, Naushir Patuck wrote:
> > Add a ispIndex field to CropParams that is used to track
> > which ISP output (0/1) will be used for a given stream during
> > configuration.
> >
> > Tracking this information is required for an upcoming change where crop
> > rectangles can be specified for each configured stream. Currently, the
> > value is fixed to 0.
>
> Maybe I'm missing something, but isn't this the same value used to
> index the cropParams_ map ?

No it's not the same, and horribly tricky :)

We have 2 indexes to track - the key of the cropParams_ maps tracks
the index of the configured stream, and CropParams::ispCrop tracks the
actual ISP output channel used for that stream.  Looking at the code
in the pisp pipeline handler [1] might make things clearer.

Regards,
Naush

[1] https://github.com/raspberrypi/libcamera/blob/08eb6758129788cb7e33cc1726aea40c672201b8/src/libcamera/pipeline/rpi/pisp/pisp.cpp#L1556

>
> >
> > Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
> > ---
> >  src/libcamera/pipeline/rpi/common/pipeline_base.h | 6 ++++--
> >  src/libcamera/pipeline/rpi/vc4/vc4.cpp            | 2 +-
> >  2 files changed, 5 insertions(+), 3 deletions(-)
> >
> > diff --git a/src/libcamera/pipeline/rpi/common/pipeline_base.h b/src/libcamera/pipeline/rpi/common/pipeline_base.h
> > index 2bed905178bc..0ccfa4e82e86 100644
> > --- a/src/libcamera/pipeline/rpi/common/pipeline_base.h
> > +++ b/src/libcamera/pipeline/rpi/common/pipeline_base.h
> > @@ -135,8 +135,8 @@ public:
> >       IPACameraSensorInfo sensorInfo_;
> >
> >       struct CropParams {
> > -             CropParams(Rectangle ispCrop_, Size ispMinCropSize_)
> > -                     : ispCrop(ispCrop_), ispMinCropSize(ispMinCropSize_)
> > +             CropParams(Rectangle ispCrop_, Size ispMinCropSize_, unsigned int ispIndex_)
> > +                     : ispCrop(ispCrop_), ispMinCropSize(ispMinCropSize_), ispIndex(ispIndex_)
> >               {
> >               }
> >
> > @@ -148,6 +148,8 @@ public:
> >               Rectangle ispCrop;
> >               /* Minimum crop size in ISP output pixels */
> >               Size ispMinCropSize;
> > +             /* Index of the ISP output channel for this crop */
> > +             unsigned int ispIndex;
> >       };
> >
> >       /* Mapping of CropParams keyed by the stream index in CameraConfiguration */
> > diff --git a/src/libcamera/pipeline/rpi/vc4/vc4.cpp b/src/libcamera/pipeline/rpi/vc4/vc4.cpp
> > index d118252f02dd..ffc45653daf1 100644
> > --- a/src/libcamera/pipeline/rpi/vc4/vc4.cpp
> > +++ b/src/libcamera/pipeline/rpi/vc4/vc4.cpp
> > @@ -715,7 +715,7 @@ int Vc4CameraData::platformConfigure(const RPi::RPiCameraConfiguration *rpiConfi
> >       cropParams_.clear();
> >       cropParams_.emplace(std::piecewise_construct,
> >                           std::forward_as_tuple(0),
> > -                         std::forward_as_tuple(scaleIspCrop(ispCrop), testCrop.size()));
> > +                         std::forward_as_tuple(scaleIspCrop(ispCrop), testCrop.size(), 0));
> >
> >       return 0;
> >  }
> > --
> > 2.34.1
> >
Laurent Pinchart Aug. 28, 2024, 5:03 p.m. UTC | #3 
Hello,

On Wed, Aug 28, 2024 at 11:03:34AM +0100, Naushir Patuck wrote:
> On Wed, 28 Aug 2024 at 10:50, Jacopo Mondi wrote:
> > On Thu, Aug 08, 2024 at 11:23:44AM GMT, Naushir Patuck wrote:
> > > Add a ispIndex field to CropParams that is used to track
> > > which ISP output (0/1) will be used for a given stream during
> > > configuration.
> > >
> > > Tracking this information is required for an upcoming change where crop
> > > rectangles can be specified for each configured stream. Currently, the
> > > value is fixed to 0.
> >
> > Maybe I'm missing something, but isn't this the same value used to
> > index the cropParams_ map ?
> 
> No it's not the same, and horribly tricky :)
> 
> We have 2 indexes to track - the key of the cropParams_ maps tracks
> the index of the configured stream, and CropParams::ispCrop tracks the
> actual ISP output channel used for that stream.  Looking at the code
> in the pisp pipeline handler [1] might make things clearer.
> 
> [1] https://github.com/raspberrypi/libcamera/blob/08eb6758129788cb7e33cc1726aea40c672201b8/src/libcamera/pipeline/rpi/pisp/pisp.cpp#L1556
> 
> > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> > > ---
> > >  src/libcamera/pipeline/rpi/common/pipeline_base.h | 6 ++++--
> > >  src/libcamera/pipeline/rpi/vc4/vc4.cpp            | 2 +-
> > >  2 files changed, 5 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/src/libcamera/pipeline/rpi/common/pipeline_base.h b/src/libcamera/pipeline/rpi/common/pipeline_base.h
> > > index 2bed905178bc..0ccfa4e82e86 100644
> > > --- a/src/libcamera/pipeline/rpi/common/pipeline_base.h
> > > +++ b/src/libcamera/pipeline/rpi/common/pipeline_base.h
> > > @@ -135,8 +135,8 @@ public:
> > >       IPACameraSensorInfo sensorInfo_;
> > >
> > >       struct CropParams {
> > > -             CropParams(Rectangle ispCrop_, Size ispMinCropSize_)
> > > -                     : ispCrop(ispCrop_), ispMinCropSize(ispMinCropSize_)
> > > +             CropParams(Rectangle ispCrop_, Size ispMinCropSize_, unsigned int ispIndex_)
> > > +                     : ispCrop(ispCrop_), ispMinCropSize(ispMinCropSize_), ispIndex(ispIndex_)
> > >               {
> > >               }
> > >
> > > @@ -148,6 +148,8 @@ public:
> > >               Rectangle ispCrop;
> > >               /* Minimum crop size in ISP output pixels */
> > >               Size ispMinCropSize;
> > > +             /* Index of the ISP output channel for this crop */
> > > +             unsigned int ispIndex;
> > >       };
> > >
> > >       /* Mapping of CropParams keyed by the stream index in CameraConfiguration */
> > > diff --git a/src/libcamera/pipeline/rpi/vc4/vc4.cpp b/src/libcamera/pipeline/rpi/vc4/vc4.cpp
> > > index d118252f02dd..ffc45653daf1 100644
> > > --- a/src/libcamera/pipeline/rpi/vc4/vc4.cpp
> > > +++ b/src/libcamera/pipeline/rpi/vc4/vc4.cpp
> > > @@ -715,7 +715,7 @@ int Vc4CameraData::platformConfigure(const RPi::RPiCameraConfiguration *rpiConfi
> > >       cropParams_.clear();
> > >       cropParams_.emplace(std::piecewise_construct,
> > >                           std::forward_as_tuple(0),
> > > -                         std::forward_as_tuple(scaleIspCrop(ispCrop), testCrop.size()));
> > > +                         std::forward_as_tuple(scaleIspCrop(ispCrop), testCrop.size(), 0));
> > >
> > >       return 0;
> > >  }

Patch
diff mbox series 

diff --git a/src/libcamera/pipeline/rpi/common/pipeline_base.h b/src/libcamera/pipeline/rpi/common/pipeline_base.h
index 2bed905178bc..0ccfa4e82e86 100644
--- a/src/libcamera/pipeline/rpi/common/pipeline_base.h
+++ b/src/libcamera/pipeline/rpi/common/pipeline_base.h
@@ -135,8 +135,8 @@  public:
 	IPACameraSensorInfo sensorInfo_;
 
 	struct CropParams {
-		CropParams(Rectangle ispCrop_, Size ispMinCropSize_)
-			: ispCrop(ispCrop_), ispMinCropSize(ispMinCropSize_)
+		CropParams(Rectangle ispCrop_, Size ispMinCropSize_, unsigned int ispIndex_)
+			: ispCrop(ispCrop_), ispMinCropSize(ispMinCropSize_), ispIndex(ispIndex_)
 		{
 		}
 
@@ -148,6 +148,8 @@  public:
 		Rectangle ispCrop;
 		/* Minimum crop size in ISP output pixels */
 		Size ispMinCropSize;
+		/* Index of the ISP output channel for this crop */
+		unsigned int ispIndex;
 	};
 
 	/* Mapping of CropParams keyed by the stream index in CameraConfiguration */
diff --git a/src/libcamera/pipeline/rpi/vc4/vc4.cpp b/src/libcamera/pipeline/rpi/vc4/vc4.cpp
index d118252f02dd..ffc45653daf1 100644
--- a/src/libcamera/pipeline/rpi/vc4/vc4.cpp
+++ b/src/libcamera/pipeline/rpi/vc4/vc4.cpp
@@ -715,7 +715,7 @@  int Vc4CameraData::platformConfigure(const RPi::RPiCameraConfiguration *rpiConfi
 	cropParams_.clear();
 	cropParams_.emplace(std::piecewise_construct,
 			    std::forward_as_tuple(0),
-			    std::forward_as_tuple(scaleIspCrop(ispCrop), testCrop.size()));
+			    std::forward_as_tuple(scaleIspCrop(ispCrop), testCrop.size(), 0));
 
 	return 0;
 }