[libcamera-devel] [PATCH v2 09/12] android: camera_request: Don't embed full camera3_stream_buffer_t
Jacopo Mondi
jacopo at jmondi.org
Tue Oct 19 14:12:08 CEST 2021
Hi
On Tue, Oct 19, 2021 at 05:17:59PM +0530, Umang Jain wrote:
> From: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
>
> The camera3_stream_buffer_t structure is meant to communicate between
> the camera service and the HAL. They are short-live structures that
> don't outlive the .process_capture_request() operation (when queuing
> requests) or the .process_capture_result() callback.
>
> We currently store copies of the camera3_stream_buffer_t passed to
> .process_capture_request() in Camera3RequestDescriptor::StreamBuffer to
> store the structure members that the HAL need, and reuse them when
> calling the .process_capture_result() callback. This is conceptually not
> right, as the camera3_stream_buffer_t pass to the callback are not the
> same objects as the ones received in .process_capture_request().
>
> Store individual fields of the camera3_stream_buffer_t in StreamBuffer
> instead of copying the whole structure. This gives the HAL full control
> of how data is stored, and properly decouples request queueing from
> result reporting.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> Signed-off-by: Umang Jain <umang.jain at ideasonboard.com>
> Reviewed-by: Umang Jain<umang.jain at ideasonboard.com>
I'm not sure I get what is functionally different, we were copying in
the received camera3_stream_buffer_t, so there was no dependency with
the ones passed in by the service.
Anyway, it shortne names, so I think it's good :)
Reviewed-by: Jacopo Mondi <jacopo at jmondi.org>
Thanks
j
> ---
> src/android/camera_device.cpp | 73 ++++++++++++++++++----------------
> src/android/camera_request.cpp | 19 +++++++--
> src/android/camera_request.h | 12 +++---
> src/android/camera_stream.cpp | 6 +--
> 4 files changed, 63 insertions(+), 47 deletions(-)
>
> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> index 0bb547ae..0a2d3826 100644
> --- a/src/android/camera_device.cpp
> +++ b/src/android/camera_device.cpp
> @@ -801,16 +801,8 @@ void CameraDevice::abortRequest(Camera3RequestDescriptor *descriptor) const
> {
> notifyError(descriptor->frameNumber_, nullptr, CAMERA3_MSG_ERROR_REQUEST);
>
> - 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.buffer.release_fence = buffer.buffer.acquire_fence;
> - buffer.buffer.acquire_fence = -1;
> - buffer.buffer.status = CAMERA3_BUFFER_STATUS_ERROR;
> - }
> + for (auto &buffer : descriptor->buffers_)
> + buffer.status = Camera3RequestDescriptor::Status::Error;
>
> descriptor->status_ = Camera3RequestDescriptor::Status::Error;
> }
> @@ -908,8 +900,8 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques
> << " with " << descriptor->buffers_.size() << " streams";
>
> for (const auto &[i, buffer] : utils::enumerate(descriptor->buffers_)) {
> - camera3_stream *camera3Stream = buffer.buffer.stream;
> - CameraStream *cameraStream = static_cast<CameraStream *>(camera3Stream->priv);
> + CameraStream *cameraStream = buffer.stream;
> + camera3_stream_t *camera3Stream = cameraStream->camera3Stream();
>
> std::stringstream ss;
> ss << i << " - (" << camera3Stream->width << "x"
> @@ -944,11 +936,11 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques
> * lifetime management only.
> */
> buffer.frameBuffer =
> - createFrameBuffer(*buffer.buffer.buffer,
> + createFrameBuffer(*buffer.camera3Buffer,
> cameraStream->configuration().pixelFormat,
> cameraStream->configuration().size);
> frameBuffer = buffer.frameBuffer.get();
> - acquireFence = buffer.buffer.acquire_fence;
> + acquireFence = buffer.fence;
> LOG(HAL, Debug) << ss.str() << " (direct)";
> break;
>
> @@ -1055,12 +1047,11 @@ void CameraDevice::requestComplete(Request *request)
> /*
> * Prepare the capture result for the Android camera stack.
> *
> - * The buffer status is set to OK and later changed to ERROR if
> + * The buffer status is set to Success and later changed to Error if
> * post-processing/compression fails.
> */
> for (auto &buffer : descriptor->buffers_) {
> - CameraStream *cameraStream =
> - static_cast<CameraStream *>(buffer.buffer.stream->priv);
> + CameraStream *stream = buffer.stream;
>
> /*
> * Streams of type Direct have been queued to the
> @@ -1073,10 +1064,9 @@ void CameraDevice::requestComplete(Request *request)
> * \todo Instrument the CameraWorker to set the acquire
> * fence to -1 once it has handled it and remove this check.
> */
> - if (cameraStream->type() == CameraStream::Type::Direct)
> - buffer.buffer.acquire_fence = -1;
> - buffer.buffer.release_fence = -1;
> - buffer.buffer.status = CAMERA3_BUFFER_STATUS_OK;
> + if (stream->type() == CameraStream::Type::Direct)
> + buffer.fence = -1;
> + buffer.status = Camera3RequestDescriptor::Status::Success;
> }
>
> /*
> @@ -1128,33 +1118,32 @@ void CameraDevice::requestComplete(Request *request)
>
> /* Handle post-processing. */
> for (auto &buffer : descriptor->buffers_) {
> - CameraStream *cameraStream =
> - static_cast<CameraStream *>(buffer.buffer.stream->priv);
> + CameraStream *stream = buffer.stream;
>
> - if (cameraStream->type() == CameraStream::Type::Direct)
> + if (stream->type() == CameraStream::Type::Direct)
> continue;
>
> - FrameBuffer *src = request->findBuffer(cameraStream->stream());
> + FrameBuffer *src = request->findBuffer(stream->stream());
> if (!src) {
> LOG(HAL, Error) << "Failed to find a source stream buffer";
> - buffer.buffer.status = CAMERA3_BUFFER_STATUS_ERROR;
> - notifyError(descriptor->frameNumber_, buffer.buffer.stream,
> + buffer.status = Camera3RequestDescriptor::Status::Error;
> + notifyError(descriptor->frameNumber_, stream->camera3Stream(),
> CAMERA3_MSG_ERROR_BUFFER);
> continue;
> }
>
> - int ret = cameraStream->process(*src, buffer, descriptor);
> + int ret = stream->process(*src, buffer, descriptor);
>
> /*
> * Return the FrameBuffer to the CameraStream now that we're
> * done processing it.
> */
> - if (cameraStream->type() == CameraStream::Type::Internal)
> - cameraStream->putBuffer(src);
> + if (stream->type() == CameraStream::Type::Internal)
> + stream->putBuffer(src);
>
> if (ret) {
> - buffer.buffer.status = CAMERA3_BUFFER_STATUS_ERROR;
> - notifyError(descriptor->frameNumber_, buffer.buffer.stream,
> + buffer.status = Camera3RequestDescriptor::Status::Error;
> + notifyError(descriptor->frameNumber_, stream->camera3Stream(),
> CAMERA3_MSG_ERROR_BUFFER);
> }
> }
> @@ -1185,8 +1174,24 @@ void CameraDevice::sendCaptureResults()
> captureResult.result = descriptor->resultMetadata_->get();
>
> std::vector<camera3_stream_buffer_t> resultBuffers;
> - for (const auto &buffer : descriptor->buffers_)
> - resultBuffers.emplace_back(buffer.buffer);
> + resultBuffers.reserve(descriptor->buffers_.size());
> +
> + for (const auto &buffer : descriptor->buffers_) {
> + camera3_buffer_status status = CAMERA3_BUFFER_STATUS_ERROR;
> +
> + if (buffer.status == Camera3RequestDescriptor::Status::Success)
> + status = CAMERA3_BUFFER_STATUS_OK;
> +
> + /*
> + * Pass the buffer fence back to the camera framework as
> + * a release fence. This instructs the framework to wait
> + * on the acquire fence in case we haven't done so
> + * ourselves for any reason.
> + */
> + resultBuffers.push_back({ buffer.stream->camera3Stream(),
> + buffer.camera3Buffer, status,
> + -1, buffer.fence });
> + }
>
> captureResult.num_output_buffers = resultBuffers.size();
> captureResult.output_buffers = resultBuffers.data();
> diff --git a/src/android/camera_request.cpp b/src/android/camera_request.cpp
> index 614baed4..faa85ada 100644
> --- a/src/android/camera_request.cpp
> +++ b/src/android/camera_request.cpp
> @@ -7,6 +7,8 @@
>
> #include "camera_request.h"
>
> +#include <libcamera/base/span.h>
> +
> using namespace libcamera;
>
> /*
> @@ -22,11 +24,20 @@ Camera3RequestDescriptor::Camera3RequestDescriptor(
> frameNumber_ = camera3Request->frame_number;
>
> /* Copy the camera3 request stream information for later access. */
> - const uint32_t numBuffers = camera3Request->num_output_buffers;
> + const Span<const camera3_stream_buffer_t> buffers{
> + camera3Request->output_buffers,
> + camera3Request->num_output_buffers
> + };
> +
> + buffers_.reserve(buffers.size());
> +
> + for (const camera3_stream_buffer_t &buffer : buffers) {
> + CameraStream *stream =
> + static_cast<CameraStream *>(buffer.stream->priv);
>
> - buffers_.resize(numBuffers);
> - for (uint32_t i = 0; i < numBuffers; i++)
> - buffers_[i].buffer = camera3Request->output_buffers[i];
> + buffers_.push_back({ stream, buffer.buffer, nullptr,
> + buffer.acquire_fence, Status::Pending });
> + }
>
> /* 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 a030febf..05dabf89 100644
> --- a/src/android/camera_request.h
> +++ b/src/android/camera_request.h
> @@ -20,6 +20,8 @@
> #include "camera_metadata.h"
> #include "camera_worker.h"
>
> +class CameraStream;
> +
> class Camera3RequestDescriptor
> {
> public:
> @@ -30,13 +32,11 @@ public:
> };
>
> 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.
> - */
> + CameraStream *stream;
> + buffer_handle_t *camera3Buffer;
> std::unique_ptr<libcamera::FrameBuffer> frameBuffer;
> + int fence;
> + Status status;
> };
>
> Camera3RequestDescriptor(libcamera::Camera *camera,
> diff --git a/src/android/camera_stream.cpp b/src/android/camera_stream.cpp
> index f3cc77e7..9b5cd0c4 100644
> --- a/src/android/camera_stream.cpp
> +++ b/src/android/camera_stream.cpp
> @@ -147,11 +147,11 @@ int CameraStream::process(const FrameBuffer &source,
> Camera3RequestDescriptor *request)
> {
> /* Handle waiting on fences on the destination buffer. */
> - int fence = dest.buffer.acquire_fence;
> + int fence = dest.fence;
> if (fence != -1) {
> int ret = waitFence(fence);
> ::close(fence);
> - dest.buffer.acquire_fence = -1;
> + dest.fence = -1;
> if (ret < 0) {
> LOG(HAL, Error) << "Failed waiting for fence: "
> << fence << ": " << strerror(-ret);
> @@ -167,7 +167,7 @@ int CameraStream::process(const FrameBuffer &source,
> * separate thread.
> */
> const StreamConfiguration &output = configuration();
> - CameraBuffer destBuffer(*dest.buffer.buffer, output.pixelFormat,
> + CameraBuffer destBuffer(*dest.camera3Buffer, output.pixelFormat,
> output.size, PROT_READ | PROT_WRITE);
> if (!destBuffer.isValid()) {
> LOG(HAL, Error) << "Failed to create destination buffer";
> --
> 2.31.0
>
More information about the libcamera-devel
mailing list