[PATCH v3 3/7] android: Correctly support multiple Mapped streams
Jacopo Mondi
jacopo.mondi at ideasonboard.com
Thu Dec 5 17:13:30 CET 2024
Hi Harvey
On Wed, Dec 04, 2024 at 04:36:28PM +0000, Harvey Yang wrote:
> The Android camera HAL supports creating streams for the Android
> framework by post-processing streams produced by libcamera.
>
> However at the moment a single Mapped stream can be associated with a
> Direct stream because, after the first post-processing, the data from
> the internal stream are returned preventing further post-processing
> passes.
>
> Fix this by storing the list of Direct stream buffers used as the
Have I suggested this ? The streams in internalBuffers_ are not Direct
but Internal. So s/Direct/Internal/
> post-processing source in an Camera3RequestDescriptor::internalBuffers_
> map to replace the single internalBuffer_ pointer and return the
> internal buffers when the capture request is deleted.
>
> Signed-off-by: Han-Lin Chen <hanlinchen at chromium.org>
> Co-developed-by: Harvey Yang <chenghaoyang at chromium.org>
> Signed-off-by: Harvey Yang <chenghaoyang at chromium.org>
> ---
> src/android/camera_device.cpp | 66 +++++++++++++++++++---------------
> src/android/camera_request.cpp | 11 +++---
> src/android/camera_request.h | 3 +-
> 3 files changed, 47 insertions(+), 33 deletions(-)
>
> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> index f6dadaf22..f2dd8d4fd 100644
> --- a/src/android/camera_device.cpp
> +++ b/src/android/camera_device.cpp
> @@ -966,9 +966,10 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques
> * to a libcamera stream. Streams of type Mapped will be handled later.
> *
> * Collect the CameraStream associated to each requested capture stream.
> - * Since requestedStreams is an std:map<>, no duplications can happen.
> + * Since directBuffers is an std:map<>, no duplications can
> + * happen.
nit: fits on the previous line
> */
> - std::map<CameraStream *, libcamera::FrameBuffer *> requestedBuffers;
> + std::map<CameraStream *, libcamera::FrameBuffer *> directBuffers;
> for (const auto &[i, buffer] : utils::enumerate(descriptor->buffers_)) {
> CameraStream *cameraStream = buffer.stream;
> camera3_stream_t *camera3Stream = cameraStream->camera3Stream();
> @@ -1007,6 +1008,8 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques
> cameraStream->configuration().size);
> frameBuffer = buffer.frameBuffer.get();
> acquireFence = std::move(buffer.fence);
> +
> + directBuffers[cameraStream] = frameBuffer;
> LOG(HAL, Debug) << ss.str() << " (direct)";
> break;
>
> @@ -1015,13 +1018,17 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques
> * Get the frame buffer from the CameraStream internal
> * buffer pool.
> *
> - * The buffer has to be returned to the CameraStream
> - * once it has been processed.
> + * The buffer will be returned to the CameraStream when
> + * the request is destroyed.
> */
> frameBuffer = cameraStream->getBuffer();
> - buffer.internalBuffer = frameBuffer;
> buffer.srcBuffer = frameBuffer;
>
> + /*
> + * Track the allocated internal buffers, which will be
> + * recycled when the descriptor destroyed.
nit: "is destroyed"
> + */
> + descriptor->internalBuffers_[cameraStream] = frameBuffer;
> LOG(HAL, Debug) << ss.str() << " (internal)";
>
> descriptor->pendingStreamsToProcess_.insert(
> @@ -1037,8 +1044,6 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques
> auto fence = std::make_unique<Fence>(std::move(acquireFence));
> descriptor->request_->addBuffer(cameraStream->stream(),
> frameBuffer, std::move(fence));
> -
> - requestedBuffers[cameraStream] = frameBuffer;
> }
>
> /*
> @@ -1076,24 +1081,43 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques
> * post-processing. No need to recycle the buffer since it's
> * owned by Android.
> */
> - auto iterDirectBuffer = requestedBuffers.find(sourceStream);
> - if (iterDirectBuffer != requestedBuffers.end()) {
> + auto iterDirectBuffer = directBuffers.find(sourceStream);
> + if (iterDirectBuffer != directBuffers.end()) {
> buffer.srcBuffer = iterDirectBuffer->second;
> continue;
> }
>
> /*
> - * If that's not the case, we need to add a buffer to the request
> - * for this stream.
> + * If that's not the case, we use an internal buffer allocated
> + * from the source stream.
> + */
> +
> + /*
> + * If an internal buffer has been requested for the source
> + * stream before, we should reuse it.
> */
> - FrameBuffer *frameBuffer = cameraStream->getBuffer();
> - buffer.internalBuffer = frameBuffer;
> + auto iterInternalBuffer = descriptor->internalBuffers_.find(sourceStream);
> + if (iterInternalBuffer != descriptor->internalBuffers_.end()) {
> + buffer.srcBuffer = iterInternalBuffer->second;
> + continue;
> + }
> +
> + /*
> + * Otherwise, we need to create an internal buffer to the
> + * request for the source stream. Get the frame buffer from the
> + * source stream's internal buffer pool.
> + *
> + * The buffer will be returned to the CameraStream when the
> + * request is destructed.
> + */
> + FrameBuffer *frameBuffer = sourceStream->getBuffer();
> buffer.srcBuffer = frameBuffer;
>
> descriptor->request_->addBuffer(sourceStream->stream(),
> frameBuffer, nullptr);
>
> - requestedBuffers[sourceStream] = frameBuffer;
> + /* Track the allocated internal buffer. */
> + descriptor->internalBuffers_[sourceStream] = frameBuffer;
Fine with me.
I wonder if inverting the logic wouldn't make it more clear
/*
* If that's not the case, we use an internal buffer allocated
* from the source stream.
*/
auto iterInternalBuffer = descriptor->internalBuffers_.find(sourceStream);
if (iterInternalBuffer == descriptor->internalBuffers_.end()) {
/*
* We need to create an internal buffer to the request
* for the source stream. Get the frame buffer from the
* source stream's internal buffer pool.
*
* The buffer will be returned to the CameraStream when
* the request is destructed.
*/
FrameBuffer *frameBuffer = sourceStream->getBuffer();
buffer.srcBuffer = frameBuffer;
descriptor->request_->addBuffer(sourceStream->stream(),
frameBuffer, nullptr);
/* Track the allocated internal buffer. */
descriptor->internalBuffers_[sourceStream] = frameBuffer;
continue;
}
/*
* Otherwise if an internal buffer has already been requested
* for the source stream before, we should reuse it.
*/
buffer.srcBuffer = iterInternalBuffer->second;
Up to you.
With the two nits fixed, and with or without the above change
Reviewed-by: Jacopo Mondi <jacopo.mondi at ideasonboard.com>
Thanks
j
> }
>
> /*
> @@ -1253,13 +1277,6 @@ void CameraDevice::requestComplete(Request *request)
> if (ret) {
> setBufferStatus(*buffer, StreamBuffer::Status::Error);
> descriptor->pendingStreamsToProcess_.erase(stream);
> -
> - /*
> - * If the framebuffer is internal to CameraStream return
> - * it back now that we're done processing it.
> - */
> - if (buffer->internalBuffer)
> - stream->putBuffer(buffer->internalBuffer);
> }
> }
>
> @@ -1378,13 +1395,6 @@ void CameraDevice::streamProcessingComplete(StreamBuffer *streamBuffer,
> {
> setBufferStatus(*streamBuffer, status);
>
> - /*
> - * If the framebuffer is internal to CameraStream return it back now
> - * that we're done processing it.
> - */
> - if (streamBuffer->internalBuffer)
> - streamBuffer->stream->putBuffer(streamBuffer->internalBuffer);
> -
> Camera3RequestDescriptor *request = streamBuffer->request;
>
> {
> diff --git a/src/android/camera_request.cpp b/src/android/camera_request.cpp
> index 52a3ac1f7..92e74ab6a 100644
> --- a/src/android/camera_request.cpp
> +++ b/src/android/camera_request.cpp
> @@ -10,6 +10,7 @@
> #include <libcamera/base/span.h>
>
> #include "camera_buffer.h"
> +#include "camera_stream.h"
>
> using namespace libcamera;
>
> @@ -138,7 +139,12 @@ Camera3RequestDescriptor::Camera3RequestDescriptor(
> request_ = camera->createRequest(reinterpret_cast<uint64_t>(this));
> }
>
> -Camera3RequestDescriptor::~Camera3RequestDescriptor() = default;
> +Camera3RequestDescriptor::~Camera3RequestDescriptor()
> +{
> + /* Recycle the allocated internal buffer back to its source stream. */
> + for (auto &[sourceStream, frameBuffer] : internalBuffers_)
> + sourceStream->putBuffer(frameBuffer);
> +}
>
> /**
> * \class StreamBuffer
> @@ -166,9 +172,6 @@ Camera3RequestDescriptor::~Camera3RequestDescriptor() = default;
> * \var StreamBuffer::status
> * \brief Track the status of the buffer
> *
> - * \var StreamBuffer::internalBuffer
> - * \brief Pointer to a buffer internally handled by CameraStream (if any)
> - *
> * \var StreamBuffer::srcBuffer
> * \brief Pointer to the source frame buffer used for post-processing
> *
> diff --git a/src/android/camera_request.h b/src/android/camera_request.h
> index 335f1985d..6b2a00795 100644
> --- a/src/android/camera_request.h
> +++ b/src/android/camera_request.h
> @@ -49,7 +49,6 @@ public:
> std::unique_ptr<HALFrameBuffer> frameBuffer;
> libcamera::UniqueFD fence;
> Status status = Status::Success;
> - libcamera::FrameBuffer *internalBuffer = nullptr;
> const libcamera::FrameBuffer *srcBuffer = nullptr;
> std::unique_ptr<CameraBuffer> dstBuffer;
> Camera3RequestDescriptor *request;
> @@ -85,6 +84,8 @@ public:
> std::unique_ptr<libcamera::Request> request_;
> std::unique_ptr<CameraMetadata> resultMetadata_;
>
> + std::map<CameraStream *, libcamera::FrameBuffer *> internalBuffers_;
> +
> bool complete_ = false;
> Status status_ = Status::Success;
>
> --
> 2.47.0.338.g60cca15819-goog
>
More information about the libcamera-devel
mailing list