| Message ID | 20220623232210.18742-2-laurent.pinchart@ideasonboard.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
Hello Laurent, I am not sure if I am well versed enough in gstreaner to review this, but here are few of my comments, pardon me if they are wrong ;) On Fri, Jun 24, 2022 at 1:22 AM Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > 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@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 I had a small doubt, shouldn't this replicate the function defined here ? https://gitlab.freedesktop.org/gstreamer/gstreamer/blob/1.18.4/gst/gsttask.c#L865 > > 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); > -- > Regards, > > Laurent Pinchart > Regards, Vedant Paranjape
Hi Vedant, On Mon, Jun 27, 2022 at 08:10:04PM +0200, Vedant Paranjape wrote: > Hello Laurent, > > I am not sure if I am well versed enough in gstreaner to review this, > but here are few of my comments, pardon me if they are wrong ;) Reviews are always appreciated :-) > On Fri, Jun 24, 2022 at 1:22 AM Laurent Pinchart wrote: > > > > 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@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 > > I had a small doubt, shouldn't this replicate the function defined > here ? https://gitlab.freedesktop.org/gstreamer/gstreamer/blob/1.18.4/gst/gsttask.c#L865 Ideally, yes, but that calls gst_task_set_state_unlocked(), which is an internal static function. I could copy its code here too, but as far as I understand, the above implementation should be functionally equivalent for our use cases (I think that was Nicolas' intent at least). > > 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);
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@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. With or without any changes here, code looks equivalent, with a return value instead of void. Reviewed-by: Nicolas Dufresne <nicolas.dufresne@collabora.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);
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@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 :-) 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. > With or without any changes here, code looks equivalent, with a return value > instead of void. > > Reviewed-by: Nicolas Dufresne <nicolas.dufresne@collabora.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); >
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@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 > :-) 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@collabora.com> For this, Reviewed-by: Umang Jain <umang.jain@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);
On Wed, Jun 29, 2022 at 04:51:35PM +0530, Umang Jain wrote: > 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@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 Both Debian and Ubuntu now ship 1.20, but their old stable releases shipped 1.14. I'd prefer giving everybody a bit more time to upgrade, given that there's no urgency on our side to drop support for 1.14. > > :-) 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@collabora.com> > > For this, > > Reviewed-by: Umang Jain <umang.jain@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);
Hello Laurent, On Mon, Jun 27, 2022 at 9:00 PM Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > Hi Vedant, > > On Mon, Jun 27, 2022 at 08:10:04PM +0200, Vedant Paranjape wrote: > > Hello Laurent, > > > > I am not sure if I am well versed enough in gstreaner to review this, > > but here are few of my comments, pardon me if they are wrong ;) > > Reviews are always appreciated :-) > > > On Fri, Jun 24, 2022 at 1:22 AM Laurent Pinchart wrote: > > > > > > 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@ideasonboard.com> Reviewed-by: Vedant Paranjape <vedantparanjape160201@gmail.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 > > > > I had a small doubt, shouldn't this replicate the function defined > > here ? https://gitlab.freedesktop.org/gstreamer/gstreamer/blob/1.18.4/gst/gsttask.c#L865 > > Ideally, yes, but that calls gst_task_set_state_unlocked(), which is an > internal static function. I could copy its code here too, but as far as > I understand, the above implementation should be functionally equivalent > for our use cases (I think that was Nicolas' intent at least). Ah, this makes sense. > > > > 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); > > -- > Regards, > > Laurent Pinchart
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@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@collabora.com> > > > For this, > > Reviewed-by: Umang Jain <umang.jain@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);
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 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);
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@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(-)