[libcamera-devel] [RFC PATCH] libcamera, cam, qcam: Reuse Request
Niklas Söderlund
niklas.soderlund at ragnatech.se
Thu Sep 24 14:09:51 CEST 2020
Hi Paul,
Thanks for your patch.
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();
>
> /*
> * 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?
>
> 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_;
> }
>
> +void Request::reset()
> +{
> + bufferMap_.clear();
> + pending_.clear();
> +
> + 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;
> +
> 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 });
> }
>
> 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_);
> }
>
> -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_ */
> --
> 2.27.0
>
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
--
Regards,
Niklas Söderlund
More information about the libcamera-devel
mailing list