[libcamera-devel] [PATCH v2 10/13] android: camera_stream: Create buffer poll
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Wed Oct 7 03:53:20 CEST 2020
Hi Jacopo,
Thank you for the patch.
On Tue, Oct 06, 2020 at 04:44:29PM +0200, Jacopo Mondi wrote:
> Add a FrameBufferAllocator class member to the CameraStream class.
> The allocator is constructed for CameraStream instances that needs
> internal allocation and automatically deleted.
>
> Allocate FrameBuffers using the allocator_ class member in the
> CameraStream class at CameraStream::configure() time and add two
> methods to the CameraStream class to get and put FrameBuffer pointers
> from the pool of allocated buffers. As buffer allocation can take place
> only after the Camera has been configured, move the CameraStream
> configuration loop in the CameraDevice class after camera_->configure()
> call.
>
> The newly created pool will be used to provide buffers to CameraStream
> that need to provide memory to libcamera where to deliver frames.
>
> Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> ---
> src/android/camera_device.cpp | 22 +++++++++---------
> src/android/camera_stream.cpp | 43 +++++++++++++++++++++++++++++++++++
> src/android/camera_stream.h | 9 ++++++++
> 3 files changed, 63 insertions(+), 11 deletions(-)
>
> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> index 1e2cbeeb92d1..ecdc0922e90b 100644
> --- a/src/android/camera_device.cpp
> +++ b/src/android/camera_device.cpp
> @@ -1282,6 +1282,17 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)
> return -EINVAL;
> }
>
> + /*
> + * Once the CameraConfiguration has been adjusted/validated
> + * it can be applied to the camera.
> + */
> + int ret = camera_->configure(config_.get());
> + if (ret) {
> + LOG(HAL, Error) << "Failed to configure camera '"
> + << camera_->id() << "'";
> + return ret;
> + }
> +
> /*
> * Configure the HAL CameraStream instances using the associated
> * StreamConfiguration and set the number of required buffers in
> @@ -1295,17 +1306,6 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)
> }
> }
>
> - /*
> - * Once the CameraConfiguration has been adjusted/validated
> - * it can be applied to the camera.
> - */
> - int ret = camera_->configure(config_.get());
> - if (ret) {
> - LOG(HAL, Error) << "Failed to configure camera '"
> - << camera_->id() << "'";
> - return ret;
> - }
> -
> return 0;
> }
>
> diff --git a/src/android/camera_stream.cpp b/src/android/camera_stream.cpp
> index f899be4fe007..eac1480530f8 100644
> --- a/src/android/camera_stream.cpp
> +++ b/src/android/camera_stream.cpp
> @@ -26,6 +26,11 @@ CameraStream::CameraStream(CameraDevice *cameraDevice, Type type,
>
> if (type_ == Type::Internal || type_ == Type::Mapped)
> encoder_ = std::make_unique<EncoderLibJpeg>();
> +
> + if (type == Type::Internal) {
> + allocator_ = std::make_unique<FrameBufferAllocator>(cameraDevice_->camera());
> + mutex_ = std::make_unique<std::mutex>();
This is a bit annoying. It comes from std::vector<T>::reserve()
requiring T to be MoveInsertable, even if no move will actually take
place as we always reserve entries starting from an empty vector (but
the compiler can't known that).
I think it would be worth using std::list instead of std::vector to
store the CameraStream instances in CameraDevice, and embedding the
mutex. You'll need to drop the reserve() call as std::list doesn't have
that (and update the comment before it accordingly).
Actually I can post the diff as I made the modifications to ensure it
would work :-)
diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
index edac9f28ab67..5a466629b78b 100644
--- a/src/android/camera_device.cpp
+++ b/src/android/camera_device.cpp
@@ -1165,13 +1165,8 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)
return -EINVAL;
}
- /*
- * Clear and remove any existing configuration from previous calls, and
- * ensure the required entries are available without further
- * reallocation.
- */
+ /* Clear and remove any existing configuration from previous calls. */
streams_.clear();
- streams_.reserve(stream_list->num_streams);
/* First handle all non-MJPEG streams. */
camera3_stream_t *jpegStream = nullptr;
diff --git a/src/android/camera_device.h b/src/android/camera_device.h
index b4b32f77a29a..d1412d8d5fb8 100644
--- a/src/android/camera_device.h
+++ b/src/android/camera_device.h
@@ -7,6 +7,7 @@
#ifndef __ANDROID_CAMERA_DEVICE_H__
#define __ANDROID_CAMERA_DEVICE_H__
+#include <list>
#include <map>
#include <memory>
#include <tuple>
@@ -121,7 +122,7 @@ private:
std::vector<Camera3StreamConfiguration> streamConfigurations_;
std::map<int, libcamera::PixelFormat> formatsMap_;
- std::vector<CameraStream> streams_;
+ std::list<CameraStream> streams_;
int facing_;
int orientation_;
diff --git a/src/android/camera_stream.cpp b/src/android/camera_stream.cpp
index eac1480530f8..707c4a5e1077 100644
--- a/src/android/camera_stream.cpp
+++ b/src/android/camera_stream.cpp
@@ -27,10 +27,8 @@ CameraStream::CameraStream(CameraDevice *cameraDevice, Type type,
if (type_ == Type::Internal || type_ == Type::Mapped)
encoder_ = std::make_unique<EncoderLibJpeg>();
- if (type == Type::Internal) {
+ if (type == Type::Internal)
allocator_ = std::make_unique<FrameBufferAllocator>(cameraDevice_->camera());
- mutex_ = std::make_unique<std::mutex>();
- }
}
const StreamConfiguration &CameraStream::configuration() const
@@ -130,7 +128,7 @@ FrameBuffer *CameraStream::getBuffer()
if (!allocator_)
return nullptr;
- std::lock_guard<std::mutex> locker(*mutex_);
+ std::lock_guard<std::mutex> locker(mutex_);
if (buffers_.empty()) {
LOG(HAL, Error) << "Buffer underrun";
@@ -148,7 +146,7 @@ void CameraStream::putBuffer(libcamera::FrameBuffer *buffer)
if (!allocator_)
return;
- std::lock_guard<std::mutex> locker(*mutex_);
+ std::lock_guard<std::mutex> locker(mutex_);
buffers_.push_back(buffer);
}
diff --git a/src/android/camera_stream.h b/src/android/camera_stream.h
index f929e8260ae3..a177a99a2ea1 100644
--- a/src/android/camera_stream.h
+++ b/src/android/camera_stream.h
@@ -138,7 +138,7 @@ private:
std::unique_ptr<Encoder> encoder_;
std::unique_ptr<libcamera::FrameBufferAllocator> allocator_;
std::vector<libcamera::FrameBuffer *> buffers_;
- std::unique_ptr<std::mutex> mutex_;
+ std::mutex mutex_;
};
#endif /* __ANDROID_CAMERA_STREAM__ */
This change can go on top or be integrated in the series (and there's no
need to report just for this), up to you.
Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> + }
> }
>
> const StreamConfiguration &CameraStream::configuration() const
> @@ -46,6 +51,16 @@ int CameraStream::configure()
> return ret;
> }
>
> + if (allocator_) {
> + int ret = allocator_->allocate(stream());
> + if (ret < 0)
> + return ret;
> +
> + /* Save a pointer to the reserved frame buffers */
> + for (const auto &frameBuffer : allocator_->buffers(stream()))
> + buffers_.push_back(frameBuffer.get());
> + }
> +
> camera3Stream_->max_buffers = configuration().bufferCount;
>
> return 0;
> @@ -109,3 +124,31 @@ int CameraStream::process(const libcamera::FrameBuffer &source,
>
> return 0;
> }
> +
> +FrameBuffer *CameraStream::getBuffer()
> +{
> + if (!allocator_)
> + return nullptr;
> +
> + std::lock_guard<std::mutex> locker(*mutex_);
> +
> + if (buffers_.empty()) {
> + LOG(HAL, Error) << "Buffer underrun";
> + return nullptr;
> + }
> +
> + FrameBuffer *buffer = buffers_.back();
> + buffers_.pop_back();
> +
> + return buffer;
> +}
> +
> +void CameraStream::putBuffer(libcamera::FrameBuffer *buffer)
> +{
> + if (!allocator_)
> + return;
> +
> + std::lock_guard<std::mutex> locker(*mutex_);
> +
> + buffers_.push_back(buffer);
> +}
> diff --git a/src/android/camera_stream.h b/src/android/camera_stream.h
> index 4c51f0fb3393..f929e8260ae3 100644
> --- a/src/android/camera_stream.h
> +++ b/src/android/camera_stream.h
> @@ -8,11 +8,14 @@
> #define __ANDROID_CAMERA_STREAM_H__
>
> #include <memory>
> +#include <mutex>
> +#include <vector>
>
> #include <hardware/camera3.h>
>
> #include <libcamera/buffer.h>
> #include <libcamera/camera.h>
> +#include <libcamera/framebuffer_allocator.h>
> #include <libcamera/geometry.h>
> #include <libcamera/pixel_format.h>
>
> @@ -117,6 +120,8 @@ public:
> int configure();
> int process(const libcamera::FrameBuffer &source,
> MappedCamera3Buffer *dest, CameraMetadata *metadata);
> + libcamera::FrameBuffer *getBuffer();
> + void putBuffer(libcamera::FrameBuffer *buffer);
>
> private:
> CameraDevice *cameraDevice_;
> @@ -129,7 +134,11 @@ private:
> * one or more streams to the Android framework.
> */
> unsigned int index_;
> +
> std::unique_ptr<Encoder> encoder_;
> + std::unique_ptr<libcamera::FrameBufferAllocator> allocator_;
> + std::vector<libcamera::FrameBuffer *> buffers_;
> + std::unique_ptr<std::mutex> mutex_;
> };
>
> #endif /* __ANDROID_CAMERA_STREAM__ */
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list