| Message ID | 20220630000251.31295-7-laurent.pinchart@ideasonboard.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
Hi Laurent, Thank you for the patch. On 6/30/22 05:32, Laurent Pinchart via libcamera-devel wrote: > Move the request wrap to a completed queue in the request completion > handler to move more of the request completion processing to the > libcamerasrc task. This lowers the amount of time spent in the > completion handler, and prepares for reworking the usage of locks. > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > Changes since v1: > > - Use GST_CLOCK_TIME_NONE and GST_CLOCK_TIME_IS_VALID() > --- > src/gstreamer/gstlibcamerasrc.cpp | 82 ++++++++++++++++++------------- > 1 file changed, 48 insertions(+), 34 deletions(-) > > diff --git a/src/gstreamer/gstlibcamerasrc.cpp b/src/gstreamer/gstlibcamerasrc.cpp > index f2f48d53991a..2cb915637874 100644 > --- a/src/gstreamer/gstlibcamerasrc.cpp > +++ b/src/gstreamer/gstlibcamerasrc.cpp > @@ -57,10 +57,13 @@ struct RequestWrap { > > std::unique_ptr<Request> request_; > std::map<Stream *, GstBuffer *> buffers_; > + > + GstClockTime latency_; > + GstClockTime pts_; > }; > > RequestWrap::RequestWrap(std::unique_ptr<Request> request) > - : request_(std::move(request)) > + : request_(std::move(request)), latency_(0), pts_(GST_CLOCK_TIME_NONE) > { > } > > @@ -109,6 +112,7 @@ struct GstLibcameraSrcState { > std::unique_ptr<CameraConfiguration> config_; > std::vector<GstPad *> srcpads_; > std::queue<std::unique_ptr<RequestWrap>> queuedRequests_; > + std::queue<std::unique_ptr<RequestWrap>> completedRequests_; > guint group_id_; > > void requestCompleted(Request *request); > @@ -165,9 +169,6 @@ GstLibcameraSrcState::requestCompleted(Request *request) > return; > } > > - GstClockTime latency; > - GstClockTime pts; > - > if (GST_ELEMENT_CLOCK(src_)) { > int64_t timestamp = request->metadata().get(controls::SensorTimestamp); > > @@ -178,31 +179,11 @@ GstLibcameraSrcState::requestCompleted(Request *request) > > /* Deduced from: sys_now - sys_base_time == gst_now - gst_base_time */ > GstClockTime sys_base_time = sys_now - (gst_now - gst_base_time); > - pts = timestamp - sys_base_time; > - latency = sys_now - timestamp; > - } else { > - latency = 0; > - pts = GST_CLOCK_TIME_NONE; > + wrap->pts_ = timestamp - sys_base_time; > + wrap->latency_ = sys_now - timestamp; > } > > - for (GstPad *srcpad : srcpads_) { > - Stream *stream = gst_libcamera_pad_get_stream(srcpad); > - GstBuffer *buffer = wrap->detachBuffer(stream); > - > - FrameBuffer *fb = gst_libcamera_buffer_get_frame_buffer(buffer); > - > - if (GST_CLOCK_TIME_IS_VALID(pts)) { > - GST_BUFFER_PTS(buffer) = pts; > - gst_libcamera_pad_set_latency(srcpad, latency); > - } else { > - GST_BUFFER_PTS(buffer) = 0; > - } > - > - GST_BUFFER_OFFSET(buffer) = fb->metadata().sequence; > - GST_BUFFER_OFFSET_END(buffer) = fb->metadata().sequence; > - > - gst_libcamera_pad_queue_buffer(srcpad, buffer); > - } > + completedRequests_.push(std::move(wrap)); > > gst_task_resume(src_->task); > } > @@ -316,6 +297,39 @@ gst_libcamera_src_task_run(gpointer user_data) > /* The RequestWrap will be deleted in the completion handler. */ > } > > + { > + GLibLocker lock(GST_OBJECT(self)); > + > + if (!state->completedRequests_.empty()) { > + wrap = std::move(state->completedRequests_.front()); > + state->completedRequests_.pop(); > + } > + } > + > + if (!wrap) { > + gst_task_pause(self->task); > + return; > + } > + > + for (GstPad *srcpad : state->srcpads_) { > + Stream *stream = gst_libcamera_pad_get_stream(srcpad); > + GstBuffer *buffer = wrap->detachBuffer(stream); > + > + FrameBuffer *fb = gst_libcamera_buffer_get_frame_buffer(buffer); > + > + if (GST_CLOCK_TIME_IS_VALID(wrap->pts_)) { > + GST_BUFFER_PTS(buffer) = wrap->pts_; > + gst_libcamera_pad_set_latency(srcpad, wrap->latency_); > + } else { > + GST_BUFFER_PTS(buffer) = 0; > + } > + > + GST_BUFFER_OFFSET(buffer) = fb->metadata().sequence; > + GST_BUFFER_OFFSET_END(buffer) = fb->metadata().sequence; > + > + gst_libcamera_pad_queue_buffer(srcpad, buffer); > + } > + > GstFlowReturn ret = GST_FLOW_OK; > gst_flow_combiner_reset(self->flow_combiner); > for (GstPad *srcpad : state->srcpads_) { > @@ -344,13 +358,11 @@ gst_libcamera_src_task_run(gpointer user_data) > * happen in lock step with the callback thread which may want > * to resume the task and might push pending buffers. > */ > - GLibLocker lock(GST_OBJECT(self)); > - bool do_pause = true; > - for (GstPad *srcpad : state->srcpads_) { > - if (gst_libcamera_pad_has_pending(srcpad)) { > - do_pause = false; > - break; > - } > + bool do_pause; > + > + { > + GLibLocker lock(GST_OBJECT(self)); > + do_pause = state->completedRequests_.empty(); > } > > if (do_pause) > @@ -504,6 +516,8 @@ gst_libcamera_src_task_leave([[maybe_unused]] GstTask *task, > > state->cam_->stop(); > > + state->completedRequests_ = {}; > + Ah so when we stop we might have some RequestWrap(s) being added to completedRequests_ but won't be processed by the libcamera_src_task_run - since we have are already in libcamera_src_task_leave. I wonder if need to do any 'processing' for them before clearing them out here. Maybe not, those are just "cancelled" ones right? So, the 'processing' part for completedRequests_ is just, detaching the buffer, setting up latency and presentation time(pts I believe) and re-queuing the buffer the pad. So that's minimal, so I guess it's safe to clear them out. Reviewed-by: Umang Jain <umang.jain@ideasonboard.com> > for (GstPad *srcpad : state->srcpads_) > gst_libcamera_pad_set_pool(srcpad, nullptr); >
On Thu, Jun 30, 2022 at 01:21:34PM +0530, Umang Jain wrote: > Hi Laurent, > > Thank you for the patch. > > On 6/30/22 05:32, Laurent Pinchart via libcamera-devel wrote: > > Move the request wrap to a completed queue in the request completion > > handler to move more of the request completion processing to the > > libcamerasrc task. This lowers the amount of time spent in the > > completion handler, and prepares for reworking the usage of locks. > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > --- > > Changes since v1: > > > > - Use GST_CLOCK_TIME_NONE and GST_CLOCK_TIME_IS_VALID() > > --- > > src/gstreamer/gstlibcamerasrc.cpp | 82 ++++++++++++++++++------------- > > 1 file changed, 48 insertions(+), 34 deletions(-) > > > > diff --git a/src/gstreamer/gstlibcamerasrc.cpp b/src/gstreamer/gstlibcamerasrc.cpp > > index f2f48d53991a..2cb915637874 100644 > > --- a/src/gstreamer/gstlibcamerasrc.cpp > > +++ b/src/gstreamer/gstlibcamerasrc.cpp > > @@ -57,10 +57,13 @@ struct RequestWrap { > > > > std::unique_ptr<Request> request_; > > std::map<Stream *, GstBuffer *> buffers_; > > + > > + GstClockTime latency_; > > + GstClockTime pts_; > > }; > > > > RequestWrap::RequestWrap(std::unique_ptr<Request> request) > > - : request_(std::move(request)) > > + : request_(std::move(request)), latency_(0), pts_(GST_CLOCK_TIME_NONE) > > { > > } > > > > @@ -109,6 +112,7 @@ struct GstLibcameraSrcState { > > std::unique_ptr<CameraConfiguration> config_; > > std::vector<GstPad *> srcpads_; > > std::queue<std::unique_ptr<RequestWrap>> queuedRequests_; > > + std::queue<std::unique_ptr<RequestWrap>> completedRequests_; > > guint group_id_; > > > > void requestCompleted(Request *request); > > @@ -165,9 +169,6 @@ GstLibcameraSrcState::requestCompleted(Request *request) > > return; > > } > > > > - GstClockTime latency; > > - GstClockTime pts; > > - > > if (GST_ELEMENT_CLOCK(src_)) { > > int64_t timestamp = request->metadata().get(controls::SensorTimestamp); > > > > @@ -178,31 +179,11 @@ GstLibcameraSrcState::requestCompleted(Request *request) > > > > /* Deduced from: sys_now - sys_base_time == gst_now - gst_base_time */ > > GstClockTime sys_base_time = sys_now - (gst_now - gst_base_time); > > - pts = timestamp - sys_base_time; > > - latency = sys_now - timestamp; > > - } else { > > - latency = 0; > > - pts = GST_CLOCK_TIME_NONE; > > + wrap->pts_ = timestamp - sys_base_time; > > + wrap->latency_ = sys_now - timestamp; > > } > > > > - for (GstPad *srcpad : srcpads_) { > > - Stream *stream = gst_libcamera_pad_get_stream(srcpad); > > - GstBuffer *buffer = wrap->detachBuffer(stream); > > - > > - FrameBuffer *fb = gst_libcamera_buffer_get_frame_buffer(buffer); > > - > > - if (GST_CLOCK_TIME_IS_VALID(pts)) { > > - GST_BUFFER_PTS(buffer) = pts; > > - gst_libcamera_pad_set_latency(srcpad, latency); > > - } else { > > - GST_BUFFER_PTS(buffer) = 0; > > - } > > - > > - GST_BUFFER_OFFSET(buffer) = fb->metadata().sequence; > > - GST_BUFFER_OFFSET_END(buffer) = fb->metadata().sequence; > > - > > - gst_libcamera_pad_queue_buffer(srcpad, buffer); > > - } > > + completedRequests_.push(std::move(wrap)); > > > > gst_task_resume(src_->task); > > } > > @@ -316,6 +297,39 @@ gst_libcamera_src_task_run(gpointer user_data) > > /* The RequestWrap will be deleted in the completion handler. */ > > } > > > > + { > > + GLibLocker lock(GST_OBJECT(self)); > > + > > + if (!state->completedRequests_.empty()) { > > + wrap = std::move(state->completedRequests_.front()); > > + state->completedRequests_.pop(); > > + } > > + } > > + > > + if (!wrap) { > > + gst_task_pause(self->task); > > + return; > > + } > > + > > + for (GstPad *srcpad : state->srcpads_) { > > + Stream *stream = gst_libcamera_pad_get_stream(srcpad); > > + GstBuffer *buffer = wrap->detachBuffer(stream); > > + > > + FrameBuffer *fb = gst_libcamera_buffer_get_frame_buffer(buffer); > > + > > + if (GST_CLOCK_TIME_IS_VALID(wrap->pts_)) { > > + GST_BUFFER_PTS(buffer) = wrap->pts_; > > + gst_libcamera_pad_set_latency(srcpad, wrap->latency_); > > + } else { > > + GST_BUFFER_PTS(buffer) = 0; > > + } > > + > > + GST_BUFFER_OFFSET(buffer) = fb->metadata().sequence; > > + GST_BUFFER_OFFSET_END(buffer) = fb->metadata().sequence; > > + > > + gst_libcamera_pad_queue_buffer(srcpad, buffer); > > + } > > + > > GstFlowReturn ret = GST_FLOW_OK; > > gst_flow_combiner_reset(self->flow_combiner); > > for (GstPad *srcpad : state->srcpads_) { > > @@ -344,13 +358,11 @@ gst_libcamera_src_task_run(gpointer user_data) > > * happen in lock step with the callback thread which may want > > * to resume the task and might push pending buffers. > > */ > > - GLibLocker lock(GST_OBJECT(self)); > > - bool do_pause = true; > > - for (GstPad *srcpad : state->srcpads_) { > > - if (gst_libcamera_pad_has_pending(srcpad)) { > > - do_pause = false; > > - break; > > - } > > + bool do_pause; > > + > > + { > > + GLibLocker lock(GST_OBJECT(self)); > > + do_pause = state->completedRequests_.empty(); > > } > > > > if (do_pause) > > @@ -504,6 +516,8 @@ gst_libcamera_src_task_leave([[maybe_unused]] GstTask *task, > > > > state->cam_->stop(); > > > > + state->completedRequests_ = {}; > > + > > > Ah so when we stop we might have some RequestWrap(s) being added to > completedRequests_ but won't be processed by the libcamera_src_task_run > - since we have are already in libcamera_src_task_leave. > > I wonder if need to do any 'processing' for them before clearing them > out here. Maybe not, those are just "cancelled" ones right? Mostly, there could also occasionally be some completed requests. > So, the 'processing' part for completedRequests_ is just, detaching the > buffer, setting up latency and presentation time(pts I believe) and > re-queuing the buffer the pad. So that's minimal, so I guess it's safe > to clear them out. Given that the task is stopped due to the element being stopped, I don't see how we could meaningful process the completed requests anyway. > Reviewed-by: Umang Jain <umang.jain@ideasonboard.com> > > > for (GstPad *srcpad : state->srcpads_) > > gst_libcamera_pad_set_pool(srcpad, nullptr); > >
diff --git a/src/gstreamer/gstlibcamerasrc.cpp b/src/gstreamer/gstlibcamerasrc.cpp index f2f48d53991a..2cb915637874 100644 --- a/src/gstreamer/gstlibcamerasrc.cpp +++ b/src/gstreamer/gstlibcamerasrc.cpp @@ -57,10 +57,13 @@ struct RequestWrap { std::unique_ptr<Request> request_; std::map<Stream *, GstBuffer *> buffers_; + + GstClockTime latency_; + GstClockTime pts_; }; RequestWrap::RequestWrap(std::unique_ptr<Request> request) - : request_(std::move(request)) + : request_(std::move(request)), latency_(0), pts_(GST_CLOCK_TIME_NONE) { } @@ -109,6 +112,7 @@ struct GstLibcameraSrcState { std::unique_ptr<CameraConfiguration> config_; std::vector<GstPad *> srcpads_; std::queue<std::unique_ptr<RequestWrap>> queuedRequests_; + std::queue<std::unique_ptr<RequestWrap>> completedRequests_; guint group_id_; void requestCompleted(Request *request); @@ -165,9 +169,6 @@ GstLibcameraSrcState::requestCompleted(Request *request) return; } - GstClockTime latency; - GstClockTime pts; - if (GST_ELEMENT_CLOCK(src_)) { int64_t timestamp = request->metadata().get(controls::SensorTimestamp); @@ -178,31 +179,11 @@ GstLibcameraSrcState::requestCompleted(Request *request) /* Deduced from: sys_now - sys_base_time == gst_now - gst_base_time */ GstClockTime sys_base_time = sys_now - (gst_now - gst_base_time); - pts = timestamp - sys_base_time; - latency = sys_now - timestamp; - } else { - latency = 0; - pts = GST_CLOCK_TIME_NONE; + wrap->pts_ = timestamp - sys_base_time; + wrap->latency_ = sys_now - timestamp; } - for (GstPad *srcpad : srcpads_) { - Stream *stream = gst_libcamera_pad_get_stream(srcpad); - GstBuffer *buffer = wrap->detachBuffer(stream); - - FrameBuffer *fb = gst_libcamera_buffer_get_frame_buffer(buffer); - - if (GST_CLOCK_TIME_IS_VALID(pts)) { - GST_BUFFER_PTS(buffer) = pts; - gst_libcamera_pad_set_latency(srcpad, latency); - } else { - GST_BUFFER_PTS(buffer) = 0; - } - - GST_BUFFER_OFFSET(buffer) = fb->metadata().sequence; - GST_BUFFER_OFFSET_END(buffer) = fb->metadata().sequence; - - gst_libcamera_pad_queue_buffer(srcpad, buffer); - } + completedRequests_.push(std::move(wrap)); gst_task_resume(src_->task); } @@ -316,6 +297,39 @@ gst_libcamera_src_task_run(gpointer user_data) /* The RequestWrap will be deleted in the completion handler. */ } + { + GLibLocker lock(GST_OBJECT(self)); + + if (!state->completedRequests_.empty()) { + wrap = std::move(state->completedRequests_.front()); + state->completedRequests_.pop(); + } + } + + if (!wrap) { + gst_task_pause(self->task); + return; + } + + for (GstPad *srcpad : state->srcpads_) { + Stream *stream = gst_libcamera_pad_get_stream(srcpad); + GstBuffer *buffer = wrap->detachBuffer(stream); + + FrameBuffer *fb = gst_libcamera_buffer_get_frame_buffer(buffer); + + if (GST_CLOCK_TIME_IS_VALID(wrap->pts_)) { + GST_BUFFER_PTS(buffer) = wrap->pts_; + gst_libcamera_pad_set_latency(srcpad, wrap->latency_); + } else { + GST_BUFFER_PTS(buffer) = 0; + } + + GST_BUFFER_OFFSET(buffer) = fb->metadata().sequence; + GST_BUFFER_OFFSET_END(buffer) = fb->metadata().sequence; + + gst_libcamera_pad_queue_buffer(srcpad, buffer); + } + GstFlowReturn ret = GST_FLOW_OK; gst_flow_combiner_reset(self->flow_combiner); for (GstPad *srcpad : state->srcpads_) { @@ -344,13 +358,11 @@ gst_libcamera_src_task_run(gpointer user_data) * happen in lock step with the callback thread which may want * to resume the task and might push pending buffers. */ - GLibLocker lock(GST_OBJECT(self)); - bool do_pause = true; - for (GstPad *srcpad : state->srcpads_) { - if (gst_libcamera_pad_has_pending(srcpad)) { - do_pause = false; - break; - } + bool do_pause; + + { + GLibLocker lock(GST_OBJECT(self)); + do_pause = state->completedRequests_.empty(); } if (do_pause) @@ -504,6 +516,8 @@ gst_libcamera_src_task_leave([[maybe_unused]] GstTask *task, state->cam_->stop(); + state->completedRequests_ = {}; + for (GstPad *srcpad : state->srcpads_) gst_libcamera_pad_set_pool(srcpad, nullptr);
Move the request wrap to a completed queue in the request completion handler to move more of the request completion processing to the libcamerasrc task. This lowers the amount of time spent in the completion handler, and prepares for reworking the usage of locks. Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> --- Changes since v1: - Use GST_CLOCK_TIME_NONE and GST_CLOCK_TIME_IS_VALID() --- src/gstreamer/gstlibcamerasrc.cpp | 82 ++++++++++++++++++------------- 1 file changed, 48 insertions(+), 34 deletions(-)