[libcamera-devel] [PATCH v2 09/12] android: camera_request: Don't embed full camera3_stream_buffer_t
Hirokazu Honda
hiroh at chromium.org
Wed Oct 20 04:13:30 CEST 2021
Hi Laurent, thank you for the patch.
On Tue, Oct 19, 2021 at 9:16 PM Laurent Pinchart
<laurent.pinchart at ideasonboard.com> wrote:
>
> Hi Jacopo,
>
> On Tue, Oct 19, 2021 at 02:12:08PM +0200, Jacopo Mondi wrote:
> > 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.
>
> We don't use the same instances as we copy them, but we reuse the value,
> which I don't think is conceptually correct. camera3_stream_buffer_t is
> meant to pass buffer information in the API, it's not a buffer object in
> itself, so I don't think it's the right level of abstraction (and we
> have to extend it with additional fields anyway).
>
> > Anyway, it shortne names, so I think it's good :)
> > Reviewed-by: Jacopo Mondi <jacopo at jmondi.org>
> >
> > > ---
> > > 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());
> > > +
If I read the change of this patch series correctly, when
abortRequest() is called, this for-loop is not reachable?
> > > + 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.
> > > - */
Would you mind commenting the ownership of these variables?
Reviewed-by: Hirokazu Honda <hiroh at chromium.org>
Thanks,
-Hiro
> > > + 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";
>
> --
> Regards,
>
> Laurent Pinchart
More information about the libcamera-devel
mailing list