[libcamera-devel] [PATCH v2 10/13] android: camera_stream: Create buffer poll
Kieran Bingham
kieran.bingham at ideasonboard.com
Wed Oct 7 11:31:28 CEST 2020
Hi Jacopo
in $SUBJECT s/poll/pool/
--
Kieran
On 07/10/2020 09:05, Jacopo Mondi wrote:
> 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
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
>
--
Regards
--
Kieran
More information about the libcamera-devel
mailing list