[libcamera-devel] [PATCH 7/8] android: camera_device: Use libcamera buffer pool
Jacopo Mondi
jacopo at jmondi.org
Fri Sep 11 18:28:00 CEST 2020
Hi Hiro,
On Fri, Sep 11, 2020 at 05:46:06PM +0900, Hirokazu Honda wrote:
> On Thu, Sep 10, 2020 at 9:19 PM Jacopo Mondi <jacopo at jmondi.org> wrote:
> >
> > Hi Niklas,
> >
> > On Thu, Sep 10, 2020 at 01:35:05PM +0200, Niklas Söderlund wrote:
> > > Hi Jacopo,
> > >
> > > Thanks for your work.
> > >
> > > On 2020-09-09 17:54:56 +0200, Jacopo Mondi wrote:
> > > > Now that we have reserved and made available to the camera HAL a
> > > > pool of libcamera allocated buffers, use them when a CameraStream
> > > > instance that requires internal allocation is processed.
> > > >
> > > > Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> > > > ---
> > > > src/android/camera_device.cpp | 47 +++++++++++++++++++++++++----------
> > > > 1 file changed, 34 insertions(+), 13 deletions(-)
> > > >
> > > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> > > > index a7cb1be03b9a..f94b313581e7 100644
> > > > --- a/src/android/camera_device.cpp
> > > > +++ b/src/android/camera_device.cpp
> > > > @@ -1422,6 +1422,8 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques
> > > > for (unsigned int i = 0; i < descriptor->numBuffers; ++i) {
> > > > CameraStream *cameraStream =
> > > > static_cast<CameraStream *>(camera3Buffers[i].stream->priv);
> > > > + const StreamConfiguration &config = config_->at(cameraStream->index());
> > > > + Stream *stream = config.stream();
> > > >
> > > > /*
> > > > * Keep track of which stream the request belongs to and store
> > > > @@ -1430,27 +1432,39 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques
> > > > descriptor->buffers[i].stream = camera3Buffers[i].stream;
> > > > descriptor->buffers[i].buffer = camera3Buffers[i].buffer;
> > > >
> > > > - /* Software streams are handled after hardware streams complete. */
> > > > - if (cameraStream->format() == formats::MJPEG)
> > > > + /* Mapped streams don't need to be added to the Request. */
> > > > + if (cameraStream->type() == CameraStream::Type::Mapped)
> > > > continue;
> > > >
> > > > - /*
> > > > - * Create a libcamera buffer using the dmabuf descriptors of
> > > > - * the camera3Buffer for each stream. The FrameBuffer is
> > > > - * directly associated with the Camera3RequestDescriptor for
> > > > - * lifetime management only.
> > > > - */
> > > > - FrameBuffer *buffer = createFrameBuffer(*camera3Buffers[i].buffer);
> > > > + FrameBuffer *buffer;
> > > > + if (cameraStream->type() == CameraStream::Type::Direct) {
> > > > + /*
> > > > + * Create a libcamera buffer using the dmabuf
> > > > + * descriptors of the camera3Buffer for each stream and
> > > > + * associate it with the Camera3RequestDescriptor for
> > > > + * lifetime management only.
> > > > + */
> > > > + buffer = createFrameBuffer(*camera3Buffers[i].buffer);
> > > > + descriptor->frameBuffers.emplace_back(buffer);
> > > > +
> > > > + } else {
> > > > + /*
> > > > + * Get the frame buffer from the CameraStream internal
> > > > + * buffer pool. The lifetime management of internal
> > > > + * buffers is connected to the one of the
> > > > + * FrameBufferAllocator instance.
> > > > + *
> > > > + * The retrieved buffer has to be returned to the
> > > > + * allocator once it has been processed.
> > > > + */
> > > > + buffer = allocator_.getBuffer(stream);
> > >
> > > Should we add a LOG(Error) if there are no free buffers to retrieve form
> > > the allocator?
> > >
> > > > + }
> > > > if (!buffer) {
> >
> > It's catched here
> >
> > > > LOG(HAL, Error) << "Failed to create buffer";
> > > > delete request;
> > > > delete descriptor;
>
> Although this is not so related to your change, shall we be able to
> use std::unique_ptr or std::shared_ptr more here to specify the
> ownerships?
> IMO, camera_device.cpp seems to have a lot of raw pointers, which
> makes readers to track their ownerships.
> For example, descriptor->frameBuffers.emplace_back(buffe) takes the
> ownership by constructing std::unique_ptr.
> It is not figured out unless seeing frameBuffers declaration.
I sadly know, having chased for a day a bug I've introduced by not
very well understanding that.
> |buffer| should be std::unique_ptr from scratch by letting
> createFrameBuffer() return std::unique_ptr.
> I don't know how feasible it is to make request and descriptor be
> std::unique_ptr/shared_ptr because it is necessary to change libcamera
> interface, but I prefer making it so much.
We could indeed do better, and here we have quite some space for
improvements without changing the libcamera API.
I'll see what I can do for next version without being too invasive.
>
> Reviewed-by: Hirokazu Honda <hiroh at chromium.org>
>
> > > > return -ENOMEM;
> > > > }
> > > > - descriptor->frameBuffers.emplace_back(buffer);
> > > > -
> > > > - StreamConfiguration *streamConfiguration = &config_->at(cameraStream->index());
> > > > - Stream *stream = streamConfiguration->stream();
> > > >
> > > > request->addBuffer(stream, buffer);
> > > > }
> > > > @@ -1561,6 +1575,13 @@ void CameraDevice::requestComplete(Request *request)
> > > > const uint32_t jpeg_orientation = 0;
> > > > resultMetadata->addEntry(ANDROID_JPEG_ORIENTATION,
> > > > &jpeg_orientation, 1);
> > > > +
> > > > + /*
> > > > + * Return the FrameBuffer to the CameraStream now that we're
> > > > + * done processing it.
> > > > + */
> > > > + if (cameraStream->type() == CameraStream::Type::Internal)
> > > > + allocator_.returnBuffer(stream, buffer);
> > >
> > > Seeing how the getBuffer() and returnBuffer() is used here could it not
> > > be replaced with a std::queue in a similar fashion we have in
> > > pipeline handlers?
> > >
> >
> > That's what I've done initially, but that's basically repeting the
> > same pattern in several parts of libcamera.
> >
> > When I saw
> >
> > void CIO2Device::tryReturnBuffer(FrameBuffer *buffer)
> > {
> > /*
> > * \todo Once more pipelines deal with buffers that may be allocated
> > * internally or externally this pattern might become a common need. At
> > * that point this check should be moved to something clever in
> > * FrameBuffer.
> > */
> > for (const std::unique_ptr<FrameBuffer> &buf : buffers_) {
> > if (buf.get() == buffer) {
> > availableBuffers_.push(buffer);
> > break;
> > }
> > }
> > }
> >
> > I'm not saying using an std::queue<> is "clever" as it's surely
> > efficient in insertion/removal but occupies quite some memory, but I
> > considered this worth to be brought to be to be part of the
> > FrameBufferAllocator interface.
> >
> > My intention was to replace the custom implementations in the
> > pipelines by using the newly defined getBuffer()/returnBuffer()
> > methods calls.
> >
> >
> > > > }
> > > >
> > > > /* Prepare to call back the Android camera stack. */
> > > > --
> > > > 2.28.0
> > > >
> > > > _______________________________________________
> > > > libcamera-devel mailing list
> > > > libcamera-devel at lists.libcamera.org
> > > > https://lists.libcamera.org/listinfo/libcamera-devel
> > >
> > > --
> > > Regards,
> > > Niklas Söderlund
> > _______________________________________________
> > libcamera-devel mailing list
> > libcamera-devel at lists.libcamera.org
> > https://lists.libcamera.org/listinfo/libcamera-devel
More information about the libcamera-devel
mailing list