| Message ID | 20210831080528.835236-1-hiroh@chromium.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Hiro, The commit message can be somewhat more contextual of the problem. On 8/31/21 1:35 PM, Hirokazu Honda wrote: > This fixes the crash when post processing is needed and some capture > request is being processed by PipelineHandler upon calling > CameraDevice::close(). > The crash happens in Request::completeBuffer(), which is invoked by > Camera::stop(). It uses FrameBuffer after free. The FrameBuffer is > allocated and owned by FrameBufferAllocator, and FrameBufferAllocator > is owned by CamerStream. So Camera::stop() should be executed before > destroying CameraStream. One question: I can see the problem but what's exactly is crashing in Request::completeBuffer() is something we should know? Is it the ASSERT? That might help us to define the problem statement better. The problem is happening because we seem to add a CameraStream associated buffer(depending on the CameraStream::Type) to the Request, in CameraDevice::processCaptureRequest(). However, when the camera stops, all the current buffers are marked with FrameMetadata::FrameCancelled and proceed to completion. But the buffer associated with the CameraStream (that was previously added to the request) has now been cleared out with a part of streams_.clear(), even before the camera stop() has been invoked. Any access to those request buffers after they have been cleared, shall result in a crash. > > Signed-off-by: Hirokazu Honda <hiroh@chromium.org> > --- > src/android/camera_device.cpp | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp > index 8ca76719..74a95a2a 100644 > --- a/src/android/camera_device.cpp > +++ b/src/android/camera_device.cpp > @@ -423,10 +423,10 @@ int CameraDevice::open(const hw_module_t *hardwareModule) > > void CameraDevice::close() > { > - streams_.clear(); > - > stop(); > > + streams_.clear(); How about this being a part of a CameraDevice::stop() instead ? > + > camera_->release(); > } >
Hi Umang, thank you for reviewing. On Tue, Aug 31, 2021 at 9:02 PM Umang Jain <umang.jain@ideasonboard.com> wrote: > > Hi Hiro, > > The commit message can be somewhat more contextual of the problem. > > On 8/31/21 1:35 PM, Hirokazu Honda wrote: > > This fixes the crash when post processing is needed and some capture > > request is being processed by PipelineHandler upon calling > > CameraDevice::close(). > > The crash happens in Request::completeBuffer(), which is invoked by > > Camera::stop(). It uses FrameBuffer after free. The FrameBuffer is > > allocated and owned by FrameBufferAllocator, and FrameBufferAllocator > > is owned by CamerStream. So Camera::stop() should be executed before > > destroying CameraStream. > > > One question: I can see the problem but what's exactly is crashing in > Request::completeBuffer() is something we should know? Is it the ASSERT? > That might help us to define the problem statement better. > It is buffer->_d()->setRequest(nullptr) in https://git.linuxtv.org/libcamera.git/tree/src/libcamera/request.cpp#n339 The buffer has been deallocated. > The problem is happening because we seem to add a CameraStream > associated buffer(depending on the CameraStream::Type) to the Request, > in CameraDevice::processCaptureRequest(). > > However, when the camera stops, all the current buffers are marked with > FrameMetadata::FrameCancelled and proceed to completion. But the buffer > associated with the CameraStream (that was previously added to the > request) has now been cleared out with a part of streams_.clear(), even > before the camera stop() has been invoked. Any access to those request > buffers after they have been cleared, shall result in a crash. > Thanks. I replaced my commit message with this. -Hiro > > > > > Signed-off-by: Hirokazu Honda <hiroh@chromium.org> > > --- > > src/android/camera_device.cpp | 4 ++-- > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp > > index 8ca76719..74a95a2a 100644 > > --- a/src/android/camera_device.cpp > > +++ b/src/android/camera_device.cpp > > @@ -423,10 +423,10 @@ int CameraDevice::open(const hw_module_t *hardwareModule) > > > > void CameraDevice::close() > > { > > - streams_.clear(); > > - > > stop(); > > > > + streams_.clear(); > > > How about this being a part of a CameraDevice::stop() instead ? > > > + > > camera_->release(); > > } > >
diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp index 8ca76719..74a95a2a 100644 --- a/src/android/camera_device.cpp +++ b/src/android/camera_device.cpp @@ -423,10 +423,10 @@ int CameraDevice::open(const hw_module_t *hardwareModule) void CameraDevice::close() { - streams_.clear(); - stop(); + streams_.clear(); + camera_->release(); }
This fixes the crash when post processing is needed and some capture request is being processed by PipelineHandler upon calling CameraDevice::close(). The crash happens in Request::completeBuffer(), which is invoked by Camera::stop(). It uses FrameBuffer after free. The FrameBuffer is allocated and owned by FrameBufferAllocator, and FrameBufferAllocator is owned by CamerStream. So Camera::stop() should be executed before destroying CameraStream. Signed-off-by: Hirokazu Honda <hiroh@chromium.org> --- src/android/camera_device.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)