[v2,3/3] gstreamer: Improve logging for buffer pool activation
diff mbox series

Message ID 20251013063820.90805-4-uajain@igalia.com
State Superseded
Headers show
Series
  • gstreamer: some assorted fixes
Related show

Commit Message

Umang Jain Oct. 13, 2025, 6:38 a.m. UTC 
If the negotiation is going to fail, log the GST_ELEMENT_ERROR in
gst_libcamera_src_negotiate(), instead of logging in downstream method
chain.

Signed-off-by: Umang Jain <uajain@igalia.com>
Reviewed-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
---
 src/gstreamer/gstlibcamerasrc.cpp | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

Comments

Barnabás Pőcze Oct. 13, 2025, 8:59 a.m. UTC | #1 
Hi

2025. 10. 13. 8:38 keltezéssel, Umang Jain írta:
> If the negotiation is going to fail, log the GST_ELEMENT_ERROR in
> gst_libcamera_src_negotiate(), instead of logging in downstream method
> chain.
> 
> Signed-off-by: Umang Jain <uajain@igalia.com>
> Reviewed-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
> ---
>   src/gstreamer/gstlibcamerasrc.cpp | 10 ++++++----
>   1 file changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/src/gstreamer/gstlibcamerasrc.cpp b/src/gstreamer/gstlibcamerasrc.cpp
> index 07bb6b2f..75e0864c 100644
> --- a/src/gstreamer/gstlibcamerasrc.cpp
> +++ b/src/gstreamer/gstlibcamerasrc.cpp
> @@ -581,9 +581,7 @@ gst_libcamera_create_video_pool(GstLibcameraSrc *self, GstPad *srcpad,
>   	}
> 
>   	if (!gst_buffer_pool_set_active(pool, true)) {
> -		GST_ELEMENT_ERROR(self, RESOURCE, SETTINGS,
> -				  ("Failed to active buffer pool"),
> -				  ("gst_libcamera_src_negotiate() failed."));
> +		GST_ERROR("Failed to activate buffer pool");

Maybe it makes sense to use `GST_ERROR_OBJECT()` to tie this to the video pool?
In any case:

Reviewed-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>


>   		return { nullptr, -EINVAL };
>   	}
> 
> @@ -686,8 +684,12 @@ gst_libcamera_src_negotiate(GstLibcameraSrc *self)
>   			std::tie(video_pool, ret) =
>   				gst_libcamera_create_video_pool(self, srcpad,
>   								caps, &info);
> -			if (ret)
> +			if (ret) {
> +				GST_ELEMENT_ERROR(self, RESOURCE, SETTINGS,
> +						  ("Failed to create video pool: %s", g_strerror(-ret)),
> +						  ("gst_libcamera_src_negotiate() failed."));
>   				return false;
> +			}
>   		}
> 
>   		GstLibcameraPool *pool = gst_libcamera_pool_new(self->allocator,
> --
> 2.51.0
>
Nicolas Dufresne Oct. 14, 2025, 12:22 p.m. UTC | #2 
Le lundi 13 octobre 2025 à 10:59 +0200, Barnabás Pőcze a écrit :
> Hi
> 
> 2025. 10. 13. 8:38 keltezéssel, Umang Jain írta:
> > If the negotiation is going to fail, log the GST_ELEMENT_ERROR in
> > gst_libcamera_src_negotiate(), instead of logging in downstream method
> > chain.
> > 
> > Signed-off-by: Umang Jain <uajain@igalia.com>
> > Reviewed-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
> > ---
> >   src/gstreamer/gstlibcamerasrc.cpp | 10 ++++++----
> >   1 file changed, 6 insertions(+), 4 deletions(-)
> > 
> > diff --git a/src/gstreamer/gstlibcamerasrc.cpp
> > b/src/gstreamer/gstlibcamerasrc.cpp
> > index 07bb6b2f..75e0864c 100644
> > --- a/src/gstreamer/gstlibcamerasrc.cpp
> > +++ b/src/gstreamer/gstlibcamerasrc.cpp
> > @@ -581,9 +581,7 @@ gst_libcamera_create_video_pool(GstLibcameraSrc *self,
> > GstPad *srcpad,
> >   	}
> > 
> >   	if (!gst_buffer_pool_set_active(pool, true)) {
> > -		GST_ELEMENT_ERROR(self, RESOURCE, SETTINGS,
> > -				  ("Failed to active buffer pool"),
> > -				  ("gst_libcamera_src_negotiate()
> > failed."));
> > +		GST_ERROR("Failed to activate buffer pool");
> 
> Maybe it makes sense to use `GST_ERROR_OBJECT()` to tie this to the video
> pool?

Good catch, I did missed it, would be nice to be fixed before merging at least.

> In any case:
> 
> Reviewed-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
> 
> 
> >   		return { nullptr, -EINVAL };
> >   	}
> > 
> > @@ -686,8 +684,12 @@ gst_libcamera_src_negotiate(GstLibcameraSrc *self)
> >   			std::tie(video_pool, ret) =
> >   				gst_libcamera_create_video_pool(self,
> > srcpad,
> >   								caps,
> > &info);
> > -			if (ret)
> > +			if (ret) {
> > +				GST_ELEMENT_ERROR(self, RESOURCE, SETTINGS,
> > +						  ("Failed to create video
> > pool: %s", g_strerror(-ret)),
> > +						 
> > ("gst_libcamera_src_negotiate() failed."));
> >   				return false;
> > +			}
> >   		}
> > 
> >   		GstLibcameraPool *pool = gst_libcamera_pool_new(self-
> > >allocator,
> > --
> > 2.51.0
> >

Patch
diff mbox series 

diff --git a/src/gstreamer/gstlibcamerasrc.cpp b/src/gstreamer/gstlibcamerasrc.cpp
index 07bb6b2f..75e0864c 100644
--- a/src/gstreamer/gstlibcamerasrc.cpp
+++ b/src/gstreamer/gstlibcamerasrc.cpp
@@ -581,9 +581,7 @@  gst_libcamera_create_video_pool(GstLibcameraSrc *self, GstPad *srcpad,
 	}
 
 	if (!gst_buffer_pool_set_active(pool, true)) {
-		GST_ELEMENT_ERROR(self, RESOURCE, SETTINGS,
-				  ("Failed to active buffer pool"),
-				  ("gst_libcamera_src_negotiate() failed."));
+		GST_ERROR("Failed to activate buffer pool");
 		return { nullptr, -EINVAL };
 	}
 
@@ -686,8 +684,12 @@  gst_libcamera_src_negotiate(GstLibcameraSrc *self)
 			std::tie(video_pool, ret) =
 				gst_libcamera_create_video_pool(self, srcpad,
 								caps, &info);
-			if (ret)
+			if (ret) {
+				GST_ELEMENT_ERROR(self, RESOURCE, SETTINGS,
+						  ("Failed to create video pool: %s", g_strerror(-ret)),
+						  ("gst_libcamera_src_negotiate() failed."));
 				return false;
+			}
 		}
 
 		GstLibcameraPool *pool = gst_libcamera_pool_new(self->allocator,