| Message ID | 20240606115223.14868-1-naush@raspberrypi.com |
|---|---|
| State | Accepted |
| Commit | 634bc7838f879c208df1915b2259e2a7f8dcefd7 |
| Headers | show |
| Series |
|
| Related | show |
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@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@raspberrypi.com> Reviewed-by: David Plowman <david.plowman@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(); > - > return config; > } > > -- > 2.34.1 >
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@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@raspberrypi.com> > > Reviewed-by: David Plowman <david.plowman@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@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 > >
Hi Kieran, On Thu, 6 Jun 2024 at 15:57, Kieran Bingham <kieran.bingham@ideasonboard.com> wrote: > > 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@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@raspberrypi.com> > > > > Reviewed-by: David Plowman <david.plowman@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? As long as we never change the defaults (I don't see why we will), I think we can remove the validate() call permanently. > > > 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). Possibly, but the key thing going wrong is for RPi validate() will update the stride fields to what the driver chooses. We use this functionality to allocate dmabufs outside of v4l2 if needed. Not sure if other pipeline handlers do this. > > > 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 ... Yes, this will still happen when configure() calls validate(). Regards, Naush > > > > Reviewed-by: Kieran Bingham <kieran.bingham@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 > > >
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(); - return config; }
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@raspberrypi.com> --- src/libcamera/pipeline/rpi/common/pipeline_base.cpp | 2 -- 1 file changed, 2 deletions(-)