[libcamera-devel] [PATCH 01/13] gstreamer: Use gst_task_resume() when available

[Nicolas Dufresne]

Le mercredi 29 juin 2022 à 16:51 +0530, Umang Jain a écrit :
> Hi,
> 
> On 6/28/22 02:17, Laurent Pinchart via libcamera-devel wrote:
> > Hi Nicolas,
> > 
> > Thank you for the review.
> > 
> > On Mon, Jun 27, 2022 at 04:33:13PM -0400, Nicolas Dufresne wrote:
> > > Le vendredi 24 juin 2022 à 02:21 +0300, Laurent Pinchart a écrit :
> > > > > The gst_libcamera_resume_task() helper is an implementation of the
> > > > > gst_task_resume() function that predates its addition to GStreamer. Use
> > > > > gst_task_resume() when available, and rename gst_libcamera_resume_task()
> > > > > to gst_task_resume() to support older GStreamer versions.
> > > > > 
> > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> > > > > ---
> > > > >   src/gstreamer/gstlibcamera-utils.cpp | 16 ++++++++++------
> > > > >   src/gstreamer/gstlibcamera-utils.h   |  4 +++-
> > > > >   src/gstreamer/gstlibcamerasrc.cpp    |  4 ++--
> > > > >   3 files changed, 15 insertions(+), 9 deletions(-)
> > > > > 
> > > > > diff --git a/src/gstreamer/gstlibcamera-utils.cpp b/src/gstreamer/gstlibcamera-utils.cpp
> > > > > index 3f2422863c03..c97c0d438de2 100644
> > > > > --- a/src/gstreamer/gstlibcamera-utils.cpp
> > > > > +++ b/src/gstreamer/gstlibcamera-utils.cpp
> > > > > @@ -224,16 +224,20 @@ gst_libcamera_configure_stream_from_caps(StreamConfiguration &stream_cfg,
> > > > >   	stream_cfg.size.height = height;
> > > > >   }
> > > > >   
> > > > > -void
> > > > > -gst_libcamera_resume_task(GstTask *task)
> > > > > +#if !GST_CHECK_VERSION(1, 17, 1)
> > > > > +gboolean
> > > > > +gst_task_resume(GstTask *task)
> > > > >   {
> > > > >   	/* We only want to resume the task if it's paused. */
> > > > >   	GLibLocker lock(GST_OBJECT(task));
> > > > > -	if (GST_TASK_STATE(task) == GST_TASK_PAUSED) {
> > > > > -		GST_TASK_STATE(task) = GST_TASK_STARTED;
> > > > > -		GST_TASK_SIGNAL(task);
> > > > > -	}
> > > > > +	if (GST_TASK_STATE(task) != GST_TASK_PAUSED)
> > > > > +		return FALSE;
> > > > > +
> > > > > +	GST_TASK_STATE(task) = GST_TASK_STARTED;
> > > > > +	GST_TASK_SIGNAL(task);
> > > > > +	return TRUE;
> > > > >   }
> > > > > +#endif
> > > Have you also considered bumping GStreamer requirement to 1.18.0 and up ? It
> > > also been a while since we have bumped this.
> > We could. I haven't considered that, mostly out of laziness as I didn't
> > spend time checking if distros usually shipped GStreamer 1.18.0 nowadays
> 
> 
> I usually do a quick check the debian stable for such decisions on 
> https://packages.debian.org

https://packages.debian.org/search?keywords=libgstreamer1.0-0
1.18.4

And the gst_task_resume() is "Since : 1.18". So if that is the rule, then yes,
we are fine.

> 
> > :-) We have a little bit of backward-compatibility code in the
> > libcamerasrc unit test too, which we could then drop, but as it's so
> > little at this point it doesn't hurt much to keep it for a bit longer.
> 
> 
> I'll take a look here later. I have few patches for gstreamer ongoing.
> 
> > 
> > > With or without any changes here, code looks equivalent, with a return value
> > > instead of void.
> > > 
> > > Reviewed-by: Nicolas Dufresne <nicolas.dufresne at collabora.com>
> 
> 
> For this,
> 
> Reviewed-by: Umang Jain <umang.jain at ideasonboard.com>
> 
> > > 
> > > p.s. I saw Vedant comment, though the code looks like proper reduction of the
> > > upstream one.
> > > 
> > > > >   
> > > > >   G_LOCK_DEFINE_STATIC(cm_singleton_lock);
> > > > >   static std::weak_ptr<CameraManager> cm_singleton_ptr;
> > > > > diff --git a/src/gstreamer/gstlibcamera-utils.h b/src/gstreamer/gstlibcamera-utils.h
> > > > > index d54f15885d0c..164189a28965 100644
> > > > > --- a/src/gstreamer/gstlibcamera-utils.h
> > > > > +++ b/src/gstreamer/gstlibcamera-utils.h
> > > > > @@ -18,7 +18,9 @@ GstCaps *gst_libcamera_stream_formats_to_caps(const libcamera::StreamFormats &fo
> > > > >   GstCaps *gst_libcamera_stream_configuration_to_caps(const libcamera::StreamConfiguration &stream_cfg);
> > > > >   void gst_libcamera_configure_stream_from_caps(libcamera::StreamConfiguration &stream_cfg,
> > > > >   					      GstCaps *caps);
> > > > > -void gst_libcamera_resume_task(GstTask *task);
> > > > > +#if !GST_CHECK_VERSION(1, 17, 1)
> > > > > +gboolean gst_task_resume(GstTask *task);
> > > > > +#endif
> > > > >   std::shared_ptr<libcamera::CameraManager> gst_libcamera_get_camera_manager(int &ret);
> > > > >   
> > > > >   /**
> > > > > diff --git a/src/gstreamer/gstlibcamerasrc.cpp b/src/gstreamer/gstlibcamerasrc.cpp
> > > > > index 46fd02d207be..9d6be075a474 100644
> > > > > --- a/src/gstreamer/gstlibcamerasrc.cpp
> > > > > +++ b/src/gstreamer/gstlibcamerasrc.cpp
> > > > > @@ -192,7 +192,7 @@ GstLibcameraSrcState::requestCompleted(Request *request)
> > > > >   		gst_libcamera_pad_queue_buffer(srcpad, buffer);
> > > > >   	}
> > > > >   
> > > > > -	gst_libcamera_resume_task(this->src_->task);
> > > > > +	gst_task_resume(src_->task);
> > > > >   }
> > > > >   
> > > > >   static bool
> > > > > @@ -453,7 +453,7 @@ gst_libcamera_src_task_enter(GstTask *task, [[maybe_unused]] GThread *thread,
> > > > >   		GstLibcameraPool *pool = gst_libcamera_pool_new(self->allocator,
> > > > >   								stream_cfg.stream());
> > > > >   		g_signal_connect_swapped(pool, "buffer-notify",
> > > > > -					 G_CALLBACK(gst_libcamera_resume_task), task);
> > > > > +					 G_CALLBACK(gst_task_resume), task);
> > > > >   
> > > > >   		gst_libcamera_pad_set_pool(srcpad, pool);
> > > > >   		gst_flow_combiner_add_pad(self->flow_combiner, srcpad);