[libcamera-devel] [PATCH v2 10/13] android: camera_stream: Create buffer poll
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Wed Oct 7 15:35:59 CEST 2020
Hi Jacopo,
On Wed, Oct 07, 2020 at 03:38:53PM +0200, Jacopo Mondi wrote:
> On Wed, Oct 07, 2020 at 04:18:02PM +0300, Laurent Pinchart wrote:
> > 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.
That's a good point, even though I'm not sure we'll need random access.
I'll let you decide which (potential) issue you like best, and pick
vector or list accordingly :-)
> > > 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