[PATCH v2 4/9] android: Migrate StreamBuffer::internalBuffer to Camera3RequestDescriptor
Cheng-Hao Yang
chenghaoyang at chromium.org
Fri Nov 29 08:51:39 CET 2024
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.
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.
[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.
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.
> > > + *
> > > + * 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.
> >
> > 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?
>
> 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.
> > 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