Message ID | 20201010095830.134084-3-jacopo@jmondi.org |
---|---|
State | Accepted |
Headers | show |
Series |
|
Related | show |
Hi Jacopo, Thank you for the patch. On Sat, Oct 10, 2020 at 11:58:29AM +0200, Jacopo Mondi wrote: > Add a CameraWorker class member to the CameraDevice class and > queue capture requests to it to delegate its handling. Start and > stop the CameraWorker when the libcamera::Camera is started or > stopped. > > Tie the CaptureRequest lifetime to the Camera3RequestDescriptor's one > by storing it as unique_ptr<> in the descriptor to simplify handling > of request creation and deletion. > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > --- > src/android/camera_device.cpp | 42 ++++++++++++++++++----------------- > src/android/camera_device.h | 7 +++++- > 2 files changed, 28 insertions(+), 21 deletions(-) > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp > index c29fcb43fe2d..0036f1140560 100644 > --- a/src/android/camera_device.cpp > +++ b/src/android/camera_device.cpp > @@ -168,11 +168,24 @@ MappedCamera3Buffer::MappedCamera3Buffer(const buffer_handle_t camera3buffer, > */ > > CameraDevice::Camera3RequestDescriptor::Camera3RequestDescriptor( A candidate for later, renaming CameraDevice::Camera3RequestDescriptor to something shorter :-) Maybe CameraDevice::RequestDescriptor, or even CameraDevice::Request ? > - unsigned int frameNumber, unsigned int numBuffers) > + Camera *camera, unsigned int frameNumber, unsigned int numBuffers) > : frameNumber(frameNumber), numBuffers(numBuffers) > { > buffers = new camera3_stream_buffer_t[numBuffers]; > + > + /* > + * FrameBuffer instances created by wrapping a camera3 provided dmabuf > + * are emplaced in this vector of unique_ptr<> for lifetime management. > + */ > frameBuffers.reserve(numBuffers); > + > + /* > + * 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. > + */ > + request = std::make_unique<CaptureRequest>(camera, > + reinterpret_cast<uint64_t>(this)); > } > > CameraDevice::Camera3RequestDescriptor::~Camera3RequestDescriptor() > @@ -519,6 +532,7 @@ void CameraDevice::close() > { > streams_.clear(); > > + worker_.stop(); > camera_->stop(); > camera_->release(); > > @@ -1350,6 +1364,8 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques > > /* Start the camera if that's the first request we handle. */ > if (!running_) { > + worker_.start(); > + > int ret = camera_->start(); > if (ret) { > LOG(HAL, Error) << "Failed to start camera"; > @@ -1372,16 +1388,9 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques > * at request complete time. > */ > Camera3RequestDescriptor *descriptor = > - new Camera3RequestDescriptor(camera3Request->frame_number, > + new Camera3RequestDescriptor(camera_.get(), camera3Request->frame_number, > camera3Request->num_output_buffers); > > - std::unique_ptr<Request> request = > - camera_->createRequest(reinterpret_cast<uint64_t>(descriptor)); > - if (!request) { > - LOG(HAL, Error) << "Failed to create request"; > - return -ENOMEM; > - } > - > LOG(HAL, Debug) << "Queueing Request to libcamera with " > << descriptor->numBuffers << " HAL streams"; > for (unsigned int i = 0; i < descriptor->numBuffers; ++i) { > @@ -1448,18 +1457,12 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques > return -ENOMEM; > } > > - request->addBuffer(cameraStream->stream(), buffer); > - } > - > - int ret = camera_->queueRequest(request.get()); > - if (ret) { > - LOG(HAL, Error) << "Failed to queue request"; > - delete descriptor; > - return ret; > + descriptor->request->addBuffer(cameraStream->stream(), buffer, > + camera3Buffers[i].acquire_fence); > } > > - /* The request will be deleted in the completion handler. */ > - request.release(); > + /* Queue the request on the CameraWorker. */ s/on the/to the/ > + worker_.queueRequest(descriptor->request.get()); > > return 0; > } > @@ -1564,7 +1567,6 @@ void CameraDevice::requestComplete(Request *request) > callbacks_->process_capture_result(callbacks_, &captureResult); > > delete descriptor; > - delete request; > } > > std::string CameraDevice::logPrefix() const > diff --git a/src/android/camera_device.h b/src/android/camera_device.h > index 777d1a35e801..86f2b8974b53 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; > @@ -73,7 +74,8 @@ private: > CameraDevice(unsigned int id, const std::shared_ptr<libcamera::Camera> &camera); > > struct Camera3RequestDescriptor { > - Camera3RequestDescriptor(unsigned int frameNumber, > + Camera3RequestDescriptor(libcamera::Camera *camera, > + unsigned int frameNumber, > unsigned int numBuffers); > ~Camera3RequestDescriptor(); > > @@ -81,6 +83,7 @@ private: > uint32_t numBuffers; > camera3_stream_buffer_t *buffers; > std::vector<std::unique_ptr<libcamera::FrameBuffer>> frameBuffers; > + std::unique_ptr<CaptureRequest> request; You could actually embed this. It may even make sense to merge the two structures together as I don't think we need two separate structures. Up to you, it can also be handled later. Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> I like the simplification :-) > }; > > struct Camera3StreamConfiguration { > @@ -108,6 +111,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 Jacopo, Nice work. On 2020-10-10 11:58:29 +0200, Jacopo Mondi wrote: > Add a CameraWorker class member to the CameraDevice class and > queue capture requests to it to delegate its handling. Start and > stop the CameraWorker when the libcamera::Camera is started or > stopped. > > Tie the CaptureRequest lifetime to the Camera3RequestDescriptor's one > by storing it as unique_ptr<> in the descriptor to simplify handling > of request creation and deletion. > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > --- > src/android/camera_device.cpp | 42 ++++++++++++++++++----------------- > src/android/camera_device.h | 7 +++++- > 2 files changed, 28 insertions(+), 21 deletions(-) > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp > index c29fcb43fe2d..0036f1140560 100644 > --- a/src/android/camera_device.cpp > +++ b/src/android/camera_device.cpp > @@ -168,11 +168,24 @@ MappedCamera3Buffer::MappedCamera3Buffer(const buffer_handle_t camera3buffer, > */ > > CameraDevice::Camera3RequestDescriptor::Camera3RequestDescriptor( > - unsigned int frameNumber, unsigned int numBuffers) > + Camera *camera, unsigned int frameNumber, unsigned int numBuffers) > : frameNumber(frameNumber), numBuffers(numBuffers) > { > buffers = new camera3_stream_buffer_t[numBuffers]; > + > + /* > + * FrameBuffer instances created by wrapping a camera3 provided dmabuf > + * are emplaced in this vector of unique_ptr<> for lifetime management. > + */ > frameBuffers.reserve(numBuffers); > + > + /* > + * 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. > + */ > + request = std::make_unique<CaptureRequest>(camera, > + reinterpret_cast<uint64_t>(this)); > } > > CameraDevice::Camera3RequestDescriptor::~Camera3RequestDescriptor() > @@ -519,6 +532,7 @@ void CameraDevice::close() > { > streams_.clear(); > > + worker_.stop(); > camera_->stop(); > camera_->release(); > > @@ -1350,6 +1364,8 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques > > /* Start the camera if that's the first request we handle. */ > if (!running_) { > + worker_.start(); > + > int ret = camera_->start(); > if (ret) { > LOG(HAL, Error) << "Failed to start camera"; > @@ -1372,16 +1388,9 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques > * at request complete time. > */ > Camera3RequestDescriptor *descriptor = > - new Camera3RequestDescriptor(camera3Request->frame_number, > + new Camera3RequestDescriptor(camera_.get(), camera3Request->frame_number, > camera3Request->num_output_buffers); > > - std::unique_ptr<Request> request = > - camera_->createRequest(reinterpret_cast<uint64_t>(descriptor)); > - if (!request) { > - LOG(HAL, Error) << "Failed to create request"; > - return -ENOMEM; > - } > - > LOG(HAL, Debug) << "Queueing Request to libcamera with " > << descriptor->numBuffers << " HAL streams"; > for (unsigned int i = 0; i < descriptor->numBuffers; ++i) { > @@ -1448,18 +1457,12 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques > return -ENOMEM; > } > > - request->addBuffer(cameraStream->stream(), buffer); > - } > - > - int ret = camera_->queueRequest(request.get()); > - if (ret) { > - LOG(HAL, Error) << "Failed to queue request"; > - delete descriptor; > - return ret; > + descriptor->request->addBuffer(cameraStream->stream(), buffer, > + camera3Buffers[i].acquire_fence); > } > > - /* The request will be deleted in the completion handler. */ > - request.release(); > + /* Queue the request on the CameraWorker. */ > + worker_.queueRequest(descriptor->request.get()); > > return 0; > } > @@ -1564,7 +1567,6 @@ void CameraDevice::requestComplete(Request *request) > callbacks_->process_capture_result(callbacks_, &captureResult); > > delete descriptor; > - delete request; > } > > std::string CameraDevice::logPrefix() const > diff --git a/src/android/camera_device.h b/src/android/camera_device.h > index 777d1a35e801..86f2b8974b53 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; > @@ -73,7 +74,8 @@ private: > CameraDevice(unsigned int id, const std::shared_ptr<libcamera::Camera> &camera); > > struct Camera3RequestDescriptor { > - Camera3RequestDescriptor(unsigned int frameNumber, > + Camera3RequestDescriptor(libcamera::Camera *camera, > + unsigned int frameNumber, > unsigned int numBuffers); > ~Camera3RequestDescriptor(); > > @@ -81,6 +83,7 @@ private: > uint32_t numBuffers; > camera3_stream_buffer_t *buffers; > std::vector<std::unique_ptr<libcamera::FrameBuffer>> frameBuffers; > + std::unique_ptr<CaptureRequest> request; > }; > > struct Camera3StreamConfiguration { > @@ -108,6 +111,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 > > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp index c29fcb43fe2d..0036f1140560 100644 --- a/src/android/camera_device.cpp +++ b/src/android/camera_device.cpp @@ -168,11 +168,24 @@ MappedCamera3Buffer::MappedCamera3Buffer(const buffer_handle_t camera3buffer, */ CameraDevice::Camera3RequestDescriptor::Camera3RequestDescriptor( - unsigned int frameNumber, unsigned int numBuffers) + Camera *camera, unsigned int frameNumber, unsigned int numBuffers) : frameNumber(frameNumber), numBuffers(numBuffers) { buffers = new camera3_stream_buffer_t[numBuffers]; + + /* + * FrameBuffer instances created by wrapping a camera3 provided dmabuf + * are emplaced in this vector of unique_ptr<> for lifetime management. + */ frameBuffers.reserve(numBuffers); + + /* + * 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. + */ + request = std::make_unique<CaptureRequest>(camera, + reinterpret_cast<uint64_t>(this)); } CameraDevice::Camera3RequestDescriptor::~Camera3RequestDescriptor() @@ -519,6 +532,7 @@ void CameraDevice::close() { streams_.clear(); + worker_.stop(); camera_->stop(); camera_->release(); @@ -1350,6 +1364,8 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques /* Start the camera if that's the first request we handle. */ if (!running_) { + worker_.start(); + int ret = camera_->start(); if (ret) { LOG(HAL, Error) << "Failed to start camera"; @@ -1372,16 +1388,9 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques * at request complete time. */ Camera3RequestDescriptor *descriptor = - new Camera3RequestDescriptor(camera3Request->frame_number, + new Camera3RequestDescriptor(camera_.get(), camera3Request->frame_number, camera3Request->num_output_buffers); - std::unique_ptr<Request> request = - camera_->createRequest(reinterpret_cast<uint64_t>(descriptor)); - if (!request) { - LOG(HAL, Error) << "Failed to create request"; - return -ENOMEM; - } - LOG(HAL, Debug) << "Queueing Request to libcamera with " << descriptor->numBuffers << " HAL streams"; for (unsigned int i = 0; i < descriptor->numBuffers; ++i) { @@ -1448,18 +1457,12 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques return -ENOMEM; } - request->addBuffer(cameraStream->stream(), buffer); - } - - int ret = camera_->queueRequest(request.get()); - if (ret) { - LOG(HAL, Error) << "Failed to queue request"; - delete descriptor; - return ret; + descriptor->request->addBuffer(cameraStream->stream(), buffer, + camera3Buffers[i].acquire_fence); } - /* The request will be deleted in the completion handler. */ - request.release(); + /* Queue the request on the CameraWorker. */ + worker_.queueRequest(descriptor->request.get()); return 0; } @@ -1564,7 +1567,6 @@ void CameraDevice::requestComplete(Request *request) callbacks_->process_capture_result(callbacks_, &captureResult); delete descriptor; - delete request; } std::string CameraDevice::logPrefix() const diff --git a/src/android/camera_device.h b/src/android/camera_device.h index 777d1a35e801..86f2b8974b53 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; @@ -73,7 +74,8 @@ private: CameraDevice(unsigned int id, const std::shared_ptr<libcamera::Camera> &camera); struct Camera3RequestDescriptor { - Camera3RequestDescriptor(unsigned int frameNumber, + Camera3RequestDescriptor(libcamera::Camera *camera, + unsigned int frameNumber, unsigned int numBuffers); ~Camera3RequestDescriptor(); @@ -81,6 +83,7 @@ private: uint32_t numBuffers; camera3_stream_buffer_t *buffers; std::vector<std::unique_ptr<libcamera::FrameBuffer>> frameBuffers; + std::unique_ptr<CaptureRequest> request; }; struct Camera3StreamConfiguration { @@ -108,6 +111,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 its handling. Start and stop the CameraWorker when the libcamera::Camera is started or stopped. Tie the CaptureRequest lifetime to the Camera3RequestDescriptor's one by storing it as unique_ptr<> in the descriptor to simplify handling of request creation and deletion. Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> --- src/android/camera_device.cpp | 42 ++++++++++++++++++----------------- src/android/camera_device.h | 7 +++++- 2 files changed, 28 insertions(+), 21 deletions(-)