[libcamera-devel] [PATCH 1/9] libcamera: stream: Provide accessors to buffers
Kieran Bingham
kieran.bingham at ideasonboard.com
Mon Jul 8 11:52:34 CEST 2019
Hi Jacopo,
On 06/07/2019 12:34, Niklas Söderlund wrote:
> Hi Jacopo,
>
> Thanks for your work.
>
> On 2019-07-05 00:53:26 +0200, Jacopo Mondi wrote:
>> All interactions with the Stream's buffers currently go through the
>> BufferPool. In order to shorten accessing the buffers array, and
>> restrict access to the Stream's internal buffer pool, provide operations
>> to access the buffers, create and destroy them.
>>
>> It is still possible to access the pool for pipeline handlers to
>> populate it by exporting buffers from a video device to the pool.
This patch confuses me a little.
All operations still need a pool, it's just that you currently don't
register the external buffer pool with the stream...
Couldn't we just stream->importBuffers(myExternalBufferPool) at some
stage to support external buffer pools?
A BufferPool is a public api object, so I haven't understood the
reasoning behind hiding it yet.
>>
>> Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
>> ---
>> include/libcamera/stream.h | 12 ++++++++++
>> src/cam/capture.cpp | 4 ++--
>> src/libcamera/camera.cpp | 6 ++---
>> src/libcamera/pipeline/ipu3/ipu3.cpp | 4 ++--
>> src/libcamera/stream.cpp | 35 ++++++++++++++++++++++++++++
>> src/qcam/main_window.cpp | 4 +---
>> test/camera/capture.cpp | 3 +--
>> test/camera/statemachine.cpp | 3 +--
>> 8 files changed, 57 insertions(+), 14 deletions(-)
>>
>> diff --git a/include/libcamera/stream.h b/include/libcamera/stream.h
>> index 5b4fea324ce4..fa7d6ba4987c 100644
>> --- a/include/libcamera/stream.h
>> +++ b/include/libcamera/stream.h
>> @@ -66,12 +66,24 @@ class Stream
>> {
>> public:
>> Stream();
>> + /*
>> + * FIXME:
>> + * If I could find a way to export buffers in pipeline handlers
>> + * without accessing the pool with
>> + * video_->exportBuffers(&stream->bufferPool());
>> + * we could remove access to the internal pool completely.
>> + */
>> BufferPool &bufferPool() { return bufferPool_; }
>> + std::vector<Buffer> &buffers() { return bufferPool_.buffers(); }
>> + unsigned int bufferCount() const { return bufferPool_.count(); }
>
> I commented on this in v2 and you replied to that and then I dropped the
> ball, sorry about. I will cite your reply to v2 here and then reply to
> it.
>
> My comment was about the possibility to add to address the FIXME.
>
> int importBuffers(V4L2VideoDevice *video)
> {
> return video->exportBuffers(&bufferPool());
> }
>
> You replied:
>
> int Stream::importBuffers() you mean ? I considered that, but to me
> what happen is that "the memory reserved in the video device is made
> accessible to the Stream", so I would either use export or even map
> as verbs. Import is not nice if you ask me, as it might overlap with the
> 'importing external buffers' idea. What do you think?
>
> Yes as Stream::importBuffers() :-) I have no strong opinion on the name
> of the function. My view however that the function name should try to
> describe how it operates on the object it's connected to. So in this
> case importBuffers() make sens to me, as the Stream will import buffers
> from the V4L2 video device.
>
>> const StreamConfiguration &configuration() const { return configuration_; }
>>
>> protected:
>> friend class Camera;
>>
>> + void createBuffers(unsigned int count);
>> + void destroyBuffers();
>> +
>> BufferPool bufferPool_;
>> StreamConfiguration configuration_;
>> };
>> diff --git a/src/cam/capture.cpp b/src/cam/capture.cpp
>> index 6b842d73390d..1bcc9c7e9cf4 100644
>> --- a/src/cam/capture.cpp
>> +++ b/src/cam/capture.cpp
>> @@ -76,7 +76,7 @@ int Capture::capture(EventLoop *loop)
>> unsigned int nbuffers = UINT_MAX;
>> for (StreamConfiguration &cfg : *config_) {
>> Stream *stream = cfg.stream();
>> - nbuffers = std::min(nbuffers, stream->bufferPool().count());
>> + nbuffers = std::min(nbuffers, stream->bufferCount());
>> }
>>
>> /*
>> @@ -95,7 +95,7 @@ int Capture::capture(EventLoop *loop)
>> std::map<Stream *, Buffer *> map;
>> for (StreamConfiguration &cfg : *config_) {
>> Stream *stream = cfg.stream();
>> - map[stream] = &stream->bufferPool().buffers()[i];
>> + map[stream] = &stream->buffers()[i];
>> }
>>
>> ret = request->setBuffers(map);
>> diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp
>> index 592dfd39eacc..088a39623e36 100644
>> --- a/src/libcamera/camera.cpp
>> +++ b/src/libcamera/camera.cpp
>> @@ -683,7 +683,7 @@ int Camera::configure(CameraConfiguration *config)
>> * Allocate buffer objects in the pool.
>> * Memory will be allocated and assigned later.
>> */
>> - stream->bufferPool().createBuffers(cfg.bufferCount);
>> + stream->createBuffers(cfg.bufferCount);
>> }
>>
>> state_ = CameraConfigured;
>> @@ -740,14 +740,14 @@ int Camera::freeBuffers()
>> return -EACCES;
>>
>> for (Stream *stream : activeStreams_) {
>> - if (!stream->bufferPool().count())
>> + if (!stream->bufferCount())
>> continue;
>>
If the external pool was registered, then I guess there would be an
approximation of the following here, and anywhere else relevant:
if (stream->memoryType == externalMemory)
continue;
>> /*
>> * All mappings must be destroyed before buffers can be freed
>> * by the V4L2 device that has allocated them.
>> */
>> - stream->bufferPool().destroyBuffers();
>> + stream->destroyBuffers();
>> }
>>
>> state_ = CameraConfigured;
>> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
>> index e4efb9722f76..2de0892138a8 100644
>> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
>> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
>> @@ -634,7 +634,7 @@ int PipelineHandlerIPU3::allocateBuffers(Camera *camera,
>> * of buffers as the active ones.
>> */
>> if (!outStream->active_) {
>> - bufferCount = vfStream->bufferPool().count();
>> + bufferCount = vfStream->bufferCount();
>> outStream->device_->pool->createBuffers(bufferCount);
>> ret = imgu->exportBuffers(outStream->device_,
>> outStream->device_->pool);
>> @@ -643,7 +643,7 @@ int PipelineHandlerIPU3::allocateBuffers(Camera *camera,
>> }
>>
>> if (!vfStream->active_) {
>> - bufferCount = outStream->bufferPool().count();
>> + bufferCount = outStream->bufferCount();
>> vfStream->device_->pool->createBuffers(bufferCount);
>> ret = imgu->exportBuffers(vfStream->device_,
>> vfStream->device_->pool);
>> diff --git a/src/libcamera/stream.cpp b/src/libcamera/stream.cpp
>> index d8e87c62281c..35197be09c26 100644
>> --- a/src/libcamera/stream.cpp
>> +++ b/src/libcamera/stream.cpp
>> @@ -418,12 +418,47 @@ Stream::Stream()
>> * \return A reference to the buffer pool
>> */
>>
>> +/**
>> + * \fn Stream::buffers()
>> + * \brief Retrieve the stream's buffers
>> + * \return The list of stream's buffers
>> + */
>> +
>> +/**
>> + * \fn Stream::bufferCount()
>> + * \brief Retrieve the number of buffers in the stream
>> + * \return The number of stream buffers
>> + */
>> +
>> /**
>> * \fn Stream::configuration()
>> * \brief Retrieve the active configuration of the stream
>> * \return The active configuration of the stream
>> */
>>
>> +/**
>> + * \brief Create buffers for the stream
>> + * \param count The number of buffers to create
>> + *
>> + * Create \a count empty buffers in the Stream's buffer pool.
>> + */
>> +void Stream::createBuffers(unsigned int count)
>> +{
>> + destroyBuffers();
>> + if (count == 0)
>> + return;
>> +
>> + bufferPool_.createBuffers(count);
>> +}
>> +
>> +/**
>> + * \brief Destroy buffers in the stream
>> + */
>> +void Stream::destroyBuffers()
>> +{
>> + bufferPool_.destroyBuffers();
>> +}
>> +
>> /**
>> * \var Stream::bufferPool_
>> * \brief The pool of buffers associated with the stream
>> diff --git a/src/qcam/main_window.cpp b/src/qcam/main_window.cpp
>> index 16b123132dd9..a0703b322c16 100644
>> --- a/src/qcam/main_window.cpp
>> +++ b/src/qcam/main_window.cpp
>> @@ -122,10 +122,8 @@ int MainWindow::startCapture()
>> return ret;
>> }
>>
>> - BufferPool &pool = stream->bufferPool();
>> std::vector<Request *> requests;
>> -
>> - for (Buffer &buffer : pool.buffers()) {
>> + for (Buffer &buffer : stream->buffers()) {
>> Request *request = camera_->createRequest();
>> if (!request) {
>> std::cerr << "Can't create request" << std::endl;
>> diff --git a/test/camera/capture.cpp b/test/camera/capture.cpp
>> index 7ce247cc482d..a0385ec2c74d 100644
>> --- a/test/camera/capture.cpp
>> +++ b/test/camera/capture.cpp
>> @@ -76,9 +76,8 @@ protected:
>> }
>>
>> Stream *stream = cfg.stream();
>> - BufferPool &pool = stream->bufferPool();
>> std::vector<Request *> requests;
>> - for (Buffer &buffer : pool.buffers()) {
>> + for (Buffer &buffer : stream->buffers()) {
>> Request *request = camera_->createRequest();
>> if (!request) {
>> cout << "Failed to create request" << endl;
>> diff --git a/test/camera/statemachine.cpp b/test/camera/statemachine.cpp
>> index 84d2a6fab5f0..c23455b5bb21 100644
>> --- a/test/camera/statemachine.cpp
>> +++ b/test/camera/statemachine.cpp
>> @@ -211,8 +211,7 @@ protected:
>> return TestFail;
>>
>> Stream *stream = *camera_->streams().begin();
>> - BufferPool &pool = stream->bufferPool();
>> - Buffer &buffer = pool.buffers().front();
>> + Buffer &buffer = stream->buffers().front();
Hrm.. I do like the readability of getting 'Stream Buffers' like that
though ...
>> std::map<Stream *, Buffer *> map = { { stream, &buffer } };
>> if (request->setBuffers(map))
>> return TestFail;
>> --
>> 2.21.0
>>
>> _______________________________________________
>> 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