| Message ID | 20201112050243.32657-1-paul.elder@ideasonboard.com |
|---|---|
| State | Accepted |
| Commit | 2d50b1664508e86e4d27b8620e74f8e82dfe1d57 |
| Headers | show |
| Series |
|
| Related | show |
Hi Paul, Thank you for the patch. On Thu, Nov 12, 2020 at 02:02:43PM +0900, Paul Elder wrote: > To enable reusing Request objects, we kept a pool of free Requests. This > pool was not cleared upon stopping capture, however, which caused a > segfault when switching to another camera. Fix this by clearing the > Request pool on stopCapture(). > > Fixes: c753223ad6b9 ("libcamera, android, cam, gstreamer, qcam, v4l2: > Reuse Request") > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > src/qcam/main_window.cpp | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/src/qcam/main_window.cpp b/src/qcam/main_window.cpp > index f140313e..2502ecd4 100644 > --- a/src/qcam/main_window.cpp > +++ b/src/qcam/main_window.cpp > @@ -579,6 +579,7 @@ void MainWindow::stopCapture() > mappedBuffers_.clear(); > > requests_.clear(); > + freeQueue_.clear(); > > delete allocator_; >
On Thu, Nov 12, 2020 at 09:24:33AM +0200, Laurent Pinchart wrote: > Hi Paul, > > Thank you for the patch. > > On Thu, Nov 12, 2020 at 02:02:43PM +0900, Paul Elder wrote: > > To enable reusing Request objects, we kept a pool of free Requests. This > > pool was not cleared upon stopping capture, however, which caused a > > segfault when switching to another camera. Fix this by clearing the > > Request pool on stopCapture(). > > > > Fixes: c753223ad6b9 ("libcamera, android, cam, gstreamer, qcam, v4l2: > > Reuse Request") Nitpicking, this line shouldn't be wrapped. > > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com> > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > --- > > src/qcam/main_window.cpp | 1 + > > 1 file changed, 1 insertion(+) > > > > diff --git a/src/qcam/main_window.cpp b/src/qcam/main_window.cpp > > index f140313e..2502ecd4 100644 > > --- a/src/qcam/main_window.cpp > > +++ b/src/qcam/main_window.cpp > > @@ -579,6 +579,7 @@ void MainWindow::stopCapture() > > mappedBuffers_.clear(); > > > > requests_.clear(); > > + freeQueue_.clear(); > > > > delete allocator_; > >
Hi Paul, Thanks for the patch ! On 12/11/2020 08:24, Laurent Pinchart wrote: > Hi Paul, > > Thank you for the patch. > > On Thu, Nov 12, 2020 at 02:02:43PM +0900, Paul Elder wrote: >> To enable reusing Request objects, we kept a pool of free Requests. This >> pool was not cleared upon stopping capture, however, which caused a >> segfault when switching to another camera. Fix this by clearing the >> Request pool on stopCapture(). >> >> Fixes: c753223ad6b9 ("libcamera, android, cam, gstreamer, qcam, v4l2: >> Reuse Request") >> Signed-off-by: Paul Elder <paul.elder@ideasonboard.com> > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> Tested-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com> > >> --- >> src/qcam/main_window.cpp | 1 + >> 1 file changed, 1 insertion(+) >> >> diff --git a/src/qcam/main_window.cpp b/src/qcam/main_window.cpp >> index f140313e..2502ecd4 100644 >> --- a/src/qcam/main_window.cpp >> +++ b/src/qcam/main_window.cpp >> @@ -579,6 +579,7 @@ void MainWindow::stopCapture() >> mappedBuffers_.clear(); >> >> requests_.clear(); >> + freeQueue_.clear(); >> >> delete allocator_; >> >
Hi Paul, Thanks for your work. On 2020-11-12 14:02:43 +0900, Paul Elder wrote: > To enable reusing Request objects, we kept a pool of free Requests. This > pool was not cleared upon stopping capture, however, which caused a > segfault when switching to another camera. Fix this by clearing the > Request pool on stopCapture(). > > Fixes: c753223ad6b9 ("libcamera, android, cam, gstreamer, qcam, v4l2: > Reuse Request") > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com> As Laurent already pointed out the fixes lines shall not be wrapped, with this fixed, Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > --- > src/qcam/main_window.cpp | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/src/qcam/main_window.cpp b/src/qcam/main_window.cpp > index f140313e..2502ecd4 100644 > --- a/src/qcam/main_window.cpp > +++ b/src/qcam/main_window.cpp > @@ -579,6 +579,7 @@ void MainWindow::stopCapture() > mappedBuffers_.clear(); > > requests_.clear(); > + freeQueue_.clear(); > > delete allocator_; > > -- > 2.27.0 > > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
Hi Paul, On 12/11/2020 05:02, Paul Elder wrote: > To enable reusing Request objects, we kept a pool of free Requests. This > pool was not cleared upon stopping capture, however, which caused a > segfault when switching to another camera. Fix this by clearing the > Request pool on stopCapture(). > > Fixes: c753223ad6b9 ("libcamera, android, cam, gstreamer, qcam, v4l2: > Reuse Request") > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com> Fixes things for me! When this crashed for me, it was hitting code in the Request class, is there anything we need to do to be more defensive about this? Was that just because we were using a Request object which had been free'd or something? Anyway, for this fix : Tested-by: Kieran Bingham <kieran.bingham@ideasonboard.com> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > --- > src/qcam/main_window.cpp | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/src/qcam/main_window.cpp b/src/qcam/main_window.cpp > index f140313e..2502ecd4 100644 > --- a/src/qcam/main_window.cpp > +++ b/src/qcam/main_window.cpp > @@ -579,6 +579,7 @@ void MainWindow::stopCapture() > mappedBuffers_.clear(); > > requests_.clear(); > + freeQueue_.clear(); > > delete allocator_; > >
On Thu, Nov 12, 2020 at 10:40:09AM +0000, Kieran Bingham wrote: > Hi Paul, > > On 12/11/2020 05:02, Paul Elder wrote: > > To enable reusing Request objects, we kept a pool of free Requests. This > > pool was not cleared upon stopping capture, however, which caused a > > segfault when switching to another camera. Fix this by clearing the > > Request pool on stopCapture(). > > > > Fixes: c753223ad6b9 ("libcamera, android, cam, gstreamer, qcam, v4l2: > > Reuse Request") > > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com> > > > Fixes things for me! > > When this crashed for me, it was hitting code in the Request class, is > there anything we need to do to be more defensive about this? > > Was that just because we were using a Request object which had been > free'd or something? Yeah, that's what's happening. processCapture() adds the request (that's prepared to be reused) to the request pool, meanwhile its unique_ptr gets freed at stopCapture() (the request_.clear() below). So of course MainWindow::queueRequest() picks up a freed request from the request pool :) I don't think this is anything that needs to be protected against on libcamera's side. qcam had a request, freed it, and then tried to reuse it. All the code that's involved in this chain is contained completely in qcam. > Anyway, for this fix : > > Tested-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> Thanks, Paul > > --- > > src/qcam/main_window.cpp | 1 + > > 1 file changed, 1 insertion(+) > > > > diff --git a/src/qcam/main_window.cpp b/src/qcam/main_window.cpp > > index f140313e..2502ecd4 100644 > > --- a/src/qcam/main_window.cpp > > +++ b/src/qcam/main_window.cpp > > @@ -579,6 +579,7 @@ void MainWindow::stopCapture() > > mappedBuffers_.clear(); > > > > requests_.clear(); > > + freeQueue_.clear(); > > > > delete allocator_; > > > >
diff --git a/src/qcam/main_window.cpp b/src/qcam/main_window.cpp index f140313e..2502ecd4 100644 --- a/src/qcam/main_window.cpp +++ b/src/qcam/main_window.cpp @@ -579,6 +579,7 @@ void MainWindow::stopCapture() mappedBuffers_.clear(); requests_.clear(); + freeQueue_.clear(); delete allocator_;
To enable reusing Request objects, we kept a pool of free Requests. This pool was not cleared upon stopping capture, however, which caused a segfault when switching to another camera. Fix this by clearing the Request pool on stopCapture(). Fixes: c753223ad6b9 ("libcamera, android, cam, gstreamer, qcam, v4l2: Reuse Request") Signed-off-by: Paul Elder <paul.elder@ideasonboard.com> --- src/qcam/main_window.cpp | 1 + 1 file changed, 1 insertion(+)