[libcamera-devel,2/2] libcamera: pipeline: ipu3: Use new Size grownBy() and shrunkBy() helpers
diff mbox series

Message ID 20211013012630.18877-3-laurent.pinchart@ideasonboard.com
State Accepted
Headers show
Series
  • libcamera: Add new helpers to the Size class
Related show

Commit Message

Laurent Pinchart Oct. 13, 2021, 1:26 a.m. UTC 
The Size class has new helpers that can simplify the code in the IPU3
pipeline handler. Use them.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 src/libcamera/pipeline/ipu3/ipu3.cpp | 15 ++++++---------
 1 file changed, 6 insertions(+), 9 deletions(-)

Comments

Jacopo Mondi Oct. 13, 2021, 6:43 a.m. UTC | #1 
Hi Laurent,

On Wed, Oct 13, 2021 at 04:26:30AM +0300, Laurent Pinchart wrote:
> The Size class has new helpers that can simplify the code in the IPU3
> pipeline handler. Use them.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>  src/libcamera/pipeline/ipu3/ipu3.cpp | 15 ++++++---------
>  1 file changed, 6 insertions(+), 9 deletions(-)
>
> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> index 92e869257e53..262b9a23703e 100644
> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> @@ -438,11 +438,10 @@ CameraConfiguration *PipelineHandlerIPU3::generateConfiguration(Camera *camera,
>  			 * \todo Clarify the alignment constraints as explained
>  			 * in validate()
>  			 */
> -			size = sensorResolution.boundedTo(IMGU_OUTPUT_MAX_SIZE);
> -			size.width = utils::alignDown(size.width - 1,
> -						      IMGU_OUTPUT_WIDTH_MARGIN);
> -			size.height = utils::alignDown(size.height - 1,
> -						       IMGU_OUTPUT_HEIGHT_MARGIN);
> +			size = sensorResolution.boundedTo(IMGU_OUTPUT_MAX_SIZE)
> +				.shrunkBy({ 1, 1 })
> +				.alignedDownTo(IMGU_OUTPUT_WIDTH_MARGIN,
> +					       IMGU_OUTPUT_HEIGHT_MARGIN);

Can you align the dots ?

			size = sensorResolution.boundedTo(IMGU_OUTPUT_MAX_SIZE)
				               .shrunkBy({ 1, 1 })
				               .alignedDownTo(IMGU_OUTPUT_WIDTH_MARGIN,
					                      IMGU_OUTPUT_HEIGHT_MARGIN);

Same below.

Or was it intentional ?

(I'm honest, I found the previous version more immediate compared than
going through shrunk).

Anyway, there's a repeating pattern of

                {size.width - 1, size.height - 1}.alignDownTo(margins)
                {size.width + 1, size.height + 1}.alignUpTo(margins)

Which makes me wonder if what we're looking for is actually a 'Align
to the next strictly minor/major value' function.

Thanks
   j


>  			pixelFormat = formats::NV12;
>  			bufferCount = IPU3_BUFFER_COUNT;
>  			streamFormats[pixelFormat] = { { IMGU_OUTPUT_MIN_SIZE, size } };
> @@ -996,8 +995,7 @@ int PipelineHandlerIPU3::initControls(IPU3CameraData *data)
>  	 */
>
>  	/* The strictly smaller size than the sensor resolution, aligned to margins. */
> -	Size minSize = Size(sensor->resolution().width - 1,
> -			    sensor->resolution().height - 1)
> +	Size minSize = sensor->resolution().shrunkBy({ 1, 1 })
>  		       .alignedDownTo(IMGU_OUTPUT_WIDTH_MARGIN,
>  				      IMGU_OUTPUT_HEIGHT_MARGIN);
>
> @@ -1005,8 +1003,7 @@ int PipelineHandlerIPU3::initControls(IPU3CameraData *data)
>  	 * Either the smallest margin-aligned size larger than the viewfinder
>  	 * size or the adjusted sensor resolution.
>  	 */
> -	minSize = Size(IPU3ViewfinderSize.width + 1,
> -		       IPU3ViewfinderSize.height + 1)
> +	minSize = IPU3ViewfinderSize.grownBy({ 1, 1 })
>  		  .alignedUpTo(IMGU_OUTPUT_WIDTH_MARGIN,
>  			       IMGU_OUTPUT_HEIGHT_MARGIN)
>  		  .boundedTo(minSize);
> --
> Regards,
>
> Laurent Pinchart
>
Laurent Pinchart Oct. 13, 2021, 8:30 a.m. UTC | #2 
Hi Jacopo,

On Wed, Oct 13, 2021 at 08:43:08AM +0200, Jacopo Mondi wrote:
> On Wed, Oct 13, 2021 at 04:26:30AM +0300, Laurent Pinchart wrote:
> > The Size class has new helpers that can simplify the code in the IPU3
> > pipeline handler. Use them.
> >
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > ---
> >  src/libcamera/pipeline/ipu3/ipu3.cpp | 15 ++++++---------
> >  1 file changed, 6 insertions(+), 9 deletions(-)
> >
> > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > index 92e869257e53..262b9a23703e 100644
> > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > @@ -438,11 +438,10 @@ CameraConfiguration *PipelineHandlerIPU3::generateConfiguration(Camera *camera,
> >  			 * \todo Clarify the alignment constraints as explained
> >  			 * in validate()
> >  			 */
> > -			size = sensorResolution.boundedTo(IMGU_OUTPUT_MAX_SIZE);
> > -			size.width = utils::alignDown(size.width - 1,
> > -						      IMGU_OUTPUT_WIDTH_MARGIN);
> > -			size.height = utils::alignDown(size.height - 1,
> > -						       IMGU_OUTPUT_HEIGHT_MARGIN);
> > +			size = sensorResolution.boundedTo(IMGU_OUTPUT_MAX_SIZE)
> > +				.shrunkBy({ 1, 1 })
> > +				.alignedDownTo(IMGU_OUTPUT_WIDTH_MARGIN,
> > +					       IMGU_OUTPUT_HEIGHT_MARGIN);
> 
> Can you align the dots ?
> 
> 			size = sensorResolution.boundedTo(IMGU_OUTPUT_MAX_SIZE)
> 				               .shrunkBy({ 1, 1 })
> 				               .alignedDownTo(IMGU_OUTPUT_WIDTH_MARGIN,
> 					                      IMGU_OUTPUT_HEIGHT_MARGIN);
> 
> Same below.
> 
> Or was it intentional ?

I've copied the style from further down in this file, I don't mind it
either way.

> (I'm honest, I found the previous version more immediate compared than
> going through shrunk).

We can skip this patch if you think it doesn't bring any readability
improvement.

> Anyway, there's a repeating pattern of
> 
>                 {size.width - 1, size.height - 1}.alignDownTo(margins)
>                 {size.width + 1, size.height + 1}.alignUpTo(margins)
> 
> Which makes me wonder if what we're looking for is actually a 'Align
> to the next strictly minor/major value' function.

Good question. For IPU3 in particular, I'm still not sure what we'll
need once we complete the (mythical) rework of the ImgU configuration
:-) I haven't checked the other pipeline handlers and IPAs yet, this
series was a by-product of the review of your frame durations series
where I noticed we were missing these helpers.

> >  			pixelFormat = formats::NV12;
> >  			bufferCount = IPU3_BUFFER_COUNT;
> >  			streamFormats[pixelFormat] = { { IMGU_OUTPUT_MIN_SIZE, size } };
> > @@ -996,8 +995,7 @@ int PipelineHandlerIPU3::initControls(IPU3CameraData *data)
> >  	 */
> >
> >  	/* The strictly smaller size than the sensor resolution, aligned to margins. */
> > -	Size minSize = Size(sensor->resolution().width - 1,
> > -			    sensor->resolution().height - 1)
> > +	Size minSize = sensor->resolution().shrunkBy({ 1, 1 })
> >  		       .alignedDownTo(IMGU_OUTPUT_WIDTH_MARGIN,
> >  				      IMGU_OUTPUT_HEIGHT_MARGIN);
> >
> > @@ -1005,8 +1003,7 @@ int PipelineHandlerIPU3::initControls(IPU3CameraData *data)
> >  	 * Either the smallest margin-aligned size larger than the viewfinder
> >  	 * size or the adjusted sensor resolution.
> >  	 */
> > -	minSize = Size(IPU3ViewfinderSize.width + 1,
> > -		       IPU3ViewfinderSize.height + 1)
> > +	minSize = IPU3ViewfinderSize.grownBy({ 1, 1 })
> >  		  .alignedUpTo(IMGU_OUTPUT_WIDTH_MARGIN,
> >  			       IMGU_OUTPUT_HEIGHT_MARGIN)
> >  		  .boundedTo(minSize);
Jacopo Mondi Oct. 14, 2021, 11:23 a.m. UTC | #3 
Hi Laurent

On Wed, Oct 13, 2021 at 11:30:45AM +0300, Laurent Pinchart wrote:
> Hi Jacopo,
>
> On Wed, Oct 13, 2021 at 08:43:08AM +0200, Jacopo Mondi wrote:
> > On Wed, Oct 13, 2021 at 04:26:30AM +0300, Laurent Pinchart wrote:
> > > The Size class has new helpers that can simplify the code in the IPU3
> > > pipeline handler. Use them.
> > >
> > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > ---
> > >  src/libcamera/pipeline/ipu3/ipu3.cpp | 15 ++++++---------
> > >  1 file changed, 6 insertions(+), 9 deletions(-)
> > >
> > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > > index 92e869257e53..262b9a23703e 100644
> > > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> > > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > > @@ -438,11 +438,10 @@ CameraConfiguration *PipelineHandlerIPU3::generateConfiguration(Camera *camera,
> > >  			 * \todo Clarify the alignment constraints as explained
> > >  			 * in validate()
> > >  			 */
> > > -			size = sensorResolution.boundedTo(IMGU_OUTPUT_MAX_SIZE);
> > > -			size.width = utils::alignDown(size.width - 1,
> > > -						      IMGU_OUTPUT_WIDTH_MARGIN);
> > > -			size.height = utils::alignDown(size.height - 1,
> > > -						       IMGU_OUTPUT_HEIGHT_MARGIN);
> > > +			size = sensorResolution.boundedTo(IMGU_OUTPUT_MAX_SIZE)
> > > +				.shrunkBy({ 1, 1 })
> > > +				.alignedDownTo(IMGU_OUTPUT_WIDTH_MARGIN,
> > > +					       IMGU_OUTPUT_HEIGHT_MARGIN);
> >
> > Can you align the dots ?
> >
> > 			size = sensorResolution.boundedTo(IMGU_OUTPUT_MAX_SIZE)
> > 				               .shrunkBy({ 1, 1 })
> > 				               .alignedDownTo(IMGU_OUTPUT_WIDTH_MARGIN,
> > 					                      IMGU_OUTPUT_HEIGHT_MARGIN);
> >
> > Same below.
> >
> > Or was it intentional ?
>
> I've copied the style from further down in this file, I don't mind it
> either way.
>
> > (I'm honest, I found the previous version more immediate compared than
> > going through shrunk).
>
> We can skip this patch if you think it doesn't bring any readability
> improvement.
>

Nah, we're talking details

> > Anyway, there's a repeating pattern of
> >
> >                 {size.width - 1, size.height - 1}.alignDownTo(margins)
> >                 {size.width + 1, size.height + 1}.alignUpTo(margins)
> >
> > Which makes me wonder if what we're looking for is actually a 'Align
> > to the next strictly minor/major value' function.
>
> Good question. For IPU3 in particular, I'm still not sure what we'll
> need once we complete the (mythical) rework of the ImgU configuration
> :-) I haven't checked the other pipeline handlers and IPAs yet, this
> series was a by-product of the review of your frame durations series
> where I noticed we were missing these helpers.
>

Sure, if we'll end up having to maintain the shrunks we'll reconsider
a new helper.

Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>

I'll base my next frame duration series on top of these patches!

Thanks
  j

> > >  			pixelFormat = formats::NV12;
> > >  			bufferCount = IPU3_BUFFER_COUNT;
> > >  			streamFormats[pixelFormat] = { { IMGU_OUTPUT_MIN_SIZE, size } };
> > > @@ -996,8 +995,7 @@ int PipelineHandlerIPU3::initControls(IPU3CameraData *data)
> > >  	 */
> > >
> > >  	/* The strictly smaller size than the sensor resolution, aligned to margins. */
> > > -	Size minSize = Size(sensor->resolution().width - 1,
> > > -			    sensor->resolution().height - 1)
> > > +	Size minSize = sensor->resolution().shrunkBy({ 1, 1 })
> > >  		       .alignedDownTo(IMGU_OUTPUT_WIDTH_MARGIN,
> > >  				      IMGU_OUTPUT_HEIGHT_MARGIN);
> > >
> > > @@ -1005,8 +1003,7 @@ int PipelineHandlerIPU3::initControls(IPU3CameraData *data)
> > >  	 * Either the smallest margin-aligned size larger than the viewfinder
> > >  	 * size or the adjusted sensor resolution.
> > >  	 */
> > > -	minSize = Size(IPU3ViewfinderSize.width + 1,
> > > -		       IPU3ViewfinderSize.height + 1)
> > > +	minSize = IPU3ViewfinderSize.grownBy({ 1, 1 })
> > >  		  .alignedUpTo(IMGU_OUTPUT_WIDTH_MARGIN,
> > >  			       IMGU_OUTPUT_HEIGHT_MARGIN)
> > >  		  .boundedTo(minSize);
>
> --
> Regards,
>
> Laurent Pinchart
Kieran Bingham Oct. 15, 2021, 10:55 p.m. UTC | #4 
Quoting Jacopo Mondi (2021-10-14 12:23:17)
> Hi Laurent
> 
> On Wed, Oct 13, 2021 at 11:30:45AM +0300, Laurent Pinchart wrote:
> > Hi Jacopo,
> >
> > On Wed, Oct 13, 2021 at 08:43:08AM +0200, Jacopo Mondi wrote:
> > > On Wed, Oct 13, 2021 at 04:26:30AM +0300, Laurent Pinchart wrote:
> > > > The Size class has new helpers that can simplify the code in the IPU3
> > > > pipeline handler. Use them.
> > > >
> > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > > ---
> > > >  src/libcamera/pipeline/ipu3/ipu3.cpp | 15 ++++++---------
> > > >  1 file changed, 6 insertions(+), 9 deletions(-)
> > > >
> > > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > > > index 92e869257e53..262b9a23703e 100644
> > > > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> > > > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > > > @@ -438,11 +438,10 @@ CameraConfiguration *PipelineHandlerIPU3::generateConfiguration(Camera *camera,
> > > >                    * \todo Clarify the alignment constraints as explained
> > > >                    * in validate()
> > > >                    */
> > > > -                 size = sensorResolution.boundedTo(IMGU_OUTPUT_MAX_SIZE);
> > > > -                 size.width = utils::alignDown(size.width - 1,
> > > > -                                               IMGU_OUTPUT_WIDTH_MARGIN);
> > > > -                 size.height = utils::alignDown(size.height - 1,
> > > > -                                                IMGU_OUTPUT_HEIGHT_MARGIN);
> > > > +                 size = sensorResolution.boundedTo(IMGU_OUTPUT_MAX_SIZE)
> > > > +                         .shrunkBy({ 1, 1 })
> > > > +                         .alignedDownTo(IMGU_OUTPUT_WIDTH_MARGIN,
> > > > +                                        IMGU_OUTPUT_HEIGHT_MARGIN);
> > >
> > > Can you align the dots ?
> > >
> > >                     size = sensorResolution.boundedTo(IMGU_OUTPUT_MAX_SIZE)
> > >                                            .shrunkBy({ 1, 1 })
> > >                                            .alignedDownTo(IMGU_OUTPUT_WIDTH_MARGIN,
> > >                                                           IMGU_OUTPUT_HEIGHT_MARGIN);
> > >
> > > Same below.
> > >
> > > Or was it intentional ?
> >
> > I've copied the style from further down in this file, I don't mind it
> > either way.

For chaining calls, I think aligning on the dots is better.

.shrunkBy({1,1}) looks cleaner than the size.width -1 ...

> >
> > > (I'm honest, I found the previous version more immediate compared than
> > > going through shrunk).
> >
> > We can skip this patch if you think it doesn't bring any readability
> > improvement.
> >
> 
> Nah, we're talking details
> 
> > > Anyway, there's a repeating pattern of
> > >
> > >                 {size.width - 1, size.height - 1}.alignDownTo(margins)
> > >                 {size.width + 1, size.height + 1}.alignUpTo(margins)
> > >
> > > Which makes me wonder if what we're looking for is actually a 'Align
> > > to the next strictly minor/major value' function.

But it does seem like this higher level helper might end up being used.
Still, these improvements stand on their own until we figure out if we
do need something else.


Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

> >
> > Good question. For IPU3 in particular, I'm still not sure what we'll
> > need once we complete the (mythical) rework of the ImgU configuration
> > :-) I haven't checked the other pipeline handlers and IPAs yet, this
> > series was a by-product of the review of your frame durations series
> > where I noticed we were missing these helpers.
> >
> 
> Sure, if we'll end up having to maintain the shrunks we'll reconsider
> a new helper.
> 
> Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
> 
> I'll base my next frame duration series on top of these patches!
> 
> Thanks
>   j
> 
> > > >                   pixelFormat = formats::NV12;
> > > >                   bufferCount = IPU3_BUFFER_COUNT;
> > > >                   streamFormats[pixelFormat] = { { IMGU_OUTPUT_MIN_SIZE, size } };
> > > > @@ -996,8 +995,7 @@ int PipelineHandlerIPU3::initControls(IPU3CameraData *data)
> > > >    */
> > > >
> > > >   /* The strictly smaller size than the sensor resolution, aligned to margins. */
> > > > - Size minSize = Size(sensor->resolution().width - 1,
> > > > -                     sensor->resolution().height - 1)
> > > > + Size minSize = sensor->resolution().shrunkBy({ 1, 1 })
> > > >                  .alignedDownTo(IMGU_OUTPUT_WIDTH_MARGIN,
> > > >                                 IMGU_OUTPUT_HEIGHT_MARGIN);
> > > >
> > > > @@ -1005,8 +1003,7 @@ int PipelineHandlerIPU3::initControls(IPU3CameraData *data)
> > > >    * Either the smallest margin-aligned size larger than the viewfinder
> > > >    * size or the adjusted sensor resolution.
> > > >    */
> > > > - minSize = Size(IPU3ViewfinderSize.width + 1,
> > > > -                IPU3ViewfinderSize.height + 1)
> > > > + minSize = IPU3ViewfinderSize.grownBy({ 1, 1 })
> > > >             .alignedUpTo(IMGU_OUTPUT_WIDTH_MARGIN,
> > > >                          IMGU_OUTPUT_HEIGHT_MARGIN)
> > > >             .boundedTo(minSize);
> >
> > --
> > Regards,
> >
> > Laurent Pinchart

Patch
diff mbox series 

diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
index 92e869257e53..262b9a23703e 100644
--- a/src/libcamera/pipeline/ipu3/ipu3.cpp
+++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
@@ -438,11 +438,10 @@  CameraConfiguration *PipelineHandlerIPU3::generateConfiguration(Camera *camera,
 			 * \todo Clarify the alignment constraints as explained
 			 * in validate()
 			 */
-			size = sensorResolution.boundedTo(IMGU_OUTPUT_MAX_SIZE);
-			size.width = utils::alignDown(size.width - 1,
-						      IMGU_OUTPUT_WIDTH_MARGIN);
-			size.height = utils::alignDown(size.height - 1,
-						       IMGU_OUTPUT_HEIGHT_MARGIN);
+			size = sensorResolution.boundedTo(IMGU_OUTPUT_MAX_SIZE)
+				.shrunkBy({ 1, 1 })
+				.alignedDownTo(IMGU_OUTPUT_WIDTH_MARGIN,
+					       IMGU_OUTPUT_HEIGHT_MARGIN);
 			pixelFormat = formats::NV12;
 			bufferCount = IPU3_BUFFER_COUNT;
 			streamFormats[pixelFormat] = { { IMGU_OUTPUT_MIN_SIZE, size } };
@@ -996,8 +995,7 @@  int PipelineHandlerIPU3::initControls(IPU3CameraData *data)
 	 */
 
 	/* The strictly smaller size than the sensor resolution, aligned to margins. */
-	Size minSize = Size(sensor->resolution().width - 1,
-			    sensor->resolution().height - 1)
+	Size minSize = sensor->resolution().shrunkBy({ 1, 1 })
 		       .alignedDownTo(IMGU_OUTPUT_WIDTH_MARGIN,
 				      IMGU_OUTPUT_HEIGHT_MARGIN);
 
@@ -1005,8 +1003,7 @@  int PipelineHandlerIPU3::initControls(IPU3CameraData *data)
 	 * Either the smallest margin-aligned size larger than the viewfinder
 	 * size or the adjusted sensor resolution.
 	 */
-	minSize = Size(IPU3ViewfinderSize.width + 1,
-		       IPU3ViewfinderSize.height + 1)
+	minSize = IPU3ViewfinderSize.grownBy({ 1, 1 })
 		  .alignedUpTo(IMGU_OUTPUT_WIDTH_MARGIN,
 			       IMGU_OUTPUT_HEIGHT_MARGIN)
 		  .boundedTo(minSize);