| Message ID | 20230523120208.31292-1-naush@raspberrypi.com |
|---|---|
| State | Accepted |
| Commit | 58f38082e14c79a16b0b6373c14b0e56c8ac15bc |
| Headers | show |
| Series |
|
| Related | show |
Hi Naush On Tue, May 23, 2023 at 01:02:08PM +0100, Naushir Patuck via libcamera-devel wrote: > If a user uses a pipeline handler config file where a custom timeout > value is specified, it would lead to a segmentation fault when > loadPipelineConfiguration() tried to access the as yet uninitialised s/the as yet/the yet/ > V4L2VideoDevice object. > > To fix this, parse the pipeline handler config file only after fully > initialising and registering the pipeline handler and V4L2VideoDevice > objects. Indeed data->frontendDevice() will return null in that case. Alternatively, if you think loadind configuration file before platformRegister() might be useful, you can set the dequeue timeout after having created the V4L2Device in platformRegister(). Either ways Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> Thanks j > > Fixes: 6c71ee1f15305 ("pipeline: raspberrypi: Introduce PipelineHandlerBase class") > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > --- > src/libcamera/pipeline/rpi/common/pipeline_base.cpp | 8 ++++---- > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp > index ba1797bcfef0..3bb5ec531e4f 100644 > --- a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp > +++ b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp > @@ -888,16 +888,16 @@ int PipelineHandlerBase::registerCamera(std::unique_ptr<RPi::CameraData> &camera > } > data->nativeBayerOrder_ = bayerFormat.order; > > + ret = platformRegister(cameraData, frontend, backend); > + if (ret) > + return ret; > + > ret = data->loadPipelineConfiguration(); > if (ret) { > LOG(RPI, Error) << "Unable to load pipeline configuration"; > return ret; > } > > - ret = platformRegister(cameraData, frontend, backend); > - if (ret) > - return ret; > - > /* Setup the general IPA signal handlers. */ > data->frontendDevice()->dequeueTimeout.connect(data, &RPi::CameraData::cameraTimeout); > data->frontendDevice()->frameStart.connect(data, &RPi::CameraData::frameStarted); > -- > 2.34.1 >
Hi Naush Thanks for the patch! On Tue, 23 May 2023 at 13:03, Naushir Patuck via libcamera-devel <libcamera-devel@lists.libcamera.org> wrote: > > If a user uses a pipeline handler config file where a custom timeout > value is specified, it would lead to a segmentation fault when > loadPipelineConfiguration() tried to access the as yet uninitialised > V4L2VideoDevice object. > > To fix this, parse the pipeline handler config file only after fully > initialising and registering the pipeline handler and V4L2VideoDevice > objects. > > Fixes: 6c71ee1f15305 ("pipeline: raspberrypi: Introduce PipelineHandlerBase class") > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> Looks good to me. (Actually, I think "the as yet" is probably OK...) Reviewed-by: David Plowman <david.plowman@raspberrypi.com> Thanks David > --- > src/libcamera/pipeline/rpi/common/pipeline_base.cpp | 8 ++++---- > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp > index ba1797bcfef0..3bb5ec531e4f 100644 > --- a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp > +++ b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp > @@ -888,16 +888,16 @@ int PipelineHandlerBase::registerCamera(std::unique_ptr<RPi::CameraData> &camera > } > data->nativeBayerOrder_ = bayerFormat.order; > > + ret = platformRegister(cameraData, frontend, backend); > + if (ret) > + return ret; > + > ret = data->loadPipelineConfiguration(); > if (ret) { > LOG(RPI, Error) << "Unable to load pipeline configuration"; > return ret; > } > > - ret = platformRegister(cameraData, frontend, backend); > - if (ret) > - return ret; > - > /* Setup the general IPA signal handlers. */ > data->frontendDevice()->dequeueTimeout.connect(data, &RPi::CameraData::cameraTimeout); > data->frontendDevice()->frameStart.connect(data, &RPi::CameraData::frameStarted); > -- > 2.34.1 >
On Wed, May 24, 2023 at 02:00:37PM +0100, David Plowman via libcamera-devel wrote: > Hi Naush > > Thanks for the patch! > > On Tue, 23 May 2023 at 13:03, Naushir Patuck via libcamera-devel > <libcamera-devel@lists.libcamera.org> wrote: > > > > If a user uses a pipeline handler config file where a custom timeout > > value is specified, it would lead to a segmentation fault when > > loadPipelineConfiguration() tried to access the as yet uninitialised > > V4L2VideoDevice object. > > > > To fix this, parse the pipeline handler config file only after fully > > initialising and registering the pipeline handler and V4L2VideoDevice > > objects. > > > > Fixes: 6c71ee1f15305 ("pipeline: raspberrypi: Introduce PipelineHandlerBase class") > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > > Looks good to me. (Actually, I think "the as yet" is probably OK...) Ah sorry, thought it was a typo.. I didn't mean to correct a native speaker :) > > Reviewed-by: David Plowman <david.plowman@raspberrypi.com> > > Thanks > David > > > --- > > src/libcamera/pipeline/rpi/common/pipeline_base.cpp | 8 ++++---- > > 1 file changed, 4 insertions(+), 4 deletions(-) > > > > diff --git a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp > > index ba1797bcfef0..3bb5ec531e4f 100644 > > --- a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp > > +++ b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp > > @@ -888,16 +888,16 @@ int PipelineHandlerBase::registerCamera(std::unique_ptr<RPi::CameraData> &camera > > } > > data->nativeBayerOrder_ = bayerFormat.order; > > > > + ret = platformRegister(cameraData, frontend, backend); > > + if (ret) > > + return ret; > > + > > ret = data->loadPipelineConfiguration(); > > if (ret) { > > LOG(RPI, Error) << "Unable to load pipeline configuration"; > > return ret; > > } > > > > - ret = platformRegister(cameraData, frontend, backend); > > - if (ret) > > - return ret; > > - > > /* Setup the general IPA signal handlers. */ > > data->frontendDevice()->dequeueTimeout.connect(data, &RPi::CameraData::cameraTimeout); > > data->frontendDevice()->frameStart.connect(data, &RPi::CameraData::frameStarted); > > -- > > 2.34.1 > >
diff --git a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp index ba1797bcfef0..3bb5ec531e4f 100644 --- a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp +++ b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp @@ -888,16 +888,16 @@ int PipelineHandlerBase::registerCamera(std::unique_ptr<RPi::CameraData> &camera } data->nativeBayerOrder_ = bayerFormat.order; + ret = platformRegister(cameraData, frontend, backend); + if (ret) + return ret; + ret = data->loadPipelineConfiguration(); if (ret) { LOG(RPI, Error) << "Unable to load pipeline configuration"; return ret; } - ret = platformRegister(cameraData, frontend, backend); - if (ret) - return ret; - /* Setup the general IPA signal handlers. */ data->frontendDevice()->dequeueTimeout.connect(data, &RPi::CameraData::cameraTimeout); data->frontendDevice()->frameStart.connect(data, &RPi::CameraData::frameStarted);
If a user uses a pipeline handler config file where a custom timeout value is specified, it would lead to a segmentation fault when loadPipelineConfiguration() tried to access the as yet uninitialised V4L2VideoDevice object. To fix this, parse the pipeline handler config file only after fully initialising and registering the pipeline handler and V4L2VideoDevice objects. Fixes: 6c71ee1f15305 ("pipeline: raspberrypi: Introduce PipelineHandlerBase class") Signed-off-by: Naushir Patuck <naush@raspberrypi.com> --- src/libcamera/pipeline/rpi/common/pipeline_base.cpp | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)