[libcamera-devel] [PATCH 05/11] android: camera_device: Create struct to track per stream buffer
Hirokazu Honda
hiroh at chromium.org
Tue Oct 19 06:48:51 CEST 2021
Hi Umang, thank you for the patch.
On Tue, Oct 19, 2021 at 1:21 AM Jacopo Mondi <jacopo at jmondi.org> wrote:
>
> 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>
Reviewed-by: Hirokazu Honda <hiroh at chromium.org>
> > ---
> > 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