[libcamera-devel] [PATCH v2] libcamera, android, cam, gstreamer, qcam, v4l2: Reuse Request
paul.elder at ideasonboard.com
paul.elder at ideasonboard.com
Wed Sep 30 11:59:08 CEST 2020
- Previous message (by thread): [libcamera-devel] [PATCH v2] libcamera, android, cam, gstreamer, qcam, v4l2: Reuse Request
- Next message (by thread): [libcamera-devel] [PATCH v2] libcamera, android, cam, gstreamer, qcam, v4l2: Reuse Request
- Messages sorted by:
[ date ]
[ thread ]
[ subject ]
[ author ]
On Wed, Sep 30, 2020 at 06:55:07PM +0900, paul.elder at ideasonboard.com wrote:
> Hi Niklas,
>
> Thank you for the review.
>
> On Wed, Sep 30, 2020 at 12:46:51AM +0200, Niklas Söderlund wrote:
> > Hi Paul,
> >
> > Thanks for your work.
> >
> > On 2020-09-29 18:52:00 +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 Request objects, so make all libcamera users (cam, qcam,
> > > v4l2-compat, gstreamer, android) do so.
> > >
> > > Signed-off-by: Paul Elder <paul.elder at ideasonboard.com>
> > >
> > > ---
> > > Changes in v2:
> > > - clear controls_ and metadata_ and validator_ in Request::reset()
> > > - use unique_ptr on application side, prior to queueRequest, and use
> > > regular pointer for completion handler
> > > - make qcam's reuse request nicer
> > > - update Camera::queueRequest() and Camera::createRequest() documentation
> > > - add documentation for Request::reset()
> > > - make v4l2-compat reuse request
> > > - make gstreamer and android use the new createRequest API, though they
> > > do not actually reuse the requests
> > > - I haven't yet tested android
> > > ---
> > > include/libcamera/camera.h | 2 +-
> > > include/libcamera/request.h | 2 ++
> > > src/android/camera_device.cpp | 8 +++++-
> > > src/cam/capture.cpp | 19 ++++++------
> > > src/cam/capture.h | 3 ++
> > > src/gstreamer/gstlibcamerasrc.cpp | 6 ++--
> > > src/libcamera/camera.cpp | 15 ++++------
> > > src/libcamera/request.cpp | 24 ++++++++++++++++
> > > src/qcam/main_window.cpp | 48 +++++++++++++++++++------------
> > > src/qcam/main_window.h | 26 +++++------------
> > > src/v4l2/v4l2_camera.cpp | 38 +++++++++++++++++-------
> > > src/v4l2/v4l2_camera.h | 4 ++-
> > > test/camera/buffer_import.cpp | 15 +++++-----
> > > test/camera/capture.cpp | 15 +++++-----
> > > test/camera/statemachine.cpp | 9 ++----
> > > 15 files changed, 141 insertions(+), 93 deletions(-)
> > >
> > > diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h
> > > index 272c12c3..e4036b63 100644
> > > --- a/include/libcamera/camera.h
> > > +++ b/include/libcamera/camera.h
> > > @@ -93,7 +93,7 @@ public:
> > > std::unique_ptr<CameraConfiguration> generateConfiguration(const StreamRoles &roles = {});
> > > int configure(CameraConfiguration *config);
> > >
> > > - Request *createRequest(uint64_t cookie = 0);
> > > + std::unique_ptr<Request> createRequest(uint64_t cookie = 0);
> >
> > I like this.
>
> :)
>
> > > int queueRequest(Request *request);
> > >
> > > int start();
> > > 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/android/camera_device.cpp b/src/android/camera_device.cpp
> > > index 70d77a17..7f288617 100644
> > > --- a/src/android/camera_device.cpp
> > > +++ b/src/android/camera_device.cpp
> > > @@ -1395,8 +1395,13 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques
> > > new Camera3RequestDescriptor(camera3Request->frame_number,
> > > camera3Request->num_output_buffers);
> > >
> > > - Request *request =
> > > + std::unique_ptr<Request> r =
> > > camera_->createRequest(reinterpret_cast<uint64_t>(descriptor));
> > > + Request *request = r.release();
> > > + if (!request) {
> > > + LOG(HAL, Error) << "Failed to create request";
> > > + return -ENOMEM;
> > > + }
> >
> > I don't like this ;-)
>
> :(
>
> > Would it not make sens to leverage the unique_ptr?
>
> From what I could tell, android doesn't requeue the request in the
> request completion handler (CameraDevice::requestComplete()), which
> means that android uses the request transiently, like how request was
> meant to be used before (no reuse). So I kept it that way.
>
> Or do you mean that I should let the unique pointer die on its own by
> letting it go out of scope? Wouldn't that free the request its pointing
> to, and segfault somewhere between this and the completion handler?
>
> >
> > >
> > > for (unsigned int i = 0; i < descriptor->numBuffers; ++i) {
> > > CameraStream *cameraStream =
> > > @@ -1593,6 +1598,7 @@ void CameraDevice::requestComplete(Request *request)
> > > callbacks_->process_capture_result(callbacks_, &captureResult);
> > >
> > > delete descriptor;
> > > + delete request;
> > > }
> > >
> > > std::string CameraDevice::logPrefix() const
> > > diff --git a/src/cam/capture.cpp b/src/cam/capture.cpp
> > > index 5510c009..5b9238e2 100644
> > > --- a/src/cam/capture.cpp
> > > +++ b/src/cam/capture.cpp
> > > @@ -65,6 +65,8 @@ int Capture::run(const OptionsParser::Options &options)
> > > writer_ = nullptr;
> > > }
> > >
> > > + requests_.clear();
> > > +
> > > delete allocator;
> > >
> > > return ret;
> > > @@ -92,9 +94,8 @@ 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();
> > > + std::unique_ptr<Request> request = camera_->createRequest();
> > > if (!request) {
> > > std::cerr << "Can't create request" << std::endl;
> > > return -ENOMEM;
> > > @@ -117,7 +118,7 @@ int Capture::capture(FrameBufferAllocator *allocator)
> > > writer_->mapBuffer(buffer.get());
> > > }
> > >
> > > - requests.push_back(request);
> > > + requests_.push_back(std::move(request));
> > > }
> > >
> > > ret = camera_->start();
> > > @@ -126,8 +127,8 @@ int Capture::capture(FrameBufferAllocator *allocator)
> > > return ret;
> > > }
> > >
> > > - for (Request *request : requests) {
> > > - ret = camera_->queueRequest(request);
> > > + for (std::unique_ptr<Request> &request : requests_) {
> > > + ret = camera_->queueRequest(request.get());
> >
> > Would it not make sens to take this all the way and have
> > Camera::queueRequest() take a unique pointer that is returned in
> > requestComplete(). Then the application could store the unique ptr and
> > reuse it or let it expire and have the request deleted?
>
> The problem with queueRequest() taking a unique pointer is that the
> camera then owns the request... but it can't give ownership back to the
> application since the completion handler signal can have multiple slots.
> We can't give all of them a unique pointer, but if we give them a
> regular pointer, then whose responsibility is it for freeing the
> request? So we make the application keep ownership of the request. If it
> relinquishes ownership then it can free it at the end of the completion
> handler (like android and gstreamer does now).
>
> > > if (ret < 0) {
> > > std::cerr << "Can't queue request" << std::endl;
> > > camera_->stop();
> > > @@ -156,7 +157,7 @@ void Capture::requestComplete(Request *request)
> > > if (request->status() == Request::RequestCancelled)
> > > 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 +207,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();
> > >
> > > 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..45e5e8a9 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<std::unique_ptr<libcamera::Request>> requests_;
> > > };
> > >
> > > #endif /* __CAM_CAPTURE_H__ */
> > > diff --git a/src/gstreamer/gstlibcamerasrc.cpp b/src/gstreamer/gstlibcamerasrc.cpp
> > > index 1bfc2e2f..ca223df6 100644
> > > --- a/src/gstreamer/gstlibcamerasrc.cpp
> > > +++ b/src/gstreamer/gstlibcamerasrc.cpp
> > > @@ -74,6 +74,8 @@ RequestWrap::~RequestWrap()
> > > if (item.second)
> > > gst_buffer_unref(item.second);
> > > }
> > > +
> > > + delete request_;
> > > }
> > >
> > > void RequestWrap::attachBuffer(GstBuffer *buffer)
> > > @@ -266,7 +268,8 @@ gst_libcamera_src_task_run(gpointer user_data)
> > > GstLibcameraSrc *self = GST_LIBCAMERA_SRC(user_data);
> > > GstLibcameraSrcState *state = self->state;
> > >
> > > - Request *request = state->cam_->createRequest();
> > > + std::unique_ptr<Request> request_unique = state->cam_->createRequest();
> > > + Request *request = request_unique.release();
> > > auto wrap = std::make_unique<RequestWrap>(request);
> > > for (GstPad *srcpad : state->srcpads_) {
> > > GstLibcameraPool *pool = gst_libcamera_pad_get_pool(srcpad);
> > > @@ -281,7 +284,6 @@ gst_libcamera_src_task_run(gpointer user_data)
> > > * queueing this one due to lack of buffers.
> > > */
> > > delete request;
> > > - request = nullptr;
> > > break;
> > > }
> > >
> > > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp
> > > index ae16a64a..8c2bbc76 100644
> > > --- a/src/libcamera/camera.cpp
> > > +++ b/src/libcamera/camera.cpp
> > > @@ -833,21 +833,23 @@ int Camera::configure(CameraConfiguration *config)
> > > * handler, and is completely opaque to libcamera.
> > > *
> > > * The ownership of the returned request is passed to the caller, which is
> > > - * responsible for either queueing the request or deleting it.
> > > + * responsible for either queueing the request or deleting it. The caller
> > > + * continues to own the request after queueing it, so deleting the request
> > > + * is the responsibility of the application after the request completes.
> > > *
> > > * \context This function is \threadsafe. It may only be called when the camera
> > > * is in the Configured or Running state as defined in \ref camera_operation.
> > > *
> > > * \return A pointer to the newly created request, or nullptr on error
> > > */
> > > -Request *Camera::createRequest(uint64_t cookie)
> > > +std::unique_ptr<Request> Camera::createRequest(uint64_t cookie)
> > > {
> > > int ret = p_->isAccessAllowed(Private::CameraConfigured,
> > > Private::CameraRunning);
> > > if (ret < 0)
> > > return nullptr;
> > >
> > > - return new Request(this, cookie);
> > > + return std::make_unique<Request>(this, cookie);
> > > }
> > >
> > > /**
> > > @@ -863,9 +865,6 @@ Request *Camera::createRequest(uint64_t cookie)
> > > * Once the request has been queued, the camera will notify its completion
> > > * through the \ref requestCompleted signal.
> > > *
> > > - * Ownership of the request is transferred to the camera. It will be deleted
> > > - * automatically after it completes.
> > > - *
> > > * \context This function is \threadsafe. It may only be called when the camera
> > > * is in the Running state as defined in \ref camera_operation.
> > > *
> > > @@ -974,13 +973,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..dd3040fe 100644
> > > --- a/src/libcamera/request.cpp
> > > +++ b/src/libcamera/request.cpp
> > > @@ -85,6 +85,30 @@ Request::~Request()
> > > delete validator_;
> > > }
> > >
> > > +/*
> > > + * \brief Reset the request
> > > + *
> > > + * Reset the request, to allow it to be reused and requeued without
> > > + * destruction. All contents of the request will be reset, so any references
> > > + * previously returned by the request will become invalid.
> > > + */
> >
> > Would it make sens to split this function in two? One that resets the
> > status_, cancelled_, metadata_ and any output controls and one that
> > clears the buffers.
> >
> > The reset of output parameters could be called by Camera::queueRequest()
> > and the clearBuffers() would be called optionally by applications.
> >
> > That way applications would be able to both reuse a request by either by
> > keeping all buffer associations or clearing them and repopulating them
> > with new buffers. Also by clearing output parameters in the framework
> > applications will be unable to get it wrong.
>
> Oh interesting idea.
>
> Okay, I did a reset() that's called in Camera::queueRequest(), that
> resets status_, cancelled_, metadata_, controls_, and validator_. Then a
> clearBuffers() that nukes the buffer map. And a reAddBuffers() that
> takes the current buffer map, and reattaches the request to the buffers
> (otherwise we get a segfault on buffer->request_ since completeBuffer()
> nulls buffer->request_ and there's no addBuffer() to reattach it).
>
> And so the apps that:
> - still use transient request (android, gstreamer)
> - can just delete it at the end of the completion handler
> - the apps the want to reuse the buffers (cam)
> - can just call request->reAddBuffers() at the end of the completion
> handler
> - and apps that need to change up their buffers (qcam, v4l2)
> - can do so at the end of the completion handler, with no special
> calls
For this last category, clearBuffers() is required before changing the
buffers.
Paul
>
> See v3 for details :)
>
>
> Thanks,
>
> Paul
>
> > > +void Request::reset()
> > > +{
> > > + bufferMap_.clear();
> > > + pending_.clear();
> > > +
> > > + status_ = RequestPending;
> > > + cancelled_ = false;
> > > +
> > > + delete metadata_;
> > > + delete controls_;
> > > + delete validator_;
> > > +
> > > + validator_ = new CameraControlValidator(camera_);
> > > + controls_ = new ControlList(controls::controls, validator_);
> > > + metadata_ = new ControlList(controls::controls);
> > > +}
> > > +
> > > /**
> > > * \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..c2f8b175 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. */
> > > @@ -486,7 +485,7 @@ int MainWindow::startCapture()
> > > while (!freeBuffers_[vfStream_].isEmpty()) {
> > > FrameBuffer *buffer = freeBuffers_[vfStream_].dequeue();
> > >
> > > - Request *request = camera_->createRequest();
> > > + std::unique_ptr<Request> request = camera_->createRequest();
> > > if (!request) {
> > > qWarning() << "Can't create request";
> > > ret = -ENOMEM;
> > > @@ -499,7 +498,7 @@ int MainWindow::startCapture()
> > > goto error;
> > > }
> > >
> > > - requests.push_back(request);
> > > + requests_.push_back(std::move(request));
> > > }
> > >
> > > /* Start the title timer and the camera. */
> > > @@ -518,8 +517,8 @@ int MainWindow::startCapture()
> > > camera_->requestCompleted.connect(this, &MainWindow::requestComplete);
> > >
> > > /* Queue all requests. */
> > > - for (Request *request : requests) {
> > > - ret = camera_->queueRequest(request);
> > > + for (std::unique_ptr<Request> &request : requests_) {
> > > + ret = camera_->queueRequest(request.get());
> > > if (ret < 0) {
> > > qWarning() << "Can't queue request";
> > > goto error_disconnect;
> > > @@ -535,8 +534,7 @@ error_disconnect:
> > > camera_->stop();
> > >
> > > error:
> > > - for (Request *request : requests)
> > > - delete request;
> > > + requests_.clear();
> > >
> > > for (auto &iter : mappedBuffers_) {
> > > const MappedBuffer &buffer = iter.second;
> > > @@ -580,6 +578,8 @@ void MainWindow::stopCapture()
> > > }
> > > mappedBuffers_.clear();
> > >
> > > + requests_.clear();
> > > +
> > > delete allocator_;
> > >
> > > isCapturing_ = false;
> > > @@ -701,7 +701,7 @@ void MainWindow::requestComplete(Request *request)
> > > */
> > > {
> > > QMutexLocker locker(&mutex_);
> > > - doneQueue_.enqueue({ request->buffers(), request->metadata() });
> > > + doneQueue_.enqueue(request);
> > > }
> > >
> > > QCoreApplication::postEvent(this, new CaptureEvent);
> > > @@ -714,8 +714,7 @@ void MainWindow::processCapture()
> > > * if stopCapture() has been called while a CaptureEvent was posted but
> > > * not processed yet. Return immediately in that case.
> > > */
> > > - CaptureRequest request;
> > > -
> > > + Request *request;
> > > {
> > > QMutexLocker locker(&mutex_);
> > > if (doneQueue_.isEmpty())
> > > @@ -725,11 +724,19 @@ void MainWindow::processCapture()
> > > }
> > >
> > > /* Process buffers. */
> > > - if (request.buffers_.count(vfStream_))
> > > - processViewfinder(request.buffers_[vfStream_]);
> > > + if (request->buffers().count(vfStream_)) {
> > > + {
> > > + QMutexLocker locker(&mutex_);
> > > + freeQueue_.enqueue(request);
> > > + }
> > > + Request::BufferMap buffers = request->buffers();
> > > + processViewfinder(buffers[vfStream_]);
> > > + }
> > >
> > > - if (request.buffers_.count(rawStream_))
> > > - processRaw(request.buffers_[rawStream_], request.metadata_);
> > > + if (request->buffers().count(rawStream_)) {
> > > + Request::BufferMap buffers = request->buffers();
> > > + processRaw(buffers[rawStream_], request->metadata());
> > > + }
> > > }
> > >
> > > void MainWindow::processViewfinder(FrameBuffer *buffer)
> > > @@ -754,12 +761,17 @@ void MainWindow::processViewfinder(FrameBuffer *buffer)
> > >
> > > void MainWindow::queueRequest(FrameBuffer *buffer)
> > > {
> > > - Request *request = camera_->createRequest();
> > > - if (!request) {
> > > - qWarning() << "Can't create request";
> > > - return;
> > > + Request *request;
> > > + {
> > > + QMutexLocker locker(&mutex_);
> > > + if (freeQueue_.isEmpty())
> > > + return;
> > > +
> > > + request = freeQueue_.dequeue();
> > > }
> > >
> > > + request->reset();
> > > +
> > > request->addBuffer(vfStream_, buffer);
> > >
> > > if (captureRaw_) {
> > > diff --git a/src/qcam/main_window.h b/src/qcam/main_window.h
> > > index 5c61a4df..64bcfebc 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"
> > > @@ -41,23 +43,6 @@ enum {
> > > OptStream = 's',
> > > };
> > >
> > > -class CaptureRequest
> > > -{
> > > -public:
> > > - CaptureRequest()
> > > - {
> > > - }
> > > -
> > > - CaptureRequest(const Request::BufferMap &buffers,
> > > - const ControlList &metadata)
> > > - : buffers_(buffers), metadata_(metadata)
> > > - {
> > > - }
> > > -
> > > - Request::BufferMap buffers_;
> > > - ControlList metadata_;
> > > -};
> > > -
> > > class MainWindow : public QMainWindow
> > > {
> > > Q_OBJECT
> > > @@ -128,13 +113,16 @@ private:
> > > Stream *vfStream_;
> > > Stream *rawStream_;
> > > std::map<const Stream *, QQueue<FrameBuffer *>> freeBuffers_;
> > > - QQueue<CaptureRequest> doneQueue_;
> > > - QMutex mutex_; /* Protects freeBuffers_ and doneQueue_ */
> > > + QQueue<Request *> doneQueue_;
> > > + QQueue<Request *> freeQueue_;
> > > + QMutex mutex_; /* Protects freeBuffers_, doneQueue_, and freeQueue_ */
> > >
> > > uint64_t lastBufferTime_;
> > > QElapsedTimer frameRateInterval_;
> > > uint32_t previousFrames_;
> > > uint32_t framesCaptured_;
> > > +
> > > + std::vector<std::unique_ptr<Request>> requests_;
> > > };
> > >
> > > #endif /* __QCAM_MAIN_WINDOW__ */
> > > diff --git a/src/v4l2/v4l2_camera.cpp b/src/v4l2/v4l2_camera.cpp
> > > index 3565f369..0d59502a 100644
> > > --- a/src/v4l2/v4l2_camera.cpp
> > > +++ b/src/v4l2/v4l2_camera.cpp
> > > @@ -49,6 +49,8 @@ int V4L2Camera::open(StreamConfiguration *streamConfig)
> > >
> > > void V4L2Camera::close()
> > > {
> > > + requestPool_.clear();
> > > +
> > > delete bufferAllocator_;
> > > bufferAllocator_ = nullptr;
> > >
> > > @@ -154,16 +156,30 @@ int V4L2Camera::validateConfiguration(const PixelFormat &pixelFormat,
> > > return 0;
> > > }
> > >
> > > -int V4L2Camera::allocBuffers([[maybe_unused]] unsigned int count)
> > > +int V4L2Camera::allocBuffers(unsigned int count)
> > > {
> > > Stream *stream = config_->at(0).stream();
> > >
> > > - return bufferAllocator_->allocate(stream);
> > > + int ret = bufferAllocator_->allocate(stream);
> > > + if (ret < 0)
> > > + return ret;
> > > +
> > > + for (unsigned int i = 0; i < count; i++) {
> > > + std::unique_ptr<Request> request = camera_->createRequest(i);
> > > + if (!request) {
> > > + requestPool_.clear();
> > > + return -ENOMEM;
> > > + }
> > > + requestPool_.push_back(std::move(request));
> > > + }
> > > +
> > > + return ret;
> > > }
> > >
> > > void V4L2Camera::freeBuffers()
> > > {
> > > pendingRequests_.clear();
> > > + requestPool_.clear();
> > >
> > > Stream *stream = config_->at(0).stream();
> > > bufferAllocator_->free(stream);
> > > @@ -192,9 +208,9 @@ int V4L2Camera::streamOn()
> > >
> > > isRunning_ = true;
> > >
> > > - for (std::unique_ptr<Request> &req : pendingRequests_) {
> > > + for (Request *req : pendingRequests_) {
> > > /* \todo What should we do if this returns -EINVAL? */
> > > - ret = camera_->queueRequest(req.release());
> > > + ret = camera_->queueRequest(req);
> > > if (ret < 0)
> > > return ret == -EACCES ? -EBUSY : ret;
> > > }
> > > @@ -226,12 +242,12 @@ int V4L2Camera::streamOff()
> > >
> > > int V4L2Camera::qbuf(unsigned int index)
> > > {
> > > - std::unique_ptr<Request> request =
> > > - std::unique_ptr<Request>(camera_->createRequest(index));
> > > - if (!request) {
> > > - LOG(V4L2Compat, Error) << "Can't create request";
> > > - return -ENOMEM;
> > > + if (index >= requestPool_.size()) {
> > > + LOG(V4L2Compat, Error) << "Invalid index";
> > > + return -EINVAL;
> > > }
> > > + Request *request = requestPool_[index].get();
> > > + request->reset();
> > >
> > > Stream *stream = config_->at(0).stream();
> > > FrameBuffer *buffer = bufferAllocator_->buffers(stream)[index].get();
> > > @@ -242,11 +258,11 @@ int V4L2Camera::qbuf(unsigned int index)
> > > }
> > >
> > > if (!isRunning_) {
> > > - pendingRequests_.push_back(std::move(request));
> > > + pendingRequests_.push_back(request);
> > > return 0;
> > > }
> > >
> > > - ret = camera_->queueRequest(request.release());
> > > + ret = camera_->queueRequest(request);
> > > if (ret < 0) {
> > > LOG(V4L2Compat, Error) << "Can't queue request";
> > > return ret == -EACCES ? -EBUSY : ret;
> > > diff --git a/src/v4l2/v4l2_camera.h b/src/v4l2/v4l2_camera.h
> > > index 1fc5ebef..a6c35a2e 100644
> > > --- a/src/v4l2/v4l2_camera.h
> > > +++ b/src/v4l2/v4l2_camera.h
> > > @@ -76,7 +76,9 @@ private:
> > > std::mutex bufferLock_;
> > > FrameBufferAllocator *bufferAllocator_;
> > >
> > > - std::deque<std::unique_ptr<Request>> pendingRequests_;
> > > + std::vector<std::unique_ptr<Request>> requestPool_;
> > > +
> > > + std::deque<Request *> pendingRequests_;
> > > std::deque<std::unique_ptr<Buffer>> completedBuffers_;
> > >
> > > int efd_;
> > > diff --git a/test/camera/buffer_import.cpp b/test/camera/buffer_import.cpp
> > > index 64e96264..9cfdb388 100644
> > > --- a/test/camera/buffer_import.cpp
> > > +++ b/test/camera/buffer_import.cpp
> > > @@ -50,7 +50,7 @@ protected:
> > > if (request->status() != Request::RequestComplete)
> > > return;
> > >
> > > - const Request::BufferMap &buffers = request->buffers();
> > > + const Request::BufferMap buffers = request->buffers();
> > >
> > > completeRequestsCount_++;
> > >
> > > @@ -58,7 +58,7 @@ protected:
> > > const Stream *stream = buffers.begin()->first;
> > > FrameBuffer *buffer = buffers.begin()->second;
> > >
> > > - request = camera_->createRequest();
> > > + request->reset();
> > > request->addBuffer(stream, buffer);
> > > camera_->queueRequest(request);
> > > }
> > > @@ -98,9 +98,8 @@ protected:
> > > if (ret != TestPass)
> > > return ret;
> > >
> > > - std::vector<Request *> requests;
> > > for (const std::unique_ptr<FrameBuffer> &buffer : source.buffers()) {
> > > - Request *request = camera_->createRequest();
> > > + std::unique_ptr<Request> request = camera_->createRequest();
> > > if (!request) {
> > > std::cout << "Failed to create request" << std::endl;
> > > return TestFail;
> > > @@ -111,7 +110,7 @@ protected:
> > > return TestFail;
> > > }
> > >
> > > - requests.push_back(request);
> > > + requests_.push_back(std::move(request));
> > > }
> > >
> > > completeRequestsCount_ = 0;
> > > @@ -125,8 +124,8 @@ protected:
> > > return TestFail;
> > > }
> > >
> > > - for (Request *request : requests) {
> > > - if (camera_->queueRequest(request)) {
> > > + for (std::unique_ptr<Request> &request : requests_) {
> > > + if (camera_->queueRequest(request.get())) {
> > > std::cout << "Failed to queue request" << std::endl;
> > > return TestFail;
> > > }
> > > @@ -160,6 +159,8 @@ protected:
> > > }
> > >
> > > private:
> > > + std::vector<std::unique_ptr<Request>> requests_;
> > > +
> > > unsigned int completeBuffersCount_;
> > > unsigned int completeRequestsCount_;
> > > std::unique_ptr<CameraConfiguration> config_;
> > > diff --git a/test/camera/capture.cpp b/test/camera/capture.cpp
> > > index 51bbd258..5c86c490 100644
> > > --- a/test/camera/capture.cpp
> > > +++ b/test/camera/capture.cpp
> > > @@ -44,7 +44,7 @@ protected:
> > > if (request->status() != Request::RequestComplete)
> > > return;
> > >
> > > - const Request::BufferMap &buffers = request->buffers();
> > > + const Request::BufferMap buffers = request->buffers();
> > >
> > > completeRequestsCount_++;
> > >
> > > @@ -52,7 +52,7 @@ protected:
> > > const Stream *stream = buffers.begin()->first;
> > > FrameBuffer *buffer = buffers.begin()->second;
> > >
> > > - request = camera_->createRequest();
> > > + request->reset();
> > > request->addBuffer(stream, buffer);
> > > camera_->queueRequest(request);
> > > }
> > > @@ -98,9 +98,8 @@ protected:
> > > if (ret < 0)
> > > return TestFail;
> > >
> > > - std::vector<Request *> requests;
> > > for (const std::unique_ptr<FrameBuffer> &buffer : allocator_->buffers(stream)) {
> > > - Request *request = camera_->createRequest();
> > > + std::unique_ptr<Request> request = camera_->createRequest();
> > > if (!request) {
> > > cout << "Failed to create request" << endl;
> > > return TestFail;
> > > @@ -111,7 +110,7 @@ protected:
> > > return TestFail;
> > > }
> > >
> > > - requests.push_back(request);
> > > + requests_.push_back(std::move(request));
> > > }
> > >
> > > completeRequestsCount_ = 0;
> > > @@ -125,8 +124,8 @@ protected:
> > > return TestFail;
> > > }
> > >
> > > - for (Request *request : requests) {
> > > - if (camera_->queueRequest(request)) {
> > > + for (std::unique_ptr<Request> &request : requests_) {
> > > + if (camera_->queueRequest(request.get())) {
> > > cout << "Failed to queue request" << endl;
> > > return TestFail;
> > > }
> > > @@ -161,6 +160,8 @@ protected:
> > > return TestPass;
> > > }
> > >
> > > + std::vector<std::unique_ptr<Request>> requests_;
> > > +
> > > std::unique_ptr<CameraConfiguration> config_;
> > > FrameBufferAllocator *allocator_;
> > > };
> > > diff --git a/test/camera/statemachine.cpp b/test/camera/statemachine.cpp
> > > index 28faeb91..e63ab298 100644
> > > --- a/test/camera/statemachine.cpp
> > > +++ b/test/camera/statemachine.cpp
> > > @@ -101,13 +101,10 @@ protected:
> > > return TestFail;
> > >
> > > /* Test operations which should pass. */
> > > - Request *request2 = camera_->createRequest();
> > > + std::unique_ptr<Request> request2 = camera_->createRequest();
> > > if (!request2)
> > > return TestFail;
> > >
> > > - /* Never handed to hardware so need to manually delete it. */
> > > - delete request2;
> > > -
> > > /* Test valid state transitions, end in Running state. */
> > > if (camera_->release())
> > > return TestFail;
> > > @@ -146,7 +143,7 @@ protected:
> > > return TestFail;
> > >
> > > /* Test operations which should pass. */
> > > - Request *request = camera_->createRequest();
> > > + std::unique_ptr<Request> request = camera_->createRequest();
> > > if (!request)
> > > return TestFail;
> > >
> > > @@ -154,7 +151,7 @@ protected:
> > > if (request->addBuffer(stream, allocator_->buffers(stream)[0].get()))
> > > return TestFail;
> > >
> > > - if (camera_->queueRequest(request))
> > > + if (camera_->queueRequest(request.get()))
> > > return TestFail;
> > >
> > > /* Test valid state transitions, end in Available state. */
> > > --
> > > 2.27.0
> > >
> > > _______________________________________________
> > > libcamera-devel mailing list
> > > libcamera-devel at lists.libcamera.org
> > > https://lists.libcamera.org/listinfo/libcamera-devel
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
- Previous message (by thread): [libcamera-devel] [PATCH v2] libcamera, android, cam, gstreamer, qcam, v4l2: Reuse Request
- Next message (by thread): [libcamera-devel] [PATCH v2] libcamera, android, cam, gstreamer, qcam, v4l2: Reuse Request
- Messages sorted by:
[ date ]
[ thread ]
[ subject ]
[ author ]
More information about the libcamera-devel
mailing list