GNOME Bugzilla – Bug 773463
core(debug): hard to distinguish related log at multi-instance env
Last modified: 2018-11-03 12:37:49 UTC
This is for debug env. When we do debug with log message, it is hard to recoginize which element/object is included in which pipeline in multi-instance environment. We've added family-id for grouping for each pipeline at downstream (I've attached the patch). To get better idea and make it public, I registered this bug. Please share your idea and suggest better solution. Thank you.
Created attachment 338389 [details] [review] add family-id for debugging in multi-instance environment
It could make sense to somehow register a weak-ref to the top-level pipeline inside GstElement and use that. Or maybe to always print the whole object path instead of just the name, like what gst-launch does? Adding new API like that seems a bit weird to me
Adding new API is not for upstream. To share what we want, I added the downstream patch. I hope someone who understand what we want, he/she can share better idea for public and we can use it at downstream too.
I agree with that you Sebastian and we already discussed that with Eunhae, I was thinking we could distribute the name of the toplevel parent to each children, or a ref to each children, but the latter might be a bit more complex for very little advantage (at first sight). I did not think about printing the path, but I am not sure I like the idea as we will get very long string for object name (but that could be activatable). What do you think would be best?
Created attachment 341506 [details] [review] object: Add a private structure and move control_bindings to it Moving control_bindings there so we still have one reservered field in the structure
Created attachment 341507 [details] [review] info: Give more control to use about how to print objects Currently using GST_DEBUG_OBJECT only the name of the object is printed, but in environments where many pipelines are running concurrently it quickly becomes hard to follow what element that refers to as several elements/pads can have the same name in those pipeline. This patch adds a new special 'GST_OBJECT_REPR' debugging category which will allow GStreamer to print the object topelevel bin name or a full object path depending on that category level. Basically setting `GST_DEBUG=GST_OBJECT_REPR:2` when debugging an element called 'e1' inside a bin called 'b1' inside a pipeline called 'p1' will be printed as '/p1/b1/e1'. Setting that category to level 1 will print '/p1/../e1'
(In reply to Thibault Saunier from comment #6) > Created attachment 341507 [details] [review] [review] > info: Give more control to use about how to print objects This patch is not totally finalized (basically more testing is needed) but I would like to know if you agree with that approach?
Created attachment 341508 [details] [review] info: Give more control to use about how to print objects Minor cleanup
Created attachment 341509 [details] [review] info: Give more control to use about how to print objects Re factor in gst_object_get_toplevel, it is not needed anymore
Instead of this "magic" debug category I would suggest a parameter to the GST_DEBUG_OPTIONS env var. I only quickly glanced at this patchset, but I'm not really sure whether I like this approach. Not that I have a thought-through alternative proposal yet though :) I understand the feature request though and think it's a valid request that'd be good to solve in some way. I'm rambling here now, but something I've been thinking of is that it might be easier to implement and more flexible in general if we simply relied on tools (log viewers) to do some postprocessing to enable this kind of thing. We still need to make that info available for tools of course. We could have global sequence numbers for all objects and then simply refer to the objects by number, and whenever their state changes we simply log that in reference to their number. Also means we don't have to take that lock constantly everywhere (which your patch also seems to do for the case where we want the old print behaviour). We have a similar/related problem in bug #761916 btw (similar in terms of what an implementation needs to cater for).
(In reply to Tim-Philipp Müller from comment #10) > Instead of this "magic" debug category I would suggest a parameter to the > GST_DEBUG_OPTIONS env var. Definitely a good suggestion, I did not know about the var. > I only quickly glanced at this patchset, but I'm not really sure whether I > like this approach. Not that I have a thought-through alternative proposal > yet though :) > > I understand the feature request though and think it's a valid request > that'd be good to solve in some way. > > I'm rambling here now, but something I've been thinking of is that it might > be easier to implement and more flexible in general if we simply relied on > tools (log viewers) to do some postprocessing to enable this kind of thing. > We still need to make that info available for tools of course. We could have > global sequence numbers for all objects and then simply refer to the objects > by number, and whenever their state changes we simply log that in reference > to their number. Hrm, I am not convinced we want to 'force' people to use gst-debug-viewer to view their logs, though a post processing script which will just update file or something could make sense but I am really not convinced :) > Also means we don't have to take that lock constantly > everywhere (which your patch also seems to do for the case where we want the > old print behaviour). No lock is needed if the feature is not activated (updating the patch to make sure of this does not happen). > We have a similar/related problem in bug #761916 btw (similar in terms of > what an implementation needs to cater for). My approach can solve this other issue quite nicely fmpov (would need an extra flag on the object to tell that in all case we should keep track of the parent name in the representation of the object).
Created attachment 341558 [details] [review] info: Give more control to use about how to print objects Currently using GST_DEBUG_OBJECT only the name of the object is printed, but in environments where many pipelines are running concurrently it quickly becomes hard to follow what element that refers to as several elements/pads can have the same name in those pipeline. This patch adds 2 options to the 'GST_DEBUG_OPTIONS' env variable "objects-path" and "objects-toplevel" which makes the debug system print the full object path in the hierarchy or only its toplevel bin. --- I am now making sure no lock is taken in the normal case.
Review of attachment 341558 [details] [review]: ::: gst/gstobject.c @@ +166,3 @@ + if (object_repr_level <= 0) { + if (self->name) + gchar *repr = NULL; either use self->name in g_strdup_printf() or const gchar* name = GST_OBJECT_NAME (self); if (name) return g_strdup_printf ("<%s>", name); ... @@ +174,3 @@ + if (self->priv->path) + repr = g_strdup_printf ("<%s>", priv->path); + if (self->name) priv->toplevel also next line @@ +181,3 @@ + else + repr = g_strdup_printf ("<%s@%p>", G_OBJECT_TYPE_NAME (self), self); + } please either add {} or leave them out @@ +208,3 @@ + g_mutex_lock (&priv->repr_lock); + if (!toplevel) + does this need to be in the lock'ed section
Created attachment 341696 [details] [review] info: Give more control to use about how to print objects Currently using GST_DEBUG_OBJECT only the name of the object is printed, but in environments where many pipelines are running concurrently it quickly becomes hard to follow what element that refers to as several elements/pads can have the same name in those pipeline. This patch adds 2 options to the 'GST_DEBUG_OPTIONS' env variable "objects-path" and "objects-toplevel" which makes the debug system print the full object path in the hierarchy or only its toplevel bin. Note that the tests are only built with meson has they require access to internal function which is only possible thanks to the meson extract_objects feature.
This new implementation is fully lockless and (somehow) simpler than the previous one (though I would have hope to make it even simpler :)).
Comment on attachment 341696 [details] [review] info: Give more control to use about how to print objects Haven't looked at the meat of the patch yet, just some quick drive-by comments: >--- a/gst/gst_private.h >+++ b/gst/gst_private.h > >+/* GstObject debugging representation level */ >+gint object_repr_level; Don't think this will work properly everywhere. I think this should be extern gint object_repr_level; in the header file and then gint object_repr_level; in gstinfo.c or wherever. >--- a/gst/gstbin.c >+++ b/gst/gstbin.c >+void >+gst_bin_update_children_repr (GstObject * object, GstObject * parent) >+{ >+#ifndef GST_DISABLE_GST_DEBUG >+ ... >+#endif >+} I think these #ifndef #endif should be outside of the entire function block and there should be a #define for the function in the header that expands for nothing if the logging system is disabled (and no prototype of course). The reason to do it that way is that if you have code like below, the run-time object run-time checks will still be executed even though the function is empty. We don't want that in that case, we want it all to be a no-op I think. >+ if (result) >+ gst_bin_update_children_repr (GST_OBJECT (element), GST_OBJECT (bin)); We know here that the objects are okay, right? So maybe GST_OBJECT_CAST to avoid the run-time check. >+ gst_object_ref (element); > GST_TRACER_BIN_REMOVE_PRE (bin, element); > result = bclass->remove_element (bin, element); > GST_TRACER_BIN_REMOVE_POST (bin, result); > >+ gst_bin_update_children_repr (GST_OBJECT_CAST (element), >+ result ? NULL : GST_OBJECT_CAST (bin)); >+ gst_object_unref (element); Why do we not just set it to NULL if result was TRUE? Wouldn't that keep it as before if not? I'm not a big fan of the "Repr" name, but not so important for now.
Created attachment 341740 [details] [review] (In reply to Tim-Philipp Müller from comment #16) >Haven't looked at the meat of the patch yet, just some quick drive-by comments: >>--- a/gst/gst_private.h >>+++ b/gst/gst_private.h >> >>+/* GstObject debugging representation level */ >>+gint object_repr_level; > >Don't think this will work properly everywhere. I think this should be > extern gint object_repr_level; > in the header file and then > gint object_repr_level; > > in gstinfo.c or wherever. -> Indeed, DONE. > >--- a/gst/gstbin.c > >+++ b/gst/gstbin.c > > >+void > >+gst_bin_update_children_repr (GstObject * object, GstObject * parent) > >+{ > >+#ifndef GST_DISABLE_GST_DEBUG > >+ ... > >+#endif > >+} > > I think these #ifndef #endif should be outside of the entire function block > and there should be a #define for the function in the header that expands > for nothing if the logging system is disabled (and no prototype of course). > > The reason to do it that way is that if you have code like below, the > run-time object run-time checks will still be executed even though the > function is empty. We don't want that in that case, we want it all to be a > no-op I think. Thought about it and checked, I think modern compilers should be able to elide function with empty core, but your solution is safer indeed. -> DONE anyway. > >+ if (result) > >+ gst_bin_update_children_repr (GST_OBJECT (element), GST_OBJECT (bin)); > > We know here that the objects are okay, right? So maybe GST_OBJECT_CAST to > avoid the run-time check. > > > >+ gst_object_ref (element); > > GST_TRACER_BIN_REMOVE_PRE (bin, element); > > result = bclass->remove_element (bin, element); > > GST_TRACER_BIN_REMOVE_POST (bin, result); > > > >+ gst_bin_update_children_repr (GST_OBJECT_CAST (element), > >+ result ? NULL : GST_OBJECT_CAST (bin)); > >+ gst_object_unref (element); > > Why do we not just set it to NULL if result was TRUE? Wouldn't that keep it > as before if not? -> DONE > I'm not a big fan of the "Repr" name, but not so important for now. I do not really care, I just reused python naming here.
> Thought about it and checked, I think modern compilers should be able to > elide function with empty core, but your solution is safer indeed. > > -> DONE anyway. Thanks, but I don't think compilers will be able to discern that the functions called to do the type checking (e.g. g_type_check_instance_cast) could be omitted in this case, so they might skip the empty function call but the type checks would still run.
Created attachment 341881 [details] [review] info: Give more control to use about how to print objects - Remove now useless priv->parent_repr
Anyone wishing to review that? :)
Ping again :)
Anyone willing to review that?
Created attachment 351634 [details] [review] info: Give more control to users about how to print objects Currently using GST_DEBUG_OBJECT only the name of the object is printed, but in environments where many pipelines are running concurrently it quickly becomes hard to follow what a name refers to as several elements/pads can have the same name in those pipeline. Implementation: --------------- This adds a GstObjectRepr private mini object (so it is ref counted) to GstObject which contains needed information about how to quickly build path to an object in its hierarchy. This structure is recreated recursively by GstBin whenever a hierarchical change is made within it (child added/removed/ etc...). To avoid needing locking, we create a new GstObjectRepr object as previous one can be used somewhere else at the same time (meaning it owns another ref on it). Usage: ------ This patch adds 2 options to the 'GST_DEBUG_OPTIONS' env variable "objects-path" and "objects-toplevel" which makes the debug system print the full object path in the hierarchy or only its toplevel bin. --- Note that the tests are only built with meson has they require access to internal function which is only possible thanks to the meson extract_objects feature.
OK, anyone against getting that in? :)
Comment on attachment 351634 [details] [review] info: Give more control to users about how to print objects Again, only a quick look. I have some smaller nitpicks, but mostly questions about thread-safety in two bits of code: >+gchar * >+_priv_gst_object_get_repr (GstObject * self) >+{ >+ ... >+ gst_object_replace (&parent, self->parent); Why use gst_object_replace() here? Is this thread-safe? self->parent can only be set once, but it's not clear to me that the underlying get+ref here is really thread-safe when it's synchronised with the OBJECT_LOCK in set_parent(). But of course we can't take the lock here, can we? >+void >+_priv_gst_object_update_repr (GstObject * self, GstObject * parent) >+{ >+ ... >+ last_repr = gst_object_repr_ref (self->priv->repr); >+ >+ /* If we deal with an always child object, we just need to make >+ * sure its representation name is always in sync */ >+ g_free (last_repr->name); >+ last_repr->unowned_name = NULL; >+ last_repr->name = g_strdup (GST_STR_NULL (self->name)); Is this thread-safe? Might not some other thread also have an ObjRepr and be accessing these strings in the other thread while we're changing them here? Or do we not have to worry about that? Then this in _update_repr(): >+ gst_mini_object_replace ((GstMiniObject **) & priv->repr, >+ (GstMiniObject *) repr); combined with this in _get_repr(): + repr = gst_object_repr_ref (priv->repr); Is this thread-safe? It's not an atomic read, and even if it was, couldn't some other thread replace+unref+free the repr between our atomic read and the ref?
-- GitLab Migration Automatic Message -- This bug has been migrated to freedesktop.org's GitLab instance and has been closed from further activity. You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.freedesktop.org/gstreamer/gstreamer/issues/201.