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

[Vedant Paranjape]

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 at 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 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

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