| Message ID | 20220623232210.18742-12-laurent.pinchart@ideasonboard.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
Hi Laurent, Le vendredi 24 juin 2022 à 02:22 +0300, Laurent Pinchart a écrit : > In order to prepare for creation and queuing of multiple requests, move > the request creation and queueing code to a separate function. No > functional change intended. > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > src/gstreamer/gstlibcamerasrc.cpp | 76 +++++++++++++++++-------------- > 1 file changed, 42 insertions(+), 34 deletions(-) > > diff --git a/src/gstreamer/gstlibcamerasrc.cpp b/src/gstreamer/gstlibcamerasrc.cpp > index 58a322b251c7..fb39d6093a3f 100644 > --- a/src/gstreamer/gstlibcamerasrc.cpp > +++ b/src/gstreamer/gstlibcamerasrc.cpp > @@ -123,6 +123,7 @@ struct GstLibcameraSrcState { > > guint group_id_; > > + int queueRequest(); > void requestCompleted(Request *request); > }; > > @@ -160,6 +161,44 @@ GstStaticPadTemplate request_src_template = { > "src_%u", GST_PAD_SRC, GST_PAD_REQUEST, TEMPLATE_CAPS > }; > > +int GstLibcameraSrcState::queueRequest() This function must be called with stream lock held, please comment above the function. > +{ > + std::unique_ptr<Request> request = cam_->createRequest(); > + if (!request) > + return -ENOMEM; > + > + std::unique_ptr<RequestWrap> wrap = > + std::make_unique<RequestWrap>(std::move(request)); > + > + for (GstPad *srcpad : srcpads_) { > + Stream *stream = gst_libcamera_pad_get_stream(srcpad); > + GstLibcameraPool *pool = gst_libcamera_pad_get_pool(srcpad); > + GstBuffer *buffer; > + GstFlowReturn ret; > + > + ret = gst_buffer_pool_acquire_buffer(GST_BUFFER_POOL(pool), > + &buffer, nullptr); > + if (ret != GST_FLOW_OK) { > + /* > + * RequestWrap has ownership of the request, and we > + * won't be queueing this one due to lack of buffers. > + */ > + return -ENOBUFS; > + } > + > + wrap->attachBuffer(stream, buffer); > + } > + > + GST_TRACE_OBJECT(src_, "Requesting buffers"); > + cam_->queueRequest(wrap->request_.get()); > + > + MutexLocker locker(lock_); > + queuedRequests_.push(std::move(wrap)); Possible maintenance trap, add a scope around the locker perhaps ? > + > + /* The RequestWrap will be deleted in the completion handler. */ > + return 0; > +} > + > void > GstLibcameraSrcState::requestCompleted(Request *request) > { > @@ -269,8 +308,8 @@ gst_libcamera_src_task_run(gpointer user_data) > GstLibcameraSrc *self = GST_LIBCAMERA_SRC(user_data); > GstLibcameraSrcState *state = self->state; > > - std::unique_ptr<Request> request = state->cam_->createRequest(); > - if (!request) { > + int err = state->queueRequest(); > + if (err == -ENOMEM) { > GST_ELEMENT_ERROR(self, RESOURCE, NO_SPACE_LEFT, > ("Failed to allocate request for camera '%s'.", > state->cam_->id().c_str()), > @@ -279,38 +318,7 @@ gst_libcamera_src_task_run(gpointer user_data) > return; > } > > - std::unique_ptr<RequestWrap> wrap = > - std::make_unique<RequestWrap>(std::move(request)); > - > - for (GstPad *srcpad : state->srcpads_) { > - Stream *stream = gst_libcamera_pad_get_stream(srcpad); > - GstLibcameraPool *pool = gst_libcamera_pad_get_pool(srcpad); > - GstBuffer *buffer; > - GstFlowReturn ret; > - > - ret = gst_buffer_pool_acquire_buffer(GST_BUFFER_POOL(pool), > - &buffer, nullptr); > - if (ret != GST_FLOW_OK) { > - /* > - * RequestWrap has ownership of the request, and we > - * won't be queueing this one due to lack of buffers. > - */ > - wrap.release(); > - break; > - } > - > - wrap->attachBuffer(stream, buffer); > - } > - > - if (wrap) { > - GST_TRACE_OBJECT(self, "Requesting buffers"); > - state->cam_->queueRequest(wrap->request_.get()); > - > - MutexLocker locker(state->lock_); > - state->queuedRequests_.push(std::move(wrap)); > - > - /* The RequestWrap will be deleted in the completion handler. */ > - } > + std::unique_ptr<RequestWrap> wrap; > > { > MutexLocker locker(state->lock_); Nothing major here: Reviewed-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
Hi Nicolas, On Tue, Jun 28, 2022 at 09:26:13AM -0400, Nicolas Dufresne wrote: > Le vendredi 24 juin 2022 à 02:22 +0300, Laurent Pinchart a écrit : > > In order to prepare for creation and queuing of multiple requests, move > > the request creation and queueing code to a separate function. No > > functional change intended. > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > --- > > src/gstreamer/gstlibcamerasrc.cpp | 76 +++++++++++++++++-------------- > > 1 file changed, 42 insertions(+), 34 deletions(-) > > > > diff --git a/src/gstreamer/gstlibcamerasrc.cpp b/src/gstreamer/gstlibcamerasrc.cpp > > index 58a322b251c7..fb39d6093a3f 100644 > > --- a/src/gstreamer/gstlibcamerasrc.cpp > > +++ b/src/gstreamer/gstlibcamerasrc.cpp > > @@ -123,6 +123,7 @@ struct GstLibcameraSrcState { > > > > guint group_id_; > > > > + int queueRequest(); > > void requestCompleted(Request *request); > > }; > > > > @@ -160,6 +161,44 @@ GstStaticPadTemplate request_src_template = { > > "src_%u", GST_PAD_SRC, GST_PAD_REQUEST, TEMPLATE_CAPS > > }; > > > > +int GstLibcameraSrcState::queueRequest() > > This function must be called with stream lock held, please comment above the > function. > > > +{ > > + std::unique_ptr<Request> request = cam_->createRequest(); > > + if (!request) > > + return -ENOMEM; > > + > > + std::unique_ptr<RequestWrap> wrap = > > + std::make_unique<RequestWrap>(std::move(request)); > > + > > + for (GstPad *srcpad : srcpads_) { > > + Stream *stream = gst_libcamera_pad_get_stream(srcpad); > > + GstLibcameraPool *pool = gst_libcamera_pad_get_pool(srcpad); > > + GstBuffer *buffer; > > + GstFlowReturn ret; > > + > > + ret = gst_buffer_pool_acquire_buffer(GST_BUFFER_POOL(pool), > > + &buffer, nullptr); > > + if (ret != GST_FLOW_OK) { > > + /* > > + * RequestWrap has ownership of the request, and we > > + * won't be queueing this one due to lack of buffers. > > + */ > > + return -ENOBUFS; > > + } > > + > > + wrap->attachBuffer(stream, buffer); > > + } > > + > > + GST_TRACE_OBJECT(src_, "Requesting buffers"); > > + cam_->queueRequest(wrap->request_.get()); > > + > > + MutexLocker locker(lock_); > > + queuedRequests_.push(std::move(wrap)); > > Possible maintenance trap, add a scope around the locker perhaps ? Both are good points, I'll fix that. > > + > > + /* The RequestWrap will be deleted in the completion handler. */ > > + return 0; > > +} > > + > > void > > GstLibcameraSrcState::requestCompleted(Request *request) > > { > > @@ -269,8 +308,8 @@ gst_libcamera_src_task_run(gpointer user_data) > > GstLibcameraSrc *self = GST_LIBCAMERA_SRC(user_data); > > GstLibcameraSrcState *state = self->state; > > > > - std::unique_ptr<Request> request = state->cam_->createRequest(); > > - if (!request) { > > + int err = state->queueRequest(); > > + if (err == -ENOMEM) { > > GST_ELEMENT_ERROR(self, RESOURCE, NO_SPACE_LEFT, > > ("Failed to allocate request for camera '%s'.", > > state->cam_->id().c_str()), > > @@ -279,38 +318,7 @@ gst_libcamera_src_task_run(gpointer user_data) > > return; > > } > > > > - std::unique_ptr<RequestWrap> wrap = > > - std::make_unique<RequestWrap>(std::move(request)); > > - > > - for (GstPad *srcpad : state->srcpads_) { > > - Stream *stream = gst_libcamera_pad_get_stream(srcpad); > > - GstLibcameraPool *pool = gst_libcamera_pad_get_pool(srcpad); > > - GstBuffer *buffer; > > - GstFlowReturn ret; > > - > > - ret = gst_buffer_pool_acquire_buffer(GST_BUFFER_POOL(pool), > > - &buffer, nullptr); > > - if (ret != GST_FLOW_OK) { > > - /* > > - * RequestWrap has ownership of the request, and we > > - * won't be queueing this one due to lack of buffers. > > - */ > > - wrap.release(); > > - break; > > - } > > - > > - wrap->attachBuffer(stream, buffer); > > - } > > - > > - if (wrap) { > > - GST_TRACE_OBJECT(self, "Requesting buffers"); > > - state->cam_->queueRequest(wrap->request_.get()); > > - > > - MutexLocker locker(state->lock_); > > - state->queuedRequests_.push(std::move(wrap)); > > - > > - /* The RequestWrap will be deleted in the completion handler. */ > > - } > > + std::unique_ptr<RequestWrap> wrap; > > > > { > > MutexLocker locker(state->lock_); > > Nothing major here: > > Reviewed-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
diff --git a/src/gstreamer/gstlibcamerasrc.cpp b/src/gstreamer/gstlibcamerasrc.cpp index 58a322b251c7..fb39d6093a3f 100644 --- a/src/gstreamer/gstlibcamerasrc.cpp +++ b/src/gstreamer/gstlibcamerasrc.cpp @@ -123,6 +123,7 @@ struct GstLibcameraSrcState { guint group_id_; + int queueRequest(); void requestCompleted(Request *request); }; @@ -160,6 +161,44 @@ GstStaticPadTemplate request_src_template = { "src_%u", GST_PAD_SRC, GST_PAD_REQUEST, TEMPLATE_CAPS }; +int GstLibcameraSrcState::queueRequest() +{ + std::unique_ptr<Request> request = cam_->createRequest(); + if (!request) + return -ENOMEM; + + std::unique_ptr<RequestWrap> wrap = + std::make_unique<RequestWrap>(std::move(request)); + + for (GstPad *srcpad : srcpads_) { + Stream *stream = gst_libcamera_pad_get_stream(srcpad); + GstLibcameraPool *pool = gst_libcamera_pad_get_pool(srcpad); + GstBuffer *buffer; + GstFlowReturn ret; + + ret = gst_buffer_pool_acquire_buffer(GST_BUFFER_POOL(pool), + &buffer, nullptr); + if (ret != GST_FLOW_OK) { + /* + * RequestWrap has ownership of the request, and we + * won't be queueing this one due to lack of buffers. + */ + return -ENOBUFS; + } + + wrap->attachBuffer(stream, buffer); + } + + GST_TRACE_OBJECT(src_, "Requesting buffers"); + cam_->queueRequest(wrap->request_.get()); + + MutexLocker locker(lock_); + queuedRequests_.push(std::move(wrap)); + + /* The RequestWrap will be deleted in the completion handler. */ + return 0; +} + void GstLibcameraSrcState::requestCompleted(Request *request) { @@ -269,8 +308,8 @@ gst_libcamera_src_task_run(gpointer user_data) GstLibcameraSrc *self = GST_LIBCAMERA_SRC(user_data); GstLibcameraSrcState *state = self->state; - std::unique_ptr<Request> request = state->cam_->createRequest(); - if (!request) { + int err = state->queueRequest(); + if (err == -ENOMEM) { GST_ELEMENT_ERROR(self, RESOURCE, NO_SPACE_LEFT, ("Failed to allocate request for camera '%s'.", state->cam_->id().c_str()), @@ -279,38 +318,7 @@ gst_libcamera_src_task_run(gpointer user_data) return; } - std::unique_ptr<RequestWrap> wrap = - std::make_unique<RequestWrap>(std::move(request)); - - for (GstPad *srcpad : state->srcpads_) { - Stream *stream = gst_libcamera_pad_get_stream(srcpad); - GstLibcameraPool *pool = gst_libcamera_pad_get_pool(srcpad); - GstBuffer *buffer; - GstFlowReturn ret; - - ret = gst_buffer_pool_acquire_buffer(GST_BUFFER_POOL(pool), - &buffer, nullptr); - if (ret != GST_FLOW_OK) { - /* - * RequestWrap has ownership of the request, and we - * won't be queueing this one due to lack of buffers. - */ - wrap.release(); - break; - } - - wrap->attachBuffer(stream, buffer); - } - - if (wrap) { - GST_TRACE_OBJECT(self, "Requesting buffers"); - state->cam_->queueRequest(wrap->request_.get()); - - MutexLocker locker(state->lock_); - state->queuedRequests_.push(std::move(wrap)); - - /* The RequestWrap will be deleted in the completion handler. */ - } + std::unique_ptr<RequestWrap> wrap; { MutexLocker locker(state->lock_);
In order to prepare for creation and queuing of multiple requests, move the request creation and queueing code to a separate function. No functional change intended. Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> --- src/gstreamer/gstlibcamerasrc.cpp | 76 +++++++++++++++++-------------- 1 file changed, 42 insertions(+), 34 deletions(-)