[PATCH] pipeline: rpi: Avoid duplicating size range for the same pixel format

[David Plowman]

Hi Jacopo

Thanks for reviewing the patch!

On Fri, 26 Apr 2024 at 14:06, Jacopo Mondi
<jacopo.mondi at ideasonboard.com> wrote:
>
> Hi David
>
> On Thu, Apr 25, 2024 at 12:37:48PM +0100, Naushir Patuck wrote:
> > Hi David,
> >
> > Thank you for this patch.
> >
> > On Thu, 25 Apr 2024 at 11:52, David Plowman
> > <david.plowman at raspberrypi.com> wrote:
> > >
> > > Some V4L2 formats translate to the same pixel format, e.g. YU12 and
> > > YM12 both produce YUV420. In this case our ISP driver advertises the
> > > same size range for both, but we must not record the same thing twice
> > > for the same pixel format (which will cause a failure later on).
>
> Am I wrong or this mostly (only) apply to contiguous and non-contiguous
> format versions ?

I think that's right. I quoted the YU12/YM12 example, but we have the
same thing with NV12 contiguous/non-contiguous and probably a few
others as well.

>
> > >
> > > Instead, ignore the V4l2 format if the pixel format has already been
> > > seen.
> > >
> > > Signed-off-by: David Plowman <david.plowman at raspberrypi.com>
> >
> > Reviewed-by: Naushir Patuck <naush at raspberrypi.com>
>
> >
> > > ---
> > >  src/libcamera/pipeline/rpi/common/pipeline_base.cpp | 6 +++++-
> > >  1 file changed, 5 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp
> > > index 7e420b3f..0034e06d 100644
> > > --- a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp
> > > +++ b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp
> > > @@ -474,7 +474,11 @@ PipelineHandlerBase::generateConfiguration(Camera *camera, Span<const StreamRole
> > >                          */
> > >                         for (const auto &format : fmts) {
> > >                                 PixelFormat pf = format.first.toPixelFormat();
> > > -                               if (pf.isValid()) {
> > > +                               /*
> > > +                                * Some V4L2 formats translate to the same pixel format (e.g. YU12, YM12
> > > +                                * both give YUV420). We must avoid duplicating the range in this case.
> > > +                                */
> > > +                               if (pf.isValid() && deviceFormats.find(pf) == deviceFormats.end()) {
>
> Or
>                                     if (pf.isValid() && !deviceFormats.count(pf)) {

True, I mostly don't think to use count because it's not such a good
way of testing "are there any?". But for "are there none?" of course
it's fine!

Thanks!
David

>
>
> The change is anyway good!
> Reviewed-by: Jacopo Mondi <jacopo.mondi at ideasonboard.com>
>
> > >                                         const SizeRange &ispSizes = format.second[0];
> > >                                         deviceFormats[pf].emplace_back(ispSizes.min, sensorSize,
> > >                                                                        ispSizes.hStep, ispSizes.vStep);
> > > --
> > > 2.39.2
> > >