[libcamera-devel] [PATCH v2 10/13] android: camera_stream: Create buffer poll
Jacopo Mondi
jacopo at jmondi.org
Wed Oct 7 10:05:32 CEST 2020
Hi Laurent,
On Wed, Oct 07, 2020 at 04:53:20AM +0300, Laurent Pinchart wrote:
> 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 :-)
We can indeed take this on top, but I wonder how that works as
std::list documentation reports:
"T must meet the requirements of CopyAssignable and CopyConstructible."
And CameraStream is not CopyConstructable as it contains unique_ptr<>
instances...
>
> 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