[libcamera-devel] [PATCH v2 10/13] android: camera_stream: Create buffer poll
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Wed Oct 7 15:18:02 CEST 2020
Hi Jacopo,
On Wed, Oct 07, 2020 at 10:05:32AM +0200, Jacopo Mondi wrote:
> On Wed, Oct 07, 2020 at 04:53:20AM +0300, Laurent Pinchart wrote:
> > 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."
That's until C++11. Starting at C++17, it states
"The requirements that are imposed on the elements depend on the actual
operations performed on the container. Generally, it is required that
element type meets the requirements of Erasable, but many member
functions impose stricter requirements. This container (but not its
members) can be instantiated with an incomplete element type if the
allocator satisfies the allocator completeness requirements."
std::list::push_back() requires CopyInsertable or MoveInsertable
(depending on which overload is used), but std::list::emplace_back()
only requires EmplaceConstructible.
> 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