| Message ID | 20251010141826.42995-3-uajain@igalia.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi, Le vendredi 10 octobre 2025 à 19:48 +0530, Umang Jain a écrit : > Instead of trying to figure out which libcamera::Stream, the GstPad > is configured with (from the libcamera pool), associate the Stream > with the GstPad directly using gst_pad_set_element_private(). > > The libcamera::Stream can now be obtained using > gst_pad_get_element_private() hence, drop the getter > gst_libcamera_pad_get_stream(). > > Additionally, drop retrieving the stream configuration > using the same index, as of the srcpads_ vector in processRequest(). > This can break in future, if CameraConfiguration moves away > from vector for storing stream configurations. Hence, retrieve > the stream configuration correctly, from GstPad's configured stream. > > Signed-off-by: Umang Jain <uajain@igalia.com> > --- > src/gstreamer/gstlibcamerapad.cpp | 11 ----------- > src/gstreamer/gstlibcamerapad.h | 2 -- > src/gstreamer/gstlibcamerasrc.cpp | 10 +++++++--- > 3 files changed, 7 insertions(+), 16 deletions(-) > > diff --git a/src/gstreamer/gstlibcamerapad.cpp b/src/gstreamer/gstlibcamerapad.cpp > index 22b96719..7855ef22 100644 > --- a/src/gstreamer/gstlibcamerapad.cpp > +++ b/src/gstreamer/gstlibcamerapad.cpp > @@ -189,17 +189,6 @@ void gst_libcamera_pad_set_video_info(GstPad *pad, const GstVideoInfo *info) > self->info = *info; > } > > -Stream * > -gst_libcamera_pad_get_stream(GstPad *pad) > -{ > - auto *self = GST_LIBCAMERA_PAD(pad); > - > - if (self->pool) > - return gst_libcamera_pool_get_stream(self->pool); > - > - return nullptr; > -} > - > void > gst_libcamera_pad_set_latency(GstPad *pad, GstClockTime latency) > { > diff --git a/src/gstreamer/gstlibcamerapad.h b/src/gstreamer/gstlibcamerapad.h > index f98b8a7f..5f355aa9 100644 > --- a/src/gstreamer/gstlibcamerapad.h > +++ b/src/gstreamer/gstlibcamerapad.h > @@ -31,6 +31,4 @@ GstVideoInfo gst_libcamera_pad_get_video_info(GstPad *pad); > > void gst_libcamera_pad_set_video_info(GstPad *pad, const GstVideoInfo *info); > > -libcamera::Stream *gst_libcamera_pad_get_stream(GstPad *pad); > - > void gst_libcamera_pad_set_latency(GstPad *pad, GstClockTime latency); > diff --git a/src/gstreamer/gstlibcamerasrc.cpp b/src/gstreamer/gstlibcamerasrc.cpp > index 9ff90d9a..a5dbdfd4 100644 > --- a/src/gstreamer/gstlibcamerasrc.cpp > +++ b/src/gstreamer/gstlibcamerasrc.cpp > @@ -195,7 +195,7 @@ int GstLibcameraSrcState::queueRequest() > std::make_unique<RequestWrap>(std::move(request)); > > for (GstPad *srcpad : srcpads_) { > - Stream *stream = gst_libcamera_pad_get_stream(srcpad); > + Stream *stream = (Stream *)gst_pad_get_element_private(srcpad); No C cast please. I would suggest to keep gst_libcamera_pad_get_stream() (can be made inline) and implement the cast in it instead. > GstLibcameraPool *pool = gst_libcamera_pad_get_pool(srcpad); > GstBuffer *buffer; > GstFlowReturn ret; > @@ -359,11 +359,11 @@ int GstLibcameraSrcState::processRequest() > > for (gsize i = 0; i < srcpads_.size(); i++) { > GstPad *srcpad = srcpads_[i]; > - Stream *stream = gst_libcamera_pad_get_stream(srcpad); > + Stream *stream = (Stream *)gst_pad_get_element_private(srcpad); > GstBuffer *buffer = wrap->detachBuffer(stream); > > FrameBuffer *fb = gst_libcamera_buffer_get_frame_buffer(buffer); > - const StreamConfiguration &stream_cfg = config_->at(i); > + const StreamConfiguration &stream_cfg = stream->configuration(); How can it break in the future ? With that claim, I would expect gst_libcamera_src_negotiate() to be impacted. One thing to remember, we don't deny pad request while streaming. > GstBufferPool *video_pool = gst_libcamera_pad_get_video_pool(srcpad); > > if (video_pool) { > @@ -698,6 +698,9 @@ gst_libcamera_src_negotiate(GstLibcameraSrc *self) > gst_libcamera_pad_set_pool(srcpad, pool); > gst_libcamera_pad_set_video_pool(srcpad, video_pool); > > + /* Associate the configured stream with the src pad. */ > + gst_pad_set_element_private(srcpad, (gpointer)stream_cfg.stream()); > + > /* Clear all reconfigure flags. */ > gst_pad_check_reconfigure(srcpad); > } > @@ -894,6 +897,7 @@ gst_libcamera_src_task_leave([[maybe_unused]] GstTask *task, > for (GstPad *srcpad : state->srcpads_) { > gst_libcamera_pad_set_latency(srcpad, GST_CLOCK_TIME_NONE); > gst_libcamera_pad_set_pool(srcpad, nullptr); > + gst_pad_set_element_private(srcpad, nullptr); > } > } >
On Fri, Oct 10, 2025 at 11:19:57AM -0400, Nicolas Dufresne wrote: > Hi, > > Le vendredi 10 octobre 2025 à 19:48 +0530, Umang Jain a écrit : > > Instead of trying to figure out which libcamera::Stream, the GstPad > > is configured with (from the libcamera pool), associate the Stream > > with the GstPad directly using gst_pad_set_element_private(). > > > > The libcamera::Stream can now be obtained using > > gst_pad_get_element_private() hence, drop the getter > > gst_libcamera_pad_get_stream(). > > > > Additionally, drop retrieving the stream configuration > > using the same index, as of the srcpads_ vector in processRequest(). > > This can break in future, if CameraConfiguration moves away > > from vector for storing stream configurations. Hence, retrieve > > the stream configuration correctly, from GstPad's configured stream. > > > > Signed-off-by: Umang Jain <uajain@igalia.com> > > --- > > src/gstreamer/gstlibcamerapad.cpp | 11 ----------- > > src/gstreamer/gstlibcamerapad.h | 2 -- > > src/gstreamer/gstlibcamerasrc.cpp | 10 +++++++--- > > 3 files changed, 7 insertions(+), 16 deletions(-) > > > > diff --git a/src/gstreamer/gstlibcamerapad.cpp b/src/gstreamer/gstlibcamerapad.cpp > > index 22b96719..7855ef22 100644 > > --- a/src/gstreamer/gstlibcamerapad.cpp > > +++ b/src/gstreamer/gstlibcamerapad.cpp > > @@ -189,17 +189,6 @@ void gst_libcamera_pad_set_video_info(GstPad *pad, const GstVideoInfo *info) > > self->info = *info; > > } > > > > -Stream * > > -gst_libcamera_pad_get_stream(GstPad *pad) > > -{ > > - auto *self = GST_LIBCAMERA_PAD(pad); > > - > > - if (self->pool) > > - return gst_libcamera_pool_get_stream(self->pool); > > - > > - return nullptr; > > -} > > - > > void > > gst_libcamera_pad_set_latency(GstPad *pad, GstClockTime latency) > > { > > diff --git a/src/gstreamer/gstlibcamerapad.h b/src/gstreamer/gstlibcamerapad.h > > index f98b8a7f..5f355aa9 100644 > > --- a/src/gstreamer/gstlibcamerapad.h > > +++ b/src/gstreamer/gstlibcamerapad.h > > @@ -31,6 +31,4 @@ GstVideoInfo gst_libcamera_pad_get_video_info(GstPad *pad); > > > > void gst_libcamera_pad_set_video_info(GstPad *pad, const GstVideoInfo *info); > > > > -libcamera::Stream *gst_libcamera_pad_get_stream(GstPad *pad); > > - > > void gst_libcamera_pad_set_latency(GstPad *pad, GstClockTime latency); > > diff --git a/src/gstreamer/gstlibcamerasrc.cpp b/src/gstreamer/gstlibcamerasrc.cpp > > index 9ff90d9a..a5dbdfd4 100644 > > --- a/src/gstreamer/gstlibcamerasrc.cpp > > +++ b/src/gstreamer/gstlibcamerasrc.cpp > > @@ -195,7 +195,7 @@ int GstLibcameraSrcState::queueRequest() > > std::make_unique<RequestWrap>(std::move(request)); > > > > for (GstPad *srcpad : srcpads_) { > > - Stream *stream = gst_libcamera_pad_get_stream(srcpad); > > + Stream *stream = (Stream *)gst_pad_get_element_private(srcpad); > > No C cast please. I would suggest to keep gst_libcamera_pad_get_stream() (can be > made inline) and implement the cast in it instead. > > > GstLibcameraPool *pool = gst_libcamera_pad_get_pool(srcpad); > > GstBuffer *buffer; > > GstFlowReturn ret; > > @@ -359,11 +359,11 @@ int GstLibcameraSrcState::processRequest() > > > > for (gsize i = 0; i < srcpads_.size(); i++) { > > GstPad *srcpad = srcpads_[i]; > > - Stream *stream = gst_libcamera_pad_get_stream(srcpad); > > + Stream *stream = (Stream *)gst_pad_get_element_private(srcpad); > > GstBuffer *buffer = wrap->detachBuffer(stream); > > > > FrameBuffer *fb = gst_libcamera_buffer_get_frame_buffer(buffer); > > - const StreamConfiguration &stream_cfg = config_->at(i); > > + const StreamConfiguration &stream_cfg = stream->configuration(); > > How can it break in the future ? With that claim, I would expect > gst_libcamera_src_negotiate() to be impacted. One thing to remember, we don't > deny pad request while streaming. Any direct usage of config_ will break and need to be fixed up, but we should look out for any indirect usages like there. We should be easily get the Stream from pad - at streaming time, without going to config_ again. I'll address your comment above and adjust the commit message accordingly. > > > GstBufferPool *video_pool = gst_libcamera_pad_get_video_pool(srcpad); > > > > if (video_pool) { > > @@ -698,6 +698,9 @@ gst_libcamera_src_negotiate(GstLibcameraSrc *self) > > gst_libcamera_pad_set_pool(srcpad, pool); > > gst_libcamera_pad_set_video_pool(srcpad, video_pool); > > > > + /* Associate the configured stream with the src pad. */ > > + gst_pad_set_element_private(srcpad, (gpointer)stream_cfg.stream()); > > + > > /* Clear all reconfigure flags. */ > > gst_pad_check_reconfigure(srcpad); > > } > > @@ -894,6 +897,7 @@ gst_libcamera_src_task_leave([[maybe_unused]] GstTask *task, > > for (GstPad *srcpad : state->srcpads_) { > > gst_libcamera_pad_set_latency(srcpad, GST_CLOCK_TIME_NONE); > > gst_libcamera_pad_set_pool(srcpad, nullptr); > > + gst_pad_set_element_private(srcpad, nullptr); > > } > > } > >
Le lundi 13 octobre 2025 à 10:46 +0530, Umang Jain a écrit : > On Fri, Oct 10, 2025 at 11:19:57AM -0400, Nicolas Dufresne wrote: > > Hi, > > > > Le vendredi 10 octobre 2025 à 19:48 +0530, Umang Jain a écrit : > > > Instead of trying to figure out which libcamera::Stream, the GstPad > > > is configured with (from the libcamera pool), associate the Stream > > > with the GstPad directly using gst_pad_set_element_private(). > > > > > > The libcamera::Stream can now be obtained using > > > gst_pad_get_element_private() hence, drop the getter > > > gst_libcamera_pad_get_stream(). > > > > > > Additionally, drop retrieving the stream configuration > > > using the same index, as of the srcpads_ vector in processRequest(). > > > This can break in future, if CameraConfiguration moves away > > > from vector for storing stream configurations. Hence, retrieve > > > the stream configuration correctly, from GstPad's configured stream. > > > > > > Signed-off-by: Umang Jain <uajain@igalia.com> > > > --- > > > src/gstreamer/gstlibcamerapad.cpp | 11 ----------- > > > src/gstreamer/gstlibcamerapad.h | 2 -- > > > src/gstreamer/gstlibcamerasrc.cpp | 10 +++++++--- > > > 3 files changed, 7 insertions(+), 16 deletions(-) > > > > > > diff --git a/src/gstreamer/gstlibcamerapad.cpp b/src/gstreamer/gstlibcamerapad.cpp > > > index 22b96719..7855ef22 100644 > > > --- a/src/gstreamer/gstlibcamerapad.cpp > > > +++ b/src/gstreamer/gstlibcamerapad.cpp > > > @@ -189,17 +189,6 @@ void gst_libcamera_pad_set_video_info(GstPad *pad, const GstVideoInfo *info) > > > self->info = *info; > > > } > > > > > > -Stream * > > > -gst_libcamera_pad_get_stream(GstPad *pad) > > > -{ > > > - auto *self = GST_LIBCAMERA_PAD(pad); > > > - > > > - if (self->pool) > > > - return gst_libcamera_pool_get_stream(self->pool); > > > - > > > - return nullptr; > > > -} > > > - > > > void > > > gst_libcamera_pad_set_latency(GstPad *pad, GstClockTime latency) > > > { > > > diff --git a/src/gstreamer/gstlibcamerapad.h b/src/gstreamer/gstlibcamerapad.h > > > index f98b8a7f..5f355aa9 100644 > > > --- a/src/gstreamer/gstlibcamerapad.h > > > +++ b/src/gstreamer/gstlibcamerapad.h > > > @@ -31,6 +31,4 @@ GstVideoInfo gst_libcamera_pad_get_video_info(GstPad *pad); > > > > > > void gst_libcamera_pad_set_video_info(GstPad *pad, const GstVideoInfo *info); > > > > > > -libcamera::Stream *gst_libcamera_pad_get_stream(GstPad *pad); > > > - > > > void gst_libcamera_pad_set_latency(GstPad *pad, GstClockTime latency); > > > diff --git a/src/gstreamer/gstlibcamerasrc.cpp b/src/gstreamer/gstlibcamerasrc.cpp > > > index 9ff90d9a..a5dbdfd4 100644 > > > --- a/src/gstreamer/gstlibcamerasrc.cpp > > > +++ b/src/gstreamer/gstlibcamerasrc.cpp > > > @@ -195,7 +195,7 @@ int GstLibcameraSrcState::queueRequest() > > > std::make_unique<RequestWrap>(std::move(request)); > > > > > > for (GstPad *srcpad : srcpads_) { > > > - Stream *stream = gst_libcamera_pad_get_stream(srcpad); > > > + Stream *stream = (Stream *)gst_pad_get_element_private(srcpad); > > > > No C cast please. I would suggest to keep gst_libcamera_pad_get_stream() (can be > > made inline) and implement the cast in it instead. > > > > > GstLibcameraPool *pool = gst_libcamera_pad_get_pool(srcpad); > > > GstBuffer *buffer; > > > GstFlowReturn ret; > > > @@ -359,11 +359,11 @@ int GstLibcameraSrcState::processRequest() > > > > > > for (gsize i = 0; i < srcpads_.size(); i++) { > > > GstPad *srcpad = srcpads_[i]; > > > - Stream *stream = gst_libcamera_pad_get_stream(srcpad); > > > + Stream *stream = (Stream *)gst_pad_get_element_private(srcpad); > > > GstBuffer *buffer = wrap->detachBuffer(stream); > > > > > > FrameBuffer *fb = gst_libcamera_buffer_get_frame_buffer(buffer); > > > - const StreamConfiguration &stream_cfg = config_->at(i); > > > + const StreamConfiguration &stream_cfg = stream->configuration(); > > > > How can it break in the future ? With that claim, I would expect > > gst_libcamera_src_negotiate() to be impacted. One thing to remember, we don't > > deny pad request while streaming. > > Any direct usage of config_ will break and need to be fixed up, but we > should look out for any indirect usages like there. We should be easily get > the Stream from pad - at streaming time, without going to config_ again. Please notice my typo, we DO deny pad request while streaming. Now, leave it alone if you don't have a plan here, since mixing up is going to create confusion. Currently, nothing ever get added or removed at run-time from config_, and there is not concurrent thread running while doing the setup. Nicolas > > I'll address your comment above and adjust the commit message > accordingly. > > > > > > > GstBufferPool *video_pool = gst_libcamera_pad_get_video_pool(srcpad); > > > > > > if (video_pool) { > > > @@ -698,6 +698,9 @@ gst_libcamera_src_negotiate(GstLibcameraSrc *self) > > > gst_libcamera_pad_set_pool(srcpad, pool); > > > gst_libcamera_pad_set_video_pool(srcpad, video_pool); > > > > > > + /* Associate the configured stream with the src pad. */ > > > + gst_pad_set_element_private(srcpad, (gpointer)stream_cfg.stream()); > > > + > > > /* Clear all reconfigure flags. */ > > > gst_pad_check_reconfigure(srcpad); > > > } > > > @@ -894,6 +897,7 @@ gst_libcamera_src_task_leave([[maybe_unused]] GstTask *task, > > > for (GstPad *srcpad : state->srcpads_) { > > > gst_libcamera_pad_set_latency(srcpad, GST_CLOCK_TIME_NONE); > > > gst_libcamera_pad_set_pool(srcpad, nullptr); > > > + gst_pad_set_element_private(srcpad, nullptr); > > > } > > > } > > > >
diff --git a/src/gstreamer/gstlibcamerapad.cpp b/src/gstreamer/gstlibcamerapad.cpp index 22b96719..7855ef22 100644 --- a/src/gstreamer/gstlibcamerapad.cpp +++ b/src/gstreamer/gstlibcamerapad.cpp @@ -189,17 +189,6 @@ void gst_libcamera_pad_set_video_info(GstPad *pad, const GstVideoInfo *info) self->info = *info; } -Stream * -gst_libcamera_pad_get_stream(GstPad *pad) -{ - auto *self = GST_LIBCAMERA_PAD(pad); - - if (self->pool) - return gst_libcamera_pool_get_stream(self->pool); - - return nullptr; -} - void gst_libcamera_pad_set_latency(GstPad *pad, GstClockTime latency) { diff --git a/src/gstreamer/gstlibcamerapad.h b/src/gstreamer/gstlibcamerapad.h index f98b8a7f..5f355aa9 100644 --- a/src/gstreamer/gstlibcamerapad.h +++ b/src/gstreamer/gstlibcamerapad.h @@ -31,6 +31,4 @@ GstVideoInfo gst_libcamera_pad_get_video_info(GstPad *pad); void gst_libcamera_pad_set_video_info(GstPad *pad, const GstVideoInfo *info); -libcamera::Stream *gst_libcamera_pad_get_stream(GstPad *pad); - void gst_libcamera_pad_set_latency(GstPad *pad, GstClockTime latency); diff --git a/src/gstreamer/gstlibcamerasrc.cpp b/src/gstreamer/gstlibcamerasrc.cpp index 9ff90d9a..a5dbdfd4 100644 --- a/src/gstreamer/gstlibcamerasrc.cpp +++ b/src/gstreamer/gstlibcamerasrc.cpp @@ -195,7 +195,7 @@ int GstLibcameraSrcState::queueRequest() std::make_unique<RequestWrap>(std::move(request)); for (GstPad *srcpad : srcpads_) { - Stream *stream = gst_libcamera_pad_get_stream(srcpad); + Stream *stream = (Stream *)gst_pad_get_element_private(srcpad); GstLibcameraPool *pool = gst_libcamera_pad_get_pool(srcpad); GstBuffer *buffer; GstFlowReturn ret; @@ -359,11 +359,11 @@ int GstLibcameraSrcState::processRequest() for (gsize i = 0; i < srcpads_.size(); i++) { GstPad *srcpad = srcpads_[i]; - Stream *stream = gst_libcamera_pad_get_stream(srcpad); + Stream *stream = (Stream *)gst_pad_get_element_private(srcpad); GstBuffer *buffer = wrap->detachBuffer(stream); FrameBuffer *fb = gst_libcamera_buffer_get_frame_buffer(buffer); - const StreamConfiguration &stream_cfg = config_->at(i); + const StreamConfiguration &stream_cfg = stream->configuration(); GstBufferPool *video_pool = gst_libcamera_pad_get_video_pool(srcpad); if (video_pool) { @@ -698,6 +698,9 @@ gst_libcamera_src_negotiate(GstLibcameraSrc *self) gst_libcamera_pad_set_pool(srcpad, pool); gst_libcamera_pad_set_video_pool(srcpad, video_pool); + /* Associate the configured stream with the src pad. */ + gst_pad_set_element_private(srcpad, (gpointer)stream_cfg.stream()); + /* Clear all reconfigure flags. */ gst_pad_check_reconfigure(srcpad); } @@ -894,6 +897,7 @@ gst_libcamera_src_task_leave([[maybe_unused]] GstTask *task, for (GstPad *srcpad : state->srcpads_) { gst_libcamera_pad_set_latency(srcpad, GST_CLOCK_TIME_NONE); gst_libcamera_pad_set_pool(srcpad, nullptr); + gst_pad_set_element_private(srcpad, nullptr); } }
Instead of trying to figure out which libcamera::Stream, the GstPad is configured with (from the libcamera pool), associate the Stream with the GstPad directly using gst_pad_set_element_private(). The libcamera::Stream can now be obtained using gst_pad_get_element_private() hence, drop the getter gst_libcamera_pad_get_stream(). Additionally, drop retrieving the stream configuration using the same index, as of the srcpads_ vector in processRequest(). This can break in future, if CameraConfiguration moves away from vector for storing stream configurations. Hence, retrieve the stream configuration correctly, from GstPad's configured stream. Signed-off-by: Umang Jain <uajain@igalia.com> --- src/gstreamer/gstlibcamerapad.cpp | 11 ----------- src/gstreamer/gstlibcamerapad.h | 2 -- src/gstreamer/gstlibcamerasrc.cpp | 10 +++++++--- 3 files changed, 7 insertions(+), 16 deletions(-)