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

[Jacopo Mondi]

Hi Kieran

On Thu, Aug 25, 2022 at 10:55:53AM +0100, Kieran Bingham wrote:
> 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>

I can update the comment when applying the patch

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