[libcamera-devel] [PATCH 7/8] android: camera_device: Use libcamera buffer pool

[Hirokazu Honda]

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.
|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.

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