[libcamera-devel] [PATCH v2] libcamera: ipu3: Use std::max() instead of expandTo() to get the max resolution

[Kieran Bingham]

Quoting Jacopo Mondi via libcamera-devel (2022-08-17 09:19:29)
> Hi Han-Lin
> 
> On Fri, Aug 12, 2022 at 05:01:03PM +0800, Han-Lin Chen via libcamera-devel wrote:
> > Using Size::expandTo() to find the max resolution might generate a non-existent
> > resolution. For example, when application request streams for 1920x1080 and
> > 1600x1200, the max resolution will be wrongly 1920x1200 and fails the
> > configuration.
> >
> > Bug: https://bugs.libcamera.org/show_bug.cgi?id=139
> > Signed-off-by: Han-Lin Chen <hanlinchen at chromium.org>
> > ---
> >  src/libcamera/pipeline/ipu3/ipu3.cpp | 22 ++++++++++++----------
> >  1 file changed, 12 insertions(+), 10 deletions(-)
> >
> > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > index f65db3c8..47311953 100644
> > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > @@ -247,6 +247,7 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate()
> >        */
> >       unsigned int rawCount = 0;
> >       unsigned int yuvCount = 0;
> > +     Size rawRequirement;
> >       Size maxYuvSize;
> >       Size rawSize;
> >
> > @@ -255,10 +256,11 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate()
> >
> >               if (info.colourEncoding == PixelFormatInfo::ColourEncodingRAW) {
> >                       rawCount++;
> > -                     rawSize.expandTo(cfg.size);
> > +                     rawSize = std::max(rawSize, cfg.size);
> >               } else {
> >                       yuvCount++;
> > -                     maxYuvSize.expandTo(cfg.size);
> > +                     maxYuvSize = std::max(maxYuvSize, cfg.size);
> > +                     rawRequirement.expandTo(cfg.size);
> >               }
> >       }
> >
> > @@ -287,17 +289,17 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate()
> >        * The output YUV streams will be limited in size to the maximum frame
> >        * size requested for the RAW stream, if present.
> >        *
> > -      * If no raw stream is requested generate a size as large as the maximum
> > -      * requested YUV size aligned to the ImgU constraints and bound by the
> > -      * sensor's maximum resolution. See
> > +      * If no raw stream is requested generate a size as large as the span

I know it's pre-existing, but could you add a comma here? Without it the
sentence flows too fast in my head.

	"If no raw stream is requested, generate a size as ...

I think this is called a "First conditional sentence" 
	https://www.grammarly.com/blog/conditional-sentences/



> > +      * of all requested YUV size aligned to the ImgU constraints and bound
> > +      * by the sensor's maximum resolution. See

I'm confused here about the use of 'a span of all requested YUV sizes'
as that feels like it gets confused with a libcamera::Span concept which
I don't think is what it means here.

I would say

"If no raw stream is requested, generate a size from the largest given
YUV stream, aligned to the ImgU constraints and bound by the sensor's
maximum resolution. See:"


> >        * https://bugs.libcamera.org/show_bug.cgi?id=32
> >        */
> >       if (rawSize.isNull())
> > -             rawSize = maxYuvSize.expandedTo({ ImgUDevice::kIFMaxCropWidth,
> > -                                               ImgUDevice::kIFMaxCropHeight })
> > -                                 .grownBy({ ImgUDevice::kOutputMarginWidth,
> > -                                            ImgUDevice::kOutputMarginHeight })
> > -                                 .boundedTo(data_->cio2_.sensor()->resolution());
> > +             rawSize = rawRequirement.expandedTo({ ImgUDevice::kIFMaxCropWidth,
> > +                                                   ImgUDevice::kIFMaxCropHeight })
> > +                               .grownBy({ ImgUDevice::kOutputMarginWidth,
> > +                                          ImgUDevice::kOutputMarginHeight })
> > +                               .boundedTo(data_->cio2_.sensor()->resolution());
> 
> This seems to fix the issue indeed, and maintain the current behaviour
> where the RAW stream size limits the YUV output size.
> 
> Reviewed-by: Jacopo Mondi <jacopo at jmondi.org>

With the comment updated in any appropriate way,

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

> 
> Thanks
>   j
> 
> >
> >       cio2Configuration_ = data_->cio2_.generateConfiguration(rawSize);
> >       if (!cio2Configuration_.pixelFormat.isValid())
> > --
> > 2.37.1.595.g718a3a8f04-goog
> >