[libcamera-devel,v2,11/20] libcamera: ipu3: Adjust full frame picture to 32 pixels
diff mbox series

Message ID 20200709084128.5316-12-jacopo@jmondi.org
State Superseded, archived
Delegated to: Jacopo Mondi
Headers show
Series
  • libcamera: ipu3: Rework configuration
Related show

Commit Message

Jacopo Mondi July 9, 2020, 8:41 a.m. UTC 
To respect the same constraint introduced in validate() that the maximum
ImgU output size shall be at least 32 pixels smaller than the full frame
size, adjust the sizes assigned to the StillCapture role in
generateConfiguration().

Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
---
 src/libcamera/pipeline/ipu3/ipu3.cpp | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

Comments

Niklas Söderlund July 9, 2020, 1:42 p.m. UTC | #1 
Hi Jacopo,

Thanks for your work.

On 2020-07-09 10:41:19 +0200, Jacopo Mondi wrote:
> To respect the same constraint introduced in validate() that the maximum
> ImgU output size shall be at least 32 pixels smaller than the full frame
> size, adjust the sizes assigned to the StillCapture role in
> generateConfiguration().
> 
> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> ---
>  src/libcamera/pipeline/ipu3/ipu3.cpp | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> index 18f4a02cc270..d07f1a7b5ae8 100644
> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> @@ -320,10 +320,14 @@ CameraConfiguration *PipelineHandlerIPU3::generateConfiguration(Camera *camera,
>  			/*
>  			 * Use the sensor resolution aligned to the ImgU
>  			 * output constraints.
> +			 *
> +			 * \todo Give 32 pixels from the sensor frame size
> +			 * for the IF and BDS rectangles to scale. See
> +			 * the todo note for te same operation in validate().

s/te/the/

Is this a todo? Don't this change introduce the 32 pixels "taking"?

>  			 */
> -			size.width = std::min(sensorResolution.width,
> +			size.width = std::min(sensorResolution.width - 32,
>  					      IPU3_OUTPUT_MAX_WIDTH);
> -			size.height = std::min(sensorResolution.height,
> +			size.height = std::min(sensorResolution.height - 32,
>  					       IPU3_OUTPUT_MAX_HEIGHT);
>  			size.width &= ~IPU3_OUTPUT_WIDTH_ALIGN;
>  			size.height &= ~IPU3_OUTPUT_HEIGHT_ALIGN;
> -- 
> 2.27.0
> 
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
Jacopo Mondi July 10, 2020, 7:15 a.m. UTC | #2 
Hi Niklas,

On Thu, Jul 09, 2020 at 03:42:54PM +0200, Niklas Söderlund wrote:
> Hi Jacopo,
>
> Thanks for your work.
>
> On 2020-07-09 10:41:19 +0200, Jacopo Mondi wrote:
> > To respect the same constraint introduced in validate() that the maximum
> > ImgU output size shall be at least 32 pixels smaller than the full frame
> > size, adjust the sizes assigned to the StillCapture role in
> > generateConfiguration().
> >
> > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> > ---
> >  src/libcamera/pipeline/ipu3/ipu3.cpp | 8 ++++++--
> >  1 file changed, 6 insertions(+), 2 deletions(-)
> >
> > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > index 18f4a02cc270..d07f1a7b5ae8 100644
> > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > @@ -320,10 +320,14 @@ CameraConfiguration *PipelineHandlerIPU3::generateConfiguration(Camera *camera,
> >  			/*
> >  			 * Use the sensor resolution aligned to the ImgU
> >  			 * output constraints.
> > +			 *
> > +			 * \todo Give 32 pixels from the sensor frame size
> > +			 * for the IF and BDS rectangles to scale. See
> > +			 * the todo note for te same operation in validate().
>
> s/te/the/
>
> Is this a todo? Don't this change introduce the 32 pixels "taking"?
>

I would have used a FIXME, but I wanted this collected by Doxygen, as
we used \todo to mark this kind of items, right ?

> >  			 */
> > -			size.width = std::min(sensorResolution.width,
> > +			size.width = std::min(sensorResolution.width - 32,
> >  					      IPU3_OUTPUT_MAX_WIDTH);
> > -			size.height = std::min(sensorResolution.height,
> > +			size.height = std::min(sensorResolution.height - 32,
> >  					       IPU3_OUTPUT_MAX_HEIGHT);
> >  			size.width &= ~IPU3_OUTPUT_WIDTH_ALIGN;
> >  			size.height &= ~IPU3_OUTPUT_HEIGHT_ALIGN;
> > --
> > 2.27.0
> >
> > _______________________________________________
> > libcamera-devel mailing list
> > libcamera-devel@lists.libcamera.org
> > https://lists.libcamera.org/listinfo/libcamera-devel
>
> --
> Regards,
> Niklas Söderlund
Laurent Pinchart July 10, 2020, 11:49 a.m. UTC | #3 
Hi Jacopo,

Thank you for the patch.

On Fri, Jul 10, 2020 at 09:15:16AM +0200, Jacopo Mondi wrote:
> On Thu, Jul 09, 2020 at 03:42:54PM +0200, Niklas Söderlund wrote:
> > On 2020-07-09 10:41:19 +0200, Jacopo Mondi wrote:
> > > To respect the same constraint introduced in validate() that the maximum
> > > ImgU output size shall be at least 32 pixels smaller than the full frame
> > > size, adjust the sizes assigned to the StillCapture role in
> > > generateConfiguration().
> > >
> > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> > > ---
> > >  src/libcamera/pipeline/ipu3/ipu3.cpp | 8 ++++++--
> > >  1 file changed, 6 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > > index 18f4a02cc270..d07f1a7b5ae8 100644
> > > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> > > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > > @@ -320,10 +320,14 @@ CameraConfiguration *PipelineHandlerIPU3::generateConfiguration(Camera *camera,
> > >  			/*
> > >  			 * Use the sensor resolution aligned to the ImgU
> > >  			 * output constraints.
> > > +			 *
> > > +			 * \todo Give 32 pixels from the sensor frame size
> > > +			 * for the IF and BDS rectangles to scale. See
> > > +			 * the todo note for te same operation in validate().
> >
> > s/te/the/
> >
> > Is this a todo? Don't this change introduce the 32 pixels "taking"?
> 
> I would have used a FIXME, but I wanted this collected by Doxygen, as
> we used \todo to mark this kind of items, right ?

Maybe

			 * Give 32 pixels from the sensor frame size for the IF
			 * and BDS rectangles to scale.
			 *
			 * \todo This suffers from the same issue as described
			 * in validate().


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

> > >  			 */
> > > -			size.width = std::min(sensorResolution.width,
> > > +			size.width = std::min(sensorResolution.width - 32,
> > >  					      IPU3_OUTPUT_MAX_WIDTH);
> > > -			size.height = std::min(sensorResolution.height,
> > > +			size.height = std::min(sensorResolution.height - 32,
> > >  					       IPU3_OUTPUT_MAX_HEIGHT);
> > >  			size.width &= ~IPU3_OUTPUT_WIDTH_ALIGN;
> > >  			size.height &= ~IPU3_OUTPUT_HEIGHT_ALIGN;

Patch
diff mbox series 

diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
index 18f4a02cc270..d07f1a7b5ae8 100644
--- a/src/libcamera/pipeline/ipu3/ipu3.cpp
+++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
@@ -320,10 +320,14 @@  CameraConfiguration *PipelineHandlerIPU3::generateConfiguration(Camera *camera,
 			/*
 			 * Use the sensor resolution aligned to the ImgU
 			 * output constraints.
+			 *
+			 * \todo Give 32 pixels from the sensor frame size
+			 * for the IF and BDS rectangles to scale. See
+			 * the todo note for te same operation in validate().
 			 */
-			size.width = std::min(sensorResolution.width,
+			size.width = std::min(sensorResolution.width - 32,
 					      IPU3_OUTPUT_MAX_WIDTH);
-			size.height = std::min(sensorResolution.height,
+			size.height = std::min(sensorResolution.height - 32,
 					       IPU3_OUTPUT_MAX_HEIGHT);
 			size.width &= ~IPU3_OUTPUT_WIDTH_ALIGN;
 			size.height &= ~IPU3_OUTPUT_HEIGHT_ALIGN;