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

[Laurent Pinchart]

Hello,

On Fri, Apr 26, 2024 at 02:16:03PM +0100, David Plowman wrote:
> On Fri, 26 Apr 2024 at 14:06, Jacopo Mondi wrote:
> > On Thu, Apr 25, 2024 at 12:37:48PM +0100, Naushir Patuck wrote:
> > > On Thu, 25 Apr 2024 at 11:52, David Plowman 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!

I'm fine either way. std::map::contains() will be nice, once we move to
C++20.

Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>

> > 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);

-- 
Regards,

Laurent Pinchart