[PATCH] pipeline: rpi: Don't validate configuration in generateConfiguration()

[Kieran Bingham]

Quoting David Plowman (2024-06-06 14:26:53)
> Hi Naush
> 
> Thanks for the patch. I agree that this is more helpful behaviour for users.
> 
> On Thu, 6 Jun 2024 at 12:52, Naushir Patuck <naush at raspberrypi.com> wrote:
> >
> > generateConfiguration() called validate() as a final step, causing the
> > stride and frameSize fields in StreamConfiguration to be filled in based
> > on the pixel format and width/height.
> >
> > If a user application did not clear the stride field when setting up a
> > custom pixel format and width/height, the pipeline handler would respect
> > this stride and possibly overallocate buffers with a larger stride than
> > needed.
> >
> > Fix this by removing the call to validate() completely, leaving the
> > stride and frameSize fields defaulting to 0. Removal of this call is
> > inconsequential as we hard-code a valid configuration for Raspberry Pi
> > platforms in generateConfiguration().
> >
> > Bug: https://github.com/raspberrypi/libcamera/issues/138
> > Bug: https://github.com/raspberrypi/libcamera/issues/141
> > Signed-off-by: Naushir Patuck <naush at raspberrypi.com>
> 
> Reviewed-by: David Plowman <david.plowman at raspberrypi.com>
> 
> Thanks
> David
> 
> > ---
> >  src/libcamera/pipeline/rpi/common/pipeline_base.cpp | 2 --
> >  1 file changed, 2 deletions(-)
> >
> > diff --git a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp
> > index 289af5165766..3041fd1ed9fd 100644
> > --- a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp
> > +++ b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp
> > @@ -496,8 +496,6 @@ PipelineHandlerBase::generateConfiguration(Camera *camera, Span<const StreamRole
> >                 config->addConfiguration(cfg);
> >         }
> >
> > -       config->validate();
> > -

I think we expect that configurations (except a completely empty one) we
return are 'valid'. We hardcode this so it should be fine, but I guess
we might potentially 'forget' and re-introduce this call later?

Does it mean we should never call validate on any pipeline that would
cause the same issue? (or maybe this is just specific to the patch that
went into the RPi kernel anyway).

I guess it's "impossible" for us to validate stride after it's gone to
the application (other than making sure it is at least bigger than the
width*bpp?) as applications might set it larger ...


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

I'll leave merging to next week incase there are any other comments /
concerns - but I do see this already fixes user facing issues.


> >         return config;
> >  }
> >
> > --
> > 2.34.1
> >