| Message ID | 20210612082231.111138-1-vedantparanjape160201@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Le samedi 12 juin 2021 à 13:52 +0530, Vedant Paranjape a écrit : > Using request pads needs virtual functions to create new request pads and > release the allocated pads. Added way to generate unique stream_id in > gst_libcamera_src_task_enter(). It failed to execute when more than one > pad was added. > > This patch defines the functions gst_libcamera_src_request_new_pad() and > gst_libcamera_src_release_pad() which handle, creating and releasing > request pads. Also assigns these functions to their callback > handlers in GstLibcameraSrcClass. > > gst_pad_create_stream_id() failed as this gstreamer element has more than 2 > source pads, and a nullptr was passed to it's stream_id argument. > This function fails to auto generate a stream id for scenario of more > than one source pad present. > > Used a gint which increments everytime new stream_id is requested and > group_id to generate an unique stream_id by appending them togther as > %i%i. > > This doesn't enable request pad support in gstreamer element. This is > one of the series of changes needed to make it work. I would suggest to rework the commit message, and focus on what feature this adds and how to use it. > > Signed-off-by: Vedant Paranjape <vedantparanjape160201@gmail.com> The code looks fine, I'll let you send the final one before adding by R-b. Note that when importing this patch: .git/rebase-apply/patch:58: trailing whitespace. return NULL; .git/rebase-apply/patch:60: trailing whitespace. .git/rebase-apply/patch:68: trailing whitespace. .git/rebase-apply/patch:85: trailing whitespace. } And also, running checkstyle.py shows some more issues: ---------------------------------------------------------------------------------------------------------- 0e4fbbad9ffaed512593848212facffed0f4620f gstreamer: Added virtual functions needed to support request pads ---------------------------------------------------------------------------------------------------------- --- src/gstreamer/gstlibcamerasrc.cpp +++ src/gstreamer/gstlibcamerasrc.cpp @@ -642,9 +641,9 @@ self->state = state; } -static GstPad* +static GstPad * gst_libcamera_src_request_new_pad(GstElement *element, GstPadTemplate *templ, - const gchar *name, [[maybe_unused]] const GstCaps *caps) + const gchar *name, [[maybe_unused]] const GstCaps *caps) { GstLibcameraSrc *self = GST_LIBCAMERA_SRC(element); g_autoptr(GstPad) pad = NULL; @@ -654,44 +653,40 @@ pad = gst_pad_new_from_template(templ, name); g_object_ref_sink(pad); - if (gst_element_add_pad(element, pad)) + if (gst_element_add_pad(element, pad)) { + GLibLocker lock(GST_OBJECT(self)); + self->state->srcpads_.push_back(reinterpret_cast<GstPad *>(g_object_ref(pad))); + } else { + GST_ELEMENT_ERROR(element, STREAM, FAILED, + ("Internal data stream error."), + ("Could not add pad to element")); + return NULL; + } + + return reinterpret_cast<GstPad *>(g_steal_pointer(&pad)); +} + +static void +gst_libcamera_src_release_pad(GstElement *element, GstPad *pad) +{ + GstLibcameraSrc *self = GST_LIBCAMERA_SRC(element); + + GST_DEBUG_OBJECT(self, "Pad %" GST_PTR_FORMAT " being released", pad); + { GLibLocker lock(GST_OBJECT(self)); - self->state->srcpads_.push_back(reinterpret_cast<GstPad*>(g_object_ref(pad))); - } - else - { - GST_ELEMENT_ERROR (element, STREAM, FAILED, - ("Internal data stream error."), - ("Could not add pad to element")); - return NULL; - } - - return reinterpret_cast<GstPad*>(g_steal_pointer(&pad)); -} - -static void -gst_libcamera_src_release_pad(GstElement *element, GstPad *pad) -{ - GstLibcameraSrc *self = GST_LIBCAMERA_SRC(element); - - GST_DEBUG_OBJECT (self, "Pad %" GST_PTR_FORMAT " being released", pad); - - { - GLibLocker lock(GST_OBJECT(self)); - std::vector<GstPad*> &pads = self->state->srcpads_; + std::vector<GstPad *> &pads = self->state->srcpads_; auto begin_iterator = pads.begin(); auto end_iterator = pads.end(); auto pad_iterator = std::find(begin_iterator, end_iterator, pad); - if (pad_iterator != end_iterator) - { + if (pad_iterator != end_iterator) { g_object_unref(*pad_iterator); pads.erase(pad_iterator); } } gst_element_remove_pad(element, pad); -} +} static void gst_libcamera_src_class_init(GstLibcameraSrcClass *klass) --- 2 potential issues detected, please review > --- > In _request_new_pad and _release_pad, a GLibLock is taken on > self->state. But since self->state is a C++ object, > GST_OBJECT(self->state) fails and returns null, so GLibLocker throws a > warning "invalid cast from '(null)' to 'GstObject'. This means that it > actually fails to get lock the resource that needs to be protected, > i.e., the srcpads_ vector. > > The following changes were tested using the following test code: https://pastebin.com/gNDF1cyG > > src/gstreamer/gstlibcamerasrc.cpp | 57 ++++++++++++++++++++++++++++++- > 1 file changed, 56 insertions(+), 1 deletion(-) > > diff --git a/src/gstreamer/gstlibcamerasrc.cpp b/src/gstreamer/gstlibcamerasrc.cpp > index ccc61590..1614971d 100644 > --- a/src/gstreamer/gstlibcamerasrc.cpp > +++ b/src/gstreamer/gstlibcamerasrc.cpp > @@ -361,10 +361,12 @@ gst_libcamera_src_task_enter(GstTask *task, [[maybe_unused]] GThread *thread, > GST_DEBUG_OBJECT(self, "Streaming thread has started"); > > guint group_id = gst_util_group_id_next(); > + gint stream_id_num = 0; > StreamRoles roles; > for (GstPad *srcpad : state->srcpads_) { > /* Create stream-id and push stream-start. */ > - g_autofree gchar *stream_id = gst_pad_create_stream_id(srcpad, GST_ELEMENT(self), nullptr); > + g_autofree gchar *stream_id_intermediate = g_strdup_printf("%i%i", group_id, stream_id_num++); > + g_autofree gchar *stream_id = gst_pad_create_stream_id(srcpad, GST_ELEMENT(self), stream_id_intermediate); > GstEvent *event = gst_event_new_stream_start(stream_id); > gst_event_set_group_id(event, group_id); > gst_pad_push_event(srcpad, event); > @@ -640,6 +642,57 @@ gst_libcamera_src_init(GstLibcameraSrc *self) > self->state = state; > } > > +static GstPad* > +gst_libcamera_src_request_new_pad(GstElement *element, GstPadTemplate *templ, > + const gchar *name, [[maybe_unused]] const GstCaps *caps) > +{ > + GstLibcameraSrc *self = GST_LIBCAMERA_SRC(element); > + g_autoptr(GstPad) pad = NULL; > + > + GST_DEBUG_OBJECT(self, "new request pad created"); > + > + pad = gst_pad_new_from_template(templ, name); > + g_object_ref_sink(pad); > + > + if (gst_element_add_pad(element, pad)) > + { > + GLibLocker lock(GST_OBJECT(self)); > + self->state->srcpads_.push_back(reinterpret_cast<GstPad*>(g_object_ref(pad))); > + } > + else > + { > + GST_ELEMENT_ERROR (element, STREAM, FAILED, > + ("Internal data stream error."), > + ("Could not add pad to element")); > + return NULL; > + } > + > + return reinterpret_cast<GstPad*>(g_steal_pointer(&pad)); > +} > + > +static void > +gst_libcamera_src_release_pad(GstElement *element, GstPad *pad) > +{ > + GstLibcameraSrc *self = GST_LIBCAMERA_SRC(element); > + > + GST_DEBUG_OBJECT (self, "Pad %" GST_PTR_FORMAT " being released", pad); > + > + { > + GLibLocker lock(GST_OBJECT(self)); > + std::vector<GstPad*> &pads = self->state->srcpads_; > + auto begin_iterator = pads.begin(); > + auto end_iterator = pads.end(); > + auto pad_iterator = std::find(begin_iterator, end_iterator, pad); > + > + if (pad_iterator != end_iterator) > + { > + g_object_unref(*pad_iterator); > + pads.erase(pad_iterator); > + } > + } > + gst_element_remove_pad(element, pad); > +} > + > static void > gst_libcamera_src_class_init(GstLibcameraSrcClass *klass) > { > @@ -650,6 +703,8 @@ gst_libcamera_src_class_init(GstLibcameraSrcClass *klass) > object_class->get_property = gst_libcamera_src_get_property; > object_class->finalize = gst_libcamera_src_finalize; > > + element_class->request_new_pad = gst_libcamera_src_request_new_pad; > + element_class->release_pad = gst_libcamera_src_release_pad; > element_class->change_state = gst_libcamera_src_change_state; > > gst_element_class_set_metadata(element_class,
On Fri, Jun 18, 2021 at 01:40:15PM -0400, Nicolas Dufresne wrote: > Le samedi 12 juin 2021 à 13:52 +0530, Vedant Paranjape a écrit : > > Using request pads needs virtual functions to create new request pads and > > release the allocated pads. Added way to generate unique stream_id in > > gst_libcamera_src_task_enter(). It failed to execute when more than one > > pad was added. > > > > This patch defines the functions gst_libcamera_src_request_new_pad() and > > gst_libcamera_src_release_pad() which handle, creating and releasing > > request pads. Also assigns these functions to their callback > > handlers in GstLibcameraSrcClass. > > > > gst_pad_create_stream_id() failed as this gstreamer element has more than 2 > > source pads, and a nullptr was passed to it's stream_id argument. > > This function fails to auto generate a stream id for scenario of more > > than one source pad present. > > > > Used a gint which increments everytime new stream_id is requested and > > group_id to generate an unique stream_id by appending them togther as > > %i%i. > > > > This doesn't enable request pad support in gstreamer element. This is > > one of the series of changes needed to make it work. > > I would suggest to rework the commit message, and focus on what feature this > adds and how to use it. > > > Signed-off-by: Vedant Paranjape <vedantparanjape160201@gmail.com> > > The code looks fine, I'll let you send the final one before adding by R-b. Note > that when importing this patch: > > .git/rebase-apply/patch:58: trailing whitespace. > return NULL; > .git/rebase-apply/patch:60: trailing whitespace. > > .git/rebase-apply/patch:68: trailing whitespace. > > .git/rebase-apply/patch:85: trailing whitespace. > } > > And also, running checkstyle.py shows some more issues: This can be automated by using the pre- or post-commit hooks: cp utils/hooks/post-commit .git/hooks/ or cp utils/hooks/pre-commit .git/hooks/ Note that checkstyle.py can only provide a best effort, it's fine to ignore some of its suggestions if there's a valid reason to do so. > ---------------------------------------------------------------------------------------------------------- > 0e4fbbad9ffaed512593848212facffed0f4620f gstreamer: Added virtual functions needed to support request pads > ---------------------------------------------------------------------------------------------------------- > --- src/gstreamer/gstlibcamerasrc.cpp > +++ src/gstreamer/gstlibcamerasrc.cpp > @@ -642,9 +641,9 @@ > self->state = state; > } > > -static GstPad* > +static GstPad * > gst_libcamera_src_request_new_pad(GstElement *element, GstPadTemplate *templ, > - const gchar *name, [[maybe_unused]] const GstCaps *caps) > + const gchar *name, [[maybe_unused]] const GstCaps *caps) > { > GstLibcameraSrc *self = GST_LIBCAMERA_SRC(element); > g_autoptr(GstPad) pad = NULL; > @@ -654,44 +653,40 @@ > pad = gst_pad_new_from_template(templ, name); > g_object_ref_sink(pad); > > - if (gst_element_add_pad(element, pad)) > + if (gst_element_add_pad(element, pad)) { > + GLibLocker lock(GST_OBJECT(self)); > + self->state->srcpads_.push_back(reinterpret_cast<GstPad *>(g_object_ref(pad))); > + } else { > + GST_ELEMENT_ERROR(element, STREAM, FAILED, > + ("Internal data stream error."), > + ("Could not add pad to element")); > + return NULL; > + } > + > + return reinterpret_cast<GstPad *>(g_steal_pointer(&pad)); > +} > + > +static void > +gst_libcamera_src_release_pad(GstElement *element, GstPad *pad) > +{ > + GstLibcameraSrc *self = GST_LIBCAMERA_SRC(element); > + > + GST_DEBUG_OBJECT(self, "Pad %" GST_PTR_FORMAT " being released", pad); > + > { > GLibLocker lock(GST_OBJECT(self)); > - self->state->srcpads_.push_back(reinterpret_cast<GstPad*>(g_object_ref(pad))); > - } > - else > - { > - GST_ELEMENT_ERROR (element, STREAM, FAILED, > - ("Internal data stream error."), > - ("Could not add pad to element")); > - return NULL; > - } > - > - return reinterpret_cast<GstPad*>(g_steal_pointer(&pad)); > -} > - > -static void > -gst_libcamera_src_release_pad(GstElement *element, GstPad *pad) > -{ > - GstLibcameraSrc *self = GST_LIBCAMERA_SRC(element); > - > - GST_DEBUG_OBJECT (self, "Pad %" GST_PTR_FORMAT " being released", pad); > - > - { > - GLibLocker lock(GST_OBJECT(self)); > - std::vector<GstPad*> &pads = self->state->srcpads_; > + std::vector<GstPad *> &pads = self->state->srcpads_; > auto begin_iterator = pads.begin(); > auto end_iterator = pads.end(); > auto pad_iterator = std::find(begin_iterator, end_iterator, pad); > > - if (pad_iterator != end_iterator) > - { > + if (pad_iterator != end_iterator) { > g_object_unref(*pad_iterator); > pads.erase(pad_iterator); > } > } > gst_element_remove_pad(element, pad); > -} > +} > > static void > gst_libcamera_src_class_init(GstLibcameraSrcClass *klass) > --- > 2 potential issues detected, please review > > > > --- > > In _request_new_pad and _release_pad, a GLibLock is taken on > > self->state. But since self->state is a C++ object, > > GST_OBJECT(self->state) fails and returns null, so GLibLocker throws a > > warning "invalid cast from '(null)' to 'GstObject'. This means that it > > actually fails to get lock the resource that needs to be protected, > > i.e., the srcpads_ vector. > > > > The following changes were tested using the following test code: https://pastebin.com/gNDF1cyG > > > > src/gstreamer/gstlibcamerasrc.cpp | 57 ++++++++++++++++++++++++++++++- > > 1 file changed, 56 insertions(+), 1 deletion(-) > > > > diff --git a/src/gstreamer/gstlibcamerasrc.cpp b/src/gstreamer/gstlibcamerasrc.cpp > > index ccc61590..1614971d 100644 > > --- a/src/gstreamer/gstlibcamerasrc.cpp > > +++ b/src/gstreamer/gstlibcamerasrc.cpp > > @@ -361,10 +361,12 @@ gst_libcamera_src_task_enter(GstTask *task, [[maybe_unused]] GThread *thread, > > GST_DEBUG_OBJECT(self, "Streaming thread has started"); > > > > guint group_id = gst_util_group_id_next(); > > + gint stream_id_num = 0; > > StreamRoles roles; > > for (GstPad *srcpad : state->srcpads_) { > > /* Create stream-id and push stream-start. */ > > - g_autofree gchar *stream_id = gst_pad_create_stream_id(srcpad, GST_ELEMENT(self), nullptr); > > + g_autofree gchar *stream_id_intermediate = g_strdup_printf("%i%i", group_id, stream_id_num++); > > + g_autofree gchar *stream_id = gst_pad_create_stream_id(srcpad, GST_ELEMENT(self), stream_id_intermediate); > > GstEvent *event = gst_event_new_stream_start(stream_id); > > gst_event_set_group_id(event, group_id); > > gst_pad_push_event(srcpad, event); > > @@ -640,6 +642,57 @@ gst_libcamera_src_init(GstLibcameraSrc *self) > > self->state = state; > > } > > > > +static GstPad* > > +gst_libcamera_src_request_new_pad(GstElement *element, GstPadTemplate *templ, > > + const gchar *name, [[maybe_unused]] const GstCaps *caps) > > +{ > > + GstLibcameraSrc *self = GST_LIBCAMERA_SRC(element); > > + g_autoptr(GstPad) pad = NULL; > > + > > + GST_DEBUG_OBJECT(self, "new request pad created"); > > + > > + pad = gst_pad_new_from_template(templ, name); > > + g_object_ref_sink(pad); > > + > > + if (gst_element_add_pad(element, pad)) > > + { > > + GLibLocker lock(GST_OBJECT(self)); > > + self->state->srcpads_.push_back(reinterpret_cast<GstPad*>(g_object_ref(pad))); > > + } > > + else > > + { > > + GST_ELEMENT_ERROR (element, STREAM, FAILED, > > + ("Internal data stream error."), > > + ("Could not add pad to element")); > > + return NULL; > > + } > > + > > + return reinterpret_cast<GstPad*>(g_steal_pointer(&pad)); > > +} > > + > > +static void > > +gst_libcamera_src_release_pad(GstElement *element, GstPad *pad) > > +{ > > + GstLibcameraSrc *self = GST_LIBCAMERA_SRC(element); > > + > > + GST_DEBUG_OBJECT (self, "Pad %" GST_PTR_FORMAT " being released", pad); > > + > > + { > > + GLibLocker lock(GST_OBJECT(self)); > > + std::vector<GstPad*> &pads = self->state->srcpads_; > > + auto begin_iterator = pads.begin(); > > + auto end_iterator = pads.end(); > > + auto pad_iterator = std::find(begin_iterator, end_iterator, pad); > > + > > + if (pad_iterator != end_iterator) > > + { > > + g_object_unref(*pad_iterator); > > + pads.erase(pad_iterator); > > + } > > + } > > + gst_element_remove_pad(element, pad); > > +} > > + > > static void > > gst_libcamera_src_class_init(GstLibcameraSrcClass *klass) > > { > > @@ -650,6 +703,8 @@ gst_libcamera_src_class_init(GstLibcameraSrcClass *klass) > > object_class->get_property = gst_libcamera_src_get_property; > > object_class->finalize = gst_libcamera_src_finalize; > > > > + element_class->request_new_pad = gst_libcamera_src_request_new_pad; > > + element_class->release_pad = gst_libcamera_src_release_pad; > > element_class->change_state = gst_libcamera_src_change_state; > > > > gst_element_class_set_metadata(element_class,
diff --git a/src/gstreamer/gstlibcamerasrc.cpp b/src/gstreamer/gstlibcamerasrc.cpp index ccc61590..1614971d 100644 --- a/src/gstreamer/gstlibcamerasrc.cpp +++ b/src/gstreamer/gstlibcamerasrc.cpp @@ -361,10 +361,12 @@ gst_libcamera_src_task_enter(GstTask *task, [[maybe_unused]] GThread *thread, GST_DEBUG_OBJECT(self, "Streaming thread has started"); guint group_id = gst_util_group_id_next(); + gint stream_id_num = 0; StreamRoles roles; for (GstPad *srcpad : state->srcpads_) { /* Create stream-id and push stream-start. */ - g_autofree gchar *stream_id = gst_pad_create_stream_id(srcpad, GST_ELEMENT(self), nullptr); + g_autofree gchar *stream_id_intermediate = g_strdup_printf("%i%i", group_id, stream_id_num++); + g_autofree gchar *stream_id = gst_pad_create_stream_id(srcpad, GST_ELEMENT(self), stream_id_intermediate); GstEvent *event = gst_event_new_stream_start(stream_id); gst_event_set_group_id(event, group_id); gst_pad_push_event(srcpad, event); @@ -640,6 +642,57 @@ gst_libcamera_src_init(GstLibcameraSrc *self) self->state = state; } +static GstPad* +gst_libcamera_src_request_new_pad(GstElement *element, GstPadTemplate *templ, + const gchar *name, [[maybe_unused]] const GstCaps *caps) +{ + GstLibcameraSrc *self = GST_LIBCAMERA_SRC(element); + g_autoptr(GstPad) pad = NULL; + + GST_DEBUG_OBJECT(self, "new request pad created"); + + pad = gst_pad_new_from_template(templ, name); + g_object_ref_sink(pad); + + if (gst_element_add_pad(element, pad)) + { + GLibLocker lock(GST_OBJECT(self)); + self->state->srcpads_.push_back(reinterpret_cast<GstPad*>(g_object_ref(pad))); + } + else + { + GST_ELEMENT_ERROR (element, STREAM, FAILED, + ("Internal data stream error."), + ("Could not add pad to element")); + return NULL; + } + + return reinterpret_cast<GstPad*>(g_steal_pointer(&pad)); +} + +static void +gst_libcamera_src_release_pad(GstElement *element, GstPad *pad) +{ + GstLibcameraSrc *self = GST_LIBCAMERA_SRC(element); + + GST_DEBUG_OBJECT (self, "Pad %" GST_PTR_FORMAT " being released", pad); + + { + GLibLocker lock(GST_OBJECT(self)); + std::vector<GstPad*> &pads = self->state->srcpads_; + auto begin_iterator = pads.begin(); + auto end_iterator = pads.end(); + auto pad_iterator = std::find(begin_iterator, end_iterator, pad); + + if (pad_iterator != end_iterator) + { + g_object_unref(*pad_iterator); + pads.erase(pad_iterator); + } + } + gst_element_remove_pad(element, pad); +} + static void gst_libcamera_src_class_init(GstLibcameraSrcClass *klass) { @@ -650,6 +703,8 @@ gst_libcamera_src_class_init(GstLibcameraSrcClass *klass) object_class->get_property = gst_libcamera_src_get_property; object_class->finalize = gst_libcamera_src_finalize; + element_class->request_new_pad = gst_libcamera_src_request_new_pad; + element_class->release_pad = gst_libcamera_src_release_pad; element_class->change_state = gst_libcamera_src_change_state; gst_element_class_set_metadata(element_class,
Using request pads needs virtual functions to create new request pads and release the allocated pads. Added way to generate unique stream_id in gst_libcamera_src_task_enter(). It failed to execute when more than one pad was added. This patch defines the functions gst_libcamera_src_request_new_pad() and gst_libcamera_src_release_pad() which handle, creating and releasing request pads. Also assigns these functions to their callback handlers in GstLibcameraSrcClass. gst_pad_create_stream_id() failed as this gstreamer element has more than 2 source pads, and a nullptr was passed to it's stream_id argument. This function fails to auto generate a stream id for scenario of more than one source pad present. Used a gint which increments everytime new stream_id is requested and group_id to generate an unique stream_id by appending them togther as %i%i. This doesn't enable request pad support in gstreamer element. This is one of the series of changes needed to make it work. Signed-off-by: Vedant Paranjape <vedantparanjape160201@gmail.com> --- In _request_new_pad and _release_pad, a GLibLock is taken on self->state. But since self->state is a C++ object, GST_OBJECT(self->state) fails and returns null, so GLibLocker throws a warning "invalid cast from '(null)' to 'GstObject'. This means that it actually fails to get lock the resource that needs to be protected, i.e., the srcpads_ vector. The following changes were tested using the following test code: https://pastebin.com/gNDF1cyG src/gstreamer/gstlibcamerasrc.cpp | 57 ++++++++++++++++++++++++++++++- 1 file changed, 56 insertions(+), 1 deletion(-)