[libcamera-devel] [PATCH 05/11] android: camera_device: Create struct to track per stream buffer
Jacopo Mondi
jacopo at jmondi.org
Mon Oct 18 18:22:28 CEST 2021
Hi Umang,
On Mon, Oct 18, 2021 at 06:59:17PM +0530, Umang Jain wrote:
> The Camera3RequestDescriptor structure stores, for each stream, the
> camera3_stream_buffer_t and the libcamera FrameBuffer in two separate
> vectors. This complicates buffer handling, as the code needs to keep
> both vectors in sync. Create a new structure to group all data about
> per-stream buffers to simplify this.
>
> As a side effect, we need to create a local vector of
> camera3_stream_buffer_t in CameraDevice::sendCaptureResults() as the
> camera3_stream_buffer_t instances stored in the new structure in
> Camera3RequestDescriptor are not contiguous anymore. This is a small
> price to pay for easier handling of buffers, and will be refactored in
> subsequent commits anyway.
>
> Signed-off-by: Umang Jain <umang.jain at ideasonboard.com>
> Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> ---
> src/android/camera_device.cpp | 75 ++++++++++++++++++----------------
> src/android/camera_request.cpp | 9 +---
> src/android/camera_request.h | 15 ++++++-
> 3 files changed, 55 insertions(+), 44 deletions(-)
>
> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> index 3bddb292..59557358 100644
> --- a/src/android/camera_device.cpp
> +++ b/src/android/camera_device.cpp
> @@ -831,9 +831,9 @@ void CameraDevice::abortRequest(Camera3RequestDescriptor *descriptor) const
> notifyError(descriptor->frameNumber_, nullptr, CAMERA3_MSG_ERROR_REQUEST);
>
> for (auto &buffer : descriptor->buffers_) {
> - buffer.release_fence = buffer.acquire_fence;
> - buffer.acquire_fence = -1;
> - buffer.status = CAMERA3_BUFFER_STATUS_ERROR;
> + buffer.buffer.release_fence = buffer.buffer.acquire_fence;
> + buffer.buffer.acquire_fence = -1;
> + buffer.buffer.status = CAMERA3_BUFFER_STATUS_ERROR;
> }
>
> descriptor->status_ = Camera3RequestDescriptor::Status::Error;
> @@ -931,8 +931,8 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques
> LOG(HAL, Debug) << "Queueing request " << descriptor->request_->cookie()
> << " with " << descriptor->buffers_.size() << " streams";
>
> - for (const auto &[i, camera3Buffer] : utils::enumerate(descriptor->buffers_)) {
> - camera3_stream *camera3Stream = camera3Buffer.stream;
> + for (const auto &[i, buffer] : utils::enumerate(descriptor->buffers_)) {
> + camera3_stream *camera3Stream = buffer.buffer.stream;
> CameraStream *cameraStream = static_cast<CameraStream *>(camera3Stream->priv);
>
> std::stringstream ss;
> @@ -949,7 +949,7 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques
> * while fences for streams of type Internal and Mapped are
> * handled at post-processing time.
> */
> - FrameBuffer *buffer = nullptr;
> + FrameBuffer *frameBuffer = nullptr;
> int acquireFence = -1;
> switch (cameraStream->type()) {
> case CameraStream::Type::Mapped:
> @@ -967,13 +967,12 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques
> * associate it with the Camera3RequestDescriptor for
> * lifetime management only.
> */
> - descriptor->frameBuffers_.push_back(
> - createFrameBuffer(*camera3Buffer.buffer,
> + buffer.frameBuffer =
> + createFrameBuffer(*buffer.buffer.buffer,
> cameraStream->configuration().pixelFormat,
> - cameraStream->configuration().size));
> -
> - buffer = descriptor->frameBuffers_.back().get();
> - acquireFence = camera3Buffer.acquire_fence;
> + cameraStream->configuration().size);
> + frameBuffer = buffer.frameBuffer.get();
> + acquireFence = buffer.buffer.acquire_fence;
> LOG(HAL, Debug) << ss.str() << " (direct)";
> break;
>
> @@ -985,18 +984,18 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques
> * The buffer has to be returned to the CameraStream
> * once it has been processed.
> */
> - buffer = cameraStream->getBuffer();
> + frameBuffer = cameraStream->getBuffer();
> LOG(HAL, Debug) << ss.str() << " (internal)";
> break;
> }
>
> - if (!buffer) {
> - LOG(HAL, Error) << "Failed to create buffer";
> + if (!frameBuffer) {
> + LOG(HAL, Error) << "Failed to create frame buffer";
> return -ENOMEM;
> }
>
> - descriptor->request_->addBuffer(cameraStream->stream(), buffer,
> - acquireFence);
> + descriptor->request_->addBuffer(cameraStream->stream(),
> + frameBuffer, acquireFence);
> }
>
> /*
> @@ -1082,9 +1081,9 @@ void CameraDevice::requestComplete(Request *request)
> * The buffer status is set to OK and later changed to ERROR if
> * post-processing/compression fails.
> */
> - for (camera3_stream_buffer_t &buffer : descriptor->buffers_) {
> + for (auto &buffer : descriptor->buffers_) {
> CameraStream *cameraStream =
> - static_cast<CameraStream *>(buffer.stream->priv);
> + static_cast<CameraStream *>(buffer.buffer.stream->priv);
>
> /*
> * Streams of type Direct have been queued to the
> @@ -1098,9 +1097,9 @@ void CameraDevice::requestComplete(Request *request)
> * fence to -1 once it has handled it and remove this check.
> */
> if (cameraStream->type() == CameraStream::Type::Direct)
> - buffer.acquire_fence = -1;
> - buffer.release_fence = -1;
> - buffer.status = CAMERA3_BUFFER_STATUS_OK;
> + buffer.buffer.acquire_fence = -1;
> + buffer.buffer.release_fence = -1;
> + buffer.buffer.status = CAMERA3_BUFFER_STATUS_OK;
> }
>
> /*
> @@ -1115,15 +1114,15 @@ void CameraDevice::requestComplete(Request *request)
> notifyError(descriptor->frameNumber_, nullptr,
> CAMERA3_MSG_ERROR_REQUEST);
>
> - for (camera3_stream_buffer_t &buffer : descriptor->buffers_) {
> + for (auto &buffer : descriptor->buffers_) {
> /*
> * Signal to the framework it has to handle fences that
> * have not been waited on by setting the release fence
> * to the acquire fence value.
> */
> - buffer.release_fence = buffer.acquire_fence;
> - buffer.acquire_fence = -1;
> - buffer.status = CAMERA3_BUFFER_STATUS_ERROR;
> + buffer.buffer.release_fence = buffer.buffer.acquire_fence;
> + buffer.buffer.acquire_fence = -1;
> + buffer.buffer.status = CAMERA3_BUFFER_STATUS_ERROR;
> }
>
> descriptor->status_ = Camera3RequestDescriptor::Status::Error;
> @@ -1160,9 +1159,9 @@ void CameraDevice::requestComplete(Request *request)
> }
>
> /* Handle post-processing. */
> - for (camera3_stream_buffer_t &buffer : descriptor->buffers_) {
> + for (auto &buffer : descriptor->buffers_) {
> CameraStream *cameraStream =
> - static_cast<CameraStream *>(buffer.stream->priv);
> + static_cast<CameraStream *>(buffer.buffer.stream->priv);
>
> if (cameraStream->type() == CameraStream::Type::Direct)
> continue;
> @@ -1170,13 +1169,13 @@ void CameraDevice::requestComplete(Request *request)
> FrameBuffer *src = request->findBuffer(cameraStream->stream());
> if (!src) {
> LOG(HAL, Error) << "Failed to find a source stream buffer";
> - buffer.status = CAMERA3_BUFFER_STATUS_ERROR;
> - notifyError(descriptor->frameNumber_, buffer.stream,
> + buffer.buffer.status = CAMERA3_BUFFER_STATUS_ERROR;
> + notifyError(descriptor->frameNumber_, buffer.buffer.stream,
> CAMERA3_MSG_ERROR_BUFFER);
> continue;
> }
>
> - int ret = cameraStream->process(*src, buffer, descriptor);
> + int ret = cameraStream->process(*src, buffer.buffer, descriptor);
>
> /*
> * Return the FrameBuffer to the CameraStream now that we're
> @@ -1186,8 +1185,8 @@ void CameraDevice::requestComplete(Request *request)
> cameraStream->putBuffer(src);
>
> if (ret) {
> - buffer.status = CAMERA3_BUFFER_STATUS_ERROR;
> - notifyError(descriptor->frameNumber_, buffer.stream,
> + buffer.buffer.status = CAMERA3_BUFFER_STATUS_ERROR;
> + notifyError(descriptor->frameNumber_, buffer.buffer.stream,
> CAMERA3_MSG_ERROR_BUFFER);
> }
> }
> @@ -1213,10 +1212,16 @@ void CameraDevice::sendCaptureResults()
> camera3_capture_result_t captureResult = {};
>
> captureResult.frame_number = descriptor->frameNumber_;
> +
> if (descriptor->resultMetadata_)
> captureResult.result = descriptor->resultMetadata_->get();
> - captureResult.num_output_buffers = descriptor->buffers_.size();
> - captureResult.output_buffers = descriptor->buffers_.data();
> +
> + std::vector<camera3_stream_buffer_t> resultBuffers;
Can't you std::vector::reserve(descriptor->streams.size()) to avoid
relocations ?
I guess I'll find out more in next patches how this change will be
used.
Reviewed-by: Jacopo Mondi <jacopo at jmondi.org>
Thanks
j
> + for (const auto &buffer : descriptor->buffers_)
> + resultBuffers.emplace_back(buffer.buffer);
> +
> + captureResult.num_output_buffers = resultBuffers.size();
> + captureResult.output_buffers = resultBuffers.data();
>
> if (descriptor->status_ == Camera3RequestDescriptor::Status::Success)
> captureResult.partial_result = 1;
> diff --git a/src/android/camera_request.cpp b/src/android/camera_request.cpp
> index 16a632b3..614baed4 100644
> --- a/src/android/camera_request.cpp
> +++ b/src/android/camera_request.cpp
> @@ -23,15 +23,10 @@ Camera3RequestDescriptor::Camera3RequestDescriptor(
>
> /* Copy the camera3 request stream information for later access. */
> const uint32_t numBuffers = camera3Request->num_output_buffers;
> +
> buffers_.resize(numBuffers);
> for (uint32_t i = 0; i < numBuffers; i++)
> - buffers_[i] = camera3Request->output_buffers[i];
> -
> - /*
> - * FrameBuffer instances created by wrapping a camera3 provided dmabuf
> - * are emplaced in this vector of unique_ptr<> for lifetime management.
> - */
> - frameBuffers_.reserve(numBuffers);
> + buffers_[i].buffer = camera3Request->output_buffers[i];
>
> /* Clone the controls associated with the camera3 request. */
> settings_ = CameraMetadata(camera3Request->settings);
> diff --git a/src/android/camera_request.h b/src/android/camera_request.h
> index db13f624..a030febf 100644
> --- a/src/android/camera_request.h
> +++ b/src/android/camera_request.h
> @@ -29,6 +29,16 @@ public:
> Error,
> };
>
> + struct StreamBuffer {
> + camera3_stream_buffer_t buffer;
> + /*
> + * FrameBuffer instances created by wrapping a camera3 provided
> + * dmabuf are emplaced in this vector of unique_ptr<> for
> + * lifetime management.
> + */
> + std::unique_ptr<libcamera::FrameBuffer> frameBuffer;
> + };
> +
> Camera3RequestDescriptor(libcamera::Camera *camera,
> const camera3_capture_request_t *camera3Request);
> ~Camera3RequestDescriptor();
> @@ -36,8 +46,9 @@ public:
> bool isPending() const { return status_ == Status::Pending; }
>
> uint32_t frameNumber_ = 0;
> - std::vector<camera3_stream_buffer_t> buffers_;
> - std::vector<std::unique_ptr<libcamera::FrameBuffer>> frameBuffers_;
> +
> + std::vector<StreamBuffer> buffers_;
> +
> CameraMetadata settings_;
> std::unique_ptr<CaptureRequest> request_;
> std::unique_ptr<CameraMetadata> resultMetadata_;
> --
> 2.31.0
>
More information about the libcamera-devel
mailing list