| Message ID | 20201006160637.29841-3-jacopo@jmondi.org |
|---|---|
| State | Superseded, archived |
| Delegated to: | Jacopo Mondi |
| Headers | show |
| Series |
|
| Related | show |
Hi Jacopo, On Tue, Oct 06, 2020 at 06:06:37PM +0200, Jacopo Mondi wrote: > Add a CameraWorker class member to the CameraDevice class and > queue capture requests to it to delegate fence handling and capture > requests queueing to the camera. > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > --- > src/android/camera_device.cpp | 21 ++++++++------------- > src/android/camera_device.h | 3 +++ > 2 files changed, 11 insertions(+), 13 deletions(-) > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp > index 8da70e817b46..edac9f28ab67 100644 > --- a/src/android/camera_device.cpp > +++ b/src/android/camera_device.cpp > @@ -194,8 +194,8 @@ CameraDevice::Camera3RequestDescriptor::~Camera3RequestDescriptor() > */ > > CameraDevice::CameraDevice(unsigned int id, const std::shared_ptr<Camera> &camera) > - : id_(id), running_(false), camera_(camera), staticMetadata_(nullptr), > - facing_(CAMERA_FACING_FRONT), orientation_(0) > + : id_(id), worker_(camera), running_(false), camera_(camera), > + staticMetadata_(nullptr), facing_(CAMERA_FACING_FRONT), orientation_(0) > { > camera_->requestCompleted.connect(this, &CameraDevice::requestComplete); > > @@ -1375,8 +1375,9 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques > new Camera3RequestDescriptor(camera3Request->frame_number, > camera3Request->num_output_buffers); > > - Request *request = > - camera_->createRequest(reinterpret_cast<uint64_t>(descriptor)); > + std::unique_ptr<CaptureRequest> captureRequest = > + std::make_unique<CaptureRequest>( > + camera_->createRequest(reinterpret_cast<uint64_t>(descriptor))); > > LOG(HAL, Debug) << "Queueing Request to libcamera with " > << descriptor->numBuffers << " HAL streams"; > @@ -1440,21 +1441,15 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques > > if (!buffer) { > LOG(HAL, Error) << "Failed to create buffer"; > - delete request; You're leaking request :p > delete descriptor; > return -ENOMEM; > } > > - request->addBuffer(cameraStream->stream(), buffer); > + captureRequest->addBuffer(cameraStream->stream(), buffer, > + camera3Buffers[i].acquire_fence); > } > > - int ret = camera_->queueRequest(request); > - if (ret) { > - LOG(HAL, Error) << "Failed to queue request"; > - delete request; > - delete descriptor; > - return ret; > - } > + worker_.queueRequest(std::move(captureRequest)); > > return 0; > } > diff --git a/src/android/camera_device.h b/src/android/camera_device.h > index 777d1a35e801..b4b32f77a29a 100644 > --- a/src/android/camera_device.h > +++ b/src/android/camera_device.h > @@ -25,6 +25,7 @@ > #include "libcamera/internal/message.h" > > #include "camera_stream.h" > +#include "camera_worker.h" > #include "jpeg/encoder.h" > > class CameraMetadata; > @@ -108,6 +109,8 @@ private: > unsigned int id_; > camera3_device_t camera3Device_; > > + CameraWorker worker_; > + > bool running_; > std::shared_ptr<libcamera::Camera> camera_; > std::unique_ptr<libcamera::CameraConfiguration> config_; > -- > 2.28.0 >
Hi Jacopo, Thank you for the patch. On Tue, Oct 06, 2020 at 06:12:17PM +0200, Jacopo Mondi wrote: > On Tue, Oct 06, 2020 at 06:06:37PM +0200, Jacopo Mondi wrote: > > Add a CameraWorker class member to the CameraDevice class and > > queue capture requests to it to delegate fence handling and capture > > requests queueing to the camera. > > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > --- > > src/android/camera_device.cpp | 21 ++++++++------------- > > src/android/camera_device.h | 3 +++ > > 2 files changed, 11 insertions(+), 13 deletions(-) > > > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp > > index 8da70e817b46..edac9f28ab67 100644 > > --- a/src/android/camera_device.cpp > > +++ b/src/android/camera_device.cpp > > @@ -194,8 +194,8 @@ CameraDevice::Camera3RequestDescriptor::~Camera3RequestDescriptor() > > */ > > > > CameraDevice::CameraDevice(unsigned int id, const std::shared_ptr<Camera> &camera) > > - : id_(id), running_(false), camera_(camera), staticMetadata_(nullptr), > > - facing_(CAMERA_FACING_FRONT), orientation_(0) > > + : id_(id), worker_(camera), running_(false), camera_(camera), > > + staticMetadata_(nullptr), facing_(CAMERA_FACING_FRONT), orientation_(0) > > { > > camera_->requestCompleted.connect(this, &CameraDevice::requestComplete); > > > > @@ -1375,8 +1375,9 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques > > new Camera3RequestDescriptor(camera3Request->frame_number, > > camera3Request->num_output_buffers); > > > > - Request *request = > > - camera_->createRequest(reinterpret_cast<uint64_t>(descriptor)); > > + std::unique_ptr<CaptureRequest> captureRequest = > > + std::make_unique<CaptureRequest>( > > + camera_->createRequest(reinterpret_cast<uint64_t>(descriptor))); > > > > LOG(HAL, Debug) << "Queueing Request to libcamera with " > > << descriptor->numBuffers << " HAL streams"; > > @@ -1440,21 +1441,15 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques > > > > if (!buffer) { > > LOG(HAL, Error) << "Failed to create buffer"; > > - delete request; > > You're leaking request :p Apart from that, and the need to start the worker on open and stop it on close (with proper synchronization to make sure nothing gets lost on the message queue), it looks good to me. > > delete descriptor; > > return -ENOMEM; > > } > > > > - request->addBuffer(cameraStream->stream(), buffer); > > + captureRequest->addBuffer(cameraStream->stream(), buffer, > > + camera3Buffers[i].acquire_fence); > > } > > > > - int ret = camera_->queueRequest(request); > > - if (ret) { > > - LOG(HAL, Error) << "Failed to queue request"; > > - delete request; > > - delete descriptor; > > - return ret; > > - } > > + worker_.queueRequest(std::move(captureRequest)); > > > > return 0; > > } > > diff --git a/src/android/camera_device.h b/src/android/camera_device.h > > index 777d1a35e801..b4b32f77a29a 100644 > > --- a/src/android/camera_device.h > > +++ b/src/android/camera_device.h > > @@ -25,6 +25,7 @@ > > #include "libcamera/internal/message.h" > > > > #include "camera_stream.h" > > +#include "camera_worker.h" > > #include "jpeg/encoder.h" > > > > class CameraMetadata; > > @@ -108,6 +109,8 @@ private: > > unsigned int id_; > > camera3_device_t camera3Device_; > > > > + CameraWorker worker_; > > + > > bool running_; > > std::shared_ptr<libcamera::Camera> camera_; > > std::unique_ptr<libcamera::CameraConfiguration> config_;
Hi Laurent, On Thu, Oct 08, 2020 at 07:24:38AM +0300, Laurent Pinchart wrote: > Hi Jacopo, > > Thank you for the patch. > > On Tue, Oct 06, 2020 at 06:12:17PM +0200, Jacopo Mondi wrote: > > On Tue, Oct 06, 2020 at 06:06:37PM +0200, Jacopo Mondi wrote: > > > Add a CameraWorker class member to the CameraDevice class and > > > queue capture requests to it to delegate fence handling and capture > > > requests queueing to the camera. > > > > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > > --- > > > src/android/camera_device.cpp | 21 ++++++++------------- > > > src/android/camera_device.h | 3 +++ > > > 2 files changed, 11 insertions(+), 13 deletions(-) > > > > > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp > > > index 8da70e817b46..edac9f28ab67 100644 > > > --- a/src/android/camera_device.cpp > > > +++ b/src/android/camera_device.cpp > > > @@ -194,8 +194,8 @@ CameraDevice::Camera3RequestDescriptor::~Camera3RequestDescriptor() > > > */ > > > > > > CameraDevice::CameraDevice(unsigned int id, const std::shared_ptr<Camera> &camera) > > > - : id_(id), running_(false), camera_(camera), staticMetadata_(nullptr), > > > - facing_(CAMERA_FACING_FRONT), orientation_(0) > > > + : id_(id), worker_(camera), running_(false), camera_(camera), > > > + staticMetadata_(nullptr), facing_(CAMERA_FACING_FRONT), orientation_(0) > > > { > > > camera_->requestCompleted.connect(this, &CameraDevice::requestComplete); > > > > > > @@ -1375,8 +1375,9 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques > > > new Camera3RequestDescriptor(camera3Request->frame_number, > > > camera3Request->num_output_buffers); > > > > > > - Request *request = > > > - camera_->createRequest(reinterpret_cast<uint64_t>(descriptor)); > > > + std::unique_ptr<CaptureRequest> captureRequest = > > > + std::make_unique<CaptureRequest>( > > > + camera_->createRequest(reinterpret_cast<uint64_t>(descriptor))); > > > > > > LOG(HAL, Debug) << "Queueing Request to libcamera with " > > > << descriptor->numBuffers << " HAL streams"; > > > @@ -1440,21 +1441,15 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques > > > > > > if (!buffer) { > > > LOG(HAL, Error) << "Failed to create buffer"; > > > - delete request; > > > > You're leaking request :p > > Apart from that, and the need to start the worker on open and stop it on > close (with proper synchronization to make sure nothing gets lost on the > message queue), it looks good to me. > depending if Paul's reusable request gets in first, this might become less awful. > > > delete descriptor; > > > return -ENOMEM; > > > } > > > > > > - request->addBuffer(cameraStream->stream(), buffer); > > > + captureRequest->addBuffer(cameraStream->stream(), buffer, > > > + camera3Buffers[i].acquire_fence); > > > } > > > > > > - int ret = camera_->queueRequest(request); > > > - if (ret) { > > > - LOG(HAL, Error) << "Failed to queue request"; > > > - delete request; > > > - delete descriptor; > > > - return ret; > > > - } > > > + worker_.queueRequest(std::move(captureRequest)); > > > > > > return 0; > > > } > > > diff --git a/src/android/camera_device.h b/src/android/camera_device.h > > > index 777d1a35e801..b4b32f77a29a 100644 > > > --- a/src/android/camera_device.h > > > +++ b/src/android/camera_device.h > > > @@ -25,6 +25,7 @@ > > > #include "libcamera/internal/message.h" > > > > > > #include "camera_stream.h" > > > +#include "camera_worker.h" > > > #include "jpeg/encoder.h" > > > > > > class CameraMetadata; > > > @@ -108,6 +109,8 @@ private: > > > unsigned int id_; > > > camera3_device_t camera3Device_; > > > > > > + CameraWorker worker_; > > > + > > > bool running_; > > > std::shared_ptr<libcamera::Camera> camera_; > > > std::unique_ptr<libcamera::CameraConfiguration> config_; > > -- > Regards, > > Laurent Pinchart
diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp index 8da70e817b46..edac9f28ab67 100644 --- a/src/android/camera_device.cpp +++ b/src/android/camera_device.cpp @@ -194,8 +194,8 @@ CameraDevice::Camera3RequestDescriptor::~Camera3RequestDescriptor() */ CameraDevice::CameraDevice(unsigned int id, const std::shared_ptr<Camera> &camera) - : id_(id), running_(false), camera_(camera), staticMetadata_(nullptr), - facing_(CAMERA_FACING_FRONT), orientation_(0) + : id_(id), worker_(camera), running_(false), camera_(camera), + staticMetadata_(nullptr), facing_(CAMERA_FACING_FRONT), orientation_(0) { camera_->requestCompleted.connect(this, &CameraDevice::requestComplete); @@ -1375,8 +1375,9 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques new Camera3RequestDescriptor(camera3Request->frame_number, camera3Request->num_output_buffers); - Request *request = - camera_->createRequest(reinterpret_cast<uint64_t>(descriptor)); + std::unique_ptr<CaptureRequest> captureRequest = + std::make_unique<CaptureRequest>( + camera_->createRequest(reinterpret_cast<uint64_t>(descriptor))); LOG(HAL, Debug) << "Queueing Request to libcamera with " << descriptor->numBuffers << " HAL streams"; @@ -1440,21 +1441,15 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques if (!buffer) { LOG(HAL, Error) << "Failed to create buffer"; - delete request; delete descriptor; return -ENOMEM; } - request->addBuffer(cameraStream->stream(), buffer); + captureRequest->addBuffer(cameraStream->stream(), buffer, + camera3Buffers[i].acquire_fence); } - int ret = camera_->queueRequest(request); - if (ret) { - LOG(HAL, Error) << "Failed to queue request"; - delete request; - delete descriptor; - return ret; - } + worker_.queueRequest(std::move(captureRequest)); return 0; } diff --git a/src/android/camera_device.h b/src/android/camera_device.h index 777d1a35e801..b4b32f77a29a 100644 --- a/src/android/camera_device.h +++ b/src/android/camera_device.h @@ -25,6 +25,7 @@ #include "libcamera/internal/message.h" #include "camera_stream.h" +#include "camera_worker.h" #include "jpeg/encoder.h" class CameraMetadata; @@ -108,6 +109,8 @@ private: unsigned int id_; camera3_device_t camera3Device_; + CameraWorker worker_; + bool running_; std::shared_ptr<libcamera::Camera> camera_; std::unique_ptr<libcamera::CameraConfiguration> config_;
Add a CameraWorker class member to the CameraDevice class and queue capture requests to it to delegate fence handling and capture requests queueing to the camera. Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> --- src/android/camera_device.cpp | 21 ++++++++------------- src/android/camera_device.h | 3 +++ 2 files changed, 11 insertions(+), 13 deletions(-)