Message ID | 20200909155457.153907-8-jacopo@jmondi.org |
---|---|
State | Superseded |
Headers | show |
Series |
|
Related | show |
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@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) { > LOG(HAL, Error) << "Failed to create buffer"; > delete request; > delete descriptor; > 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? > } > > /* Prepare to call back the Android camera stack. */ > -- > 2.28.0 > > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
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@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; > > 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@lists.libcamera.org > > https://lists.libcamera.org/listinfo/libcamera-devel > > -- > Regards, > Niklas Söderlund
On Thu, Sep 10, 2020 at 9:19 PM Jacopo Mondi <jacopo@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@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@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@lists.libcamera.org > > > https://lists.libcamera.org/listinfo/libcamera-devel > > > > -- > > Regards, > > Niklas Söderlund > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
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@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@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@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@lists.libcamera.org > > > > https://lists.libcamera.org/listinfo/libcamera-devel > > > > > > -- > > > Regards, > > > Niklas Söderlund > > _______________________________________________ > > libcamera-devel mailing list > > libcamera-devel@lists.libcamera.org > > https://lists.libcamera.org/listinfo/libcamera-devel
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); + } if (!buffer) { LOG(HAL, Error) << "Failed to create buffer"; delete request; delete descriptor; 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); } /* Prepare to call back the Android camera stack. */
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@jmondi.org> --- src/android/camera_device.cpp | 47 +++++++++++++++++++++++++---------- 1 file changed, 34 insertions(+), 13 deletions(-)