[libcamera-devel] [PATCH v3 2/4] libcamera: framebuffer_allocator: Make allocate() require count
Nícolas F. R. A. Prado
nfraprado at collabora.com
Mon Apr 26 19:40:07 CEST 2021
Em 2021-04-25 23:59, Laurent Pinchart escreveu:
> Hi Nícolas,
>
> Thank you for the patch.
>
> On Wed, Apr 21, 2021 at 01:51:37PM -0300, Nícolas F. R. A. Prado wrote:
> > Make FrameBufferAllocator::allocate() require a 'count' argument for the
> > amount of buffers to be allocated.
> >
> > Signed-off-by: Nícolas F. R. A. Prado <nfraprado at collabora.com>
> > ---
> > include/libcamera/camera.h | 2 +-
> > include/libcamera/framebuffer_allocator.h | 2 +-
> > include/libcamera/internal/pipeline_handler.h | 2 +-
> > src/cam/capture.cpp | 10 ++++------
> > src/gstreamer/gstlibcameraallocator.cpp | 4 +++-
> > src/lc-compliance/simple_capture.cpp | 13 +++++++++++--
> > src/lc-compliance/simple_capture.h | 1 +
> > src/lc-compliance/single_stream.cpp | 6 ++++++
> > src/libcamera/camera.cpp | 4 ++--
> > src/libcamera/framebuffer_allocator.cpp | 5 +++--
> > src/libcamera/pipeline/ipu3/ipu3.cpp | 4 ++--
> > src/libcamera/pipeline/raspberrypi/raspberrypi.cpp | 4 ++--
> > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 4 ++--
> > src/libcamera/pipeline/simple/simple.cpp | 4 ++--
> > src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 7 ++++---
> > src/libcamera/pipeline/vimc/vimc.cpp | 7 ++++---
> > src/libcamera/pipeline_handler.cpp | 1 +
> > src/qcam/main_window.cpp | 4 +++-
> > src/v4l2/v4l2_camera.cpp | 2 +-
> > test/camera/capture.cpp | 4 +++-
> > test/camera/statemachine.cpp | 4 +++-
> > test/mapped-buffer.cpp | 4 +++-
> > 22 files changed, 63 insertions(+), 35 deletions(-)
> >
> > diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h
> > index d71641805c0a..656cd92aecde 100644
> > --- a/include/libcamera/camera.h
> > +++ b/include/libcamera/camera.h
> > @@ -115,7 +115,7 @@ private:
> > void requestComplete(Request *request);
> >
> > friend class FrameBufferAllocator;
> > - int exportFrameBuffers(Stream *stream,
> > + int exportFrameBuffers(Stream *stream, unsigned int count,
> > std::vector<std::unique_ptr<FrameBuffer>> *buffers);
> > };
> >
> > diff --git a/include/libcamera/framebuffer_allocator.h b/include/libcamera/framebuffer_allocator.h
> > index 0c85631a1da2..f1ae37288d50 100644
> > --- a/include/libcamera/framebuffer_allocator.h
> > +++ b/include/libcamera/framebuffer_allocator.h
> > @@ -25,7 +25,7 @@ public:
> > FrameBufferAllocator(std::shared_ptr<Camera> camera);
> > ~FrameBufferAllocator();
> >
> > - int allocate(Stream *stream);
> > + int allocate(Stream *stream, unsigned int count);
> > int free(Stream *stream);
> >
> > bool allocated() const { return !buffers_.empty(); }
> > diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h
> > index c6454db6b2c4..dc04ba041fe5 100644
> > --- a/include/libcamera/internal/pipeline_handler.h
> > +++ b/include/libcamera/internal/pipeline_handler.h
> > @@ -75,7 +75,7 @@ public:
> > const StreamRoles &roles) = 0;
> > virtual int configure(Camera *camera, CameraConfiguration *config) = 0;
> >
> > - virtual int exportFrameBuffers(Camera *camera, Stream *stream,
> > + virtual int exportFrameBuffers(Camera *camera, Stream *stream, unsigned int count,
> > std::vector<std::unique_ptr<FrameBuffer>> *buffers) = 0;
> >
> > virtual int start(Camera *camera, const ControlList *controls) = 0;
> > diff --git a/src/cam/capture.cpp b/src/cam/capture.cpp
> > index 7b55fc677022..8ad121f5adc8 100644
> > --- a/src/cam/capture.cpp
> > +++ b/src/cam/capture.cpp
> > @@ -10,6 +10,8 @@
> > #include <limits.h>
> > #include <sstream>
> >
> > +#include <libcamera/property_ids.h>
> > +
> > #include "capture.h"
> > #include "main.h"
> >
> > @@ -77,17 +79,13 @@ int Capture::capture(FrameBufferAllocator *allocator)
> > {
> > int ret;
> >
> > - /* Identify the stream with the least number of buffers. */
> > - unsigned int nbuffers = UINT_MAX;
> > + unsigned int nbuffers = camera_->properties().get(properties::QueueDepth);
> > for (StreamConfiguration &cfg : *config_) {
> > - ret = allocator->allocate(cfg.stream());
> > + ret = allocator->allocate(cfg.stream(), nbuffers);
> > if (ret < 0) {
> > std::cerr << "Can't allocate buffers" << std::endl;
> > return -ENOMEM;
> > }
> > -
> > - unsigned int allocated = allocator->buffers(cfg.stream()).size();
> > - nbuffers = std::min(nbuffers, allocated);
>
> If not enough memory is available to allocate nbuffers, or if a driver
> decides that the number is too high, we could receive less buffers than
> requested. We would then crash below, when creating the requests. I
> think we still need to handle this case.
Hm, but doesn't V4L2VideoDevice::requestBuffers() catch that in
if (rb.count < count) {
LOG(V4L2, Error)
<< "Not enough buffers provided by V4L2VideoDevice";
requestBuffers(0, memoryType);
return -ENOMEM;
}
? That return value is returned all the way up which means Capture::capture()
will return as well.
Not so much related to this but I just realized it: Isn't it a problem that the
QueueDepth is a property global to the camera, instead of specific to a stream?
>
> > }
> >
> > /*
> > diff --git a/src/gstreamer/gstlibcameraallocator.cpp b/src/gstreamer/gstlibcameraallocator.cpp
> > index 7bd8ba2db71d..e7a5b5a27226 100644
> > --- a/src/gstreamer/gstlibcameraallocator.cpp
> > +++ b/src/gstreamer/gstlibcameraallocator.cpp
> > @@ -10,6 +10,7 @@
> >
> > #include <libcamera/camera.h>
> > #include <libcamera/framebuffer_allocator.h>
> > +#include <libcamera/property_ids.h>
> > #include <libcamera/stream.h>
> >
> > #include "gstlibcamera-utils.h"
> > @@ -188,13 +189,14 @@ gst_libcamera_allocator_new(std::shared_ptr<Camera> camera,
> > {
> > auto *self = GST_LIBCAMERA_ALLOCATOR(g_object_new(GST_TYPE_LIBCAMERA_ALLOCATOR,
> > nullptr));
> > + int bufferCount = camera->properties().get(properties::QueueDepth);
>
> It's an unsigned int above, should it be unsigned here too ? Same for
> lc-compliance and other places below.
Sure. I didn't use unsigned int because int32_t in the property_ids.yaml sounds like
signed. I'll change everything to unsigned.
>
> >
> > self->fb_allocator = new FrameBufferAllocator(camera);
> > for (StreamConfiguration &streamCfg : *config_) {
> > Stream *stream = streamCfg.stream();
> > gint ret;
> >
> > - ret = self->fb_allocator->allocate(stream);
> > + ret = self->fb_allocator->allocate(stream, bufferCount);
> > if (ret == 0)
> > return nullptr;
> >
> > diff --git a/src/lc-compliance/simple_capture.cpp b/src/lc-compliance/simple_capture.cpp
> > index 64e862a08e3a..875772a80c27 100644
> > --- a/src/lc-compliance/simple_capture.cpp
> > +++ b/src/lc-compliance/simple_capture.cpp
> > @@ -5,6 +5,8 @@
> > * simple_capture.cpp - Simple capture helper
> > */
> >
> > +#include <libcamera/property_ids.h>
> > +
> > #include "simple_capture.h"
> >
> > using namespace libcamera;
> > @@ -39,12 +41,19 @@ Results::Result SimpleCapture::configure(StreamRole role)
> > return { Results::Pass, "Configure camera" };
> > }
> >
> > -Results::Result SimpleCapture::start()
> > +Results::Result SimpleCapture::allocateBuffers()
> > {
> > + int minBuffers = camera_->properties().get(properties::QueueDepth);
>
> Depending on how you decide to document and name QueueDepth (see the
> review of 1/4), this variable may be better named numBuffers (or
> something else).
>
> > Stream *stream = config_->at(0).stream();
> > - if (allocator_->allocate(stream) < 0)
> > +
> > + if (allocator_->allocate(stream, minBuffers) < 0)
> > return { Results::Fail, "Failed to allocate buffers" };
> >
> > + return { Results::Pass, "Allocated buffers" };
>
> Do we need a message when returning Pass ?
Well, the Pass message gets used to report which test passed, although since
this is only part of the test it doesn't get used. In any case the return type
requires a string, so at least an empty string should be returned (but then,
might as well give a meaningful message).
Thanks,
Nícolas
More information about the libcamera-devel
mailing list