| Message ID | 20221028115200.8138-3-naush@raspberrypi.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Naush Thanks for the patch! On Fri, 28 Oct 2022 at 12:52, Naushir Patuck via libcamera-devel <libcamera-devel@lists.libcamera.org> wrote: > > When a camera is terminated, do not unconditionally free buffers in the > RPiCameraData destructor. Otherwise, this causes harmless error log messages > to be displayed if no buffer have previously been allocated. > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> All looks good! Tested-by: David Plowman <david.plowman@raspberrypi.com> Reviewed-by: David Plowman <david.plowman@raspberrypi.com> Thanks David > --- > src/libcamera/pipeline/raspberrypi/raspberrypi.cpp | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > index 343f8cb2c7ed..31107e1338bf 100644 > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > @@ -191,7 +191,8 @@ public: > > ~RPiCameraData() > { > - freeBuffers(); > + if (buffersAllocated_) > + freeBuffers(); > } > > void freeBuffers(); > -- > 2.25.1 >
Quoting Naushir Patuck via libcamera-devel (2022-10-28 12:52:00) > When a camera is terminated, do not unconditionally free buffers in the > RPiCameraData destructor. Otherwise, this causes harmless error log messages > to be displayed if no buffer have previously been allocated. > Much like the V4L2 patch, I would think putting this check into the beginning of void RPiCameraData::freeBuffers() would be safer. I see freeBuffers is called a few times, (configure, start), which I presume already ensure they are allocated ... but catching this for sure for all cases could make sense. Anyway, that's up to you, however you prefer: Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > --- > src/libcamera/pipeline/raspberrypi/raspberrypi.cpp | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > index 343f8cb2c7ed..31107e1338bf 100644 > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > @@ -191,7 +191,8 @@ public: > > ~RPiCameraData() > { > - freeBuffers(); > + if (buffersAllocated_) > + freeBuffers(); > } > > void freeBuffers(); > -- > 2.25.1 >
Hi Kieran, Thanks for the review. On Fri, 28 Oct 2022 at 14:46, Kieran Bingham < kieran.bingham@ideasonboard.com> wrote: > Quoting Naushir Patuck via libcamera-devel (2022-10-28 12:52:00) > > When a camera is terminated, do not unconditionally free buffers in the > > RPiCameraData destructor. Otherwise, this causes harmless error log > messages > > to be displayed if no buffer have previously been allocated. > > > > Much like the V4L2 patch, I would think putting this check into > the beginning of void RPiCameraData::freeBuffers() would be safer. > > I see freeBuffers is called a few times, (configure, start), which I > presume already ensure they are allocated ... but catching this for sure > for all cases could make sense. > Agree, I'll make the update shortly. Again, I blame Friday!! Naush > > Anyway, that's up to you, however you prefer: > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > > --- > > src/libcamera/pipeline/raspberrypi/raspberrypi.cpp | 3 ++- > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > index 343f8cb2c7ed..31107e1338bf 100644 > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > @@ -191,7 +191,8 @@ public: > > > > ~RPiCameraData() > > { > > - freeBuffers(); > > + if (buffersAllocated_) > > + freeBuffers(); > > } > > > > void freeBuffers(); > > -- > > 2.25.1 > > >
diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp index 343f8cb2c7ed..31107e1338bf 100644 --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp @@ -191,7 +191,8 @@ public: ~RPiCameraData() { - freeBuffers(); + if (buffersAllocated_) + freeBuffers(); } void freeBuffers();
When a camera is terminated, do not unconditionally free buffers in the RPiCameraData destructor. Otherwise, this causes harmless error log messages to be displayed if no buffer have previously been allocated. Signed-off-by: Naushir Patuck <naush@raspberrypi.com> --- src/libcamera/pipeline/raspberrypi/raspberrypi.cpp | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)