[PATCH v3 3/7] android: Correctly support multiple Mapped streams
Cheng-Hao Yang
chenghaoyang at chromium.org
Mon Dec 9 06:23:00 CET 2024
Hi Jacopo,
Will upload a new version later.
On Fri, Dec 6, 2024 at 12:13 AM Jacopo Mondi
<jacopo.mondi at ideasonboard.com> wrote:
>
> 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/
Ah right, I should've fixed it. Thanks!
>
> > 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
Done
>
> > */
> > - 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"
Done
>
> > + */
> > + 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.
Ah, I understand that reverting the logic would make the comment
clearer, while I still prefer the current order, which I believe also
involves less line changes.
>
> With the two nits fixed, and with or without the above change
> Reviewed-by: Jacopo Mondi <jacopo.mondi at ideasonboard.com>
Thanks!
BR,
Harvey
>
> 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