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

[Han-lin Chen]

On Thu, Aug 25, 2022 at 6:05 PM Jacopo Mondi via libcamera-devel
<libcamera-devel at lists.libcamera.org> wrote:
>
> 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
Many thanks Jacopo :)
>
> >
> > >
> > > Thanks
> > >   j
> > >
> > > >
> > > >       cio2Configuration_ = data_->cio2_.generateConfiguration(rawSize);
> > > >       if (!cio2Configuration_.pixelFormat.isValid())
> > > > --
> > > > 2.37.1.595.g718a3a8f04-goog
> > > >



-- 
Cheers.
Hanlin Chen