[PATCH v2 4/9] android: Migrate StreamBuffer::internalBuffer to Camera3RequestDescriptor
Jacopo Mondi
jacopo.mondi at ideasonboard.com
Mon Dec 2 16:39:51 CET 2024
Hi Harvey
On Fri, Nov 29, 2024 at 03:51:39PM +0800, Cheng-Hao Yang wrote:
> Hi Jacopo and Laurent,
>
> On Thu, Nov 28, 2024 at 10:32 PM Laurent Pinchart
> <laurent.pinchart at ideasonboard.com> wrote:
> >
> > On Thu, Nov 28, 2024 at 03:24:44PM +0100, Jacopo Mondi wrote:
> > > Hi Harevy
> > >
> > > On Wed, Nov 27, 2024 at 09:25:54AM +0000, Harvey Yang wrote:
> > > > Previously there's a potential issue in StreamBuffer::internalBuffer's
> > > > lifetime, when more than one streams depend on the same internal buffer.
> > >
> > > s/.//
> > >
> > > lifetime, when more than one streams depend on the same internal buffer
> > > for post-processing, the buffer is returned to the CameraStream pool
> > > as soon as the first post-processing request has completed.
>
> Updated.
>
> > >
> > > >
> > > > This patch fixes the issue by returning the buffer when the whole
> > > > Camera3RequestDescriptor is destructed.
> > >
> > > Actually it does more I guess, it allows to map multiple streams of
> > > type ::Mapped to the same libcamera::Stream, doesn't it ?
>
> According to the previous implementation, I'd say that's already
> allowed, just not correctly implemented yet.
> Looking into [1], the condition doesn't care if the sourceStream
> has been mapped by another mapped stream or not.
No but as you can see it is not supported as we immediately return the
source stream buffer once post-processing is done.
However, something tells me this was by design as we allow a single
(Mapped) JPEG stream
/* Defer handling of MJPEG streams until all others are known. */
if (stream->format == HAL_PIXEL_FORMAT_BLOB) {
if (jpegStream) {
LOG(HAL, Error)
<< "Multiple JPEG streams are not supported";
return -EINVAL;
}
jpegStream = stream;
continue;
}
What is the use case you have for mapping multiple Mapped streams to
the same Direct|Internal one ? Multiple streams produced using libyuv
conversion ? (afaict that's the only other post-processing case we
support)
>
> Unless you mean that because there's such a bug, it's not allowed...
> I'd say that we should have a FATAL/ERROR log at least, or even
> block such a configuration.
>
Well, this patch fixes it, doesn't it ?
> [1]: https://git.libcamera.org/libcamera/libcamera.git/tree/src/android/camera_device.cpp#n1071
>
>
> > >
> > > >
> > > > 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 | 63 +++++++++++++++++++---------------
> > > > src/android/camera_request.cpp | 13 ++++---
> > > > src/android/camera_request.h | 3 +-
> > > > 3 files changed, 46 insertions(+), 33 deletions(-)
> > > >
> > > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> > > > index 11613fac9..62f724041 100644
> > > > --- a/src/android/camera_device.cpp
> > > > +++ b/src/android/camera_device.cpp
> > > > @@ -967,9 +967,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 requestedDirectBuffers is an std:map<>, no duplications can
> > > > + * happen.
> > > > */
> > > > - std::map<CameraStream *, libcamera::FrameBuffer *> requestedBuffers;
> > > > + std::map<CameraStream *, libcamera::FrameBuffer *> requestedDirectBuffers;
> > >
> > > Do you actually need to rename ?
>
> In this patch, this map only includes buffers from direct type.
> The internal type buffers are included by
> `Camera3RequestDescriptor::internalBuffers_` instead.
>
let's call it directBuffers as a middle ground
> If we don't exclude the internal ones in this map, we don't
> need to rename it as well. WDYT?
>
> > >
> > > > for (const auto &[i, buffer] : utils::enumerate(descriptor->buffers_)) {
> > > > CameraStream *cameraStream = buffer.stream;
> > > > camera3_stream_t *camera3Stream = cameraStream->camera3Stream();
> > > > @@ -1009,7 +1010,7 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques
> > > > frameBuffer = buffer.frameBuffer.get();
> > > > acquireFence = std::move(buffer.fence);
> > > >
> > > > - requestedBuffers[cameraStream] = frameBuffer;
> > > > + requestedDirectBuffers[cameraStream] = frameBuffer;
> > > > LOG(HAL, Debug) << ss.str() << " (direct)";
> > > > break;
> > > >
> > > > @@ -1018,14 +1019,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 destructed.
> > >
> > > or 'destroyed'
>
> Done
>
> > >
> > > > */
> > > > frameBuffer = cameraStream->getBuffer();
> > > > - buffer.internalBuffer = frameBuffer;
> > > > buffer.srcBuffer = frameBuffer;
> > > >
> > > > - requestedBuffers[cameraStream] = frameBuffer;
> > > > + /*
> > > > + * Track the allocated internal buffers, which will be
> > > > + * recycled when the descriptor destroyed.
> > > > + * */
> > >
> > > Stray * */
>
> Updated
>
> > >
> > > > + descriptor->internalBuffers_[cameraStream] = frameBuffer;
> > > > LOG(HAL, Debug) << ss.str() << " (internal)";
> > > >
> > > > descriptor->pendingStreamsToProcess_.insert(
> > > > @@ -1079,24 +1083,41 @@ 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 = requestedDirectBuffers.find(sourceStream);
> > > > + if (iterDirectBuffer != requestedDirectBuffers.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.
> > > > + *
Could you break this comment in two ? The first part apply to the rest
of the for() loop, the second part only to the following if()
/*
* 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.
*/
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);
> > > > + * If an internal buffer has been requested for the source
> > > > + * stream before, we should reuse it.
> > > > + */
> > > > + 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 = cameraStream->getBuffer();
> > > > - buffer.internalBuffer = frameBuffer;
> > > > + FrameBuffer *frameBuffer = sourceStream->getBuffer();
> > >
> > > Here I don't see why we're using the sourceStream's pool instead
> > > of the cameraStream's pool
>
> Because the buffer recycle process is different. In this patch, it'll be
> returned to the stream set to `internalBuffers_`. sourceStream is the
> correct one after this patch.
>
Ok. As I understand this it is because, now that we allow multiple
Mapped stream to be ... mapped ... on a Direct stream. If we fall here
it's because the Direct stream its not part of the capture_request, so
we have to create a buffer from a pool and map multiple processed
streams on it. So yes the pool to use is the sourceStream one.
> > >
> > > However, I think this goes in the direction of making the HAL capable
> > > of more things, and I'm looking at the diff of this and the previous
> > > patch combined and I think you should squash the two and make a commit
> > > about "Enabling multiple processed streams to map to the same internal
> > > buffer".
>
> Hmm, as I stated above. I think multiple processed streams to map to the same
> internal buffer is already allowed, just that it's not handled properly.
> Do you still think that we should squash the two into one?
>
Looking at the combined diff it seems easier to make a single commit
along the lines of
-------------------------------------------------------------------------------
android: Correctly support multiple Mapped streams
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
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.
-------------------------------------------------------------------------------
I'm sure this can be written a 1000 times better than this, but
at least, this is the level of detail and description a commit message
should provide
> >
> > Processed streams are expensive, is there enough CPU power to make that
> > possible ? I suppose it wouldn't be an issue when producing, for
> > instance, a processed JPEG stream with a hardware encoder and a scaled
> > YUV stream with libyuv from the same stream.
> >
> > This is interesting, but isn't mentioned at all in the cover letter, and
> > goes well beyond the subject of the series. Is is an intentional change
> > ?
>
> Therefore, I took this as a fix of an issue, instead of adding a feature.
>
> Let me know if you prefer to merge this as a separate series.
>
> >
> > > Looking at the combined diff, it feels to me you could populate
> > > buffer.src for the Direct case in the switch() and keep using an
>
> Sorry, I don't get it. Mapped streams need to be handled the last,
> so that other streams and `requestedDirectBuffers` and
> `internalBuffers_` are setup, IIUC.
>
No you're right, there's no buffer.srcBuffer to populate for Direct
Thanks
j
> > > std::set for requestedDirectBuffers (or requestedStreams). This would
> > > make the diff smaller probably.
> > >
> > >
> > > > buffer.srcBuffer = frameBuffer;
> > > >
> > > > descriptor->request_->addBuffer(sourceStream->stream(),
> > > > frameBuffer, nullptr);
> > > >
> > > > - requestedBuffers[sourceStream] = frameBuffer;
> > > > + /* Track the allocated internal buffer. */
> > > > + descriptor->internalBuffers_[sourceStream] = frameBuffer;
> > > > }
> > > >
> > > > /*
> > > > @@ -1256,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);
> > > > }
> > > > }
> > > >
> > > > @@ -1381,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..a9240a83c 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,14 @@ 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.
> > > > + */
> > >
> > > Fits in one line most probably
>
> Done
>
> BR,
> Harvey
>
> > >
> > > > + for (auto &[sourceStream, frameBuffer] : internalBuffers_)
> > > > + sourceStream->putBuffer(frameBuffer);
> > > > +}
> > > >
> > > > /**
> > > > * \class StreamBuffer
> > > > @@ -166,9 +174,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;
> > > >
> >
> > --
> > Regards,
> >
> > Laurent Pinchart
More information about the libcamera-devel
mailing list