[libcamera-devel] [RFC PATCH] libcamera, cam, qcam: Reuse Request
paul.elder at ideasonboard.com
paul.elder at ideasonboard.com
Fri Sep 25 04:20:38 CEST 2020
Hello,
On Thu, Sep 24, 2020 at 03:40:56PM +0300, Laurent Pinchart wrote:
> Hello,
>
> On Thu, Sep 24, 2020 at 02:09:51PM +0200, Niklas Söderlund wrote:
> > On 2020-09-23 21:14:48 +0900, Paul Elder wrote:
> > > Allow reuse of the Request object by implementing a reset() function.
> > > This means that the applications now have the responsibility of freeing
> > > the the Request objects, so make cam and qcam do so.
> > >
> > > Signed-off-by: Paul Elder <paul.elder at ideasonboard.com>
> > >
> > > ---
> > > In this RFC, I've only made qcam and cam reuse the Request object. Just
> > > gauging the direction, and then I'll put it in for andoird and gstreamer
> > > and v4l2-compat as well.
> > > ---
> > > include/libcamera/request.h | 2 ++
> > > src/cam/capture.cpp | 20 ++++++++++----------
> > > src/cam/capture.h | 3 +++
> > > src/libcamera/camera.cpp | 4 +---
> > > src/libcamera/request.cpp | 9 +++++++++
> > > src/qcam/main_window.cpp | 26 ++++++++++++--------------
> > > src/qcam/main_window.h | 14 ++++++++++----
> > > src/qcam/viewfinder.h | 4 +++-
> > > src/qcam/viewfinder_gl.cpp | 9 +++++----
> > > src/qcam/viewfinder_gl.h | 5 +++--
> > > src/qcam/viewfinder_qt.cpp | 10 ++++++----
> > > src/qcam/viewfinder_qt.h | 8 ++++++--
> > > 12 files changed, 70 insertions(+), 44 deletions(-)
> > >
> > > diff --git a/include/libcamera/request.h b/include/libcamera/request.h
> > > index 5976ac50..9f615a24 100644
> > > --- a/include/libcamera/request.h
> > > +++ b/include/libcamera/request.h
> > > @@ -38,6 +38,8 @@ public:
> > > Request &operator=(const Request &) = delete;
> > > ~Request();
> > >
> > > + void reset();
> > > +
> > > ControlList &controls() { return *controls_; }
> > > ControlList &metadata() { return *metadata_; }
> > > const BufferMap &buffers() const { return bufferMap_; }
> > > diff --git a/src/cam/capture.cpp b/src/cam/capture.cpp
> > > index 5510c009..fe9d591a 100644
> > > --- a/src/cam/capture.cpp
> > > +++ b/src/cam/capture.cpp
> > > @@ -65,6 +65,9 @@ int Capture::run(const OptionsParser::Options &options)
> > > writer_ = nullptr;
> > > }
> > >
> > > + for (Request *req : requests_)
> > > + delete req;
> > > +
> > > delete allocator;
> > >
> > > return ret;
> > > @@ -92,7 +95,6 @@ int Capture::capture(FrameBufferAllocator *allocator)
> > > * example pushing a button. For now run all streams all the time.
> > > */
> > >
> > > - std::vector<Request *> requests;
> > > for (unsigned int i = 0; i < nbuffers; i++) {
> > > Request *request = camera_->createRequest();
> > > if (!request) {
> > > @@ -117,7 +119,7 @@ int Capture::capture(FrameBufferAllocator *allocator)
> > > writer_->mapBuffer(buffer.get());
> > > }
> > >
> > > - requests.push_back(request);
> > > + requests_.push_back(request);
> > > }
> > >
> > > ret = camera_->start();
> > > @@ -126,7 +128,7 @@ int Capture::capture(FrameBufferAllocator *allocator)
> > > return ret;
> > > }
> > >
> > > - for (Request *request : requests) {
> > > + for (Request *request : requests_) {
> > > ret = camera_->queueRequest(request);
> > > if (ret < 0) {
> > > std::cerr << "Can't queue request" << std::endl;
> > > @@ -153,10 +155,12 @@ int Capture::capture(FrameBufferAllocator *allocator)
> > >
> > > void Capture::requestComplete(Request *request)
> > > {
> > > - if (request->status() == Request::RequestCancelled)
> > > + if (request->status() == Request::RequestCancelled) {
> > > + request->reset();
> > > return;
> > > + }
> > >
> > > - const Request::BufferMap &buffers = request->buffers();
> > > + const Request::BufferMap buffers = request->buffers();
>
> A copy isn't very nice, but we can live with it for now.
Without the copy, all the buffers become invalid upon reset :/
> > >
> > > /*
> > > * Compute the frame rate. The timestamp is arbitrarily retrieved from
> > > @@ -206,11 +210,7 @@ void Capture::requestComplete(Request *request)
> > > * Create a new request and populate it with one buffer for each
> > > * stream.
> > > */
> > > - request = camera_->createRequest();
> > > - if (!request) {
> > > - std::cerr << "Can't create request" << std::endl;
> > > - return;
> > > - }
> > > + request->reset();
> >
> > Hum, is this really "reusing" the request object? I imagined a reused
> > request would retain its buffers while gaining the ability to be be
> > requeued to the Camera after it have completed. With this patch we still
> > need to keep the logic to fill the request with buffers each iteration,
> > is there some advantage to this I'm missing?
>
> Two advantages, we don't free and reallocate the request needlessly, and
> it can be stored in the application and used after the request handler
> completes.
>
> I don't think retaining buffers by default is a good idea, in many cases
> we'll have a different number of buffers that can be cycled through than
> the number of allocated requests.
>
> > >
> > > for (auto it = buffers.begin(); it != buffers.end(); ++it) {
> > > const Stream *stream = it->first;
> > > diff --git a/src/cam/capture.h b/src/cam/capture.h
> > > index 0aebdac9..62cf9bf1 100644
> > > --- a/src/cam/capture.h
> > > +++ b/src/cam/capture.h
> > > @@ -9,6 +9,7 @@
> > >
> > > #include <memory>
> > > #include <stdint.h>
> > > +#include <vector>
> > >
> > > #include <libcamera/buffer.h>
> > > #include <libcamera/camera.h>
> > > @@ -43,6 +44,8 @@ private:
> > > EventLoop *loop_;
> > > unsigned int captureCount_;
> > > unsigned int captureLimit_;
> > > +
> > > + std::vector<libcamera::Request *> requests_;
> > > };
> > >
> > > #endif /* __CAM_CAPTURE_H__ */
> > > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp
> > > index ae16a64a..4cc68bce 100644
> > > --- a/src/libcamera/camera.cpp
> > > +++ b/src/libcamera/camera.cpp
> > > @@ -974,13 +974,11 @@ int Camera::stop()
> > > * \param[in] request The request that has completed
> > > *
> > > * This function is called by the pipeline handler to notify the camera that
> > > - * the request has completed. It emits the requestCompleted signal and deletes
> > > - * the request.
> > > + * the request has completed. It emits the requestCompleted signal.
> > > */
> > > void Camera::requestComplete(Request *request)
> > > {
> > > requestCompleted.emit(request);
> > > - delete request;
> > > }
> > >
> > > } /* namespace libcamera */
> > > diff --git a/src/libcamera/request.cpp b/src/libcamera/request.cpp
> > > index 60b30692..606866a6 100644
> > > --- a/src/libcamera/request.cpp
> > > +++ b/src/libcamera/request.cpp
> > > @@ -85,6 +85,15 @@ Request::~Request()
> > > delete validator_;
> > > }
> > >
>
> No documentation ? :-) You should also update the documentation of
Not yet :p
> Camera::queueRequest(). Thinking about it, I think we should enforce
> ownership of requests by the application by returning a
> std::unique_ptr<Request> from Camera::createRequest(). We may even want
> to drop Camera::createRequest() completely, but that can be done in a
> separate patch.
Yeah... but I thought there was a problem when the Request completes,
since there could be multiple slots, and they all can't receive unique
pointers.
On the other hand... it's only one application... could we require that
the application only connect one slot to the Request completion handler,
and then the application can split it among multiple sub-handlers if it
wants to? Then we can keep unique_ptr for the Request, and the ownership
can be clear.
> > > +void Request::reset()
> > > +{
> > > + bufferMap_.clear();
> > > + pending_.clear();
>
> You need to clear controls_ and metadata_ too.
Oh okay, those can't be reused?
> > > +
> > > + status_ = RequestPending;
> > > + cancelled_ = false;
> > > +}
> > > +
> > > /**
> > > * \fn Request::controls()
> > > * \brief Retrieve the request's ControlList
> > > diff --git a/src/qcam/main_window.cpp b/src/qcam/main_window.cpp
> > > index 985743f3..1531b44f 100644
> > > --- a/src/qcam/main_window.cpp
> > > +++ b/src/qcam/main_window.cpp
> > > @@ -367,7 +367,6 @@ void MainWindow::toggleCapture(bool start)
> > > int MainWindow::startCapture()
> > > {
> > > StreamRoles roles = StreamKeyValueParser::roles(options_[OptStream]);
> > > - std::vector<Request *> requests;
> > > int ret;
> > >
> > > /* Verify roles are supported. */
> > > @@ -499,7 +498,7 @@ int MainWindow::startCapture()
> > > goto error;
> > > }
> > >
> > > - requests.push_back(request);
> > > + requests_.push_back(request);
> > > }
> > >
> > > /* Start the title timer and the camera. */
> > > @@ -518,7 +517,7 @@ int MainWindow::startCapture()
> > > camera_->requestCompleted.connect(this, &MainWindow::requestComplete);
> > >
> > > /* Queue all requests. */
> > > - for (Request *request : requests) {
> > > + for (Request *request : requests_) {
> > > ret = camera_->queueRequest(request);
> > > if (ret < 0) {
> > > qWarning() << "Can't queue request";
> > > @@ -535,7 +534,7 @@ error_disconnect:
> > > camera_->stop();
> > >
> > > error:
> > > - for (Request *request : requests)
> > > + for (Request *request : requests_)
> > > delete request;
> > >
> > > for (auto &iter : mappedBuffers_) {
> > > @@ -580,6 +579,9 @@ void MainWindow::stopCapture()
> > > }
> > > mappedBuffers_.clear();
> > >
> > > + for (Request *req : requests_)
> > > + delete req;
>
> Have you tried stopping and restarting capture ? You delete all the
> requests here, but the vector still contains the pointers. When capture
> is restarted I expect issue. A
>
> requests_.clear();
Ah, right.
> is needed, and I would also turn it into a
> std::vector<std::unique_ptr<Request>> to avoid the manual delete loops.
> Same thing for the cam application (even if in that case capture can't
> be stopped and restarted at the moment, but let's still do it right).
>
> > > +
> > > delete allocator_;
> > >
> > > isCapturing_ = false;
> > > @@ -701,7 +703,7 @@ void MainWindow::requestComplete(Request *request)
> > > */
> > > {
> > > QMutexLocker locker(&mutex_);
> > > - doneQueue_.enqueue({ request->buffers(), request->metadata() });
> > > + doneQueue_.enqueue({ request->buffers(), request->metadata(), request });
>
> You aobut enqueuing the request only ? The reason to enqueue a copy of
> buffers() and metadata() was because the request was deleted right after
> this function returns, now that it's not the case anymore, we can just
> pass the request around. The MainWindow::CaptureRequest class can be
> removed.
ack
> > > }
> > >
> > > QCoreApplication::postEvent(this, new CaptureEvent);
> > > @@ -726,13 +728,13 @@ void MainWindow::processCapture()
> > >
> > > /* Process buffers. */
> > > if (request.buffers_.count(vfStream_))
> > > - processViewfinder(request.buffers_[vfStream_]);
> > > + processViewfinder(request.request_, request.buffers_[vfStream_]);
> > >
> > > if (request.buffers_.count(rawStream_))
> > > processRaw(request.buffers_[rawStream_], request.metadata_);
>
> Instead of passing the request to processViewfinder(), how about adding
> it to a free list here, and taking a request from the free list in
> MainWindow::queueRequest() ? That would avoid the changes to the
> viewfinder.
ack
> > > }
> > >
> > > -void MainWindow::processViewfinder(FrameBuffer *buffer)
> > > +void MainWindow::processViewfinder(Request *request, FrameBuffer *buffer)
> > > {
> > > framesCaptured_++;
> > >
> > > @@ -749,16 +751,12 @@ void MainWindow::processViewfinder(FrameBuffer *buffer)
> > > << "fps:" << Qt::fixed << qSetRealNumberPrecision(2) << fps;
> > >
> > > /* Render the frame on the viewfinder. */
> > > - viewfinder_->render(buffer, &mappedBuffers_[buffer]);
> > > + viewfinder_->render(request, buffer, &mappedBuffers_[buffer]);
> > > }
> > >
> > > -void MainWindow::queueRequest(FrameBuffer *buffer)
> > > +void MainWindow::queueRequest(Request *request, FrameBuffer *buffer)
> > > {
> > > - Request *request = camera_->createRequest();
> > > - if (!request) {
> > > - qWarning() << "Can't create request";
> > > - return;
> > > - }
> > > + request->reset();
> > >
> > > request->addBuffer(vfStream_, buffer);
> > >
> > > diff --git a/src/qcam/main_window.h b/src/qcam/main_window.h
> > > index 5c61a4df..83535373 100644
> > > --- a/src/qcam/main_window.h
> > > +++ b/src/qcam/main_window.h
> > > @@ -8,6 +8,7 @@
> > > #define __QCAM_MAIN_WINDOW_H__
> > >
> > > #include <memory>
> > > +#include <vector>
> > >
> > > #include <QElapsedTimer>
> > > #include <QIcon>
> > > @@ -22,6 +23,7 @@
> > > #include <libcamera/camera_manager.h>
> > > #include <libcamera/controls.h>
> > > #include <libcamera/framebuffer_allocator.h>
> > > +#include <libcamera/request.h>
> > > #include <libcamera/stream.h>
> > >
> > > #include "../cam/stream_options.h"
> > > @@ -49,13 +51,15 @@ public:
> > > }
> > >
> > > CaptureRequest(const Request::BufferMap &buffers,
> > > - const ControlList &metadata)
> > > - : buffers_(buffers), metadata_(metadata)
> > > + const ControlList &metadata,
> > > + Request * const request)
> > > + : buffers_(buffers), metadata_(metadata), request_(request)
> > > {
> > > }
> > >
> > > Request::BufferMap buffers_;
> > > ControlList metadata_;
> > > + Request *request_;
> > > };
> > >
> > > class MainWindow : public QMainWindow
> > > @@ -79,7 +83,7 @@ private Q_SLOTS:
> > > void captureRaw();
> > > void processRaw(FrameBuffer *buffer, const ControlList &metadata);
> > >
> > > - void queueRequest(FrameBuffer *buffer);
> > > + void queueRequest(Request *request, FrameBuffer *buffer);
> > >
> > > private:
> > > int createToolbars();
> > > @@ -96,7 +100,7 @@ private:
> > > void requestComplete(Request *request);
> > > void processCapture();
> > > void processHotplug(HotplugEvent *e);
> > > - void processViewfinder(FrameBuffer *buffer);
> > > + void processViewfinder(Request *request, FrameBuffer *buffer);
> > >
> > > /* UI elements */
> > > QToolBar *toolbar_;
> > > @@ -135,6 +139,8 @@ private:
> > > QElapsedTimer frameRateInterval_;
> > > uint32_t previousFrames_;
> > > uint32_t framesCaptured_;
> > > +
> > > + std::vector<Request *> requests_;
> > > };
> > >
> > > #endif /* __QCAM_MAIN_WINDOW__ */
> > > diff --git a/src/qcam/viewfinder.h b/src/qcam/viewfinder.h
> > > index 67da1df2..26c20f1b 100644
> > > --- a/src/qcam/viewfinder.h
> > > +++ b/src/qcam/viewfinder.h
> > > @@ -27,7 +27,9 @@ public:
> > > virtual const QList<libcamera::PixelFormat> &nativeFormats() const = 0;
> > >
> > > virtual int setFormat(const libcamera::PixelFormat &format, const QSize &size) = 0;
> > > - virtual void render(libcamera::FrameBuffer *buffer, MappedBuffer *map) = 0;
> > > + virtual void render(libcamera::Request *request,
> > > + libcamera::FrameBuffer *buffer,
> > > + MappedBuffer *map) = 0;
> > > virtual void stop() = 0;
> > >
> > > virtual QImage getCurrentImage() = 0;
> > > diff --git a/src/qcam/viewfinder_gl.cpp b/src/qcam/viewfinder_gl.cpp
> > > index fbe21dcf..c65b41f6 100644
> > > --- a/src/qcam/viewfinder_gl.cpp
> > > +++ b/src/qcam/viewfinder_gl.cpp
> > > @@ -23,7 +23,7 @@ static const QList<libcamera::PixelFormat> supportedFormats{
> > > };
> > >
> > > ViewFinderGL::ViewFinderGL(QWidget *parent)
> > > - : QOpenGLWidget(parent), buffer_(nullptr), yuvData_(nullptr),
> > > + : QOpenGLWidget(parent), request_(nullptr), buffer_(nullptr), yuvData_(nullptr),
> > > fragmentShader_(nullptr), vertexShader_(nullptr),
> > > vertexBuffer_(QOpenGLBuffer::VertexBuffer),
> > > textureU_(QOpenGLTexture::Target2D),
> > > @@ -67,7 +67,7 @@ int ViewFinderGL::setFormat(const libcamera::PixelFormat &format,
> > > void ViewFinderGL::stop()
> > > {
> > > if (buffer_) {
> > > - renderComplete(buffer_);
> > > + renderComplete(request_, buffer_);
> > > buffer_ = nullptr;
> > > }
> > > }
> > > @@ -79,7 +79,7 @@ QImage ViewFinderGL::getCurrentImage()
> > > return grabFramebuffer();
> > > }
> > >
> > > -void ViewFinderGL::render(libcamera::FrameBuffer *buffer, MappedBuffer *map)
> > > +void ViewFinderGL::render(libcamera::Request *request, libcamera::FrameBuffer *buffer, MappedBuffer *map)
> > > {
> > > if (buffer->planes().size() != 1) {
> > > qWarning() << "Multi-planar buffers are not supported";
> > > @@ -87,11 +87,12 @@ void ViewFinderGL::render(libcamera::FrameBuffer *buffer, MappedBuffer *map)
> > > }
> > >
> > > if (buffer_)
> > > - renderComplete(buffer_);
> > > + renderComplete(request_, buffer_);
> > >
> > > yuvData_ = static_cast<unsigned char *>(map->memory);
> > > update();
> > > buffer_ = buffer;
> > > + request_ = request;
> > > }
> > >
> > > bool ViewFinderGL::selectFormat(const libcamera::PixelFormat &format)
> > > diff --git a/src/qcam/viewfinder_gl.h b/src/qcam/viewfinder_gl.h
> > > index 69502b7a..7db41841 100644
> > > --- a/src/qcam/viewfinder_gl.h
> > > +++ b/src/qcam/viewfinder_gl.h
> > > @@ -36,13 +36,13 @@ public:
> > > const QList<libcamera::PixelFormat> &nativeFormats() const override;
> > >
> > > int setFormat(const libcamera::PixelFormat &format, const QSize &size) override;
> > > - void render(libcamera::FrameBuffer *buffer, MappedBuffer *map) override;
> > > + void render(libcamera::Request *request, libcamera::FrameBuffer *buffer, MappedBuffer *map) override;
> > > void stop() override;
> > >
> > > QImage getCurrentImage() override;
> > >
> > > Q_SIGNALS:
> > > - void renderComplete(libcamera::FrameBuffer *buffer);
> > > + void renderComplete(libcamera::Request *request, libcamera::FrameBuffer *buffer);
> > >
> > > protected:
> > > void initializeGL() override;
> > > @@ -60,6 +60,7 @@ private:
> > > void doRender();
> > >
> > > /* Captured image size, format and buffer */
> > > + libcamera::Request *request_;
> > > libcamera::FrameBuffer *buffer_;
> > > libcamera::PixelFormat format_;
> > > QSize size_;
> > > diff --git a/src/qcam/viewfinder_qt.cpp b/src/qcam/viewfinder_qt.cpp
> > > index e436714c..7f9f913b 100644
> > > --- a/src/qcam/viewfinder_qt.cpp
> > > +++ b/src/qcam/viewfinder_qt.cpp
> > > @@ -34,7 +34,7 @@ static const QMap<libcamera::PixelFormat, QImage::Format> nativeFormats
> > > };
> > >
> > > ViewFinderQt::ViewFinderQt(QWidget *parent)
> > > - : QWidget(parent), buffer_(nullptr)
> > > + : QWidget(parent), request_(nullptr), buffer_(nullptr)
> > > {
> > > icon_ = QIcon(":camera-off.svg");
> > > }
> > > @@ -78,7 +78,8 @@ int ViewFinderQt::setFormat(const libcamera::PixelFormat &format,
> > > return 0;
> > > }
> > >
> > > -void ViewFinderQt::render(libcamera::FrameBuffer *buffer, MappedBuffer *map)
> > > +void ViewFinderQt::render(libcamera::Request *request,
> > > + libcamera::FrameBuffer *buffer, MappedBuffer *map)
> > > {
> > > if (buffer->planes().size() != 1) {
> > > qWarning() << "Multi-planar buffers are not supported";
> > > @@ -106,6 +107,7 @@ void ViewFinderQt::render(libcamera::FrameBuffer *buffer, MappedBuffer *map)
> > > size / size_.height(),
> > > ::nativeFormats[format_]);
> > > std::swap(buffer, buffer_);
> > > + std::swap(request, request_);
> > > } else {
> > > /*
> > > * Otherwise, convert the format and release the frame
> > > @@ -118,7 +120,7 @@ void ViewFinderQt::render(libcamera::FrameBuffer *buffer, MappedBuffer *map)
> > > update();
> > >
> > > if (buffer)
> > > - renderComplete(buffer);
> > > + renderComplete(request, buffer);
> > > }
> > >
> > > void ViewFinderQt::stop()
> > > @@ -126,7 +128,7 @@ void ViewFinderQt::stop()
> > > image_ = QImage();
> > >
> > > if (buffer_) {
> > > - renderComplete(buffer_);
> > > + renderComplete(request_, buffer_);
> > > buffer_ = nullptr;
> > > }
> > >
> > > diff --git a/src/qcam/viewfinder_qt.h b/src/qcam/viewfinder_qt.h
> > > index d7554288..5f62d7f6 100644
> > > --- a/src/qcam/viewfinder_qt.h
> > > +++ b/src/qcam/viewfinder_qt.h
> > > @@ -17,6 +17,7 @@
> > > #include <libcamera/buffer.h>
> > > #include <libcamera/formats.h>
> > > #include <libcamera/pixel_format.h>
> > > +#include <libcamera/request.h>
> > >
> > > #include "format_converter.h"
> > > #include "viewfinder.h"
> > > @@ -32,13 +33,15 @@ public:
> > > const QList<libcamera::PixelFormat> &nativeFormats() const override;
> > >
> > > int setFormat(const libcamera::PixelFormat &format, const QSize &size) override;
> > > - void render(libcamera::FrameBuffer *buffer, MappedBuffer *map) override;
> > > + void render(libcamera::Request *request,
> > > + libcamera::FrameBuffer *buffer,
> > > + MappedBuffer *map) override;
> > > void stop() override;
> > >
> > > QImage getCurrentImage() override;
> > >
> > > Q_SIGNALS:
> > > - void renderComplete(libcamera::FrameBuffer *buffer);
> > > + void renderComplete(libcamera::Request *request, libcamera::FrameBuffer *buffer);
> > >
> > > protected:
> > > void paintEvent(QPaintEvent *) override;
> > > @@ -56,6 +59,7 @@ private:
> > > QPixmap pixmap_;
> > >
> > > /* Buffer and render image */
> > > + libcamera::Request *request_;
> > > libcamera::FrameBuffer *buffer_;
> > > QImage image_;
> > > QMutex mutex_; /* Prevent concurrent access to image_ */
>
> --
> Regards,
>
> Laurent Pinchart
More information about the libcamera-devel
mailing list