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 at 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 at ideasonboard.com>
>
> > for (GstPad *srcpad : state->srcpads_)
> > gst_libcamera_pad_set_pool(srcpad, nullptr);
> >
--
Regards,
Laurent Pinchart