[libcamera-devel] [PATCH v2 10/13] android: camera_stream: Create buffer poll
Jacopo Mondi
jacopo at jmondi.org
Wed Oct 7 15:38:53 CEST 2020
Hi Laurent
On Wed, Oct 07, 2020 at 04:18:02PM +0300, Laurent Pinchart wrote:
> 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:
[snip]
> > > >
> > > > 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.
>
Ah, thanks for the details.
Last attempt of push back: using std::list we lose random access,
which we don't need at the moment as we only cycle on them or retrieve
them by pointer from the camera3 streams, but if we need to do so in
future we will have to backtrack.
> > 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