[libcamera-devel] [PATCH v6] android: CameraDevice: Fix Camera3RequestDescriptor leakage
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Tue Apr 20 05:11:21 CEST 2021
Hi Hiro,
On Wed, Apr 07, 2021 at 11:48:05AM +0900, Hirokazu Honda wrote:
> Laurent, can you push this and stop() patch to tree?
I'm sorry, those two patches have fallen through the crack (they were
still visible in patchwork though, so I suppose they haven't fallen
completely and would have resurfaced, I'm happy Kieran kept pushing to
have a patchwork instance and got it up and running).
Anyway, patches pushed. Thanks for reminding me.
> On Tue, Apr 6, 2021 at 9:30 PM Jacopo Mondi <jacopo at jmondi.org> wrote:
> > On Sat, Apr 03, 2021 at 10:57:34PM +0900, Hirokazu Honda wrote:
> > > CameraDevice creates Camera3RequestDescriptor in
> > > processCaptureRequest() and disallocates in requestComplete().
> > > Camera3RequestDescriptor can never be destroyed if
> > > requestComplete() is never called. This avoid the memory
> > > leakage by storing them in map CameraRequestDescriptor.
> > >
> > > Signed-off-by: Hirokazu Honda <hiroh at chromium.org>
> > > Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> > > ---
> > > src/android/camera_device.cpp | 80 ++++++++++++++++++++---------------
> > > src/android/camera_device.h | 10 ++++-
> > > src/android/camera_worker.cpp | 4 +-
> > > src/android/camera_worker.h | 2 +-
> > > 4 files changed, 57 insertions(+), 39 deletions(-)
> > >
> > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> > > index b45d3a54..9d6be1f9 100644
> > > --- a/src/android/camera_device.cpp
> > > +++ b/src/android/camera_device.cpp
> > > @@ -286,16 +286,12 @@ CameraDevice::Camera3RequestDescriptor::Camera3RequestDescriptor(
> > > settings_ = CameraMetadata(camera3Request->settings);
> > >
> > > /*
> > > - * Create the libcamera::Request unique_ptr<> to tie its lifetime
> > > - * to the descriptor's one. Set the descriptor's address as the
> > > - * request's cookie to retrieve it at completion time.
> > > + * Create the libcamera::CaptureRequest unique_ptr<> to tie its lifetime
> > > + * to the descriptor's one.
> > > */
> > > - request_ = std::make_unique<CaptureRequest>(camera,
> > > - reinterpret_cast<uint64_t>(this));
> > > + request_ = std::make_unique<CaptureRequest>(camera);
> > > }
> > >
> > > -CameraDevice::Camera3RequestDescriptor::~Camera3RequestDescriptor() = default;
> > > -
> > > /*
> > > * \class CameraDevice
> > > *
> > > @@ -672,6 +668,7 @@ void CameraDevice::stop()
> >
> > I have a bit lost track of where stop() is called, as this patch is
> > based on a pile of patches in review. My only concern is that
> > descriptor_ shall be cleared also at configureStreams() time.
> >
> > With that confirmed
> > Reviewed-by: Jacopo Mondi <jacopo at jmondi.org>
> >
> > > worker_.stop();
> > > camera_->stop();
> > >
> > > + descriptors_.clear();
> > > running_ = false;
> > > }
> > >
> > > @@ -1818,8 +1815,8 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques
> > > * The descriptor and the associated memory reserved here are freed
> > > * at request complete time.
> > > */
> > > - Camera3RequestDescriptor *descriptor =
> > > - new Camera3RequestDescriptor(camera_.get(), camera3Request);
> > > + Camera3RequestDescriptor descriptor(camera_.get(), camera3Request);
> > > +
> > > /*
> > > * \todo The Android request model is incremental, settings passed in
> > > * previous requests are to be effective until overridden explicitly in
> > > @@ -1829,12 +1826,12 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques
> > > if (camera3Request->settings)
> > > lastSettings_ = camera3Request->settings;
> > > else
> > > - descriptor->settings_ = lastSettings_;
> > > + descriptor.settings_ = lastSettings_;
> > >
> > > - LOG(HAL, Debug) << "Queueing request " << descriptor->request_->cookie()
> > > - << " with " << descriptor->buffers_.size() << " streams";
> > > - for (unsigned int i = 0; i < descriptor->buffers_.size(); ++i) {
> > > - const camera3_stream_buffer_t &camera3Buffer = descriptor->buffers_[i];
> > > + LOG(HAL, Debug) << "Queueing request " << descriptor.request_->cookie()
> > > + << " with " << descriptor.buffers_.size() << " streams";
> > > + for (unsigned int i = 0; i < descriptor.buffers_.size(); ++i) {
> > > + const camera3_stream_buffer_t &camera3Buffer = descriptor.buffers_[i];
> > > camera3_stream *camera3Stream = camera3Buffer.stream;
> > > CameraStream *cameraStream = static_cast<CameraStream *>(camera3Stream->priv);
> > >
> > > @@ -1867,7 +1864,7 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques
> > > * lifetime management only.
> > > */
> > > buffer = createFrameBuffer(*camera3Buffer.buffer);
> > > - descriptor->frameBuffers_.emplace_back(buffer);
> > > + descriptor.frameBuffers_.emplace_back(buffer);
> > > LOG(HAL, Debug) << ss.str() << " (direct)";
> > > break;
> > >
> > > @@ -1886,11 +1883,10 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques
> > >
> > > if (!buffer) {
> > > LOG(HAL, Error) << "Failed to create buffer";
> > > - delete descriptor;
> > > return -ENOMEM;
> > > }
> > >
> > > - descriptor->request_->addBuffer(cameraStream->stream(), buffer,
> > > + descriptor.request_->addBuffer(cameraStream->stream(), buffer,
> > > camera3Buffer.acquire_fence);
> > > }
> > >
> > > @@ -1898,11 +1894,16 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques
> > > * Translate controls from Android to libcamera and queue the request
> > > * to the CameraWorker thread.
> > > */
> > > - int ret = processControls(descriptor);
> > > + int ret = processControls(&descriptor);
> > > if (ret)
> > > return ret;
> > >
> > > - worker_.queueRequest(descriptor->request_.get());
> > > + worker_.queueRequest(descriptor.request_.get());
> > > +
> > > + {
> > > + std::scoped_lock<std::mutex> lock(mutex_);
> > > + descriptors_[descriptor.request_->cookie()] = std::move(descriptor);
> > > + }
> > >
> > > return 0;
> > > }
> > > @@ -1912,8 +1913,21 @@ void CameraDevice::requestComplete(Request *request)
> > > const Request::BufferMap &buffers = request->buffers();
> > > camera3_buffer_status status = CAMERA3_BUFFER_STATUS_OK;
> > > std::unique_ptr<CameraMetadata> resultMetadata;
> > > - Camera3RequestDescriptor *descriptor =
> > > - reinterpret_cast<Camera3RequestDescriptor *>(request->cookie());
> > > +
> > > + decltype(descriptors_)::node_type node;
> > > + {
> > > + std::scoped_lock<std::mutex> lock(mutex_);
> > > + auto it = descriptors_.find(request->cookie());
> > > + if (it == descriptors_.end()) {
> > > + LOG(HAL, Fatal)
> > > + << "Unknown request: " << request->cookie();
> > > + status = CAMERA3_BUFFER_STATUS_ERROR;
> > > + return;
> > > + }
> > > +
> > > + node = descriptors_.extract(it);
> > > + }
> > > + Camera3RequestDescriptor &descriptor = node.mapped();
> > >
> > > if (request->status() != Request::RequestComplete) {
> > > LOG(HAL, Error) << "Request not successfully completed: "
> > > @@ -1922,7 +1936,7 @@ void CameraDevice::requestComplete(Request *request)
> > > }
> > >
> > > LOG(HAL, Debug) << "Request " << request->cookie() << " completed with "
> > > - << descriptor->buffers_.size() << " streams";
> > > + << descriptor.buffers_.size() << " streams";
> > >
> > > /*
> > > * \todo The timestamp used for the metadata is currently always taken
> > > @@ -1931,10 +1945,10 @@ void CameraDevice::requestComplete(Request *request)
> > > * pipeline handlers) timestamp in the Request itself.
> > > */
> > > uint64_t timestamp = buffers.begin()->second->metadata().timestamp;
> > > - resultMetadata = getResultMetadata(*descriptor, timestamp);
> > > + resultMetadata = getResultMetadata(descriptor, timestamp);
> > >
> > > /* Handle any JPEG compression. */
> > > - for (camera3_stream_buffer_t &buffer : descriptor->buffers_) {
> > > + for (camera3_stream_buffer_t &buffer : descriptor.buffers_) {
> > > CameraStream *cameraStream =
> > > static_cast<CameraStream *>(buffer.stream->priv);
> > >
> > > @@ -1949,7 +1963,7 @@ void CameraDevice::requestComplete(Request *request)
> > >
> > > int ret = cameraStream->process(*src,
> > > *buffer.buffer,
> > > - descriptor->settings_,
> > > + descriptor.settings_,
> > > resultMetadata.get());
> > > if (ret) {
> > > status = CAMERA3_BUFFER_STATUS_ERROR;
> > > @@ -1966,17 +1980,17 @@ void CameraDevice::requestComplete(Request *request)
> > >
> > > /* Prepare to call back the Android camera stack. */
> > > camera3_capture_result_t captureResult = {};
> > > - captureResult.frame_number = descriptor->frameNumber_;
> > > - captureResult.num_output_buffers = descriptor->buffers_.size();
> > > - for (camera3_stream_buffer_t &buffer : descriptor->buffers_) {
> > > + captureResult.frame_number = descriptor.frameNumber_;
> > > + captureResult.num_output_buffers = descriptor.buffers_.size();
> > > + for (camera3_stream_buffer_t &buffer : descriptor.buffers_) {
> > > buffer.acquire_fence = -1;
> > > buffer.release_fence = -1;
> > > buffer.status = status;
> > > }
> > > - captureResult.output_buffers = descriptor->buffers_.data();
> > > + captureResult.output_buffers = descriptor.buffers_.data();
> > >
> > > if (status == CAMERA3_BUFFER_STATUS_OK) {
> > > - notifyShutter(descriptor->frameNumber_, timestamp);
> > > + notifyShutter(descriptor.frameNumber_, timestamp);
> > >
> > > captureResult.partial_result = 1;
> > > captureResult.result = resultMetadata->get();
> > > @@ -1989,13 +2003,11 @@ void CameraDevice::requestComplete(Request *request)
> > > * is here signalled. Make sure the error path plays well with
> > > * the camera stack state machine.
> > > */
> > > - notifyError(descriptor->frameNumber_,
> > > - descriptor->buffers_[0].stream);
> > > + notifyError(descriptor.frameNumber_,
> > > + descriptor.buffers_[0].stream);
> > > }
> > >
> > > callbacks_->process_capture_result(callbacks_, &captureResult);
> > > -
> > > - delete descriptor;
> > > }
> > >
> > > std::string CameraDevice::logPrefix() const
> > > diff --git a/src/android/camera_device.h b/src/android/camera_device.h
> > > index 39cf95ad..c63e8e21 100644
> > > --- a/src/android/camera_device.h
> > > +++ b/src/android/camera_device.h
> > > @@ -9,6 +9,7 @@
> > >
> > > #include <map>
> > > #include <memory>
> > > +#include <mutex>
> > > #include <tuple>
> > > #include <vector>
> > >
> > > @@ -69,11 +70,13 @@ private:
> > > CameraDevice(unsigned int id, std::shared_ptr<libcamera::Camera> camera);
> > >
> > > struct Camera3RequestDescriptor {
> > > + Camera3RequestDescriptor() = default;
> > > + ~Camera3RequestDescriptor() = default;
> > > Camera3RequestDescriptor(libcamera::Camera *camera,
> > > const camera3_capture_request_t *camera3Request);
> > > - ~Camera3RequestDescriptor();
> > > + Camera3RequestDescriptor &operator=(Camera3RequestDescriptor &&) = default;
> > >
> > > - uint32_t frameNumber_;
> > > + uint32_t frameNumber_ = 0;
> > > std::vector<camera3_stream_buffer_t> buffers_;
> > > std::vector<std::unique_ptr<libcamera::FrameBuffer>> frameBuffers_;
> > > CameraMetadata settings_;
> > > @@ -124,6 +127,9 @@ private:
> > > std::map<int, libcamera::PixelFormat> formatsMap_;
> > > std::vector<CameraStream> streams_;
> > >
> > > + std::mutex mutex_; /* Protect descriptors_ */
> > > + std::map<uint64_t, Camera3RequestDescriptor> descriptors_;
> > > +
> > > std::string maker_;
> > > std::string model_;
> > >
> > > diff --git a/src/android/camera_worker.cpp b/src/android/camera_worker.cpp
> > > index df436460..300ddde0 100644
> > > --- a/src/android/camera_worker.cpp
> > > +++ b/src/android/camera_worker.cpp
> > > @@ -26,10 +26,10 @@ LOG_DECLARE_CATEGORY(HAL)
> > > * by the CameraWorker which queues it to the libcamera::Camera after handling
> > > * fences.
> > > */
> > > -CaptureRequest::CaptureRequest(libcamera::Camera *camera, uint64_t cookie)
> > > +CaptureRequest::CaptureRequest(libcamera::Camera *camera)
> > > : camera_(camera)
> > > {
> > > - request_ = camera_->createRequest(cookie);
> > > + request_ = camera_->createRequest(reinterpret_cast<uint64_t>(this));
> > > }
> > >
> > > void CaptureRequest::addBuffer(Stream *stream, FrameBuffer *buffer, int fence)
> > > diff --git a/src/android/camera_worker.h b/src/android/camera_worker.h
> > > index d7060576..64b1658b 100644
> > > --- a/src/android/camera_worker.h
> > > +++ b/src/android/camera_worker.h
> > > @@ -22,7 +22,7 @@ class CameraDevice;
> > > class CaptureRequest
> > > {
> > > public:
> > > - CaptureRequest(libcamera::Camera *camera, uint64_t cookie);
> > > + CaptureRequest(libcamera::Camera *camera);
> > >
> > > const std::vector<int> &fences() const { return acquireFences_; }
> > > libcamera::ControlList &controls() { return request_->controls(); }
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list